Bug 188592 – NetworkCORSPreflightChecker should proceed in case of ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested even though the WebKit app is not implementing the didReceiveAuthenticationChallenge/didReceiveAuthenticationChallengeInFrame callback

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

Otherwise the preflight check might hang

[youenn fablet]

<rdar://problem/43210331>

[youenn fablet]

Created attachment 347132 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

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?

[Chris Dumez]

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?

[youenn fablet]

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.

[youenn fablet]

> >> 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.

[Alex Christensen]

Created attachment 347182 [details]
Patch

[Alex Christensen]

Created attachment 347183 [details]
Patch

[youenn fablet]

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.

[Alex Christensen]

Created attachment 347191 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r234896

[Ross Kirsling]

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.

[Ross Kirsling]

Committed r234908: <https://trac.webkit.org/changeset/234908>