fix: Reject an upload URL whose host differs from the configured upload host - #4556
sushant-me wants to merge 3 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@googlebot I signed it! |
UploadReleaseAssetFromRelease takes release.UploadURL from the server's response, and NewUploadRequest lets an absolute URL replace the client's configured upload host entirely. A response naming a different host was therefore able to receive the artifact together with the caller's Authorization header, while the caller's own WithEnterpriseURLs/WithURLs configuration was silently ignored. Resolve the value against the configured upload origin and reject a differing host, so an upload can only go where the client was told to send it. Relative URLs (the normal case, and what GitHub returns in practice) are unaffected, and the comparison is against uploadURL rather than baseURL because uploads.github.com differs from api.github.com by design.
f8441ba to
cac306d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4556 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 197 197
Lines 18291 18299 +8
=======================================
+ Hits 18030 18038 +8
Misses 261 261 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@sushant-me - can you please increase the CodeCov results for this critical piece of code? |
Exercises the url.Parse error return in UploadReleaseAssetFromRelease so every line of the new host check is covered. A URL containing an ASCII control character makes net/url refuse the value, and the helper must surface that as an error rather than uploading to an unchecked host.
|
@gmlewis done - added |
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @sushant-me!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
|
Thanks for the LGTM, @gmlewis! Everything is green CLA, codecov patch/project, and the full test matrix and the branch is current with master, so this is ready for a second LGTM whenever someone has bandwidth. Happy to make any adjustments if something needs changing. |
UploadReleaseAssetFromReleasetakesrelease.UploadURLfrom the server's response, andNewUploadRequestlets an absolute URL replace the client's configured upload host entirely (url.URL.Parsediscards the receiver for an absolute reference). A response naming a different host therefore received the artifact and the caller'sAuthorizationheader, while the caller's ownWithURLs/WithEnterpriseURLsupload configuration was silently ignored.The check runs only for absolute values; relative URLs — what GitHub actually returns, and the case the existing tests cover — are untouched. It compares against
c.uploadURLrather thanc.baseURL, becauseuploads.github.comandapi.github.comdiffer by design.This does not overlap with #4363/#4364/#4366: those scope the token for a foreign origin but would still POST the artifact body there. Rejecting the request keeps both on the configured host.
TestRepositoriesService_UploadReleaseAssetFromRelease_ForeignHostIsRejectedfails onmaster(expected an error for an upload URL naming a foreign host, got nil) and passes with this change.go test ./github/andscript/lint.shboth pass.Assisted contribution: an AI tool drafted the change, the test, and this description. I reviewed the rendered diff, ran the full
githubpackage test suite andscript/lint.sh, and verified the new test fails without the fix.