Bug 209066 – Take into account referrer-policy in append Origin header algorithm

• Component: New Bugs
• Status: RESOLVED INVALID

[Rob Buis]

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

[Rob Buis]

Created attachment 393665 [details]
Patch

[Rob Buis]

Created attachment 394074 [details]
Patch

[youenn fablet]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 394364 [details]
Patch

[Rob Buis]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[youenn fablet]

> 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.

[Darin Adler]

(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.

[Rob Buis]

Created attachment 394476 [details]
Patch

[Rob Buis]

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.

[youenn fablet]

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&)

[Rob Buis]

Created attachment 394496 [details]
Patch

[Rob Buis]

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.

[youenn fablet]

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

[Rob Buis]

Created attachment 394540 [details]
Patch

[EWS]

Committed r259036: <https://trac.webkit.org/changeset/259036>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394540 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60908874>

[Chris Dumez]

Comment on attachment 394540 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Take into account referrer-policy in append Origin header algorithm

I assume this has a behavior change. Why no tests?

[Rob Buis]

(In reply to Chris Dumez from comment #20)
> Comment on attachment 394540 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394540&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Take into account referrer-policy in append Origin header algorithm
> 
> I assume this has a behavior change. Why no tests?

This is tested by fetch/origin/assorted.window.js. It got imported after this patch, so possibly at the time this patch landed it was not enough to pass that test and I had to fix something else before finally importing that test (which needed an update later on). Sorry my memory is bad on these things, I do too much context switching to remember :)

[Chris Dumez]

(In reply to Rob Buis from comment #21)
> (In reply to Chris Dumez from comment #20)
> > Comment on attachment 394540 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394540&action=review
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        Take into account referrer-policy in append Origin header algorithm
> > 
> > I assume this has a behavior change. Why no tests?
> 
> This is tested by fetch/origin/assorted.window.js. It got imported after
> this patch, so possibly at the time this patch landed it was not enough to
> pass that test and I had to fix something else before finally importing that
> test (which needed an update later on). Sorry my memory is bad on these
> things, I do too much context switching to remember :)

Yes, I am looking into this test right now and we seem to be failing:
http://w3c-test.org/fetch/origin/assorted.window.html

We time out on:
Origin header and POST cross-origin navigation with Referrer-Policy origin-when-cross-origin

[Chris Dumez]

(In reply to Chris Dumez from comment #22)
> (In reply to Rob Buis from comment #21)
> > (In reply to Chris Dumez from comment #20)
> > > Comment on attachment 394540 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=394540&action=review
> > > 
> > > > Source/WebCore/ChangeLog:3
> > > > +        Take into account referrer-policy in append Origin header algorithm
> > > 
> > > I assume this has a behavior change. Why no tests?
> > 
> > This is tested by fetch/origin/assorted.window.js. It got imported after
> > this patch, so possibly at the time this patch landed it was not enough to
> > pass that test and I had to fix something else before finally importing that
> > test (which needed an update later on). Sorry my memory is bad on these
> > things, I do too much context switching to remember :)
> 
> Yes, I am looking into this test right now and we seem to be failing:
> http://w3c-test.org/fetch/origin/assorted.window.html
> 
> We time out on:
> Origin header and POST cross-origin navigation with Referrer-Policy
> origin-when-cross-origin

I will also note that Chrome's behavior on this test is different and might explain regressions such as Bug 211122 in Safari only.
I understand we match the spec. However, it does not mean the change is Web-Compatible.

[Chris Dumez]

Reopening since this was reverted in https://trac.webkit.org/changeset/261036 due to Jira breakage.