Bug 184996 – Add initial support for 'Cross-Origin-Options' HTTP response header

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Add initial support for 'X-Disown-Opener' HTTP response header which severs the link between the window and its opener upon loading.

[Chris Dumez]

<rdar://problem/39664620>

[Chris Dumez]

Created attachment 338788 [details]
WIP patch

[EWS Watchlist]

Comment on attachment 338788 [details]
WIP patch

Attachment 338788 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7458542

New failing tests:
http/tests/navigation/disown-opener-header-same-origin.html
http/tests/navigation/disown-opener-header-cross-origin.html

[EWS Watchlist]

Created attachment 338803 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 338788 [details]
WIP patch

Attachment 338788 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7459393

New failing tests:
http/tests/navigation/disown-opener-header-same-origin.html
http/tests/navigation/disown-opener-header-cross-origin.html

[EWS Watchlist]

Created attachment 338811 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 338788 [details]
WIP patch

Attachment 338788 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7459032

New failing tests:
http/tests/navigation/disown-opener-header-same-origin.html
http/tests/navigation/disown-opener-header-cross-origin.html

[EWS Watchlist]

Created attachment 338812 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 338788 [details]
WIP patch

Attachment 338788 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7460128

New failing tests:
http/tests/navigation/disown-opener-header-same-origin.html
http/tests/navigation/disown-opener-header-cross-origin.html

[EWS Watchlist]

Created attachment 338820 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 338825 [details]
WIP patch

[EWS Watchlist]

Comment on attachment 338825 [details]
WIP patch

Attachment 338825 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7461914

New failing tests:
http/tests/navigation/disown-opener-header-same-origin.html
http/tests/navigation/disown-opener-header-cross-origin.html

[EWS Watchlist]

Created attachment 338840 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 339215 [details]
WIP patch

[EWS Watchlist]

Comment on attachment 339215 [details]
WIP patch

Attachment 339215 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7527805

New failing tests:
http/tests/navigation/x-frame-isolate-popup.html

[EWS Watchlist]

Created attachment 339223 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 339215 [details]
WIP patch

Attachment 339215 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7527947

New failing tests:
http/tests/navigation/x-frame-isolate-popup.html

[EWS Watchlist]

Created attachment 339225 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 339215 [details]
WIP patch

Attachment 339215 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7528257

New failing tests:
http/tests/navigation/x-frame-isolate-popup.html

[EWS Watchlist]

Created attachment 339226 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 339242 [details]
WIP patch

[Chris Dumez]

Created attachment 339897 [details]
WIP patch

Working on adding more testing.

[EWS Watchlist]

Attachment 339897 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/HTTPParsers.h:102:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 339897 [details]
WIP patch

Attachment 339897 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7618268

New failing tests:
http/wpt/cross-origin-options/cross-origin-options-header.html

[EWS Watchlist]

Created attachment 339922 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 339897 [details]
WIP patch

Attachment 339897 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7618435

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html
http/tests/security/canvas-remote-read-remote-video-localhost.html
http/tests/security/webaudio-render-remote-audio-allowed-crossorigin.html

[EWS Watchlist]

Created attachment 339923 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Chris Dumez]

Created attachment 339973 [details]
WIP patch

[Chris Dumez]

Created attachment 339978 [details]
WIP patch

[Chris Dumez]

Created attachment 339980 [details]
WIP patch

[EWS Watchlist]

Comment on attachment 339980 [details]
WIP patch

Attachment 339980 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7625565

New failing tests:
http/wpt/cross-origin-options/allow-postmessage.html

[EWS Watchlist]

Created attachment 339995 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 339980 [details]
WIP patch

Attachment 339980 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7625976

New failing tests:
http/wpt/cross-origin-options/allow-postmessage.html

[EWS Watchlist]

Created attachment 339998 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 339980 [details]
WIP patch

Attachment 339980 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7625715

New failing tests:
http/wpt/cross-origin-options/allow-postmessage.html

[EWS Watchlist]

Created attachment 340000 [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.13.4

[EWS Watchlist]

Comment on attachment 339980 [details]
WIP patch

Attachment 339980 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7626237

New failing tests:
http/wpt/cross-origin-options/allow-postmessage.html

[EWS Watchlist]

Created attachment 340007 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 340010 [details]
Patch

[EWS Watchlist]

Comment on attachment 340010 [details]
Patch

Attachment 340010 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7627488

New failing tests:
http/wpt/cross-origin-options/allow-postmessage-from-deny.html
http/wpt/cross-origin-options/allow-postmessage.html
http/wpt/cross-origin-options/cross-origin-options-header.html

[EWS Watchlist]

Created attachment 340022 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340010 [details]
Patch

Attachment 340010 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7627972

New failing tests:
http/wpt/cross-origin-options/allow-postmessage.html

[EWS Watchlist]

Created attachment 340031 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340010 [details]
Patch

Attachment 340010 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7627740

New failing tests:
http/wpt/cross-origin-options/allow-postmessage.html

[EWS Watchlist]

Created attachment 340035 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Chris Dumez]

Created attachment 340039 [details]
Patch

[Chris Dumez]

Created attachment 340041 [details]
Patch

[Chris Dumez]

Created attachment 340042 [details]
Patch

[Geoffrey Garen]

Comment on attachment 340042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340042&action=review

r=me if EWS is happy

> Source/WebCore/dom/Document.cpp:7815
> +void Document::setCrossOriginOptions(CrossOriginOptions value)
> +{
> +    m_crossOriginOptions = value;
> +    if (auto* window = domWindow())
> +        window->setCrossOriginOptions(value);
> +}

Do we need to propagate m_crossOriginOptions inside Document::createDOMWindow() (covering the case where domWindow() is null at this point)?

[Chris Dumez]

Comment on attachment 340042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340042&action=review

>> Source/WebCore/dom/Document.cpp:7815
>> +}
> 
> Do we need to propagate m_crossOriginOptions inside Document::createDOMWindow() (covering the case where domWindow() is null at this point)?

No.

> Source/WebCore/page/DOMWindow.cpp:405
> +    : AbstractDOMWindow(GlobalWindowIdentifier { Process::identifier(), generateObjectIdentifier<WindowIdentifierType>() }, document.crossOriginOptions())

because we already take care of grabbing the options from the Document when constructing the Window.

> Source/WebCore/page/DOMWindow.cpp:416
> +    setCrossOriginOptions(document.crossOriginOptions());

And even when moving a DOMWindow from one document to another.

[Brent Fulgham]

Comment on attachment 340042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340042&action=review

> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:229
>      HRESULT setMenuItemElementEnabled([in] BOOL enabled);

}

[uuid(<<Generate and fill in some kind of UUID here>>)]
interface IWebPreferencesPrivate7 : IWebPreferencesPrivate6
{

> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:231
> +    HRESULT setCrossOriginOptionsSupportEnabled([in] BOOL enabled);

You can't add these here without breaking existing WebKit clients on Windows (e.g., iTunes).

Instead, we have to create a new IWebPreferencesPrivate7 with these two new values.

> Source/WebKitLegacy/win/WebView.cpp:5152
>  

COMPtr<IWebPreferencesPrivate7> prefsPrivate { Query, preferences };

> Source/WebKitLegacy/win/WebView.cpp:5158
>      COMPtr<IWebPreferencesPrivate6> prefsPrivate { Query, preferences };

Then delete this one line ^^^

[Brent Fulgham]

Comment on attachment 340042 [details]
Patch

r- because this will break Windows clients. I've tried to give you the code changes to make for this to work safely.

[Chris Dumez]

Created attachment 340046 [details]
Patch

[Brent Fulgham]

Comment on attachment 340046 [details]
Patch

Looks good to me with the latest changes.

[Chris Dumez]

Created attachment 340047 [details]
Patch

[Chris Dumez]

(In reply to Brent Fulgham from comment #54)
> Comment on attachment 340046 [details]
> Patch
> 
> Looks good to me with the latest changes.

Thanks.

[Chris Dumez]

Created attachment 340049 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #57)
> Created attachment 340049 [details]
> Patch

Turned on the feature by default as suggested by Geoff offline.

[WebKit Commit Bot]

Comment on attachment 340049 [details]
Patch

Clearing flags on attachment: 340049

Committed r231622: <https://trac.webkit.org/changeset/231622>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Windows build fix: <https://trac.webkit.org/changeset/231625>

[sideshowbarker]

Has a spec for Cross-Origin-Options been published somewhere other than just in the changelog? (https://trac.webkit.org/changeset/231622/webkit)

[Fujii Hironori]

Windows port crashes soon. I'm going to fix it.

Bug 185505 – REGRESSION(r231622) [Win] Crashes for null dereference of prefsPrivate in WebView::notifyPreferencesChanged

[Daniel Bates]

Comment on attachment 340049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340049&action=review

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:248
> +    auto isCrossOriginAccess = [&] {

Not at a computer with a checkout at the moment. I take it the return value and error message can change after calling BindingSecurity::shouldAllowAccessToDOMWindow() more than once with the same arguments and hence we need to use a lambda and 2 local variable as opposed to just a 2 local variable to cache its result and error message?

> Source/WebCore/dom/Document.cpp:7812
> +    m_crossOriginOptions = value;

Is it necessary to store this state on the document given that we also store it on the window?

> Source/WebCore/loader/FrameLoader.cpp:748
> +            if (!crossOriginOptionsHeader.isEmpty())

This code said that and empty header is treated analogous to “allow”. For your consideration, I suggest that this logic be part of parseCrossOriginOptionsHeader() so as to centralize all the parsing and default semantics related to this header similar to how we parse X-Frame-Options and then update this code to only call the parse routine if crossOriginOptionsHeader is non-null (as the null string is an artifact of how ResourceResponse::httpHeaderField() works).

> Source/WebCore/platform/network/HTTPParsers.cpp:918
> +    auto strippedHeader = stripLeadingAndTrailingHTTPSpaces(header);

I would have stored the return value in header as I do not feel that “stripped” adds much value, header is copied by value, and we can re-use the stack-space of the parameter instead of stack-allocating space for a local.

> Source/WebCore/platform/network/HTTPParsers.cpp:919
> +    if (strippedHeader.isEmpty())

Look at this. You handle the empty string case. This conditional always evaluates to false because we never invoke this function when header is the empty string by line 748 of FrameLoader.cpp. And FrameLoader is the only caller of this function. See my remark in that file. If you have your heart set on keeping line 748 as-is then we should remove this line and line 920 and add an assertion that header is a non-empty string.

Even better, just remove this code and let us fall through to the default case on line 928.

> LayoutTests/http/wpt/cross-origin-options/cross-origin-options-header-expected.txt:3
> +PASS Cross-origin iframe with 'Cross-Origin-Options: deny' HTTP header 

We should add tests for mIXED caSe header values since you use equalLettersIgnoringASCIICasein the parsing code. Also, we should add tests for when multiple Cross-Origin-Options are given in the same HTTP response.

[Chris Dumez]

Comment on attachment 340049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340049&action=review

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:248
>> +    auto isCrossOriginAccess = [&] {
> 
> Not at a computer with a checkout at the moment. I take it the return value and error message can change after calling BindingSecurity::shouldAllowAccessToDOMWindow() more than once with the same arguments and hence we need to use a lambda and 2 local variable as opposed to just a 2 local variable to cache its result and error message?

Not sure I understand your comment. The result can change when called several times, which is why I cannot cache the result in a static.
The reason I have a lambda is to lazily initialize the cache value because there is one (common) code path that does not require a cross-origin check (There is a frame with this index and crossOriginOptions is Allow). Using a lambda avoids duplicating this code in other code paths.

>> Source/WebCore/dom/Document.cpp:7812
>> +    m_crossOriginOptions = value;
> 
> Is it necessary to store this state on the document given that we also store it on the window?

It'd be nice indeed but it is a little tricky because you can have a Document without a Window (and vice-versa). I'll dig deeper into it to see if this is feasible and follow-up if I can.

>> Source/WebCore/loader/FrameLoader.cpp:748
>> +            if (!crossOriginOptionsHeader.isEmpty())
> 
> This code said that and empty header is treated analogous to “allow”. For your consideration, I suggest that this logic be part of parseCrossOriginOptionsHeader() so as to centralize all the parsing and default semantics related to this header similar to how we parse X-Frame-Options and then update this code to only call the parse routine if crossOriginOptionsHeader is non-null (as the null string is an artifact of how ResourceResponse::httpHeaderField() works).

parseCrossOriginOptionsHeader() already does the right thing when passed an empty string so it would be fine to use isNull() here. I would do an unnecessary function call but this is not hot code anyway.

>> LayoutTests/http/wpt/cross-origin-options/cross-origin-options-header-expected.txt:3
>> +PASS Cross-origin iframe with 'Cross-Origin-Options: deny' HTTP header 
> 
> We should add tests for mIXED caSe header values since you use equalLettersIgnoringASCIICasein the parsing code. Also, we should add tests for when multiple Cross-Origin-Options are given in the same HTTP response.

Good idea, will extend testing.