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

How to handle event handler attributes #226

Open
lukewarlow opened this issue Apr 23, 2024 · 6 comments
Open

How to handle event handler attributes #226

lukewarlow opened this issue Apr 23, 2024 · 6 comments

Comments

@lukewarlow
Copy link
Contributor

Afaik the current plan for the sanitizer API is that there would be a list of all known attributes and from that you'd subtract an underlying blocklist of unsafe attributes, event handlers are tricky because there's lots of them. If we have a list of event handlers to remove it would be useful if it was defined separately from other attributes so that the trusted types specification could piggy back off of it.

Having said that all current ones follow the on* pattern, should we just carve out on* attributes as being unsafe and block them all? I'm no perf specialist but I imagine that would be faster to process (both WebKit and Chromium already have some paths that do the on prefix check for example).

@annevk
Copy link
Collaborator

annevk commented Apr 23, 2024

I think from a re-parsing perspective or letting something through in one browser that's blocked in another this would also be an improvement. Although it would also be a tiny bit of magic again that cannot currently be explained through syntax, but that seems reasonable.

@lukewarlow
Copy link
Contributor Author

Has their been any discussion about allowing stuff like regex in the configs? Not needed for a v1 but if we did allow some form of pattern based matching then it would solve the fact we can't currently define it in syntax.

@domenic
Copy link

domenic commented Apr 24, 2024

I worry that this restricts the space of future HTML attribute design in a way that's hard to understand. E.g. we could not use attributes named things like once="", only="", online="", ongoing="", onto="".

Maybe it would still be OK as long as we'd consider it a backward-compatible change to modify the sanitization algorithm in the future. E.g. today we'd strip all attributes starting with on, but in the future when we add an only="" attribute, we strip all attributes starting with on except we let only="" through.

@annevk
Copy link
Collaborator

annevk commented Apr 24, 2024

I think we do have the expectation that what is being sanitized and how will subtly change over time due to the HTML (including SVG/MathML) language evolving. New "safe" attributes getting safelisted seems in line with that.

@otherdaniel
Copy link
Collaborator

I'm not fond of this, and would prefer us to be precise with the event handler attributes.

With Trusted Types, we initially had a similar logic, but kept getting bug reports about sites using their own on* attributes that would get erroneously blocked by TT. So we eventually re-implemented this with the actual list of actual event handlers, which solves that set of problems. I'd prefer to not repeat that experience.

Examples:

Also, I don't quite understand why this would be needed at all: Our default-config -- where these matter -- are allow-lists, and in an allow-lists the event handlers are very much expected to be absent. That is, no on* attribute -- whether a browser-defined event handler or not -- should show up in any built-in config. So why would we care? Or why would it make a perf difference?

For the config pre-processing, we do need to decide whether an attribute is known or not -- otherwise one could never override the built-in default. That indeed requires a list of known attributes, including the known event handlers. But that's required only during config normalization; and that requires every single HTML attribute ever. I doubt the event handlers make that much of a difference there.

@annevk
Copy link
Collaborator

annevk commented Apr 24, 2024

Fair enough. List it is then.

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

No branches or pull requests

4 participants