Added a duplicate attribute present member to element create call #4223

andypaicu · 2018-12-05T14:07:15Z

Added a flag to set during tokenizer parsing step if a duplicate attribute is found.
Changed the create an element call to pass this new flag.

💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:58 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

<html>
<head><title>504 Gateway Time-out</title></head>
<body bgcolor="white">
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>nginx/1.10.3</center>
</body>
</html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

andypaicu · 2018-12-05T14:08:12Z

dom PR: whatwg/dom#721

andypaicu · 2018-12-10T10:34:23Z

@annevk Hi Anne, could I bother you to look at this PR?

annevk

So this approach is much more general than the issue suggested we needed. Instead of passing it off to "create an element", could we not use it if that ends up creating a script element? And then set one of the existing bits on the script element (perhaps renaming it to make more sense)?

source

andypaicu · 2018-12-12T14:19:01Z

I'm now setting it after constructing the element as you suggested. I'm not entirely sure if there's any reason to only set it on script elements though. I know the CSP algorithm only applies to script elements currently but is there any reason not to have this information on all elements?

andypaicu · 2018-12-12T14:20:00Z

The travis CI is failing because I now am referencing concept-element-attributes-append which is part of the DOM PR. Once that lands this should pass too.

annevk · 2018-12-12T14:29:57Z

I'd rather only modify script elements, as we already know they have an internal slot around this, whereas adding a new internal slot to all elements is more invasive (e.g., it requires changing DOM).

What's the reason to expose it on all elements if you can only observe it for script elements?

andypaicu · 2018-12-12T14:39:08Z

I'd rather only modify script elements, as we already know they have an internal slot around this, whereas adding a new internal slot to all elements is more invasive (e.g., it requires changing DOM).

This is news to me. What is this internal slot and where is documented? Or are we talking here about implementations already having this slot?

What's the reason to expose it on all elements if you can only observe it for script elements?

Well currently none really, but in the future I imagine all sorts of elements might be whitelisteable via nonces and we will probably need to have the same safeguards.

annevk · 2018-12-12T14:43:47Z

I assumed the slot discussed in #3257 (comment) was standardized. I'm not entirely sure it is though, but it seems this could also be done with "ready to be parser-executed".

andypaicu · 2018-12-12T15:22:44Z

Hmm in what way could this be done with ready to be parser-executed? I've looked at it for the first time and I don't see it working (but I'm happy to be wrong):

There's logic that seems to rely on ready to be parser-executed being eventually set. Algorithms spin the event loop until a script is available to be executed up to the point where the list of scripts that will execute when the document has finished parsing is empty. (https://html.spec.whatwg.org/#the-end for example).
(Maybe more importantly) a duplicate attribute does not actually mean that the script will never be allowed to execute because of CSP. Sure it can't be nonce blessed but other mechanisms will still allow it to execute.

I take it that changes to DOM are to be avoided if possible so perhaps a new flag similar to ready to be parser-executed on script elements is a better solution?

annevk · 2018-12-12T15:37:11Z

Yeah, I think I'd prefer not generalizing until the need is concrete. Also, I didn't realize it was still somewhat dynamic, in that it depends on a CSP check, in which case I agree that it needs to be a new flag of sorts.

andypaicu · 2018-12-13T14:37:02Z

I've updated this PR and closed the DOM PR.

andypaicu · 2018-12-13T14:39:16Z

There is a test for the duplicate attribute vulnerability:
https://github.com/web-platform-tests/wpt/blob/master/content-security-policy/script-src/nonce-enforce-blocked.html

And I represent implementer interest from the side of Chrome.

annevk

Looks good modulo nits, thanks!

source

andypaicu · 2018-12-13T15:28:53Z

Thank you @annevk I've addressed the nits

source

annevk · 2018-12-13T15:47:00Z

Thanks for driving the editorial work over the finish line. Couple other things to address before we can land this. Hope that's understandable.

Is the CSP logic yet to be written? Although maybe it's already written given there's non-tentative tests? How does that work?

Mike indicated this was discussed at TPAC, but the minutes weren't detailed enough around this particular item to discern interest from others. I'm willing to accept Henri's analysis though as meaning Mozilla would eventually adopt this.

We'll need bugs against Firefox and Safari.

andypaicu · 2018-12-13T16:01:02Z

The CSP logic needs to be reworded to use this flag. Currently it is written around the duplicate-attribute error and has an issue raised against it because there is no actual way to tell if this error occurred. https://w3c.github.io/webappsec-csp/#is-element-nonceable

The nonce hijacking consideration section also mentions this again with an issue raised. (https://w3c.github.io/webappsec-csp/#dangling-markup-attacks)

I should be able to spin a PR pretty quick if it's needed before this PR lands but I obviously can't land that PR in CSP before this one lands.

The reason we have the test is because it was identified as a security vulnerability and we wanted to get it patched asap in Chrome. This is currently already implemented in Chrome (version 72 which I think is just now getting to beta).

I am happy to raise bugs against Firefox and Safari. Should I raise them now or wait until this PR is part of the spec?

annevk · 2018-12-13T16:08:14Z

You can raise those now. And let's do the CSP cleanup PR also now (and just rerun the build after this lands) for completeness as that's what we typically do to make sure a new primitive actually meets the needs of consumers. I'll probably land this tomorrow then unless someone else gets to it first.

andypaicu · 2018-12-13T17:07:25Z

Safari: https://bugs.webkit.org/show_bug.cgi?id=192667
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1513968

CSP PR: w3c/webappsec-csp#377

annevk · 2018-12-14T10:43:32Z

source

@@ -57856,6 +57856,12 @@ o............A....e
  <span data-x="prepare a script">prepared</span>, based on the <code
  data-x="attr-script-src">src</code> attribute of the element at that time.</p>

+  <p>The seventh is a flag indicating whether or not the script has a <dfn
+  data-x="concept-script-duplicate-attribute">duplicate attribute</dfn>. This flag is intially


Thanks to the CSP PR I realized we need to export this. You'll need to add: data-export="" data-dfn-type="dfn" data-dfn-for="script" to the <dfn>.

annevk · 2020-10-08T15:49:34Z

@andypaicu did this ship in Chrome, should this still be standardized?

zcorpan · 2024-02-20T14:34:26Z

This flag is needed for both style and script, right?

andypaicu mentioned this pull request Dec 5, 2018

Added a duplicate attribute present member to element whatwg/dom#721

Closed

annevk reviewed Dec 10, 2018

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

andypaicu added 2 commits December 11, 2018 15:04

Added a duplicate attribute present member to element create call

6d147a5

Setting flag directly after constructing element

654efb4

andypaicu force-pushed the duplicate-attr-flag branch from d77b1e8 to 654efb4 Compare December 12, 2018 14:06

sideshowbarker added normative change needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels Dec 13, 2018

Added a flag on the script element

76b7975

annevk reviewed Dec 13, 2018

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

Addressed feedback nits

ed2aefa

annevk reviewed Dec 13, 2018

View reviewed changes

source Show resolved Hide resolved

Proper spacing around <cite>

9dbf649

annevk approved these changes Dec 13, 2018

View reviewed changes

andypaicu mentioned this pull request Dec 13, 2018

Use the duplicate attribute flag is nonceable check w3c/webappsec-csp#377

Open

annevk reviewed Dec 14, 2018

View reviewed changes

Base automatically changed from master to main January 15, 2021 07:57

domenic force-pushed the main branch from 0947339 to 1b5099f Compare July 27, 2021 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a duplicate attribute present member to element create call #4223

Added a duplicate attribute present member to element create call #4223

andypaicu commented Dec 5, 2018 •

edited by pr-preview bot

Loading

andypaicu commented Dec 5, 2018

andypaicu commented Dec 10, 2018

annevk left a comment

andypaicu commented Dec 12, 2018

andypaicu commented Dec 12, 2018

annevk commented Dec 12, 2018

andypaicu commented Dec 12, 2018 •

edited

Loading

annevk commented Dec 12, 2018

andypaicu commented Dec 12, 2018 •

edited

Loading

annevk commented Dec 12, 2018

andypaicu commented Dec 13, 2018

andypaicu commented Dec 13, 2018

annevk left a comment

andypaicu commented Dec 13, 2018

annevk commented Dec 13, 2018

andypaicu commented Dec 13, 2018

annevk commented Dec 13, 2018

andypaicu commented Dec 13, 2018

annevk Dec 14, 2018 •

edited

Loading

annevk commented Oct 8, 2020

zcorpan commented Feb 20, 2024

Added a duplicate attribute present member to element create call #4223

Are you sure you want to change the base?

Added a duplicate attribute present member to element create call #4223

Conversation

andypaicu commented Dec 5, 2018 • edited by pr-preview bot Loading

💥 Error: Wattsi server error 💥

andypaicu commented Dec 5, 2018

andypaicu commented Dec 10, 2018

annevk left a comment

Choose a reason for hiding this comment

andypaicu commented Dec 12, 2018

andypaicu commented Dec 12, 2018

annevk commented Dec 12, 2018

andypaicu commented Dec 12, 2018 • edited Loading

annevk commented Dec 12, 2018

andypaicu commented Dec 12, 2018 • edited Loading

annevk commented Dec 12, 2018

andypaicu commented Dec 13, 2018

andypaicu commented Dec 13, 2018

annevk left a comment

Choose a reason for hiding this comment

andypaicu commented Dec 13, 2018

annevk commented Dec 13, 2018

andypaicu commented Dec 13, 2018

annevk commented Dec 13, 2018

andypaicu commented Dec 13, 2018

annevk Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

annevk commented Oct 8, 2020

zcorpan commented Feb 20, 2024

andypaicu commented Dec 5, 2018 •

edited by pr-preview bot

Loading

andypaicu commented Dec 12, 2018 •

edited

Loading

andypaicu commented Dec 12, 2018 •

edited

Loading

annevk Dec 14, 2018 •

edited

Loading