1466508 - Fix race condition within wpt test policy-inherited-correctly-by-plznavigate.html

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Christoph Kerschbaumer [:ckerschb]]

As discussed within [1] the srcdoc frame might not be loaded before the document.write happens, we should fix that race condition!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=965637#c43

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1466508_fix_race_within_wpt_test.patch

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c71a38c8c5c
Fix race condition within wpt test policy-inherited-correctly-by-plznavigate.html. r=jgraham

Comment 3

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11325 for changes under testing/web-platform/tests

Comment 4

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The new test is racy in a different way: The code in the script can run _after_ the load event from the frame has fired, in which case none of the test will get run....

Comment 6

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0c71a38c8c5c

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1466508_fix_race_within_wpt_test_part2.patch

Boris, thanks for taking another look here. Here is a new version, what do you think?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8983166 [details] [diff] [review]
bug_1466508_fix_race_within_wpt_test_part2.patch

This looks better, yes.

That said, do we need to make sure that the <script async defer> bit runs _after_ all this stuff?  I haven't looked into exactly what that does, but I'm a bit suspicious.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1466508_fix_race_within_wpt_test_part2.patch

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> That said, do we need to make sure that the <script async defer> bit runs
> _after_ all this stuff?  I haven't looked into exactly what that does, but
> I'm a bit suspicious.

Boris, most of the web platform tests simply use <script async defer ...> for testing the CSP error reporting within a test. I looked through the various wpt tests in our tree and also found [1] which generates that script using JS. Since we are already on this test file, we might also just do the same to make sure there is no race within this test.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/object-src/object-src-2_2.html#47-50

Comment 11

[Boris Zbarsky [:bzbarsky]]

> most of the web platform tests simply use <script async defer ...> for testing the CSP error
> reporting within a test.

Right.  Is that supported by the spec?  That is, does the spec require such a script to run after the CSP reporting bits have happened?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8983340 [details] [diff] [review]
bug_1466508_fix_race_within_wpt_test_part2.patch

r=me

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> > most of the web platform tests simply use <script async defer ...> for testing the CSP error
> > reporting within a test.
> 
> Right.  Is that supported by the spec?  That is, does the spec require such
> a script to run after the CSP reporting bits have happened?

Sorry, I think I can't follow what you mean. The wpt tests use a script to query information from a pseudo server to check if CSP reports were send correctly. The spec only requires policy reports to be send when a resource load was blocked by CSP, not in what order that needs to happen.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac57eb7e7aa
Fix race condition within wpt test policy-inherited-correctly-by-plznavigate.html part 2. r=bz

Comment 15

[Boris Zbarsky [:bzbarsky]]

> The wpt tests use a script to query information from a pseudo server to check if CSP reports were send correctly.

Right.  My question is whether the spec requires the reports to be sent and received on the server before this script would hit the server with its query.  If not, then the tests are all racy and we should get a bug filed on them upstream or something...

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> > The wpt tests use a script to query information from a pseudo server to check if CSP reports were send correctly.
> 
> Right.  My question is whether the spec requires the reports to be sent and
> received on the server before this script would hit the server with its
> query.  If not, then the tests are all racy and we should get a bug filed on
> them upstream or something...

I am pretty sure all those tests suffer from that race condition. I filed Bug 1467155 to get that updated and fixed.

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/fac57eb7e7aa

Comment 18

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11416 for changes under testing/web-platform/tests