WKWebView has old URL while displaying SafeBrowsing interstitial, for link-click...
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Dec 2018 18:26:33 +0000 (18:26 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Dec 2018 18:26:33 +0000 (18:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192675

Reviewed by Geoffrey Garen.

Source/WebKit:

When a safe browsing warning is being shown, WKWebView.URL should be the unsafe website, not the safe website before it.

* UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _showSafeBrowsingWarningWithTitle:warning:details:completionHandler:]):
(-[WKWebView _showSafeBrowsingWarningWithURL:title:warning:details:completionHandler:]):
* UIProcess/API/Cocoa/WKWebViewPrivate.h:
* UIProcess/Cocoa/SafeBrowsingWarningCocoa.mm:
(WebKit::SafeBrowsingWarning::SafeBrowsingWarning):
* UIProcess/SafeBrowsingWarning.h:
(WebKit::SafeBrowsingWarning::create):
(WebKit::SafeBrowsingWarning::url const):
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::decidePolicyForNavigationAction):

Tools:

* TestWebKitAPI/Tests/WebKitCocoa/SafeBrowsing.mm:
(goBack):
(TEST):
(visitUnsafeSite):
(-[SafeBrowsingHelper observeValueForKeyPath:ofObject:change:context:]):
(-[SafeBrowsingHelper webView:runJavaScriptAlertPanelWithMessage:initiatedByFrame:completionHandler:]):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239339 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h
Source/WebKit/UIProcess/Cocoa/SafeBrowsingWarningCocoa.mm
Source/WebKit/UIProcess/SafeBrowsingWarning.h
Source/WebKit/UIProcess/WebPageProxy.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/SafeBrowsing.mm

index 6c8822d..d6429bd 100644 (file)
@@ -1,3 +1,24 @@
+2018-12-18  Alex Christensen  <achristensen@webkit.org>
+
+        WKWebView has old URL while displaying SafeBrowsing interstitial, for link-click navigations
+        https://bugs.webkit.org/show_bug.cgi?id=192675
+
+        Reviewed by Geoffrey Garen.
+
+        When a safe browsing warning is being shown, WKWebView.URL should be the unsafe website, not the safe website before it.
+
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _showSafeBrowsingWarningWithTitle:warning:details:completionHandler:]):
+        (-[WKWebView _showSafeBrowsingWarningWithURL:title:warning:details:completionHandler:]):
+        * UIProcess/API/Cocoa/WKWebViewPrivate.h:
+        * UIProcess/Cocoa/SafeBrowsingWarningCocoa.mm:
+        (WebKit::SafeBrowsingWarning::SafeBrowsingWarning):
+        * UIProcess/SafeBrowsingWarning.h:
+        (WebKit::SafeBrowsingWarning::create):
+        (WebKit::SafeBrowsingWarning::url const):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
+
 2018-12-18  Chris Dumez  <cdumez@apple.com>
 
         Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
index b9bed1d..06a15b2 100644 (file)
@@ -4800,7 +4800,13 @@ FOR_EACH_PRIVATE_WKCONTENTVIEW_ACTION(FORWARD_ACTION_TO_WKCONTENTVIEW)
 
 - (void)_showSafeBrowsingWarningWithTitle:(NSString *)title warning:(NSString *)warning details:(NSAttributedString *)details completionHandler:(void(^)(BOOL))completionHandler
 {
-    auto safeBrowsingWarning = WebKit::SafeBrowsingWarning::create(title, warning, details);
+    // FIXME: Adopt _showSafeBrowsingWarningWithURL and remove this function.
+    [self _showSafeBrowsingWarningWithURL:nil title:title warning:warning details:details completionHandler:completionHandler];
+}
+
+- (void)_showSafeBrowsingWarningWithURL:(NSURL *)url title:(NSString *)title warning:(NSString *)warning details:(NSAttributedString *)details completionHandler:(void(^)(BOOL))completionHandler
+{
+    auto safeBrowsingWarning = WebKit::SafeBrowsingWarning::create(url, title, warning, details);
     auto wrapper = [completionHandler = makeBlockPtr(completionHandler)] (Variant<WebKit::ContinueUnsafeLoad, URL>&& variant) {
         switchOn(variant, [&] (WebKit::ContinueUnsafeLoad continueUnsafeLoad) {
             switch (continueUnsafeLoad) {
index 170c2aa..8af852e 100644 (file)
@@ -194,6 +194,7 @@ typedef NS_OPTIONS(NSUInteger, _WKRectEdge) {
 + (NSURL *)_confirmMalwareSentinel WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 + (NSURL *)_visitUnsafeWebsiteSentinel WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 - (void)_showSafeBrowsingWarningWithTitle:(NSString *)title warning:(NSString *)warning details:(NSAttributedString *)details completionHandler:(void(^)(BOOL))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
+- (void)_showSafeBrowsingWarningWithURL:(NSURL *)url title:(NSString *)title warning:(NSString *)warning details:(NSAttributedString *)details completionHandler:(void(^)(BOOL))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 
 - (void)_isJITEnabled:(void(^)(BOOL))completionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
 - (void)_removeDataDetectedLinks:(dispatch_block_t)completion WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
index 8281f6c..0984b5b 100644 (file)
@@ -150,15 +150,17 @@ static NSMutableAttributedString *safeBrowsingDetailsText(const URL& url, SSBSer
 }
 
 SafeBrowsingWarning::SafeBrowsingWarning(const URL& url, SSBServiceLookupResult *result)
-    : m_title(safeBrowsingTitleText(result))
+    : m_url(url)
+    , m_title(safeBrowsingTitleText(result))
     , m_warning(safeBrowsingWarningText(result))
     , m_details(safeBrowsingDetailsText(url, result))
 {
 }
 #endif
 
-SafeBrowsingWarning::SafeBrowsingWarning(String&& title, String&& warning, RetainPtr<NSAttributedString>&& details)
-    : m_title(WTFMove(title))
+SafeBrowsingWarning::SafeBrowsingWarning(URL&& url, String&& title, String&& warning, RetainPtr<NSAttributedString>&& details)
+    : m_url(WTFMove(url))
+    , m_title(WTFMove(title))
     , m_warning(WTFMove(warning))
     , m_details(WTFMove(details))
 {
index 8dcb125..f1908c6 100644 (file)
@@ -45,12 +45,13 @@ public:
     }
 #endif
 #if PLATFORM(COCOA)
-    static Ref<SafeBrowsingWarning> create(String&& title, String&& warning, RetainPtr<NSAttributedString>&& details)
+    static Ref<SafeBrowsingWarning> create(URL&& url, String&& title, String&& warning, RetainPtr<NSAttributedString>&& details)
     {
-        return adoptRef(*new SafeBrowsingWarning(WTFMove(title), WTFMove(warning), WTFMove(details)));
+        return adoptRef(*new SafeBrowsingWarning(WTFMove(url), WTFMove(title), WTFMove(warning), WTFMove(details)));
     }
 #endif
 
+    const URL& url() const { return m_url; }
     const String& title() const { return m_title; }
     const String& warning() const { return m_warning; }
 #if PLATFORM(COCOA)
@@ -65,9 +66,10 @@ private:
     SafeBrowsingWarning(const URL&, SSBServiceLookupResult *);
 #endif
 #if PLATFORM(COCOA)
-    SafeBrowsingWarning(String&&, String&&, RetainPtr<NSAttributedString>&&);
+    SafeBrowsingWarning(URL&&, String&&, String&&, RetainPtr<NSAttributedString>&&);
 #endif
 
+    URL m_url;
     String m_title;
     String m_warning;
 #if PLATFORM(COCOA)
index 10e4367..23b4514 100644 (file)
@@ -4339,6 +4339,12 @@ void WebPageProxy::decidePolicyForNavigationAction(WebFrameProxy& frame, WebCore
         m_pageClient->clearSafeBrowsingWarning();
 
         if (safeBrowsingWarning) {
+            if (frame->isMainFrame() && safeBrowsingWarning->url().isValid()) {
+                auto transaction = m_pageLoadState.transaction();
+                m_pageLoadState.setPendingAPIRequestURL(transaction, safeBrowsingWarning->url());
+                m_pageLoadState.commitChanges();
+            }
+
             m_pageClient->showSafeBrowsingWarning(*safeBrowsingWarning, [protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler), policyAction] (auto&& result) mutable {
                 switchOn(result, [&] (const URL& url) {
                     completionHandler(WebPolicyAction::Ignore);
index 1525bb6..f3ca88f 100644 (file)
@@ -1,3 +1,17 @@
+2018-12-18  Alex Christensen  <achristensen@webkit.org>
+
+        WKWebView has old URL while displaying SafeBrowsing interstitial, for link-click navigations
+        https://bugs.webkit.org/show_bug.cgi?id=192675
+
+        Reviewed by Geoffrey Garen.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/SafeBrowsing.mm:
+        (goBack):
+        (TEST):
+        (visitUnsafeSite):
+        (-[SafeBrowsingHelper observeValueForKeyPath:ofObject:change:context:]):
+        (-[SafeBrowsingHelper webView:runJavaScriptAlertPanelWithMessage:initiatedByFrame:completionHandler:]):
+
 2018-12-18  Chris Dumez  <cdumez@apple.com>
 
         Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
index d5f71fc..3f3b9e7 100644 (file)
@@ -35,6 +35,8 @@
 #import <WebKit/WKUIDelegatePrivate.h>
 #import <WebKit/WKWebViewPrivate.h>
 #import <wtf/RetainPtr.h>
+#import <wtf/URL.h>
+#import <wtf/Vector.h>
 
 static bool committedNavigation;
 static bool warningShown;
@@ -208,15 +210,25 @@ static void checkTitleAndClick(UIButton *button, const char* expectedTitle)
 }
 #endif
 
-TEST(SafeBrowsing, GoBack)
+template<typename ViewType> void goBack(ViewType *view)
 {
-    auto webView = safeBrowsingView();
-    auto warning = [webView _safeBrowsingWarning];
-    auto box = warning.subviews.firstObject;
+    WKWebView *webView = (WKWebView *)view.superview;
+    auto box = view.subviews.firstObject;
     checkTitleAndClick(box.subviews[3], "Go Back");
     EXPECT_EQ([webView _safeBrowsingWarning], nil);
 }
 
+TEST(SafeBrowsing, GoBack)
+{
+    auto webView = safeBrowsingView();
+    goBack([webView _safeBrowsingWarning]);
+}
+
+template<typename ViewType> void visitUnsafeSite(ViewType *view)
+{
+    [view performSelector:NSSelectorFromString(@"clickedOnLink:") withObject:[NSURL URLWithString:@"WKVisitUnsafeWebsiteSentinel"]];
+}
+
 TEST(SafeBrowsing, VisitUnsafeWebsite)
 {
     auto webView = safeBrowsingView();
@@ -228,7 +240,7 @@ TEST(SafeBrowsing, VisitUnsafeWebsite)
     checkTitleAndClick(warning.subviews.firstObject.subviews[4], "Show Details");
     EXPECT_EQ(warning.subviews.count, 2ull);
     EXPECT_FALSE(committedNavigation);
-    [warning performSelector:NSSelectorFromString(@"clickedOnLink:") withObject:[NSURL URLWithString:@"WKVisitUnsafeWebsiteSentinel"]];
+    visitUnsafeSite(warning);
     TestWebKitAPI::Util::run(&committedNavigation);
 }
 
@@ -248,7 +260,7 @@ TEST(SafeBrowsing, ShowWarningSPI)
     auto webView = adoptNS([WKWebView new]);
     auto showWarning = ^{
         completionHandlerCalled = false;
-        [webView _showSafeBrowsingWarningWithTitle:@"test title" warning:@"test warning" details:[[[NSAttributedString alloc] initWithString:@"test details"] autorelease] completionHandler:^(BOOL shouldContinue) {
+        [webView _showSafeBrowsingWarningWithURL:nil title:@"test title" warning:@"test warning" details:[[[NSAttributedString alloc] initWithString:@"test details"] autorelease] completionHandler:^(BOOL shouldContinue) {
             shouldContinueValue = shouldContinue;
             completionHandlerCalled = true;
         }];
@@ -268,6 +280,83 @@ TEST(SafeBrowsing, ShowWarningSPI)
     EXPECT_TRUE(shouldContinueValue);
 }
 
+static Vector<URL> urls;
+static bool done;
+
+@interface SafeBrowsingHelper : NSObject<WKNavigationDelegate, WKUIDelegate>
+@end
+
+@implementation SafeBrowsingHelper
+
+- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary<NSString *, id> *)change context:(void *)context
+{
+    urls.append((NSURL *)[change objectForKey:NSKeyValueChangeNewKey]);
+}
+
+- (void)webView:(WKWebView *)webView runJavaScriptAlertPanelWithMessage:(NSString *)message initiatedByFrame:(WKFrameInfo *)frame completionHandler:(void (^)(void))completionHandler
+{
+    done = true;
+    completionHandler();
+}
+
+@end
+
+TEST(SafeBrowsing, URLObservation)
+{
+    ClassMethodSwizzler swizzler(objc_getClass("SSBLookupContext"), @selector(sharedLookupContext), [TestLookupContext methodForSelector:@selector(sharedLookupContext)]);
+
+    RetainPtr<NSURL> simpleURL = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+    RetainPtr<NSURL> simple2URL = [[NSBundle mainBundle] URLForResource:@"simple2" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+    auto helper = adoptNS([SafeBrowsingHelper new]);
+
+    auto webViewWithWarning = [&] () -> RetainPtr<WKWebView> {
+        auto webView = adoptNS([WKWebView new]);
+        [webView setUIDelegate:helper.get()];
+        [webView setNavigationDelegate:helper.get()];
+        [webView addObserver:helper.get() forKeyPath:@"URL" options:NSKeyValueObservingOptionNew context:nil];
+
+        [webView loadHTMLString:@"<script>alert('loaded')</script>" baseURL:simpleURL.get()];
+        while (![webView _safeBrowsingWarning])
+            TestWebKitAPI::Util::spinRunLoop();
+        visitUnsafeSite([webView _safeBrowsingWarning]);
+        TestWebKitAPI::Util::run(&done);
+        EXPECT_FALSE(!![webView _safeBrowsingWarning]);
+
+        done = false;
+        [webView evaluateJavaScript:[NSString stringWithFormat:@"window.location='%@'", simple2URL.get()] completionHandler:nil];
+        while (![webView _safeBrowsingWarning])
+            TestWebKitAPI::Util::spinRunLoop();
+        return webView;
+    };
+    
+    auto checkURLs = [&] (Vector<RetainPtr<NSURL>>&& expected) {
+        EXPECT_EQ(urls.size(), expected.size());
+        if (urls.size() != expected.size())
+            return;
+        for (size_t i = 0; i < expected.size(); ++i)
+            EXPECT_STREQ(urls[i].string().utf8().data(), [expected[i] absoluteString].UTF8String);
+    };
+
+    {
+        auto webView = webViewWithWarning();
+        checkURLs({ simpleURL, simple2URL });
+        goBack([webView _safeBrowsingWarning]);
+        checkURLs({ simpleURL, simple2URL, simpleURL });
+        [webView removeObserver:helper.get() forKeyPath:@"URL"];
+    }
+    
+    urls.clear();
+
+    {
+        auto webView = webViewWithWarning();
+        checkURLs({ simpleURL, simple2URL });
+        visitUnsafeSite([webView _safeBrowsingWarning]);
+        TestWebKitAPI::Util::spinRunLoop(5);
+        checkURLs({ simpleURL, simple2URL });
+        [webView removeObserver:helper.get() forKeyPath:@"URL"];
+    }
+}
+
 @interface NullLookupContext : NSObject
 @end
 @implementation NullLookupContext