fix: close security-relevant gaps found in codebase audit - #547
Closed
PierrunoYT wants to merge 2 commits into
Closed
fix: close security-relevant gaps found in codebase audit#547PierrunoYT wants to merge 2 commits into
PierrunoYT wants to merge 2 commits into
Conversation
Split out of Gitlawb#481 per review feedback, grouping the security-sensitive findings from issue Gitlawb#480 (a multi-agent codebase audit) separately from the UX/robustness ones: - sandbox: grant_scope path comparisons were case-sensitive, letting a persisted deny grant (e.g. `deny --path C:\Users\me\project\Secrets`) be silently bypassed by spelling a Windows path with different case - securefile/oauth: securefile.go's createSecretFile lacked the ERROR_ACCESS_DENIED handling oauth/encrypt.go's already had, so concurrent zero processes on Windows could spuriously hard-fail credstore's encrypted API-key storage with "Access is denied"; both also discarded the real lock-creation error in favor of a subsequent unrelated ErrNotExist, masking genuine ACL failures behind a misleading "timed out: file does not exist" - update: checksum verification didn't cross-check the archive filename it verified against the one being extracted, so a checksum file naming a different (but validly-hashed) archive could vouch for the wrong bytes before extraction; extractZip didn't reject symlink-mode entries like extractTarGz already did All three fixes ship with regression tests. Refs Gitlawb#480.
This was referenced Jul 6, 2026
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR updates secret-file retry handling, sandbox scope path matching, archive checksum validation, and ZIP extraction entry checks. ChangesBug fixes and validation improvements
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses security-sensitive findings from the codebase audit by hardening update-archive verification/extraction, fixing Windows lock-contention behavior in secret-file creation, and ensuring sandbox persisted grants can’t be bypassed via Windows path casing differences.
Changes:
- Update: enforce checksum filename cross-checking and reject non-regular ZIP entries (e.g., symlink-mode) during extraction.
- Secure storage: retry secret lock creation on Windows
ERROR_ACCESS_DENIED-style contention and preserve the underlying lock error instead of masking it. - Sandbox: make file/dir scope matching case-insensitive on Windows by routing comparisons through
filepath.Rel.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/update/extract.go | Rejects non-regular ZIP entries to prevent symlink/special-entry extraction. |
| internal/update/extract_test.go | Adds regression coverage for rejecting symlink-mode ZIP entries. |
| internal/update/apply.go | Adds verifyArchiveChecksum to cross-check checksum target name vs expected archive. |
| internal/update/apply_test.go | Adds regression test ensuring filename mismatches are rejected even if the checksum line is otherwise valid. |
| internal/securefile/securefile.go | Treats Windows os.ErrPermission from lock creation as contention and preserves the lock creation error for timeouts. |
| internal/securefile/securefile_test.go | Adds a regression test verifying retry behavior when lock creation returns os.ErrPermission. |
| internal/sandbox/grant_scope.go | Uses filepath.Rel-based helpers for scope path equality/containment to avoid case-based bypass on Windows. |
| internal/sandbox/grant_scope_test.go | Adds a Windows-only regression test for case-insensitive scope matching. |
| internal/oauth/encrypt.go | Aligns OAuth token secret creation timeout error preservation with securefile behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+135
to
138
| lastErr = err | ||
| if data, rerr := readSecretFileRetry(path); rerr == nil { | ||
| return data, nil | ||
| } else { | ||
| lastErr = rerr | ||
| } |
Comment on lines
+129
to
132
| lastErr = err | ||
| if data, rerr := readSecretFileRetry(path); rerr == nil { | ||
| return data, nil | ||
| } else { | ||
| lastErr = rerr | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
Summary
Fixes #545. Split out of #481 per review feedback, grouping the security-sensitive findings from #480 separately from the UX/robustness ones (see #546 for those).
grant_scopepath comparisons were case-sensitive, letting a persisted deny grant be bypassed by spelling a Windows path with different casesecurefile.go'screateSecretFilelacked theERROR_ACCESS_DENIEDhandlingoauth/encrypt.goalready had, so concurrentzeroprocesses on Windows could spuriously hard-fail credstore's encrypted API-key storage with "Access is denied"; both also discarded the real lock-creation error in favor of a subsequent unrelatedErrNotExist, masking genuine ACL failures behind a misleading "timed out: file does not exist" (not explicitly named in the split request, but grouped here since it's a matched pair and touches credential storage)extractZipdidn't reject symlink-mode entries likeextractTarGzalready didTest plan
go build ./...cleango test ./internal/sandbox/... ./internal/securefile/... ./internal/oauth/... ./internal/update/...passesSummary by CodeRabbit