Bug 193508

Summary: sendBeacon to previously-unvisited https domain always fails
Product: WebKit Reporter: Ali Juma <ajuma>
Component: Page LoadingAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, ggaren, stefan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=878562
Attachments:
Description Flags
Patch ggaren: review+

Description Ali Juma 2019-01-16 13:51:13 PST 
PingLoad::didReceiveChallenge always returns AuthenticationChallengeDisposition::Cancel, so calls to sendBeacon(url) where url is an https URL for a domain that we're establishing an https connection for the first time will always fail.

Once we establish an https connection some other way (e.g. by sending an xhr to the same domain), PingLoads no longer receive a challenge, so beacons are successfully sent.

This bug doesn't affect Safari, because in [WKNetworkSessionDelegate URLSession:task:didReceiveChallenge:completionHandler], _session->networkProcess().canHandleHTTPSServerTrustEvaluation() is false, so we always call the completion handler with NSURLSessionAuthChallengeRejectProtectionSpace.

However, for other WebKit embedders (e.g., MiniBrowser, and all non-Safari browsers on iOS), canHandleHTTPSServerTrustEvaluation() is true, so we do call into PingLoad::didReceiveChallenge and cancel the network task.

A possible fix would making PingLoad::didReceiveChallenge return RejectProtectionSpaceAndContinue, which would have the effect of allowing connections to sites with valid certificates and rejecting otherwise.
Comment 1 Alex Christensen 2019-01-16 14:03:46 PST 
We could fix this by using PerformDefaultHandling if it's for ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested
Comment 2 Alex Christensen 2019-01-16 15:48:21 PST 
Created attachment 359325 [details]
Patch
Comment 3 Geoffrey Garen 2019-01-16 15:59:45 PST 
Comment on attachment 359325 [details]
Patch

Can we add a regression test for this?
Comment 4 youenn fablet 2019-01-16 16:00:53 PST 
Comment on attachment 359325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359325&action=review

> Source/WebKit/NetworkProcess/PingLoad.cpp:123
>      auto weakThis = makeWeakPtr(*this);

This line could go after the if check.
Comment 5 Alex Christensen 2019-01-16 16:20:50 PST 
(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 359325 [details]
> Patch
> 
> Can we add a regression test for this?
We currently do not have the infrastructure to do so.  I would like to develop such infrastructure.

http://trac.webkit.org/r240094