Bug 38128 – Don't add empty credential to CredentialStorage.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yongjun Zhang]

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.

[Yongjun Zhang]

Created attachment 54315 [details]
First attempt to fix this issue.

Add empty credential check before adding it to CredentialStorage.

[David Levin]

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]

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

[Alexey Proskuryakov]

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]

(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]

> 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]

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

[Alexey Proskuryakov]

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

r=me, thanks!

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.