Bug 66354 - Regression: On Lion, redirects lose HTTP authentication headers
Summary: Regression: On Lion, redirects lose HTTP authentication headers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
: 66389 (view as bug list)
Depends on: 67374
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-16 17:10 PDT by Brady Eidson
Modified: 2011-09-01 10:47 PDT (History)
2 users (show)

See Also:


Attachments
Patch v1 - Apply any known credentials inside willSendRequest for same origin redirects. (3.16 KB, patch)
2011-08-16 17:25 PDT, Brady Eidson
ap: review-
beidson: commit-queue-
Details | Formatted Diff | Diff
Patch v2 - Requested changes + layout tests (37.82 KB, patch)
2011-08-17 14:13 PDT, Brady Eidson
ap: review+
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2011-08-16 17:10:32 PDT
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>
Comment 1 Brady Eidson 2011-08-16 17:25:36 PDT
Created attachment 104129 [details]
Patch v1 - Apply any known credentials inside willSendRequest for same origin redirects.
Comment 2 Alexey Proskuryakov 2011-08-16 17:51:58 PDT
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:pass@new.host/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.
Comment 3 Brady Eidson 2011-08-17 10:20:14 PDT
*** Bug 66389 has been marked as a duplicate of this bug. ***
Comment 4 Brady Eidson 2011-08-17 14:13:20 PDT
Created attachment 104239 [details]
Patch v2 - Requested changes + layout tests
Comment 5 Alexey Proskuryakov 2011-08-17 14:28:54 PDT
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.
Comment 6 Darin Adler 2011-08-17 14:31:18 PDT
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.
Comment 7 Brady Eidson 2011-08-17 14:39:58 PDT
(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.
Comment 8 Brady Eidson 2011-08-17 14:44:01 PDT
http://trac.webkit.org/changeset/93247
Comment 9 Ryosuke Niwa 2011-09-01 00:55:33 PDT
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
Comment 10 Alexey Proskuryakov 2011-09-01 09:03:49 PDT
I'm going to look into this today if Brady doesn't beat me to it.