8000 Prevent kcSanitize bypass that reactivates URLs with JavaScript scheme on the login page by michalvavrik · Pull Request #53886 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Prevent kcSanitize bypass that reactivates URLs with JavaScript scheme on the login page - #53886

Merged
rmartinc merged 1 commit into
keycloak:mainfrom
michalvavrik:in-review/53813
Oct 14, 2026
Merged

Prevent kcSanitize bypass that reactivates URLs with JavaScript scheme on the login page#53886
rmartinc merged 1 commit into
keycloak:mainfrom
michalvavrik:in-review/53813

Conversation

@michalvavrik
@michalvavrik michalvavrik commented Oct 12, 2026
Copy link
Copy Markdown
Member

@michalvavrik
michalvavrik requested a review from a team as a code owner October 12, 2026 22:36
Copilot AI balanced review requested due to automatic review settings October 12, 2026 22:36
Copilot AI reviewed Oct 12, 2026
View reviewed changes
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The parser-sensitive security fix warrants final human validation against browser URL parsing behavior.

0 open findings

What changed in this PR

Hardens login-theme HTML sanitization against numeric-character-reference URL scheme bypasses.

Changes:

  • Revalidates decoded links against allowed protocols.
  • Adds sanitizer regression tests for encoded JavaScript/VBScript schemes.
  • Adds login-page integration coverage while preserving benign markup.
File Description
services/​src/​main/​java/​org/​keycloak/​theme/​KeycloakSanitizerMethod.java Decodes numeric entities and rejects disallowed schemes.
services/​src/​test/​java/​org/​keycloak/​theme/​KeycloakSanitizerTest.java Tests malicious and legitimate sanitizer inputs.
tests/​base/​src/​test/​java/​org/​keycloak/​tests/​forms/​LoginPageDisplayNameHtmlSanitizationTest.java Verifies rendered login-page sanitization.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

mabartos
mabartos reviewed Oct 13, 2026
View reviewed changes
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalvavrik Thanks! Agree to go with this approach, as I don't think we can find better approach to not also break the email templates and custom display names.

I've added some suggestions to improve tests and have the full coverage. Let me know WDYT.

Comment thread tests/base/src/test/java/org/keycloak/tests/forms/LoginPageDisplayNameHtmlSanitizationTest.java Resolved
Comment thread services/src/test/java/org/keycloak/theme/KeycloakSanitizerTest.java Resolved
Comment thread tests/base/src/test/java/org/keycloak/tests/forms/LoginPageDisplayNameHtmlSanitizationTest.java Resolved
@michalvavrik
prevent kcSanitize from re-activating disallowed URL schemes after li…
1c5e0d3 
…nk fixup

closes: github.com/keycloak/issues/53813

Signed-off-by: Michal Vavřík <dev@michalvavrik.net>
Copilot AI balanced review requested due to automatic review settings October 13, 2026 09:49
Copilot AI reviewed Oct 13, 2026
View reviewed changes
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The sanitizer closes the reported reactivation path and includes focused unit and integration coverage.

0 open findings

🧠 Review effort: Balanced

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Oct 13, 2026
View reviewed changes
@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@keycloak-github-bot
keycloak-github-bot Bot commented Oct 13, 2026
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.cluster.ComponentInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertFalse.failNotFalse(AssertFalse.java:63)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:36)
...

Report flaky test

@michalvavrik
michalvavrik requested a review from mabartos October 13, 2026 11:32
mabartos
mabartos approved these changes Oct 13, 2026
View reviewed changes
@mabartos mabartos left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalvavrik LGTM, thanks!

@michalvavrik
michalvavrik requested a review from rmartinc October 13, 2026 13:53
rmartinc
rmartinc approved these changes Oct 14, 2026
View reviewed changes
@rmartinc rmartinc left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @michalvavrik, @mabartos and @vaceksimon! LGTM! I have been doing some tests and I think I cannot trick the html display now. I don't like the idea that sometimes we create link with empty href and sometimes no link at all, but we cannot go into owasp sanitizer to change it.

@rmartinc
rmartinc merged commit 3e0fda9 into keycloak:main Oct 14, 2026
97 checks passed
@michalvavrik
michalvavrik deleted the in-review/53813 branch October 14, 2026 08:42
@michalvavrik michalvavrik mentioned this pull request Oct 14, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@rmartinc rmartinc rmartinc approved these changes
@mabartos mabartos mabartos approved these changes
+2 more reviewers
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
@vaceksimon vaceksimon vaceksimon approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/core-authn

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CVE-2026-107889] Stored XSS on the login page via kcSanitize bypass

5 participants

@michalvavrik @rmartinc @mabartos @vaceksimon
0