Fix OAuth account enrichment with tooltip explanation - #148
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 7fe24bb 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:
📝 WalkthroughWalkthroughThis change propagates ChangesHandle mode callback contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AuthService
participant EPDSCallback
participant PDSCore
participant EnrichmentScript
participant ConsentPage
AuthService->>EPDSCallback: send signed epds_handle_mode
EPDSCallback->>PDSCore: verify callback and redirect to authorize
PDSCore->>EnrichmentScript: inject resolved handle mode
EnrichmentScript->>ConsentPage: replace identifier and attach tooltip
ConsentPage-->>EnrichmentScript: expose email and accessible handle description
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 pr-5c336d-148 environment in ePDS
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/complete.ts (1)
164-169:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSign
epds_handle_modeas part of the callback contract.
epds_handle_modeis being appended aftersignCallback(), so the browser can alter it before/oauth/epds-callbackreaches pds-core. Because the callback value is later used to drive the chooser/consent presentation, this makes the privacy/display mode user-tamperable. Please either include it in the signed payload end-to-end or stop trusting the callback query for this field.As per coding guidelines,
packages/{auth-service,pds-core}/**/*.{ts,tsx,js,mjs}: All epds-callback redirects must be HMAC-SHA256 signed using signCallback()/verifyCallback() from@certified-app/shared.Also applies to: 208-213
🤖 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/complete.ts` around lines 164 - 169, The fix: include epds_handle_mode in the signed callback instead of appending it afterward — add epds_handle_mode: flow.handleMode into the callbackParams object before calling signCallback(callbackParams, ctx.config.epdsCallbackSecret) so the generated ts/sig cover that field, and keep using URLSearchParams(params) afterwards; ensure any other occurrences (the similar block around the later 208-213) follow the same pattern so all epds-callback redirects are HMAC-SHA256 signed via signCallback()/verifyCallback().packages/auth-service/src/routes/choose-handle.ts (1)
366-371:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid adding
epds_handle_modeoutside the signed callback payload.This POST path has the same integrity gap as
/auth/complete:epds_handle_modeis mutated onto the redirect URL aftersignCallback(). That leaves the mode browser-controlled on the hop to pds-core, so the final consent/chooser UI can be flipped without invalidating the callback signature.As per coding guidelines,
packages/{auth-service,pds-core}/**/*.{ts,tsx,js,mjs}: All epds-callback redirects must be HMAC-SHA256 signed using signCallback()/verifyCallback() from@certified-app/shared.🤖 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/choose-handle.ts` around lines 366 - 371, The code currently appends epds_handle_mode to params after calling signCallback, leaving it unsigned; instead, if flow.handleMode is present add it into the callbackParams object before calling signCallback (e.g. set callbackParams.epds_handle_mode = flow.handleMode), then call signCallback(callbackParams, ctx.config.epdsCallbackSecret) and build URLSearchParams from the resulting signed callback (remove the later params.set('epds_handle_mode', ...) call); this ensures the handle mode is included in the HMAC produced by signCallback/verifyCallback.
🧹 Nitpick comments (2)
.changeset/preserve-handle-mode-callback.md (1)
5-5: ⚡ Quick winRewrite the summary line in plainer end-user language.
app-requested handle display modereads like implementation terminology. Since this first line is the headline release note for End users, I’d rephrase it in terms of what people actually see during sign-up/approval.As per coding guidelines,
.changeset/*.md: Changeset summary line is the first non-frontmatter line, read by every listed audience; if End users is an audience, write in plain language without OTP/DID/PAR/OAuth jargon or implementation concepts.🤖 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/preserve-handle-mode-callback.md at line 5, Rewrite the first non-frontmatter line (currently: "Sign-up screens now keep the app-requested handle display mode through the final approval step.") into plain end-user language that describes what the user sees during sign-up/approval without implementation jargon; e.g. state that the app will remember and show the preferred handle/display option throughout the sign-up and final approval screens, using simple terms like "remember" or "keep showing" instead of "app-requested handle display mode" or other technical phrases.e2e/step-definitions/session-reuse-bugs.steps.ts (1)
439-506: ⚡ Quick winAssert the exact hidden handle, not just a non-empty suffix.
This still passes if
aria-describedbypoints at a generic placeholder or the wrong row's description. Since the callback already hashandleLabel, capture its text and compare the description body to that exact hidden handle.Suggested tightening
type HiddenHandleDescriptionRow = { describedBy: string | null descriptions: { id: string isHiddenHandleDescription: boolean text: string }[] emailTitle: string | null + hiddenHandleText: string rowIndex: number } ... return { describedBy, descriptions, emailTitle: emailLabel.getAttribute('title'), + hiddenHandleText: handleLabel.textContent?.trim() ?? '', rowIndex, } ... - expect( - descriptionText.slice(prefixIndex + prefix.length).trim().length, - ).toBeGreaterThan(0) + expect(descriptionText.slice(prefixIndex + prefix.length).trim()).toBe( + row.hiddenHandleText, + )🤖 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 `@e2e/step-definitions/session-reuse-bugs.steps.ts` around lines 439 - 506, The test currently only asserts that the hidden-handle description contains a non-empty suffix; instead capture the actual handle text from the DOM (use handleLabel.textContent.trim() inside the evaluateAll mapping and include it on the returned HiddenHandleDescriptionRow), then in the outer assertions compare description.text exactly (or ensure it contains) that captured handleText (not just non-empty trimmed suffix). Update references in the mapping for handleLabel and the returned object (e.g., add handleText) and replace the loose suffix checks on descriptionText with a precise comparison against that handleText.
🤖 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.
Inline comments:
In `@e2e/step-definitions/consent.steps.ts`:
- Around line 162-172: The test reads aria-describedby into describedBy and
constructs a locator with page.locator(`#${describedBy?.trim()}`), but
aria-describedby can contain multiple space-separated IDs; split describedBy on
whitespace (or use describedBy?.split(/\s+/)[0]) and guard for null/empty before
building the selector so you only target the first ID; update the code around
describedBy, tooltipControl and the tooltip locator to use the first token (and
bail or assert if none) when calling page.locator and subsequent expects.
In `@packages/pds-core/src/chooser-enrichment.ts`:
- Around line 895-910: The catch block that silences failures when resolving
request_uri/metadata should log the failure at debug level: thread the pds-core
logger into this middleware (or import the existing logger) and in the catch for
the resolveOAuthClientIdFromQuery/resolveMeta sequence emit logger.debug with a
concise message that includes the failing inputs (e.g. the query/request_uri and
resolved clientId if available) and the caught error/stack; keep behavior
unchanged otherwise so metaMode still falls back to query/env. Ensure the log
uses logger.debug(...) and references resolveOAuthClientIdFromQuery,
resolveClientIdFromRequestUri and resolveMeta to locate the code to modify.
---
Outside diff comments:
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 366-371: The code currently appends epds_handle_mode to params
after calling signCallback, leaving it unsigned; instead, if flow.handleMode is
present add it into the callbackParams object before calling signCallback (e.g.
set callbackParams.epds_handle_mode = flow.handleMode), then call
signCallback(callbackParams, ctx.config.epdsCallbackSecret) and build
URLSearchParams from the resulting signed callback (remove the later
params.set('epds_handle_mode', ...) call); this ensures the handle mode is
included in the HMAC produced by signCallback/verifyCallback.
In `@packages/auth-service/src/routes/complete.ts`:
- Around line 164-169: The fix: include epds_handle_mode in the signed callback
instead of appending it afterward — add epds_handle_mode: flow.handleMode into
the callbackParams object before calling signCallback(callbackParams,
ctx.config.epdsCallbackSecret) so the generated ts/sig cover that field, and
keep using URLSearchParams(params) afterwards; ensure any other occurrences (the
similar block around the later 208-213) follow the same pattern so all
epds-callback redirects are HMAC-SHA256 signed via
signCallback()/verifyCallback().
---
Nitpick comments:
In @.changeset/preserve-handle-mode-callback.md:
- Line 5: Rewrite the first non-frontmatter line (currently: "Sign-up screens
now keep the app-requested handle display mode through the final approval
step.") into plain end-user language that describes what the user sees during
sign-up/approval without implementation jargon; e.g. state that the app will
remember and show the preferred handle/display option throughout the sign-up and
final approval screens, using simple terms like "remember" or "keep showing"
instead of "app-requested handle display mode" or other technical phrases.
In `@e2e/step-definitions/session-reuse-bugs.steps.ts`:
- Around line 439-506: The test currently only asserts that the hidden-handle
description contains a non-empty suffix; instead capture the actual handle text
from the DOM (use handleLabel.textContent.trim() inside the evaluateAll mapping
and include it on the returned HiddenHandleDescriptionRow), then in the outer
assertions compare description.text exactly (or ensure it contains) that
captured handleText (not just non-empty trimmed suffix). Update references in
the mapping for handleLabel and the returned object (e.g., add handleText) and
replace the loose suffix checks on descriptionText with a precise comparison
against that handleText.
🪄 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: 3cebbd6d-6d03-4125-9514-8c20d849467e
📒 Files selected for processing (21)
.beads/issues.jsonl.changeset/preserve-handle-mode-callback.md.changeset/random-handle-consent-metadata.md.changeset/scoped-account-enrichment.mde2e/step-definitions/consent.steps.tse2e/step-definitions/session-reuse-bugs.steps.tsfeatures/consent-screen.featurefeatures/session-reuse-bugs.featurepackages/auth-service/src/__tests__/callback-handle-mode.test.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/pds-core/src/__tests__/chooser-enrichment.test.tspackages/pds-core/src/__tests__/epds-callback-authorize.test.tspackages/pds-core/src/__tests__/preview-chooser.test.tspackages/pds-core/src/__tests__/preview-consent.test.tspackages/pds-core/src/chooser-enrichment.tspackages/pds-core/src/index.tspackages/pds-core/src/lib/client-css-injection.tspackages/pds-core/src/lib/epds-callback-authorize.tspackages/pds-core/src/lib/oauth-request-context.tspackages/pds-core/src/lib/preview-consent.ts
|
(reply generated by gpt-5.5) Review follow-up status:
|
|
(reply generated by gpt-5.5) Follow-up: added |
![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.
This comment has been minimized.
This comment has been minimized.
Note on the e2e failure seen on this branch (resolved — flaky, not a regression)The first E2E run on Re-running the same job on the identical commit passed, so this is flake rather than a regression from the rebase. All checks on Why it looks like a product bug but isn't: better-auth throws Suspected root cause, unfixed and pre-existing: UPDATE verification SET expiresAt = ? WHERE identifier = ?with no liveness guard, whereas the sibling I have deliberately not fixed this here: it is unrelated to this PR's scope and would widen the diff. Worth a separate issue if the scenario keeps failing. Tracked separately as #214 (with the better-auth |
|
(reply generated by Claude Opus 5 via Claude Code) Re: the four nitpicks in CodeRabbit's latest review summaryThese were in the review body rather than as inline threads, so replying here. Three addressed, one declined. 1. Escape-to-dismiss for the identity tooltip ( 2. Hoist the duplicated handle-mode resolver ( 3. Document the fake 4. Use a CSS class for runtime visibility ( All green on 7f76cd6 locally: build, lint, format, and 1132 unit tests (Node 20, matching CI). |
This comment has been minimized.
This comment has been minimized.
Three suites drifted against changes that landed on main while this branch was open: - callback-handle-mode: /auth/complete now signs client_id as well, so the verification helper must include it or the signature never matches. - login-page: the readiness-gate test banned setTimeout/setInterval across the whole page, which now trips on the unrelated PAR heartbeat interval. Scope it to the gap between the last handler registration and the enable, which is the actual invariant. - chooser-enrichment: main renders the chooser email label with a leading space for visual separation (e873e7a); trim it in the assertions since that space is presentation, not identity.
Matches the sibling flowRequestUri / flowClientId args, and the name the branch had already converged on before the rebase collapsed it against main's buildEpdsCallbackUrl extraction. Optional so main's existing callers in build-epds-callback-url.test.ts stay valid.
The metadata/request-uri fallback path logs via `logger?.debug`, and a unit test covers it when a logger is injected — but the production call site in index.ts never passed one, so the optional chain made it a no-op everywhere it mattered. That left handle-mode mismatches undebuggable in production, which is the problem the logging was added to solve.
Address CodeRabbit review findings on #148. The description id was derived from the per-tick match index. enrich() rebuilds that list on every MutationObserver tick and skips rows already marked epdsEnriched, so a row enriched on a later re-render restarts at index 0 and collides with an earlier row's id. Duplicate ids make aria-describedby resolve to the first matching node, so a row could announce a different account's handle. Use a monotonic counter that survives across ticks, with a regression test that replays a tick and fails on the old code. Also: - assert the login-page readiness gate against the submit binding rather than the last click handler, which was a proxy that would still pass if the submit binding moved below the enable - take .first() in the e2e identity-tooltip helper: enrichment adds an icon per matching identity node, so a page with two approved phrasings tripped Playwright strict mode - add direct unit coverage for resolveOAuthClientIdFromQuery, previously only exercised through its callers
Address a CodeRabbit nitpick on #148. This PR added a second byte-identical copy of resolveQueryHandleMode to preview-consent.ts alongside the existing one in preview-chooser.ts. Both previews must interpret an unknown metadata value the same way as the production chooserEnrichment middleware, so the duplication is a drift risk rather than harmless repetition. preview-shared.ts already owns the helpers both preview routes share, so move it there and drop the now-unused resolveHandleMode / VALID_HANDLE_MODES / ClientMetadata imports from both call sites.
Address CodeRabbit nitpicks on #148. WCAG 1.4.13 (Content on Hover or Focus) requires hover/focus content to be dismissible without moving the pointer or focus. The tooltip had no key handler at all, and once pinned by click hide() returns early, so a keyboard or AT user who pinned it could not close it without moving focus back to the button. Escape now clears pinned and hides, on keyup so we do not race the surrounding page for the same key. Also document the deliberate divergence in the test's fake createTreeWalker: it snapshots descendants where a real walker is live, so enrichment-inserted nodes are not revisited. Benign today because nothing the script inserts can match the identity predicates, but worth flagging before those loosen.
|
|
@Kzoeps I've split this into four stacked PRs to make it reviewable. The motivation: this PR is +2918/−173, but ~1450 of that is a single test file, so the real change is ~700 lines covering four independent concerns. In particular the HMAC payload change is currently reviewed alongside 1400 lines of DOM tests, and it deserves its own scrutiny.
#242 is entirely independent and could merge today. #241 is a near-trivial refactor. #243 carries the HMAC change. #244 is the actual user-visible enrichment work. Nothing was changed in the process. Merging all four branches produces a tree byte-identical to this PR's head ( Each branch independently passes Two things I flagged in the split that are worth your eyes:
Your two review threads here are both marked outdated; the 🤖 Generated with Claude Code |
2 participants
Summary
This PR restores account identity enrichment across the OAuth and account-management surfaces for ePDS deployments that use generated/random handles.
The user-facing goal is simple: when a handle is system-generated and not meaningful to the user, ePDS should identify the account primarily by email while still keeping the public AT Protocol handle available where it is useful for context, accessibility, or app interoperability.
closes #143
What changed
Restores scoped account identity enrichment on:
/account/account/:didMakes random-handle flows email-first:
Tightens the DOM enrichment logic so only known account identity elements are rewritten.
This avoids accidentally mutating legal copy, technical details, connected-app rows, device rows, footer text, or arbitrary prose that happens to contain handle-like text.
Makes
epds_handle_moderesolution consistent between auth-service and pds-core:request_uri/oauth/epds-callbackShares OAuth request-context resolution between chooser enrichment and client CSS injection so PAR-backed authorize pages can still resolve the client id correctly.
Updates random-handle e2e coverage to assert the accessible hidden-handle behavior instead of the previous stale
titletooltip behavior.Summary by CodeRabbit
UI Changes
when handle mode is picker or picker with random
when handle mode is random