[WIP] many usability fixes (needs untangling) - #165
Closed
aspiers wants to merge 37 commits into
Closed
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 473f6a0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects when an upstream Pushed Authorization Request (PAR) is no longer recoverable and updates OTP UI/flow: client heartbeat and visibility checks reconcile standalone “Resend code” versus an imperatively inserted “Start over”; centralizes OTP verify error messages; tightens OTP input sanitization; adds tests and changeset notes. ChangesPAR-Death UI Recovery
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant AuthService
participant DemoApp
User->>Browser: open OTP page
Browser->>AuthService: GET /auth/ping (pingHeartbeat)
AuthService-->>Browser: 200 or network error / status mapping
Browser->>Browser: evaluate parLikelyDead() -> refreshResendVisibility()
Browser-->>User: show Start over OR show Resend
DemoApp->>Browser: callback redirect (/?error=session_expired or auth_failed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the ePDS-pr-165 environment in ePDS
|
Coverage Report for CI Build 25494650133Coverage increased (+0.2%) to 55.72%Details
Uncovered Changes
Coverage Regressions7 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.changeset/hide-resend-when-sign-in-cannot-recover.md (1)
1-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the required
Affects:line and avoid jargon for End users.Two coding-guideline violations on this changeset:
- Missing
Affects:line. Every changeset must include an**Affects:**line listing audiences in the order End users, Client app developers, Operators.- End-user jargon. Once End users is an audience, the summary must avoid OTP/DID/PAR/OAuth jargon. "OTP screen" should be reworded for end users (the second sentence already does it well by talking about codes and emails).
📝 Proposed rewrite
--- '@certified-app/auth-service': patch --- -Hide the "Resend code" button on the OTP screen when the sign-in can no longer be recovered, and offer "Start over" instead. Previously, a user who left the OTP screen open long enough for the underlying sign-in window to lapse could click "Resend code", receive a fresh email, type the new code, and only then see "Sign in failed" — wasting their time on a code that could not have worked. The screen now only ever offers actions that can actually complete the sign-in. +**Affects:** End users + +Hide the "Resend code" button on the code-entry screen when the sign-in can no longer be recovered, and offer "Start over" instead. Previously, a user who left this screen open long enough for the sign-in to lapse could click "Resend code", receive a fresh email, type the new code, and only then see "Sign in failed" — wasting their time on a code that could not have worked. The screen now only ever offers actions that can actually complete the sign-in.As per coding guidelines: "Every changeset must include an Affects: line listing audiences (End users, Client app developers, Operators in that order)" and "Write changeset summaries in plain language without OTP/DID/PAR/OAuth jargon when End users is an audience".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/hide-resend-when-sign-in-cannot-recover.md around lines 1 - 6, The changeset is missing the required "Affects:" line listing the audiences in the order End users, Client app developers, Operators. Add this line to comply with the guideline. Also, rewrite the summary text to remove jargon like "OTP screen" to make it clear for End users, using simple terms like "verification code screen" or "code entry screen" instead.packages/auth-service/src/routes/login-page.ts (1)
775-793:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTwo "Start over" buttons render simultaneously when
/auth/pingreturnspar_expired.When
pingHeartbeat()resolves to{ ok: false, reason: 'par_expired' }:
- The
.then()branch (lines 777–784) callsshowFlowAbortedNotice(), which creates a button with classflash-actioninside the error banner.- The
.finally()branch (line 792) callsrefreshResendVisibility(). BecauseflowAbortedis now true,parLikelyDead()returns true and a second button (#btn-start-over, classbtn-secondary) is inserted before the resend button.Both buttons appear on the page simultaneously, though in different locations. The user sees two visually distinct "Start over" controls — one styled as inline text within the error notice, another as a secondary button in the form actions area.
To fix: Have
showFlowAbortedNotice()set a flag thatrefreshResendVisibility()checks — if the abort notice has already fired, skip inserting the#btn-start-overbutton since one is already present and the form is disabled. Alternatively, haveshowFlowAbortedNotice()not render its own button and rely solely onrefreshResendVisibility()for the "Start over" surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth-service/src/routes/login-page.ts` around lines 775 - 793, When showFlowAbortedNotice() renders the abort banner, set a persistent flag (e.g., flowAbortedShown = true) that indicates the abort notice has already provided a "Start over" control; then update refreshResendVisibility() (and any helpers it uses such as parLikelyDead()) to check that flag and skip inserting the auxiliary `#btn-start-over` button when flowAbortedShown is true so only one "Start over" control is ever rendered; ensure the flag is cleared where the flow is truly restarted so normal behavior resumes.
🧹 Nitpick comments (1)
packages/auth-service/src/routes/login-page.ts (1)
869-892: 💤 Low valuePrefer a CSS class over inline
style.displayfor the Resend toggle.This is runtime visibility (not "dynamic values set at render time"), so per the repo guideline it should be driven by a class such as
.hidden. The existingdisplay: nonedeclarations on.step-otp/.step-email.hiddenalready follow that convention; this new toggle could match.♻️ Sketch of class-based toggle
function refreshResendVisibility() { var resendBtn = document.getElementById('btn-resend'); var startOverLink = document.getElementById('btn-start-over'); if (!resendBtn) return; if (parLikelyDead()) { - resendBtn.style.display = 'none'; + resendBtn.classList.add('hidden'); if (!startOverLink) { ... } } else { - resendBtn.style.display = ''; + resendBtn.classList.remove('hidden'); if (startOverLink && startOverLink.parentNode) { startOverLink.parentNode.removeChild(startOverLink); } } }…with a generic
.hidden { display: none; }rule alongside the existing.step-email.hidden. (Pre-existingerrorEl.style.displayusage inshowFlashis out of scope here.)As per coding guidelines: "Use CSS classes to control visibility (hidden, active) rather than inline display style except for dynamic values set at render time".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth-service/src/routes/login-page.ts` around lines 869 - 892, The refreshResendVisibility function currently toggles visibility via resendBtn.style.display and should instead add/remove a CSS visibility class; update the logic in refreshResendVisibility to use resendBtn.classList.add('hidden') / .remove('hidden') (and mirror removal for startOverLink via classList or insert/remove DOM as before) rather than setting style.display, ensure the created startOverLink does not rely on inline display and that the code references the existing .hidden CSS class (keep element IDs btn-resend and btn-start-over and the same click handler).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.changeset/hide-resend-when-sign-in-cannot-recover.md:
- Around line 1-6: The changeset is missing the required "Affects:" line listing
the audiences in the order End users, Client app developers, Operators. Add this
line to comply with the guideline. Also, rewrite the summary text to remove
jargon like "OTP screen" to make it clear for End users, using simple terms like
"verification code screen" or "code entry screen" instead.
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 775-793: When showFlowAbortedNotice() renders the abort banner,
set a persistent flag (e.g., flowAbortedShown = true) that indicates the abort
notice has already provided a "Start over" control; then update
refreshResendVisibility() (and any helpers it uses such as parLikelyDead()) to
check that flag and skip inserting the auxiliary `#btn-start-over` button when
flowAbortedShown is true so only one "Start over" control is ever rendered;
ensure the flag is cleared where the flow is truly restarted so normal behavior
resumes.
---
Nitpick comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 869-892: The refreshResendVisibility function currently toggles
visibility via resendBtn.style.display and should instead add/remove a CSS
visibility class; update the logic in refreshResendVisibility to use
resendBtn.classList.add('hidden') / .remove('hidden') (and mirror removal for
startOverLink via classList or insert/remove DOM as before) rather than setting
style.display, ensure the created startOverLink does not rely on inline display
and that the code references the existing .hidden CSS class (keep element IDs
btn-resend and btn-start-over and the same click handler).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2295e746-1f09-4076-90ad-8f0dc32c74e0
📒 Files selected for processing (4)
.changeset/hide-resend-when-sign-in-cannot-recover.mde2e/step-definitions/auth.steps.tsfeatures/passwordless-authentication.featurepackages/auth-service/src/routes/login-page.ts
There was a problem hiding this comment.
Pull request overview
This PR updates the auth-service login OTP UI to proactively stop offering “Resend code” once the underlying OAuth/PAR flow can no longer complete, replacing it with a “Start over” action, and adds an e2e scenario to assert the behavior.
Changes:
- Track the last successful heartbeat timestamp and derive
parLikelyDead()to decide when the flow is no longer recoverable. - Toggle OTP-screen actions via
refreshResendVisibility()(hide Resend code, show Start over) and re-check on heartbeat ticks andvisibilitychange. - Add an e2e scenario + step definitions asserting Resend is hidden and Start over is shown when the PAR is expired.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/auth-service/src/routes/login-page.ts | Adds PAR-liveness tracking and UI toggling to hide Resend when the flow is no longer recoverable. |
| features/passwordless-authentication.feature | Adds a new scenario asserting Resend is not offered when PAR is dead. |
| e2e/step-definitions/auth.steps.ts | Adds steps to trigger the UI re-check and assert Resend hidden / Start over visible. |
| .changeset/hide-resend-when-sign-in-cannot-recover.md | Patch changeset documenting the UX fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+767
to
+770
| function parLikelyDead() { | ||
| if (flowAborted) return true; | ||
| return Date.now() - lastSuccessfulHeartbeatAt >= parInactivityTimeoutMs; | ||
| } |
Comment on lines
+873
to
+885
| if (parLikelyDead()) { | ||
| resendBtn.style.display = 'none'; | ||
| if (!startOverLink) { | ||
| startOverLink = document.createElement('button'); | ||
| startOverLink.type = 'button'; | ||
| startOverLink.id = 'btn-start-over'; | ||
| startOverLink.className = 'btn-secondary'; | ||
| startOverLink.textContent = 'Start over'; | ||
| startOverLink.addEventListener('click', function() { | ||
| window.location.href = '/auth/abort'; | ||
| }); | ||
| resendBtn.parentNode.insertBefore(startOverLink, resendBtn); | ||
| } |
Previously the OTP screen always offered "Resend code", even when the upstream PAR row had silently lapsed (suspended tab, mobile background, heartbeat throttling). The user could click Resend, receive a fresh email, type the new code, and only then see "Sign in failed" — wasting their time on a code that could not have worked. The screen now never surfaces actions that cannot complete the flow: - Track lastSuccessfulHeartbeatAt; treat the PAR as dead once we cross upstream's 5 min AUTHORIZATION_INACTIVITY_TIMEOUT without a fresh ok ping (the upstream death point is exact — no margin needed). - Hide #btn-resend and surface a #btn-start-over (→ /auth/abort) the moment parLikelyDead() flips. Reconciled on every heartbeat tick (including transient ticks, so a stale-by-time case still hides the button) and on the visibilitychange event (so a backgrounded tab returning to focus reflects reality immediately). - Inline "Send a new code" action on the OTP-expired error now branches: parLikelyDead() → "Start over"; otherwise existing "Send a new code". This is the proactive UI complement to the existing reactive abort gate. Server-side enforcement of the same invariant on /email-otp/send- verification-otp and /sign-in/email-otp is a separate follow-up. Test: new @resend-hidden-when-par-dead scenario; full @otp-and-par-expiry / @par-heartbeat / @resend-after-par-dead / @otp-expiry suite still passes (7 scenarios, 78 steps). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
fix/otp-expiry-resend-bug
branch
from
May 7, 2026 03:51
d0a5ecf to
ccf4ece
Compare
…doesn't
Two changes that close the bug-report hole on the demo client:
1. Bump oauth_state cookie maxAge from 600s to 3600s, matching
auth-service's auth_flow row TTL. The demo's cookie carries the
state, code verifier, token endpoint and issuer for the OAuth
callback to complete; if it expires before the user submits the
OTP, the callback can't find any of that and bounces silently.
600s was shorter than realistic OTP-form sit times (the bug
report was an 11-minute wait). Aligning with auth_flow's 60-min
budget means as long as the auth-service can still recover the
flow, the demo can too.
2. Distinguish "cookie missing" from "auth failed" on the callback.
Previously every silent-fail path bounced to /?error=auth_failed
("Authentication failed. Please try again."), which is misleading
when the sign-in itself succeeded — the user typed a fresh OTP
correctly, the auth-service issued the OAuth code, the demo just
couldn't finish because its own session cookie had aged out. Now
the cookie-missing branch redirects to /?error=session_expired
("Your sign-in took too long to finish. Please sign in again.")
so the user understands what happened without thinking they
typed the code wrong.
Test: new @demo-cookie-expiry @bug-report scenario clears the cookie
mid-flow and asserts the session-expired error surfaces.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uses it The OTP form's "Use different email" button took the user back to the email-entry form but left the previous email pre-filled. That's exactly the misleading "looks like the form remembered me" UX they were trying to escape — and forces an extra clearing keystroke before they can type the new address. Fix: showEmailStep() now resets emailInput.value, currentEmail, and focuses the field so the user can just start typing. Test: new @use-different-email scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+869
to
+886
| function refreshResendVisibility() { | ||
| var resendBtn = document.getElementById('btn-resend'); | ||
| var startOverLink = document.getElementById('btn-start-over'); | ||
| if (!resendBtn) return; | ||
| if (parLikelyDead()) { | ||
| resendBtn.style.display = 'none'; | ||
| if (!startOverLink) { | ||
| startOverLink = document.createElement('button'); | ||
| startOverLink.type = 'button'; | ||
| startOverLink.id = 'btn-start-over'; | ||
| startOverLink.className = 'btn-secondary'; | ||
| startOverLink.textContent = 'Start over'; | ||
| startOverLink.addEventListener('click', function() { | ||
| window.location.href = '/auth/abort'; | ||
| }); | ||
| resendBtn.parentNode.insertBefore(startOverLink, resendBtn); | ||
| } | ||
| } else { |
Comment on lines
262
to
268
| resp2.cookies.set(oauthCookie.name, oauthCookie.value, { | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: 'lax', | ||
| maxAge: 600, | ||
| maxAge: 60 * 60, | ||
| path: '/', | ||
| }) |
Comment on lines
279
to
286
| const response = NextResponse.redirect(authUrl) | ||
| response.cookies.set(oauthCookie.name, oauthCookie.value, { | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: 'lax', | ||
| maxAge: 600, | ||
| maxAge: 60 * 60, | ||
| path: '/', | ||
| }) |
Clicking Verify before typing the code (or pressing Enter on an empty form) flashed "Invalid OTP" — misleading (the user typed nothing, not an invalid code) AND burned a real /sign-in/email-otp call against better-auth's rate limiter, so a confused user tab-clicking Verify could lock themselves out faster than necessary. Fix: short-circuit the submit handler when otp.length is shorter than the number of OTP boxes; focus the first empty box instead. Test: new @verify-empty-otp scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After 5 wrong codes, better-auth's email-otp plugin throws
TOO_MANY_ATTEMPTS and deletes the verification row in the same
call. Subsequent attempts hit the deleted row and return INVALID_OTP
("Invalid OTP") — so the user, who has just been locked out, sees
a generic typo-shaped error and naturally tries again. Each retry
just confirms the same dead-end.
The only forward path is a fresh Resend. Make the page surface
that path inline next to the lockout error, the same way the
OTP-expired branch already does.
Implementation: widen the inline-action regex from /expir|too long/
to /expir|too long|too many|attempt/ so it matches better-auth's
TOO_MANY_ATTEMPTS message ("Too many attempts"). Rename the var
isExpired → isUnrecoverable to reflect the broader contract.
Test: new @too-many-attempts scenario burns 5 attempts via direct
fetch then triggers the 6th through the form, asserts both the
"Too many attempts" error AND the Send-a-new-code inline action.
Existing unit tests updated to reference the new var name + regex.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/pds-core/src/lib/reserved-handle.ts:1
- Using the
inoperator can produce false positives for keys inherited fromObject.prototype(e.g."toString","constructor","__proto__"), which would incorrectly flag some handles as reserved. Prefer an own-property check such asObject.hasOwn(reservedSubdomains, local.toLowerCase())(orObject.prototype.hasOwnProperty.call(...)) to avoid prototype-chain matches.
| document.addEventListener('visibilitychange', function() { | ||
| if (document.visibilityState === 'visible' && heartbeatEnabled) { | ||
| pingHeartbeat(); | ||
| refreshResendVisibility(); |
Comment on lines
+812
to
+814
| (url) => | ||
| url.origin === origin && | ||
| /^[?&](error=(auth_failed|session_expired))/.test(url.search), |
Comment on lines
+904
to
+912
| // `maxAge: 600` (see packages/demo/src/app/api/oauth/login/route.ts). | ||
| // If the user spends longer than 10 minutes between starting the OAuth | ||
| // flow and the callback firing — most realistic cause: dawdling on the | ||
| // OTP form, then clicking Resend after the 10-minute mark — the cookie | ||
| // expires before the callback runs, so the callback handler can't find | ||
| // the OAuth state and silently bounces to /?error=auth_failed. | ||
| // | ||
| // This step deletes the cookie programmatically so we can exercise the | ||
| // post-cookie-expiry callback path without a 10-minute wall-clock wait. |
This comment has been minimized.
This comment has been minimized.
…route
The /_internal/check-handle handler open-coded the OR-shape that
combines DB existence with reserved-subdomain match. Extract that
into a pure helper (handleIsUnavailable) on the existing
reserved-handle module so:
- The route handler stays thin (one call instead of three lines).
- Unit tests can cover the OR-shape directly without standing up
a PDS instance — including the corner cases (account exists +
not reserved, account missing + reserved, both, neither, case-
insensitive matching, empty fullHandle).
Six new unit tests on the helper. No behaviour change at the HTTP
boundary.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The shared renderError() helper produces every styled HTML fallback page (Sign-in session expired, internal failures, session not found, …). The error message <p> was a plain paragraph; screen-reader users got no announcement when these pages loaded. role="alert" tells assistive tech to announce immediately on page render. Matches the role=alert treatment we just gave the inline server-rendered error banners on account-login and recovery. One-character behaviour change; no test fixtures depend on the exact HTML. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+904
to
+912
| // `maxAge: 600` (see packages/demo/src/app/api/oauth/login/route.ts). | ||
| // If the user spends longer than 10 minutes between starting the OAuth | ||
| // flow and the callback firing — most realistic cause: dawdling on the | ||
| // OTP form, then clicking Resend after the 10-minute mark — the cookie | ||
| // expires before the callback runs, so the callback handler can't find | ||
| // the OAuth state and silently bounces to /?error=auth_failed. | ||
| // | ||
| // This step deletes the cookie programmatically so we can exercise the | ||
| // post-cookie-expiry callback path without a 10-minute wall-clock wait. |
| // flow and the callback firing — most realistic cause: dawdling on the | ||
| // OTP form, then clicking Resend after the 10-minute mark — the cookie | ||
| // expires before the callback runs, so the callback handler can't find | ||
| // the OAuth state and silently bounces to /?error=auth_failed. |
resolveClientMetadata returned `{ client_name: clientId }` when the
clientId failed to parse as a URL, or used a non-http(s) protocol.
The intent was probably "give the caller something to display" —
but the upshot was that a malformed query-string client_id ("Sign
in to not-a-url") or a schemeless one ("Sign in to javascript:…")
showed up verbatim in:
- the auth-service login page <title> tag
- the consent screen header
- the OTP form's "Sign in to <appName>" heading
— effectively letting the URL bar shape user-visible text.
Return `{}` from those branches instead, so the caller's
fallback chain (`client_name || extractDomain(clientId) ||
'an application'`) ends in the static "an application" string.
Tests: two existing assertions on the leaky behaviour are
inverted to assert the fallback path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…er every action Every POST handler in /account/* redirects back to /account with a ?success=<code> or ?error=<code> query param to confirm the action took effect. The GET handler was completely ignoring those query params — the user clicked "Add backup email" → server processed it → server redirected back → user landed on the same page with zero indication anything had happened. Wire the params through to renderSettingsPage and render a green flash banner on success, red on error. Whitelist the recognised codes via FLASH_SUCCESS_MESSAGES / FLASH_ERROR_MESSAGES (both exported, both unit-tested) so an attacker can't craft a URL like `?error=Some+raw+text+to+display` to inject arbitrary copy. Banners go through escapeHtml on render too as defence in depth. Also added two missing redirects: - POST /account/backup-email/remove → ?success=backup_removed - POST /account/sessions/revoke → ?success=session_revoked Tests: new account-settings-flash.test.ts asserts every code redirected to from a POST handler has a matching entry, plus the unknown-code → undefined safety test and the no-HTML test on the message values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+18
to
+22
| import { reservedSubdomains } from '@atproto/pds/dist/handle/reserved.js' | ||
|
|
||
| export function isReservedSubdomain(local: string): boolean { | ||
| return local.toLowerCase() in reservedSubdomains | ||
| } |
Comment on lines
+904
to
+912
| // `maxAge: 600` (see packages/demo/src/app/api/oauth/login/route.ts). | ||
| // If the user spends longer than 10 minutes between starting the OAuth | ||
| // flow and the callback firing — most realistic cause: dawdling on the | ||
| // OTP form, then clicking Resend after the 10-minute mark — the cookie | ||
| // expires before the callback runs, so the callback handler can't find | ||
| // the OAuth state and silently bounces to /?error=auth_failed. | ||
| // | ||
| // This step deletes the cookie programmatically so we can exercise the | ||
| // post-cookie-expiry callback path without a 10-minute wall-clock wait. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/auth-service/src/routes/account-settings.ts (2)
260-276:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
?success=backup_removedis shown even when removal is silently skippedWhen
didisnull(PDS account not found for the signed-in email), theif (did && email)guard skipsctx.db.removeBackupEmailentirely, but line 274 still redirects to?success=backup_removed, showing "Backup email removed." for an operation that did nothing. There is also no try/catch around the DB call; a synchronous throw from better-sqlite3 inside an async Express 4 handler will become an unhandled rejection rather than a clean error redirect.🐛 Proposed fix
- if (did && email) { - ctx.db.removeBackupEmail(did, email) - } - res.redirect(303, '/account?success=backup_removed') + if (!did || !email) { + res.redirect(303, '/account?error=account_not_found') + return + } + try { + ctx.db.removeBackupEmail(did, email) + res.redirect(303, '/account?success=backup_removed') + } catch (err) { + logger.error({ err }, 'Failed to remove backup email') + res.redirect(303, '/account?error=delete_failed') + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth-service/src/routes/account-settings.ts` around lines 260 - 276, The POST /account/backup-email/remove handler currently always redirects with ?success=backup_removed even when getDidByEmail returns null or the removal is skipped, and it lacks error handling for ctx.db.removeBackupEmail which may throw; modify the async handler around getDidByEmail and ctx.db.removeBackupEmail so that you only redirect with success when a removal actually ran (e.g., when did and email are truthy and removeBackupEmail completes), wrap the removeBackupEmail call in a try/catch to catch synchronous DB errors from ctx.db.removeBackupEmail and on error redirect to an error query (e.g., /account?error=backup_remove_failed) or send an appropriate 500, and ensure you call res.redirect only once (success path after successful removal, not unconditionally at the end). Use the existing symbols: the route handler for '/account/backup-email/remove', getDidByEmail, ctx.db.removeBackupEmail, and res.redirect to locate and implement the fix.
279-295:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
?success=session_revokedis sent even when the revocation failsThe catch block (lines 290–292) logs the warning and falls through; execution always reaches line 294, so a user now sees the "Session revoked." success banner even when
revokeSessionthrew. Before this PR the redirect was to plain/accountwith no feedback, so the misleading message is newly introduced here.🐛 Proposed fix
const tokenToRevoke = req.body.session_token as string if (tokenToRevoke) { + let revokeOk = false try { await auth.api.revokeSession({ body: { token: tokenToRevoke }, headers: fromNodeHeaders(req.headers), }) + revokeOk = true } catch (err) { logger.warn({ err }, 'Failed to revoke session') } + if (!revokeOk) { + res.redirect(303, '/account?error=revoke_failed') + return + } } res.redirect(303, '/account?success=session_revoked')Also add
revoke_failedtoFLASH_ERROR_MESSAGES:+ revoke_failed: "We couldn't revoke that session. Please try again.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth-service/src/routes/account-settings.ts` around lines 279 - 295, The POST /account/session/revoke route always redirects with ?success=session_revoked even when auth.api.revokeSession throws; update the route handler (router.post('/account/session/revoke')) so that inside the catch block you redirect to '/account?error=revoke_failed' (or otherwise avoid sending the success query) instead of falling through to the success redirect, and also add 'revoke_failed' to the FLASH_ERROR_MESSAGES constant so the UI can show the corresponding error message for the revoke failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/auth-service/src/routes/account-settings.ts`:
- Around line 260-276: The POST /account/backup-email/remove handler currently
always redirects with ?success=backup_removed even when getDidByEmail returns
null or the removal is skipped, and it lacks error handling for
ctx.db.removeBackupEmail which may throw; modify the async handler around
getDidByEmail and ctx.db.removeBackupEmail so that you only redirect with
success when a removal actually ran (e.g., when did and email are truthy and
removeBackupEmail completes), wrap the removeBackupEmail call in a try/catch to
catch synchronous DB errors from ctx.db.removeBackupEmail and on error redirect
to an error query (e.g., /account?error=backup_remove_failed) or send an
appropriate 500, and ensure you call res.redirect only once (success path after
successful removal, not unconditionally at the end). Use the existing symbols:
the route handler for '/account/backup-email/remove', getDidByEmail,
ctx.db.removeBackupEmail, and res.redirect to locate and implement the fix.
- Around line 279-295: The POST /account/session/revoke route always redirects
with ?success=session_revoked even when auth.api.revokeSession throws; update
the route handler (router.post('/account/session/revoke')) so that inside the
catch block you redirect to '/account?error=revoke_failed' (or otherwise avoid
sending the success query) instead of falling through to the success redirect,
and also add 'revoke_failed' to the FLASH_ERROR_MESSAGES constant so the UI can
show the corresponding error message for the revoke failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d1b9886-341d-4ee6-9f4c-0591b729f615
📒 Files selected for processing (7)
.changeset/account-settings-flash-messages.md.changeset/fallback-app-name-on-malformed-client-id.mdpackages/auth-service/src/__tests__/account-settings-flash.test.tspackages/auth-service/src/routes/account-settings.tspackages/shared/src/__tests__/client-metadata.test.tspackages/shared/src/client-metadata.tspackages/shared/src/render-error.ts
✅ Files skipped from review due to trivial changes (4)
- packages/shared/src/render-error.ts
- packages/auth-service/src/tests/account-settings-flash.test.ts
- .changeset/account-settings-flash-messages.md
- .changeset/fallback-app-name-on-malformed-client-id.md
…ndler
The GET /account handler open-coded the success/error code →
message lookup in 4 lines. Lift that into a pure helper
(resolveAccountFlashFromQuery) so:
- Coverage tooling can see the lookup branches without an
integration test (route handler stays at the same coverage
level, but the lookup itself is now fully covered).
- Future maintainers swapping which params plumb through
don't have to re-derive the safety logic.
- Non-string query values (?success=foo&success=bar arrays,
numeric values) are handled explicitly rather than relying
on Express type coercion.
5 new unit tests covering known codes, empty query, unknown
codes (URL-injection safety), and non-string query values.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
6 tasks
Contributor
Author
|
Updating description regarding the status of this PR, and converting to draft. |
This was referenced Jul 30, 2026
Contributor
Author
|
Superseded by many smaller PRs. |
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.
WIP CAVEAT: This was originally a few simple fixes which have since landed separately in #187. However an over-zealous AI agent was given a bit too much freedom and found a ton of other things to add, some of which are genuine improvements, and others of questionable benefit.
So we need to triage the remaining commits and decide which to keep and which to ditch. They should certainly be split up into smaller PRs.
N.B. The rest of this description is probably out of date!
Summary
visibilitychangeevent, so a backgrounded tab returning to focus reflects reality immediately.Why
Bug report: user requested an OTP, waited ~11 minutes, got "OTP expired" (correct), clicked Send a new code, received a fresh email, typed it, and only then saw "Sign in failed". A fresh code was issued that could never have worked because the upstream PAR row had silently lapsed during the wait.
The earlier reactive abort gate (PR #154) bounces the user cleanly to the OAuth client when this happens, but only after the user has already invested time typing a code that was never going to succeed. This change makes the offer disappear before the user can act on it, so the only forward paths surfaced are paths that actually work.
How
lastSuccessfulHeartbeatAttimestamp updated on everyok:trueping. Initialised to page-load time (PAR is fresh on render).parLikelyDead()returns true whennow - lastSuccessfulHeartbeatAt >= 5min(upstream'sAUTHORIZATION_INACTIVITY_TIMEOUT) or when the page has already been told the flow aborted.refreshResendVisibility()toggles#btn-resend↔#btn-start-overbased onparLikelyDead(). Called from heartbeat tick, visibilitychange, OTP-step show, and the inline-action error path.Out of scope
Server-side enforcement of the same invariant —
/email-otp/send-verification-otpand/sign-in/email-otpwill still process direct POSTs that bypass the UI. Worth doing as a defence-in-depth follow-up; not load-bearing for honest-user UX, which is what this PR addresses.Test plan
@resend-hidden-when-par-deadscenario passes.@otp-and-par-expiry/@par-heartbeat/@resend-after-par-dead/@otp-expirysuite passes (7 scenarios, 78 steps).pnpm format:check,pnpm lint,pnpm typecheck,pnpm test(1017 unit tests) all green locally.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests