fix(session-reuse): recover from stale cookies + fix broken chooser affordances#103
Merged
fix(session-reuse): recover from stale cookies + fix broken chooser affordances#103
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: aa6bb06 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:
📝 WalkthroughWalkthroughAuth-service now requires both Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Client)
participant PDS as pds-core (Welcome Guard)
participant Auth as auth-service
participant Provider as upstream OAuth Provider
Browser->>PDS: GET /oauth/authorize?client_id=... (Cookie header)
PDS->>PDS: parseDeviceCookies(req.Cookie) -> {devId?, sesId?}
alt orphan/half-pair OR zero bound accounts
PDS->>Auth: 303 Location -> /oauth/authorize?...&prompt=login
PDS->>Browser: Set-Cookie: dev-id/ses-id (Max-Age=0 host & Domain), Cache-Control: no-store
Browser-->>Auth: follows redirect -> auth renders email+OTP form
else both cookies present and bindings > 0
PDS->>Provider: next() (pass-through)
Provider->>Browser: serve chooser HTML (enriched)
Browser->>Browser: enrichment script reads metas, hides signup, rebinds "Another account"
Browser->>Auth: "Another account" click -> hard-navigate to /oauth/authorize?prompt=login
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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-103 environment in ePDS
|
Coverage Report for CI Build 25013031214Coverage increased (+3.2%) to 46.754%Details
Uncovered Changes
Coverage Regressions4 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
Hardens cross-client OAuth session reuse against stale/partial dev-id/ses-id cookies, prevents upstream’s stock welcome/sign-in UIs from surfacing in ePDS flows, and updates chooser enrichment behavior (including handle-mode driven UI tweaks).
Changes:
- Tighten auth-service session reuse detection to require a full
dev-id+ses-idpair and clear orphan half-pairs on the email/OTP form response. - Add a pds-core pre-route “welcome-page guard” that bounces empty-device requests to auth-service with cookie clears +
prompt=login. - Update chooser enrichment to (a) rebind upstream “Another account”, (b) hide upstream “Sign up”, and (c) propagate per-request handle-mode via
<meta>tags; plus improved E2E debugging output capture.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/index.ts | Re-export shared resolveHandleMode. |
| packages/shared/src/handle.ts | Add shared handle-mode resolution helper used by both services. |
| packages/pds-core/src/welcome-page-guard.ts | New pre-route guard to prevent upstream stock welcome page rendering and clear stale cookies. |
| packages/pds-core/src/index.ts | Wire welcome-page guard and update chooser-enrichment middleware construction. |
| packages/pds-core/src/chooser-enrichment.ts | Enrichment script updates (Another account rebind, Sign up hide, random-handle UI) + meta injection and per-request handle-mode resolution. |
| packages/pds-core/src/tests/welcome-page-guard.test.ts | Unit tests for new guard utilities and middleware behavior. |
| packages/pds-core/src/tests/chooser-enrichment.test.ts | Update/enhance tests for new enrichment behavior and meta injection. |
| packages/auth-service/src/routes/login-page.ts | Clear orphan cookie half-pairs and use shared handle-mode resolver wrapper. |
| packages/auth-service/src/lib/session-reuse.ts | Require full cookie pair for reuse; add orphan detection + cookie-domain derivation + cookie clearing helpers. |
| packages/auth-service/src/tests/session-reuse.test.ts | Expanded unit tests for new session-reuse/orphan-cookie behavior. |
| features/session-reuse-bugs.feature | New E2E feature coverage for stale-cookie scenarios + chooser affordances (some pending). |
| features/passwordless-authentication.feature | Update scenario wording to “Another account” and add assertion against upstream stock sign-in form. |
| e2e/support/world.ts | Add consoleCapture handle to world state. |
| e2e/support/utils.ts | Reattach console/pageerror capture after browser context resets. |
| e2e/support/hooks.ts | Create per-scenario console log file; write page HTML on failure. |
| e2e/step-definitions/session-reuse.steps.ts | Update step to click upstream “Another account” button. |
| e2e/step-definitions/session-reuse-bugs.steps.ts | New step definitions for stale-cookie resilience scenarios. |
| e2e/cucumber.mjs | Exclude @pending scenarios from the session-reuse profile. |
| docs/design/session-reuse-bugs.md | New design doc capturing regression taxonomy and layered fixes. |
| docs/design/pds-white-boxing.md | Update design docs to reflect new chooser enrichment + welcome-page guard. |
| docs/design/cross-client-session-reuse.md | Update docs to reflect “Another account” rebind approach. |
| .changeset/session-reuse-robustness.md | Patch changeset documenting user/operator-facing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/pds-white-boxing.md (1)
294-320:⚠️ Potential issue | 🟡 MinorUpdate this section for the new cookie-pair contract.
This still documents auth-service session reuse as
dev-id-only and shows the old implementation. With this PR, the risk is now tied to thedev-id+ses-idpair and orphan cleanup, so the white-boxing doc will mislead future upstream bump audits.📝 Suggested doc update
-### 17. `dev-id` cookie detection on the auth-service side +### 17. Device-session cookie-pair detection on the auth-service side **File:** `packages/auth-service/src/lib/session-reuse.ts`, called from `packages/auth-service/src/routes/login-page.ts` The auth-service's `GET /oauth/authorize` route reads the upstream -`dev-id` cookie directly to decide whether to bypass its own email/OTP form -and redirect to pds-core's stock `/oauth/authorize`: +`dev-id` and `ses-id` cookies directly to decide whether to bypass its own +email/OTP form and redirect to pds-core's stock `/oauth/authorize`: ```ts export function hasDeviceSessionCookie(req: SessionReuseRequest): boolean { - if (req.cookies && typeof req.cookies['dev-id'] === 'string') return true - const raw = req.headers.cookie ?? '' - return /(?:^|;\s*)dev-id=/.test(raw) + const hasDevId = hasCookie(req, 'dev-id') + const hasSesId = hasCookie(req, 'ses-id') + return hasDevId && hasSesId }-The auth-service does not parse or verify the cookie; it just treats
-presence as "the browser has an upstream device session, defer to pds-core".
+The auth-service does not verify the cookie values; it just requires the full
+pair before treating the browser as having an upstream device session. A
+half-pair is treated as an orphan state and cleared before falling back to the
+email/OTP form.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/design/pds-white-boxing.mdaround lines 294 - 320, Update the doc to
reflect the new cookie-pair contract: change the example implementation of
hasDeviceSessionCookie (in packages/auth-service/src/lib/session-reuse.ts) to
check for both 'dev-id' and 'ses-id' (use hasCookie(req, 'dev-id') and
hasCookie(req, 'ses-id') and return their logical AND) instead of checking only
'dev-id', and update the explanatory text to state that the auth-service
requires the pair but does not verify values (half-pairs are treated as orphans
and cleared before falling back to email/OTP).</details> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@e2e/support/hooks.ts`: - Around line 69-75: The empty catch in the HTML artifact capture (surrounding writeFile, this.page.content, and safeName) swallows useful diagnostics; replace the silent catch with a call to logger.debug(...) that includes the caught error and contextual info (safeName and target path like `reports/screenshots/${safeName}.html`) so failures are recorded at debug level; ensure you use the existing logger instance (not logger.info) and keep the operation best-effort (i.e., don't rethrow). - Around line 51-55: The per-scenario console WriteStream created by createWriteStream and stored on this.consoleCapture (and attached via attachConsoleCapture) is never closed, leaking file descriptors and leaving logs unflushed; in the After hook, check for this.consoleCapture and gracefully end/close it (e.g., call end() and/or close()) and remove any listeners if attachConsoleCapture added them so the stream is fully flushed and released before the scenario completes. In `@packages/auth-service/src/lib/session-reuse.ts`: - Around line 142-149: The cookie cleanup currently expires only the base cookies ('dev-id' and 'ses-id') in the loop in session-reuse.ts; also expire the signed sidecars by extending the loop in the code that uses res and cookieDomain to clear both `${name}` and `${name}:hash` (i.e., append Set-Cookie for `${name}=; Max-Age=0; Path=/` and for `${name}:hash=; Max-Age=0; Path=/`, and likewise with Domain=${cookieDomain} when cookieDomain is set) so the signed `:hash` cookies are removed as well. In `@packages/pds-core/src/__tests__/welcome-page-guard.test.ts`: - Around line 153-172: appendCookieClearHeaders currently only expires dev-id and ses-id; update it to also expire the sidecar cookies named with the :hash suffix (as defined in DEVICE_COOKIE_NAMES in cookie-domain.ts) so that both dev-id:hash and ses-id:hash are cleared host-only and, when a cookie domain is provided, cleared with Domain=<domain> as well; update the test in welcome-page-guard.test.ts to expect Set-Cookie entries for dev-id:hash and ses-id:hash in the same order/positions as the corresponding dev-id and ses-id entries for both host-only and domain-scoped variants. In `@packages/pds-core/src/index.ts`: - Around line 657-668: Ensure the welcome-page guard installation fails loudly instead of silently degrading: when installing welcomePageGuardMiddleware on pds.app, first verify pds.app._router and pds.app._router.stack exist, throw an error if missing; when obtaining stack and popping the guardLayer confirm guardLayer is defined and not null, throw an error if it is missing; when searching for the expressInit insertion index (guardIdx) ensure the splice actually inserts the guardLayer before upstream routes (and throw if expressInit not found and insertion would append to tail) and only call logger.info('Welcome-page guard installed') after successful validation and insertion. In `@packages/pds-core/src/welcome-page-guard.ts`: - Around line 55-73: The parseDeviceCookies function currently calls decodeURIComponent unconditionally which throws on malformed percent-escapes; update it to avoid crashing by only decoding values for the cookies we care about ("dev-id" and "ses-id") or by wrapping decodeURIComponent in a try/catch and skipping/ignoring values that fail to decode. Concretely, inside parseDeviceCookies (the loop that builds jar) check the cookie name first and if name === 'dev-id' || name === 'ses-id' then attempt decodeURIComponent(value) inside a try/catch (skip that cookie on error); continue to populate jar only for those names so DEVICE_ID_RE and SESSION_ID_RE are validated against safely-decoded values and the function returns null rather than throwing on malformed cookies. --- Outside diff comments: In `@docs/design/pds-white-boxing.md`: - Around line 294-320: Update the doc to reflect the new cookie-pair contract: change the example implementation of hasDeviceSessionCookie (in packages/auth-service/src/lib/session-reuse.ts) to check for both 'dev-id' and 'ses-id' (use hasCookie(req, 'dev-id') and hasCookie(req, 'ses-id') and return their logical AND) instead of checking only 'dev-id', and update the explanatory text to state that the auth-service requires the pair but does not verify values (half-pairs are treated as orphans and cleared before falling back to email/OTP).🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
025d9891-ff34-4cb2-b41f-7f022e01493f📒 Files selected for processing (22)
.changeset/session-reuse-robustness.mddocs/design/cross-client-session-reuse.mddocs/design/pds-white-boxing.mddocs/design/session-reuse-bugs.mde2e/cucumber.mjse2e/step-definitions/session-reuse-bugs.steps.tse2e/step-definitions/session-reuse.steps.tse2e/support/hooks.tse2e/support/utils.tse2e/support/world.tsfeatures/passwordless-authentication.featurefeatures/session-reuse-bugs.featurepackages/auth-service/src/__tests__/session-reuse.test.tspackages/auth-service/src/lib/session-reuse.tspackages/auth-service/src/routes/login-page.tspackages/pds-core/src/__tests__/chooser-enrichment.test.tspackages/pds-core/src/__tests__/welcome-page-guard.test.tspackages/pds-core/src/chooser-enrichment.tspackages/pds-core/src/index.tspackages/pds-core/src/welcome-page-guard.tspackages/shared/src/handle.tspackages/shared/src/index.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/step-definitions/session-reuse-bugs.steps.ts (2)
216-240: Stale comment and redundant check — theSign indiscriminator described in the comment is not actually implemented.The comment at lines 227–231 promises that the assertion covers both "landed on email form" and "landed on chooser" cases by leveraging the presence of
Sign infrom the stock welcome button — but the code that follows (lines 232–238) just repeats the "Create a new account" check via a rawhtml.includes(). That substring scan is strictly weaker than the accessible-role check at lines 224–226 (it matches script data, JSON hydration, aria-live regions, etc.) and adds no new signal.Either drop the duplicate or implement what the comment describes.
♻️ Suggested simplification
Then( 'the stock upstream welcome page is not shown', async function (this: EpdsWorld) { const page = getPage(this) - // The stock welcome page renders three buttons: "Authenticate", - // "Create a new account", and "Sign in". If any of them appear we've - // leaked the page. We check by accessible-button label so a future - // upstream tweak to the DOM tree does not silently bypass the assertion. + // The stock welcome page renders a "Create a new account" button; its + // absence (by accessible role) is our signal the guard intercepted + // before upstream's signin handler rendered. "Sign in" is overloaded + // with the auth-service form's submit button so we don't assert on it. await expect( page.getByRole('button', { name: 'Create a new account' }), ).toHaveCount(0) - // "Sign in" is overloaded with the auth-service form's submit button — - // but the auth-service form has an `#email` input that the stock welcome - // page does not. If we find neither, the welcome page is not shown. - // This lets the same assertion cover both the "landed on email form" - // and "landed on chooser" cases. - const html = await page.content() - // The stock page uses the exact phrase "Create a new account" in the - // button label; if that's gone, the welcome page isn't rendering. - expect( - html.includes('Create a new account'), - `Expected no stock welcome page, but found its "Create a new account" button on ${page.url()}`, - ).toBe(false) }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/session-reuse-bugs.steps.ts` around lines 216 - 240, The test contains a stale comment about disambiguating "Sign in" via an `#email` field but instead runs a redundant html.includes('Create a new account') string scan; either remove the duplicate check or implement the promised discriminator: replace the html.includes check with an explicit DOM presence/assertion for the email input (e.g. using getPage(this) then page.locator('#email') or page.getByRole('textbox', { name: /email/i }) to assert presence/absence) so the test actually distinguishes the auth-service email form from the stock welcome page; update or delete the comment accordingly.
403-411: Click step doesn't wait for the hard-navigation to complete.Per the PR summary, "Another account" rebinds the upstream button with a capture-phase handler that then does a
window.locationhard-navigate toauth.<host>/oauth/authorize?prompt=login. A bare.click()returns as soon as the event dispatches, so the follow-onThenstep (presumably "the browser lands on the auth-service email-and-OTP form") races the navigation. On a slow CI node this will manifest as flake rather than a real failure.♻️ Suggested fix — await the navigation alongside the click
When( 'the user clicks "Another account" on the enriched account picker', async function (this: EpdsWorld) { const page = getPage(this) - await page - .getByRole('button', { name: 'Login to account that is not listed' }) - .click() + await Promise.all([ + page.waitForURL(/\/oauth\/authorize\b.*\bprompt=login\b/, { + timeout: 30_000, + }), + page + .getByRole('button', { name: 'Login to account that is not listed' }) + .click(), + ]) }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/session-reuse-bugs.steps.ts` around lines 403 - 411, The click on the "Login to account that is not listed" button needs to be awaited together with the resulting hard navigation to avoid races; replace the bare await page.getByRole(...).click() in the When handler with a coordinated wait (e.g., use Promise.all to await page.waitForNavigation(...) or page.waitForURL matching the auth.<host>/oauth/authorize?prompt=login pattern alongside calling page.getByRole(...).click()) so the step does not return until the browser has completed the navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/step-definitions/session-reuse-bugs.steps.ts`:
- Around line 216-240: The test contains a stale comment about disambiguating
"Sign in" via an `#email` field but instead runs a redundant html.includes('Create
a new account') string scan; either remove the duplicate check or implement the
promised discriminator: replace the html.includes check with an explicit DOM
presence/assertion for the email input (e.g. using getPage(this) then
page.locator('#email') or page.getByRole('textbox', { name: /email/i }) to
assert presence/absence) so the test actually distinguishes the auth-service
email form from the stock welcome page; update or delete the comment
accordingly.
- Around line 403-411: The click on the "Login to account that is not listed"
button needs to be awaited together with the resulting hard navigation to avoid
races; replace the bare await page.getByRole(...).click() in the When handler
with a coordinated wait (e.g., use Promise.all to await
page.waitForNavigation(...) or page.waitForURL matching the
auth.<host>/oauth/authorize?prompt=login pattern alongside calling
page.getByRole(...).click()) so the step does not return until the browser has
completed the navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fad951c-a211-43ed-901f-23438061bed0
📒 Files selected for processing (2)
e2e/step-definitions/session-reuse-bugs.steps.tsfeatures/session-reuse-bugs.feature
🚧 Files skipped from review as they are similar to previous changes (1)
- features/session-reuse-bugs.feature
aspiers
added a commit
that referenced
this pull request
Apr 21, 2026
- welcome-page-guard: wrap decodeURIComponent in try/catch so a malformed percent-escape in an unrelated cookie (RFC 6265 permits literal %) no longer crashes the middleware; only the two device cookies are decoded. - welcome-page-guard: skip the bounce when /account* is hit without a request_uri (direct nav / bookmark) — auth-service would 400 on a missing request_uri, worse UX than the stock welcome page we're suppressing. OAuth flows still bounce as before. - welcome-page-guard + auth-service: extend the cleared cookie set to include the :hash sidecars upstream may emit when cookie.keys is configured (ePDS doesn't wire that today, but clearing them now is defensive future-proofing and matches the broadening name list). - chooser-enrichment: HTML-escape authOrigin when embedding it into the <meta name="epds-auth-origin"> attribute. Operator-configured, not user-controlled, but cheap defense-in-depth against attribute-escape injection from a misconfigured value. - chooser-enrichment: correct the random-mode comment — display:none on the handle span is intentional, not a bug. Removing opaque random handles from the accessibility tree is the right UX; the tooltip on the parent preserves the value for inspection. - pds-core/index.ts: fail-closed if the Express _router.stack splice can't install the welcome-page guard, instead of silently booting with an inert guard. - e2e/support/hooks.ts: close the per-scenario consoleCapture stream in After to avoid file-descriptor leaks and lost buffered output across a long run; log HTML-capture failures instead of swallowing them. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to the cross-client session-reuse work, tightening session detection and adding guards/enrichment so stale/partial device-session cookies can’t land users on upstream @atproto/oauth-provider’s stock welcome/sign-in surfaces. It also fixes chooser affordances (“Another account”, “Sign up”) and adds random-handle-mode presentation consistency across auth-service and pds-core.
Changes:
- Centralize handle-mode precedence via shared
resolveHandleMode()and apply it consistently (auth-service + pds-core chooser enrichment), including hiding handles inrandommode. - Add a pds-core pre-route “welcome-page guard” that redirects empty-device flows to auth-service with cookie clears.
- Update chooser enrichment to hide upstream “Sign up” and rebind upstream “Another account” to hard-navigate to auth-service, plus add/extend unit + e2e coverage and design docs.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/index.ts | Re-exports resolveHandleMode from shared. |
| packages/shared/src/handle.ts | Adds shared resolveHandleMode() implementing precedence + fallback. |
| packages/pds-core/src/welcome-page-guard.ts | New pre-route middleware to bounce empty-device flows and clear cookies. |
| packages/pds-core/src/index.ts | Wires welcome-page guard and updates chooser enrichment wiring (deps + authOrigin). |
| packages/pds-core/src/chooser-enrichment.ts | Enrichment script updates: handle-mode meta, hide “Sign up”, rebind “Another account”, authOrigin meta escaping. |
| packages/pds-core/src/tests/welcome-page-guard.test.ts | Unit coverage for guarded paths, cookie parsing, bounce behavior, and cookie clears. |
| packages/pds-core/src/tests/chooser-enrichment.test.ts | Updates/expands unit coverage for deterministic script + meta injection and new behaviors. |
| packages/auth-service/src/routes/login-page.ts | Uses shared handle-mode resolver; clears orphan half-pair cookies on email-form responses. |
| packages/auth-service/src/lib/session-reuse.ts | Tightens session detection to require both cookies; adds orphan detection + cookie-domain derivation + clear helpers. |
| packages/auth-service/src/tests/session-reuse.test.ts | Expands unit tests for new cookie-pair/orphan logic and clearing behavior. |
| features/session-reuse-bugs.feature | New e2e feature spec for stale-cookie resilience + chooser affordances + random-handle behavior. |
| features/passwordless-authentication.feature | Updates scenario wording/steps for “Another account” behavior and asserts upstream sign-in isn’t shown. |
| e2e/support/world.ts | Adds per-scenario consoleCapture handle on world. |
| e2e/support/utils.ts | Reattaches console capture after browser context resets. |
| e2e/support/hooks.ts | Implements per-scenario console capture and HTML snapshot on failures; closes streams in After hook. |
| e2e/step-definitions/session-reuse.steps.ts | Updates chooser click step to target upstream “Another account” button. |
| e2e/step-definitions/session-reuse-bugs.steps.ts | Adds step definitions for new stale-cookie resilience scenarios and chooser assertions. |
| e2e/cucumber.mjs | Excludes @pending scenarios from the @session-reuse profile. |
| docs/design/session-reuse-bugs.md | New design doc detailing failure modes and layered fixes. |
| docs/design/pds-white-boxing.md | Updates design notes to reflect new enrichment behaviors + welcome-page guard. |
| docs/design/cross-client-session-reuse.md | Updates design doc references from injected link to rebound “Another account”. |
| .changeset/session-reuse-robustness.md | Patch changeset describing end-user/operator/dev-facing behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aspiers
added a commit
that referenced
this pull request
Apr 21, 2026
- welcome-page-guard.buildBounceUrl: assign the raw serialized query string (target.search = parsed.search) instead of copying via searchParams.forEach + set(). The forEach+set() pattern collapsed repeated keys like scope=atproto&scope=transition:generic to the last value, contradicting the "preserves verbatim" contract. Added a unit test that asserts both scope values survive the bounce. - chooser-enrichment.buildAnotherAccountUrl: return '' when the current URL has no request_uri, and skip the rebind in rebindAnotherAccount when the target URL would be empty. Previously a standalone /account click on "Another account" would navigate to /oauth/authorize?prompt=login, which auth-service rejects with a 400; letting upstream's default handler run is strictly better UX. - e2e/support/hooks.ts: fix the now-inaccurate comment in After — resetBrowserContext re-attaches the same stream to the new Page, never creates a replacement, so there is only ever one stream per scenario to close. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 22, 2026
- welcome-page-guard: wrap decodeURIComponent in try/catch so a malformed percent-escape in an unrelated cookie (RFC 6265 permits literal %) no longer crashes the middleware; only the two device cookies are decoded. - welcome-page-guard: skip the bounce when /account* is hit without a request_uri (direct nav / bookmark) — auth-service would 400 on a missing request_uri, worse UX than the stock welcome page we're suppressing. OAuth flows still bounce as before. - welcome-page-guard + auth-service: extend the cleared cookie set to include the :hash sidecars upstream may emit when cookie.keys is configured (ePDS doesn't wire that today, but clearing them now is defensive future-proofing and matches the broadening name list). - chooser-enrichment: HTML-escape authOrigin when embedding it into the <meta name="epds-auth-origin"> attribute. Operator-configured, not user-controlled, but cheap defense-in-depth against attribute-escape injection from a misconfigured value. - chooser-enrichment: correct the random-mode comment — display:none on the handle span is intentional, not a bug. Removing opaque random handles from the accessibility tree is the right UX; the tooltip on the parent preserves the value for inspection. - pds-core/index.ts: fail-closed if the Express _router.stack splice can't install the welcome-page guard, instead of silently booting with an inert guard. - e2e/support/hooks.ts: close the per-scenario consoleCapture stream in After to avoid file-descriptor leaks and lost buffered output across a long run; log HTML-capture failures instead of swallowing them. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 22, 2026
- welcome-page-guard.buildBounceUrl: assign the raw serialized query string (target.search = parsed.search) instead of copying via searchParams.forEach + set(). The forEach+set() pattern collapsed repeated keys like scope=atproto&scope=transition:generic to the last value, contradicting the "preserves verbatim" contract. Added a unit test that asserts both scope values survive the bounce. - chooser-enrichment.buildAnotherAccountUrl: return '' when the current URL has no request_uri, and skip the rebind in rebindAnotherAccount when the target URL would be empty. Previously a standalone /account click on "Another account" would navigate to /oauth/authorize?prompt=login, which auth-service rejects with a 400; letting upstream's default handler run is strictly better UX. - e2e/support/hooks.ts: fix the now-inaccurate comment in After — resetBrowserContext re-attaches the same stream to the new Page, never creates a replacement, so there is only ever one stream per scenario to close. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aspiers
force-pushed
the
session-reuse-bugs
branch
from
98f7386 to
82db640
Compare
…ucture rules
The cross-client-session-reuse changeset had drifted into PR-description
territory, with per-audience sections referencing internal middleware
names (`pds-core mounts a pre-route guard…`), upstream source-code paths
(`upstream's request-manager.js force-overrides…`), and step-by-step
request-flow narratives. Release-notes readers consume ePDS as a single
product and don't need any of that to adapt their config or code.
Two changes in concert:
- Rewrite the changeset's per-audience sections as short bullet lists of
"what do I need to do differently" points. End-user prose stays
end-user-flavoured; client-app and operator sections name only the
fields, env vars, and conditions that readers actually adapt against.
- Add two new subsections to the writing-changesets skill:
- "Depth check: adaptation detail, not architecture detail" — the
bar to clear before including any technical detail in a
per-audience section is whether the reader's adaptation depends
on knowing it. Lists what to avoid (internal package/module
names, upstream source-code references, request-flow narratives,
"why" explanations of internal mechanics) and what to keep
(env vars, fields, error messages, observable old-vs-new).
- "Structure dense sections as bullets" — when a per-audience
section has 3+ distinct points, prefer a bullet list over a
wall-of-text paragraph. Two-or-fewer points stay inline. Bullet
lists survive the changesets-github 2-space indent.
The example block in the skill grows a second example showing the
bullet form for a denser change.
…ate-limit bullet The "auto-pre-populated chooser on login_hint match" framing was wrong twice over: the chooser is a clickable list (nothing to pre-populate), and current shipped behaviour renders the chooser identically with or without login_hint (auto-skip on hint match is the @pending P-series). Also drop the EPDS_DISABLE_RATE_LIMIT bullet — that flag is its own operator-facing change, not a deliverable of cross-client session reuse. Moved to .changeset/rate-limit-disable-flag.md in a follow-up commit. Mirror the same corrections in the writing-changesets skill example so the next changeset doesn't re-learn the same mistakes.
Split out from the cross-client session-reuse changeset. The flag is a standalone operator-facing knob for single-source-IP test environments; shipping it under "session reuse" conflates an internal testing affordance with a user-visible feature.
"Pass prompt=login on /oauth/authorize" assumes the reader hand-builds authorization URLs, which most clients don't — they call into an OAuth library or hit the PAR endpoint. Reframe as the OIDC parameter name and where it goes (library option or PAR body), so a client developer knows what to do without us prescribing one OAuth stack.
Renders a "Force re-authentication (prompt=login)" checkbox on every demo sign-in form (homepage email/handle, plus flow2/3/4 pages). When checked, the demo passes prompt=login both in the PAR body and on the authorize redirect URL. The URL placement is the load-bearing one: auth-service's shouldReuseSession reads only the /oauth/authorize query string at the AS-metadata redirect, never the PAR body. PAR-body prompt=login alone left users on the chooser and made the changeset's claim look broken. Factored the checkbox into ForceLoginCheckbox so both LoginForm and FlowLoginPage share one uncontrolled implementation.
ePDS's session-reuse short-circuit reads the /oauth/authorize query
string, not the PAR body. Putting prompt=login in PAR alone is silently
ignored — the user still lands on the chooser. Codify this so the next
implementer doesn't waste time discovering it from logs:
- new "Forcing a Fresh Sign-In" section with where-to-put-it table and
hand-rolled / NodeOAuthClient examples (the latter shows post-hoc
searchParams.set on the URL the library returns)
- pitfall row mapping the symptom ("prompt=login ignored, chooser still
shown") to the fix
Earlier wording said "set the OIDC prompt parameter on the authorization request your OAuth library sends (typically a prompt: 'login' option, or include prompt=login in the PAR body)". The PAR-body suggestion was actively misleading: ePDS's short-circuit only reads the URL query string, so a client following that advice would still hit the chooser. Point readers at the epds-login skill for the URL-vs-body details instead of trying to compress them into a release-note bullet.
|
aspiers
commented
Apr 27, 2026
| **Important — where to put it:** ePDS's auth service decides whether to | ||
| engage session reuse by inspecting the **query string** of the | ||
| `/oauth/authorize` redirect, not the PAR body. PAR-body `prompt=login` | ||
| alone is **not** enough. |
Contributor
Author
There was a problem hiding this comment.
It's not just not enough, it's pointless - right?
aspiers
commented
Apr 27, 2026
Comment on lines
+199
to
+203
| | Sent in | Engages ePDS short-circuit? | | ||
| | ------------------ | --------------------------- | | ||
| | Authorize URL only | Yes | | ||
| | PAR body only | No | | ||
| | Both | Yes | |
Contributor
Author
There was a problem hiding this comment.
This table seems superfluous.
This was referenced Apr 27, 2026
aspiers
added a commit
to aspiers/ePDS
that referenced
this pull request
Apr 27, 2026
Address review feedback on PR hypercerts-org#103 (threads on .agents/skills/epds-login/SKILL.md:197 and :203): - PAR-body `prompt=login` isn't merely "not enough" — it's ignored, so say so directly. - Drop the 3-row table; it added no information beyond the prose plus the "URL only / both" rows could mislead readers into thinking dual placement is meaningful. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Apr 28, 2026
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
Follow-up to the cross-client session-reuse work shipped in #96, addressing two classes of post-ship bug uncovered after that PR merged to main:
Stale/partial device-session cookies land the user on upstream
@atproto/oauth-provider's stock welcome page instead of the email/OTP form. Layered fix: auth-service now requires bothdev-id+ses-idbefore trusting the device-session signal and falls through to the email form otherwise, clearing orphan cookies in both host-only and shared-parent-domain scopes. pds-core adds a pre-route guard in front of/oauth/authorizeand/accountthat intercepts requests resolving to a device with zero bound accounts (migration-005 TTL purge, fixation race, purged bindings) and 303-redirects to auth-service withprompt=loginplus cookie clears — so the stock three-button welcome page is never rendered from an ePDS entry point.Two chooser affordances miscategorised as "already correct" / "unreachable" in the Layer 3 design doc:
<div role="button" aria-label="Login to account that is not listed">. React's delegated root-level click listener intercepts first and swaps the chooser for upstream's stock sign-in form — so any injected anchor was cosmetic. The chooser-enrichment snippet now rebinds the upstream div with a capture-phase handler thatpreventDefault()+stopImmediatePropagation()and hard-navigates toauth.<host>/oauth/authorize?prompt=login. Device bindings are preserved — "Another account" adds a new binding rather than replacing.Random-handle mode: when the flow resolves to
epds_handle_mode=random, handles are hidden from the chooser row (surfaced only as atitletooltip) so server-generated opaque handles don't become the primary identifier. Uses the sharedresolveHandleModeprecedence so pds-core and auth-service can't disagree.Test plan
pnpm format:check && pnpm lint && pnpm typecheck && pnpm test— all green (650/650 unit tests).pnpm docker:build && docker compose up -d— full stack up.@session-reusescenarios targeting the chooser fixes (Another account,Sign up,different account) all pass.@session-reusesuite — 4 scenarios intermittently fail in the existing OTP setup step (#step-otp.activetiming out on returning-user sign-ins); pre-existing flakiness, not introduced by this PR.auth.<host>/oauth/authorize?prompt=loginand the ePDS email form renders (not upstream's vanilla sign-in).Changesets
.changeset/session-reuse-robustness.mdcovers end-user, client-developer, and operator scope.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests