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

Fixes #476: Correctly handle URI which contains hostname #477

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeroenvervaeke
Copy link

See detailed explanation in issue #476.

Description

This code fixes #476.

The code is now correctly handling URLs like http://example.com/favicon.ico.

Previously, because Uri::builder() .path_and_query(p) was used uri.host() would be none and path_and_query would be /http://example.com/favicon.ico.

This PR fixes that issue.
After my fix the host is now example.com and the path_and_query are /favicon.ico.

I've added the \\ normalization to fix the test_single_header and test_multiple_header unit tests.

@drcaramelsyrup
Copy link
Contributor

Sorry for the delay, and thanks! It has been a known issue that the HTTP/1.1 absolute-form is not well-supported, and pingora generally assumes that we're working with the far more common origin-form.

The main issue that I see is that we really want to preserve the original form of the input uri as much as possible (the raw_path() exists as well in case the URI is not valid utf-8, but regardless aside from that we don't expect additional transformations). Is the backslash normalization required as part of using Uri::from_maybe_shared?

My other concern is that there are places in the pingora_proxy code that make the origin-form assumption, such as the way we update the h2 authority for proxying. While the origin-form doesn't seem to be breaking since the existing integration tests are passing, I wonder if the absolute-form is proxied correctly, and so it would be ideal if we could add basic proxying integration tests to h1/h2 upstreams for a client using absolute-form.

@drcaramelsyrup drcaramelsyrup added bug Something isn't working enhancement New feature or request labels Dec 6, 2024
@jeroenvervaeke
Copy link
Author

Thanks for the feedback, I'll take a look when I have some more time.

Yes, the backslash normalization is a requirement as part of using Uri::from_maybe_shared.
I've added the backslash normalization because of this test:

let mut req = RequestHeader::build("GET", b"\\", None).unwrap();

I'm not sure if \ should pass to be honest, I can't find it anywhere in the spec.
Is this a typo in the unit test? Or am I missing something.

In case this is a typo in the unit test we can safely get rid of the backslash normalization

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

Successfully merging this pull request may close these issues.

pingora-http incorrectly handling URI with host
3 participants