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

Prevent [[CryptographicNonce]] from being emptied #5300

Merged
merged 3 commits into from
Feb 20, 2020
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented Feb 18, 2020

And also clarify some prose around the nonce content attribute; fixes #5288.

(See WHATWG Working Mode: Changes for more details.)


💥 Error: EISDIR: illegal operation on a directory, read 💥

PR Preview failed to build. (Last tried on Feb 20, 2020, 9:07 AM UTC).

More

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.

And also clarify some prose around the nonce content attribute; fixes #5288.
@annevk annevk requested review from mikewest and smaug---- February 18, 2020 10:46
Copy link

@smaug---- smaug---- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to me.
The other option would be to never update the slot when content attribute changes but only right before setting content attribute to empty string. But perhaps that isn't backwards compatible if one really wants to use setAttribute to change nonce

@annevk
Copy link
Member Author

annevk commented Feb 18, 2020

It seems that was discussed at #2373 (comment) (and might even have been the original suggestion), but it would require a "weird" IDL attribute that returns either the slot or the content attribute or some such.

And insertion would also have to branch on the IDL attribute already being set or some such. So I think I'm inclined to stick with what we have here.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Feb 18, 2020
Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think I'm inclined to stick with what we have here.

Like @annevk, I'd prefer we stick with the patch he's written here. I agree that this mechanism is somewhat odd, but it's been shipping in Chromium for ~years, and I'd rather not change it if the concerns are aesthetic.

This patch LGTM, thank you both!

@annevk
Copy link
Member Author

annevk commented Feb 19, 2020

@mikewest so @smaug---- spotted one more oddity in the specification and I just wrote a test for it. It seems that Chrome clears the IDL attribute when the nonce attribute is removed, despite the specification not having a hook for removal. I'll change the specification for that too (and adjust the pass condition for that test as well), unless you think Chrome should fix that?

@mikewest
Copy link
Member

Chrome clears the IDL attribute when the nonce attribute is removed

I don't have a strong opinion about it, and would defer to y'all's preferences. I don't think there's any security implication either way.

@annevk
Copy link
Member Author

annevk commented Feb 19, 2020

Made this use the attribute change steps and also account for removal (value will be null).

annevk added a commit to web-platform-tests/wpt that referenced this pull request Feb 19, 2020
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with potential improvement

steps</span> are used for the <code data-x="attr-nonce">nonce</code> content attribute:

<ol>
<li><p>If <var>element</var> does not include <code>HTMLOrSVGElement</code>, then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk annevk merged commit 931ecf4 into master Feb 20, 2020
@annevk annevk deleted the annevk/nonce branch February 20, 2020 09:51
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Feb 24, 2020
…testonly

Automatic update from web-platform-tests
nonce attribute: no longer tentative

For whatwg/html#5300.

Supersedes #5423
--

wpt-commits: 2ca72d0f4b39e6007ae10e78d25f352dab56b2d2
wpt-pr: 21853
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 25, 2020
…testonly

Automatic update from web-platform-tests
nonce attribute: no longer tentative

For whatwg/html#5300.

Supersedes #5423
--

wpt-commits: 2ca72d0f4b39e6007ae10e78d25f352dab56b2d2
wpt-pr: 21853

UltraBlame original commit: f6e9e0c531a648d841cd116948906c025caa4921
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 25, 2020
…testonly

Automatic update from web-platform-tests
nonce attribute: no longer tentative

For whatwg/html#5300.

Supersedes #5423
--

wpt-commits: 2ca72d0f4b39e6007ae10e78d25f352dab56b2d2
wpt-pr: 21853

UltraBlame original commit: f6e9e0c531a648d841cd116948906c025caa4921
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Feb 25, 2020
…testonly

Automatic update from web-platform-tests
nonce attribute: no longer tentative

For whatwg/html#5300.

Supersedes #5423
--

wpt-commits: 2ca72d0f4b39e6007ae10e78d25f352dab56b2d2
wpt-pr: 21853

UltraBlame original commit: f6e9e0c531a648d841cd116948906c025caa4921
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 25, 2020
…testonly

Automatic update from web-platform-tests
nonce attribute: no longer tentative

For whatwg/html#5300.

Supersedes #5423
--

wpt-commits: 2ca72d0f4b39e6007ae10e78d25f352dab56b2d2
wpt-pr: 21853


--HG--
rename : testing/web-platform/tests/content-security-policy/nonce-hiding/script-nonces-hidden.tentative.html.headers => testing/web-platform/tests/content-security-policy/nonce-hiding/script-nonces-hidden.html.headers
rename : testing/web-platform/tests/content-security-policy/nonce-hiding/svgscript-nonces-hidden.tentative.html.headers => testing/web-platform/tests/content-security-policy/nonce-hiding/svgscript-nonces-hidden.html.headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Clarify nonce attribute behavior
4 participants