Skip to content

Conversation

@jamesqh
Copy link
Contributor

@jamesqh jamesqh commented Jun 19, 2024

I tried to implement the redirect from the TLS examples in my project, and it didn't work - I like to use port 6789 when I'm testing things locally and this caused big problems. So I thought I'd try fixing it and filing a PR.

tl;dr: The redirect showcased in the TLS examples is very fragile, I am proposing a more robust alternative.

Motivation:

The HTTP->HTTPS redirect showcased in these examples changes the port with a simple find-and-replace over the entire authority, including the hostname. This works well in the median case but:

  • Custom HTTPS ports with default HTTP ports fail to redirect properly, as the HTTP port is not present in the string: http://example.com -> https://example.com (not https://example.com:4443 or whatever port you're using.) This is the one that bit me!

  • IP addresses can be mangled: http://80.80.80.80 -> https://443.443.443.443, http://[::80] -> https://[::443]

  • As, I think worst of all, can hostnames, like this real and very cool website: http://io808.com -> https://io4438.com !

Solution:

I've written an alternative based on parsing into an Authority; this type's .port() should guarantee with a None return that the authority string is a bare hostname/IP, or with Some(port) that the string ends with :{port} and so we can safely strip that suffix. Adding new port only conditionally would be easy, but browsers tend to be smart about eliding unnecessary :443s, so it seemed an overcomplication?

I also changed the args, since this redirect solution doesn't actually need to know the HTTP port, and since clippy's suggestion to use an &str seemed reasonable.

I'm not very expert at Rust, so hopefully my solution doesn't have any glaring problems! It may be better to simply use the url crate, as was proposed (and rejected) previously, but it was more fun for me to write this.

@jamesqh jamesqh force-pushed the main branch 4 times, most recently from dbdda72 to 9242b9d Compare June 19, 2024 20:37
Copy link
Collaborator

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while I'm not a huge fan of the unwrap() calls, this at least looks less error prone as before. IMHO this seems good to merge.

@Turbo87 Turbo87 requested a review from jplatte December 27, 2024 13:16
@jplatte jplatte merged commit 3497e5d into tokio-rs:main Dec 27, 2024
18 checks passed
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

Successfully merging this pull request may close these issues.

3 participants