Add tests regarding navigation inside sandboxed iframes. #6221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc'ing @natechapin too. |
Firefox (nightly)Testing web-platform-tests at revision 813c23d All results6 tests ran/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-2.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-1.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-2.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_descendants.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_itself.html
|
Sauce (safari)Testing web-platform-tests at revision 813c23d All results6 tests ran/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-2.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-1.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-2.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_descendants.html
/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_itself.html
|
Chrome (unstable)Testing web-platform-tests at revision 813c23d |
|
Note: I modified the test for allow-top-navigation to make it more reliable. If needed, we can add a similar test to check that top navigation is blocked when allow-top-navigation is unset, however this is already done by iframe_sandbox_allow_top_navigation_by_user_activation_without_user_gesture.html |
mikewest
reviewed
Jun 13, 2017
| opener.postMessage({data: e.data, origin: e.origin}, "*") | ||
| window.close(); | ||
| } | ||
| document.querySelector("iframe").src = "support/iframe-that-performs-top-navigation-without-user-gesture-passed.html"; |
There was a problem hiding this comment.
It seems like the timing here's not going to work. The frame loads iframe-that-performs-top-navigation-without-user-gesture-passed.html, which navigates the page containing the frame. Depending on when that navigation fires, there might or might not actually be a message handler to postMessage() up to the opener.
| // trying to modifying the top frame and transmit the result to our | ||
| // opener. | ||
| onmessage = function(e) { | ||
| opener.postMessage({data: e.data, origin: e.origin}, "*") |
There was a problem hiding this comment.
Why pass the origin if you're not using it in the check on line 27?
| assert_equals(e.data.data, "PASS", "Should have the right message"); | ||
| }); | ||
| var popupWin = window.open(); | ||
| popupWin.location.href = location.href; |
There was a problem hiding this comment.
Tiny style nit: I'd write this as:
async_test(t => {
window.addEventListener('message', t.step_func_done(e => {
assert_equals(e.data.data, "PASS");
e.source.close();
}));
window.open(location.href);
}, "Frames with `allow-top-navigation` should be able to navigate the top frame.");
Closing the window helps clean things up when you're running the test manually, and I don't think you need the extra t or popupWin variables.
…check that navigation is not allowed when the flag is absent.
|
@mikewest Thanks. I pushed another commit that hopefully fixes your concern. I finally also added a similar test when allow-top-navigation is unset. |
|
@mikewest ¿review ping? |
mikewest
approved these changes
Jun 21, 2017
| <body> | ||
| <script> | ||
| window.onload = function() | ||
| { |
There was a problem hiding this comment.
Style nit: The brace is on the same line with the function definition in all the other files. Let's move it up here as well.
| <script> | ||
| window.onload = function() | ||
| { | ||
| try { |
|
@mikewest thanks! |
hubot
pushed a commit
to WebKit/WebKit-http
that referenced
this pull request
Jun 21, 2017
https://bugs.webkit.org/show_bug.cgi?id=173649 Patch by Frederic Wang <[email protected]> on 2017-06-21 Reviewed by Youenn Fablet. This import new tests added in web-platform-tests/wpt#6221 to verify sandboxing of iframes and will help to test the changes in bug 173162. * resources/import-expectations.json: * resources/resource-files.json: * web-platform-tests/html/semantics/embedded-content/the-iframe-element/content_document_changes_only_after_load_matures-expected.txt: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/content_document_changes_only_after_load_matures.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1-expected.txt: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-2-expected.txt: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-2.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-1-expected.txt: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-1.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-2-expected.txt: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-2.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_descendants-expected.txt: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_descendants.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_itself-expected.txt: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_itself.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/support/iframe-that-performs-top-navigation-on-popup.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/support/iframe-that-tries-to-navigate-parent-and-sends-result-to-grandparent.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/support/iframe-tried-to-be-navigated-by-its-child.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/support/iframe-trying-to-navigate-its-child.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/support/iframe-trying-to-navigate-itself.html: Added. * web-platform-tests/html/semantics/embedded-content/the-iframe-element/support/w3c-import.log: * web-platform-tests/html/semantics/embedded-content/the-iframe-element/w3c-import.log: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@218639 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds basic tests for frame navigation to verify some cases handled by Chromium, that I'm trying to implement in WebKit [1] [2]. cc'ing @mikewest @RByers @japhet who have worked on this. Also cc'ing @cdumez and @bzbarsky for WebKit & Mozilla.
[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?type=cs&q=LocalFrame::CanNavigateWithoutFramebusting
[2] https://bugs.webkit.org/show_bug.cgi?id=173162