8000 Allow target attribute for anchor tags in html-sanitizer by rr-it · Pull Request #42700 · keycloak/keycloak · GitHub
[go: up one dir, main page]

Allow target attribute for anchor tags in html-sanitizer - #42700

Merged
ahus1 merged 2 commits into
keycloak:mainfrom
rr-it:fix/target-for-achors
Sep 19, 2025
Merged

ahus1 merged 2 commits into
keycloak:mainfrom
rr-it:fix/target-for-achors

Conversation

@rr-it
@rr-it rr-it commented Sep 17, 2025
Copy link
Copy Markdown
Contributor

kcSanitize allows for a target attribute in anchor tags.

Especially on the registration or accept terms of service page the links to terms of service must open in a new window. If the link opens in the same window, the user is thrown out of the registration or login process.

Therefore the html-sanitizer must not remove the target attribute from anchor tags like:
<a href="…" target="_blank">…</a>

This fixes #28846

@rr-it
rr-it requested a review from a team as a code owner September 17, 2025 14:11
@rr-it
rr-it force-pushed the fix/target-for-achors branch from 9a2dba4 to b621b02 Compare September 17, 2025 14:24
@ahus1 ahus1 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.

Thank you for this PR for this issue that has several upvotes. Please see below for a suggested change.

@@ -102,6 +102,7 @@ public class KeycloakSanitizerPolicy {
.allowStandardUrlProtocols()
.allowAttributes("nohref").onElements("a")
.allowAttributes("name").matching(NAME).onElements("a")
.allowAttributes("target").matching(NAME).onElements("a")
@ahus1 ahus1 Sep 17, 2025 •
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.

I would like to be more restrictive here and not just allow any target, but only _blank. This is the most common case that is being asked for, and also avoid opening content in an already named window.

When you add a change to allow only _blank, please add also a test that shows that other targets are not allowed.

@ahus1 ahus1 self-assigned this Sep 17, 2025
@rr-it
rr-it force-pushed the fix/target-for-achors branch from b621b02 to 53cb6bf Compare September 18, 2025 15:31
@ahus1 ahus1 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.

Thank you for the PR. I slightly adjusted the test case, as the sanitation library will only add the additional rel values when the target is accepted.

@ahus1
ahus1 enabled auto-merge (squash) September 19, 2025 07:00
@ahus1
ahus1 commented Sep 19, 2025
Copy link
Copy Markdown
Member

Waiting for #42752 to be fixed first

rr-it and others added 2 commits September 19, 2025 10:40
Signed-off-by: Ayke Halder <rr-it@users.noreply.github.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
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.

Allow the target attribute on <a> in the kcSanitize

2 participants

0