Bug 193355

Created attachment 358919 [details] Patch

<rdar://problem/30641705>

Comment on attachment 358919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358919&action=review > Source/WebKit/ChangeLog:9 > + If ssuccessful, let loading proceed as usual. successful > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:557 > + if (!canNSURLSessionTrustEvaluate()) > + return completionHandler(NSURLSessionAuthChallengeRejectProtectionSpace, nil); On platforms that don't have the SPI we will need to keep existing behavior. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:585 > + auto completionHandlerCopy = Block_copy(completionHandler); Let's use BlockPtr instead of Block_copy/Block_release > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1028 > +void NetworkSessionCocoa::continueDidReceiveChallenge(const WebCore::AuthenticationChallenge& challenge, NetworkDataTaskCocoa::TaskIdentifier taskIdentifier, NetworkDataTaskCocoa* networkDataTask, WTF::CompletionHandler<void(WebKit::AuthenticationChallengeDisposition, const WebCore::Credential&)>&& completionHandler) WTF:: is unnecessary.

Created attachment 360386 [details] Patch

Attachment 360386 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:484: The parameter name "canHandle" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 360386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360386&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:56 > +@interface NSURLSession () This should be in CFNetworkSPI.h. We should add a HAVE_STRICT_TRUST_EVALUATE in Platform.h with proper OS version checks. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:554 > + if (canNSURLSessionTrustEvaluate()) { This needs an else which does the exact same thing as the current existing behavior. That should also be used on OSes where _strictTrustEvaluate is not available. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:564 > + _session->continueDidReceiveChallenge(challenge, taskIdentifier, networkDataTask.get(), [completionHandler = WTFMove(completionHandler), secTrust = WTFMove(secTrust)] (WebKit::AuthenticationChallengeDisposition disposition, const WebCore::Credential& credential) mutable { This is a lot of code. It should be put in a subroutine which takes a completion handler with a BOOL-like result (whether it was determined to be a good server or not).

Comment on attachment 360386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360386&action=review >> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:56 >> +@interface NSURLSession () > > This should be in CFNetworkSPI.h. We should add a HAVE_STRICT_TRUST_EVALUATE in Platform.h with proper OS version checks. Ok >> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:554 >> + if (canNSURLSessionTrustEvaluate()) { > > This needs an else which does the exact same thing as the current existing behavior. That should also be used on OSes where _strictTrustEvaluate is not available. There is no need for else since we return at the end of the if. Behavior if strictEvaluate is not there should be preserved.

Created attachment 360478 [details] Patch

Attachment 360478 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:484: The parameter name "canHandle" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 360478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360478&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:571 > + if (canNSURLSessionTrustEvaluate()) { This needs to be inside the !canHandleHTTPSServerTrustEvaluation case. Otherwise we break API compatibility. > LayoutTests/http/tests/ssl/certificate-validation.html:33 > + const iframe = await with_iframe("https://localhost:8443"); I'm not sure this is a good test. Often we will have a cached connection to localhost:8443

Created attachment 360497 [details] Patch

Attachment 360497 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:484: The parameter name "canHandle" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 360497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360497&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:532 > + if (trustResult == noErr || networkDataTask->state() == NetworkDataTask::State::Canceling || networkDataTask->state() == NetworkDataTask::State::Completed) { We need to null check networkDataTask here before using it. I'm also not sure we should check these two states. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:537 > + RetainPtr<SecTrustRef> secTrust = challenge.protectionSpace.serverTrust; This could be declared inside the lambda capture: secTrust = retainPtr(challenge.protectionSpace.serverTrust) > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1044 > + extra space

Created attachment 360503 [details] Patch for landing

Attachment 360503 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:484: The parameter name "canHandle" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 360503 [details] Patch for landing Clearing flags on attachment: 360503 Committed r240689: <https://trac.webkit.org/changeset/240689>

All reviewed patches have been landed. Closing bug.