OOR-CORS: ServiceWorkerBrowserTest.CrossOriginFetchWithSaveData fails |
|||||
Issue descriptionNow the test checks if 1. the CORS preflight request has 'Save-Data: on' header, AND 2. it also does not have 'save-data' in the 'Access-Control-Allow-Headers' header. But, as far as reading comments, checking 2. seems to be important here. Do you think we still need the check 1.? Once OOR-CORS is enabled, the CORS preflight request is initiated in the NetworkService, and at this moment, we do not have a code path to inject the Save-Data headers here. If we need, probably, we can just copy it from the original request header?
,
Nov 21
https://fetch.spec.whatwg.org/#cors-preflight-fetch Current fetch spec takes the Save-Data into account, as it is in the CORS-safelisted request-header, but there is no description that says CORS preflight request also can/should have the Save-Data header. https://httpwg.org/http-extensions/client-hints.html#save-data Also, client hints spec does not say anything about CORS. So my current preference is not to send it in the CORS preflight, and remove the checks from the test. We may add an opposite check, no 'Save-Data' check, once OOR-CORS is enabled. WDYT? Assign to tbansal@ to hear opinion from a Save-Data expert.
,
Nov 21
Just to be clear, save-data is a CORS safelisted header. So, my understanding is that save-data would never be included in Access-Control-Request-Headers. Is that correct? I do not think "save-data: on" is needed in the HTTP request headers for the preflight request.
,
Nov 21
> client hints spec does not say anything about CORS. Client hints headers are CORS safelisted. I think the spec only mentions this for "DPR", "Viewport-Width" and few other client hints, but not all.
,
Nov 21
> Just to be clear, save-data is a CORS safelisted header. So, my understanding is that save-data would never be included in Access-Control-Request-Headers. Is that correct? Yes, it's correct. Only unsafe headers should appear there. > I do not think "save-data: on" is needed in the HTTP request headers for the preflight request. Thanks! So, I will make a change for the test, and send a review request to you and service worker people.
,
Nov 21
,
Nov 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b662cdc3a304b1d2a73738e290e9be7be0baca6a commit b662cdc3a304b1d2a73738e290e9be7be0baca6a Author: Takashi Toyoshima <toyoshim@chromium.org> Date: Thu Nov 22 03:49:44 2018 OOR-CORS: CORS-preflight requests do not need to have "Save-Data" header Currently ServiceWorkerBrowserTest.CrossOriginFetchWithSaveData checks if CORS-preflight requests have "Save-Data" header. This is existing behavior, but new CORS stack does not add the "Save-Data" header for the CORS-preflight. In terms of the fetch spec, it will be better not to have the header in the preflight request. Bug: 907389 Change-Id: Ic6906f5a9ce270a403b7be6f48274f78d2fce546 Reviewed-on: https://chromium-review.googlesource.com/c/1345685 Reviewed-by: Tarun Bansal <tbansal@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org> Cr-Commit-Position: refs/heads/master@{#610295} [modify] https://crrev.com/b662cdc3a304b1d2a73738e290e9be7be0baca6a/content/browser/service_worker/service_worker_browsertest.cc
,
Nov 22
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by toyoshim@chromium.org
, Nov 21