Bug 188592

<rdar://problem/43210331>

Created attachment 347132 [details]
Patch

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

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

Created attachment 347182 [details]
Patch

Created attachment 347183 [details]
Patch

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.

Created attachment 347191 [details]
Patch

http://trac.webkit.org/r234896

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.

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