Bug 173162

Summary: Align Document::canNavigate on the HTM5 specification
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: FramesAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, danyao, dbates, esprehn+autocc, fred.wang, kangil.han, rniwa
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
URL: https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate
Bug Depends on: 158875, 173649, 173657    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch cdumez: review+, cdumez: commit-queue-

Frédéric Wang (:fredw)
Reported 2017-06-09 09:04:58 PDT
See the following test cases: - web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html - https://codepen.io/tibit/pen/remQwm/
Attachments
Patch (7.78 KB, patch)
2017-06-09 09:29 PDT, Frédéric Wang (:fredw)
no flags
Patch (7.78 KB, patch)
2017-06-09 11:05 PDT, Frédéric Wang (:fredw)
no flags
Patch (7.74 KB, patch)
2017-06-12 03:07 PDT, Frédéric Wang (:fredw)
no flags
Patch (42.69 KB, patch)
2017-06-13 07:56 PDT, Frédéric Wang (:fredw)
no flags
Patch (9.75 KB, patch)
2017-06-21 09:55 PDT, Frédéric Wang (:fredw)
no flags
Patch (8.61 KB, patch)
2017-06-26 02:19 PDT, Frédéric Wang (:fredw)
no flags
Patch (9.18 KB, patch)
2017-06-27 02:06 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-elcapitan (1022.22 KB, application/zip)
2017-06-27 03:08 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.28 MB, application/zip)
2017-06-27 03:21 PDT, Build Bot
no flags
Archive of layout-test-results from ews114 for mac-elcapitan (1.79 MB, application/zip)
2017-06-27 03:43 PDT, Build Bot
no flags
Archive of layout-test-results from ews123 for ios-simulator-wk2 (40.82 MB, application/zip)
2017-06-27 05:49 PDT, Build Bot
no flags
Patch (10.03 KB, patch)
2017-06-28 01:40 PDT, Frédéric Wang (:fredw)
no flags
Patch (9.74 KB, patch)
2017-06-28 01:46 PDT, Frédéric Wang (:fredw)
no flags
Archive of layout-test-results from ews101 for mac-elcapitan (1012.63 KB, application/zip)
2017-06-28 02:45 PDT, Build Bot
no flags
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.07 MB, application/zip)
2017-06-28 02:52 PDT, Build Bot
no flags
Archive of layout-test-results from ews114 for mac-elcapitan (1.83 MB, application/zip)
2017-06-28 03:05 PDT, Build Bot
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (939.04 KB, application/zip)
2017-06-28 03:13 PDT, Build Bot
no flags
Patch (9.74 KB, patch)
2017-06-28 03:41 PDT, Frédéric Wang (:fredw)
cdumez: review+
cdumez: commit-queue-
Frédéric Wang (:fredw)
Comment 2 2017-06-09 09:29:44 PDT
Frédéric Wang (:fredw)
Comment 3 2017-06-09 11:05:00 PDT
Frédéric Wang (:fredw)
Comment 4 2017-06-12 02:04:41 PDT
Comment on attachment 312459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312459&action=review > Source/WebCore/dom/Document.cpp:3044 > + else if (targetFrame != &m_frame->tree().top() && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame)) Sorry, I made a mistake here when trying to split the patch from bug 158875. It should be !targetFrame->tree().parent() targetFrame != &m_frame->tree().top() && ...
Frédéric Wang (:fredw)
Comment 5 2017-06-12 02:25:09 PDT
(In reply to Frédéric Wang (:fredw) from comment #23 bug 158875) > >> Source/WebCore/dom/Document.cpp:2982 > >> + else if (!targetFrame->tree().parent() && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame)) > > > > Again, I do not understand, how can a frame NOT have a parent and NOT be the frame tree top? > > > > The implementation of FrameTree::top() looks like so: > > Frame& FrameTree::top() const > > { > > Frame* frame = &m_thisFrame; > > for (Frame* parent = &m_thisFrame; parent; parent = parent->tree().parent()) > > frame = parent; > > return *frame; > > } > > > > If frame does not have a parent, top() will return the frame itself... > > I believe you are right. I copied that part from Chromium, but they their > "top" implementation seems a bit more complicated. OK, I stand corrected. The initial condition was right: We say that the targetFrame is a top frame but not the one of the current frame. This happens for example when you open a new popup and tries to change its URL. I checked the current tests (W3C and chromium) a bit more. iframe_sandbox_popups_escaping-3.html and iframe_sandbox_popups_nonescaping-3.html will open http://w3c-test.org/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_helper-3.html which will attempt to navigate a popup from its opener (!targetFrame->tree().parent() && targetFrame != &m_frame->tree().top()). In the escaping case, isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) is false so the error does not happen. In the non-escaping case, isSandboxed(SandboxPopups) is false but targetFrame->loader().opener() == m_frame so again the error does not happen. The tests from https://bugs.chromium.org/p/chromium/issues/detail?id=577330 and https://bugs.chromium.org/p/chromium/issues/detail?id=597641 verify navigation in the same situation (allow-popups + allow-popups-to-escape-sandbox or allow-popups alone). The latter also verifies "closing the popup", which exercises the same canNavigate code path. So I don't think chromium's tests bring more than the W3C one here.
Frédéric Wang (:fredw)
Comment 6 2017-06-12 03:07:12 PDT
Created attachment 312646 [details] Patch In the past, when targetFrame->tree().isDescendantOf(m_frame) was true, canNavigate returned true. Now, if targetFrame == m_frame it follows the early return. Otherwise targetFrame is not a top frame, so none of the other failure conditions happen. Then it follow the normal case, which will again return true ("A document can navigate its decendant frames"). In the past, when targetFrame->tree().isDescendantOf(m_frame) was false, the failure was "'allow-top-navigation' flag is not set" when navigating the top frame of m_frame or "disallowed from navigating its ancestors" otherwise. Now, if targetFrame is not a top frame, the first condition is executed with failure "disallowed from navigating its ancestors". Otherwise, if targetFrame is the top frame of m_frame, then we follow the second condition, with failure "'allow-top-navigation' flag is not set". Otherwise, we *may* follow the third condition and raise a failure about "navigate a popup". Hence this patch releases the condition as follows: We allow a frame to navigate a popup, when allow-popups-to-escape-sandbox is not set or (allow-popup is set and the frame is the opener of the popup) .
Frédéric Wang (:fredw)
Comment 7 2017-06-13 07:56:18 PDT
Frédéric Wang (:fredw)
Comment 8 2017-06-21 09:55:11 PDT
Chris Dumez
Comment 9 2017-06-24 21:58:20 PDT
Comment on attachment 313525 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313525&action=review > Source/WebCore/dom/Document.cpp:-3107 > - if (targetFrame->tree().isDescendantOf(m_frame)) Can we try to match the spec [1] as closely as possible? I think this would be close: // From https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate. // 1. If A is not the same browsing context as B, and A is not one of the ancestor browsing contexts of B, and B is not a top-level browsing context, and A's active document's active sandboxing // flag set has its sandboxed navigation browsing context flag set, then abort these steps negatively. if (m_frame != targetFrame && isSandboxed(SandboxNavigation) && targetFrame->tree().parent() && !targetFrame->tree().isDescendantOf(m_frame)) { printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors.")); return false; } // 2. Otherwise, if B is a top-level browsing context, and is one of the ancestor browsing contexts of A, and A's active document's active sandboxing flag set has its sandboxed // top-level navigation browsing context flag set, then abort these steps negatively. if (targetFrame == &m_frame->tree().top() && isSandboxed(SandboxTopNavigation)) { printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors.")); return false; } // 3. Otherwise, if B is a top-level browsing context, and is neither A nor one of the ancestor browsing contexts of A, and A's Document's active sandboxing flag set has its // sandboxed navigation browsing context flag set, and A is not the one permitted sandboxed navigator of B, then abort these steps negatively. if (!targetFrame->tree().parent() && m_frame != targetFrame && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxNavigation) && targetFrame->loader().opener() != m_frame) { printNavigationErrorMessage(targetFrame, url(), ASCIILiteral("The frame attempting navigation is sandboxed, and is not allowed to navigate this popup")); return false; } // 4. Otherwise, terminate positively! If for some reason, we *really* cannot match the spec, I would propose the following factoring which should be equivalent to what you proposed but a little bit more efficient (and I think a little clearer): if (!targetFrame->tree().parent()) { if (targetFrame == &m_frame->tree().top()) failureReason = "The frame attempting navigation of the top-level window is sandboxed, but the 'allow-top-navigation' flag is not set."; else if (isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame)) failureReason = "The frame attempting navigation is sandboxed and is trying to navigate a popup, but is not the popup's opener and is not set to propagate sandboxing to popups."; } else if (!targetFrame->tree().isDescendantOf(m_frame)) failureReason = "The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors."; But in your code, I don't fully understand the use of isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) and isSandboxed(SandboxPopups), which do not seem to be part of the spec text for allowing navigating. They're about allowing popup *opening* and propagation of sandbox flags from opener to popups AFAICT. [1] https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate
Chris Dumez
Comment 10 2017-06-24 21:59:28 PDT
Sorry about the review delay, I had a crazy week.
Frédéric Wang (:fredw)
Comment 11 2017-06-25 02:46:06 PDT
(In reply to Chris Dumez from comment #9) > But in your code, I don't fully understand the use of > isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) and > isSandboxed(SandboxPopups), which do not seem to be part of the spec text > for allowing navigating. They're about allowing popup *opening* and > propagation of sandbox flags from opener to popups AFAICT. > OK, I'll try your suggestion. As I said, in the ChangeLog, I was not able to write tests (or to find some in the W3C or Chromium repos) about these cases as other security protections make these hard to do: - a frame with allow-popups disabled but being a popup opener. - or a frame trying to access the popup opened by another unrelated frame. so I suspect we could indeed remove isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) and isSandboxed(SandboxPopups). At least the two W3C tests should still work, I expect. https://bugs.chromium.org/p/chromium/issues/detail?id=597641 gives some spec references on the "one permitted sandboxed navigator" but I have the impression that Chromium is over-protecting navigation of popups, based on which situations such popups are permitted to be opened. I have to check whether I can actually remove some conditions in canNavigate because when I initially tried (e.g. the one about "Frame-busting is generally allowed..."), this broke existing tests in WebKit.
Frédéric Wang (:fredw)
Comment 12 2017-06-26 02:19:42 PDT
Chris Dumez
Comment 13 2017-06-26 19:20:44 PDT
Comment on attachment 313822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313822&action=review > Source/WebCore/dom/Document.cpp:3104 > if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) Why is this still here? > Source/WebCore/dom/Document.cpp:3108 > if (isSandboxed(SandboxNavigation)) { if I remember correctly, this is only supposed to affect some of the if checks below (see the code I posted earlier). > Source/WebCore/dom/Document.cpp:3118 > + if (m_frame != targetFrame && targetFrame == &m_frame->tree().top()) { This was supposed to have the isSandboxed(SandboxTopNavigation) check, no isSandboxed(SandboxNavigation) check.
Frédéric Wang (:fredw)
Comment 14 2017-06-27 00:47:33 PDT
Comment on attachment 313822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313822&action=review >> Source/WebCore/dom/Document.cpp:3118 >> + if (m_frame != targetFrame && targetFrame == &m_frame->tree().top()) { > > This was supposed to have the isSandboxed(SandboxTopNavigation) check, no isSandboxed(SandboxNavigation) check. Sorry, I missed that. Which explains the two points above... I'll upload a new patch. BTW, Note that I added m_frame != targetFrame here compared to your suggestion, because I understand that the spec says a frame is not "ancestor" of itself. Otherwise a SandboxTopNavigation top frame won't be able to navigate itself.
Frédéric Wang (:fredw)
Comment 15 2017-06-27 02:06:06 PDT
Build Bot
Comment 16 2017-06-27 03:08:37 PDT
Comment on attachment 313904 [details] Patch Attachment 313904 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4005284 New failing tests: http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html http/tests/fileapi/create-blob-url-from-data-url.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html fast/frames/sandboxed-iframe-close-top.html http/tests/security/xss-DENIED-window-open-parent.html fast/files/null-origin-string.html
Build Bot
Comment 17 2017-06-27 03:08:39 PDT
Created attachment 313905 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 18 2017-06-27 03:21:00 PDT
Comment on attachment 313904 [details] Patch Attachment 313904 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4005309 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html http/tests/fileapi/create-blob-url-from-data-url.html fast/frames/sandboxed-iframe-close-top.html http/tests/security/xss-DENIED-window-open-parent.html
Build Bot
Comment 19 2017-06-27 03:21:02 PDT
Created attachment 313906 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Build Bot
Comment 20 2017-06-27 03:43:25 PDT
Comment on attachment 313904 [details] Patch Attachment 313904 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4005308 New failing tests: http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html http/tests/fileapi/create-blob-url-from-data-url.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html fast/frames/sandboxed-iframe-close-top.html http/tests/security/xss-DENIED-window-open-parent.html fast/files/null-origin-string.html
Build Bot
Comment 21 2017-06-27 03:43:27 PDT
Created attachment 313907 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 22 2017-06-27 05:49:46 PDT
Comment on attachment 313904 [details] Patch Attachment 313904 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4005720 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-1.html http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html http/tests/fileapi/create-blob-url-from-data-url.html fast/frames/sandboxed-iframe-close-top.html http/tests/security/xss-DENIED-window-open-parent.html
Build Bot
Comment 23 2017-06-27 05:49:48 PDT
Created attachment 313909 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Frédéric Wang (:fredw)
Comment 24 2017-06-28 01:28:37 PDT
(In reply to Build Bot from comment #20) > New failing tests: > http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html > http/tests/fileapi/create-blob-url-from-data-url.html > imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe- > element/iframe_sandbox_allow_top_navigation-1.html > fast/frames/sandboxed-iframe-close-top.html These tests seem to fail because of the removal of "frame-busting". > http/tests/security/xss-DENIED-window-open-parent.html This one too, although it's just the message error which is different from the reference. > fast/files/null-origin-string.html This one seems unrelated to this patch.
Frédéric Wang (:fredw)
Comment 25 2017-06-28 01:40:07 PDT
Created attachment 314004 [details] Patch (In reply to Chris Dumez from comment #13) > > Source/WebCore/dom/Document.cpp:3104 > > if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) > > Why is this still here? (In reply to Frédéric Wang (:fredw) from comment #11) > I have to check whether I can actually remove some conditions in canNavigate > because when I initially tried (e.g. the one about "Frame-busting is > generally allowed..."), this broke existing tests in WebKit. So indeed, removing "frame-busting" breaks tests (comment 20) and it seems this is still used by many pages, so I suspect we won't be able to remove it so easily. For the record, Chromium people are trying to get rid of it but have failed so far: https://bugs.chromium.org/p/chromium/issues/detail?id=640057 https://github.com/WICG/interventions/issues/16 https://bugs.chromium.org/p/chromium/issues/detail?id=624061 https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?type=cs&q=Frame-busting
Build Bot
Comment 26 2017-06-28 01:42:09 PDT
Attachment 314004 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:23: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Frédéric Wang (:fredw)
Comment 27 2017-06-28 01:46:59 PDT
Build Bot
Comment 28 2017-06-28 02:45:39 PDT
Comment on attachment 314005 [details] Patch Attachment 314005 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4012659 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html
Build Bot
Comment 29 2017-06-28 02:45:40 PDT
Created attachment 314006 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 30 2017-06-28 02:52:04 PDT
Comment on attachment 314005 [details] Patch Attachment 314005 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4012667 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html
Build Bot
Comment 31 2017-06-28 02:52:05 PDT
Created attachment 314007 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Build Bot
Comment 32 2017-06-28 03:05:37 PDT
Comment on attachment 314005 [details] Patch Attachment 314005 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4012669 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html
Build Bot
Comment 33 2017-06-28 03:05:38 PDT
Created attachment 314008 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 34 2017-06-28 03:13:09 PDT
Comment on attachment 314005 [details] Patch Attachment 314005 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4012691 New failing tests: imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html
Build Bot
Comment 35 2017-06-28 03:13:10 PDT
Created attachment 314009 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Frédéric Wang (:fredw)
Comment 36 2017-06-28 03:41:54 PDT
Chris Dumez
Comment 37 2017-06-28 09:24:06 PDT
Comment on attachment 314011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review > Source/WebCore/dom/Document.cpp:3156 > + if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave?
Frédéric Wang (:fredw)
Comment 38 2017-06-28 09:52:17 PDT
Comment on attachment 314011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review >> Source/WebCore/dom/Document.cpp:3156 >> + if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) > > So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave? Yes, see comment 24 and 25. Chromium first checks navigation without frame busting in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?l=913 similarly to what we do and then uses frame busting (with a warning message) with their current default setting: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalFrame.cpp?l=844
Chris Dumez
Comment 39 2017-06-28 10:24:05 PDT
Comment on attachment 314011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review > Source/WebCore/dom/Document.cpp:-3103 > - if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) Since we need to keep this logic, let's keep it here instead of moving it below. > Source/WebCore/dom/Document.cpp:-3107 > - if (targetFrame->tree().isDescendantOf(m_frame)) Dropping this makes us stricter and I worry about making our behavior stricter at this point. Can we please keep this? The spec says to return true in step 4 but we currently do not (we have some extra origin checks). Therefore, keeping this check here keeps us closer to the spec as far as I can tell. So something like: if (isSandboxed(SandboxNavigation) && targetFrame->tree().isDescendantOf(m_frame)) return true; > Source/WebCore/dom/Document.cpp:3126 > // This is the normal case. A document can navigate its decendant frames, Can you please fix the "decendant" typo while you're here. >> Source/WebCore/dom/Document.cpp:3156 >> + if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) > > So we allow a subframe to navigate its top frame if it has 'allow-top-navigation', even if it is cross-origin? I suppose you kept this to address layout test failures? Is this how other browsers behave? Looks like this is fine but I'd rather we keep it where it was, at the beginning of this method.
Frédéric Wang (:fredw)
Comment 40 2017-06-29 00:05:05 PDT
Comment on attachment 314011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314011&action=review >> Source/WebCore/dom/Document.cpp:-3103 >> - if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top()) > > Since we need to keep this logic, let's keep it here instead of moving it below. Done. >> Source/WebCore/dom/Document.cpp:-3107 >> - if (targetFrame->tree().isDescendantOf(m_frame)) > > Dropping this makes us stricter and I worry about making our behavior stricter at this point. Can we please keep this? The spec says to return true in step 4 but we currently do not (we have some extra origin checks). Therefore, keeping this check here keeps us closer to the spec as far as I can tell. So something like: > if (isSandboxed(SandboxNavigation) && targetFrame->tree().isDescendantOf(m_frame)) > return true; OK I guess this is fine. Done too. >> Source/WebCore/dom/Document.cpp:3126 >> // This is the normal case. A document can navigate its decendant frames, > > Can you please fix the "decendant" typo while you're here. Done.
Frédéric Wang (:fredw)
Comment 41 2017-06-29 00:43:17 PDT
Note You need to log in before you can comment on or make changes to this bug.