fix(core): honor ForceAuthn in SAML application sign-in - #9563
fix(core): honor ForceAuthn in SAML application sign-in#9563Igor-Techsee wants to merge 9 commits into
Conversation
COMPARE TO
|
| Name | Diff |
|---|---|
| .changeset/quiet-owls-remember.md | 📈 +1.28 KB |
| packages/core/src/libraries/saml-application/saml-applications.test.ts | 📈 +1.02 KB |
| packages/core/src/libraries/saml-application/saml-applications.ts | 📈 +103 Bytes |
| packages/core/src/libraries/saml-application/utils.ts | 📈 +601 Bytes |
| packages/core/src/oidc/utils.test.ts | 📈 +304 Bytes |
| packages/core/src/oidc/utils.ts | 📈 +82 Bytes |
| packages/core/src/queries/saml-application/index.ts | 📈 +153 Bytes |
| packages/core/src/routes/saml-application/anonymous.ts | 📈 +191 Bytes |
| packages/core/src/routes/saml-application/index.openapi.json | 📈 +3.63 KB |
| packages/core/src/routes/saml-application/index.test.ts | 📈 +1.83 KB |
| packages/core/src/routes/saml-application/index.ts | 📈 +100 Bytes |
| packages/core/src/routes/saml-application/saml-app-auth-flow.openapi.json | 📈 +654 Bytes |
| packages/core/src/saml-application/SamlApplication/mocks/saml-application.ts | 📈 +1.5 KB |
| packages/core/src/saml-application/SamlApplication/authn-policy.test.ts | 📈 +1.32 KB |
| packages/core/src/saml-application/SamlApplication/callback.test.ts | 📈 +3.63 KB |
| packages/core/src/saml-application/SamlApplication/consts.ts | 0 Bytes |
| packages/core/src/saml-application/SamlApplication/index.test.ts | 📈 +261 Bytes |
| packages/core/src/saml-application/SamlApplication/index.ts | 📈 +1.49 KB |
| packages/core/src/saml-application/SamlApplication/signature.test.ts | 📈 +5.7 KB |
| packages/core/src/saml-application/SamlApplication/utils.test.ts | 📈 +1.47 KB |
| packages/core/src/saml-application/SamlApplication/utils.ts | 📈 +2.35 KB |
| packages/schemas/alterations/next-1788946735-add-saml-authn-request-config.ts | 📈 +469 Bytes |
| packages/schemas/src/foundations/jsonb-types/saml-application-configs.test.ts | 📈 +1.16 KB |
| packages/schemas/src/foundations/jsonb-types/saml-application-configs.ts | 📈 +732 Bytes |
| packages/schemas/src/types/saml-application.ts | 📈 +28 Bytes |
| packages/schemas/tables/saml_application_configs.sql | 📈 +64 Bytes |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, consistently applied across both SAML bindings, and includes targeted unit coverage for the new parsing and URL-building behavior.
Pull request overview
This PR updates Logto’s SAML IdP flow to only force an interactive login (prompt=login in the internal OIDC authorization request) when the SAML service provider explicitly requests re-authentication via ForceAuthn="true" on the incoming AuthnRequest, bringing SAML app sign-in behavior in line with OIDC apps that reuse existing Logto sessions.
Changes:
- Add
isForceAuthnRequested()to readForceAuthnfrom the decoded AuthnRequest XML and treattrue/1as enabled. - Update
SamlApplication.getSignInUrl()to appendprompt=loginonly whenforceAuthnis true. - Wire the flag through both SAML authn endpoints (Redirect + POST bindings) and add unit tests + a changeset.
File summaries
| File | Description |
|---|---|
| packages/core/src/saml-application/SamlApplication/utils.ts | Adds isForceAuthnRequested() using samlify’s extractor to read the ForceAuthn attribute. |
| packages/core/src/saml-application/SamlApplication/utils.test.ts | Adds tests covering ForceAuthn absent/true/1/false cases. |
| packages/core/src/saml-application/SamlApplication/index.ts | Makes prompt=login conditional in getSignInUrl() via the new forceAuthn parameter. |
| packages/core/src/saml-application/SamlApplication/index.test.ts | Adds tests validating getSignInUrl() includes no prompt by default and adds it when forceAuthn is true. |
| packages/core/src/routes/saml-application/anonymous.ts | Extracts forceAuthn from the parsed request and passes it to getSignInUrl() for both bindings. |
| .changeset/quiet-owls-remember.md | Adds a minor changeset describing the user-visible behavior change for @logto/core. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| const { forceAuthn } = saml.Extractor.extract(authnRequestXml, [ | ||
| { key: 'forceAuthn', localPath: ['AuthnRequest'], attributes: ['ForceAuthn'] }, | ||
| ]); |
…ion in SP-initiated SSO Stacked on logto-io#9563. Fixes logto-io/logto#9569 (isPassive ignored: SP forces an interactive login when the IdP requests passive auth) and logto-io/logto#9570 (assertions accepted without SubjectConfirmation bearer validation).
Stacked on logto-io#9563. Fixes logto-io/logto#9569 (IsPassive ignored: passive requests get an interactive sign-in instead of prompt=none, and the failure mode is a sign-in page instead of a NoPassive status response) and logto-io/logto#9570 (<saml:Subject> ignored: the asserted NameID can belong to a different principal instead of returning an UnknownPrincipal status).
f6f2969 to
2ad2fd5
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped, align with the SAML ForceAuthn semantics, and are covered by targeted unit tests for both URL construction and XML extraction.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes authentication/session reuse behavior and adds signature-verification enforcement paths, which are security-sensitive and warrant final human review despite good test coverage.
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new SAML policy/API surface and runtime behavior are not guarded by isDevFeaturesEnabled, and the corresponding OpenAPI dev-feature markers are missing.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/schemas/src/foundations/jsonb-types/saml-application-configs.ts:58
- This introduces a new public SAML policy/API surface and changes the default sign-in behavior without an
isDevFeaturesEnabledguard. The repository convention requires new features to be gated by that flag by default (and the OpenAPI supplement supportsDev feature/x-logto-dev-featuremarkers); otherwise every tenant receives this surface immediately. Please either guard the policy/runtime/docs as a dev feature or make the intended release exception explicit before shipping.
.changeset/quiet-owls-remember.md:6
- This PR adds a new Management API policy and new signed-request behavior, but neither the routes/schema nor the SAML flow is guarded by
isDevFeaturesEnabledwhile this changeset publishes the feature. The repository convention is to guard new features by default (the IdP-initiated SSO routes do this inpackages/core/src/routes/sso-connector/index.ts:378-381); please add the corresponding guards and OpenAPI dev-feature markers, or split this so only the released bug fix remains here.
"@logto/core": minor
"@logto/schemas": minor
---
honor `ForceAuthn` in SAML application authentication requests
- Files reviewed: 26/26 changed files
- Comments generated: 0 new
- Review effort level: Lite
When Logto acts as a SAML identity provider, `SamlApplication.getSignInUrl` always added `prompt=login` to the internal OIDC authorization request, so a user who already had a Logto session was sent to the sign-in page again for every SAML application. OIDC applications reuse the session. Send `prompt=login` only when the service provider's `AuthnRequest` carries `ForceAuthn="true"` (SAML 2.0 core, section 3.4.1). samlify's default login-request extractor does not read that attribute, so `isForceAuthnRequested` reads it from the decoded request XML; both authn endpoints (redirect and POST binding) pass the result to `getSignInUrl`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The service provider initiates this flow, so the browser reaches the callback through a cross-site navigation. `strict` withholds the cookie for every hop of that navigation, so once an existing session skips the sign-in page the callback finds no session and fails with `sp_initiated_saml_sso_session_not_found_in_cookies`. Matching the OIDC `state` against the stored session covers CSRF.
33235c7 to
0f7d9f9
Compare
Existing and new SAML applications keep forcing fresh authentication, as they did before ForceAuthn was honored. Only an explicit `authnRequestConfig.forceAuthn: false` reuses an existing session, so a config replaced without the field falls back to forcing rather than to session reuse.
There was a problem hiding this comment.
🔵 Needs a closer look
Add feature gating and map token-verification failures to the established invalid-token error path.
Review details
Suppressed comments (3)
packages/core/src/routes/saml-application/index.openapi.json:60
- These new SAML authentication-policy fields are an unguarded new feature: the changed API/schema/flow contains no
isDevFeaturesEnabledcheck, and the corresponding OpenAPI supplement is not markedDev feature. This exposes and persists the new Management API in production despite the repository's feature-flag policy; add the guard and matching OpenAPI marker, or otherwise split the released bug fix from this feature.
"authnRequestConfig": {
"type": "object",
"nullable": true,
"description": "IdP-side authentication policy. Null or an empty object allows session reuse unless the SP requests ForceAuthn. PATCH replaces this object.",
"properties": {
"forceAuthn": {
"type": "boolean",
"default": false,
"description": "Require fresh authentication for every sign-in to this application, even when the SP omits ForceAuthn."
},
"requireSignedAuthnRequests": {
"type": "boolean",
"default": false,
"description": "Require signed AuthnRequests for both HTTP-POST and HTTP-Redirect bindings. Defaults to false."
},
"signingCertificate": {
"type": "string",
"description": "PEM-encoded RSA X.509 signing certificate of the service provider. Required when requireSignedAuthnRequests is true. Used independently of the assertion encryption certificate."
}
packages/core/src/saml-application/SamlApplication/index.ts:261
- The new ID-token verification path lets
jwtVerify()and theauth_timeschema parse throw raw JOSE/Zod errors. A malformed or invalid token response therefore reacheskoa-error-handleras a 500 instead of the existingoidc.invalid_tokenerror path used for token-response and subject validation. Catch this verification/claim-parsing block and map failures consistently.
const { payload } = await jwtVerify(idToken, this.envSet.oidc.localJWKSet, {
issuer: this.issuer,
audience: this.samlApplicationId,
requiredClaims: ['sub', 'iat', 'exp', 'auth_time'],
});
packages/schemas/src/foundations/jsonb-types/saml-application-configs.ts:59
- This adds a new Management API policy and changes the released SAML authentication behavior, but the new surface is not gated by
isDevFeaturesEnabled(and the OpenAPI properties are not marked as dev features). New features in this codebase are expected to remain behind that guard until release; please add the runtime/API/OpenAPI guard, or split the release-only changes into a follow-up. The fix spans multiple files, so an inline replacement is not safe.
export const samlAuthnRequestConfigGuard = z
.object({
forceAuthn: z.boolean().optional(),
requireSignedAuthnRequests: z.boolean().optional(),
signingCertificate: z.string().trim().min(1).optional(),
})
- Files reviewed: 26/26 changed files
- Comments generated: 0 new
- Review effort level: Lite
Show the "Always force authentication" setting with a tooltip in the SAML application settings. Saving carries over the request signature settings, since PATCH replaces `authnRequestConfig`.
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate unresolved findings remain around the ForceAuthn default and dev-feature gating.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
packages/schemas/src/types/saml-application.ts:20
- This adds a new Management API property and UI behavior directly to the production surface without the repository's dev-feature gate. Unreleased behavior elsewhere is gated with
EnvSet.values.isDevFeaturesEnabled(for examplepackages/core/src/routes/sso-connector/index.ts:378-381), and the Swagger builder removesDev featureoperations when the flag is disabled (packages/core/src/routes/swagger/utils/general.ts:263-267). Please gate this feature consistently until release, or explicitly treat it as released rather than exposing it unconditionally.
- Files reviewed: 47/47 changed files
- Comments generated: 1
- Review effort level: Lite
| export const isSamlForceAuthnEnabled = (config?: SamlAuthnRequestConfig | null): boolean => | ||
| config?.forceAuthn !== false; |
Summary
SAML applications currently send users through fresh authentication for every sign-in, even when they already have a Logto session. This change reuses that session unless the service provider requests
ForceAuthnor the application's authentication policy requires a fresh sign-in.trueand1, including surrounding XML whitespace.auth_time.authnRequestConfig.forceAuthnto the SAML application Management API so administrators can require fresh authentication independently of the service provider.authnRequestConfig.requireSignedAuthnRequestsandsigningCertificatesettings. When enabled, verify HTTP-POST and HTTP-Redirect requests against the configured SP certificate, require POST signatures to cover the enclosing request, and advertise the requirement in IdP metadata.Upgrade behavior
Session reuse is the default. Applications that need the previous always-authenticate behavior can set
authnRequestConfig.forceAuthntotrue. Signed requests remain optional by default; enabling the requirement also requires a PEM-encoded RSA X.509 SP signing certificate. UpdatingauthnRequestConfigreplaces the policy object.IsPassiveand requestedSubjecthandling remain follow-up TODOs.Testing
Unit tests