fix(auth): bound GitHub OAuth verification phases - #2482
Conversation
Share request deadlines, API headers, and transient errors across membership and OAuth. Give code exchange its own bounded attempt without automatic retry, and bound each post-exchange identity/email/membership verification attempt while preserving encrypted credential reuse and the existing retry. Prevent late responses from sealing credentials or completing timed-out callbacks.
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 22, 2026, 7:08 AM ET / 11:08 UTC. ClawSweeper reviewWhat this changesThe PR gives GitHub OAuth exchange and verification bounded 15-second phases using a shared request helper, with regression coverage and operator documentation. Merge readiness✅ Ready for maintainer review Keep open: this fixes an OAuth timeout gap still present on main and in v0.64.0. No actionable patch defect was found, and the supplied native HTTP trace supports the changed timeout behavior. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s coordinator exchanges GitHub login codes and verifies identity, email, and membership before issuing CLI or portal sessions. These changes bound upstream waits while preserving credential custody and callback retries. flowchart TD
A[CLI or portal login callback] --> B[Validate callback binding]
B --> C[Exchange code within 15 seconds]
C --> D[Retain encrypted credential]
D --> E[Verify identity and membership within 15 seconds]
E --> F[Issue session]
E --> G[Retry verification once]
G --> E
C --> H[Return temporary failure on timeout]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Use one shared deadline owner for bounded OAuth phases while retaining the existing authorization checks, encrypted credentials, and retry semantics. Do we have a high-confidence way to reproduce the issue? Yes, from source: a GitHub exchange or identity response that never completes leaves main awaiting an unbounded fetch or JSON body. This review did not execute a current-main reproduction. Is this the best way to solve the issue? Yes. Reusing the existing deadline mechanism closes the uncovered OAuth phases without introducing a competing retry policy or changing stored credentials. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 52ae9994e14c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Landed as e423497 through the normal protected squash path. The fix shares the GitHub deadline owner across OAuth phases while preserving the one-use-code exchange boundary, existing verification retry, encrypted credential reuse, and late-response fencing. The actual merged checkout passed 63 focused tests with one skip, and coordinator deployment succeeded. The original main CI run encountered a separate GCP HTTPS fixture failure, not a Worker failure. That fixture correction is now landed via #2484. Superseding main CI containing the unchanged OAuth fix succeeded at https://github.com/openclaw/crabbox/actions/runs/35724738623; deployment of that tree succeeded at https://github.com/openclaw/crabbox/actions/runs/35724738659. The real-clock OAuth proof uses native loopback HTTP and synthetic credentials, not live GitHub. |
1 participant
Summary
GitHub OAuth code exchange and identity/email lookups still awaited response headers and JSON bodies without a deadline. Bound the exchange to one 15-second attempt, and give each existing post-exchange verification attempt one shared 15-second budget across identity, verified-email, organization, and team checks.
Move the membership deadline, canonical GitHub API headers, and transient-error type into one shared request owner. OAuth retains its retry and credential-custody rules: code exchange is never automatically retried; post-exchange verification still retries once; an already exchanged credential stays encrypted for later callback retries. Late responses cannot store credentials or finish a timed-out callback. An exchange timeout leaves GitHub's remote outcome unknown and may require starting a new login. No persisted schema, authorization policy, credential configuration, or permission is added.
Verification
The stalled-exchange regression failed against unchanged production code because the callback remained pending after its intended deadline. Added coverage includes stalled exchange headers and successful/error bodies, absence of automatic exchange retries and late credential writes, a shared post-exchange budget, and recovery using the already encrypted credential.
Observed real-clock HTTP proof
The existing native HTTP OAuth scenario also passed with its test clock controls replaced by actual waits; production deadline logic was unchanged. Passive observation recorded one loopback POST, HTTP 200 headers with an incomplete JSON body, abort after approximately 15 real seconds, and response closure before explicit test cleanup. The passing test asserts callback status 503, one exchange request, and no credential, issued token, or active callback claim in the pending record.
The recorded production/test source hashes were verified identical to commit
c49820ca80b00b14793df3ed486775817d74ae4aafter committing. The trace run itself occurred before that commit. Final head3be32181ad022f6a5d40cca70e70331031be98d4changes only the changelog link; all recorded runtime/test hashes also match that final head.elapsedMSis real monotonic time; the two abort observations do not represent additional requests.[ { "event": "server_received_exchange", "elapsedMS": 134.264417, "method": "POST", "loopback": true }, { "event": "server_sent_headers", "elapsedMS": 134.63175, "status": 200 }, { "event": "deadline_aborted_signal", "elapsedMS": 15132.930792, "aborted": true }, { "event": "deadline_aborted_signal", "elapsedMS": 15132.98275, "aborted": true }, { "event": "server_response_closed", "elapsedMS": 15134.011, "bodyEnded": false } ]An independent read-only evidence audit confirmed the trace and assertion ordering: 1 passed, 15 skipped, exit 0. This uses native loopback HTTP, synthetic credentials, and in-memory coordinator storage—not live GitHub or a production database. No credentials, request bodies, headers, host addresses, or unrelated traffic were retained in the trace.