Add path traversal validation to LocalStore.getFilePath#4738
Add path traversal validation to LocalStore.getFilePath#4738glitch-ux wants to merge 1 commit intostacklok:mainfrom
Conversation
LocalStore.getFilePath constructs file paths by joining basePath with a user-provided name but does not validate that the resolved path stays within basePath. A crafted name containing ".." components can escape the designated state directory. Add containment validation using the strings.HasPrefix pattern already established in pkg/fileutils/contained.go (WriteContainedFile). All callers now receive an error for names that resolve outside basePath. Fixes stacklok#4736 Signed-off-by: José Maia <glitch-ux@users.noreply.github.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ChrisJBurns
left a comment
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: go-security-reviewer, go-expert-developer, code-reviewer, toolhive-expert
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | basePath not resolved via EvalSymlinks in production |
10/10 | HIGH | Fix |
| 2 | Duplicated containment check with fileutils/contained.go |
8/10 | MEDIUM | Fix |
| 3 | sub/file test case likely fails — implementation doesn't reject subdirectory paths |
7/10 | HIGH | Fix |
Overall
This PR addresses a real path traversal vulnerability in LocalStore.getFilePath where crafted names containing .. could escape the state directory. The fix uses the well-established filepath.Clean + strings.HasPrefix(resolved, basePath + separator) pattern already present in pkg/fileutils/contained.go, and the test coverage is thorough with table-driven tests, real-world attack patterns, and a round-trip functional test.
However, two issues need to be addressed before merging. First, the production NewLocalStore constructor does not resolve symlinks on basePath, while the test helper does — on macOS (where /var symlinks to /private/var), the HasPrefix check could reject every legitimate name, making this a correctness regression rather than just a security hardening. Second, the sub/file test case expects rejection but the containment check would actually pass for subdirectory paths within basePath. The implementation either needs an explicit path separator check, or the test expectation needs correction. The DRY violation with fileutils/contained.go is a strong recommendation for maintainability but not blocking.
Generated with Claude Code
| // Verify the resolved path is contained within basePath. | ||
| // The trailing separator prevents prefix collisions (e.g. basePath | ||
| // "/state/toolhive" matching "/state/toolhive-evil/foo"). | ||
| if !strings.HasPrefix(resolved, s.basePath+string(os.PathSeparator)) { |
There was a problem hiding this comment.
[HIGH] basePath not resolved via EvalSymlinks in production (Consensus: 10/10)
NewLocalStore sets basePath without resolving symlinks. On macOS, xdg.StateHome typically resolves through /var → /private/var. When getFilePath calls filepath.Clean(filepath.Join(s.basePath, name)), the resolved path may start with /private/var/... while basePath is /var/..., causing this HasPrefix check to reject every legitimate name.
The test helper newTestStore correctly calls filepath.EvalSymlinks, masking this production bug. Add filepath.EvalSymlinks(basePath) after MkdirAll in NewLocalStore.
Raised by: all 4 agents (unanimous)
| } | ||
| return filepath.Join(s.basePath, name) | ||
|
|
||
| resolved := filepath.Clean(filepath.Join(s.basePath, name)) |
There was a problem hiding this comment.
[MEDIUM] Duplicated containment check with fileutils/contained.go (Consensus: 8/10)
This filepath.Clean + strings.HasPrefix(resolved, basePath + separator) pattern is structurally identical to WriteContainedFile in pkg/fileutils/contained.go:19-23. Consider extracting a shared IsPathContained(baseDir, resolved string) bool function in pkg/fileutils/ so both call sites share one auditable implementation.
Raised by: security-review, go-quality-review, code-standards-review
| require.NoError(t, err) | ||
| _, err = writer.Write([]byte(`{"key":"value"}`)) | ||
| require.NoError(t, err) | ||
| require.NoError(t, writer.Close()) |
There was a problem hiding this comment.
[HIGH] sub/file test case likely fails (Consensus: 7/10)
This test expects sub/file to be rejected, but the containment check would pass: filepath.Join(basePath, "sub/file.json") resolves to basePath/sub/file.json, which starts with basePath/. If subdirectory paths should be rejected (correct for a flat state store), add an explicit strings.ContainsAny(name, "/\\") check in getFilePath before the containment check.
Raised by: go-quality-review
2 participants
Summary
LocalStore.getFilePathto prevent crafted state names from escaping the designated directory via..componentsGetReader,GetWriter,CreateExclusive,Delete,Exists) to propagate the validation errorDetails
getFilePathjoinsbasePathwith a user-providednameusingfilepath.Joinbut did not validate that the resolved path stays withinbasePath. A name like../../../etc/passwdwould resolve outside the state directory.The fix uses the
strings.HasPrefix(resolved, basePath + separator)pattern already established inpkg/fileutils/contained.go(WriteContainedFile). The trailing separator prevents prefix collisions (e.g. basePath/state/toolhiveincorrectly matching/state/toolhive-evil/foo).Note: workload-facing code paths already validate names via
fileutils.ValidateWorkloadNameForPath, but the lower-levelLocalStoreis also used for RunConfig and group persistence where that upstream validation may not be applied.Fixes #4736
Test plan
getFilePathcovering valid names and attack patternsCreateExclusiveconflict test