-
Notifications
You must be signed in to change notification settings - Fork 284
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
Trusted types attributes #1268
base: main
Are you sure you want to change the base?
Trusted types attributes #1268
Conversation
This is a clone of #1247 where I will finish any outstanding work to get this across the line |
3562fcb
to
ac5b4aa
Compare
524d8cd
to
f8877b6
Compare
See also #1258 which is another integration point with the DOM spec that we need for TT. |
f8877b6
to
ee9915e
Compare
dom.bs
Outdated
<li><p><a>Validate and set attribute value</a> <var>attr</var>'s <a for="Attr">value</a> for | ||
<var>attr</var> with <var>element</var>. | ||
|
||
<li><p>If <var>element</var> <a lt="has an attribute">has</a> an <a>attribute</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find it difficult to wrap my head around this logic. I think conceptually, we want to have the old value -- from before the call, and thus before a default policy might have mucked with it. That's what should go into the change attribute logic. But once we have that, I'm not sure why we'd need to throw an exception here. I'm not sure why we'd have to care whether the default policy does anything funny with the attribute in the mean time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've read through this spec path and I believe that, if the default policy removes the existing attribute node, then this will call replace an attribute, which in turn calls replace a list item, which results in a no-op because the old value no longer exists to be replaced.
So I think in this situation you're probably right the spec doesn't need to do anything, will make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having said that I'm slightly uneasy about stuff like attribute change steps still firing and how that all works spec wise.
Both Chromium and WebKit don't actually follow the spec 100% here and so I think would actually result in a different behaviour to this spec (the index lookup happens after the TT call) so that's also not ideal.
I think I've addressed all the comments from #1247. I do want to point out Chrome and WebKit don't (or at least not in a way obvious to me) 100% follow the flow of the spec and as a result this may result in differences specifically in weird cases with attribute mutation. So that bit especially it would be good to get feedback on. It's also worth being aware that like Chromium's implementation this spec means that certain ways to update a nodes value don't work with a trusted type object as a user might expect. (e.g. |
ba80870
to
36476b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is either incomplete or makes many cosmetic changes that would be best proposed separately as they confuse me quite a bit.
370f5c9
to
4421d50
Compare
Apologies I thought I'd reverted all of these changes but I missed a few, it's because I changed stuff and then changed it back and this led to some wonky diffs. Have hopefully reverted all of these unnecessary changes |
1d42460
to
1eeaf00
Compare
dom.bs
Outdated
<li><p>If <var>attribute</var>'s <a for=Attr>element</a> <a lt="has an attribute">has</a> | ||
an <a>attribute</a> <var>attribute</var>, then <a>handle attribute changes</a> for | ||
<var>attribute</var> with <var>attribute</var>'s <a for=Attr>element</a>, <var>oldValue</var>, and | ||
<var>value</var>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be value or attribute's value? They can be different, right?
@otherdaniel, @annevk , and @smaug---- regarding the case where the default policy changes assumptions about the existence of an attribute mid-way through what would you prefer the spec say to do? Currently I've specced to throw, but Chromium currently re-looks up the index (spec doesn't explicitly work on an index basis but Chromium and WebKit do) |
Attributes are stored in a list and those do have indices per Infra. What am I missing? |
Sorry I mean algorithmically the spec and implementations don't follow the same flow. So it's trickier to reason between the spec and implementation. This might just be my lack of familiarity with these APIs too. |
The path of least resistance is prolly matching Chromium. Introducing new paths that throw is always risky. If you are looking for guidance as to how, I'd need quite a bit more context to provide helpful suggestions. |
4089eaf
to
02db8c7
Compare
I've partially matched Chromium but gone a bit further with the error correction as Chromium still has some bugs wrt mutating attributes within the default policy. |
@otherdaniel if you've got time could you read through this to ensure it makes sense to you? |
02db8c7
to
55af76e
Compare
@@ -6530,23 +6545,39 @@ string <var>namespace</var> (default null):</p> | |||
|
|||
<div algorithm> | |||
<p>To <dfn export id=concept-element-attributes-set-value>set an attribute value</dfn> given an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This algorithm covers setAttributeNS
<a for=Attr>local name</a> is <var>qualifiedName</var>, <a for=Attr>value</a> is | ||
<var>value</var>, and <a for=Node>node document</a> is <a>this</a>'s <a for=Node>node document</a>, | ||
then <a lt="append an attribute">append</a> this <a>attribute</a> to <a>this</a>, and then return. | ||
<li><p>Let <var>attributeExists</var> be false if <var>attribute</var> is null, and true otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This algorithm covers setAttribute
@@ -7382,7 +7430,13 @@ string <var>value</var>, run these steps: | |||
<li><p>If <var>attribute</var>'s <a for=Attr>element</a> is null, then set <var>attribute</var>'s | |||
<a for=Attr>value</a> to <var>value</var>. | |||
|
|||
<li><p>Otherwise, <a lt="change an attribute">change</a> <var>attribute</var> to <var>value</var>. | |||
<li><p>Otherwise: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers attr.nodeValue, attr.value, and attr.textContent.
@@ -6519,6 +6529,11 @@ string <var>namespace</var> (default null):</p> | |||
|
|||
<li><p>If <var>oldAttr</var> is <var>attr</var>, return <var>attr</var>. | |||
|
|||
<li><p>Let <var>verifiedValue</var> be the result of calling <a>verify attribute value</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers, setAttributeNode, setAttributeNodeNS, setNamedItem and setNamedItemNS
@@ -6870,11 +6918,17 @@ method steps are: | |||
<p>If <var>attribute</var> is null, then: | |||
|
|||
<ol> | |||
<li><p>If <var>force</var> is not given or is true, create an <a>attribute</a> whose | |||
<li><p>Set <var>attribute</var> to an <a>attribute</a> whose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers toggleAttribute. I need to double check this because I think it could still lead to a weird state in it's current form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated this to work how chrome appears to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TT-check in Chromium's toggleAttribute
was unintentional. It came about as a side effect of how toggleAttribute
was implemented on top of other methods. We can't come up with any XSS based on toggleAttribute
. It'd probably be best to remove it from the PR, and that we fix our implementation on our end.
This calls the get Trusted Types-compliant attribute value algorithm from Trusted Types (w3c/trusted-types#418) from attribute's change, append, and replace.
Changed the signature of setAttribute and setAttributeNS to accept Trusted Types as values. The underlying Attr node's values continue to be DOMString, so moving nodes across elements or adding standalone attributes to elements can cause TT violations. This matches WPT tests and the Chromium's implementation.
See and #789. Supercedes #809 and #1247
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff