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.

Sign up for GitHub

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

Jump to bottom

Test hidden nonce content attribute behavior. #5423

Closed
wants to merge 3 commits into from
Closed

Test hidden nonce content attribute behavior. #5423

wants to merge 3 commits into from

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented Apr 7, 2017 •
edited by foolip

As specified in whatwg/html#2373.

@mikewest
Copy link
Member Author

mikewest commented Apr 7, 2017

First pass at tests for whatwg/html#2373. What else would y'all like to see, @annevk, @arturjanc, and @zcorpan?

@wpt-pr-bot
Copy link
Collaborator

Notifying @hillbrad. (Learn how reviewing works.)

@annevk
Copy link
Member

annevk commented Apr 7, 2017

This all looks resaonable to me. Wouldn't getComputedStyle be an easier way to test CSS? Or did you specifically want to test that event? It seems with that event you don't know if it failed if the browser decided not to fetch the image for some other reason.

@w3c-bots
Copy link

w3c-bots commented Apr 7, 2017 •
edited

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 91f34f2
Using browser at version BuildID 20170406100208; SourceStamp 3c68d659c2b715f811708f043a1e7169d77be2ba
Starting 10 test iterations
All results were stable

All results

1 test ran
/content-security-policy/script-src/script-nonces-hidden.html
Subtest Results Messages
OK
HTML: Reading 'nonce' content attribute and IDL attribute. FAIL assert_equals: expected Element node <script nonce="abc" id="testScript" executed="yay">\n doc... but got null
HTML: Cloned node retains nonce. FAIL assert_equals: IDL attribute expected (string) "abc" but got (undefined) undefined
HTML: Cloned node retains nonce when inserted. FAIL assert_equals: expected (string) "abc" but got (undefined) undefined
HTML: Writing 'nonce' content attribute. FAIL assert_equals: expected (string) "abc" but got (undefined) undefined
HTML: Writing 'nonce' IDL attribute. PASS
Document-written script executes. PASS
HTML: Document-written script's nonce value. FAIL assert_equals: expected "" but got "abc"
HTML: createElement.nonce. FAIL assert_equals: expected (string) "yay" but got (object) null
HTML: createElement.setAttribute. FAIL assert_equals: Pre-insertion IDL expected (string) "" but got (undefined) undefined
Nonces don't leak via CSS side-channels. FAIL assert_equals: expected "none" but got "url(\"http://web-platform.test:8000/security/resources/abe.png\")"

@w3c-bots
Copy link

w3c-bots commented Apr 7, 2017 •
edited

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 91f34f2
Using browser at version 59.0.3063.4 dev
Starting 10 test iterations

@mikewest
Copy link
Member Author

mikewest commented Apr 7, 2017

Ah, I didn't think about getComputedStyle. :) That's simpler.

@w3c-bots
Copy link

w3c-bots commented Apr 7, 2017 •
edited

These tests are now available on w3c-test.org

@mikewest
Copy link
Member Author

mikewest commented Apr 7, 2017

Updated the last test to use getComputedStyle. Not sure if the requestAnimationFrame is necessary; I have no idea what the timing of CSS style resolution/application is in relation to the current JavaScript task...

@zcorpan zcorpan self-requested a review April 7, 2017 14:14
@annevk
Copy link
Member

annevk commented Apr 7, 2017

getComputedStyle forces style resolution, but the loading of background images is not deterministic, so that event might never fire I suppose.

@annevk
Copy link
Member

annevk commented Apr 7, 2017

(Does it fire on time in browsers today?)

@mikewest
Copy link
Member Author

mikewest commented Apr 7, 2017

It doesn't. :( I'll drop the event. It's just going to be flaky even if it works, and the getComputedStyle bit is good enough in itself.

@mikewest mikewest mentioned this pull request Apr 10, 2017
Merged
zcorpan
zcorpan reviewed Apr 10, 2017
View reviewed changes
content-security-policy/script-src/script-nonces-hidden.html
</style>
<script nonce="abc" id="cssTest">
async_test(t => {
requestAnimationFrame(t.step_func_done(_ => {
Copy link
Member

Choose a reason for hiding this comment

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

No need for rAF. Can just make this a sync test().

zcorpan
zcorpan reviewed Apr 10, 2017
View reviewed changes
content-security-policy/script-src/script-nonces-hidden.html

window.addEventListener('load', t.step_func_done(_ => {
assert_equals(s.nonce, 'abc', "Post-insertion IDL");
assert_equals(s.getAttribute('nonce'), '', "Post-insertion content");
Copy link
Member

Choose a reason for hiding this comment

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

I think the above two should be asserted directly after document.head.appendChild(s);

zcorpan
zcorpan reviewed Apr 10, 2017
View reviewed changes
content-security-policy/script-src/script-nonces-hidden.html

window.addEventListener('load', t.step_func_done(_ => {
assert_equals(s.nonce, 'abc');
assert_equals(s.getAttribute('nonce'), null);
Copy link
Member

Choose a reason for hiding this comment

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

I think the above two should be asserted directly after document.head.appendChild(s);

zcorpan
zcorpan reviewed Apr 10, 2017
View reviewed changes
content-security-policy/script-src/script-nonces-hidden.html
async_test(t => {
var s = document.createElement('script');
s.innerText = script.innerText;
s.nonce = 'abc';
Copy link
Member

Choose a reason for hiding this comment

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

Assert pre-insertion IDL and content attribute here?

@mikewest mikewest changed the title Test nonce-hiding behavior. Test hidden nonce content attribute behavior. Apr 11, 2017
@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the nonce-hiding branch January 24, 2020 18:07
@gsnedders gsnedders restored the nonce-hiding branch January 24, 2020 18:45
@Hexcles Hexcles reopened this Jan 24, 2020
annevk added a commit that referenced this pull request Feb 18, 2020
@annevk
nonce attribute: no longer tentative
878b7c8 
For whatwg/html#5300.

Supersedes #5423.
@annevk annevk mentioned this pull request Feb 18, 2020
Merged
@annevk
Copy link
Member

annevk commented Feb 18, 2020

This is now #21853. I didn't address @zcorpan's nits though and they still seem valid...

@wpt-pr-bot wpt-pr-bot requested a deployment to wpt-preview-5423 February 18, 2020 13:17 Abandoned
@annevk
Copy link
Member

annevk commented Feb 18, 2020

Remaining nits now addressed in the other PR. Some were already addressed.

@annevk annevk closed this Feb 18, 2020
@annevk annevk deleted the nonce-hiding branch February 18, 2020 13:22
annevk added a commit that referenced this pull request Feb 19, 2020
@annevk
nonce attribute: no longer tentative
2ca72d0 
For whatwg/html#5300.

Supersedes #5423
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcorpan zcorpan zcorpan left review comments

Assignees
No one assigned
Labels
content-security-policy status:needs-spec-decision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mikewest @wpt-pr-bot @annevk @w3c-bots @zcorpan @gsnedders @Hexcles