Skip to content

Allow percent encoding in URI host - #528

Open
paullgdc wants to merge 1 commit into
hyperium:masterfrom
paullgdc:paullgdc/allow_pct_encoding_in_host
Open

Allow percent encoding in URI host#528
paullgdc wants to merge 1 commit into
hyperium:masterfrom
paullgdc:paullgdc/allow_pct_encoding_in_host

Conversation

@paullgdc

@paullgdc paullgdc commented Mar 1, 2022

Copy link
Copy Markdown

According to RFC 3986 section 3.2.2 https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.2 , the host section of the authority of an url is allowed to carry percent-encoded characters.

The parsing code only allowed % in ip adresses, and userinfo, which is both needlessly restrictive, and allows some invalid urls if there isn't two hex characters after the percent character.

This PR allows percent encoding everywhere in the host, and checks if the percent-encoding is valid.

This is useful for instance to implements Unix domain sockets URI, that would look like unix://<percent encoded socket path>/<http request path>

@paullgdc paullgdc changed the title Allow percent encoding in host Allow percent encoding in URI host Mar 1, 2022
Comment thread src/uri/authority.rs
b'[' => {
if has_percent || start_bracket {
if start_bracket {
// Something other than the userinfo has a `%`, so reject it.

@robjtede robjtede Apr 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

comment is not accurate any more

@robjtede

robjtede commented May 1, 2022

Copy link
Copy Markdown

Torn on whether this is a good thing for the crate, here's some facts though:

Given these, and without requiring an up to date public suffix list referenced by the WHATWG standard, it feels like allowing percent encoding as this PR does would be largely standard compliant.

Edit: having some doubts after reading more (research, curl PR, curl mailing list), it seems we'd need to actually do the percent decoding (sometimes?) to prevent this feature turning into a security risk

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.

2 participants