From bb72f219cf3924c930bde8db3454f64e67e973ee Mon Sep 17 00:00:00 2001 From: mstmdev Date: Thu, 3 Aug 2023 16:27:27 +0800 Subject: [PATCH] Fix some special characters may break the file path (#221) --- fs/fs.go | 19 ++++++++++++ fs/fs_test.go | 29 +++++++++++++++++++ .../testdata/test/test-gofs-remote-disk.yaml | 12 ++++++++ .../testdata/test/test-gofs-remote-push.yaml | 12 ++++++++ monitor/remote_client_monitor.go | 6 ++-- server/handler/file_api_handler.go | 2 ++ sync/remote_client_sync.go | 2 +- 7 files changed, 77 insertions(+), 5 deletions(-) diff --git a/fs/fs.go b/fs/fs.go index 7a3d8734..1d683688 100644 --- a/fs/fs.go +++ b/fs/fs.go @@ -93,3 +93,22 @@ func IsSub(parent, child string) (bool, error) { relPath = filepath.ToSlash(relPath) return relPath != ".." && !strings.HasPrefix(relPath, "../"), nil } + +// SafePath encode some special characters for the path like "?", "#" etc. +func SafePath(path string) string { + if len(path) == 0 { + return path + } + filterRule := []struct { + old string + new string + }{ + {"%", "%25"}, // stay in the first position + {"?", "%3F"}, + {"#", "%23"}, + } + for _, r := range filterRule { + path = strings.ReplaceAll(path, r.old, r.new) + } + return path +} diff --git a/fs/fs_test.go b/fs/fs_test.go index 9be83bd9..77c629d0 100644 --- a/fs/fs_test.go +++ b/fs/fs_test.go @@ -3,6 +3,7 @@ package fs import ( "errors" "io" + "net/url" "os" "path/filepath" "testing" @@ -350,6 +351,34 @@ func TestIsSub_ReturnError_Windows(t *testing.T) { } } +func TestSafePath(t *testing.T) { + testCases := []struct { + path string + }{ + {""}, + {" "}, + {"/hello/world"}, + {"/1 1"}, + {"/#1/#2"}, + {"/?1/?2"}, + {"/%25/%3F/%23"}, + } + + for _, tc := range testCases { + t.Run(tc.path, func(t *testing.T) { + safePath := SafePath(tc.path) + u, err := url.Parse(safePath) + if err != nil { + t.Errorf("parse url error: %v path=%s safe path=%s", err, tc.path, safePath) + return + } + if u.Path != tc.path { + t.Errorf("test SafePath error, expect to get %s, but actual get %s", tc.path, u.Path) + } + }) + } +} + func isNotExistAlwaysFalseMock(err error) bool { return false } diff --git a/integration/testdata/test/test-gofs-remote-disk.yaml b/integration/testdata/test/test-gofs-remote-disk.yaml index 6a67aabf..8e09c174 100644 --- a/integration/testdata/test/test-gofs-remote-disk.yaml +++ b/integration/testdata/test/test-gofs-remote-disk.yaml @@ -9,6 +9,14 @@ init: - mkdir: source: ./rc/dest actions: + - mkdir: + source: ./rs/source/#1/%25/%3F/%23 + - touch: + source: ./rs/source/#1/%25/%3F/%23/#%25.txt + - echo: + source: ./rs/source/#1/%25/%3F/%23/#%25.txt + input: Hello World + no-newline: true - cp: source: ./integration_test.go dest: ./rs/source/integration_test.go.bak1 @@ -35,6 +43,10 @@ actions: source: ./rs/source/empty2 no-newline: true - sleep: 10s + - hash: + algorithm: md5 + source: ./rc/dest/#1/%25/%3F/%23/#%25.txt + expect: b10a8db164e0754105b7a99be72e3fe5 - is-equal: source: ./integration_test.go dest: ./rc/dest/integration_test.go.bak1 diff --git a/integration/testdata/test/test-gofs-remote-push.yaml b/integration/testdata/test/test-gofs-remote-push.yaml index 84749423..489430cf 100644 --- a/integration/testdata/test/test-gofs-remote-push.yaml +++ b/integration/testdata/test/test-gofs-remote-push.yaml @@ -9,6 +9,14 @@ init: - mkdir: source: ./rc-push/dest actions: + - mkdir: + source: ./rc-push/source/#1/%25/%3F/%23 + - touch: + source: ./rc-push/source/#1/%25/%3F/%23/#%25.txt + - echo: + source: ./rc-push/source/#1/%25/%3F/%23/#%25.txt + input: Hello World + no-newline: true - cp: source: ./integration_test.go dest: ./rc-push/source/integration_test.go.bak1 @@ -35,6 +43,10 @@ actions: source: ./rc-push/source/empty2 no-newline: true - sleep: 10s + - hash: + algorithm: md5 + source: ./rs-push/source/#1/%25/%3F/%23/#%25.txt + expect: b10a8db164e0754105b7a99be72e3fe5 - is-equal: source: ./rc-push/source/integration_test.go.bak1 dest: ./rs-push/source/integration_test.go.bak1 diff --git a/monitor/remote_client_monitor.go b/monitor/remote_client_monitor.go index 7a762b5f..3bacf941 100644 --- a/monitor/remote_client_monitor.go +++ b/monitor/remote_client_monitor.go @@ -5,7 +5,6 @@ import ( "fmt" "net/url" "os" - "strings" "time" "github.com/no-src/gofs/action" @@ -14,6 +13,7 @@ import ( "github.com/no-src/gofs/auth" "github.com/no-src/gofs/contract" "github.com/no-src/gofs/eventlog" + "github.com/no-src/gofs/fs" "github.com/no-src/gofs/ignore" "github.com/no-src/gofs/internal/cbool" "github.com/no-src/gofs/internal/clist" @@ -204,9 +204,7 @@ func (m *remoteClientMonitor) execSync(msg *monitor.MonitorMessage) (err error) if len(fi.HashValues) > 0 { values.Add(contract.FsHashValues, stringutil.String(fi.HashValues)) } - - // replace question marks with "%3F" to avoid parse the path is breaking when it contains some question marks - path := msg.BaseUrl + strings.ReplaceAll(fi.Path, "?", "%3F") + fmt.Sprintf("?%s", values.Encode()) + path := msg.BaseUrl + fs.SafePath(fi.Path) + fmt.Sprintf("?%s", values.Encode()) switch action.Action(msg.Action) { case action.CreateAction: diff --git a/server/handler/file_api_handler.go b/server/handler/file_api_handler.go index f1a7ea35..37ef3c98 100644 --- a/server/handler/file_api_handler.go +++ b/server/handler/file_api_handler.go @@ -74,6 +74,8 @@ func (h *fileApiHandler) Handle(c *gin.Context) { } return } + defer f.Close() + stat, err := f.Stat() if err != nil { h.logger.Error(err, "file server get file stat error") diff --git a/sync/remote_client_sync.go b/sync/remote_client_sync.go index 81d83ef6..1d95d056 100644 --- a/sync/remote_client_sync.go +++ b/sync/remote_client_sync.go @@ -385,7 +385,7 @@ func (rs *remoteClientSync) syncFiles(files []contract.FileInfo, serverAddr, pat values.Add(contract.FsCtime, stringutil.String(file.CTime)) values.Add(contract.FsAtime, stringutil.String(file.ATime)) values.Add(contract.FsMtime, stringutil.String(file.MTime)) - syncPath := fmt.Sprintf("%s/%s?%s", serverAddr, currentPath, values.Encode()) + syncPath := fmt.Sprintf("%s/%s?%s", serverAddr, fs.SafePath(currentPath), values.Encode()) // create directory or file log.ErrorIf(rs.Create(syncPath), "sync create directory or file error => [syncPath=%s]", syncPath)