Bug 209066 - Take into account referrer-policy in append Origin header algorithm
Summary: Take into account referrer-policy in append Origin header algorithm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-13 10:49 PDT by Rob Buis
Modified: 2020-03-26 00:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (10.89 KB, patch)
2020-03-16 10:58 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.96 KB, patch)
2020-03-20 03:41 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.53 KB, patch)
2020-03-24 07:24 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (14.77 KB, patch)
2020-03-25 02:01 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (14.95 KB, patch)
2020-03-25 08:10 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2020-03-25 13:36 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2020-03-13 10:49:36 PDT
The spec states that for the append Origin header algorithm [1] the referrer-policy has to be taken
into account in the case of response tainting not being "cors" and the request is not GET or HEAD.

[1] https://fetch.spec.whatwg.org/#append-a-request-origin-header
Comment 1 Rob Buis 2020-03-16 10:58:50 PDT
Created attachment 393665 [details]
Patch
Comment 2 Rob Buis 2020-03-20 03:41:19 PDT
Created attachment 394074 [details]
Patch
Comment 3 youenn fablet 2020-03-23 07:19:03 PDT
Comment on attachment 394074 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:-2982
> -    addHTTPOriginIfNeeded(request, String());

Why do we do not need this call but we still apparently need applyUserAgentIfNeeded?
Ideally, we would only update user agent and origin as part of CachedResourceLoader::updateHTTPRequestHeaders.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:-240
> -    FrameLoader::addHTTPOriginIfNeeded(m_resourceRequest, outgoingOrigin);

It seems we could compute the referrer policy and then call FrameLoader::addHTTPOriginIfNeeded unconditionally.
Again, it would be good to make addHTTPOriginIfNeeded take a Function returning a String, but only called if actually needed.
Comment 4 Rob Buis 2020-03-23 07:29:14 PDT
Comment on attachment 394074 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.cpp:-2982
>> -    addHTTPOriginIfNeeded(request, String());
> 
> Why do we do not need this call but we still apparently need applyUserAgentIfNeeded?
> Ideally, we would only update user agent and origin as part of CachedResourceLoader::updateHTTPRequestHeaders.

I noticed the same thing, I plan to fix that in a follow up patch.

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:-240
>> -    FrameLoader::addHTTPOriginIfNeeded(m_resourceRequest, outgoingOrigin);
> 
> It seems we could compute the referrer policy and then call FrameLoader::addHTTPOriginIfNeeded unconditionally.
> Again, it would be good to make addHTTPOriginIfNeeded take a Function returning a String, but only called if actually needed.

Ah, I did not get that idea before, will work on that.
Comment 5 Rob Buis 2020-03-24 07:24:21 PDT
Created attachment 394364 [details]
Patch
Comment 6 Rob Buis 2020-03-24 08:09:18 PDT
Comment on attachment 394074 [details]
Patch

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

>>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:-240
>>> -    FrameLoader::addHTTPOriginIfNeeded(m_resourceRequest, outgoingOrigin);
>> 
>> It seems we could compute the referrer policy and then call FrameLoader::addHTTPOriginIfNeeded unconditionally.
>> Again, it would be good to make addHTTPOriginIfNeeded take a Function returning a String, but only called if actually needed.
> 
> Ah, I did not get that idea before, will work on that.

I made that change in the latest upload.
Comment 7 Darin Adler 2020-03-24 09:22:46 PDT
Comment on attachment 394364 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.h:227
> +    static void addHTTPOriginIfNeeded(ResourceRequest&, const ReferrerPolicy&, WTF::Function<RefPtr<SecurityOrigin>()>);

I don’t understand the rationale for using a function here; is it important optimize the case where the function isn’t called? Couldn’t we take the SecurityOrigin and ignore it if not needed. When is the function called? I don’t see anything in the code that makes this clear. Nor the change log!

I suggest just passing the information in, unless it’s critical to save the work for performance.

Or maybe the timing of calling the function is important for some other reason?

Why is the return type RefPtr instead of Ref? That’s not good since the code can’t handle a null result.
Comment 8 Darin Adler 2020-03-24 09:24:24 PDT
Comment on attachment 394364 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.h:227
>> +    static void addHTTPOriginIfNeeded(ResourceRequest&, const ReferrerPolicy&, WTF::Function<RefPtr<SecurityOrigin>()>);
> 
> I don’t understand the rationale for using a function here; is it important optimize the case where the function isn’t called? Couldn’t we take the SecurityOrigin and ignore it if not needed. When is the function called? I don’t see anything in the code that makes this clear. Nor the change log!
> 
> I suggest just passing the information in, unless it’s critical to save the work for performance.
> 
> Or maybe the timing of calling the function is important for some other reason?
> 
> Why is the return type RefPtr instead of Ref? That’s not good since the code can’t handle a null result.

I see that Youenn suggested this (passing a function instead of a security origin value), but I still don’t think it’s an easy-to-understand pattern without more comments, and I think the optimization isn’t necessarily a valuable one.

I think it’s also not great to take a WTF::Function by value since that means we’ll be copying it as part of passing it in.
Comment 9 youenn fablet 2020-03-24 09:40:07 PDT
> I see that Youenn suggested this (passing a function instead of a security
> origin value), but I still don’t think it’s an easy-to-understand pattern
> without more comments, and I think the optimization isn’t necessarily a
> valuable one.

If we do not do that, we might create a temporary SecurityOrigin without actually using it.

In many fetch/xhr, the Origin header is already set, form submissions might use POST quite a bit.

That is why for instance, we have an early return in CachedResourceRequest::updateReferrerOriginAndUserAgentHeaders, to prevent creating this object for every load.
I guess the early return is good enough for most cases so maybe this is overkill.

Or we could add a routine that returns whether we should set the origin header or not and then call setHTTPOrigin.
Comment 10 Darin Adler 2020-03-24 10:01:31 PDT
(In reply to youenn fablet from comment #9)
> > I see that Youenn suggested this (passing a function instead of a security
> > origin value), but I still don’t think it’s an easy-to-understand pattern
> > without more comments, and I think the optimization isn’t necessarily a
> > valuable one.
> 
> If we do not do that, we might create a temporary SecurityOrigin without
> actually using it.

Yes, understood.

> In many fetch/xhr, the Origin header is already set, form submissions might
> use POST quite a bit.
> 
> That is why for instance, we have an early return in
> CachedResourceRequest::updateReferrerOriginAndUserAgentHeaders, to prevent
> creating this object for every load.
> I guess the early return is good enough for most cases so maybe this is
> overkill.

Typically the best way to optimize performance is to actually measure it.

I think that this is a particularly subtle and hard to understand pattern. We don’t typically pass functions to compute things synchronously. Maybe it’s the start of a new pattern, but I think it’s quite peculiar.

> Or we could add a routine that returns whether we should set the origin
> header or not and then call setHTTPOrigin.

Sure, just not sure what problem we are solving.
Comment 11 Rob Buis 2020-03-25 02:01:33 PDT
Created attachment 394476 [details]
Patch
Comment 12 Rob Buis 2020-03-25 03:12:22 PDT
Comment on attachment 394476 [details]
Patch

After the discussion yesterday, I had a new idea, to add a canSetHTTPOrigin method in ResourceRequest.

This will allow the computing of the SecurityOrigin to be done only if needed.

I inlined addHTTPOriginIfNeeded in place, if that is too much we can keep addHTTPOriginIfNeeded and make the call sites a bit smaller. addHTTPOriginIfNeeded would assert canSetHTTPOrigin is true and then compute SecurityOrigin and call setHTTPOrigin.

Note that the ultimate goal is too minimize the number of places where we set the origin, ideally it will just be in updateReferrerOriginAndUserAgentHeaders and loadResourceSynchronously.
Comment 13 youenn fablet 2020-03-25 07:31:44 PDT
Comment on attachment 394476 [details]
Patch

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

> Source/WebCore/loader/FormSubmission.cpp:253
> +        String origin = SecurityPolicy::generateOriginHeader(frameRequest.requester().referrerPolicy(), frameRequest.resourceRequest().url(), securityOrigin);

Could be auto here and below.
Or directly passed to setHTTPOrigin if we at some point make setHTTPOrigin use a String&&.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:406
> +bool ResourceRequestBase::canSetHTTPOrigin() const

s/canSetHTTPOrigin/needsHTTPOrigin.
Could also be a free function: bool doesRequestNeedHTTPOriginHeader(const ResourceRequest&)
Comment 14 Rob Buis 2020-03-25 08:10:23 PDT
Created attachment 394496 [details]
Patch
Comment 15 Rob Buis 2020-03-25 09:19:54 PDT
Comment on attachment 394476 [details]
Patch

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

>> Source/WebCore/loader/FormSubmission.cpp:253
>> +        String origin = SecurityPolicy::generateOriginHeader(frameRequest.requester().referrerPolicy(), frameRequest.resourceRequest().url(), securityOrigin);
> 
> Could be auto here and below.
> Or directly passed to setHTTPOrigin if we at some point make setHTTPOrigin use a String&&.

Made it auto for now.

>> Source/WebCore/platform/network/ResourceRequestBase.cpp:406
>> +bool ResourceRequestBase::canSetHTTPOrigin() const
> 
> s/canSetHTTPOrigin/needsHTTPOrigin.
> Could also be a free function: bool doesRequestNeedHTTPOriginHeader(const ResourceRequest&)

I chose the free function solution.
Comment 16 youenn fablet 2020-03-25 13:33:13 PDT
Comment on attachment 394496 [details]
Patch

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

> Source/WebCore/loader/PingLoader.cpp:140
> +        String origin = SecurityPolicy::generateOriginHeader(document.referrerPolicy(), request.url(), document.securityOrigin());

auto
Comment 17 Rob Buis 2020-03-25 13:36:09 PDT
Created attachment 394540 [details]
Patch
Comment 18 EWS 2020-03-26 00:38:32 PDT
Committed r259036: <https://trac.webkit.org/changeset/259036>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394540 [details].
Comment 19 Radar WebKit Bug Importer 2020-03-26 00:39:15 PDT
<rdar://problem/60908874>