Bug 188592 - NetworkCORSPreflightChecker should proceed in case of ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested even though the WebKit app is not implementing the didReceiveAuthenticationChallenge/didReceiveAuthenticationChallengeInFrame callback
Summary: NetworkCORSPreflightChecker should proceed in case of ProtectionSpaceAuthenti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-14 16:53 PDT by youenn fablet
Modified: 2018-08-15 17:05 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.42 KB, patch)
2018-08-14 17:00 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (20.40 KB, patch)
2018-08-15 11:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (23.27 KB, patch)
2018-08-15 11:39 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (23.47 KB, patch)
2018-08-15 12:10 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-08-14 16:53:28 PDT
Otherwise the preflight check might hang
Comment 1 youenn fablet 2018-08-14 16:53:53 PDT
<rdar://problem/43210331>
Comment 2 youenn fablet 2018-08-14 17:00:31 PDT
Created attachment 347132 [details]
Patch
Comment 3 Alex Christensen 2018-08-14 17:04:08 PDT
Comment on attachment 347132 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:107
> +    // FIXME: We should implement PROTECTION_SPACE_AUTH_CALLBACK code path.

We most definitely should not.  We should deprecate and remove it, not add to it.
Comment 4 Alex Christensen 2018-08-14 17:08:54 PDT
Comment on attachment 347132 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:109
> +        if (disposition == AuthenticationChallengeDisposition::Cancel && weakThis)

This feels iffy to me.  What about RejectProtectionSpace?

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:111
> +            weakThis->wasBlocked();
> +        completionHandler(disposition, credential);

This flow is not very clear.  Why do we need to call a completionHandler and m_completionCallback, which seem to do separate things?
Comment 5 Chris Dumez 2018-08-14 17:13:10 PDT
Comment on attachment 347132 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:98
> +        completionHandler(AuthenticationChallengeDisposition::RejectProtectionSpace, { });

The changelog says you proceed but here it seems to reject?

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:45
>      struct Parameters {

I admit, I do not really understand the change. I thought I had re-introduced previous behavior but apparently not?
Comment 6 youenn fablet 2018-08-14 20:45:00 PDT
Comment on attachment 347132 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:98
>> +        completionHandler(AuthenticationChallengeDisposition::RejectProtectionSpace, { });
> 
> The changelog says you proceed but here it seems to reject?

We reject the protection space, we do not cancel the load.
This is what happens for NetworkResourceLoader in NetworkLoad::continueCanAuthenticateAgainstProtectionSpace and what was used before r234626.

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:107
>> +    // FIXME: We should implement PROTECTION_SPACE_AUTH_CALLBACK code path.
> 
> We most definitely should not.  We should deprecate and remove it, not add to it.

We should be consistent with NetworkResourceLoader, whatever the code path is.

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:109
>> +        if (disposition == AuthenticationChallengeDisposition::Cancel && weakThis)
> 
> This feels iffy to me.  What about RejectProtectionSpace?

This is called for client certificate challenges only.
If the result is cancel (which is what happens if the callback is not implemented), we should make sure the preflight checker completion handler is called.
Ideally, if the callback is not implemented, it should return something like AuthenticationChallengeDisposition::Default but this is risky to do that change right now.

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:111
>> +        completionHandler(disposition, credential);
> 
> This flow is not very clear.  Why do we need to call a completionHandler and m_completionCallback, which seem to do separate things?

They indeed do different things.
We need to call completionHandler as it is a CompletionHandler.
We need to call m_completionCallback so that the network preflight checker is completed and does not hang.

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:45
>>      struct Parameters {
> 
> I admit, I do not really understand the change. I thought I had re-introduced previous behavior but apparently not?

NetworkResourceLoader is using NetworkLoad while NetworkCORSPreflightChecker is using NetworkDataTask.
See NetworkLoad::continueCanAuthenticateAgainstProtectionSpace for instance.
Comment 7 youenn fablet 2018-08-15 11:04:34 PDT
> >> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:107
> >> +    // FIXME: We should implement PROTECTION_SPACE_AUTH_CALLBACK code path.
> > 
> > We most definitely should not.  We should deprecate and remove it, not add to it.

Discussing with Alex, we will probably go down that way so that even for preflights, the app is notified of this challenge.
Comment 8 Alex Christensen 2018-08-15 11:34:12 PDT
Created attachment 347182 [details]
Patch
Comment 9 Alex Christensen 2018-08-15 11:39:49 PDT
Created attachment 347183 [details]
Patch
Comment 10 youenn fablet 2018-08-15 12:02:21 PDT
Comment on attachment 347183 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:681
> +    m_canAuthenticateAgainstProtectionSpaceCompletionHandlers.set(completionHandlerID, WTFMove(completionHandler));

s/set/add/, could add ASSERT(!contains...) in case we want to be extra sure.

id could be an ObjectIdentfiier as well but it is simpler to limit the changes.

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:342
> +    m_client->didReceiveChallenge(WTFMove(challenge), [this, protectedThis = makeRef(*this), challenge](AuthenticationChallengeDisposition disposition, const Credential& credential) {

Probably need to copy internals of challenge, protectionSpace maybe only?

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:523
> +    m_client->didReceiveChallenge(WTFMove(challenge), [this, protectedThis = makeRef(*this), challenge](AuthenticationChallengeDisposition disposition, const Credential& credential) {

Same issue for soup.
Comment 11 Alex Christensen 2018-08-15 12:10:01 PDT
Created attachment 347191 [details]
Patch
Comment 12 Alex Christensen 2018-08-15 13:16:12 PDT
http://trac.webkit.org/r234896
Comment 13 Ross Kirsling 2018-08-15 13:35:11 PDT
Comment on attachment 347191 [details]
Patch

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

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:342
> +    m_client->didReceiveChallenge(AuthenticationChallenge(challenge), [this, protectedThis = makeRef(*this), ](AuthenticationChallengeDisposition disposition, const Credential& credential) {

The syntax error on this line (dangling comma) breaks the WinCairo build.
Comment 14 Ross Kirsling 2018-08-15 17:05:07 PDT
Committed r234908: <https://trac.webkit.org/changeset/234908>