feat(core,phrases): enforce the pure step-up route allow-list - #9580
feat(core,phrases): enforce the pure step-up route allow-list#9580wangsijie wants to merge 5 commits into
Conversation
COMPARE TO
|
| Name | Diff |
|---|---|
| packages/core/src/routes/experience/index.test.ts | 📈 +4.52 KB |
| packages/core/src/routes/experience/index.ts | 📈 +104 Bytes |
| packages/core/src/routes/experience/middleware/koa-step-up-route-guard.test.ts | 📈 +9 KB |
| packages/core/src/routes/experience/middleware/koa-step-up-route-guard.ts | 📈 +2.37 KB |
| packages/core/src/routes/experience/verification-routes/password-verification.test.ts | 📈 +814 Bytes |
| packages/core/src/routes/experience/verification-routes/password-verification.ts | 📈 +227 Bytes |
| packages/core/src/routes/experience/verification-routes/verification-code.ts | 📈 +616 Bytes |
| packages/integration-tests/src/tests/api/experience-api/step-up-interaction/routes.test.ts | 📈 +4.5 KB |
| packages/integration-tests/src/tests/api/experience-api/step-up-interaction/subject-verification.test.ts | 📈 +240 Bytes |
| packages/integration-tests/src/tests/api/oidc/acr-interaction-policy.test.ts | 📈 +118 Bytes |
| packages/phrases/src/locales/ar/errors/session.ts | 📈 +111 Bytes |
| packages/phrases/src/locales/de/errors/session.ts | 📈 +96 Bytes |
| packages/phrases/src/locales/en/errors/session.ts | 📈 +81 Bytes |
| packages/phrases/src/locales/es/errors/session.ts | 📈 +90 Bytes |
| packages/phrases/src/locales/fa-ir/errors/session.ts | 📈 +104 Bytes |
| packages/phrases/src/locales/fr/errors/session.ts | 📈 +96 Bytes |
| packages/phrases/src/locales/it/errors/session.ts | 📈 +89 Bytes |
| packages/phrases/src/locales/ja/errors/session.ts | 📈 +106 Bytes |
| packages/phrases/src/locales/ko/errors/session.ts | 📈 +89 Bytes |
| packages/phrases/src/locales/pl-pl/errors/session.ts | 📈 +86 Bytes |
| packages/phrases/src/locales/pt-br/errors/session.ts | 📈 +86 Bytes |
| packages/phrases/src/locales/pt-pt/errors/session.ts | 📈 +86 Bytes |
| packages/phrases/src/locales/ru/errors/session.ts | 📈 +147 Bytes |
| packages/phrases/src/locales/th/errors/session.ts | 📈 +199 Bytes |
| packages/phrases/src/locales/tr-tr/errors/session.ts | 📈 +92 Bytes |
| packages/phrases/src/locales/zh-cn/errors/session.ts | 📈 +70 Bytes |
| packages/phrases/src/locales/zh-hk/errors/session.ts | 📈 +70 Bytes |
| packages/phrases/src/locales/zh-tw/errors/session.ts | 📈 +70 Bytes |
wangsijie
left a comment
There was a problem hiding this comment.
Two follow-ups on the route allow-list — one behavioral, one documentation. Neither blocks the approach, which looks right to me: the deny-by-default matrix in koa-step-up-route-guard.test.ts and the end-to-end step-up-interaction/routes.test.ts make the policy easy to verify.
Generated by Claude Code
af42e8b to
e2f75b7
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The route guard can be bypassed on specific endpoints, and related 403 responses are undocumented.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds pure step-up route allow-list enforcement, rejects raw-identifier verification payloads, and adds localized error messages.
Changes:
- Adds deny-by-default middleware and tests.
- Restricts password and verification-code flows.
- Adds
forbidden_routetranslations across 18 locales.
File summaries
| File | Reviewed change |
|---|---|
packages/phrases/src/locales/zh-tw/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/zh-hk/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/zh-cn/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/tr-tr/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/th/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/ru/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/pt-pt/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/pt-br/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/pl-pl/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/ko/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/ja/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/it/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/fr/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/fa-ir/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/es/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/en/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/de/errors/session.ts |
Adds localized error text. |
packages/phrases/src/locales/ar/errors/session.ts |
Adds localized error text. |
packages/integration-tests/src/tests/api/oidc/acr-interaction-policy.test.ts |
Updates step-up verification coverage. |
packages/integration-tests/src/tests/api/experience-api/step-up-interaction/subject-verification.test.ts |
Tests raw-identifier rejection. |
packages/integration-tests/src/tests/api/experience-api/step-up-interaction/routes.test.ts |
Tests the route allow-list. |
packages/core/src/routes/experience/verification-routes/verification-code.ts |
Rejects raw identifiers during pure step-up. |
packages/core/src/routes/experience/verification-routes/password-verification.ts |
Rejects raw-identifier passwords. |
packages/core/src/routes/experience/verification-routes/password-verification.test.ts |
Tests password restrictions. |
packages/core/src/routes/experience/middleware/koa-step-up-route-guard.ts |
Adds deny-by-default route enforcement. |
packages/core/src/routes/experience/middleware/koa-step-up-route-guard.test.ts |
Tests route enforcement. |
packages/core/src/routes/experience/index.ts |
Registers the route guard. |
packages/core/src/routes/experience/index.test.ts |
Tests route behavior. |
Review details
Suppressed comments (3)
packages/core/src/routes/experience/verification-routes/password-verification.ts:295
- The new pure-step-up branch returns
403 session.step_up.forbidden_routewhen a raw identifier is supplied, butverification-routes/password-verification.step-up.openapi.jsonstill documents only the 404 subject-missing case. Add this dev-feature 403 response so clients can discover the new request restriction.
assertThat(
!experienceInteraction.isStepUp || !identifier,
new RequestError({ code: 'session.step_up.forbidden_route', status: 403 })
);
packages/core/src/routes/experience/verification-routes/verification-code.ts:80
- Both new raw-identifier guards return
403 session.step_up.forbidden_route, butverification-routes/verification-code.step-up.openapi.jsonstill documents only the subject/record 404 cases for the send and verify operations. Add the dev-feature 403 responses to that supplement so the API contract matches these branches.
assertThat(
!experienceInteraction.isStepUp || identifierPayload.value === undefined,
new RequestError({ code: 'session.step_up.forbidden_route', status: 403 })
);
packages/phrases/src/locales/tr-tr/errors/session.ts:55
- The Turkish sentence is grammatically incorrect:
izin vermektakes the dative case, soBu yol ... izin verilmezshould usebu yola. Please use the corrected wording below.
forbidden_route: 'Bu yol, yükseltilmiş kimlik doğrulama sırasında izin verilmez.',
- Files reviewed: 28/28 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…oute guard The guard read the `ExperienceInteraction` instance, which `koaExperienceInteraction` never builds for the routes it whitelists, so `GET /experience/sso-connectors` and the passkey sign-in preflight skipped the deny-by-default allow-list, and the preflight route still wrote `signInPasskey` into the interaction record. Classify the mode from the interaction record `koaInteractionDetails` provides: the login prompt details, falling back to the stored result an interaction is restored from. Add `PUT /experience` to the allow-list, since creating and re-mounting a step-up interaction goes through it. Document the step-up 403 of the passkey sign-in verify operation, which the allow-list now denies before its identity check.
There was a problem hiding this comment.
🟡 Changes recommended
Critical route-ordering and path-normalization issues, plus contract and test gaps, must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (1) — in code that hasn't changed since the last review.
packages/phrases/src/locales/tr-tr/errors/session.ts:55
- This Turkish sentence is grammatically incorrect:
izin vermekrequires the dative case, soBu yol ... izin verilmezshould usebu rotaya(orbu yola).
packages/core/src/routes/experience/middleware/koa-step-up-route-guard.ts:59
- Because this assertion rejects every non-allow-listed Experience route, it introduces
session.step_up.forbidden_routefor the social/SSO, TOTP, backup-code, new-password, one-time-token, profile/MFA, avatar, and anonymous passkey/SSO routes. Their OpenAPI documents still omit this 403 response; the only supplement changed here covers sign-in-passkey verification, while the create and preflight routes (and the other deny-listed operations) remain undocumented. Add dev-feature supplements or 403 responses for all deny-listed operations so the generated API contract matches runtime behavior.
assertThat(
allowedStepUpRoutes.has(`${ctx.method.toUpperCase()} ${path}`),
new RequestError({ code: 'session.step_up.forbidden_route', status: 403 })
packages/core/src/routes/experience/verification-routes/password-verification.ts:295
- Although
/verification/passwordis on the route allow-list, this new assertion deliberately returns 403session.step_up.forbidden_routefor a raw identifier in pure step-up.password-verification.step-up.openapi.jsonstill documents only the 404 response, so the dev-feature contract omits a response clients must handle. Add the 403 response to that supplement.
assertThat(
!experienceInteraction.isStepUp || !identifier,
new RequestError({ code: 'session.step_up.forbidden_route', status: 403 })
);
packages/core/src/routes/experience/verification-routes/verification-code.ts:80
- The allow-list does not make every payload on this route successful: this assertion now returns 403
session.step_up.forbidden_routefor raw identifiers, and the verify handler has the same check.verification-code.step-up.openapi.jsonstill lists only 404 for both operations, so its dev-feature contract is stale. Add the 403 response to both operations.
assertThat(
!experienceInteraction.isStepUp || identifierPayload.value === undefined,
new RequestError({ code: 'session.step_up.forbidden_route', status: 403 })
);
packages/core/src/routes/experience/verification-routes/verification-code.ts:71
- This 403 is only produced for the dev-feature pure step-up path, but it is added unconditionally to the guard status.
koaGuarduses this list to generate the base OpenAPI operation, so disabling dev features still exposes this step-up-only response on the released verification-code endpoint. Gate 403 withEnvSet.values.isDevFeaturesEnabled, as the password and WebAuthn routes do, and apply the same change to the verify route's status list.
// 403: a pure step-up supplied a raw identifier
// 404: subject-bound variant without a subject; 429: rate limited; 501: connector not found
status: [200, 400, 403, 404, 422, 429, 501],
packages/core/src/routes/experience/verification-routes/web-authn-verification.step-up.openapi.json:21
- The route catalog also denies
POST /api/experience/verification/sign-in-passkey/authenticationin pure step-up, but this supplement adds the 403 only to/.../authentication/verify. A client invoking the first step now receives the samesession.step_up.forbidden_routewhile its operation has no dev-feature 403 contract. Add the sibling operation and response to this supplement.
"description": "Passkey sign-in is not available in a pure step-up: the route is not on the step-up route allow-list and fails with `session.step_up.forbidden_route` before the credential is verified."
packages/integration-tests/src/tests/api/experience-api/step-up-interaction/subject-verification.test.ts:197
- This replacement removes the end-to-end assertion for identifying an account different from the pinned subject. The PR description still claims integration coverage for “identifying another account in step-up is 403”, but this suite now has no
session.identity_conflictassertion; retain or add a separate cross-account identification test so the subject-pinning behavior remains covered at the API boundary.
it('rejects raw identifier payloads in pure step-up', async () => {
const client = await signInWithPassword(user);
await startStepUp(client);
await expectRejects(
- Files reviewed: 30/30 changed files
- Comments generated: 2
- Review effort level: Lite
The guard re-parsed the whole interaction storage on every experience request to read the mode, duplicating the parse just done to build the interaction, and read an unparseable record as "not a step-up", which lifted the restriction. Read `ctx.experienceInteraction.isStepUp` wherever the instance exists, and keep a fallback that parses only `authenticationContext` for the three whitelisted routes, failing closed with 404 when the stored record cannot be read.
There was a problem hiding this comment.
🔵 Needs a closer look
The documented 403 responses and Turkish localization still need correction.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
packages/core/src/routes/experience/verification-routes/password-verification.ts:278
- This new 403 is reachable even though the password path is allow-listed: the handler rejects any value-bearing identifier in pure step-up. However,
password-verification.step-up.openapi.jsonstill documents only the subject-missing 404, so the dev-feature contract omitssession.step_up.forbidden_route. Add that 403 response to the supplement.
packages/core/src/routes/experience/verification-routes/verification-code.ts:71 - Both verification-code handlers now return
session.step_up.forbidden_routefor a raw identifier even though the routes themselves are allow-listed, butverification-code.step-up.openapi.jsondocuments only the 404 cases. Add the new 403 response to both send and verify operations so the dev-feature contract matches the runtime behavior.
packages/core/src/routes/experience/verification-routes/web-authn-verification.step-up.openapi.json:21 - The route allow-list also rejects
POST /api/experience/verification/sign-in-passkey/authentication, but this supplement adds a 403 only for the/verifyoperation. The base WebAuthn supplement has no step-up 403 for the create operation, so clients can still receive an undocumentedsession.step_up.forbidden_route; add the matching 403 response for the create route as well.
packages/phrases/src/locales/tr-tr/errors/session.ts:55 - The Turkish sentence is grammatically incorrect:
izin vermektakes the dative case, soBu yolshould beBu yola; the wording can also use the natural possessive form for authentication. Please use the corrected translation.
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Stacked on #9575 (subject-bound first-factor verification, itself stacked on #9567). Implements the M3 route restrictions from LOG-14106.
session.step_up.forbidden_route. A SignIn with a requested ACR is not restricted.session.step_up.forbidden_routeacross all 18 locales.Resolves LOG-14106.
Testing
Unit tests
Integration tests
Checklist
.changeset(dev feature; M8 adds the changeset)