Skip to content

fix: Add Url validation in Anchor and Page#open#24371

Draft
TatuLund wants to merge 14 commits into
mainfrom
url-validation
Draft

fix: Add Url validation in Anchor and Page#open#24371
TatuLund wants to merge 14 commits into
mainfrom
url-validation

Conversation

@TatuLund
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 19, 2026

Test Results

 1 410 files  ±0   1 410 suites  ±0   1h 20m 45s ⏱️ +40s
10 160 tests ±0  10 091 ✅ ±0  69 💤 ±0  0 ❌ ±0 
10 635 runs  ±0  10 564 ✅ ±0  71 💤 ±0  0 ❌ ±0 

Results for commit bf87464. ± Comparison against base commit 1a88177.

♻️ This comment has been updated with latest results.

public class UrlUtil {

private static final Set<String> ALLOWED_SCHEMES = Set.of(
"http", "https", "mailto", "ftp");
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.

This is going to create really weird and hard to spot bugs for user that e.g. rely on custom schemes to redirect their users to other apps / services and so on :/

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.

Yes, looks like the wrong approach with an "allow" list instead of a "disallow" list

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could be. I could just disallow "javascript".

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.

Yes, and if so, there should be a way to opt-out from the check also, to avoid breaking apps where you actually use javascript: but not combine it with user supplied strings

Copy link
Copy Markdown
Contributor

@knoobie knoobie May 19, 2026

Choose a reason for hiding this comment

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

Example; we use javascript:scrollFocus inside an anchor to create accessible skip links :(

(scrollFocus is a method we have written)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@knoobie in case of Anchor, this check could be overriden by extending Anchor overriding setHref. @Artur- is that enough for opt-out?

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.

Is that consistent with how other similar cases are handled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added override API to PR

TatuLund added 2 commits May 25, 2026 14:16
Updated the disallowed URL schemes to include 'data' and added methods to set custom disallowed schemes and check if a URL is allowed.
@sonarqubecloud
Copy link
Copy Markdown

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

Labels

Projects

Status: 🔎Iteration reviews

Development

Successfully merging this pull request may close these issues.

4 participants