Skip to content

fix(saml): use connection samlAudienceOverride as Issuer in SLO LogoutRequest - #4075

Open
LeighBriody24 wants to merge 1 commit into
ory:mainfrom
LeighBriody24:fix/slo-samlaudienceoverride-issuer
Open

fix(saml): use connection samlAudienceOverride as Issuer in SLO LogoutRequest#4075
LeighBriody24 wants to merge 1 commit into
ory:mainfrom
LeighBriody24:fix/slo-samlaudienceoverride-issuer

Conversation

@LeighBriody24

@LeighBriody24 LeighBriody24 commented Aug 10, 2026

Copy link
Copy Markdown

Fixes #4076

Summary

When Polis builds the SAML LogoutRequest, it uses the global samlAudience as the <saml:Issuer> and ignores the connection's samlAudienceOverride. The login path uses the override (connection.samlAudienceOverride ?? samlAudience), so any connection that sets samlAudienceOverride sends a different Issuer at login and at logout.

Whether that breaks logout depends on the IdP. Strict IdPs (Microsoft Entra / Azure AD in particular) reject it, because the Issuer doesn't match the participant recorded at login, returning AADSTS50068 and leaving the IdP session open (screenshot below). Lenient IdPs accept the mismatch, which is why it isn't always visible.

image

What does this PR do?

For SLO to work, login and logout have to send the same SP Issuer. The login path already handles this: it uses connection.samlAudienceOverride ?? samlAudience. The logout path doesn't; it always sends opts.samlAudience.

This change makes logout read samlAudienceOverride off the connection when building the LogoutRequest, falling back to the global audience just like login does. Connections without an override are unaffected.

Problem

LogoutController.createRequest built the request like this:

providerName: this.opts.samlAudience!,

When a connection has a samlAudienceOverride, the IdP records that override as the session participant at login. Logout then arrives claiming the global audience instead, so the IdP has no matching participant to sign out (the error above).

Solution

const {
  idpMetadata: { slo, provider },
  samlAudienceOverride,
} = samlConnection;

// ...
providerName: samlAudienceOverride ?? this.opts.samlAudience!,

Same behaviour as the login path. SAMLConnection also gains the optional samlAudienceOverride field.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

Two cases added to test/sso/logout.test.ts:

  • With samlAudienceOverride set, the LogoutRequest <saml:Issuer> is the override value.

  • Without it, the Issuer falls back to the global samlAudience (no regression).

  • New and existing unit tests pass locally with my changes

  • Existing unit tests

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes

…tRequest

Single Logout fails for any SAML connection with a samlAudienceOverride set.
The LogoutRequest was built with the global samlAudience as its <saml:Issuer>,
ignoring the per-connection override used at login. Strict IdPs (e.g. Microsoft
Entra) reject the mismatched Issuer with AADSTS50068, leaving the session open.

Read samlAudienceOverride off the connection when building the LogoutRequest,
with the same fallback to the global audience as the login path. Connections
without an override are unaffected.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The SAML connection supports an optional audience override. Logout request generation uses the override when configured and falls back to the global audience. Tests decode the compressed request and verify both issuer values.

Changes

SAML logout audience selection

Layer / File(s) Summary
Audience override contract and logout validation
npm/src/typings.ts, npm/src/controller/logout.ts, npm/test/sso/logout.test.ts
SAMLConnection accepts samlAudienceOverride. Logout requests use this value when present and use samlAudience otherwise. Tests inflate and decode requests to verify both issuer paths.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: deepakprabhakara

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the SAML logout fix and the use of the connection-specific audience override.
Description check ✅ Passed The description explains the problem, solution, testing, issue reference, change type, and relevant checklist items.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@deepakprabhakara

Copy link
Copy Markdown
Collaborator

Thanks @LeighBriody24, the team will review your PR soon.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Single Logout uses global samlAudience as Issuer instead of the connection's samlAudienceOverride

3 participants