1460299 - Fetch: content-length header is being added to the safe-list

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Shachar]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36

Steps to reproduce:

1. create a CORS fetch request to a url that has content-length in its response and doesn't add content-length to access-control-expose-headers
2.  content-length should be accessible in the response's headers


Actual results:

content-length is not exposed



Expected results:

When doing a CORS fetch request if content-length exists in the http response it should be exposed on the response headers even if it's not added to the access-control-expose-headers.


The spec is under change:
https://github.com/whatwg/fetch/pull/626
https://github.com/w3c/web-platform-tests/pull/10930

Comment 1

[Shachar]

Hey any update here?
Edge fixed the issue - https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/17440391/
Webkit as well - https://bugs.webkit.org/show_bug.cgi?id=185473

Comment 2

[Forrest]

Attachment: Bug 1460299 - Add content-length as a CORS-safelisted response header.

Reference: https://fetch.spec.whatwg.org/#cors-safelisted-response-header

Comment 3

[Forrest]

Who would be the most suitable person to review the changes? This is my first time here, so I'm not familiar with the process of requesting a reviewer.

Comment 5

[Anne (:annevk)]

Hey Forrest (and Shachar), apologies for the lack of follow-up here. Junior can most likely take a look at your patch.

Comment 6

[Junior [inactive]]

Hello Forrest, do you want to continue the process?
I can do the review. Just set the reviewer to JuniorHsu in phabricator.

Comment 7

[Forrest]

Sorry for the extended delay. I'm pretty sure I accidentally deleted the notification emails a while back when mass-deleting old emails, and I'm just revisiting this.

Yes, I'm interested in moving forward with this patch. However, it appears that Junior's account is disabled in Phabricator. Is there anyone else I can set as the reviewer?

Comment 8

[Tom S [:evilpie]]

I think Andrea [baku] might be able to review this based on some blame info for InternalHeaders.cpp. I added them as a reviewer.

Comment 9

[ehsueh]

Hi all, what's the status on this issue? Is this content-length now added as a safelisted header now?

Comment 10

[Anne (:annevk)]

ehsueh, thanks for the reminder. Valentin, can you ensure https://phabricator.services.mozilla.com/D58492 lands? Looks like all is in order there.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aefbe65483ef
Add content-length as a CORS-safelisted response header. r=valentin,baku

Comment 12

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/27481 for changes under testing/web-platform/tests

Comment 13

[Atila Butkovits]

Backed out for causing failures cors-filtering.sub.any.worker.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/55cf2cda9a1002e345e5f5821701fb314fa2aab1

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Cwebrender%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-android-em-7.0-x86_64-qr%2Fdebug-geckoview-web-platform-tests-e10s%2Cwpt3&revision=aefbe65483efbc4e3bc28899351e4a06a292eac3&selectedTaskRun=D-608QhQTn-10kxkIb2PJA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328841702&repo=autoland&lineNumber=2485

Comment 14

[Valentin Gosu [:valentin] (he/him)]

It seems you just need to remove this file:
https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/testing/web-platform/meta/fetch/api/cors/cors-filtering.sub.any.js.ini

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 16

[Forrest]

I've updated that test-case to expect passing results.

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1460299 - Remove cors-filtering.sub.any.js.ini a=test-only

Depends on D58492

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4615f67be1a8
Add content-length as a CORS-safelisted response header. r=valentin,baku
https://hg.mozilla.org/integration/autoland/rev/3b0711521076
Remove cors-filtering.sub.any.js.ini a=test-only

Comment 19

[Forrest]

Oh thanks! I pushed the change and didn't notice that it was already closed after you made the change. You can ignore the recent change on phabricator.

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4615f67be1a8
https://hg.mozilla.org/mozilla-central/rev/3b0711521076

Comment 21

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 22

[Hamish Willee]

FF87 docs for this are being done as part of https://github.com/mdn/content/pull/2766. Still waiting on discussion on BCD. Note also a little scope creep in docs for this one because I found the existing descriptions for the CORS response header safelist a bit confusing.

Comment 23

[Chris Mills [:cmills]]

Setting this one to DDC; the BCD work is very nearly there.