search

luvit / lit

Toolkit for developing, sharing, and running luvit/lua programs and libraries.
http://lit.luvit.io/
Apache License 2.0
245 stars 58 forks source link

coro-http does not resolve relative paths #322

Open Bilal2453 opened 1 year ago

Bilal2453 commented 1 year ago

RFC 7231 specify that when the client receives a redirection code 3xx (such as 302 or 307), the Location header can be provided to suggest the new URL for the request. It also specifies that the Location header could either contain an absolute URI reference, or a relative one.

A Location header of the absolute form would look like this https://example.com/redirection/path?query=foo#fragment, this is the effective URI to use for the redirected request.

Lets assume you make a GET request to the previous URI, from which you receive a 307 code with the Location header ./../another_path, in this case the mentioned RFC section 7.1.2 and RFC 3986 expect you to compute the effective new URI of the new request using the specified algorithm, in which the effective URI will be https://example.com/redirection/another_path?query=foo#fragment.

The Location header could also be a full one, in that it includes the host, but the path can still be relative, such as https://example.com/redirection/../another_path and it would produce the same effective URI.

Currently, coro-http does automatic redirections without ever accounting for this type of Location header, even though it is required by specification to, automatically making your request... fail.

From what I have noticed, this is not an uncommon problem either, the Microsoft API would often use this type of Location headers, so do many services!

I have been working on a fix for this but I didn't realize that all paths needed to be resolved. And frankly I believe I am re-implementing logic that is already implemented in Luvit modules, such as the url.parse (the current coro_http.parseUrl is not compliant) and parts of the path for relative paths.

But then, it seems to me we won't be able to depend on url.parse as that would be a breaking change. I am not sure what decision to make here regarding this situation.

Bilal2453 commented 1 year ago

If anyone wants to see the progress I have made, here https://github.com/Bilal2453/lit/blob/corohttp-redirect-fix/deps/coro-http.lua. (it is not currently in an intended to work state)

TohruMKDM commented 1 year ago

An additional issue I have observed with coro_http.parseUrl is that it assumes the path component in a url cannot be empty despite RFC 3986 Section 3.3 specifying that the path must be either empty or begin with "/". This issue causes parseUrl to incorrectly handle valid urls such as https://url.com?foo=bar

Bilal2453 commented 1 year ago

Worth mentioning then: it does not handle other parts of the specification from what I can see, no Authority (section 3.2), no hash, for example. Those are handled in the Luvit's implementation.