OWASP / java-html-sanitizer

Takes third-party HTML and produces HTML that is safe to embed in your web application. Fast and easy to configure.
Other
851 stars 214 forks source link

Attributes allowed globally together with "style" are lost #237

Open corebonts opened 2 years ago

corebonts commented 2 years ago

Since the commit 020d5d0d7b8e985be32d3608612a9889135ef060 all attributes that are allowed globally are ignored, if "style" is given as the first attribute.

Problematic code:

    public HtmlPolicyBuilder globally() {
      if(attributeNames.get(0).equals("style")) {
        return allowStyling();
      } else {
        return HtmlPolicyBuilder.this.allowAttributesGlobally(
            policy, attributeNames);
      }
    }

Proof

 @Test
  public static final void testStyleWithOtherAttributesGlobally() {
    PolicyFactory policyBuilder = new HtmlPolicyBuilder()
            .allowAttributes("style", "align").globally()
            .allowElements("a", "label", "h1", "h2", "h3", "h4", "h5", "h6")
            .toFactory();
    String input = "<h1 style=\"color:green ;name:user ;\" align=\"center\">This is some green text</h1>";
    String want = "<h1 style=\"color:green\" align=\"center\">This is some green text</h1>";
    assertEquals(want, policyBuilder.sanitize(input));
  }

Note that align="center" is missing from the output.

I will file a PR to fix the issue

sgesin commented 2 years ago

Another problem that we're seeing with 020d5d0 is that it assumes the attributeNames list is non-empty. The proposed change in b493617 would help prevent having to write empty list checks prior to calling globally.

corebonts commented 2 years ago

That's true. I got some comments on my pull request but I will try to adjust it today so hopefully these things will be fixed soon.