pkg/tool/path: use IsMaliciousPath to replace SanitizePath (#5558)

2 files changed, 17 insertions, 17 deletions

diff --git a/pkg/tool/path.go b/pkg/tool/path.go
index e8f7bcbe..e95bba8b 100644
--- a/pkg/tool/path.go
+++ b/pkg/tool/path.go
@@ -5,6 +5,7 @@
 package tool
 
 import (
+	"path/filepath"
 	"strings"
 )
 
@@ -15,10 +16,8 @@ func IsSameSiteURLPath(url string) bool {
 	return len(url) >= 2 && url[0] == '/' && url[1] != '/' && url[1] != '\\'
 }
 
-// SanitizePath sanitizes user-defined file paths to prevent remote code execution.
-func SanitizePath(path string) string {
-	path = strings.TrimLeft(path, "/")
-	path = strings.Replace(path, "../", "", -1)
-	path = strings.Replace(path, "..\\", "", -1)
-	return path
+// IsMaliciousPath returns true if given path is an absolute path or contains malicious content
+// which has potential to traverse upper level directories.
+func IsMaliciousPath(path string) bool {
+	return filepath.IsAbs(path) || strings.Contains(path, "..")
 }
diff --git a/pkg/tool/path_test.go b/pkg/tool/path_test.go
index d9b9fb21..44ee975f 100644
--- a/pkg/tool/path_test.go
+++ b/pkg/tool/path_test.go
@@ -31,22 +31,23 @@ func Test_IsSameSiteURLPath(t *testing.T) {
 	})
 }
 
-func Test_SanitizePath(t *testing.T) {
-	Convey("Sanitize malicious user-defined path", t, func() {
+func Test_IsMaliciousPath(t *testing.T) {
+	Convey("Detects malicious path", t, func() {
 		testCases := []struct {
 			path   string
-			expect string
+			expect bool
 		}{
-			{"../../../../../../../../../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", "data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8"},
-			{"data/gogs/../../../../../../../../../data/sessions/a/9/a9f0ab6c3ef63dd8", "data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8"},
-			{"..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", "data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8"},
-			{"data\\gogs\\..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", "data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8"},
-
-			{"data/sessions/a/9/a9f0ab6c3ef63dd8", "data/sessions/a/9/a9f0ab6c3ef63dd8"},
-			{"data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", "data\\sessions\\a\\9\\a9f0ab6c3ef63dd8"},
+			{"../../../../../../../../../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", true},
+			{"..\\/..\\/../data/gogs/data/sessions/a/9/a9f0ab6c3ef63dd8", true},
+			{"data/gogs/../../../../../../../../../data/sessions/a/9/a9f0ab6c3ef63dd8", true},
+			{"..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\gogs\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", true},
+			{"data\\gogs\\..\\..\\..\\..\\..\\..\\..\\..\\..\\data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", true},
+
+			{"data/sessions/a/9/a9f0ab6c3ef63dd8", false},
+			{"data\\sessions\\a\\9\\a9f0ab6c3ef63dd8", false},
 		}
 		for _, tc := range testCases {
-			So(SanitizePath(tc.path), ShouldEqual, tc.expect)
+			So(IsMaliciousPath(tc.path), ShouldEqual, tc.expect)
 		}
 	})
 }