Bug 193508 - sendBeacon to previously-unvisited https domain always fails
Summary: sendBeacon to previously-unvisited https domain always fails
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
Keywords: InRadar
Depends on:
Reported: 2019-01-16 13:51 PST by Ali Juma
Modified: 2019-04-25 06:56 PDT (History)
7 users (show)

See Also:

Patch (1.67 KB, patch)
2019-01-16 15:48 PST, Alex Christensen
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 3 Geoffrey Garen 2019-01-16 15:59:45 PST
Comment on attachment 359325 [details]

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

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.