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
HTML: Test ordering of COEP reporting vs CSP and X-Frame-Options #28281
Conversation
Did this change since whatwg/html#5859? @jugglinmike |
https://wpt.fyi/results/html/browsers/browsing-the-web/navigating-across-documents/failure-check-sequence.https.html?label=master&label=experimental&aligned still passes in Chrome, so I guess the order did not change. The other possibility is that the COEP check happens even though the CSP check has already disallowed the response. |
That is my suspicion at this point; I've asked around for code pointers to see if I can confirm. @yutakahirano is probably the best person to determine what the desired behavior is here. |
Sorry for the late reply. Chromium's implementation is here. CSP seems to be checked earlier but I may be wrong. @ArthurSonzogni , can you take a look? |
You compared COEP and CSPEE (CSP embedded enforcement). The real CSP check is later For now, in Chrome after receiving the navigation response:
For now, my reading of the spec is:
|
Thanks @ArthurSonzogni ! So yeah, it seems there's a discrepancy between what's implemented in chromium and what's in the spec. (allowedToDownload is checked after the others, in step 7.) I'll file an issue for whatwg/html |
Following the current spec: checking COEP in between CSP and X-Frame-Option seems complex, because we currently check both CSP:frame-ancestors and X-Frame-Option together in Chrome. They are about the exact same thing. Your test is very useful! |
@ArthurSonzogni if you think the test matches what the spec currently requires, approving the PR so we can merge it would be useful. Thanks! I imagine whatwg/html#6564 will require more tests to be written, but doesn't need to be in this PR. |
I have no OWNER powers on github, but I can say your patch looks good to me. I would like the specification to be different in order to be able to implement it. However your patch does indeed match my reading of the spec. So +1. |
@ArthurSonzogni I've invited you to the reviewers team which should give you write access and ability to approve PRs with GitHub's review functionality in the "Files changed" tab. |
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.
Thanks! LGTM!
When receiving a navigation response, the current HTML specification checks policies in the following order: - Check CSP - Check COEP/CORS - Check X-Frame-Options X-Frame-Options and CSP:frame-ancestors are very close. They serve the same purpose. CSP:frame-ancestors is the same thing, but more flexible. It overrides X-Frame-Options when defined. They are also warning being displayed to developers when they are using unreasonable combination of both. The check about COEP in between CSP and X-Frame-Options is unfortunately badly placed. This patch propose checking it one step earlier. Related issues:: - zcorpan@ tested ordering: web-platform-tests/wpt#28281 - zcorpan@ opened an issue about inconsistencies about the specification and Chromimum whatwg#6564
See https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-response step 4
Chromium seems to issue the COEP report even when CSP blocks the response. Is that right? Should the spec have a different order of these checks, such that COEP is checked first? or checked unconditionally?