Bug 199261 – Align with Origin header changes

• Component: DOM
• Status: RESOLVED FIXED

[Anne van Kesteren]

https://github.com/whatwg/fetch/pull/908 clarifies how the Origin header interacts with Referrer Policy. https://github.com/web-platform-tests/wpt/pull/14260 adds a number of tests around that.

[Rob Buis]

Created attachment 388777 [details]
Patch

[Rob Buis]

Created attachment 388779 [details]
Patch

[Rob Buis]

Created attachment 392257 [details]
Patch

[Rob Buis]

Created attachment 392400 [details]
Patch

[youenn fablet]

Comment on attachment 392400 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:504
> +    submission->setOrigin(SecurityPolicy::generateOriginHeader(m_frame.document()->referrerPolicy(), submission->requestURL(), SecurityOrigin::createFromString(outgoingOrigin())));

Let's change SecurityOrigin::createFromString(outgoingOrigin()) into m_frame.document()->securityOrigin()

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:233
> +        outgoingOrigin = SecurityPolicy::generateOriginHeader(m_options.referrerPolicy, m_resourceRequest.url(), SecurityOrigin::createFromString(outgoingReferrer));

If the resource request has a referrer, we will create a SecurityOrigin from the outgoingReferrer twice.
Can we update the code to only do that once if needed?

Also, it is not clear to me why we have to check the mode here.
The spec does not say anything about that.

[Rob Buis]

Created attachment 393050 [details]
Patch

[youenn fablet]

Comment on attachment 393050 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:230
>      }

Remove { }

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:231
> +    if (m_options.mode == FetchOptions::Mode::Cors)

If mode is CORS, we probably already set the origin header, right?
So FrameLoader::addHTTPOriginIfNeeded will be a no-op.

It might be better to do all this processing if the request does not have any origin.
Either using 
if (!m_resourceRequest.httpOrigin().isEmpty())
    return;

Or by introducing something like FrameLoader::addHTTPOriginIfiNeeded(Request, const Function<String()>& computeOriginValue)

[Rob Buis]

Created attachment 393063 [details]
Patch

[Rob Buis]

Comment on attachment 392400 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.cpp:504
>> +    submission->setOrigin(SecurityPolicy::generateOriginHeader(m_frame.document()->referrerPolicy(), submission->requestURL(), SecurityOrigin::createFromString(outgoingOrigin())));
> 
> Let's change SecurityOrigin::createFromString(outgoingOrigin()) into m_frame.document()->securityOrigin()

Done.

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:233
>> +        outgoingOrigin = SecurityPolicy::generateOriginHeader(m_options.referrerPolicy, m_resourceRequest.url(), SecurityOrigin::createFromString(outgoingReferrer));
> 
> If the resource request has a referrer, we will create a SecurityOrigin from the outgoingReferrer twice.
> Can we update the code to only do that once if needed?
> 
> Also, it is not clear to me why we have to check the mode here.
> The spec does not say anything about that.

Done. The spec does not say anything about mode but does mention tainting, step 2 in https://fetch.spec.whatwg.org/#append-a-request-origin-header.
I am not sure if I can test the tainting here though.

[Darin Adler]

Comment on attachment 393063 [details]
Patch

EWS is showing origin-related test failures. Please don’t land without resolving those

[Rob Buis]

Yes I was trying to verify Youenn's question "If mode is CORS, we probably already set the origin header, right?". Since the ASSERT_NOT_REACHED is reached, we know the answer is no.
But anyway I want Youenn to approve the patch before landing, so hopefully some time tomorrow.

[Rob Buis]

Created attachment 393073 [details]
Patch

[Rob Buis]

Comment on attachment 393050 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:230
>>      }
> 
> Remove { }

Done.

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:231
>> +    if (m_options.mode == FetchOptions::Mode::Cors)
> 
> If mode is CORS, we probably already set the origin header, right?
> So FrameLoader::addHTTPOriginIfNeeded will be a no-op.
> 
> It might be better to do all this processing if the request does not have any origin.
> Either using 
> if (!m_resourceRequest.httpOrigin().isEmpty())
>     return;
> 
> Or by introducing something like FrameLoader::addHTTPOriginIfiNeeded(Request, const Function<String()>& computeOriginValue)

So for CORS we do not set the origin header (I added an ASSERT_NOT_REACHED and it was hit a lot as you can see in the previous upload).
I would really like to centralize the places to where the origin header is set, ideally to one, so here, but I know it is needed in a few other places like sync loading.

I made an attempt to return early if the Origin is already set to make things a bit more effiicient.

[youenn fablet]

Comment on attachment 393073 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:236
> +        outgoingOrigin = SecurityOrigin::createFromString(outgoingReferrer)->toString();

The append Origin algorithm is making use of response tainting == cors, which is different from the mode.
Probably we have some slight behavioural differences, or maybe we already set the origin header in the cases that matter.
Would be worth investigating.

It would also be nice to add a routine that implements the whole https://fetch.spec.whatwg.org/#append-a-request-origin-header algorithm.
We have some bits here and there which makes it hard to know whether we are doing the right thing.
For instance HEAD/GET check is done in FrameLoader::addHTTPOriginIfNeeded, not here.

All of this might best be done as follow-ups though, let's go with this patch.

[Rob Buis]

Comment on attachment 393073 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:236
>> +        outgoingOrigin = SecurityOrigin::createFromString(outgoingReferrer)->toString();
> 
> The append Origin algorithm is making use of response tainting == cors, which is different from the mode.
> Probably we have some slight behavioural differences, or maybe we already set the origin header in the cases that matter.
> Would be worth investigating.
> 
> It would also be nice to add a routine that implements the whole https://fetch.spec.whatwg.org/#append-a-request-origin-header algorithm.
> We have some bits here and there which makes it hard to know whether we are doing the right thing.
> For instance HEAD/GET check is done in FrameLoader::addHTTPOriginIfNeeded, not here.
> 
> All of this might best be done as follow-ups though, let's go with this patch.

Yes, it is possible that cors taint vs mode has a slight behavior difference, will keep it in mind. I also agree that a central place for https://fetch.spec.whatwg.org/#append-a-request-origin-header is needed, addHTTPOriginIfNeeded seems the best candidate and I will keep working on it. Thanks for the reviews!

[WebKit Commit Bot]

Comment on attachment 393073 [details]
Patch

Clearing flags on attachment: 393073

Committed r258194: <https://trac.webkit.org/changeset/258194>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60268381>