-
Notifications
You must be signed in to change notification settings - Fork 31
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
Is iframe blocked by default? #124
Comments
I can see that some people may want to allow iframes (though likely not in user-generated content). In contrast to other elements that we don't allow, this at least wouldn't cause XSS (but spoofs, with an iframes executing |
@otherdaniel and I met and agreed that this might be a useful feature, albeit for a later version. FWIW; I can see your use cases of allow-listing origins in CSP (e.g., using |
Does this mean that inserting an iframe using |
Our threat model strictly states that we do not ever want any XSS. The Sanitizer needs to guarantee that. |
Ah, I assumed that was only guaranteed for the default configuration. Good point. I was only wondering why inserting an iframe didn't work with |
@DanielHerr @Nicd If you'd like to experiment with this I've written an implementation of a ~subset of the Sanitizer API with support for iframes, precisely because for my use case I also need to support embedding youtube videos and the like. It's still work in progress though.
@mozfreddyb I think an approach could be to remove the |
I think you are glossing over a couple of things. What about I'm eager to hear your suggestions. |
@mozfreddyb I don't know the right answers to those questions, what I know is that being able to embed some safe iframes is essential for some applications, and safe iframes exist, so that makes the current Sanitizer API unusable for those applications, so that should be addressed. From my point of view unnecessary limitations should be deleted other time.
I don't know how to detect safe data urls, I'm fine with just deleting them all on iframes, that's better than deleting the entire iframe. Once this is figured out I suppose those should be sanitized if possible, and deleted otherwise.
I've never seen that used, I guess it could just be deleted until a more sophisticated logic can be figured out and implemented, if we want to go there.
Possibly this should be configurable. Perhaps more generally a set of origins that can be linked to should exist and all urls matched against that. Perhaps the list of origins should be scoped to specific tag names too. If the iframe is sandboxed I'm not sure whether the url is same-origin or cross-origin matters. Just some thoughts, as of today I can't use the Sanitizer API for my application. At this point it doesn't really matter much to me if this is changed or not, but I think it should be. |
@fabiospampinato Under what circumstances do you need to show iframes from unsafe arbitrary strings of HTML? If you intend to show them, can't they be generated in trusted HTML strings? Or perhaps you could use a tiny wrapper custom HTML element, with "allowCustomElements" set to true in the sanitizer? |
Allowing
I would not consider them safe, but that they do stay shy of direct script execution so perhaps it should be possible. |
We should discuss this. I do tend to agree with @fabiospampinato that this is important. I see these options as the most plausible:
Having written them down, I tend to prefer 1, even though that would mean I would personally end up putting (Some more esoteric options came to mind as well, such that |
We discussed this in the meeting again, though with no immediate consensus. We'll explore a "disallow XSS in the same document only" for now and will see how far that gets us. |
It seems like there are instances where allowing embedding iframes would be desired, like letting users put YouTube videos in a document. There could be an option to allow embedding, maybe also with the ability to whitelist origins, restrict capabilities, etc.
The text was updated successfully, but these errors were encountered: