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

HTML: Test ordering of COEP reporting vs CSP and X-Frame-Options #28281

Merged
merged 1 commit into from Apr 8, 2021

Conversation

@zcorpan
Copy link
Member

@zcorpan zcorpan commented Mar 29, 2021

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?

@domenic
Copy link
Member

@domenic domenic commented Mar 29, 2021

Did this change since whatwg/html#5859? @jugglinmike

@zcorpan
Copy link
Member Author

@zcorpan zcorpan commented Mar 29, 2021

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.

@domenic
Copy link
Member

@domenic domenic commented Mar 29, 2021

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.

@yutakahirano
Copy link
Contributor

@yutakahirano yutakahirano commented Apr 7, 2021

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?

@ArthurSonzogni
Copy link
Contributor

@ArthurSonzogni ArthurSonzogni commented Apr 7, 2021

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
The current order is CSPEE < COEP < CSP in Chrome.

For now, in Chrome after receiving the navigation response:

  1. Check CSP embedded enforcement.
  2. Check COEP/CORP
  3. Check COOP vs sandbox
  4. Check download
  5. Check CSP (navigate-to, frame-src, prefetch-src, etc...) + before & after UpgradeInsecureRequest in the middle.
  6. Check CSP frame-ancestors
  7. Check X-Frame-Option

For now, my reading of the spec is:

  1. Check CSP (every directives)
  2. Check COEP
  3. Check X-Frame-Option
@zcorpan
Copy link
Member Author

@zcorpan zcorpan commented Apr 7, 2021

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

@ArthurSonzogni
Copy link
Contributor

@ArthurSonzogni ArthurSonzogni commented Apr 7, 2021

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.
Moving the COEP check one step earlier in the spec would be much simpler. However, I don't know what other web implementations would prefer.


Your test is very useful!
Do you need anything else from me? Approving this PR? More informations?

@zcorpan
Copy link
Member Author

@zcorpan zcorpan commented Apr 8, 2021

@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.

@ArthurSonzogni
Copy link
Contributor

@ArthurSonzogni ArthurSonzogni commented Apr 8, 2021

@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.

@zcorpan
Copy link
Member Author

@zcorpan zcorpan commented Apr 8, 2021

@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.

Copy link
Contributor

@ArthurSonzogni ArthurSonzogni left a comment

Thanks! LGTM!

@zcorpan zcorpan merged commit 89da710 into master Apr 8, 2021
20 checks passed
20 checks passed
@azure-pipelines
Azure Pipelines Build #20210329.25 succeeded
Details
@azure-pipelines
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
@azure-pipelines
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
@azure-pipelines
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
@azure-pipelines
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
@azure-pipelines
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
@community-tc-integration
download-firefox-nightly Community-TC (pull_request)
Details
@community-tc-integration
lint Community-TC (pull_request)
Details
@community-tc-integration
sink-task Community-TC (pull_request)
Details
@community-tc-integration
update-built Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-results Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-stability Community-TC (pull_request)
Details
@community-tc-integration
wpt-decision-task Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-results Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
@wpt-fyi
wpt.fyi - chrome[experimental] Chrome results
Details
@wpt-fyi
wpt.fyi - firefox[experimental] Firefox results
Details
@wpt-fyi
wpt.fyi - safari[experimental] Safari results
Details
@zcorpan zcorpan deleted the bocoup/coep-reporting-vs-csp-xframeoptions branch Apr 8, 2021
ArthurSonzogni added a commit to ArthurSonzogni/html that referenced this pull request Apr 11, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants