Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Location header is the empty string #669

Open
mfalken opened this issue Feb 9, 2018 · 7 comments
Open

Location header is the empty string #669

mfalken opened this issue Feb 9, 2018 · 7 comments

Comments

@mfalken
Copy link

mfalken commented Feb 9, 2018

I'm trying to figure out what happens when you do a fetch() in follow mode, and get a 302 response with a Location header whose value is the empty string.

Chrome returns a response with type 'basic', status 302, and a Location header with the empty string. (It also crashes in debug mode.) Firefox returns a network error.

For a similar test where Location is omitted, Chrome returns a response with type 'basic', status 302, and no Location header. Firefox returns the same.

What does the spec say and people think? Here is my proposed WPT test for reference:
https://chromium-review.googlesource.com/c/chromium/src/+/910753

@wanderview

@mfalken
Copy link
Author

mfalken commented Feb 9, 2018

We ran through the spec steps and decided it's saying network error, which aligns with Firefox.

@domenic
Copy link
Member

domenic commented Feb 9, 2018

To me the steps appear unclear:

5.2. Let location be the result of extracting header list values given Location and actualResponse’s header list.

5.3. If location is a value, then set location to the result of parsing location with actualResponse’s url.

I'm not sure when location could ever be a value; "extract header list values" returns either null or a list. Seems like a spec bug.

But if we assume that conditions is not triggered, then we set location URL to null. Then in HTTP-redirect fetch step 2, response gets returned, so more like Chrome's behavior.

So per the spec as written I think Chrome's behavior makes sense. But the spec has a pretty bad bug in this area which casts doubt on the result, IMO.

I'm curious to see what your analysis was; perhaps I missed something.

@mfalken
Copy link
Author

mfalken commented Feb 9, 2018

Good catch on 5.2 and 5.3. I just assumed location would be the empty string, which is a value.
Editing slightly from @yutakahirano 's comment on the CL:

@domenic
Copy link
Member

domenic commented Feb 9, 2018

I think the URL parsing there is incorrect. The URL parser takes two parameters: input and base. It is called with two arguments by Fetch: location and actualResponse's url. So you should parse the empty string relative to the base URL given by the response URL. So you get the response URL as the resulting returned URL (= url inside the algorithm).

Of course by that reading I think we end up with an infinite redirect loop to itself, at least until the 20-redirect-limit is hit. So that seems like a spec bug too.

@mfalken
Copy link
Author

mfalken commented Feb 12, 2018

Interesting. The redirect-limit hit seems to match what Chrome does when Location is some nonsense characters like "#invalidurl:" or "\b", so maybe the spec is not so off. That's why the WPT test redirect-location.https.html https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/fetch/api/redirect/redirect-location.js?l=43&rcl=2676595f0db73d5f35359dd4a33fd8b98122bd7c is so slow in Chrome (the invalid URL test redirects to itself).

I'm not sure what the other browsers are doing in that test.

@annevk
Copy link
Member

annevk commented Feb 16, 2018

Of course by that reading I think we end up with an infinite redirect loop to itself, at least until the 20-redirect-limit is hit. So that seems like a spec bug too.

Why? If you instead put in the parsed-then-serialized URL you'd get the same result.

I agree that extracting header values needs to be improved at some point...

@mfalken
Copy link
Author

mfalken commented Feb 16, 2018

I added a WPT in web-platform-tests/wpt#9453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants