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

COOP and COEP tests #17606

Merged
merged 26 commits into from
Aug 14, 2019
Merged

COOP and COEP tests #17606

merged 26 commits into from
Aug 14, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Jul 2, 2019

Initial (incomplete) take at restructuring some of the existing tests to account for name and logic changes.

TODO:

  • Rewrite COEP popup tests (at the end of require-corp.tentative.html) into COOP+COEP tests.
  • Add more COOP+COEP tests.
  • Add COOP unsafe-inherit tests. (Writing this down I realize currently it also inherits COEP which is a bug. Make sure to test that and fix the draft.)
  • Add some tests for initial/non-initial about:blank/blob/data URLs.
  • Workers...
  • [Suggestions?]

@csreis
Copy link

csreis commented Jul 3, 2019

Thanks for adding these tests! I hadn't seen the existing WPT tests for COOP and Cross-Origin, so I'll try to take a closer look at them. Your proposed changes here seem reasonable to me.

One question I was wondering about when working on the Chrome implementation, and which might be useful to cover in a test-- when a frame is navigating, is the COOP comparison made against the current document in the frame or the initiator of the navigation? For example: page A1 (with a policy and unsafe-allow-origin) opens page B (no header), staying in the group.

  1. If A1 then tells B's frame to navigate to A2 (with a matching policy to A1), is that considered a match with A1 and stays in the group? Or is it compared against B and considered a mismatch?
  2. If instead, B navigates itself to A2, is that considered a policy mismatch forcing a new group? Or did B inherit A1's policy and it's considered a match?

@wpt-pr-bot

This comment has been minimized.

@annevk
Copy link
Member Author

annevk commented Jul 3, 2019

As currently written currentCOOP would always be from B and there's no inheritance, so a mismatch in both cases. The only time the initiator comes into play (and you could talk about inheritance of sorts) is with initial about:blank. (And yeah, we should test this!)

@mystor
Copy link
Contributor

mystor commented Jul 3, 2019

Did a quick brainstorm of some things which might be worth testing re. these headers:

  • COOP header appearing in various places during the redirect chain:
    • Final URL in chain has COOP
    • Intermediate URL has COOP but final URL doesn't
    • Final URL is non-HTTP (e.g. ftp://)
    • Final response synthesized by a ServiceWorker (with COOP/COEP or with CO*P in intermediate, but not in final response)
    • COOP document detecting a late <meta charset> requiring the parser to change the encoding while parsing, and re-navigate. (Anne: filed https://bugzilla.mozilla.org/show_bug.cgi?id=1566431 to see if we can instead not do this.)
  • COEP embedding of non-HTTP loads (IIRC these won't work because they don't have the header?):
    • blob (same & cross-origin)
    • data
    • srcdoc
    • non-initial about:blank
    • ftp (Anne: see Support FTP #17836)
  • Sandboxing within COEP
    • Framing URIs in non-"allow-same-origin" frames.
    • non-"allow-same-origin" frames embedding same origin content
    • non-"allow-same-origin" frames loading same-origin subresources
    • COEP and COOP in sandboxed popups

aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 31, 2019
 - "cross-origin-embedder-policy" set's the policy for frame.
 - dedicated worker's policy is set to the ancestor frame's policy.
 - nested frames with a conflicting policy is blocked.

SharedWorker / ServiceWorker are not yet supported. All the
implementation is behind the flag, and tested manually with
work in progress WPTs[1].

1: web-platform-tests/wpt#17606

Bug: 887967
Change-Id: I70ed24841afde1b3c72dad40288744bb92a6f5dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715378
Commit-Queue: Yutaka Hirano <[email protected]>
Reviewed-by: Hajime Hoshi <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Cr-Commit-Position: refs/heads/master@{#682609}
@hsivonen
Copy link
Member

hsivonen commented Aug 5, 2019

re-navigation due to <meta charset> I'd like to outlaw if possible.

I'd be OK with adding a flag to Gecko's HTML parser to prevent renavigation on late <meta charset> when certain headers are set if WebKit and Blink make their meta prescanner strictly obey the 1024-byte limit when those headers are set.

@annevk
Copy link
Member Author

annevk commented Aug 9, 2019

This is ready for another round of feedback. #18354 lists the remaining work. Please add suggestions for further tests there.

@yutakahirano
Copy link
Contributor

LGTM (I only looked at the COEP part).

Copy link
Contributor

@JuniorHsu JuniorHsu left a comment

Choose a reason for hiding this comment

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

Mostly from the point of readability.

coop_coep_test(t, host, coop, "", channelName, hasOpener);
}

function run_coop_tests(mainTest, testArray) {
Copy link
Contributor

Choose a reason for hiding this comment

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

annotate or use variable name to indicate mainTest is a COOP value.

}

function coop_coep_test(t, host, coop, coep, channelName, hasOpener) {
url_test(t, `${host.origin}/html/cross-origin-opener-policy/resources/coop-coep.py?coop=${encodeURIComponent(coop)}&coep=${coep}&channel=${channelName}`, channelName, hasOpener);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is too long. One solution might be
let url = ${host.origin}/html/cross-origin-opener-policy/resources/coop-coep.py;
url += `?coop=...

const SAME_SITE = {origin: get_host_info().HTTPS_REMOTE_ORIGIN, name: "SAME_SITE"};
const CROSS_ORIGIN = {origin: get_host_info().HTTPS_NOTSAMESITE_ORIGIN, name: "CROSS_ORIGIN"}

function url_test(t, url, channelName, hasOpener) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name url_test is kinda misleading. Actually we're test hasOpener

const navigateHost = site === "same-origin" ? SAME_ORIGIN : SAME_SITE;
const navigateURL = `${navigateHost.origin}/html/cross-origin-opener-policy/resources/coop-coep.py?coop=${variant.coop}&coep=${variant.coep}&channel=${channel}`;
const opener = site === "same-origin" ? variant.opener : false;

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's a complicated test. Could you put some comments about the testing flow?

popup.close();
t.done();
}, 500);
}, "Cross-Origin-Opener-Policy only works over HTTPS");
Copy link
Contributor

Choose a reason for hiding this comment

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

s/HTTPS/secure contexts

@annevk
Copy link
Member Author

annevk commented Aug 13, 2019

Proposed commit message:

COOP and COEP tests

See the added html/cross-origin-opener-policy/README.md for details.

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 13, 2019

ERROR:lint:html/cross-origin-embedder-policy/resources/navigate-none.sub.html:9: setTimeout used; step_timeout should typically be used instead (SET TIMEOUT)
ERROR:lint:html/cross-origin-embedder-policy/resources/navigate-require-corp.sub.html:15: setTimeout used; step_timeout should typically be used instead (SET TIMEOUT)

@annevk annevk merged commit 5690096 into master Aug 14, 2019
@annevk annevk deleted the annevk/coop-and-coep branch August 14, 2019 11:41
@annevk annevk restored the annevk/coop-and-coep branch August 16, 2019 12:37
@annevk annevk deleted the annevk/coop-and-coep branch August 16, 2019 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants