REGRESSION (Safari 16.3): Passing "noopener" as third argument opens …

…tabs into new window

https://bugs.webkit.org/show_bug.cgi?id=259093
rdar://112086061

Reviewed by BJ Burg and Chris Dumez.

The changes herein bring WebKit up to the HTML specs for window.open().

First, this commit now parses the popup field which is specified in the specs but
was missing in our parser. We also start parsing the resizable field which is defined
in our API but was never parsed. Finally, we mark all other member fields of optional
rather than setting a default value for each of them.

Second, this adds the member fields wantsPopup and hasAdditionalFeatures:
hasAdditionalFeatures is serialized and sent to the UI Process to indicate that we
detected other strings than the ones we normally parse.

The field wantsPopup implements the algorithm described here:
https://html.spec.whatwg.org/multipage/nav-history-apis.html#apis-for-creating-and-navigating-browsing-contexts-by-name
aimed at determining whether we should display a popup or not. Its value is determined
in the following way:
  - features noopener and noreferrer do not count towards the tested feature string.
  - empty features (two commas in a row), strings made of spaces only do not towards
    the tested feature string. So spaces and commas don't make the string not empty.
  - every other words or characters are counted.

As such the first test to determine the value of wantsPopup is to check whether any
feature is defined or if the string contains additional features, the latter flag is
only set if the string contains anything but spaces and commas. This matches the
behavior from other browsers and prescribed by the specs.

Finally, despite being declared as nullable, the corresponding properties in WKWindowFeatures
could never be in fact nil; changing all member fields in WindowFeatures to std::optional
allows us to return nil in the corresponding properties when those features are missing.
We also add private properties for the corresponding WindowFeatures member fields.

These changes ostensibly fixes the issue in question since we needed the ability to check
for the absence of the window features. However, to be correct a browser should adopt the
new properties as well.

* Source/WebCore/loader/FrameLoader.cpp:
(WebCore::createWindow):

* Source/WebCore/page/Chrome.cpp:
(WebCore::Chrome::createWindow):

* Source/WebCore/page/LocalDOMWindow.cpp:
(WebCore::LocalDOMWindow::createWindow):

* Source/WebCore/page/WindowFeatures.cpp:
(WebCore::WindowFeatures::wantsPopup const):
  Implements the specs algorithm to check if we need to get a popup. No features declared
  besides noopener and noreferrer means we do not want a popup.

(WebCore::parseWindowFeatures):
  Stop setting default values for the window features to avoid indicating that those features
  were declared in the string. Compute wantsPopup after we are done parsing the string.

(WebCore::setWindowFeature):
  Add a parse step for the popup feature and set the additional features when they are not empty
  strings or strings made of spaces.

(WebCore::parseDialogFeatures):
  Dialogs are always popups.

* Source/WebCore/page/WindowFeatures.h:
  Make all potential features optional. Add convenience member functions to use instead of
  using noopener and noreferrer directly.

(WebCore::WindowFeatures::wantsNoOpener const):
(WebCore::WindowFeatures::wantsNoReferrer const):

* Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in:
  Match the declaration above. Serialize the three new fields.

* Source/WebKit/UIProcess/API/C/WKPage.cpp:
(WKPageSetPageUIClient):
  Save the new fields in the window feature dictionary for legacy clients.

* Source/WebKit/UIProcess/API/Cocoa/WKWindowFeatures.mm:
(-[WKWindowFeatures menuBarVisibility]):
(-[WKWindowFeatures statusBarVisibility]):
(-[WKWindowFeatures toolbarsVisibility]):
(-[WKWindowFeatures allowsResizing]):
(-[WKWindowFeatures _wantsPopup]):
(-[WKWindowFeatures _hasAdditionalFeatures]):
(-[WKWindowFeatures _popup]):
(-[WKWindowFeatures _locationBarVisibility]):
(-[WKWindowFeatures _scrollbarsVisibility]):
(-[WKWindowFeatures _fullscreenDisplay]):
(-[WKWindowFeatures _dialogDisplay]):

* Source/WebKit/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h:
  Check if the value is defined and return it as a boolean otherwise return nil.

* Source/WebKit/UIProcess/API/glib/WebKitWindowProperties.cpp:
  Convert optional features into glib API.

(webkitWindowPropertiesSetGeometry):
(webkitWindowPropertiesSetWantsPopup):

* Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:
(WebChromeClient::createWindow):
  Write the new fields in the window feature dictionary for legacy clients.
  Only write optional fields if they are not null.

* Tools/TestWebKitAPI/Tests/WebKitCocoa/OpenAndCloseWindow.mm:
(TEST(WebKit, OpenWindowFeatures)):
  - Reenable the tests checking for the nil case when the string is empty.
  - Add checks for the new fields in the API.
  - Add tests for noopener and noreferrer features.
  - Add test for hasAdditionalFeatures.
  - Add test for a string containing only separators.

Canonical link: https://commits.webkit.org/267354@main

Source/WebCore/loader/FrameLoader.cpp

@@ -4420,7 +4420,7 @@ RefPtr<LocalFrame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame
 
     ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy = shouldOpenExternalURLsPolicyToApply(openerFrame, request);
     NavigationAction action { request.requester(), request.resourceRequest(), request.initiatedByMainFrame(), NavigationType::Other, shouldOpenExternalURLsPolicy };
-    action.setNewFrameOpenerPolicy(features.noopener || features.noreferrer ? NewFrameOpenerPolicy::Suppress : NewFrameOpenerPolicy::Allow);
+    action.setNewFrameOpenerPolicy(features.wantsNoOpener() ? NewFrameOpenerPolicy::Suppress : NewFrameOpenerPolicy::Allow);
     Page* page = oldPage->chrome().createWindow(openerFrame, features, action);
     if (!page)
         return nullptr;
@@ -4439,19 +4439,23 @@ RefPtr<LocalFrame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame
 
     if (!frame->page())
         return nullptr;
-    page->chrome().setStatusbarVisible(features.statusBarVisible);
+    if (features.statusBarVisible)
+        page->chrome().setStatusbarVisible(*features.statusBarVisible);
 
     if (!frame->page())
         return nullptr;
-    page->chrome().setScrollbarsVisible(features.scrollbarsVisible);
+    if (features.scrollbarsVisible)
+        page->chrome().setScrollbarsVisible(*features.scrollbarsVisible);
 
     if (!frame->page())
         return nullptr;
-    page->chrome().setMenubarVisible(features.menuBarVisible);
+    if (features.menuBarVisible)
+        page->chrome().setMenubarVisible(*features.menuBarVisible);
 
     if (!frame->page())
         return nullptr;
-    page->chrome().setResizable(features.resizable);
+    if (features.resizable)
+        page->chrome().setResizable(*features.resizable);
 
     // 'x' and 'y' specify the location of the window, while 'width' and 'height'
     // specify the size of the viewport. We can only resize the window, so adjust

Source/WebCore/page/Chrome.cpp

@@ -198,7 +198,7 @@ Page* Chrome::createWindow(LocalFrame& frame, const WindowFeatures& features, co
     if (!newPage)
         return nullptr;
 
-    if (!features.noopener && !features.noreferrer)
+    if (!features.wantsNoOpener())
         m_page.storageNamespaceProvider().copySessionStorageNamespace(m_page, *newPage);
 
     return newPage;

Source/WebCore/page/LocalDOMWindow.cpp

@@ -2607,7 +2607,7 @@ ExceptionOr<RefPtr<LocalFrame>> LocalDOMWindow::createWindow(const String& urlSt
     WindowFeatures windowFeatures = initialWindowFeatures;
 
     // For whatever reason, Firefox uses the first frame to determine the outgoingReferrer. We replicate that behavior here.
-    String referrer = windowFeatures.noreferrer ? String() : SecurityPolicy::generateReferrerHeader(firstFrame.document()->referrerPolicy(), completedURL, firstFrame.loader().outgoingReferrer(), OriginAccessPatternsForWebProcess::singleton());
+    String referrer = windowFeatures.wantsNoReferrer() ? String() : SecurityPolicy::generateReferrerHeader(firstFrame.document()->referrerPolicy(), completedURL, firstFrame.loader().outgoingReferrer(), OriginAccessPatternsForWebProcess::singleton());
     auto initiatedByMainFrame = activeFrame->isMainFrame() ? InitiatedByMainFrame::Yes : InitiatedByMainFrame::Unknown;
 
     ResourceRequest resourceRequest { completedURL, referrer };
@@ -2624,7 +2624,7 @@ ExceptionOr<RefPtr<LocalFrame>> LocalDOMWindow::createWindow(const String& urlSt
     if (!newFrame)
         return RefPtr<LocalFrame> { nullptr };
 
-    bool noopener = windowFeatures.noopener || windowFeatures.noreferrer;
+    bool noopener = windowFeatures.wantsNoOpener();
     if (!noopener)
         newFrame->loader().setOpener(&openerFrame);
 

Source/WebCore/page/WindowFeatures.cpp

@@ -53,25 +53,11 @@ static bool isSeparator(UChar character, FeatureMode mode)
 
 WindowFeatures parseWindowFeatures(StringView featuresString)
 {
-    // The IE rule is: all features except for channelmode and fullscreen default to YES, but
-    // if the user specifies a feature string, all features default to NO. (There is no public
-    // standard that applies to this method.)
-    //
-    // <http://msdn.microsoft.com/workshop/author/dhtml/reference/methods/open_0.asp>
-    // We always allow a window to be resized, which is consistent with Firefox.
-
     WindowFeatures features;
 
     if (featuresString.isEmpty())
         return features;
 
-    features.menuBarVisible = false;
-    features.statusBarVisible = false;
-    features.toolBarVisible = false;
-    features.locationBarVisible = false;
-    features.scrollbarsVisible = false;
-    features.noopener = false;
-
     processFeaturesString(featuresString, FeatureMode::Window, [&features](StringView key, StringView value) {
         setWindowFeature(features, key, value);
     });
@@ -147,6 +133,8 @@ static void setWindowFeature(WindowFeatures& features, StringView key, StringVie
         features.width = numericValue;
     else if (equalLettersIgnoringASCIICase(key, "height"_s) || equalLettersIgnoringASCIICase(key, "innerheight"_s))
         features.height = numericValue;
+    else if (equalLettersIgnoringASCIICase(key, "popup"_s))
+        features.popup = numericValue;
     else if (equalLettersIgnoringASCIICase(key, "menubar"_s))
         features.menuBarVisible = numericValue;
     else if (equalLettersIgnoringASCIICase(key, "toolbar"_s))
@@ -159,12 +147,17 @@ static void setWindowFeature(WindowFeatures& features, StringView key, StringVie
         features.fullscreen = numericValue;
     else if (equalLettersIgnoringASCIICase(key, "scrollbars"_s))
         features.scrollbarsVisible = numericValue;
+    else if (equalLettersIgnoringASCIICase(key, "resizable"_s))
+        features.resizable = numericValue;
     else if (equalLettersIgnoringASCIICase(key, "noopener"_s))
         features.noopener = numericValue;
     else if (equalLettersIgnoringASCIICase(key, "noreferrer"_s))
         features.noreferrer = numericValue;
-    else if (numericValue == 1)
-        features.additionalFeatures.append(key.toString());
+    else if (key.length() || value.length()) {
+        features.hasAdditionalFeatures = true;
+        if (numericValue == 1)
+            features.additionalFeatures.append(key.toString());
+    }
 }
 
 WindowFeatures parseDialogFeatures(StringView dialogFeaturesString, const FloatRect& screenAvailableRect)

Source/WebCore/page/WindowFeatures.h

@@ -39,24 +39,77 @@ namespace WebCore {
 class FloatRect;
 
 struct WindowFeatures {
+    bool hasAdditionalFeatures { false };
+
     std::optional<float> x;
     std::optional<float> y;
     std::optional<float> width;
     std::optional<float> height;
 
-    bool menuBarVisible { true };
-    bool statusBarVisible { true };
-    bool toolBarVisible { true };
-    bool locationBarVisible { true };
-    bool scrollbarsVisible { true };
-    bool resizable { true };
+    std::optional<bool> popup;
+    std::optional<bool> menuBarVisible;
+    std::optional<bool> statusBarVisible;
+    std::optional<bool> toolBarVisible;
+    std::optional<bool> locationBarVisible;
+    std::optional<bool> scrollbarsVisible;
+    std::optional<bool> resizable;
 
-    bool fullscreen { false };
-    bool dialog { false };
-    bool noopener { false };
-    bool noreferrer { false };
+    std::optional<bool> fullscreen;
+    std::optional<bool> dialog;
+    std::optional<bool> noopener { std::nullopt };
+    std::optional<bool> noreferrer { std::nullopt };
 
     Vector<String> additionalFeatures { };
+
+    bool wantsNoOpener() const { return (noopener && *noopener) || (noreferrer && *noreferrer); }
+    bool wantsNoReferrer() const { return (noreferrer && *noreferrer); }
+
+    // Follow the HTML standard on how to parse the window features indicated here:
+    // https://html.spec.whatwg.org/multipage/nav-history-apis.html#apis-for-creating-and-navigating-browsing-contexts-by-name
+    bool wantsPopup() const
+    {
+        // If the WindowFeatures string contains nothing more than noopener and noreferrer we
+        // consider the string to be empty and thus return false based on the algorithm above.
+        if (!hasAdditionalFeatures
+            && !x
+            && !y
+            && !width
+            && !height
+            && !popup
+            && !menuBarVisible
+            && !statusBarVisible
+            && !toolBarVisible
+            && !locationBarVisible
+            && !scrollbarsVisible
+            && !resizable)
+            return false;
+
+        // If popup is defined, return its value as a boolean.
+        if (popup)
+            return *popup;
+
+        // If location (default to false) and toolbar (default to false) are false return true.
+        if ((!locationBarVisible || !*locationBarVisible) && (!toolBarVisible || !*toolBarVisible))
+            return true;
+
+        // If menubar (default to false) is false return true.
+        if (!menuBarVisible || !*menuBarVisible)
+            return true;
+
+        // If resizable (default to true) is false return true.
+        if (resizable && !*resizable)
+            return true;
+
+        // If scrollbars (default to false) is false return false.
+        if (!scrollbarsVisible || !*scrollbarsVisible)
+            return true;
+
+        // If status (default to false) is false return true.
+        if (!statusBarVisible || !*statusBarVisible)
+            return true;
+
+        return false;
+    }
 };
 
 WindowFeatures parseWindowFeatures(StringView windowFeaturesString);

Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in

@@ -1858,22 +1858,25 @@ enum class WebCore::DiagnosticLoggingDomain : uint8_t {
 };
 
 struct WebCore::WindowFeatures {
+    bool hasAdditionalFeatures;
+
     std::optional<float> x;
     std::optional<float> y;
     std::optional<float> width;
     std::optional<float> height;
 
-    bool menuBarVisible;
-    bool statusBarVisible;
-    bool toolBarVisible;
-    bool locationBarVisible;
-    bool scrollbarsVisible;
-    bool resizable;
-
-    bool fullscreen;
-    bool dialog;
-    [NotSerialized] bool noopener;
-    [NotSerialized] bool noreferrer;
+    std::optional<bool> popup;
+    std::optional<bool> menuBarVisible;
+    std::optional<bool> statusBarVisible;
+    std::optional<bool> toolBarVisible;
+    std::optional<bool> locationBarVisible;
+    std::optional<bool> scrollbarsVisible;
+    std::optional<bool> resizable;
+
+    std::optional<bool> fullscreen;
+    std::optional<bool> dialog;
+    [NotSerialized] std::optional<bool> noopener;
+    [NotSerialized] std::optional<bool> noreferrer;
 
     [NotSerialized] Vector<String> additionalFeatures;
 };

Source/WebKit/UIProcess/API/C/WKPage.cpp

@@ -1599,6 +1599,8 @@ void WKPageSetPageUIClient(WKPageRef pageRef, const WKPageUIClientBase* wkClient
 
             if (m_client.createNewPage_deprecatedForUseWithV1 || m_client.createNewPage_deprecatedForUseWithV0) {
                 API::Dictionary::MapType map;
+                map.set("wantsPopup"_s, API::Boolean::create(windowFeatures.wantsPopup()));
+                map.set("hasAdditionalFeatures"_s, API::Boolean::create(windowFeatures.hasAdditionalFeatures));
                 if (windowFeatures.x)
                     map.set("x"_s, API::Double::create(*windowFeatures.x));
                 if (windowFeatures.y)
@@ -1607,14 +1609,24 @@ void WKPageSetPageUIClient(WKPageRef pageRef, const WKPageUIClientBase* wkClient
                     map.set("width"_s, API::Double::create(*windowFeatures.width));
                 if (windowFeatures.height)
                     map.set("height"_s, API::Double::create(*windowFeatures.height));
-                map.set("menuBarVisible"_s, API::Boolean::create(windowFeatures.menuBarVisible));
-                map.set("statusBarVisible"_s, API::Boolean::create(windowFeatures.statusBarVisible));
-                map.set("toolBarVisible"_s, API::Boolean::create(windowFeatures.toolBarVisible));
-                map.set("locationBarVisible"_s, API::Boolean::create(windowFeatures.locationBarVisible));
-                map.set("scrollbarsVisible"_s, API::Boolean::create(windowFeatures.scrollbarsVisible));
-                map.set("resizable"_s, API::Boolean::create(windowFeatures.resizable));
-                map.set("fullscreen"_s, API::Boolean::create(windowFeatures.fullscreen));
-                map.set("dialog"_s, API::Boolean::create(windowFeatures.dialog));
+                if (windowFeatures.popup)
+                    map.set("popup"_s, API::Boolean::create(*windowFeatures.popup));
+                if (windowFeatures.menuBarVisible)
+                    map.set("menuBarVisible"_s, API::Boolean::create(*windowFeatures.menuBarVisible));
+                if (windowFeatures.statusBarVisible)
+                    map.set("statusBarVisible"_s, API::Boolean::create(*windowFeatures.statusBarVisible));
+                if (windowFeatures.toolBarVisible)
+                    map.set("toolBarVisible"_s, API::Boolean::create(*windowFeatures.toolBarVisible));
+                if (windowFeatures.locationBarVisible)
+                    map.set("locationBarVisible"_s, API::Boolean::create(*windowFeatures.locationBarVisible));
+                if (windowFeatures.scrollbarsVisible)
+                    map.set("scrollbarsVisible"_s, API::Boolean::create(*windowFeatures.scrollbarsVisible));
+                if (windowFeatures.resizable)
+                    map.set("resizable"_s, API::Boolean::create(*windowFeatures.resizable));
+                if (windowFeatures.fullscreen)
+                    map.set("fullscreen"_s, API::Boolean::create(*windowFeatures.fullscreen));
+                if (windowFeatures.dialog)
+                    map.set("dialog"_s, API::Boolean::create(*windowFeatures.dialog));
                 Ref<API::Dictionary> featuresMap = API::Dictionary::create(WTFMove(map));
 
                 if (m_client.createNewPage_deprecatedForUseWithV1) {

Source/WebKit/UIProcess/API/Cocoa/WKWindowFeatures.mm

@@ -44,22 +44,34 @@ - (void)dealloc
 
 - (NSNumber *)menuBarVisibility
 {
-    return @(_windowFeatures->windowFeatures().menuBarVisible);
+    if (auto menuBarVisible = _windowFeatures->windowFeatures().menuBarVisible)
+        return @(*menuBarVisible);
+
+    return nil;
 }
 
 - (NSNumber *)statusBarVisibility
 {
-    return @(_windowFeatures->windowFeatures().statusBarVisible);
+    if (auto statusBarVisible = _windowFeatures->windowFeatures().statusBarVisible)
+        return @(*statusBarVisible);
+
+    return nil;
 }
 
 - (NSNumber *)toolbarsVisibility
 {
-    return @(_windowFeatures->windowFeatures().toolBarVisible);
+    if (auto toolBarVisible = _windowFeatures->windowFeatures().toolBarVisible)
+        return @(*toolBarVisible);
+
+    return nil;
 }
 
 - (NSNumber *)allowsResizing
 {
-    return @(_windowFeatures->windowFeatures().resizable);
+    if (auto resizable = _windowFeatures->windowFeatures().resizable)
+        return @(*resizable);
+
+    return nil;
 }
 
 - (NSNumber *)x
@@ -105,24 +117,54 @@ - (NSNumber *)height
 
 @implementation WKWindowFeatures (WKPrivate)
 
+- (BOOL)_wantsPopup
+{
+    return _windowFeatures->windowFeatures().wantsPopup();
+}
+
+- (BOOL)_hasAdditionalFeatures
+{
+    return _windowFeatures->windowFeatures().hasAdditionalFeatures;
+}
+
+- (NSNumber *)_popup
+{
+    if (auto popup = _windowFeatures->windowFeatures().popup)
+        return @(*popup);
+
+    return nil;
+}
+
 - (NSNumber *)_locationBarVisibility
 {
-    return @(_windowFeatures->windowFeatures().locationBarVisible);
+    if (auto locationBarVisible = _windowFeatures->windowFeatures().locationBarVisible)
+        return @(*locationBarVisible);
+
+    return nil;
 }
 
 - (NSNumber *)_scrollbarsVisibility
 {
-    return @(_windowFeatures->windowFeatures().scrollbarsVisible);
+    if (auto scrollbarsVisible = _windowFeatures->windowFeatures().scrollbarsVisible)
+        return @(*scrollbarsVisible);
+
+    return nil;
 }
 
 - (NSNumber *)_fullscreenDisplay
 {
-    return @(_windowFeatures->windowFeatures().fullscreen);
+    if (auto fullscreen = _windowFeatures->windowFeatures().fullscreen)
+        return @(*fullscreen);
+
+    return nil;
 }
 
 - (NSNumber *)_dialogDisplay
 {
-    return @(_windowFeatures->windowFeatures().dialog);
+    if (auto dialog = _windowFeatures->windowFeatures().dialog)
+        return @(*dialog);
+
+    return nil;
 }
 
 @end

Source/WebKit/UIProcess/API/Cocoa/WKWindowFeaturesPrivate.h

@@ -29,6 +29,9 @@ NS_ASSUME_NONNULL_BEGIN
 
 @interface WKWindowFeatures (WKPrivate)
 
+@property (nonatomic, readonly) BOOL _wantsPopup WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
+@property (nonatomic, readonly) BOOL _hasAdditionalFeatures WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
+@property (nullable, nonatomic, readonly) NSNumber *_popup WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
 @property (nullable, nonatomic, readonly) NSNumber *_locationBarVisibility WK_API_AVAILABLE(macos(10.13), ios(11.0));
 @property (nullable, nonatomic, readonly) NSNumber *_scrollbarsVisibility WK_API_AVAILABLE(macos(10.13), ios(11.0));
 @property (nullable, nonatomic, readonly) NSNumber *_fullscreenDisplay WK_API_AVAILABLE(macos(10.13), ios(11.0));