feat(auth): sign epds_handle_mode through the callback hop - #243
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 534647d 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 |
|
Warning Review limit reached
Next review available in: 12 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
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-243 environment in ePDS
|
There was a problem hiding this comment.
Pull request overview
This PR ensures epds_handle_mode (handle assignment/display mode) survives the auth-service → pds-core callback hop by including it in the HMAC-signed callback payload, and then only forwarding valid values back into the upstream /oauth/authorize redirect. This supports consistent account labelling on chooser/consent screens, including PAR-backed flows.
Changes:
- Extend the signed epds-callback payload to include
epds_handle_mode, with empty-string sentinel handling aligned with existing optional fields. - Add pds-core callback redirect sanitization/forwarding for
epds_handle_mode(forward valid values, drop invalid). - Add targeted unit/integration tests in shared, auth-service, and pds-core to pin signing, verification, and forwarding behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/crypto.ts | Adds epds_handle_mode to the signed callback payload and verification payload construction. |
| packages/shared/src/tests/crypto.test.ts | Adds coverage for signing/verifying epds_handle_mode and rejecting tampering. |
| packages/pds-core/src/lib/epds-callback-authorize.ts | New helper to validate and forward epds_handle_mode on the post-callback /oauth/authorize redirect. |
| packages/pds-core/src/index.ts | Threads epds_handle_mode into callback verification and uses the new authorize-redirect builder. |
| packages/pds-core/src/tests/epds-callback-authorize.test.ts | New unit tests ensuring valid epds_handle_mode is forwarded and invalid values are dropped. |
| packages/auth-service/src/routes/complete.ts | Signs epds_handle_mode into the callback URL and refactors identity resolution into resolveCompleteIdentity(). |
| packages/auth-service/src/routes/choose-handle.ts | Includes epds_handle_mode in the signed callback params for chosen-handle flows. |
| packages/auth-service/src/tests/callback-handle-mode.test.ts | New integration-ish tests asserting handle mode is preserved and signatures verify across key flows. |
| .changeset/handle-mode-through-approval.md | Adds a client-developer-facing changeset documenting epds_handle_mode resolution/behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
daa7cea to
84de1ac
Compare
ee1ec80 to
707355b
Compare
84de1ac to
14dd1b4
Compare
14dd1b4 to
bf37444
Compare
Coverage Report for CI Build 31508182435Coverage increased (+2.2%) to 60.533%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
The chooser and consent screens need to know whether the account's handle was user-chosen or server-generated, so they can decide which identifier to show as primary. That mode was resolved on the way in but lost on the auth-service -> pds-core hop, so the approval step could fall back to showing a generated random handle. Carry epds_handle_mode across the hop, and sign it rather than appending it afterwards: the parameter decides what the approval screen shows, so leaving it unsigned would let the browser flip the presentation of a flow it does not own. Invalid values are dropped at the boundary instead of being forwarded. Adding a field to the signed payload changes the HMAC input, so auth-service and pds-core must be deployed together. In-flight callbacks signed by the old auth-service will fail verification on the new pds-core, and the user retries the sign-in. Also extracts resolveCompleteIdentity() from the /auth/complete handler. Unrelated to handle mode, but the recovery-email lookup was already nested three levels deep in a function this change had to touch anyway. Split out of #148. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bf37444 to
534647d
Compare
|
Split 3 of 4 from #148. Based on #241 — review that first; this diff shows only its own commit once #241 merges.
What it does
The chooser and consent screens need to know whether an account's handle was user-chosen or server-generated, so they can decide which identifier to show as primary (that's split 4).
epds_handle_modewas already being resolved correctly on the way in, and stored on the auth flow — but it was dropped on the auth-service → pds-core hop. By the time the approval step ran, pds-core had no idea what mode the flow was in and fell back to the default, so a flow that asked for a chosen handle could still show a generated random one.This PR carries the already-resolved mode across that hop, signed, and forwards it to the final
/oauth/authorize. Invalid values arriving on the callback are dropped rather than forwarded.Resolution is unchanged
To be clear about scope: this PR does not change how the mode is resolved. That logic lives in
resolveHandleMode()(packages/shared/src/handle.ts) and is untouched here — query parameter beats client metadata beatsEPDS_DEFAULT_HANDLE_MODE, falling back topicker-with-random. What changes is only whether the resolved value survives the hop:/oauth/epds-callback→ pds-core/oauth/authorizeepds_handle_modeis signed rather than appended after signing. The parameter decides what the approval screen shows, so leaving it unsigned would let the browser flip the presentation of a flow it does not own.That means the HMAC input changes:
The empty-string sentinel convention is preserved, which makes the rollout one-directional, not symmetric:
undefined ?? ''. Payloads match.So deploy pds-core before or together with auth-service — never after. In that order there is no broken window. If the order is reversed, in-flight callbacks fail verification until pds-core catches up; the failure mode is a rejected callback and the user retrying sign-in, not a security hole, but it is user-visible.
Per
writing-changesets, the HMAC boundary itself is contributor-facing and gets no changeset; the client-developer-facing behaviour does.Unrelated extraction, flagged
The commit also extracts
resolveCompleteIdentity()from the/auth/completehandler. Nothing to do with handle mode — the recovery-email lookup was already nested three levels deep in a function this change had to touch anyway. Called out here rather than shipped silently; happy to drop it if you'd rather keep this PR single-purpose.Verification
typecheck,lint,formatclean; 76 test files / 1137 tests pass. Tamper rejection and invalid-value dropping are both covered by tests.Series
epds_handle_modethrough the callback hop🤖 Generated with Claude Code