fix(api): hide cross-repo PR head repository/SHA from public-only tokens - #39167
Open
qw3rtyou wants to merge 2 commits into
Open
fix(api): hide cross-repo PR head repository/SHA from public-only tokens#39167qw3rtyou wants to merge 2 commits into
qw3rtyou wants to merge 2 commits into
Conversation
GetPullRequest, GetPullRequestByBaseHead, GetPullRequestByMergedCommit, ListPullRequests, ListPinnedPullRequests, and EditPullRequest all serialize pr.HeadRepo via convert.ToAPIPullRequest(s) without checking whether the current API token is scoped to see that repository. A public-only, read:repository token can therefore read a private (or otherwise restricted) head repository's metadata and latest commit SHA through any of these endpoints on a public base repository, even though it was never granted access to the head repository itself. convert.ToAPIPullRequest(s) can't apply the check itself -- it only receives a doer, not the APIContext the token scope lives on. This adds hideCrossRepoPRHead(s) in routers/api/v1/repo, called after conversion in every affected handler, which nulls Head.Repository/Head.Sha when ctx.TokenCanAccessRepo(pr.HeadRepo) fails. Mirrors the guard already applied to UpdatePullRequest and parseCompareInfo (the fix for the earlier, related advisory). Follow-up to GHSA-m78w-jjjx-gp8r, which reported this second code path alongside the primary parseCompareInfo fix.
GiteaBot
added
the
lgtm/need 2
…llRequests) TestAPIGetPullRequestPublicOnlyToken and TestAPIListPullRequestsPublicOnlyToken, modeled on the existing TestAPIComparePublicOnlyToken. Both verified: full token still sees the private head repo/SHA, public-only token sees neither, base repo/PR itself stays visible either way. Ran locally end-to-end (real server, sqlite, actual HTTP requests) -- both new tests pass, plus no regression in TestAPIViewPulls, TestAPIViewPullsByBaseHead, TestAPIEditPull, TestAPIViewPullFilesWithHeadRepoDeleted, TestAPIListPinnedPullrequests.
qw3rtyou
changed the title
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
2 participants
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
Follow-up to GHSA-m78w-jjjx-gp8r. Alongside the
parseCompareInfoscope gap (already fixed), theadvisory noted a second, distinct code path:
services/convert.ToAPIPullRequest(s)serializespr.HeadRepo(repository metadata, clone URLs,privateflag) and the head commit SHA with nocheck on whether the current API token is scoped to see that repository.
convert.ToAPIPullRequest(s)can't apply the check itself — it only receives adoer, not theAPIContextthe token scope lives on — so the guard has to live in every handler that serializes aPR's head. These didn't have it:
GetPullRequestGetPullRequestByBaseHeadGetPullRequestByMergedCommitListPullRequestsListPinnedPullRequestsEditPullRequestA public-only,
read:repositorytoken can reach any of these through a public base repository andread a private (or otherwise restricted) head repository's metadata and latest commit SHA, even
though it was never granted access to the head repository itself.
Fix
Adds
hideCrossRepoPRHead(s)inrouters/api/v1/repo, called afterconvert.ToAPIPullRequest(s)in every affected handler. It nulls
Head.Repository/Head.Shawhenctx.TokenCanAccessRepo(pr.HeadRepo)fails — mirroring the guard already applied toUpdatePullRequestandparseCompareInfo.CreatePullRequestis intentionally left alone: the caller must already have push access to thehead branch to open the PR, so there's no token-scope gap there.
Test plan
Added
TestAPIGetPullRequestPublicOnlyTokenandTestAPIListPullRequestsPublicOnlyToken(
tests/integration/api_pull_public_only_test.go), modeled on the existingTestAPIComparePublicOnlyToken. Ran locally end-to-end against a real sqlite-backed instance:neither, base repo/PR stays visible either way.
TestAPIViewPulls,TestAPIViewPullsByBaseHead,TestAPIEditPull,TestAPIViewPullFilesWithHeadRepoDeleted,TestAPIListPinnedPullrequests.GetPullRequestByMergedCommit(couldn't find an existing APItest file for that endpoint to model one on) — happy to add if useful.