Changeset 255533 in webkit

Timestamp:

Jan 31, 2020 4:47:57 PM (4 years ago)

Author:

david_quesada@apple.com

Message:

REGRESSION(r252185): NetworkSessionCocoa cancels downloads that receive authentication challenges
​https://bugs.webkit.org/show_bug.cgi?id=206984
rdar://problem/58999654

Reviewed by Brady Eidson.

Source/WebKit:

r252185 changed the early return in WKNetworkSessionDelegate's -...task:didReceiveChallenge:... method
from "cancel the task and return early if self has no _session" to "cancel the task and return early
if we can't determine the network session for the given data task". When this method is called for
an NSURLSessionDownloadTask, this early return is hit because there is no NetworkDataTaskCocoa
for an active download. As a result, the download is canceled when it might have otherwise been able
to proceed.

Fix this by adding a code path to fetch the NetworkSession associated with the Download when an
NSURLSessionDownloadTask receives an authentication challenge. This ensures we can actually handle
the challenge appropriately and not just cancel the task.

• NetworkProcess/Downloads/Download.h:

(WebKit::Download::sessionID const):

Expose the session ID so we can use it to look up the NetworkSession for a Download.

• NetworkProcess/cocoa/NetworkSessionCocoa.mm:

(-[WKNetworkSessionDelegate URLSession:task:didReceiveChallenge:completionHandler:]):

Remove an unnecessary redeclaration of networkDataTask, and also an unneeded assertion that
networkDataTask != nullptr. Even if this is the case, the code that eventually handles this
task will null check it and handle the challenge as a websocket task or download task
based on the taskIdentifier.

Tools:

Add an API test for a resumed download that receives an authentication challenge. The download
delegate should be asked to handle the challenge, and the download should be able to finish.

• TestWebKitAPI/Tests/WebKitCocoa/Download.mm:

(-[DownloadCancelingDelegate _download:decideDestinationWithSuggestedFilename:completionHandler:]):
(-[DownloadCancelingDelegate _download:didReceiveData:]):
(-[DownloadCancelingDelegate _downloadDidCancel:]):
(-[AuthenticationChallengeHandlingDelegate _download:didReceiveAuthenticationChallenge:completionHandler:]):
(-[AuthenticationChallengeHandlingDelegate _downloadDidFinish:]):
(TEST):

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/NetworkProcess/Downloads/Download.h (modified) (1 diff)
• Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm (modified) (2 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-01-31  David Quesada  <david_quesada@apple.com>
+
+        REGRESSION(r252185): NetworkSessionCocoa cancels downloads that receive authentication challenges
+        https://bugs.webkit.org/show_bug.cgi?id=206984
+        rdar://problem/58999654
+
+        Reviewed by Brady Eidson.
+
+        r252185 changed the early return in WKNetworkSessionDelegate's -...task:didReceiveChallenge:... method
+        from "cancel the task and return early if self has no _session" to "cancel the task and return early
+        if we can't determine the network session for the given data task". When this method is called for
+        an NSURLSessionDownloadTask, this early return is hit because there is no NetworkDataTaskCocoa
+        for an active download. As a result, the download is canceled when it might have otherwise been able
+        to proceed.
+
+        Fix this by adding a code path to fetch the NetworkSession associated with the Download when an
+        NSURLSessionDownloadTask receives an authentication challenge. This ensures we can actually handle
+        the challenge appropriately and not just cancel the task.
+
+        * NetworkProcess/Downloads/Download.h:
+        (WebKit::Download::sessionID const):
+            Expose the session ID so we can use it to look up the NetworkSession for a Download.
+        * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
+        (-[WKNetworkSessionDelegate URLSession:task:didReceiveChallenge:completionHandler:]):
+            Remove an unnecessary redeclaration of networkDataTask, and also an unneeded assertion that
+            networkDataTask != nullptr. Even if this is the case, the code that eventually handles this
+            task will null check it and handle the challenge as a websocket task or download task
+            based on the taskIdentifier.
+
 2020-01-31  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebKit/NetworkProcess/Downloads/Download.h

@@ -83 +83 @@
 
     DownloadID downloadID() const { return m_downloadID; }
+    PAL::SessionID sessionID() const { return m_sessionID; }
     const String& suggestedName() const { return m_suggestedName; }
 

trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm

@@ -614 +614 @@
     auto* networkDataTask = [self existingTask:task];
     auto* sessionCocoa = networkDataTask ? static_cast<NetworkSessionCocoa*>(networkDataTask->networkSession()) : nullptr;
+    if (!networkDataTask) {
+        ASSERT(!sessionCocoa);
+        auto downloadID = _sessionWrapper->downloadMap.get(task.taskIdentifier);
+        auto download = downloadID.downloadID() ? _session->networkProcess().downloadManager().download(downloadID) : nil;
+        sessionCocoa = download ? static_cast<NetworkSessionCocoa*>(_session->networkProcess().networkSession(download->sessionID())) : nil;
+    }
     if (!sessionCocoa || [task state] == NSURLSessionTaskStateCanceling) {
         completionHandler(NSURLSessionAuthChallengeCancelAuthenticationChallenge, nil);
@@ -652 +658 @@
         if (sessionCocoa->fastServerTrustEvaluationEnabled() && negotiatedLegacyTLS == NegotiatedLegacyTLS::No) {
 #if HAVE(CFNETWORK_NSURLSESSION_STRICTRUSTEVALUATE)
-            auto* networkDataTask = [self existingTask:task];
-            ASSERT(networkDataTask);
             auto decisionHandler = makeBlockPtr([weakSelf = WeakObjCPtr<WKNetworkSessionDelegate>(self), sessionCocoa = makeWeakPtr(sessionCocoa), completionHandler = makeBlockPtr(completionHandler), taskIdentifier, networkDataTask = makeRefPtr(networkDataTask), negotiatedLegacyTLS](NSURLAuthenticationChallenge *challenge, OSStatus trustResult) mutable {
                 auto strongSelf = weakSelf.get();

trunk/Tools/ChangeLog

@@ +1 @@
+2020-01-31  David Quesada  <david_quesada@apple.com>
+
+        REGRESSION(r252185): NetworkSessionCocoa cancels downloads that receive authentication challenges
+        https://bugs.webkit.org/show_bug.cgi?id=206984
+        rdar://problem/58999654
+
+        Reviewed by Brady Eidson.
+
+        Add an API test for a resumed download that receives an authentication challenge. The download
+        delegate should be asked to handle the challenge, and the download should be able to finish.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
+        (-[DownloadCancelingDelegate _download:decideDestinationWithSuggestedFilename:completionHandler:]):
+        (-[DownloadCancelingDelegate _download:didReceiveData:]):
+        (-[DownloadCancelingDelegate _downloadDidCancel:]):
+        (-[AuthenticationChallengeHandlingDelegate _download:didReceiveAuthenticationChallenge:completionHandler:]):
+        (-[AuthenticationChallengeHandlingDelegate _downloadDidFinish:]):
+        (TEST):
+
 2020-01-31  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm

@@ -1030 +1030 @@
 }
 
+@interface DownloadCancelingDelegate : NSObject <_WKDownloadDelegate>
+@property (readonly) RetainPtr<NSData> resumeData;
+@property (readonly) RetainPtr<NSString> path;
+@end
+
+@implementation DownloadCancelingDelegate
+
+- (void)_download:(_WKDownload *)download decideDestinationWithSuggestedFilename:(NSString *)filename completionHandler:(void (^)(BOOL allowOverwrite, NSString *destination))completionHandler
+{
+    FileSystem::PlatformFileHandle fileHandle;
+    _path = FileSystem::openTemporaryFile("TestWebKitAPI", fileHandle);
+    EXPECT_TRUE(fileHandle != FileSystem::invalidPlatformFileHandle);
+    FileSystem::closeFile(fileHandle);
+    completionHandler(YES, _path.get());
+}
+
+- (void)_download:(_WKDownload *)download didReceiveData:(uint64_t)length
+{
+    EXPECT_EQ(length, 5000ULL);
+    [download cancel];
+}
+
+- (void)_downloadDidCancel:(_WKDownload *)download
+{
+    EXPECT_NOT_NULL(download.resumeData);
+    _resumeData = download.resumeData;
+    isDone = true;
+}
+
+@end
+
+@interface AuthenticationChallengeHandlingDelegate : NSObject <_WKDownloadDelegate>
+@end
+
+@implementation AuthenticationChallengeHandlingDelegate {
+    bool _didReceiveAuthenticationChallenge;
+}
+
+- (void)_download:(_WKDownload *)download didReceiveAuthenticationChallenge:(NSURLAuthenticationChallenge *)challenge completionHandler:(void (^)(NSURLSessionAuthChallengeDisposition, NSURLCredential*))completionHandler
+{
+    _didReceiveAuthenticationChallenge = true;
+    completionHandler(NSURLSessionAuthChallengeUseCredential, nil);
+}
+
+- (void)_downloadDidFinish:(_WKDownload *)download
+{
+    EXPECT_TRUE(_didReceiveAuthenticationChallenge);
+    isDone = true;
+}
+
+@end
+
+TEST(_WKDownload, ResumedDownloadCanHandleAuthenticationChallenge)
+{
+    using namespace TestWebKitAPI;
+
+    std::atomic<bool> receivedFirstConnection { false };
+
+    TCPServer server([&](int socket) {
+        if (!receivedFirstConnection.exchange(true)) {
+            TCPServer::read(socket);
+
+            const char* responseHeader =
+            "HTTP/1.1 200 OK\r\n"
+            "ETag: test\r\n"
+            "Content-Length: 10000\r\n\r\n";
+            TCPServer::write(socket, responseHeader, strlen(responseHeader));
+
+            char data[5000];
+            memset(data, 0, 5000);
+            TCPServer::write(socket, data, 5000);
+
+            // Wait for the client to cancel the download before closing the connection.
+            Util::run(&isDone);
+        } else {
+            TCPServer::read(socket);
+            const char* challengeHeader =
+            "HTTP/1.1 401 Unauthorized\r\n"
+            "Date: Sat, 23 Mar 2019 06:29:01 GMT\r\n"
+            "Content-Length: 0\r\n"
+            "WWW-Authenticate: Basic realm=\"testrealm\"\r\n\r\n";
+            TCPServer::write(socket, challengeHeader, strlen(challengeHeader));
+
+            TCPServer::read(socket);
+
+            const char* responseHeader =
+            "HTTP/1.1 206 Partial Content\r\n"
+            "ETag: test\r\n"
+            "Content-Range: bytes 5000-9999/10000\r\n"
+            "Content-Length: 5000\r\n\r\n";
+            TCPServer::write(socket, responseHeader, strlen(responseHeader));
+
+            char data[5000];
+            memset(data, 1, 5000);
+            TCPServer::write(socket, data, 5000);
+        }
+    }, 2);
+
+    auto processPool = adoptNS([[WKProcessPool alloc] init]);
+    auto websiteDataStore = adoptNS([WKWebsiteDataStore defaultDataStore]);
+
+    auto delegate1 = adoptNS([[DownloadCancelingDelegate alloc] init]);
+    [processPool _setDownloadDelegate:delegate1.get()];
+
+    isDone = false;
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:[NSString stringWithFormat:@"http://127.0.0.1:%d/", server.port()]]];
+    [processPool _downloadURLRequest:request websiteDataStore:websiteDataStore.get() originatingWebView:nil];
+
+    Util::run(&isDone);
+
+    isDone = false;
+    auto delegate2 = adoptNS([[AuthenticationChallengeHandlingDelegate alloc] init]);
+    [processPool _setDownloadDelegate:delegate2.get()];
+    [processPool _resumeDownloadFromData:[delegate1 resumeData].get() websiteDataStore:websiteDataStore.get() path:[delegate1 path].get() originatingWebView:nil];
+
+    Util::run(&isDone);
+}
+
 #endif // PLATFORM(MAC) || PLATFORM(IOS)