Bug 178393 – The authentication challenge sender should not be used when network session is used.

• Component: WebCore Misc.
• Status: NEW

[Per Arne Vollan]

Instead, the completion handler with the appropriate disposition should be used.

[Per Arne Vollan]

Created attachment 324019 [details]
Patch

[Per Arne Vollan]

Created attachment 324046 [details]
Patch

[Per Arne Vollan]

Created attachment 324059 [details]
Patch

[Per Arne Vollan]

<rdar://problem/30675510>

[Per Arne Vollan]

Created attachment 324115 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 324115 [details]
Patch

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

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:35
> +#if !USE(CFURLCONNECTION) && !USE(NETWORK_SESSION)

How do we end up in this function when we shouldn't?

Presumably we need to respond in some other way to avoid permanently leaking connection data.

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #6)
> Comment on attachment 324115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324115&action=review
> 
> > Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:35
> > +#if !USE(CFURLCONNECTION) && !USE(NETWORK_SESSION)
> 
> How do we end up in this function when we shouldn't?
> 

I believe this happens because we did not find the challenge ID in the challenge map in the method AuthenticationManager::coalesceChallengesMatching, since the challenge ID has already been previously coalesced with a matching challenge ID. If that happens, we will currently go on and add the challenge ID to the vector of challenge IDs to be processed. This is incorrect, since the challenge ID has already been processed. Next, we will call 'm_challenges.take(challengeID);' in e.g. the method rejectProtectionSpaceAndContinueForSingleChallenge. We will then get an empty Challenge object because the challenge ID does no longer exist in the m_challenge map. Since the Challenge object is empty, we will not call the completion handler as we should in the USE(NETWORK_SESSION) case, but instead go on and call the associated method on the authentication challenge sender. Since the sender is not guaranteed to have this method, we will possibly crash when an exception is raised.

> Presumably we need to respond in some other way to avoid permanently leaking
> connection data.

If my reasoning above is correct, we should never get to the point where we call methods on the authentication challenge sender if we return an empty vector of challenge IDs when the challenge ID is not found in the map in AuthenticationManager::coalesceChallengesMatching.

[Alex Christensen]

Comment on attachment 324115 [details]
Patch

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

I'm ok with all of these changes if Alexey is.

> Source/WebKit/Shared/Authentication/AuthenticationManager.cpp:99
> +        // It is possible to not find the challenge ID in the map, if the challenge ID has already been previously
> +        // coalesced with a matching challenge ID. In this case, we should return an empty vector here.
> +        return Vector<uint64_t>();

This comment seems unnecessary.  It just explains that we are returning an empty vector.
I'd do this instead:
ASSERT_NOT_REACHED(); // instead of the above assertion
return { };

> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:42
> +#if !USE(CFURLCONNECTION) && !USE(NETWORK_SESSION)

I'd put an #else ASSERT_NOT_REACHED.

[Per Arne Vollan]

Created attachment 324135 [details]
Patch

[Per Arne Vollan]

(In reply to Alex Christensen from comment #8)
> Comment on attachment 324115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324115&action=review
> 
> I'm ok with all of these changes if Alexey is.
> 
> > Source/WebKit/Shared/Authentication/AuthenticationManager.cpp:99
> > +        // It is possible to not find the challenge ID in the map, if the challenge ID has already been previously
> > +        // coalesced with a matching challenge ID. In this case, we should return an empty vector here.
> > +        return Vector<uint64_t>();
> 
> This comment seems unnecessary.  It just explains that we are returning an
> empty vector.
> I'd do this instead:
> ASSERT_NOT_REACHED(); // instead of the above assertion
> return { };
> 
> > Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:42
> > +#if !USE(CFURLCONNECTION) && !USE(NETWORK_SESSION)
> 
> I'd put an #else ASSERT_NOT_REACHED.

The compiler forced me to add a NO_RETURN to the method declaration when I added this assert. Since I had to wrap this in #if USE(CFURLCONNECTION) || USE(NETWORK_SESSION), it became a little hard to read, so I added asserts in AuthenticationManager.cpp instead.

Thanks for reviewing, all!

[Per Arne Vollan]

Created attachment 324227 [details]
Patch

[Per Arne Vollan]

Created attachment 324229 [details]
Patch

[Per Arne Vollan]

Created attachment 324247 [details]
Patch

[Per Arne Vollan]

Created attachment 324304 [details]
Patch

[Per Arne Vollan]

Created attachment 324411 [details]
Patch

[Alex Christensen]

Comment on attachment 324411 [details]
Patch

There should at least be an ASSERT_NOT_REACHED in the lambda.

[Alex Christensen]

Comment on attachment 324411 [details]
Patch

And in the case that we're receiving an authentication challenge from the frame, we shouldn't just do nothing with it or that would break AppCache authentication.  We need to land https://bugs.webkit.org/show_bug.cgi?id=178540 and remove that entire code path.

[Per Arne Vollan]

Created attachment 324589 [details]
Patch

[Per Arne Vollan]

Created attachment 324602 [details]
Patch

[Per Arne Vollan]

Created attachment 324626 [details]
Patch

[Alex Christensen]

Comment on attachment 324626 [details]
Patch

This solution is similar to but not as thorough as https://bugs.webkit.org/show_bug.cgi?id=160234
It would leave challenges hanging.