Skip to content

Added notes about XSS when using URIFragments directly #4416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 3, 2025
Merged

Conversation

edler-san
Copy link
Contributor

No description provided.

@edler-san edler-san requested a review from Copilot July 2, 2025 16:33
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds security guidance to prevent XSS when using URI fragments and shows how to sanitize them in examples.

  • Documents the risk of enabling HTML in notifications and points to sanitization guidance.
  • Updates the advanced URI-fragment example to recommend sanitization before use.
  • Changes a servlet example to use a sanitizeContent method and provides its stub.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
articles/framework/articles/AccessingWebPageAndBrowserInformation.asciidoc Adds a note about XSS risk when HTML is enabled in notifications
articles/framework/advanced/advanced-urifu.asciidoc Recommends sanitizing fragment, updates servlet example to call sanitizeContent and adds its stub
Comments suppressed due to low confidence (4)

articles/framework/articles/AccessingWebPageAndBrowserInformation.asciidoc:41

  • [nitpick] This note about enabling HTML in notifications appears under the URI fragment listener section and may confuse readers. Consider moving it to a dedicated notifications page or clarifying its relevance here.
Note: HTML is disabled by default, but can be enabled with

articles/framework/advanced/advanced-urifu.asciidoc:111

  • Recommend specifying a concrete sanitization or escaping function (for example, StringEscapeUtils.escapeHtml4) and linking to the sanitization guide to help readers implement it correctly.
        // it is good practice to not use the fragment directly but to sanitize it first

articles/framework/advanced/advanced-urifu.asciidoc:178

  • The sanitizeContent stub needs a concrete implementation and corresponding unit tests to verify that inputs are properly escaped before rendering.
    private String sanitizeContent(String s){

articles/framework/advanced/advanced-urifu.asciidoc:178

  • [nitpick] The method name sanitizeContent is generic; consider renaming it to escapeHtml or sanitizeHtmlFragment for clearer intent.
    private String sanitizeContent(String s){

@@ -155,7 +157,7 @@ public class MyCustomServlet extends VaadinServlet
Writer writer = response.getWriter();
writer.append("<html><body>"+
"<p>Here is some crawlable "+
"content about " + fragment + "</p>");
"content about " + sanitizeContent(fragment) + "</p>");
Copy link
Preview

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

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

Instead of a custom stub, consider using a well-tested HTML escaping library (e.g., Apache Commons Text's StringEscapeUtils.escapeHtml4) to ensure robust XSS protection.

Copilot uses AI. Check for mistakes.

@ZheSun88 ZheSun88 merged commit daf0ba5 into v8 Jul 3, 2025
1 check passed
@ZheSun88 ZheSun88 deleted the v8-xss-warning branch July 3, 2025 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants