From 68faf056d3e093668f223caadb536e9fb2440719 Mon Sep 17 00:00:00 2001 From: Sungkyu Yoo Date: Sun, 19 Apr 2026 22:32:41 +0900 Subject: [PATCH 1/3] Potential fix for code scanning alert no. 9: Uncontrolled data used in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- internal/services/s3/store.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/internal/services/s3/store.go b/internal/services/s3/store.go index 177754b..6d06700 100644 --- a/internal/services/s3/store.go +++ b/internal/services/s3/store.go @@ -26,10 +26,22 @@ func NewFileStore(baseDir string) *FileStore { // safePath joins the components under baseDir and verifies the result does not // escape the base directory. It returns an error on path traversal attempts. func (fs *FileStore) safePath(parts ...string) (string, error) { + for _, part := range parts { + if part == "" || part == "." || part == ".." || + strings.Contains(part, "/") || strings.Contains(part, "\\") { + return "", fmt.Errorf("invalid path component: %q", part) + } + } + joined := filepath.Join(append([]string{fs.baseDir}, parts...)...) cleaned := filepath.Clean(joined) + + rel, err := filepath.Rel(fs.baseDir, cleaned) + if err != nil { + return "", fmt.Errorf("invalid path: %w", err) + } // Ensure the resolved path is still under baseDir. - if !strings.HasPrefix(cleaned, fs.baseDir+string(filepath.Separator)) && cleaned != fs.baseDir { + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { return "", fmt.Errorf("path traversal detected: %s", cleaned) } return cleaned, nil From aafb8e11ac9d91a27d52a0684fe01436e638366f Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Mon, 20 Apr 2026 05:19:35 +0900 Subject: [PATCH 2/3] fix: normalize baseDir and use portable path separator check in FileStore Address code review feedback: apply filepath.Clean to baseDir at construction time for consistent absolute paths, and replace hardcoded "/" and "\\" with os.IsPathSeparator for cross-platform correctness. --- internal/services/s3/store.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/services/s3/store.go b/internal/services/s3/store.go index 6d06700..a712892 100644 --- a/internal/services/s3/store.go +++ b/internal/services/s3/store.go @@ -18,9 +18,9 @@ type FileStore struct { func NewFileStore(baseDir string) *FileStore { abs, err := filepath.Abs(baseDir) if err != nil { - abs = baseDir + abs = filepath.Clean(baseDir) } - return &FileStore{baseDir: abs} + return &FileStore{baseDir: filepath.Clean(abs)} } // safePath joins the components under baseDir and verifies the result does not @@ -28,7 +28,8 @@ func NewFileStore(baseDir string) *FileStore { func (fs *FileStore) safePath(parts ...string) (string, error) { for _, part := range parts { if part == "" || part == "." || part == ".." || - strings.Contains(part, "/") || strings.Contains(part, "\\") { + strings.ContainsAny(part, "/\\") || + strings.ContainsFunc(part, func(r rune) bool { return os.IsPathSeparator(byte(r)) }) { return "", fmt.Errorf("invalid path component: %q", part) } } From a9c5e8c931dc1326b3e3a9f1aefb04bcc64586dd Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Mon, 20 Apr 2026 07:14:12 +0900 Subject: [PATCH 3/3] fix: allow slashes in S3 object keys while keeping path traversal protection The safePath function rejected all components containing path separators, but S3 object keys like "photos/a.jpg" legitimately contain slashes. Split objectPath to validate accountID/bucket strictly (no separators), then allow slashes in the key while still enforcing baseDir containment via filepath.Rel. --- internal/services/s3/store.go | 44 +++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/internal/services/s3/store.go b/internal/services/s3/store.go index a712892..a732834 100644 --- a/internal/services/s3/store.go +++ b/internal/services/s3/store.go @@ -23,14 +23,26 @@ func NewFileStore(baseDir string) *FileStore { return &FileStore{baseDir: filepath.Clean(abs)} } +// validPathComponent checks that a single path segment contains no path +// separators, dot-only components, or empty values. +func validPathComponent(part string) error { + if part == "" || part == "." || part == ".." { + return fmt.Errorf("invalid path component: %q", part) + } + if strings.ContainsAny(part, "/\\") || + strings.ContainsFunc(part, func(r rune) bool { return os.IsPathSeparator(byte(r)) }) { + return fmt.Errorf("invalid path component: %q", part) + } + return nil +} + // safePath joins the components under baseDir and verifies the result does not // escape the base directory. It returns an error on path traversal attempts. +// All components must be single path segments (no separators). func (fs *FileStore) safePath(parts ...string) (string, error) { for _, part := range parts { - if part == "" || part == "." || part == ".." || - strings.ContainsAny(part, "/\\") || - strings.ContainsFunc(part, func(r rune) bool { return os.IsPathSeparator(byte(r)) }) { - return "", fmt.Errorf("invalid path component: %q", part) + if err := validPathComponent(part); err != nil { + return "", err } } @@ -49,8 +61,30 @@ func (fs *FileStore) safePath(parts ...string) (string, error) { } // objectPath returns the absolute filesystem path for the given object. +// Unlike safePath, the key may contain '/' (e.g. "photos/a.jpg") which is +// valid for S3 object keys. Containment under baseDir is still enforced. func (fs *FileStore) objectPath(accountID, bucket, key string) (string, error) { - return fs.safePath(accountID, bucket, key) + if err := validPathComponent(accountID); err != nil { + return "", err + } + if err := validPathComponent(bucket); err != nil { + return "", err + } + if key == "" { + return "", fmt.Errorf("invalid path component: %q", key) + } + + joined := filepath.Join(fs.baseDir, accountID, bucket, key) + cleaned := filepath.Clean(joined) + + rel, err := filepath.Rel(fs.baseDir, cleaned) + if err != nil { + return "", fmt.Errorf("invalid path: %w", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + return "", fmt.Errorf("path traversal detected: %s", cleaned) + } + return cleaned, nil } // bucketDir returns the absolute filesystem path for the given bucket.