microcosm-cc / bluemonday

bluemonday: a fast golang HTML sanitizer (inspired by the OWASP Java HTML Sanitizer) to scrub user generated content of XSS
https://github.com/microcosm-cc/bluemonday
BSD 3-Clause "New" or "Revised" License
3.12k stars 176 forks source link

AllowNoAttrs doesn't work with Matching #147

Closed MrParano1d closed 2 years ago

MrParano1d commented 2 years ago

If I have a custom web component in my HTML, it is deleted by the sanitizer even though the rule p.AllowElementsMatching(regexp.MustCompile("^custom-")) is defined.

It's deleted because allowNoAttrs matches.

A rule like p.AllowNoAttrs().Matching(regexp.MustCompile("^custom-")) is also ignored here, because in

func (p *Policy) allowNoAttrs(elementName string) bool { in sanitize.go

only the static map p.setOfElementsMatchingAllowedWithoutAttrs is iterated through and the matching is ignored.

My input looks like this:

<h1 id="headline-1">Headline 1</h1>
<p>Some text</p>
<p>
    Some More Text
</p>
<custom-component>
    Some Component
</custom-component>

The policy

p := bluemonday.NewPolicy()
    p.AllowElements("h1", "p")
    p.AllowElementsMatching(regexp.MustCompile(`^custom-`))
    p.AllowNoAttrs().Matching(regexp.MustCompile(`^custom-`))

makes it

<h1>Headline 1</h1>
<p>Some text</p>
<p>
    Some More Text
</p>

    Some Component

However, if I use

p := bluemonday.NewPolicy()
    p.AllowElements("h1", "p")
    p.AllowElementsMatching(regexp.MustCompile(`^custom-`))
    p.AllowNoAttrs().OnElements("custom-component")

and adding the p.AllowNoAttrs() to a static element, then I get the correct output:

<h1>Headline 1</h1>
<p>Some text</p>
<p>
    Some More Text
</p>
<custom-component>
    Some Component
</custom-component>
MrParano1d commented 2 years ago

I'm using v1.0.18.

buro9 commented 2 years ago

Try this:

p.AllowNoAttrs().Matching(regexp.MustCompile(`^custom-`)).OnElementsMatching(regexp.MustCompile(`^custom-`))

The problem is that the .Matching() returns an attributeBuilder and that needs to be bound to a set of elements to test, so merely adding .OnElements() or .OnElementsMatching() resolves this.

I've added a test anyway, because examples are good and complex behaviour is where regressions may occur and I'll catch that if it does regress.