feat(e2e): add TOTP support for 2FA-enabled GitHub accounts - #1211
Conversation
Site previewPreview: https://d9bf420a-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsHigh
Low
Previous runReviewFindingsHigh
Low
Previous run (2)ReviewFindingsHigh
Previous run (3)ReviewFindingsHigh
Low
Previous run (4)ReviewFindingsHigh
Low
Previous run (5)ReviewFindingsHigh
Low
Previous run (6)ReviewFindingsHigh
Medium
Low
Previous run (7)ReviewFindingsHigh
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad Report — 10 agents (3x claude-coder, 3x claude-researcher, 2x gemini-code-review, 2x cursor-code-review)
Well-architected PR with clean code, good security practices (TOTP secret never logged, masked in CI, internal/ package), and solid documentation (ADR 0039 properly cross-linked). Two HIGH findings need attention before merge; five MEDIUM findings are worth addressing.
Findings: 2 HIGH, 5 MEDIUM (after dedup from 80+ raw findings, 4 false positives removed)
| Severity | Finding | Consensus |
|---|---|---|
| HIGH | TOTP retry reuses same code ~93% of the time | 8/10 agents |
| HIGH | Sudo TOTP fallback unreachable when password field visible but empty | 7/10 agents |
| MEDIUM | TOTP sudo path skips page departure verification | 2/10 agents |
| MEDIUM | NetworkIdle wait is fragile (Playwright discourages it) |
7/10 agents |
| MEDIUM | GenerateCode TOCTOU gap between boundary sleep and code generation |
5/10 agents |
| MEDIUM | No unit tests for EnterTOTPCode (most complex function) |
6/10 agents |
| MEDIUM | waitForPageToLeave busy-polls instead of Playwright waits |
6/10 agents |
See inline comments for details and suggested fixes.
|
Addressed review findings from the latest review comment: High (protected-path): Human-approved — this is the PR author authorizing the workflow change. Low (docs-currency, ADR 0010:90): Updated Low (docs-currency, ADR 0010:134): Updated "Two repo secrets" to mention the optional third secret ( E2E fix: The sudo confirmation page does not auto-submit TOTP codes like the 2FA login page. Added explicit submit button click after typing the TOTP code. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
All 7 findings from the 10-agent review squad have been addressed:
Fixed (6/7):
- HIGH TOTP retry now waits for next period boundary instead of fixed 2s sleep
- HIGH Sudo handler now falls back to TOTP when password field is visible but empty
- MEDIUM Both TOTP paths now call
waitForPageToLeavefor departure verification - MEDIUM
NetworkIdlereplaced withDOMContentLoaded - MEDIUM
GenerateCodecapturestime.Now()once to eliminate TOCTOU gap - MEDIUM
waitForPageToLeaverewritten withpage.WaitForFunction(event-driven)
Acknowledged (1/7):
- MEDIUM
EnterTOTPCodeunit tests deferred — reasonable given the mocking cost for e2e support code validated by integration flow
The e2e test account now has 2FA enabled. Add TOTP code generation so that both export-session (local login) and the e2e test suite (sudo confirmation) can handle TOTP challenges automatically. - Add e2e/internal/otp package wrapping pquerna/otp for shared TOTP code generation with unit tests - Add handleTOTPIfPresent() in login.go for Playwright TOTP form interaction, used by both sudo handling and export-session - Update handleSudoIfPresent() to try TOTP when password field is not visible (2FA accounts show TOTP on sudo pages) - Update export-session to detect and complete 2FA after login - Pass E2E_GITHUB_TOTP_SECRET through CI workflow, envConfig, and all call sites Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Verify TOTP submission succeeded by checking URL/title after
navigation (handleTOTPIfPresent returned true without confirming
the page actually left the TOTP screen)
- Add debug screenshots on TOTP failure paths (screenshotDir param
was accepted but unused)
- Fix WaitForURL glob in export-session (exact match could fail if
GitHub redirects to /dashboard instead of /)
- Fix flaky TestGenerateCodeDeterministic (sleep could cross a 30s
TOTP window boundary; now handles the boundary case)
- Improve error message when password field is visible but
E2E_GITHUB_PASSWORD is not set
- Fix stale comment ("Use Type" -> "Use PressSequentially")
Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
- Extract TOTP page interaction into otp.EnterTOTPCode shared helper, eliminating divergent duplicate logic between login.go and export-session/main.go - Add period-boundary avoidance in GenerateCode to prevent generating codes that expire during the ~300ms typing delay - Add single retry on TOTP rejection to handle clock skew between CI runners and GitHub servers - Fix waitForPageToLeave to poll page title instead of relying on stale DOMContentLoaded state, and propagate Title() errors instead of treating them as success Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add the new E2E_GITHUB_TOTP_SECRET env var to the "Running e2e tests" section of CLAUDE.md so contributors with 2FA-enabled test accounts can discover it. Write ADR 0039 documenting the TOTP automation decision and link it from ADR 0010. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- TOTP fallback when password field visible but empty: try TOTP via toggle before erroring - TOTP retry waits for next 30s period boundary instead of fixed 2s - Add waitForPageToLeave after TOTP succeeds on sudo page - Replace NetworkIdle with DOMContentLoaded in EnterTOTPCode - Fix TOCTOU gap: capture time.Now() once after boundary sleep - Replace busy-poll waitForPageToLeave with WaitForFunction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The sudo confirmation page does not auto-submit TOTP codes like the 2FA login page does. After typing the code, explicitly click the submit button if one is visible. Also updates ADR 0010 to reflect that handleSudoIfPresent now handles TOTP and that a third secret (E2E_GITHUB_TOTP_SECRET) is needed when the test account has 2FA enabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GitHub now redirects /installations/new to /installations/select_target where the user picks a target org. Add handleSelectTargetPage to click the test org and proceed to the install page. Also reverts the GenerateCode doc comment back to "4 seconds" — the guard rem >= 26 covers seconds 26-29 (4-second detection window). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a4d4b55 to
87ad0d0
Compare
The password path in handleSudoIfPresent now falls back to TOTP if the password submission doesn't navigate away from the sudo page. Also moves the confirm-button click into the password branch, removing previously unreachable dead code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Final fix to be done in #1253. |
3 participants
Summary
e2e/internal/otppackage for shared TOTP code generation (wrapspquerna/otp)handleTOTPIfPresent()handles TOTP forms on both sudo and 2FA pages via Playwrightexport-sessiondetects and completes 2FA after login (PressSequentiallytriggers GitHub's auto-submit)E2E_GITHUB_TOTP_SECRETwired through CI workflow,envConfig, and all call siteshandleSelectTargetPage()added to handle GitHub's/installations/select_targetredirect during app installation — discovered during 2FA CI testing when GitHub changed the installation flow to require org selectionmake e2e-export-sessionandmake e2e-upload-sessionboth succeed with 2FAWhy stored sessions are still needed
GitHub blocks password-based login from Actions runner IPs (Azure datacenter ranges). TOTP doesn't change this — the block happens at the
/loginform before reaching the 2FA prompt. The stored session bypasses login entirely. TOTP is needed for:make e2e-export-session(local login with 2FA to generate the session)Test plan
make e2e-export-sessionsucceeds with 2FAmake e2e-upload-sessionsucceeds (session uploaded to repo secret)go test ./e2e/internal/otp/ -v— all unit tests passgo vet ./e2e/...— clean-tags e2eE2E_GITHUB_TOTP_SECRETsecret (secret already set in repo)🤖 Generated with Claude Code
Blocked by #1253