Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399713002:
window.close() should work from a sandboxed iframe if iframe is opener (Closed)
|
Created:
4 years, 2 months ago by Nate Chapin Modified:
4 years ago Reviewers:
Mike West CC:
blink-reviews, chromium-reviews, dcheng, mlamouri+watch-blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionwindow.close() should work from a sandboxed iframe if iframe is opener
BUG=597641
Committed: https://crrev.com/53f3802c16e36675a690990d8d8b616e31817c0f
Cr-Commit-Position: refs/heads/master@{#433998}
Patch Set 1 #Patch Set 2 : isFamiliarWith #Patch Set 3 : Revert most refactoring #Patch Set 4 : Revert most refactoring #Patch Set 5 : Fix tests #Patch Set 6 : Fix more tests #Patch Set 7 : +test #
Total comments: 2
Patch Set 8 : +close() test #
Messages
Total messages: 44 (36 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== window.close() works in a sandboxed iframe if iframe is opener BUG= ========== to ========== window.close() works in a sandboxed iframe if iframe is opener BUG=597641 ==========
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== window.close() works in a sandboxed iframe if iframe is opener BUG=597641 ========== to ========== window.close() should work from a sandboxed iframe if iframe is opener BUG=597641 ==========
japhet@chromium.org changed reviewers: + mkwst@chromium.org
I did some cleanup in this CL to try to map Frame::canNavigate to the spec, but it's imperfect and I think the spec may be underspecified. WDYT? https://codereview.chromium.org/2399713002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2399713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:214: !targetFrame.isMainFrame()) { I inverted this block so that it more closely matches https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate (albeit with clauses 2 and 3 reversed). https://codereview.chromium.org/2399713002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/Frame.cpp:245: return true; We seem to depend on skipping origin checks for allow-top-navigation. This is complicated by the fact that the spec splits navigation permissions into an "is familiar with" step and an "is allowed to navigate" step. We combine them in this function: the "if (securityContext()->isSandboxed(SandboxNavigation))" block corresponds to "is allowed to navigate", and the rest to "is familiar with". "is familiar with" is used far less frequently, according to the spec, and it's not clear to me that what the spec defines is actually what browsers do in practice. The spec implies that an Location object you can reach without looking up by name/target (that is, top, opener, and any of their descendants) can be navigated without any familiarity requirements. That's not what we do, and it's not what firefox does AFAICT.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
What does the test test? I don't see `close()` being called anywhere... What am I missing?
On 2016/11/03 13:04:57, Mike West wrote: > What does the test test? I don't see `close()` being called anywhere... What am > I missing? Heh, I flaked. I tested a different artifact of the underlying canNavigate() check. If the opener can navigate the opened window, it should be able to close it to. Will see how difficult that is to fix.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/03 17:38:28, Nate Chapin wrote: > On 2016/11/03 13:04:57, Mike West wrote: > > What does the test test? I don't see `close()` being called anywhere... What > am > > I missing? > > Heh, I flaked. I tested a different artifact of the underlying canNavigate() > check. If the opener can navigate the opened window, it should be able to close > it to. Will see how difficult that is to fix. Added a separate test for the window.close() case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM.
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1479846085287090,
"parent_rev": "08ca14f726db9638c50180ea2d1c9fc52d02dc75", "commit_rev":
"a80e34c9bf224e4c767647933c2f6fca4f0ad093"}
Message was sent while issue was closed.
Description was changed from ========== window.close() should work from a sandboxed iframe if iframe is opener BUG=597641 ========== to ========== window.close() should work from a sandboxed iframe if iframe is opener BUG=597641 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== window.close() should work from a sandboxed iframe if iframe is opener BUG=597641 ========== to ========== window.close() should work from a sandboxed iframe if iframe is opener BUG=597641 Committed: https://crrev.com/53f3802c16e36675a690990d8d8b616e31817c0f Cr-Commit-Position: refs/heads/master@{#433998} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/53f3802c16e36675a690990d8d8b616e31817c0f Cr-Commit-Position: refs/heads/master@{#433998} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
