Bug 230935 - Authorization header lost on 30x redirects
Summary: Authorization header lost on 30x redirects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Safari 14
Hardware: All All
: P2 Normal
Assignee: Chris Dumez
URL: https://fetch.spec.whatwg.org/#concep...
Keywords: InRadar
: 168433 239944 (view as bug list)
Depends on: 234253
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-28 21:41 PDT by rli
Modified: 2022-05-17 22:58 PDT (History)
9 users (show)

See Also:


Attachments
WIP Patch (1.47 KB, patch)
2021-10-04 12:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2021-10-04 13:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.84 KB, patch)
2021-10-04 15:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.70 KB, patch)
2021-10-05 08:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description rli 2021-09-28 21:41:06 PDT
When making a request to an endpoint that returns 303 with a Location header, the Authorization header is not being sent on the second request. Behavior seems to be the same with the other 3xx redirect status codes. 
Example https://stackblitz.com/edit/js-xkjyaj?file=index.js
Comment 1 Alexey Proskuryakov 2021-09-29 10:33:09 PDT
Confirmed that this test behaves differently between Safari and Chrome.
Comment 2 Radar WebKit Bug Importer 2021-09-29 14:55:37 PDT
<rdar://problem/83689955>
Comment 3 Chris Dumez 2021-10-01 15:43:07 PDT
https://fetch.spec.whatwg.org/#concept-http-redirect-fetch says to use the same request as the original one except for the URL in this case. 

NetworkResourceLoader::willSendRedirectedRequest() gets called with a redirectRequest which already doesn't include the Authorization header so I guess it is not WebKit that's dropping it but likely CFNetwork under us?
Comment 4 Chris Dumez 2021-10-04 12:13:15 PDT
(In reply to Chris Dumez from comment #3)
> https://fetch.spec.whatwg.org/#concept-http-redirect-fetch says to use the
> same request as the original one except for the URL in this case. 
> 
> NetworkResourceLoader::willSendRedirectedRequest() gets called with a
> redirectRequest which already doesn't include the Authorization header so I
> guess it is not WebKit that's dropping it but likely CFNetwork under us?

I added logging inside:
- (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task willPerformHTTPRedirection:(NSHTTPURLResponse *)response newRequest:(NSURLRequest *)request completionHandler:(void (^)(NSURLRequest *))completionHandler;

And I see that the NSURLRequest provided by CFNetwork for the redirect is missing the Authorization header. I believe this confirms that CFNetwork is indeed dropping the header. I guess WebKit could add the authorization header back after getting the request from CFNetwork but it would feel a bit hacking. I am curious why CFNetwork would be dropping it.
Comment 5 Chris Dumez 2021-10-04 12:29:26 PDT
Created attachment 440087 [details]
WIP Patch
Comment 6 Alex Christensen 2021-10-04 12:30:57 PDT
Comment on attachment 440087 [details]
WIP Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:501
> +        if (auto authorization = m_firstRequest.httpHeaderField(WebCore::HTTPHeaderName::Authorization); !authorization.isNull())

We likely only want to do this if it's not a cross-origin redirect.  Line 470 could be reused to tell.
Comment 7 Chris Dumez 2021-10-04 12:32:02 PDT
Comment on attachment 440087 [details]
WIP Patch

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

>> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:501
>> +        if (auto authorization = m_firstRequest.httpHeaderField(WebCore::HTTPHeaderName::Authorization); !authorization.isNull())
> 
> We likely only want to do this if it's not a cross-origin redirect.  Line 470 could be reused to tell.

That's what I did, no? That's why I am in the else case (the if case on line 494 clears the Authorization header).
Comment 8 Alex Christensen 2021-10-04 12:32:40 PDT
Comment on attachment 440087 [details]
WIP Patch

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

>>> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:501
>>> +        if (auto authorization = m_firstRequest.httpHeaderField(WebCore::HTTPHeaderName::Authorization); !authorization.isNull())
>> 
>> We likely only want to do this if it's not a cross-origin redirect.  Line 470 could be reused to tell.
> 
> That's what I did, no? That's why I am in the else case (the if case on line 494 clears the Authorization header).

Oh, you're right.
Comment 9 Chris Dumez 2021-10-04 13:27:58 PDT
Created attachment 440098 [details]
Patch
Comment 10 Chris Dumez 2021-10-04 15:23:40 PDT
Created attachment 440111 [details]
Patch
Comment 11 Chris Dumez 2021-10-05 08:35:08 PDT
Created attachment 440217 [details]
Patch
Comment 12 EWS 2021-10-05 11:48:18 PDT
Committed r283565 (242530@main): <https://commits.webkit.org/242530@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440217 [details].
Comment 13 rli 2021-10-07 21:25:23 PDT
Thanks for verifying and making the change! I'm not familiar with the process, how do I find out when I can test this in browsers?
Comment 14 Chris Dumez 2021-10-08 08:07:07 PDT
(In reply to rli from comment #13)
> Thanks for verifying and making the change! I'm not familiar with the
> process, how do I find out when I can test this in browsers?

We are unfortunately unable to provide insight about when Apple will ship a particular WebKit fix in Safari. What I can say is that you'll be able to test this change either in the next Safari Technology Preview build or the one after that (You can check the STP changelog to confirm this change is in there).

When it comes to regular Safari, all I can say is that historically, Safari has been picking up trunk WebKit every 6 months.
Comment 15 rli 2021-10-08 08:16:32 PDT
Thank you!
Comment 16 Arcady Goldmints-Orlov 2021-11-19 14:31:48 PST
*** Bug 168433 has been marked as a duplicate of this bug. ***
Comment 17 youenn fablet 2022-05-17 22:56:27 PDT
*** Bug 239944 has been marked as a duplicate of this bug. ***
Comment 18 youenn fablet 2022-05-17 22:58:16 PDT
It shipped in iOS 15.4/macOS12.3 Safaris