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: test COOP popup from a CSP-sandboxed popup #21111

Merged
merged 9 commits into from
Aug 28, 2020

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jan 9, 2020

Part of #18354.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

It'd be interesting to also navigate the CSP popup to a COOP document as that should work (since it's the document that's sandboxed and not the browsing context).

html/cross-origin-opener-policy/resources/csp-sandbox.py Outdated Show resolved Hide resolved
<script>
const params = new URL(location).searchParams;
params.delete("sandbox");
window.open(`${get_host_info().HTTPS_ORIGIN}/html/cross-origin-opener-policy/resources/coop-coep.py?${params}`)
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be the same origin?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, yes, but I realize now that get_host_info isn't needed, it can just be a relative URL instead.

@ParisMeuleman
Copy link
Contributor

It'd be interesting to also navigate the CSP popup to a COOP document as that should work (since it's the document that's sandboxed and not the browsing context).

Thanks for that comment, it helped clarify a doubt in Chromium's implem: https://crrev.com/c/1995268/3/content/browser/cross_origin_opener_policy_browsertest.cc

Hence, I think adding this test is a good idea!

cc: @camillelamy @ArthurSonzogni @hemeryar

@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the zcorpan/coop-csp-sandbox branch January 24, 2020 18:08
@gsnedders gsnedders restored the zcorpan/coop-csp-sandbox branch January 24, 2020 18:51
@Hexcles Hexcles reopened this Jan 24, 2020
@zcorpan
Copy link
Member Author

zcorpan commented Aug 20, 2020

@annevk

It'd be interesting to also navigate the CSP popup to a COOP document as that should work (since it's the document that's sandboxed and not the browsing context).

Now added in 1fc0b75

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21111 August 20, 2020 14:16 Inactive
@zcorpan
Copy link
Member Author

zcorpan commented Aug 21, 2020

Similarly to #20873 (comment) , changing common.js makes stability checks time out.

t.step_timeout(() => {
assert_unreached('Navigation from CSP sandbox to COOP document failed')
}, 1500);
}));
Copy link
Member

Choose a reason for hiding this comment

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

Since you have a reference to the popup and it won't be replaced, can't you assert something about the popup? That it's not closed and that it's document is no longer same-origin? I guess that doesn't work for the case where allow-same-origin isn't set.

Copy link
Member Author

Choose a reason for hiding this comment

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

But closed should be true after the navigation? The CSP sandbox doesn't have COOP, and then when navigating to the COOP document the browsing context is swapped.

The "is no longer same origin" is interesting. Should it not be same-origin for any of these 2 cases?

Copy link
Member

Choose a reason for hiding this comment

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

I misread the test.

html/cross-origin-opener-policy/resources/common.js Outdated Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21111 August 26, 2020 14:55 Inactive
@annevk
Copy link
Member

annevk commented Aug 26, 2020

This looks good at a high-level, but seems like it still needs conflicts resolved.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 26, 2020

I added a same-origin check, and have different results between Firefox and Chrome:

Test Firefox Chrome
CSP: sandbox allow-popups allow-scripts allow-same-origin; CSP sandbox popup navigate to Cross-Origin-Opener-Policy document should work Fail Fail
CSP: sandbox allow-popups allow-scripts; CSP sandbox popup navigate to Cross-Origin-Opener-Policy document should work Pass Fail

I'm not sure what is right. Should the popup after navigation (which causes a BC swap) not be same-origin with the opener?

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21111 August 26, 2020 15:20 Inactive
@annevk
Copy link
Member

annevk commented Aug 27, 2020

Looking at html/cross-origin-opener-policy/coop-csp-sandbox-navigate.https.html I think Firefox is correct. In particular assert_throws_dom("SecurityError", () => { popup.document; }, 'same-origin check'); should not throw when the CSP popup has allow-same-origin. Note that popup cannot have a reference to the document being navigated to as it's already closed (and replaced by a new thing you cannot have a reference to by design) at that point.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 27, 2020

@annevk thank you for explaining that, I had misremembered how Window/WindowProxy works on navigation. popup is the old window which still has the CSP sandbox applied. I've fixed the test and added comments.

@annevk
Copy link
Member

annevk commented Aug 27, 2020

Will you file a bug against Chrome? It seems they made a mistake somewhere (or you/I analyzed incorrectly).

@zcorpan
Copy link
Member Author

zcorpan commented Aug 27, 2020

@zcorpan
Copy link
Member Author

zcorpan commented Aug 28, 2020

I'll admin-merge this, see #21111 (comment)

@zcorpan zcorpan merged commit bc2b0f3 into master Aug 28, 2020
@zcorpan zcorpan deleted the zcorpan/coop-csp-sandbox branch August 28, 2020 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants