WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(3.64 KB, patch)
2017-10-17 13:13 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(5.80 KB, patch)
2017-10-17 14:27 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(4.91 KB, patch)
2017-10-18 07:35 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(6.33 KB, patch)
2017-10-18 10:59 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(8.52 KB, patch)
2017-10-19 07:46 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(8.57 KB, patch)
2017-10-19 07:49 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(7.93 KB, patch)
2017-10-19 10:21 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(9.67 KB, patch)
2017-10-19 15:46 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(1.91 KB, patch)
2017-10-20 09:52 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(3.34 KB, patch)
2017-10-23 14:38 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(3.42 KB, patch)
2017-10-23 15:48 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(2.90 KB, patch)
2017-10-23 19:01 PDT
,
Per Arne Vollan
achristensen
: review-
Details
Formatted Diff
Diff
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
Per Arne Vollan
Comment 1
2017-10-17 09:49:51 PDT
Created
attachment 324019
[details]
Patch
Per Arne Vollan
Comment 2
2017-10-17 13:13:00 PDT
Created
attachment 324046
[details]
Patch
Per Arne Vollan
Comment 3
2017-10-17 14:27:25 PDT
Created
attachment 324059
[details]
Patch
Per Arne Vollan
Comment 4
2017-10-17 15:57:17 PDT
<
rdar://problem/30675510
>
Per Arne Vollan
Comment 5
2017-10-18 07:35:35 PDT
Created
attachment 324115
[details]
Patch
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
Created
attachment 324135
[details]
Patch
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
Created
attachment 324227
[details]
Patch
Per Arne Vollan
Comment 12
2017-10-19 07:49:40 PDT
Created
attachment 324229
[details]
Patch
Per Arne Vollan
Comment 13
2017-10-19 10:21:27 PDT
Created
attachment 324247
[details]
Patch
Per Arne Vollan
Comment 14
2017-10-19 15:46:30 PDT
Created
attachment 324304
[details]
Patch
Per Arne Vollan
Comment 15
2017-10-20 09:52:42 PDT
Created
attachment 324411
[details]
Patch
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
Created
attachment 324589
[details]
Patch
Per Arne Vollan
Comment 19
2017-10-23 15:48:48 PDT
Created
attachment 324602
[details]
Patch
Per Arne Vollan
Comment 20
2017-10-23 19:01:17 PDT
Created
attachment 324626
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug