- Revision
- 255533
- Author
- [email protected]
- Date
- 2020-01-31 16:47:57 -0800 (Fri, 31 Jan 2020)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (255532 => 255533)
--- trunk/Source/WebKit/ChangeLog 2020-02-01 00:44:07 UTC (rev 255532)
+++ trunk/Source/WebKit/ChangeLog 2020-02-01 00:47:57 UTC (rev 255533)
@@ -1,3 +1,32 @@
+2020-01-31 David Quesada <[email protected]>
+
+ 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 <[email protected]>
Add support for specifying background colors when setting marked text
Modified: trunk/Source/WebKit/NetworkProcess/Downloads/Download.h (255532 => 255533)
--- trunk/Source/WebKit/NetworkProcess/Downloads/Download.h 2020-02-01 00:44:07 UTC (rev 255532)
+++ trunk/Source/WebKit/NetworkProcess/Downloads/Download.h 2020-02-01 00:47:57 UTC (rev 255533)
@@ -82,6 +82,7 @@
#endif
DownloadID downloadID() const { return m_downloadID; }
+ PAL::SessionID sessionID() const { return m_sessionID; }
const String& suggestedName() const { return m_suggestedName; }
void setSandboxExtension(RefPtr<SandboxExtension>&& sandboxExtension) { m_sandboxExtension = WTFMove(sandboxExtension); }
Modified: trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm (255532 => 255533)
--- trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm 2020-02-01 00:44:07 UTC (rev 255532)
+++ trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm 2020-02-01 00:47:57 UTC (rev 255533)
@@ -613,6 +613,12 @@
{
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);
return;
@@ -651,8 +657,6 @@
// Handle server trust evaluation at platform-level if requested, for performance reasons and to use ATS defaults.
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();
if (!strongSelf)
Modified: trunk/Tools/ChangeLog (255532 => 255533)
--- trunk/Tools/ChangeLog 2020-02-01 00:44:07 UTC (rev 255532)
+++ trunk/Tools/ChangeLog 2020-02-01 00:47:57 UTC (rev 255533)
@@ -1,3 +1,22 @@
+2020-01-31 David Quesada <[email protected]>
+
+ 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 <[email protected]>
Add support for specifying background colors when setting marked text
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm (255532 => 255533)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm 2020-02-01 00:44:07 UTC (rev 255532)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm 2020-02-01 00:47:57 UTC (rev 255533)
@@ -1029,4 +1029,122 @@
[TestProtocol unregister];
}
+@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)
