fix: cp should not mount if src and dst credentials are different#1903
Conversation
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1903 +/- ##
==========================================
- Coverage 87.31% 87.29% -0.02%
==========================================
Files 143 143
Lines 5525 5535 +10
==========================================
+ Hits 4824 4832 +8
- Misses 415 417 +2
Partials 286 286 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a security issue where the cp command could incorrectly attempt to mount blobs across repositories when the source and destination use different credentials. The fix extracts mount eligibility logic into a dedicated function that now validates credential equality before allowing cross-repository mounting.
- Extracted mount check logic into a new
checkMountfunction for better testability and maintainability - Added credential comparison to prevent mounting when source and destination have different authentication
- Added comprehensive test coverage for the new function covering multiple scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/oras/root/cp.go | Extracts mount checking logic into checkMount function and adds credential comparison to prevent mounting with different credentials |
| cmd/oras/root/cp_test.go | Adds comprehensive test suite for checkMount function covering same/different registries, same/different credentials, and remote/non-remote sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TerryHowe
left a comment
There was a problem hiding this comment.
/lgtm
nit I think something like "canMount" would be a better name.
I first named the function |
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
|
@shizhMSFT Do you want to take a look |
…as-project#1903) Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #1892
Please check the following list: