|
Description
Chris Dumez
2018-04-25 13:30:00 PDT
Created attachment 338788 [details]
WIP patch
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 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
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 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
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 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
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 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
Created attachment 338825 [details]
WIP patch
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 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
Created attachment 339215 [details]
WIP patch
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 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
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 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
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 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
Created attachment 339242 [details]
WIP patch
Created attachment 339897 [details]
WIP patch
Working on adding more testing.
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.
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 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
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 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
Created attachment 339973 [details]
WIP patch
Created attachment 339978 [details]
WIP patch
Created attachment 339980 [details]
WIP patch
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 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
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 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
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 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
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 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
Created attachment 340010 [details]
Patch
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 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
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 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
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 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
Created attachment 340039 [details]
Patch
Created attachment 340041 [details]
Patch
Created attachment 340042 [details]
Patch
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)? 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. 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 ^^^ 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.
Created attachment 340046 [details]
Patch
Comment on attachment 340046 [details]
Patch
Looks good to me with the latest changes.
Created attachment 340047 [details]
Patch
(In reply to Brent Fulgham from comment #54) > Comment on attachment 340046 [details] > Patch > > Looks good to me with the latest changes. Thanks. Created attachment 340049 [details]
Patch
(In reply to Chris Dumez from comment #57) > Created attachment 340049 [details] > Patch Turned on the feature by default as suggested by Geoff offline. Comment on attachment 340049 [details] Patch Clearing flags on attachment: 340049 Committed r231622: <https://trac.webkit.org/changeset/231622> All reviewed patches have been landed. Closing bug. Windows build fix: <https://trac.webkit.org/changeset/231625> Has a spec for Cross-Origin-Options been published somewhere other than just in the changelog? (https://trac.webkit.org/changeset/231622/webkit) Windows port crashes soon. I'm going to fix it. Bug 185505 – REGRESSION(r231622) [Win] Crashes for null dereference of prefsPrivate in WebView::notifyPreferencesChanged 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. 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. |