NEW 178393
The authentication challenge sender should not be used when network session is used.
https://bugs.webkit.org/show_bug.cgi?id=178393
Summary The authentication challenge sender should not be used when network session i...
Per Arne Vollan
Reported 2017-10-17 09:47:42 PDT
Instead, the completion handler with the appropriate disposition should be used.
Attachments
Patch (3.11 KB, patch)
2017-10-17 09:49 PDT, Per Arne Vollan
no flags
Patch (3.64 KB, patch)
2017-10-17 13:13 PDT, Per Arne Vollan
no flags
Patch (5.80 KB, patch)
2017-10-17 14:27 PDT, Per Arne Vollan
no flags
Patch (4.91 KB, patch)
2017-10-18 07:35 PDT, Per Arne Vollan
no flags
Patch (6.33 KB, patch)
2017-10-18 10:59 PDT, Per Arne Vollan
no flags
Patch (8.52 KB, patch)
2017-10-19 07:46 PDT, Per Arne Vollan
no flags
Patch (8.57 KB, patch)
2017-10-19 07:49 PDT, Per Arne Vollan
no flags
Patch (7.93 KB, patch)
2017-10-19 10:21 PDT, Per Arne Vollan
no flags
Patch (9.67 KB, patch)
2017-10-19 15:46 PDT, Per Arne Vollan
no flags
Patch (1.91 KB, patch)
2017-10-20 09:52 PDT, Per Arne Vollan
no flags
Patch (3.34 KB, patch)
2017-10-23 14:38 PDT, Per Arne Vollan
no flags
Patch (3.42 KB, patch)
2017-10-23 15:48 PDT, Per Arne Vollan
no flags
Patch (2.90 KB, patch)
2017-10-23 19:01 PDT, Per Arne Vollan
achristensen: review-
Per Arne Vollan
Comment 1 2017-10-17 09:49:51 PDT
Per Arne Vollan
Comment 2 2017-10-17 13:13:00 PDT
Per Arne Vollan
Comment 3 2017-10-17 14:27:25 PDT
Per Arne Vollan
Comment 4 2017-10-17 15:57:17 PDT
Per Arne Vollan
Comment 5 2017-10-18 07:35:35 PDT
Alexey Proskuryakov
Comment 6 2017-10-18 09:29:14 PDT
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
Comment 7 2017-10-18 09:44:16 PDT
(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 8 2017-10-18 10:05:39 PDT
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
Comment 9 2017-10-18 10:59:54 PDT
Per Arne Vollan
Comment 10 2017-10-18 11:03:24 PDT
(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
Comment 11 2017-10-19 07:46:37 PDT
Per Arne Vollan
Comment 12 2017-10-19 07:49:40 PDT
Per Arne Vollan
Comment 13 2017-10-19 10:21:27 PDT
Per Arne Vollan
Comment 14 2017-10-19 15:46:30 PDT
Per Arne Vollan
Comment 15 2017-10-20 09:52:42 PDT
Alex Christensen
Comment 16 2017-10-20 10:57:46 PDT
Comment on attachment 324411 [details] Patch There should at least be an ASSERT_NOT_REACHED in the lambda.
Alex Christensen
Comment 17 2017-10-20 11:04:10 PDT
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
Comment 18 2017-10-23 14:38:09 PDT
Per Arne Vollan
Comment 19 2017-10-23 15:48:48 PDT
Per Arne Vollan
Comment 20 2017-10-23 19:01:17 PDT
Alex Christensen
Comment 21 2017-10-24 10:25:05 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.