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

coro-http does not resolve relative paths #322

Open
Bilal2453 opened this issue Jul 21, 2023 · 3 comments
Open

coro-http does not resolve relative paths #322

Bilal2453 opened this issue Jul 21, 2023 · 3 comments

Comments

@Bilal2453
Copy link
Contributor

Bilal2453 commented Jul 21, 2023

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
Copy link
Contributor Author

Bilal2453 commented Jul 21, 2023

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
Copy link

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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants