Bug 66354 – Regression: On Lion, redirects lose HTTP authentication headers

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

Regression:  On Lion, redirects lose HTTP authentication headers

Lion CFNetwork no longer carries offer authentication headers for redirects.  The WebCore credential storage can easily do it's automatic application of authentication headers in the case where redirects are within the same origin.

In radar as <rdar://problem/9965209>

[Brady Eidson]

Created attachment 104129 [details]
Patch v1 - Apply any known credentials inside willSendRequest for same origin redirects.

[Alexey Proskuryakov]

Comment on attachment 104129 [details]
Patch v1 - Apply any known credentials inside willSendRequest for same origin redirects.

View in context: https://bugs.webkit.org/attachment.cgi?id=104129&action=review

Looks good to me, but the fix needs a test and a ResourceHandleCFNet.cpp counterpart.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:503
> +        Credential credential = CredentialStorage::get(request.url());

Stored credentials probably shouldn't be applied if the redirect had credentials in its Location header (Location: http://user:[e-mail]/path).

Also, perhaps there should be some kind of comment explaining that the above request.clearHTTPAuthorization() call is not needed on Lion. Right now, the code looks a bit confusing unless you know that we try to work regardless of whether CFNetwork has removed credentials.

[Brady Eidson]

*** Bug 66389 has been marked as a duplicate of this bug. ***

[Brady Eidson]

Created attachment 104239 [details]
Patch v2 - Requested changes + layout tests

[Alexey Proskuryakov]

Comment on attachment 104239 [details]
Patch v2 - Requested changes + layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=104239&action=review

Great tests!

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:556
> +                d->m_initialCredential = credential;
> +                
> +                // FIXME: Support Digest authentication, and Proxy-Authorization.
> +                applyBasicAuthorizationHeader(request, d->m_initialCredential);

I think that you wanted to limit this to actual redirect case for now, since for original request, we have already applied the credentials (but do get a willSendRequest on at least some platforms, I think).

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:512
> +                d->m_initialCredential = credential;
> +                
> +                // FIXME: Support Digest authentication, and Proxy-Authorization.
> +                applyBasicAuthorizationHeader(request, d->m_initialCredential);

Ditto.

> LayoutTests/ChangeLog:11
> +        * fast/loader/stateobjects/state-url-sets-links-visited.html:

This file shouldn't have been changed.

> LayoutTests/fast/loader/stateobjects/state-url-sets-links-visited.html:17
> +        alert(style1.color);
> +        alert(style2.color);

Accidental changes here.

[Darin Adler]

Comment on attachment 104239 [details]
Patch v2 - Requested changes + layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=104239&action=review

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:139
> +    String authHeader = "Basic " + base64Encode(String(credential.user() + ":" + credential.password()).utf8());

I see no reason to use an abbreviation "auth" here. I suggest naming this local variable value, field, headerValue, or authorization. Or, authorizationHeader. Something made out of words.

I’m surprised the explicit cast to String is needed. Can't you use use parentheses there without the String?

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:548
>          request.clearHTTPAuthorization();

We could consider clearing authorization explicitly in all cases, since we will re-apply it now. Probably a risky change, though.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:550
> +        if (d->m_user.isEmpty() && d->m_pass.isEmpty()) {

I think this needs a why comment. It's not clear to me at all why the next code should run only if the user and password is empty.

[Brady Eidson]

(In reply to comment #6)
> I’m surprised the explicit cast to String is needed. Can't you use use parentheses there without the String?

Nope - Something about WTF::StringAppend.  Fun times.

[Brady Eidson]

http://trac.webkit.org/changeset/93247

[Ryosuke Niwa]

According to test failures, the following tests have been failing since this patch was landed.
http/tests/misc/authentication-redirect-2/authentication-sent-to-redirect-same-origin.html
http/tests/misc/authentication-redirect-3/authentication-sent-to-redirect-same-origin-with-location-credentials.html

[Alexey Proskuryakov]

I'm going to look into this today if Brady doesn't beat me to it.