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

Add RequireSandboxOnIFrame #136

Closed kiwiz closed 2 years ago

kiwiz commented 2 years ago

WIP implementation of RequireSandboxOnIFrame.

Open questions:

Closes #135

buro9 commented 2 years ago

This looks good and would merge as-is if you want.

To answer your questions though:

  1. Should we call it implicitly? No... don't surprise people. Just add it and have it as a documented func to be called. You might consider a helper to add iframes, and that could call both p.AllowAttrs("sandbox").OnElements("iframe") and RequireSandboxOnIFrame()... but I have avoided doing anything implicit outside of the helpers (see https://github.com/microcosm-cc/bluemonday/blob/main/helpers.go#L176-L189 as an example... it isolates complexity to the helpers and keeps the core interface simple and unsurprising).
  2. Should we enforce a blank sandbox? Hmm... if there are zero side effects for HTML that didn't previously have it then sure... I'm up for that. However, if you have the slightest doubt just rely on a helper and encourage people to use a helper called AllowIFrames to do the job for you (see the first point)
  3. Should we dedupe values? Probably... but that's a nit, I'm not sure a browser will care. But probably.

All-in-all... a nice addition, and yes there's a nit, and yes there's scope for a simple helper to add iframe support which would encapsulate the recommendations... but the core PR looks fine, so the choice is up to you, should I merge and release or would you like to add the helper and dedupe and then I'll merge, etc

kiwiz commented 2 years ago

Thanks for the feedback! 1 & 3 are addressed in the new commits and I think 2 might be a non issue.

Just to be explicit though:

Calling RequireSandboxOnIFrame will add sandbox="" to all iframe tags that don't have it, which would change the behaviour of the iframe (but is the point of this feature).

However, if the input HTML has an empty <iframe></iframe>, that gets removed entirely. I don't think an empty iframe tag can be used for anything interesting. Some JS could manipulate it down the line, but you'd expect an id or class attr to be populated.

If that sounds sane to you, then I think this is ready to go. :+1:

buro9 commented 2 years ago

Sounds sane to me, will merge and release.