Bug 38128

Summary:

Don't add empty credential to CredentialStorage.

Product:

WebKit

Reporter:

Yongjun Zhang <yongjun_zhang>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, commit-queue, mitz

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac (Intel)

OS:

OS X 10.5

Attachments:

Description	Flags
First attempt to fix this issue.	levin: review-
change the ChangeLog as per Davin's comment.	ap: review-
Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments.	none

Yongjun Zhang

Reported 2010-04-26 10:06:37 PDT

If the user respond an authentication challenge with an empty credential, it is added to the CredentialStorage. However, an empty credential is currently regarded as a invalid login because it means a missing value in protectionSpaceToCredentialMap.

Attachments
First attempt to fix this issue. (1.67 KB, patch) 2010-04-26 10:18 PDT, Yongjun Zhang	levin: review-	Details Formatted Diff Diff
change the ChangeLog as per Davin's comment. (1.75 KB, patch) 2010-04-26 10:54 PDT, Yongjun Zhang	ap: review-	Details Formatted Diff Diff
Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments. (1.63 KB, patch) 2010-04-26 14:08 PDT, Yongjun Zhang	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yongjun Zhang

Comment 1 2010-04-26 10:18:56 PDT

Created attachment 54315 [details] First attempt to fix this issue. Add empty credential check before adding it to CredentialStorage.

David Levin

Comment 2 2010-04-26 10:32:56 PDT

Comment on attachment 54315 [details] First attempt to fix this issue. Needs a layout test or an explanation in the ChangeLog of why a layout test isn't possible.

Yongjun Zhang

Comment 3 2010-04-26 10:54:04 PDT

Created attachment 54318 [details] change the ChangeLog as per Davin's comment.

Alexey Proskuryakov

Comment 4 2010-04-26 12:16:36 PDT

Comment on attachment 54318 [details] change the ChangeLog as per Davin's comment. This patch only modifies one code path, avoiding badness in CredentialStorage::set() on some platforms. I think that it would be better for consistency to add an early return regardless of OS X version, something like if (credential.isEmpty()) { clearAuthentication(); return; } + No new tests added because it doesn't change the current behavior. I think that that a more accurate explanation would be "because this only affects credentials entered by the user, and we cannot test authentication dialog in DumpRenderTree". We need to make the same changes on Windows, but that can be a separate patch that someone (possibly myself) can make later.

Yongjun Zhang

Comment 5 2010-04-26 13:43:40 PDT

(In reply to comment #4) > (From update of attachment 54318 [details]) > This patch only modifies one code path, avoiding badness in > CredentialStorage::set() on some platforms. I think that it would be better for > consistency to add an early return regardless of OS X version, something like > > if (credential.isEmpty()) { > clearAuthentication(); > return; > } Hmm.. Wouldn't that make WebCore loading stay at a undefined state? For example, the progress bar is started because user clicks a link which leads to a page requires authentication. Then the challenge comes and user enters an empty credential, then we just bail out early without sending the _invalid_ credential out. I believe the progress bar will stay running because the FrameLoader is not notified in any way in this case. so I might need to change to something like: if (credential.isEmpty()) { m_client->cancel(ResourceError); // tell the FrameLoader to clear the Provisional Load and stop the progress bar. clearAuthentiation(); return; } > + No new tests added because it doesn't change the current behavior. > > I think that that a more accurate explanation would be "because this only > affects credentials entered by the user, and we cannot test authentication > dialog in DumpRenderTree". Good point! I will update the Changelog. > We need to make the same changes on Windows, but that can be a separate patch > that someone (possibly myself) can make later.

Alexey Proskuryakov

Comment 6 2010-04-26 13:53:12 PDT

> Wouldn't that make WebCore loading stay at a undefined state? Oops, you are right. I think that it would be most consistent to call receivedRequestToContinueWithoutCredential().

Yongjun Zhang

Comment 7 2010-04-26 14:08:17 PDT

Created attachment 54329 [details] Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments.

Alexey Proskuryakov

Comment 8 2010-04-26 14:24:29 PDT

Comment on attachment 54329 [details] Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments. r=me, thanks!

WebKit Commit Bot

Comment 9 2010-04-26 14:44:01 PDT

Comment on attachment 54329 [details] Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments. Clearing flags on attachment: 54329 Committed r58264: <http://trac.webkit.org/changeset/58264>

WebKit Commit Bot

Comment 10 2010-04-26 14:44:07 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.