Potential fix for code scanning alert no. 8: Uncontrolled data used in path expression#3
Potential fix for code scanning alert no. 8: Uncontrolled data used in path expression#3VisualBean wants to merge 1 commit into
Conversation
…n path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/storage/filesystem.go`:
- Around line 675-692: Update every call to sanitizeReference in ManifestPath,
GetManifest, and DeleteManifest to handle its (string, error) result before
passing the sanitized value to safeJoin, propagating or returning the validation
error appropriately. Remove the redundant slash and backslash checks from
sanitizeReference, leaving the whitelist validation to reject them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0eb2518-572a-45fa-b46d-9984c4199522
📒 Files selected for processing (1)
internal/storage/filesystem.go
| func sanitizeReference(reference string) (string, error) { | ||
| reference = strings.TrimSpace(reference) | ||
| if reference == "" { | ||
| return "", fmt.Errorf("invalid reference") | ||
| } | ||
| if strings.Contains(reference, "/") || strings.Contains(reference, "\\") || strings.Contains(reference, "..") { | ||
| return "", fmt.Errorf("invalid reference") | ||
| } | ||
| for _, r := range reference { | ||
| if (r >= 'a' && r <= 'z') || | ||
| (r >= 'A' && r <= 'Z') || | ||
| (r >= '0' && r <= '9') || | ||
| r == '.' || r == '-' || r == '_' { | ||
| continue | ||
| } | ||
| return "", fmt.Errorf("invalid reference") | ||
| } | ||
| return reference, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of sanitizeReference to ensure all call sites are updated.
rg -n -C 2 'sanitizeReference\(' internal/storage/Repository: ByteBardOrg/simplecontainerregistry
Length of output: 1852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,190p' internal/storage/filesystem.go | cat -nRepository: ByteBardOrg/simplecontainerregistry
Length of output: 4758
Handle sanitizeReference's error at every call site
ManifestPath, GetManifest, and DeleteManifest still pass sanitizeReference(...) straight into safeJoin; each needs to unwrap the (string, error) result first or this file won't compile. The / and \ checks are redundant with the whitelist loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/storage/filesystem.go` around lines 675 - 692, Update every call to
sanitizeReference in ManifestPath, GetManifest, and DeleteManifest to handle its
(string, error) result before passing the sanitized value to safeJoin,
propagating or returning the validation error appropriately. Remove the
redundant slash and backslash checks from sanitizeReference, leaving the
whitelist validation to reject them.
1 participant
Potential fix for https://github.com/ByteBardOrg/simplecontainerregistry/security/code-scanning/8
General fix: validate untrusted path components with strict allowlisting before using them in filesystem paths, and fail closed when invalid. For tag names used as filenames, accept only safe filename characters and reject separators / traversal-like input.
Best fix here without changing intended behavior: in
internal/storage/filesystem.go, hardensanitizeReferenceso it returns an error and enforces a safe filename policy (non-empty, no/or\, no.., only[A-Za-z0-9._-]). Then updatelinkManifestTagto use this validator and return the error instead of writing with an unsafe path. This addresses both CodeQL variants because both flows end at the same sink (os.WriteFile(tagPath, ...)).Changes needed:
sanitizeReferencesignature:func sanitizeReference(reference string) (string, error)strings/fmtimports (no new dependency needed).linkManifestTagto handle error and use validated name insafeJoin.Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by CodeRabbit