π fix(dashboard): send flow_id from the in-page GitHub sign-in overlay - #6217
Open
Danathar wants to merge 1 commit into
Open
π fix(dashboard): send flow_id from the in-page GitHub sign-in overlay#6217Danathar wants to merge 1 commit into
Danathar wants to merge 1 commit into
Conversation
hivecommons#5748 made `flow_id` mandatory on /api/gh-user-auth/poll β the binding that stops an anonymous poller racing the operator for a freshly approved session, since the cookie is minted on the poll response and both routes are public. It taught the standalone login page and `hivectl login` to send it and missed the overlay in static/index.html, which is a second, separate implementation of the same flow. So the overlay kept POSTing an empty body. Every poll was refused 400 before GitHub was contacted, and the login could never complete: GitHub reported the device activated while the dashboard sat on "Waiting for authorization..." indefinitely. It failed SILENTLY rather than erroring because jsonError writes {"ok":false,"error":"..."} with no `status` field. pollGHAuth branched only on complete / slow_down / error, so the 400 body matched none of them, the function returned without touching the timer, and setInterval fired again β an eternal spinner instead of a message. The standalone login page already guards this ("Any other shape is terminal"); the overlay did not. Why it survived a release: authenticate() serves the login page only for an UNTRUSTED non-/api request. An open hive, or one already carrying HIVE_DASHBOARD_TOKEN, gets index.html, where the overlay is the only sign-in path. The two implementations are reached by disjoint sets of users, so the login page's tests stayed green throughout. - startGHLogin() keeps data.flow_id in ghFlowID, and the poll sends {"flow_id": ghFlowID} with a JSON content type β the same contract as the login page. Cleared on success, on a terminal failure, and on cancel, so an abandoned flow's secret is not carried into the next thing the operator does. - A terminal branch: !resp.ok or an `error` with no recognised `status` now stops the timer and paints the message, via a shared failGHAuth() the server's own error path also routes through so the two cannot drift. Tests. The regression guard asserts the PAGE carries the id β server-side tests cannot catch this direction, because there the browser is the thing being correctly rejected (TestCovDF_GHUserAuthPoll_MissingFlowIDRejected even names "a pre-binding client" as the caller it turns away). All four client assertions fail against upstream/v4 and pass here. One more test closes the loop end to end: it starts a flow through the public handler and polls with the literal `{"flow_id":"..."}` bytes JSON.stringify now produces, as an anonymous browser β every other poll test marshals a Go map, so none of them pinned the wire shape the page actually emits, and hivecommons#6216 was exactly a wire-shape mismatch. Deliberately not touched: startCopilotLogin and the Claude login flow are the same shape of unbound public poll, but they have no flow_id binding server-side at all, so they are unaffected by this bug. Binding them is hardening, not this regression, and belongs in its own change. Closes hivecommons#6216 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UQTim25GU8Yk2HrCh39i1u Signed-off-by: Douglas Baggett <doug.baggett@gmail.com>
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
Sign in with GitHub on the dashboard works again.
You clicked it, pasted the code at
github.com/login/device, GitHub said thedevice was activated β and the overlay sat on
Waiting for authorization...forever. No error, no session, just a spinner.
Every one of those polls was being refused with
400. #5748 started requiring aflow_idon/api/gh-user-auth/polland taught the standalone login page andhivectl loginto send it, but the dashboard's own in-page overlay is asecond implementation of the same flow and was missed. It kept posting an
empty body, so it could never complete.
This makes the overlay send the
flow_id, and stop with a visible error when itgets an answer it does not understand.
Closes #6216.
Why it broke, and why nobody noticed
Two separate things went wrong, and the second is what turned a bug into a
mystery.
It was rejected.
startGHLogin()readuser_code,verification_uriandintervalout of the/startresponse and droppedflow_idon the floor.pollGHAuth()then polled with no body at all:The server compares the presented
flow_idagainst the one it minted andanswers
400before contacting GitHub or setting any cookie. That binding isdeliberate and load-bearing β the session cookie is minted on the poll
response and both routes are public, so a poll that cannot prove it started the
flow must get nothing, or an anonymous poller could race the operator for a
freshly approved session (#5742). The overlay simply never presented it.
It was silent.
jsonErrorwrites{"ok":false,"error":"β¦"}β nostatusfield.
pollGHAuthbranched only oncomplete,slow_downanderror, so the400 body matched none of them, the function returned without touching the timer,
and
setIntervalfired again. An eternal spinner instead of a message.It survived a release because the two implementations are reached by
disjoint sets of users.
authenticate()serves the standalone login page onlyfor an untrusted non-
/apirequest; an open hive, or one already carryingHIVE_DASHBOARD_TOKEN, getsindex.html, where the overlay is the only sign-inpath available. The login page β which threads
flow_idcorrectly β was neverbroken, and its tests stayed green from the 2026-09-02 merge through v4.18.1.
The fix
startGHLogin()keepsdata.flow_idin a module-scopedghFlowID, andpollGHAuth()sends{"flow_id": ghFlowID}with a JSON content type β thesame contract the login page already has. Cleared on success, on a terminal
failure and on cancel, so an abandoned flow's secret is not carried into
whatever the operator does next.
!resp.ok, or anerrorwith no recognisedstatus, nowstops the timer and paints the message. Both that path and the server's own
{"status":"error"}route through one sharedfailGHAuth(), so they cannotdrift apart. An explicit
pendingbranch documents the steady state thatshould keep polling, so the fallback can never swallow it.
The global
window.fetchwrapper mutatesoptsin place to add anAuthorizationheader and passes the same object through, so the new body andContent-Typesurvive it β including its one 401/403 retry, which shallow-copiesopts.Testing
The interesting part of this fix is the regression guard, because the
server-side tests could not have caught it. There, the browser is the thing
being correctly rejected β
TestCovDF_GHUserAuthPoll_MissingFlowIDRejectedevennames "a pre-binding client" as the caller it turns away, and that caller was
this overlay. Only an assertion on the client catches this direction.
Four client assertions, scoped to the relevant function bodies rather than the
whole file:
TestGHLoginOverlayPollSendsFlowIDβ the poll carriesflow_id, and carriesghFlowIDrather than some constant.TestGHLoginOverlayCapturesFlowIDFromStartβstartGHLogin()actually storesdata.flow_id.TestGHLoginOverlayStopsOnUnrecognizedResponseβ the poll inspectsresp.okand routes terminal answers through
failGHAuth.TestGHLoginOverlayFailHelperClearsTimerAndShowsMessageβ that helper reallydoes clear the interval and write to the overlay.
All four fail against a pristine
upstream/v4and pass on this branch β Ireplayed each assertion's exact slice bounds and predicates against the
unmodified file to confirm the guard would have caught #5748's miss rather than
merely describing it.
One more test closes the loop end to end.
TestGHUserAuthPollAcceptsBrowserBodyShapestarts a flow through the public handler and polls with the literal bytes
JSON.stringify({flow_id: ghFlowID})now produces, as an anonymous browser withno role headers. Every other poll test marshals a Go map, so none of them pinned
the wire shape the page actually emits β and #6216 was precisely a wire-shape
mismatch that survived because the client and server were only ever tested apart.
pkg/dashboardpasses in full, andgolangci-lintreports 0 issues for it.One flaky test to declare:
TestLeaseRestart_ResumeSurvivesHubRestart(contributeleases, untouched by this change) fails intermittently. I measured it β 2
failures in 10 runs on a clean
upstream/v4worktree, 1 in 10 on this branchβ so it is a pre-existing flake, not a regression here. Worth its own issue.
Deliberately not in scope
The issue notes that
startCopilotLoginand the Claude login flow are the sameshape of unbound public poll. They have no
flow_idbinding server-side atall, so they are unaffected by this bug β binding them is new hardening, not
this regression, and changing those auth paths under a bugfix PR would be the
wrong place to review it.
β hive: backend=claude model=claude-opus-5