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

Function to get the redirect URL from the Location header is invalid #2783

Closed
linkdd opened this issue May 14, 2024 · 2 comments · Fixed by #2793
Closed

Function to get the redirect URL from the Location header is invalid #2783

linkdd opened this issue May 14, 2024 · 2 comments · Fixed by #2793
Labels
bug Something isn't working
Milestone

Comments

@linkdd
Copy link
Contributor

linkdd commented May 14, 2024

What is the current bug behavior?

When doing a redirection, hurl use the Location header, which can either be a relative URL or an absolute URL.

However the function to compute the redirect URL is wrong, it assumes only strings starting with / are relative URLs.

Steps to reproduce

A GET / request to an OpenObserve instance returns a response with status code 307 and Location: ./web/. Hurl catches this as an "absolute URL" and then fails to resolve the domain name . (obviously).

But we could imagine other cases:

  • a GET /foo could return a Location: ../bar
  • a GET / (via http) could return a Location: https://... (why not ftp://, or gopher://, those are valid URLs as well)

The Location URL could also be //hostname/path (scheme relative URL).

What is the expected correct behavior?

If we take the initial absolute URL of the request, and join it with the value of the Location header, we should get the absolute redirect URL

Execution context

  • Hurl Version (hurl --version): 4.3.0

Possible fixes

Related function:

/// Returns the redirect url.
fn get_redirect_url(location: &str, base_url: &str) -> String {
if location.starts_with('/') {
format!("{base_url}{location}")
} else {
location.to_string()
}
}

Example of using the crate url to join the Location header:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=89982618796837fd896caa33eb45d1bb

@linkdd linkdd added the bug Something isn't working label May 14, 2024
@jcamiel jcamiel added this to the 5.0.0 milestone May 14, 2024
@jcamiel
Copy link
Collaborator

jcamiel commented May 16, 2024

Thanks a lot for the detailed report @linkdd the current code to deal with URL redirections was overly naive!

We've improved it with a proper Url struct (a wrapper on url crate), your snippets has help a lot!
Would you be kind to test the new version once the PR has been merged (if you can of course)?
Thanks,

(a small note: I've keep some logic on rejecting other scheme than http/https. I'll discuss with the others maintainers to see if we keep it)

@linkdd
Copy link
Contributor Author

linkdd commented Jun 4, 2024

Sorry, I was on vacation :)

I just tested, works well, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants