Rewrite metadata functions #457
Conversation
|
|
||
| <table> | ||
| <thead> | ||
| <tr><th>Element<th>Property name<th>TrustedType |
There was a problem hiding this comment.
I feel like this table needs a baseVal property for the SVGAnimatedString type somehow, not sure on the specifics of that SVG integration (I also am not sure that baseVal is accounted for in Chromium if it is required in this list).
lukewarlow
force-pushed
the
metadata-rewrite
branch
2 times, most recently
from
436bc9e
to
862c806
Compare
lukewarlow
force-pushed
the
metadata-rewrite
branch
from
862c806
to
6362f3a
Compare
|
@mbrodesser-Igalia do you think you'd be able to give this a review? |
If this could wait until I actually start implementing it in Gecko, that might be most effective. |
Thorough reviews are time-consuming. |
lukewarlow
force-pushed
the
metadata-rewrite
branch
2 times, most recently
from
d2d9227
to
648f6b2
Compare
lukewarlow
force-pushed
the
metadata-rewrite
branch
from
3d67bea
to
b2dfa87
Compare
lukewarlow
force-pushed
the
metadata-rewrite
branch
from
7197ef1
to
7fa73e4
Compare
- getAttributeType and getPropertyType now use lookup tables.
lukewarlow
force-pushed
the
metadata-rewrite
branch
from
7fa73e4
to
123ce3a
Compare
| To <dfn abstract-op>Get Trusted Type data for attribute</dfn> given |element|, |attribute|, |attributeNs|, perform the following steps: | ||
|
|
||
| 1. Let |data| be null. | ||
| 1. If |attributeNs| is null, and |attribute| is the name of an [=event handler content attribute=], then: |
There was a problem hiding this comment.
I'm not really sure how you envision this lookup to work. There are many event handler content attributes that are not applicable to elements. And some only apply to a specific element.
There was a problem hiding this comment.
There are many event handler content attributes
I don't fully understand what you mean by this? What event handler content attribute doesn't apply to an element?
Do you mean legacy ones no longer applicable or something like that?
And some only apply to a specific element.
So for the purposes of trusted types I don't think we mind being overly restrictive here, it seems fine to consider them independent of the element (this is the case in Chromium's implementation)?
For context the plan in WebKit is to use HTMLElement::eventNameForEventHandlerAttribute with a null check.
There was a problem hiding this comment.
onreadystatechange?
I'm not sure we have a specification concept for eventNameForEventHandlerAttribute.
There was a problem hiding this comment.
Chrome enforces the TT on that attribute for a div element for example, so that aspect is fine.
There was a problem hiding this comment.
Can you clarify how that is fine?
How about onloadend?
There was a problem hiding this comment.
If I understand correctly the issue would be that you require a trusted object to set an inline event listener that is "safe" for a given element. Given inline event listeners are bad practice and a strict csp would already block them from actually running I don't think blocking "harmless" but otherwise useless (like are people actually going to be trying to set onreadystatechange on an arbitrary element?) attributes from being set from a string is fine?
There was a problem hiding this comment.
onloadend also requires a trusted script to be set on div
There was a problem hiding this comment.
What about onblah?
My worry here is about the hook not being defined in sufficient detail for an interoperable implementation. Implementing "is an event handler content attribute" requires knowing about all event handler attributes in existence, not just those defined in HTML.
There was a problem hiding this comment.
onblah is allowed. I think it's using IDL to generate the list from attribute EventHandlers. onrepeat for example which as far as I can tell is only SVG related is being blocked.
There was a problem hiding this comment.
To close the loop there's a discussion about just using on* pattern to block anything that resembles an event listener. WICG/sanitizer-api#226
Fixes #456 and #423, and #474
Preview | Diff