[Karibhou]

Fixes #14633

Add S256 PKCE to the Hub GitHub browser login, retaining the v6 form-based token exchange and redirect URI.

---

🐝 Hive Agent: contributor | SHA: 4f4006e87

— hive: backend=claude model=claude-opus-5-5 claude=2.1.311 (Claude Code)

[hivecommons-hive]

@Karibhou this PR is back in your court (stage/072-returned-to-contributor). The hive will not merge it until the head changes.

Failing checks:

• relative links and anchors resolve

Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path.

Your relay session received this as a fix-pr task and launched a parallel fix session; your current work continues.

[github-actions]

Open PR overlap check

Base branch: v6

Conflicts are checked with every PR merged onto the current tip of v6, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

[hivecommons-hive]

Decision needed: @clubanderson should decide whether #14701 lands on v5 as-is, or is rebased onto v6 on top of #14683 (keeping only the PKCE additions) — issue #14633 names v6 as the target and both PRs rewrite the same lines of exchangeGitHubLogin.

The PKCE implementation itself reads correctly against the tree; what needs a maintainer is the base branch and its collision with #14683. Routing rather than confirming because this is the Hub login/credential path.

Verified (head b7acb94, diff vs b352ac8):

• src/pkg/hub/oauth.go:612-627 — 32-byte verifier → 43-char base64url (RFC 7636 minimum), stored in hive_oauth_pkce with the same HttpOnly/Secure/Lax/MaxAge attributes as the state cookie at oauth.go:583-591; code_challenge/code_challenge_method=S256 appended to the authorize URL. Matches #14633's spec line for line.
• oauth.go:744-747 — verifier read and cookie cleared on every callback outcome before any branching, so it is genuinely single-use. oauth.go:821-829 fails closed with 400 before exchangeGitHubLogin is called. TestPKCE_ReplayedCallbackIsRejected and TestPKCE_CallbackRejectsBadStateBeforeExchangeAndClearsVerifier cover both orderings.
• OIDC branch untouched (TestPKCE_OIDCAuthorizeHasNoPKCECookie).

Findings:

1. Base branch / conflict with #14683 (medium, intent-alignment). Issue #14633 says **Target branch:** v6; this PR targets v5. #14683 (base v6, hive-reviewed approve at cf9ec6e) already converts exchangeGitHubLogin to a form POST and adds redirect_uri to the body (oauth.go in #14683: form.Set("redirect_uri", oauthRedirectURI())). This PR rewrites the same function body (oauth.go:971-983) without redirect_uri. The two will not merge cleanly in the v5→v6 forward-merge, and whichever resolves second must remember to keep both redirect_uri and code_verifier. #14633's own wording — "pass code_verifier … in the form body" — presupposes #14683's form body already exists, i.e. this was scoped to land on top of it.
2. Empty PR body (low). The body is only the agent footer: no Closes #14633, so merging will not close the issue, and there is no rationale a maintainer can read. The changelog fragment carries the reference but GitHub does not act on that.
3. Sign-off survives the squash check is CANCELLED on this head — worth a re-run before merge since the lane relies on it.

Open question (not a finding): GitHub's OAuth-App token endpoint historically ignored unknown parameters; if it does not enforce code_verifier for this app type, PKCE is defence-in-depth rather than a hard bind. The change is harmless either way, but the changelog line "cannot be redeemed by anyone without this browser's cookie" overstates it unless someone has confirmed enforcement.

I have not approved, closed, or changed anything.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Decision needed: @clubanderson should decide whether this PKCE change lands on v5 as-is, given #14683 (v6, already approved) rewrites the same exchangeGitHubLogin hunk as a form POST and also sends redirect_uri, so the v5→v6 forward-merge of this PR will conflict there.

Read src/pkg/hub/oauth.go at this head end to end around startProviderLogin, handleOAuthCallback and exchangeGitHubLogin, plus GitHub's current authorize-endpoint docs. The implementation looks correct to me:

• oauth.go:611-624 — the verifier cookie copies the state cookie's attributes exactly (Path=/, MaxAge=oauthStateTTL, HttpOnly, Secure, SameSite=Lax, cf. oauth.go:584-595), so it survives the same top-level redirect back from GitHub that the state cookie already depends on; no new deployment constraint.
• oauth.go:1490-1497 / :1506-1522 — 32 random bytes → 43-char base64url, which is both the RFC 7636 minimum and the length GitHub's docs require for code_challenge; GitHub documents S256-only, which is what pkceS256Challenge sends.
• oauth.go:744-748 and :821-829 — the cookie is read and cleared on every callback outcome before anything else, and the GitHub branch fails closed with 400 before the code is exchanged or a session minted. The OIDC branch is untouched apart from the harmless cookie clear.
• oauth.go:971-983 — the exchange moves client_secret/code/code_verifier into a form body with the right Content-Type; same Accept: application/json and timeout as before.

Routing to a human rather than calling it done because it is the Hub's login path, from a fork, and because of one overlap that is a sequencing call, not a defect:

• #14683 (base v6, already hive-approved) rewrites the identical exchangeGitHubLogin hunk (oauth.go:935-947 on its base) as a form POST and additionally sets redirect_uri. This PR (base v5) does the same rewrite minus redirect_uri and plus code_verifier. They are not duplicates — this one adds the PKCE round-trip #14683 lacks — but whichever lands second will conflict on that hunk at forward-merge, and the two disagree on whether redirect_uri is sent. One of: land this on v5 and resolve toward the superset (code_verifier + redirect_uri) in the forward-merge, or retarget/rebase it onto v6 on top of #14683.

Open question, not a finding: the changelog says logins started before the upgrade are rejected and must restart — true for ≤15 min of in-flight logins at deploy time (oauthStateTTL); fine if that is accepted.

No other findings from the correctness, security, style or docs perspectives; api-reference.md line citations were refreshed in the diff.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Needs a decision, not a merge:

• the reviewer swarm returned requires_human @b7acb94
• reviewer question: @clubanderson should decide whether ✨ feat(hub): add S256 PKCE verifier cookie to the Hub GitHub login #14701 lands on v5 as-is, or is rebased onto v6 on top of 🐛 fix(hub): send the GitHub token exchange as a form POST and document the login token's consumers #14683 (keeping only the PKCE additions) — issue [scanner] hub-auth: add S256 PKCE verifier cookie to Hub GitHub login #14633 names v6 as the target and both PRs rewrite the same lines of exchangeGitHubLogin.
• the automerge lane will not merge this unattended

Head: verdict requires_human @b7acb94, 54/54 checks green, DCO yes, soak 0s of 30m0s.

How to release it:

• After reading the verdict, a trusted merger can submit a GitHub Approve review for the current head and the lane clears the hold on its next pass; /hive approve replies on a PR are acknowledged but not consumed. An ordinary contributor cannot release their own PR.

_{Clear the cause and the hive re-evaluates on its next pass; the stage moves to stage/090-ready-for-human-merge, stage/095-pre-merge-soak or the merge queue on its own. Approval does not bypass CI, hold labels or other merge gates.}

[hivecommons-hive]

Needs a decision, not a merge:

• the reviewer swarm returned requires_human @b7acb94
• reviewer question: @clubanderson should decide whether this PKCE change lands on v5 as-is, given 🐛 fix(hub): send the GitHub token exchange as a form POST and document the login token's consumers #14683 (v6, already approved) rewrites the same exchangeGitHubLogin hunk as a form POST and also sends redirect_uri, so the v5→v6 forward-merge of this PR will conflict there.
• the automerge lane will not merge this unattended

Head: verdict requires_human @b7acb94, 54/54 checks green, DCO yes, soak 8m0s of 30m0s.

How to release it:

• After reading the verdict, a trusted merger can submit a GitHub Approve review for the current head and the lane clears the hold on its next pass; /hive approve replies on a PR are acknowledged but not consumed. An ordinary contributor cannot release their own PR.

_{Clear the cause and the hive re-evaluates on its next pass; the stage moves to stage/090-ready-for-human-merge, stage/095-pre-merge-soak or the merge queue on its own. Approval does not bypass CI, hold labels or other merge gates.}

[clubanderson]

/hive decision approve

Decision: retarget this PR to v6 and rebase it onto the merged #14683 — do not land it on v5.

Rationale: issue #14633 explicitly says Target branch: v6 and "depends on the form-POST token exchange child" — that child is #14683, which merged to v6 today and is not on v5. Landing here on v5 would re-implement #14683's form-POST rewrite in a diverging shape (no redirect_uri), guarantee a conflict on exchangeGitHubLogin at the v5→v6 forward-merge, and leave whoever resolves it to remember the superset by hand. Rebasing onto #14683 makes this PR exactly what the issue scoped: additive PKCE on top of an existing form body. The PKCE implementation itself is correct and I'm happy with it (32-byte crypto/rand verifier → 43-char base64url, S256, RFC 7636 test vector, HttpOnly+Secure+Lax cookie mirroring the state cookie, read-and-clear on every callback outcome, fail-closed 400 before any exchange or session, state nonce untouched, OIDC untouched, tests assert zero token-endpoint hits on rejection).

Instructions for the fix agent:

1. Change the PR base from v5 to v6 and rebase feat/hub-pkce-s256 onto current v6 (which includes 🐛 fix(hub): send the GitHub token exchange as a form POST and document the login token's consumers #14683). Keep the same DCO sign-off on every commit.
2. In exchangeGitHubLogin, keep 🐛 fix(hub): send the GitHub token exchange as a form POST and document the login token's consumers #14683's body as-is (including form.Set("redirect_uri", oauthRedirectURI())) and only add the codeVerifier parameter and form.Set("code_verifier", codeVerifier). Do not drop redirect_uri, and don't re-write the comment 🐛 fix(hub): send the GitHub token exchange as a form POST and document the login token's consumers #14683 added that points at src/docs/hub-login-token.md.
3. Add a test assertion (in oauth_pkce_test.go's fake token endpoint) that the exchange body contains both redirect_uri and code_verifier, so the two changes can't be silently un-merged again.
4. Fill in the PR body: one paragraph of rationale plus Closes #14633 so the issue closes on merge (the changelog reference alone does not).
5. Soften the changelog fragment and the authorize-URL comment: say PKCE "binds the authorization code to the browser that started the login (defence-in-depth)" rather than "cannot be redeemed by anyone without this browser's cookie" — we have not confirmed GitHub enforces code_verifier for this app type, and we shouldn't overstate it.
6. Re-check src/docs/api-reference.md line citations after the rebase (they were re-anchored against v5 and will shift on v6), and re-run the Sign-off survives the squash check so it is green rather than cancelled.

Once those are applied on v6, I'm fine with this merging through the normal lane.

[hivecommons-hive]

Needs a decision, not a merge:

• the reviewer swarm returned requires_human @2b44f90
• reviewer question: @clubanderson should confirm the v6 retarget satisfies the change request on b7acb94 so this fork-authored change to the Hub auth path can proceed.
• the automerge lane will not merge this unattended

Head: verdict requires_human @2b44f90, 53/53 checks green, DCO yes, soak 0s of 30m0s.

How to release it:

• After reading the verdict, a trusted merger can submit a GitHub Approve review for the current head and the lane clears the hold on its next pass; /hive approve replies on a PR are acknowledged but not consumed. An ordinary contributor cannot release their own PR.

_{Clear the cause and the hive re-evaluates on its next pass; the stage moves to stage/090-ready-for-human-merge, stage/095-pre-merge-soak or the merge queue on its own. Approval does not bypass CI, hold labels or other merge gates.}

[hivecommons-hive]

@Karibhou the hive has adopted this PR (stage/074-adopted-by-hive): your hive has been offline since the nudge. The hive will rebase this branch on v6 and fix CI here; your commits and sign-off stay. Push a new head yourself at any time and the hive yields it back to you.

[hivecommons-hive]

Needs a decision, not a merge:

• the reviewer swarm returned requires_human @2b44f90
• reviewer question: @clubanderson should confirm the v6 retarget satisfies the change request on b7acb94 so this fork-authored change to the Hub auth path can proceed.

Head: verdict requires_human @2b44f90, 53/53 checks green, DCO yes, soak 30m0s elapsed.

How to release it:

• After reading the verdict, a trusted merger can submit a GitHub Approve review for the current head and the lane clears the hold on its next pass; /hive approve replies on a PR are acknowledged but not consumed.

_{Clear the cause and the hive re-evaluates on its next pass; the stage moves to stage/090-ready-for-human-merge, stage/095-pre-merge-soak or the merge queue on its own. Approval does not bypass CI, hold labels or other merge gates.}

[hivecommons-hive]

The hive adopted this PR (stage/074-adopted-by-hive) but its head 2b44f9045a47 has not moved for 4h26m and the hive has nothing it can push: adoption-stalled:contributor-lane:changes-requested.

Blocker: lane reason contributor-lane:changes-requested on this head and nothing changed.

Moving to stage/085-needs-decision so a person sees it. This is not a merge-approval wait: the lane has not admitted this head.

[clubanderson]

Owner-delegate re-review (follow-up to my 03:33Z changes-requested).

Both asks are addressed at head 2b44f90: the PR is now based on v6 and layers code_verifier onto #14683's existing form-POST exchange (redirect_uri is untouched context, not re-implemented), so the v5→v6 forward-merge hazard is gone. PKCE implementation unchanged and still correct (S256, single-use HttpOnly cookie, fail-closed 400 before exchange). Hub browser login only — no spoke/relay surface touched.

Approving; previous changes-requested is superseded by this review.

[kubestellar-prow]

LGTM label has been added.

Details

Git tree hash: a534a485965edb0969246887a445549bbab708eb

[hivecommons-hive]

Evidence bundle `#147012b44f90` sha256:2f15190cffb4edf86af68ab167c0d75519518b9c5340f89ecb9e6c4654c139f1 — download from the dashboard or `hivectl review evidence #14701`.

[hivecommons-hive]

Merged by hive contributor-lane auto-merge: CI green for ≥30m, reviewer verdict not required, head 2b44f9045a47 adopted by the hive (stage/074-adopted-by-hive) with @Karibhou's commits — merged under the hive's own authority.