fix: Potential fix for code scanning alert no. 7: Uncontrolled data used in path expression#18
Merged
Conversation
…n path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideHardens Lambda code storage path construction to validate user-derived path components and enforce robust canonical containment checks against the configured code directory, returning a fully resolved safe absolute path or a deterministic error on invalid/traversal input. 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 found 1 issue, and left some high level feedback:
- Returning
absCleanedinstead of the previously relativecleanedvalue changes the function’s output semantics; double-check callers aren’t relying oncodePathreturning a path relative tos.codeDirbefore making this behavior change. - The
isSafeComponenthelper rejects any value containing"..", which is stricter than just blocking".."as a component and may unexpectedly reject valid names (e.g.,"user..1"); consider tightening the check to disallow only path-segment traversal rather than any substring. - The new path validation logic (clean + Abs + Rel checks) is fairly intricate; consider adding a short comment or extracting it into a reusable helper to clarify the invariants (e.g., which inputs are considered valid components and why) and reduce the chance of future regressions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Returning `absCleaned` instead of the previously relative `cleaned` value changes the function’s output semantics; double-check callers aren’t relying on `codePath` returning a path relative to `s.codeDir` before making this behavior change.
- The `isSafeComponent` helper rejects any value containing `".."`, which is stricter than just blocking `".."` as a component and may unexpectedly reject valid names (e.g., `"user..1"`); consider tightening the check to disallow only path-segment traversal rather than any substring.
- The new path validation logic (clean + Abs + Rel checks) is fairly intricate; consider adding a short comment or extracting it into a reusable helper to clarify the invariants (e.g., which inputs are considered valid components and why) and reduce the chance of future regressions.
## Individual Comments
### Comment 1
<location path="internal/services/lambda/store.go" line_range="157" />
<code_context>
-// It validates the result stays under codeDir to prevent path traversal.
+// It validates path components and ensures the resolved path stays under codeDir.
func (s *LambdaStore) codePath(accountID, functionName string) (string, error) {
+ isSafeComponent := func(v string) bool {
+ if v == "" || v == "." || v == ".." {
+ return false
+ }
+ if filepath.IsAbs(v) {
+ return false
+ }
+ if strings.Contains(v, "/") || strings.Contains(v, "\\") || strings.Contains(v, "..") {
+ return false
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `strings.Contains(v, "..")` check may be stricter than necessary and disallow valid IDs with consecutive dots.
This condition rejects any value containing ".." anywhere (e.g. "user..1"), which may be valid depending on how IDs are defined. If the aim is only to block path traversal, `filepath.Clean` + `Rel` plus the explicit `v == ".."` check already cover that. Consider dropping the `strings.Contains(v, "..")` check and relying on the path normalization instead, unless you explicitly want to forbid all such IDs.
```suggestion
if strings.Contains(v, "/") || strings.Contains(v, "\\") {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ing check, return cleaned path - Extract isSafePathComponent as a package-level helper with a doc comment explaining why ".." as a substring is allowed (traversal is caught by Rel) - Drop the overly strict strings.Contains(v, "..") check that rejected valid names like "user..1" - Return cleaned (relative) instead of absCleaned to preserve original output semantics for callers - Apply the same isSafePathComponent + filepath.Rel containment check in DeleteFunction for consistency
Keep isSafePathComponent (loosened ".." substring check per review), main's separate field-specific error messages, slog.Debug logging in DeleteFunction, and filepath.IsAbs(rel) defense-in-depth check.
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/7
General fix: ensure user-derived path components are validated as safe path components, and enforce canonical containment checks using
filepath.Rel(or equivalent), while handling all path resolution errors.Best fix here without changing functionality:
internal/services/lambda/store.go, hardencodePath:accountID/functionNameif they are absolute, contain separators, or contain...s.codeDirand candidate to absolute paths with error checks.filepath.Rel(absBase, absCandidate)and reject if it starts with..or is absolute.No changes are required in
provider.gofor this specific sink once store-layer validation is robust (defense-in-depth at sink boundary).Suggested fixes powered by Copilot Autofix. Review carefully before merging.