Bug 38128 - Don't add empty credential to CredentialStorage.
Summary: Don't add empty credential to CredentialStorage.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-26 10:06 PDT by Yongjun Zhang
Modified: 2010-04-26 14:44 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 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.
Comment 1 Yongjun Zhang 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.
Comment 2 David Levin 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.
Comment 3 Yongjun Zhang 2010-04-26 10:54:04 PDT
Created attachment 54318 [details]
change the ChangeLog as per Davin's comment.
Comment 4 Alexey Proskuryakov 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.
Comment 5 Yongjun Zhang 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.
Comment 6 Alexey Proskuryakov 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().
Comment 7 Yongjun Zhang 2010-04-26 14:08:17 PDT
Created attachment 54329 [details]
Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments.
Comment 8 Alexey Proskuryakov 2010-04-26 14:24:29 PDT
Comment on attachment 54329 [details]
Use receivedRequestToContinueWithoutCredential for empty credentials as per Alexey comments.

r=me, thanks!
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2010-04-26 14:44:07 PDT
All reviewed patches have been landed.  Closing bug.