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.
Created attachment 388777 [details] Patch
Created attachment 388779 [details] Patch
Created attachment 392257 [details] Patch
Created attachment 392400 [details] Patch
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.
Created attachment 393050 [details] Patch
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)
Created attachment 393063 [details] Patch
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.
Comment on attachment 393063 [details] Patch EWS is showing origin-related test failures. Please don’t land without resolving those
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.
Created attachment 393073 [details] Patch
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.
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.
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!
Comment on attachment 393073 [details] Patch Clearing flags on attachment: 393073 Committed r258194: <https://trac.webkit.org/changeset/258194>
All reviewed patches have been landed. Closing bug.
<rdar://problem/60268381>