fix: Potential fix for code scanning alert no. 15: Uncontrolled data used in path expression#10
Merged
Merged
Conversation
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors FileStore.safePath to use absolute paths and filepath.Rel for secure, platform-correct containment checks instead of a fragile strings.HasPrefix prefix check, improving protection against path traversal while preserving existing behavior for nested object keys. 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:
- Consider computing and storing the absolute base directory once in
NewFileStore(e.g.,fs.baseAbs) instead of callingfilepath.Abs(fs.baseDir)on everysafePathinvocation to avoid repeated work and potential inconsistencies ifbaseDirchanges. - The traversal check
len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator)is a bit brittle; usingstrings.HasPrefix(rel, ".."+string(filepath.Separator))would be clearer and avoid manual length/slice handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider computing and storing the absolute base directory once in `NewFileStore` (e.g., `fs.baseAbs`) instead of calling `filepath.Abs(fs.baseDir)` on every `safePath` invocation to avoid repeated work and potential inconsistencies if `baseDir` changes.
- The traversal check `len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator)` is a bit brittle; using `strings.HasPrefix(rel, ".."+string(filepath.Separator))` would be clearer and avoid manual length/slice handling.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ation with symlink resolution
skyoo2003
added a commit
that referenced
this pull request
Apr 29, 2026
Scope the override to next's postcss dependency and pin to an exact version to reduce risk of unintended side effects.
skyoo2003
added a commit
that referenced
this pull request
Apr 29, 2026
) * fix: override postcss to >=8.5.10 to resolve XSS vulnerability (dependabot #10) PostCSS <8.5.10 has an XSS vulnerability via unescaped <style> in its CSS Stringify Output. Next.js 16.2.4 depends on postcss@8.4.31 which is vulnerable. Add npm overrides to force postcss@8.5.12. * fix: narrow postcss override to next, pin to 8.5.12 (dependabot #10) Scope the override to next's postcss dependency and pin to an exact version to reduce risk of unintended side effects.
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/15
Use a safer containment check in
internal/services/s3/store.goby validating withfilepath.Relbetween canonical absolute paths, instead ofstrings.HasPrefix.Best fix (without changing functionality):
safePath, build candidate path withfilepath.Join.fs.baseDirand candidate withfilepath.Abs.rel, err := filepath.Rel(baseAbs, candAbs).rel == ".."or starts with".."+separator(or is absolute), otherwise accept.This preserves support for nested object keys while preventing traversal outside
baseDirin a platform-correct way.Suggested fixes powered by Copilot Autofix. Review carefully before merging.