From 20a47fd35ee49fc4c2ffadfad01b0c7128a12c61 Mon Sep 17 00:00:00 2001 From: yangxin Date: Fri, 24 Apr 2026 18:02:28 +0800 Subject: [PATCH 1/4] Improve export download file handling --- internal/util/download.go | 79 ++++++++++++++++++++++++- internal/util/download_test.go | 103 +++++++++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 2 deletions(-) create mode 100644 internal/util/download_test.go diff --git a/internal/util/download.go b/internal/util/download.go index a9b50635..407100d3 100644 --- a/internal/util/download.go +++ b/internal/util/download.go @@ -21,6 +21,9 @@ import ( "net/http" "os" "path/filepath" + "runtime" + "strings" + "unicode" "github.com/go-resty/resty/v2" ) @@ -60,9 +63,14 @@ func GetResponse(url string, debug bool) (*http.Response, error) { // CreateFile creates a file if it does not exist func CreateFile(path, fileName string) (*os.File, error) { - filePath := filepath.Join(path, fileName) - if _, err := os.Stat(filePath); err == nil { + filePath, err := safeDownloadPath(path, fileName) + if err != nil { + return nil, err + } + if _, err := os.Lstat(filePath); err == nil { return nil, fmt.Errorf("file already exists") + } else if !os.IsNotExist(err) { + return nil, err } file, err := os.Create(filePath) if err != nil { @@ -71,6 +79,73 @@ func CreateFile(path, fileName string) (*os.File, error) { return file, nil } +func safeDownloadPath(basePath, fileName string) (string, error) { + if fileName == "" { + return "", downloadDestinationError(fileName, "cannot be empty") + } + if fileName == "." || fileName == ".." { + return "", downloadDestinationError(fileName, "must refer to a file") + } + if filepath.IsAbs(fileName) || isWindowsAbs(fileName) { + return "", downloadDestinationError(fileName, "must be relative to the output path") + } + if containsControlCharacter(fileName) { + return "", downloadDestinationError(fileName, "contains unsupported characters") + } + + baseAbs, err := filepath.Abs(basePath) + if err != nil { + return "", err + } + baseClean := filepath.Clean(baseAbs) + baseReal, err := filepath.EvalSymlinks(baseClean) + if err != nil { + return "", err + } + + candidate := filepath.Clean(filepath.Join(baseClean, fileName)) + rel, err := filepath.Rel(baseClean, candidate) + if err != nil { + return "", err + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) || filepath.IsAbs(rel) { + return "", downloadDestinationError(fileName, "is outside the output path") + } + + parentReal, err := filepath.EvalSymlinks(filepath.Dir(candidate)) + if err != nil { + return "", err + } + parentRel, err := filepath.Rel(baseReal, parentReal) + if err != nil { + return "", err + } + if parentRel == ".." || strings.HasPrefix(parentRel, ".."+string(os.PathSeparator)) || filepath.IsAbs(parentRel) { + return "", downloadDestinationError(fileName, "is outside the output path") + } + + return candidate, nil +} + +func downloadDestinationError(fileName, reason string) error { + return fmt.Errorf("download destination %q %s", fileName, reason) +} + +func containsControlCharacter(s string) bool { + return strings.ContainsRune(s, 0) || strings.IndexFunc(s, unicode.IsControl) >= 0 +} + +func isWindowsAbs(path string) bool { + if runtime.GOOS == "windows" { + return false + } + if len(path) >= 3 && path[1] == ':' && (path[2] == '\\' || path[2] == '/') { + c := path[0] + return ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') + } + return strings.HasPrefix(path, `\\`) +} + // CreateFolder creates a folder if it does not exist func CreateFolder(path string) error { if path == "" { diff --git a/internal/util/download_test.go b/internal/util/download_test.go new file mode 100644 index 00000000..65a56a56 --- /dev/null +++ b/internal/util/download_test.go @@ -0,0 +1,103 @@ +// Copyright 2026 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package util + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +func TestCreateFileAllowsPathInsideBase(t *testing.T) { + base := t.TempDir() + + file, err := CreateFile(base, "export.sql.gz") + if err != nil { + t.Fatalf("CreateFile() error = %v", err) + } + file.Close() + + if _, err := os.Stat(filepath.Join(base, "export.sql.gz")); err != nil { + t.Fatalf("expected file inside base: %v", err) + } +} + +func TestCreateFileAllowsExistingSubdirectoryInsideBase(t *testing.T) { + base := t.TempDir() + if err := os.Mkdir(filepath.Join(base, "schema"), 0755); err != nil { + t.Fatalf("Mkdir() error = %v", err) + } + + file, err := CreateFile(base, filepath.Join("schema", "export.sql.gz")) + if err != nil { + t.Fatalf("CreateFile() error = %v", err) + } + file.Close() + + if _, err := os.Stat(filepath.Join(base, "schema", "export.sql.gz")); err != nil { + t.Fatalf("expected nested file inside base: %v", err) + } +} + +func TestCreateFileRejectsPathOutsideBase(t *testing.T) { + base := t.TempDir() + outside := filepath.Join(base, "..", "outside.sql.gz") + + tests := []string{ + "", + ".", + "..", + filepath.Join("..", "outside.sql.gz"), + filepath.Join("nested", "..", "..", "outside.sql.gz"), + outside, + "C:\\Temp\\outside.sql.gz", + "\\\\server\\share\\outside.sql.gz", + "bad\x00name.sql.gz", + "bad\nname.sql.gz", + } + + for _, tt := range tests { + t.Run(strings.ReplaceAll(tt, string(os.PathSeparator), "_"), func(t *testing.T) { + file, err := CreateFile(base, tt) + if err == nil { + file.Close() + t.Fatalf("CreateFile() succeeded for %q", tt) + } + }) + } +} + +func TestCreateFileRejectsSymlinkedParentOutsideBase(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink behavior requires additional privileges on Windows") + } + + base := t.TempDir() + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(base, "link")); err != nil { + t.Fatalf("Symlink() error = %v", err) + } + + file, err := CreateFile(base, filepath.Join("link", "export.sql.gz")) + if err == nil { + file.Close() + t.Fatalf("CreateFile() succeeded through symlinked parent") + } + if _, statErr := os.Stat(filepath.Join(outside, "export.sql.gz")); !os.IsNotExist(statErr) { + t.Fatalf("expected no file outside base, stat error = %v", statErr) + } +} From 1354b85a5d41311a4172f0c55f153cb2437df162 Mon Sep 17 00:00:00 2001 From: yangxin Date: Fri, 24 Apr 2026 18:12:05 +0800 Subject: [PATCH 2/4] Update export download file tests --- internal/util/download_test.go | 102 ++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 41 deletions(-) diff --git a/internal/util/download_test.go b/internal/util/download_test.go index 65a56a56..c7148b45 100644 --- a/internal/util/download_test.go +++ b/internal/util/download_test.go @@ -18,64 +18,84 @@ import ( "os" "path/filepath" "runtime" - "strings" "testing" ) -func TestCreateFileAllowsPathInsideBase(t *testing.T) { - base := t.TempDir() - - file, err := CreateFile(base, "export.sql.gz") - if err != nil { - t.Fatalf("CreateFile() error = %v", err) +func TestCreateFileAllowsRelativeDestinations(t *testing.T) { + tests := []struct { + name string + fileName string + }{ + { + name: "normal file name", + fileName: "a.sql.gz", + }, + { + name: "subdirectory", + fileName: filepath.Join("folder", "a.sql.gz"), + }, + { + name: "nested subdirectory", + fileName: filepath.Join("a", "b", "c.sql.gz"), + }, } - file.Close() - if _, err := os.Stat(filepath.Join(base, "export.sql.gz")); err != nil { - t.Fatalf("expected file inside base: %v", err) - } -} - -func TestCreateFileAllowsExistingSubdirectoryInsideBase(t *testing.T) { - base := t.TempDir() - if err := os.Mkdir(filepath.Join(base, "schema"), 0755); err != nil { - t.Fatalf("Mkdir() error = %v", err) - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base := t.TempDir() + parent := filepath.Dir(filepath.Join(base, tt.fileName)) + if err := os.MkdirAll(parent, 0755); err != nil { + t.Fatalf("MkdirAll() error = %v", err) + } - file, err := CreateFile(base, filepath.Join("schema", "export.sql.gz")) - if err != nil { - t.Fatalf("CreateFile() error = %v", err) - } - file.Close() + file, err := CreateFile(base, tt.fileName) + if err != nil { + t.Fatalf("CreateFile() error = %v", err) + } + file.Close() - if _, err := os.Stat(filepath.Join(base, "schema", "export.sql.gz")); err != nil { - t.Fatalf("expected nested file inside base: %v", err) + if _, err := os.Stat(filepath.Join(base, tt.fileName)); err != nil { + t.Fatalf("expected file inside base: %v", err) + } + }) } } -func TestCreateFileRejectsPathOutsideBase(t *testing.T) { +func TestCreateFileRejectsUnsupportedDestinations(t *testing.T) { base := t.TempDir() - outside := filepath.Join(base, "..", "outside.sql.gz") - tests := []string{ - "", - ".", - "..", - filepath.Join("..", "outside.sql.gz"), - filepath.Join("nested", "..", "..", "outside.sql.gz"), - outside, - "C:\\Temp\\outside.sql.gz", - "\\\\server\\share\\outside.sql.gz", - "bad\x00name.sql.gz", - "bad\nname.sql.gz", + tests := []struct { + name string + fileName string + }{ + { + name: "path outside output path", + fileName: filepath.Join("..", "..", "tmp", "pwned"), + }, + { + name: "absolute path", + fileName: filepath.Join(string(os.PathSeparator), "tmp", "pwned"), + }, + { + name: "empty file name", + fileName: "", + }, + { + name: "current directory", + fileName: ".", + }, + { + name: "parent directory", + fileName: "..", + }, } for _, tt := range tests { - t.Run(strings.ReplaceAll(tt, string(os.PathSeparator), "_"), func(t *testing.T) { - file, err := CreateFile(base, tt) + t.Run(tt.name, func(t *testing.T) { + file, err := CreateFile(base, tt.fileName) if err == nil { file.Close() - t.Fatalf("CreateFile() succeeded for %q", tt) + t.Fatalf("CreateFile() succeeded for %q", tt.fileName) } }) } From f834eab23dfee2170de1d512affeef094083148b Mon Sep 17 00:00:00 2001 From: yangxin Date: Fri, 24 Apr 2026 18:29:56 +0800 Subject: [PATCH 3/4] Use rooted export download file creation --- internal/util/download.go | 54 ++++++++++++---------------------- internal/util/download_test.go | 48 ++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 36 deletions(-) diff --git a/internal/util/download.go b/internal/util/download.go index 407100d3..5e799321 100644 --- a/internal/util/download.go +++ b/internal/util/download.go @@ -63,23 +63,31 @@ func GetResponse(url string, debug bool) (*http.Response, error) { // CreateFile creates a file if it does not exist func CreateFile(path, fileName string) (*os.File, error) { - filePath, err := safeDownloadPath(path, fileName) + filePath, err := safeDownloadPath(fileName) if err != nil { return nil, err } - if _, err := os.Lstat(filePath); err == nil { - return nil, fmt.Errorf("file already exists") - } else if !os.IsNotExist(err) { - return nil, err + + if path == "" { + path = "." } - file, err := os.Create(filePath) + root, err := os.OpenRoot(path) if err != nil { return nil, err } + defer root.Close() + + file, err := root.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0644) + if err != nil { + if os.IsExist(err) { + return nil, fmt.Errorf("file already exists") + } + return nil, fmt.Errorf("download destination %q cannot be created: %w", fileName, err) + } return file, nil } -func safeDownloadPath(basePath, fileName string) (string, error) { +func safeDownloadPath(fileName string) (string, error) { if fileName == "" { return "", downloadDestinationError(fileName, "cannot be empty") } @@ -93,38 +101,12 @@ func safeDownloadPath(basePath, fileName string) (string, error) { return "", downloadDestinationError(fileName, "contains unsupported characters") } - baseAbs, err := filepath.Abs(basePath) - if err != nil { - return "", err - } - baseClean := filepath.Clean(baseAbs) - baseReal, err := filepath.EvalSymlinks(baseClean) - if err != nil { - return "", err - } - - candidate := filepath.Clean(filepath.Join(baseClean, fileName)) - rel, err := filepath.Rel(baseClean, candidate) - if err != nil { - return "", err - } - if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) || filepath.IsAbs(rel) { - return "", downloadDestinationError(fileName, "is outside the output path") - } - - parentReal, err := filepath.EvalSymlinks(filepath.Dir(candidate)) - if err != nil { - return "", err - } - parentRel, err := filepath.Rel(baseReal, parentReal) - if err != nil { - return "", err - } - if parentRel == ".." || strings.HasPrefix(parentRel, ".."+string(os.PathSeparator)) || filepath.IsAbs(parentRel) { + clean := filepath.Clean(fileName) + if clean == ".." || strings.HasPrefix(clean, ".."+string(os.PathSeparator)) || filepath.IsAbs(clean) { return "", downloadDestinationError(fileName, "is outside the output path") } - return candidate, nil + return clean, nil } func downloadDestinationError(fileName, reason string) error { diff --git a/internal/util/download_test.go b/internal/util/download_test.go index c7148b45..e16610fe 100644 --- a/internal/util/download_test.go +++ b/internal/util/download_test.go @@ -101,6 +101,54 @@ func TestCreateFileRejectsUnsupportedDestinations(t *testing.T) { } } +func TestCreateFileDoesNotOverwriteExistingDestination(t *testing.T) { + base := t.TempDir() + path := filepath.Join(base, "a.sql.gz") + if err := os.WriteFile(path, []byte("existing"), 0644); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + file, err := CreateFile(base, "a.sql.gz") + if err == nil { + file.Close() + t.Fatalf("CreateFile() succeeded for existing destination") + } + + content, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile() error = %v", err) + } + if string(content) != "existing" { + t.Fatalf("existing destination was overwritten: %q", string(content)) + } +} + +func TestCreateFileUsesCurrentDirectoryWhenBaseIsEmpty(t *testing.T) { + wd, err := os.Getwd() + if err != nil { + t.Fatalf("Getwd() error = %v", err) + } + base := t.TempDir() + if err := os.Chdir(base); err != nil { + t.Fatalf("Chdir() error = %v", err) + } + t.Cleanup(func() { + if err := os.Chdir(wd); err != nil { + t.Fatalf("restore working directory: %v", err) + } + }) + + file, err := CreateFile("", "a.sql.gz") + if err != nil { + t.Fatalf("CreateFile() error = %v", err) + } + file.Close() + + if _, err := os.Stat(filepath.Join(base, "a.sql.gz")); err != nil { + t.Fatalf("expected file in current directory: %v", err) + } +} + func TestCreateFileRejectsSymlinkedParentOutsideBase(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("symlink behavior requires additional privileges on Windows") From e3f9fabb4584ee553eadd923fa09bf8780249c36 Mon Sep 17 00:00:00 2001 From: yangxin Date: Wed, 29 Apr 2026 11:21:44 +0800 Subject: [PATCH 4/4] Refine export download destination checks --- internal/util/download.go | 5 ++++- internal/util/download_test.go | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/util/download.go b/internal/util/download.go index 5e799321..acb579e6 100644 --- a/internal/util/download.go +++ b/internal/util/download.go @@ -102,7 +102,10 @@ func safeDownloadPath(fileName string) (string, error) { } clean := filepath.Clean(fileName) - if clean == ".." || strings.HasPrefix(clean, ".."+string(os.PathSeparator)) || filepath.IsAbs(clean) { + if clean == "." || clean == ".." { + return "", downloadDestinationError(fileName, "must refer to a file") + } + if strings.HasPrefix(clean, ".."+string(os.PathSeparator)) || filepath.IsAbs(clean) { return "", downloadDestinationError(fileName, "is outside the output path") } diff --git a/internal/util/download_test.go b/internal/util/download_test.go index e16610fe..0574cdaf 100644 --- a/internal/util/download_test.go +++ b/internal/util/download_test.go @@ -88,6 +88,10 @@ func TestCreateFileRejectsUnsupportedDestinations(t *testing.T) { name: "parent directory", fileName: "..", }, + { + name: "cleans to current directory", + fileName: filepath.Join("a", ".."), + }, } for _, tt := range tests {