Skip to content

Commit 8e53ae2

Browse files
committed
refactor: use filepath.Rel for path containment checks per review
Replace string HasPrefix checks with isWithinDir helper using filepath.Rel for more robust cross-platform containment validation.
1 parent 9fbb3dd commit 8e53ae2

3 files changed

Lines changed: 57 additions & 8 deletions

File tree

internal/services/ecr/store.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,23 @@ import (
1919
"github.com/skyoo2003/devcloud/internal/storage/sqlite"
2020
)
2121

22+
// isWithinDir returns true if child resolves to a path within parent.
23+
func isWithinDir(child, parent string) bool {
24+
absChild, err := filepath.Abs(filepath.Clean(child))
25+
if err != nil {
26+
return false
27+
}
28+
absParent, err := filepath.Abs(filepath.Clean(parent))
29+
if err != nil {
30+
return false
31+
}
32+
rel, err := filepath.Rel(absParent, absChild)
33+
if err != nil {
34+
return false
35+
}
36+
return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel)
37+
}
38+
2239
var (
2340
ErrRepositoryNotFound = errors.New("repository not found")
2441
ErrRepositoryAlreadyExists = errors.New("repository already exists")
@@ -404,9 +421,8 @@ func (s *ECRStore) UploadLayerPart(accountID, repoName, uploadID string, partFir
404421

405422
// Save blob to filesystem.
406423
dir := filepath.Join(s.dataDir, "_layers", uploadID, "parts")
407-
cleaned := filepath.Clean(dir)
408-
if !strings.HasPrefix(cleaned, filepath.Clean(s.dataDir)+string(filepath.Separator)) {
409-
return fmt.Errorf("path traversal detected: %s", cleaned)
424+
if !isWithinDir(dir, s.dataDir) {
425+
return fmt.Errorf("path traversal detected: %s", dir)
410426
}
411427
if err := os.MkdirAll(dir, 0o755); err != nil {
412428
return fmt.Errorf("mkdir layer parts: %w", err)

internal/services/lambda/store.go

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,23 @@ func isSafePathComponent(v string) bool {
153153
!strings.ContainsAny(v, "/\\")
154154
}
155155

156+
// isWithinDir returns true if child resolves to a path within parent.
157+
func isWithinDir(child, parent string) bool {
158+
absChild, err := filepath.Abs(filepath.Clean(child))
159+
if err != nil {
160+
return false
161+
}
162+
absParent, err := filepath.Abs(filepath.Clean(parent))
163+
if err != nil {
164+
return false
165+
}
166+
rel, err := filepath.Rel(absParent, absChild)
167+
if err != nil {
168+
return false
169+
}
170+
return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel)
171+
}
172+
156173
// codePath returns the filesystem path for a function's code zip.
157174
// It validates that accountID and functionName are single path segments and that
158175
// the resolved path stays within s.codeDir to prevent path traversal.
@@ -196,7 +213,7 @@ func (s *LambdaStore) CreateFunction(info *FunctionInfo, codeZip []byte) (*Funct
196213
return nil, err
197214
}
198215
dir := filepath.Dir(path)
199-
if !strings.HasPrefix(filepath.Clean(dir), filepath.Clean(s.codeDir)+string(filepath.Separator)) {
216+
if !isWithinDir(dir, s.codeDir) {
200217
return nil, fmt.Errorf("path traversal detected: %s", dir)
201218
}
202219
if err := os.MkdirAll(dir, 0o755); err != nil {
@@ -414,7 +431,7 @@ func (s *LambdaStore) UpdateFunctionCode(accountID, functionName string, codeZip
414431
return nil, err
415432
}
416433
dir := filepath.Dir(path)
417-
if !strings.HasPrefix(filepath.Clean(dir), filepath.Clean(s.codeDir)+string(filepath.Separator)) {
434+
if !isWithinDir(dir, s.codeDir) {
418435
return nil, fmt.Errorf("path traversal detected: %s", dir)
419436
}
420437
if err := os.MkdirAll(dir, 0o755); err != nil {

internal/services/s3/provider.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,23 @@ import (
2626
"github.com/skyoo2003/devcloud/internal/plugin"
2727
)
2828

29+
// isWithinDir returns true if child resolves to a path within parent.
30+
func isWithinDir(child, parent string) bool {
31+
absChild, err := filepath.Abs(filepath.Clean(child))
32+
if err != nil {
33+
return false
34+
}
35+
absParent, err := filepath.Abs(filepath.Clean(parent))
36+
if err != nil {
37+
return false
38+
}
39+
rel, err := filepath.Rel(absParent, absChild)
40+
if err != nil {
41+
return false
42+
}
43+
return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel)
44+
}
45+
2946
const defaultAccountID = plugin.DefaultAccountID
3047

3148
// S3Provider implements plugin.ServicePlugin using FileStore and MetadataStore.
@@ -470,11 +487,10 @@ func (p *S3Provider) multipartDir(uploadID string) (string, error) {
470487
return "", fmt.Errorf("invalid upload id")
471488
}
472489
dir := filepath.Join(p.fileStore.baseDir, "_multipart", uploadID)
473-
cleaned := filepath.Clean(dir)
474-
if !strings.HasPrefix(cleaned, filepath.Clean(p.fileStore.baseDir)+string(filepath.Separator)) {
490+
if !isWithinDir(dir, p.fileStore.baseDir) {
475491
return "", fmt.Errorf("path traversal detected in multipart dir")
476492
}
477-
return cleaned, nil
493+
return filepath.Clean(dir), nil
478494
}
479495

480496
// partPath returns the path to a specific part file.

0 commit comments

Comments
 (0)