⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 280826 in webkit


Ignore:
Timestamp:
Aug 10, 2021, 1:25:31 AM (5 years ago)
Author:
ysuzuki@apple.com
Message:

Partially revert r280256 behavior change since it broke Facetime
​https://bugs.webkit.org/show_bug.cgi?id=228943
rdar://81700268

Reviewed by Mark Lam.

Source/WebCore:

In r280256, custom accessors start getting holder's JSGlobalObject (this is right behavior).
We also introduced IncumbentWindow IDL attribute to get the caller's JSGlobalObject, which is
necessary in Location. At that time, we replaced all ActiveWindow to IncumbentWindow, but it
changes the JSGlobalObject of replace, assign, and reload since they are functions (not
custom accessors). We observed that, after this change, facetime Web page is broken: when opening
the link to Facetime, it should show the prompt of "Open in Facetime app" in iOS. But after this
change, it won't happen.

We revert the semantic change by using ActiveWindow for replace, assign, and reload. This is
not the correct semantics from the spec, but this is the same to one before r280256, and we
will change this once we find the way to bypass this issue.

  • page/Location.cpp:

(WebCore::Location::assign):
(WebCore::Location::replace):
(WebCore::Location::reload):

  • page/Location.h:
  • page/Location.idl:

LayoutTests:

  • fast/frames/sandboxed-iframe-navigation-parent-expected.txt:
  • fast/frames/sandboxed-iframe-navigation-parent.html:
  • http/tests/security/frameNavigation/context-for-location-assign-expected.txt:
Location:
trunk
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r280824 r280826  
     12021-08-09  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        Partially revert r280256 behavior change since it broke Facetime
     4        https://bugs.webkit.org/show_bug.cgi?id=228943
     5        rdar://81700268
     6
     7        Reviewed by Mark Lam.
     8
     9        * fast/frames/sandboxed-iframe-navigation-parent-expected.txt:
     10        * fast/frames/sandboxed-iframe-navigation-parent.html:
     11        * http/tests/security/frameNavigation/context-for-location-assign-expected.txt:
     12
    1132021-08-09  Chris Dumez  <cdumez@apple.com>
    214

  • trunk/LayoutTests/fast/frames/sandboxed-iframe-navigation-parent-expected.txt

    r280256 r280826  
    1 CONSOLE MESSAGE: Unsafe JavaScript attempt to initiate navigation for frame with URL 'sandboxed-iframe-navigation-source.html'. The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors.
    2 
    3 CONSOLE MESSAGE: SecurityError: The operation is insecure.
    4 This test verifies that a sandboxed IFrame cannot navigate an ancestor in the frame tree by assigning the location attribute.
    5 
    6 This is done by loading ten non-sandboxed IFrames, and a single sandboxed one. Expect ten frames to be navigated, but the sandboxed one to not be one of them.
     1This test verifies that a sandboxed IFrame with the keywords allow-scripts and allow-same-origin is allowed to navigate an ancestor in the frame tree by assigning the location attribute.
    72
    83This test will print "PASS" on success.

  • trunk/LayoutTests/fast/frames/sandboxed-iframe-navigation-parent.html

    r280256 r280826  
    1414    var testStatus = document.getElementById('testStatus');
    1515
    16     if (self.navigated.location.href.indexOf('parent.html') == -1)
    17         testStatus.innerHTML = 'FAIL: disallowed frame navigated';
    18     else if (navigatedFrames == 10)
     16    if (self.navigated.location.href.indexOf('parent.html') != -1)
     17        testStatus.innerHTML = 'FAIL: sandboxed frame couldn\'t navigate';
     18    else if (navigatedFrames == 11)
    1919        testStatus.innerHTML = 'PASS';
    2020    else
    … …  
    2929    ++navigatedFrames;
    3030
    31     if (navigatedFrames == 10)
     31    if (navigatedFrames == 11)
    3232        checkResults();
    3333}
    … …  
    3838<body>
    3939
    40     <p>This test verifies that a sandboxed IFrame cannot navigate an ancestor
     40    <p>This test verifies that a sandboxed IFrame with the keywords
     41    allow-scripts and allow-same-origin is allowed to navigate an ancestor
    4142    in the frame tree by assigning the location attribute.</p>
    4243
    43     <p>This is done by loading ten non-sandboxed IFrames, and a single
    44     sandboxed one. Expect ten frames to be navigated, but the sandboxed
    45     one to <b>not</b> be one of them.</p>
    46    
    4744    <p>This test will print &quot;PASS&quot; on success.</p>
    4845

  • trunk/LayoutTests/http/tests/security/frameNavigation/context-for-location-assign-expected.txt

    r280256 r280826  
    1 document.referrer = http://127.0.0.1:8000/security/frameNavigation/context-for-location-assign.html
     1document.referrer =
    22

  • trunk/Source/WebCore/ChangeLog

    r280824 r280826  
     12021-08-09  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        Partially revert r280256 behavior change since it broke Facetime
     4        https://bugs.webkit.org/show_bug.cgi?id=228943
     5        rdar://81700268
     6
     7        Reviewed by Mark Lam.
     8
     9        In r280256, custom accessors start getting holder's JSGlobalObject (this is right behavior).
     10        We also introduced IncumbentWindow IDL attribute to get the caller's JSGlobalObject, which is
     11        necessary in Location. At that time, we replaced all ActiveWindow to IncumbentWindow, but it
     12        changes the JSGlobalObject of `replace`, `assign`, and `reload` since they are functions (not
     13        custom accessors). We observed that, after this change, facetime Web page is broken: when opening
     14        the link to Facetime, it should show the prompt of "Open in Facetime app" in iOS. But after this
     15        change, it won't happen.
     16
     17        We revert the semantic change by using ActiveWindow for replace, assign, and reload. This is
     18        not the correct semantics from the spec, but this is the same to one before r280256, and we
     19        will change this once we find the way to bypass this issue.
     20
     21        * page/Location.cpp:
     22        (WebCore::Location::assign):
     23        (WebCore::Location::replace):
     24        (WebCore::Location::reload):
     25        * page/Location.h:
     26        * page/Location.idl:
     27
    1282021-08-09  Chris Dumez  <cdumez@apple.com>
    229

  • trunk/Source/WebCore/page/Location.cpp

    r280333 r280826  
    211211}
    212212
    213 ExceptionOr<void> Location::assign(DOMWindow& incumbentWindow, DOMWindow& firstWindow, const String& url)
     213ExceptionOr<void> Location::assign(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
    214214{
    215215    if (!frame())
    216216        return { };
    217     return setLocation(incumbentWindow, firstWindow, url);
    218 }
    219 
    220 ExceptionOr<void> Location::replace(DOMWindow& incumbentWindow, DOMWindow& firstWindow, const String& urlString)
     217    return setLocation(activeWindow, firstWindow, url);
     218}
     219
     220ExceptionOr<void> Location::replace(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& urlString)
    221221{
    222222    auto* frame = this->frame();
    … …  
    235235
    236236    // We call DOMWindow::setLocation directly here because replace() always operates on the current frame.
    237     frame->document()->domWindow()->setLocation(incumbentWindow, completedURL, LockHistoryAndBackForwardList);
     237    frame->document()->domWindow()->setLocation(activeWindow, completedURL, LockHistoryAndBackForwardList);
    238238    return { };
    239239}
    240240
    241 void Location::reload(DOMWindow& incumbentWindow)
     241void Location::reload(DOMWindow& activeWindow)
    242242{
    243243    auto* frame = this->frame();
    … …  
    245245        return;
    246246
    247     ASSERT(incumbentWindow.document());
     247    ASSERT(activeWindow.document());
    248248    ASSERT(frame->document());
    249249    ASSERT(frame->document()->domWindow());
    250250
    251     auto& activeDocument = *incumbentWindow.document();
     251    auto& activeDocument = *activeWindow.document();
    252252    auto& targetDocument = *frame->document();
    253253
    … …  
    257257    if (!activeDocument.securityOrigin().isSameOriginDomain(targetDocument.securityOrigin())) {
    258258        auto& targetWindow = *targetDocument.domWindow();
    259         targetWindow.printErrorMessage(targetWindow.crossDomainAccessErrorMessage(incumbentWindow, IncludeTargetOrigin::Yes));
     259        targetWindow.printErrorMessage(targetWindow.crossDomainAccessErrorMessage(activeWindow, IncludeTargetOrigin::Yes));
    260260        return;
    261261    }

  • trunk/Source/WebCore/page/Location.h

    r280313 r280826  
    4646    String href() const;
    4747
    48     ExceptionOr<void> assign(DOMWindow& incumbentWindow, DOMWindow& firstWindow, const String&);
    49     ExceptionOr<void> replace(DOMWindow& incumbentWindow, DOMWindow& firstWindow, const String&);
    50     void reload(DOMWindow& incumbentWindow);
     48    ExceptionOr<void> assign(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     49    ExceptionOr<void> replace(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     50    void reload(DOMWindow& activeWindow);
    5151
    5252    ExceptionOr<void> setProtocol(DOMWindow& incumbentWindow, DOMWindow& firstWindow, const String&);

  • trunk/Source/WebCore/page/Location.idl

    r280256 r280826  
    4646    [SetterCallWith=IncumbentWindow&FirstWindow, DoNotCheckSecurityOnSetter] stringifier attribute USVString href;
    4747
    48     [CallWith=IncumbentWindow&FirstWindow] undefined assign(USVString url);
    49     [CallWith=IncumbentWindow&FirstWindow, DoNotCheckSecurity] undefined replace(USVString url);
    50     [CallWith=IncumbentWindow] undefined reload();
     48    // FIXME: We should use IncumbentWindow once we found why https://bugs.webkit.org/show_bug.cgi?id=228943 issue occured.
     49    [CallWith=ActiveWindow&FirstWindow] undefined assign(USVString url);
     50    [CallWith=ActiveWindow&FirstWindow, DoNotCheckSecurity] undefined replace(USVString url);
     51    [CallWith=ActiveWindow] undefined reload();
    5152
    5253    // URI decomposition attributes
Note: See TracChangeset for help on using the changeset viewer.