search

grantila / fetch-h2

HTTP/1+2 Fetch API client for Node.js
MIT License
336 stars 16 forks source link

Allow illegal redirect responses #40

Closed triblondon closed 1 year ago

triblondon commented 5 years ago

An HTTP response that has a redirection status but no Location header is invalid per the Fetch spec, but it's still a valid HTTP response. It seems reasonable that a developer may want to receive and parse the response as normal, and there should be an escape valve from the illegal redirect error.

This strikes me as a similar case to other browser-security-model rules of fetch, such as suppressing certain headers, rules which are required in the browser but make fetch-h2 less useful as a generic HTTP client server-side. Therefore my tentative suggestion is to disable illegal redirect errors if the existing allowForbiddenHeaders flag is true.

I also considered suggesting an additional flag, but that seems like overkill to me. However, it does leave the flag slightly awkwardly named, and I guess enforceValidation might be a more generic way to refer to it if you agree to bundle these behaviours into one flag.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.09%) to 83.193% when pulling 90dd7e383b3c407ed89dc35d893de82239f1ab92 on triblondon:patch-2 into 89236f4d7b3e1c7616ea7bbb5ed59308832e6768 on grantila:master.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.09%) to 83.193% when pulling 90dd7e383b3c407ed89dc35d893de82239f1ab92 on triblondon:patch-2 into 89236f4d7b3e1c7616ea7bbb5ed59308832e6768 on grantila:master.

grantila commented 5 years ago

Hmm, what can you do with such a response? Is it at all useful? Surely it's proper HTTP (if the headers and body is well-formed), but semantically it's similar to a broken link. "Please go to the url undefined".

The forbidden headers are mostly for security which "only" applies to browsers, that's true, but this is more of a way to handle actual semantic bugs, right?

triblondon commented 5 years ago

My use case is an HTTP client, so I want to be able to show the response that was received and report that the location header was missing. But right now we are losing all the data and just getting an exception.

colinbendell commented 4 years ago

+1 for @triblondon's use case. Like him, we are using fetch-h2 in one part as a triage tool to detect misbehaving servers. Escape valves like this are useful for the savvy client (though clearly not intended for the most common use cases)

triblondon commented 4 years ago

Hi @grantila when you have time could you give us some indication of whether you'd be willing to accept this or some other solution to this problem? Ie. let's start with: do you accept this use case as something fetch-h2 can support? If not, we can close the issue.

But if so, then I feel like the proposed PR is one possible option, and happy to discuss others.

grantila commented 4 years ago

Sorry for the delay @triblondon and @colinbendell, I agree with you.

I'm starting to think that fetch-h2 should not be Fetch API compliant by default, in that it should allow "illegal" headers, and it should allow this kind of "broken" response etc. To act more like browser fetch, the option {allowForbiddenHeaders: true|false} should be converted into a {compliance: 'strict'|'loose'} where loose is the default (not sure about the naming though).

This will make fetch-h2 more useful as an arbitrary http client by default.

It would however be a breaking change, but probably worth it.

triblondon commented 4 years ago

Agreed. If this would be the only breaking change you have on the horizon, you could leave it in compliance:strict mode by default. I'd be happy to throw in the extra param to turn it off. Then you could flip to loose-by-default on the next major release?

triblondon commented 1 year ago

@grantila should I close this? Wether or not the library provides some solution to the problem I was trying to address, it seems like it won't be this solution, right?