Bug 184723 - Allow SameOrigin credentials handling for synchronous XHR
Summary: Allow SameOrigin credentials handling for synchronous XHR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-17 19:18 PDT by youenn fablet
Modified: 2018-04-18 17:48 PDT (History)
4 users (show)

See Also:


Attachments
Patch (19.28 KB, patch)
2018-04-17 19:25 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (19.29 KB, patch)
2018-04-17 21:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (21.90 KB, patch)
2018-04-18 16:20 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-04-17 19:18:34 PDT
Allow cross-origin redirections for synchronous XHR
Comment 1 youenn fablet 2018-04-17 19:25:48 PDT
Created attachment 338185 [details]
Patch
Comment 2 youenn fablet 2018-04-17 21:59:20 PDT
Created attachment 338195 [details]
Patch
Comment 3 Alex Christensen 2018-04-18 10:16:15 PDT
Comment on attachment 338195 [details]
Patch

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

Are these WK1 regressions, or are we just preserving existing behavior and that's hard to see in the patch?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:134
> +    void startNetworkLoad(WebCore::ResourceRequest&&, FirstLoad = FirstLoad::Yes);

I'd prefer to not have a default parameter so we don't accidentally omit the parameter when writing redirection code.
Comment 4 youenn fablet 2018-04-18 10:23:17 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 338195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338195&action=review
> 
> Are these WK1 regressions, or are we just preserving existing behavior and
> that's hard to see in the patch?

For WK1, we keep the previous behavior (no change to WebCore nor WebKitLegacy) which is to not follow any cross origin redirection.

> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.h:134
> > +    void startNetworkLoad(WebCore::ResourceRequest&&, FirstLoad = FirstLoad::Yes);
> 
> I'd prefer to not have a default parameter so we don't accidentally omit the
> parameter when writing redirection code.

There are 6 calls to startNetworkLoad so I would tend to prefer keeping it, but I can make the change.
If we are not using the right parameter, there will be crashes anyway since we are consuming sandbox extensions.
Comment 5 Alex Christensen 2018-04-18 14:14:17 PDT
Comment on attachment 338195 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:134
>>> +    void startNetworkLoad(WebCore::ResourceRequest&&, FirstLoad = FirstLoad::Yes);
>> 
>> I'd prefer to not have a default parameter so we don't accidentally omit the parameter when writing redirection code.
> 
> There are 6 calls to startNetworkLoad so I would tend to prefer keeping it, but I can make the change.
> If we are not using the right parameter, there will be crashes anyway since we are consuming sandbox extensions.

I'd still prefer to add explicit calls.
Comment 6 youenn fablet 2018-04-18 16:20:18 PDT
Created attachment 338273 [details]
Patch for landing
Comment 7 youenn fablet 2018-04-18 16:20:39 PDT
(In reply to Alex Christensen from comment #5)
> Comment on attachment 338195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338195&action=review
> 
> >>> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:134
> >>> +    void startNetworkLoad(WebCore::ResourceRequest&&, FirstLoad = FirstLoad::Yes);
> >> 
> >> I'd prefer to not have a default parameter so we don't accidentally omit the parameter when writing redirection code.
> > 
> > There are 6 calls to startNetworkLoad so I would tend to prefer keeping it, but I can make the change.
> > If we are not using the right parameter, there will be crashes anyway since we are consuming sandbox extensions.
> 
> I'd still prefer to add explicit calls.

Done in the uploaded patch
Comment 8 WebKit Commit Bot 2018-04-18 17:47:26 PDT
Comment on attachment 338273 [details]
Patch for landing

Clearing flags on attachment: 338273

Committed r230791: <https://trac.webkit.org/changeset/230791>
Comment 9 WebKit Commit Bot 2018-04-18 17:47:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-04-18 17:48:23 PDT
<rdar://problem/39548418>