fix: Potential fix for code scanning alert no. 11: Uncontrolled data used in path expression#14
Closed
skyoo2003 wants to merge 0 commit into
Closed
fix: Potential fix for code scanning alert no. 11: Uncontrolled data used in path expression#14skyoo2003 wants to merge 0 commit into
skyoo2003 wants to merge 0 commit into
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a defense-in-depth path component validator to the S3 file store and integrates it into safePath to prevent unsafe/uncontrolled path segments from being used in filesystem operations while preserving the existing baseDir containment check. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Validating every
safePathpart withvalidatePathPartwill reject keys that contain intended subdirectories (e.g.foo/bar) or empty segments, which may be valid in the current API; consider limiting this validation to bucket/accountID or splitting the key into components and validating each segment instead of forbidding all separators in the raw key. - By hard-rejecting
"."and".."as path parts, you may be disallowing currently valid object names that happen to be exactly these strings; if those are allowed today, adjust the validation to only treat them as dangerous when they appear as traversal components inside a longer path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Validating every `safePath` part with `validatePathPart` will reject keys that contain intended subdirectories (e.g. `foo/bar`) or empty segments, which may be valid in the current API; consider limiting this validation to bucket/accountID or splitting the key into components and validating each segment instead of forbidding all separators in the raw key.
- By hard-rejecting `"."` and `".."` as path parts, you may be disallowing currently valid object names that happen to be exactly these strings; if those are allowed today, adjust the validation to only treat them as dangerous when they appear as traversal components inside a longer path.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
4b7904a to
9931144
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/skyoo2003/devcloud/security/code-scanning/11
Use defense-in-depth in
internal/services/s3/store.goby validating each untrusted path component before constructing filesystem paths.Best fix (without changing intended functionality):
".",".."components/or\) inside a single componentsafePath, validate every entry inpartswith that helper beforefilepath.Join.This ensures
bucket/accountIDremain single components andkeycannot inject traversal segments. It prevents tainted user path input from reaching filesystem APIs in dangerous form.Suggested fixes powered by Copilot Autofix. Review carefully before merging.