Skip to content
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

Add RequireSandboxOnIFrame #136

Merged
merged 3 commits into from Dec 31, 2021
Merged

Add RequireSandboxOnIFrame #136

merged 3 commits into from Dec 31, 2021

Conversation

kiwiz
Copy link
Contributor

@kiwiz kiwiz commented Dec 28, 2021

WIP implementation of RequireSandboxOnIFrame.

Open questions:

  • Should RequireSandboxOnIFrame internally call p.AllowAttrs("sandbox").OnElements("iframe"), or should we leave that to the user?
  • When an input <iframe> does NOT have a sandbox attribute, the entire tag is omitted entirely. However, what we actually want to do is keep the tag and enforce a blank sandbox="" attribute. (Might be related to add iframe to default elements without attributes #68?)
  • This implementation will not dedupe values in the input sandbox attribute (ex: allow-downloads allow-downloads -> allow-downloads allow-downloads). Is that something we should do?

Closes #135

@buro9
Copy link
Member

buro9 commented Dec 30, 2021

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
Copy link
Contributor Author

kiwiz commented Dec 30, 2021

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. 👍

@buro9
Copy link
Member

buro9 commented Dec 31, 2021

Sounds sane to me, will merge and release.

@buro9 buro9 merged commit ce0adc5 into microcosm-cc:main Dec 31, 2021
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.

Support for the <iframe> sandbox attribute
2 participants