Apply browser security headers on SP-initiated SAML endpoints by edewit · Pull Request #54159 · keycloak/keycloak

edewit · 2026-10-20T10:35:37Z

Return Response synchronously from /protocol/saml GET/POST (including
artifact resolution) so KeycloakSecurityHeadersFilter can apply realm
browser security headers. Previously @suspended AsyncResponse closed the
session before the filter ran, omitting CSP and X-Frame-Options.

Closes #54122

@suspended

Return Response synchronously from /protocol/saml GET/POST (including artifact resolution) so KeycloakSecurityHeadersFilter can apply realm browser security headers. Previously @suspended AsyncResponse closed the session before the filter ran, omitting CSP and X-Frame-Options. Closes keycloak#54122 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Copilot

🟡 Changes recommended

Artifact resolution loses HTTP-worker isolation and lacks security-header regression coverage.

2 open issues

What changed in this PR

Addresses #54122 by returning SAML responses synchronously so Keycloak’s security-header filter can apply realm browser protections.

Changes:

Converts GET/POST and artifact resolution to synchronous response handling.
Adds security-header tests and Redirect-binding test support.

File	Description
tests/base/src/test/java/org/keycloak/tests/saml/SamlSloSessionHijackTest.java	Adapts to the generalized request type.
tests/base/src/test/java/org/keycloak/tests/saml/SamlSecurityHeadersTest.java	Tests POST and Redirect security headers.
tests/base/src/test/java/org/keycloak/tests/saml/SamlClient.java	Adds Redirect-binding request support.
services/src/main/java/org/keycloak/protocol/saml/SamlService.java	Replaces asynchronous SAML response handling.

_{Review effort: Balanced}

Copilot

🟡 Changes recommended

The rewritten artifact-resolution lifecycle lacks regression coverage verifying security headers.

3 open issues

_{Review effort: Balanced}

lhanusov

@edewit thanks for this fix, I've added one comment to address, also, can you please check those created by Copilot? thanks 👍

Restore async artifact SOAP I/O so a slow SP does not block shared HTTP workers, while still applying realm browser security headers before AsyncResponse resume for SP-initiated SAML endpoints. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Copilot

🟡 Changes recommended

Header handling bypasses logout-specific CSP options and leaves rollback-generated artifact error pages unprotected.

4 open issues

1 resolved since last review

Artifact resolution blocks shared HTTP workers on slow SOAP endpoints

_{Review effort: Balanced}

Route async SAML header application through SecurityHeadersProvider so allowFrameSrc from FrontChannelLogoutHandler is not overwritten by raw realm defaults. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Copilot

🔵 Needs a closer look

The security-sensitive HTML error-response path still needs GET and POST regression coverage.

3 open issues

1 previously missed issue

In code that hasn't changed since last review

Add GET and POST tests for security headers on SAML parameter errors

services/src/main/java/org/keycloak/protocol/saml/SamlService.java:803

Add GET and POST regression tests that omit all SAML parameters and assert status 400 plus the realm's configured security headers on the HTML error page. Both new tests send valid AuthnRequests, leaving this separately decorated early-error path untested even though error pages are explicitly part of #54122.

1 resolved since last review

Preserve logout iframe options when applying security headers

_{Review effort: Balanced}

lhanusov · 2026-10-22T11:59:46Z

@edewit Hi Erik, the artifact worker pool and logout CSP handling look good now. There’s still one gap: if the artifact transaction fails to commit, AsyncResponseTransaction.rollback() replaces the response with a fresh 500 error page, losing the security headers applied here.

Could we apply the headers after the final response is selected and add a regression test for that rollback path? The current tests only send SAMLRequest, so they don’t cover artifact resolution.

Thank you.

lhanusov

please, check my comment

…e final response Rollback-generated artifact error pages were missing realm browser security headers because they replaced an already-decorated success response. Apply headers on commit/rollback after the final response is chosen, and cover the artifact resolution rollback path in SamlSecurityHeadersTest. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

edewit · 2026-10-30T07:44:55Z

@lhanusov Thanks — addressed in 387b52b.

AsyncResponseTransaction now applies browser security headers to the response that is actually resumed after commit/rollback selects it, so a rollback-generated 500 still gets the realm headers. Added testBrowserSecurityHeadersOnArtifactResolutionRollback covering the artifact path through that transaction.

Copilot

🟢 Approval recommended

No active review issues remain for the SAML security-header changes.

No open issues 🎉

3 resolved since last review

_{Review effort: Balanced}

Mutating Response.getHeaders() is not propagated by Quarkus REST on AsyncResponse resume. Rebuild the Response via Response.fromResponse so realm security headers are present on the wire. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Copilot

🟡 Changes recommended

Generated security headers append to existing CSP values, causing browsers to enforce intersecting policies that may block resources or frames permitted by the realm policy.

1 open issue

Replace existing CSP headers before adding generated policies · New

_{Review effort: Balanced}

…onses Resolve SamlClient conflict by keeping both unsigned-response helpers from main and artifact-request helpers from this branch. Clear copied header values before applying realm browser security headers so CSP is replaced rather than duplicated. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Copilot

🟢 Approval recommended

No active review issues were identified for the SAML security-header changes.

No open issues 🎉

1 resolved since last review

Replace existing CSP headers before adding generated policies

_{Review effort: Balanced}

lhanusov

LGTM @edewit thank you for the fix

rmartinc

Thanks @edewit and @lhanusov!

@suspended

…ak#54159) * Apply browser security headers on SP-initiated SAML endpoints Return Response synchronously from /protocol/saml GET/POST (including artifact resolution) so KeycloakSecurityHeadersFilter can apply realm browser security headers. Previously @suspended AsyncResponse closed the session before the filter ran, omitting CSP and X-Frame-Options. Closes keycloak#54122 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * Keep SAML artifact resolution on saml-artifact-pool Restore async artifact SOAP I/O so a slow SP does not block shared HTTP workers, while still applying realm browser security headers before AsyncResponse resume for SP-initiated SAML endpoints. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * Preserve front-channel logout CSP when applying SAML security headers Route async SAML header application through SecurityHeadersProvider so allowFrameSrc from FrontChannelLogoutHandler is not overwritten by raw realm defaults. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * Apply SAML security headers after AsyncResponseTransaction selects the final response Rollback-generated artifact error pages were missing realm browser security headers because they replaced an already-decorated success response. Apply headers on commit/rollback after the final response is chosen, and cover the artifact resolution rollback path in SamlSecurityHeadersTest. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * Rebuild SAML responses when applying browser security headers Mutating Response.getHeaders() is not propagated by Quarkus REST on AsyncResponse resume. Rebuild the Response via Response.fromResponse so realm security headers are present on the wire. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> --------- Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

#55043) Closes #54122 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> (cherry picked from commit 42a3b96)

Copilot AI balanced review requested due to automatic review settings October 20, 2026 10:35

edewit requested a review from a team as a code owner October 20, 2026 10:35

keycloak-github-bot Bot added the team/core-protocols label Oct 20, 2026

Copilot started reviewing on behalf of edewit October 20, 2026 10:36

View session

edewit force-pushed the fix-54122-saml-security-headers branch from f5df67f to 87c60c0 Compare October 20, 2026 10:37

Copilot AI reviewed Oct 20, 2026

View reviewed changes

Comment thread services/src/main/java/org/keycloak/protocol/saml/SamlService.java Resolved

Comment thread services/src/main/java/org/keycloak/protocol/saml/SamlService.java Resolved

Copilot AI balanced review requested due to automatic review settings October 20, 2026 10:40

Copilot started reviewing on behalf of edewit October 20, 2026 10:40

View session

Copilot AI reviewed Oct 20, 2026

View reviewed changes

Comment thread services/src/main/java/org/keycloak/protocol/saml/SamlService.java Resolved

lhanusov suggested changes Oct 21, 2026

View reviewed changes

Comment thread services/src/main/java/org/keycloak/protocol/saml/SamlService.java Resolved

Copilot AI balanced review requested due to automatic review settings October 21, 2026 14:24

Copilot started reviewing on behalf of edewit October 21, 2026 14:26

View session

Copilot AI reviewed Oct 21, 2026

View reviewed changes

Comment thread services/src/main/java/org/keycloak/protocol/saml/SamlService.java Resolved

Comment thread services/src/main/java/org/keycloak/protocol/saml/SamlService.java Resolved

edewit requested a review from lhanusov October 21, 2026 14:58

Preserve front-channel logout CSP when applying SAML security headers

ab192ad

Route async SAML header application through SecurityHeadersProvider so allowFrameSrc from FrontChannelLogoutHandler is not overwritten by raw realm defaults. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>

Copilot AI balanced review requested due to automatic review settings October 21, 2026 15:20

Copilot started reviewing on behalf of edewit October 21, 2026 15:26

View session

Copilot AI reviewed Oct 21, 2026

View reviewed changes

lhanusov suggested changes Oct 22, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 30, 2026 07:44

Copilot started reviewing on behalf of edewit October 30, 2026 07:45

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 30, 2026 08:05

Copilot started reviewing on behalf of edewit October 30, 2026 08:05

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

Comment thread services/src/main/java/org/keycloak/headers/SecurityHeadersUtils.java Resolved

Copilot AI balanced review requested due to automatic review settings October 30, 2026 10:46

Copilot started reviewing on behalf of edewit October 30, 2026 10:47

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

lhanusov approved these changes Oct 30, 2026

View reviewed changes

rmartinc approved these changes Oct 30, 2026

View reviewed changes

rmartinc merged commit 42a3b96 into keycloak:main Oct 30, 2026
97 checks passed

mposolda pushed a commit that referenced this pull request Nov 12, 2026

Apply browser security headers on SP-initiated SAML endpoints (#54159) (

4ee3b55

#55043) Closes #54122 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> (cherry picked from commit 42a3b96)

Navigation Menu

Repository navigation

Apply browser security headers on SP-initiated SAML endpoints - #54159

Apply browser security headers on SP-initiated SAML endpoints#54159
rmartinc merged 6 commits into
keycloak:mainfrom
edewit:fix-54122-saml-security-headers

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants