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

Should http::Uri handle IDNA? #259

Open
KiChjang opened this issue Oct 6, 2018 · 9 comments
Open

Should http::Uri handle IDNA? #259

KiChjang opened this issue Oct 6, 2018 · 9 comments
Labels
A-uri Area: Uri and parts B-rfc Blocked: request for comments. More discussion would help move this along. S-feature Severity: feature. This adds something new.

Comments

@KiChjang
Copy link

KiChjang commented Oct 6, 2018

Or should it delegate it to the url crate somehow?

@seanmonstar
Copy link
Member

That's a good question. What would be involved if we were to say that this crate should handle it? What are the use cases? What should this crate do if it were decided to not handle IDNA?

I don't believe a server needs to handle this, as the recurved URI should have been encoded correctly. So is it mostly for clients?

@seanmonstar seanmonstar added S-feature Severity: feature. This adds something new. B-rfc Blocked: request for comments. More discussion would help move this along. A-uri Area: Uri and parts labels Oct 18, 2018
@troelsarvin
Copy link

See sagebind/isahc#381 and related sagebind/isahc#382 for why this is relevant.
I like to use isahc, but as it is, isahc's reliance on http::Uri has turned out to be a problem, and I probably need to convert URL strings into the url::Url and then further into http://Uri to be able to handle many perfect perfectly fine real-life URLs.

Non-ASCII domain names become more and more wide-spread, and for a good reason.

So I hope someone can find a good way to accept parse non-ASCII strings as Uris.

@Xuanwo
Copy link

Xuanwo commented Aug 19, 2022

I try to fix this issue via allow unicode in uri: #565

PTAL.

@troelsarvin
Copy link

@Xuanwo , it seems to me your pull request was closed but never accepted into the http crate, or am I misunderstanding Github?

@Xuanwo
Copy link

Xuanwo commented Apr 10, 2023

@Xuanwo , it seems to me your pull request was closed but never accepted into the http crate, or am I misunderstanding Github?

Yes. I gave up for working on fixing this issue any more. Feel free to reuse my work.

@micolous
Copy link

It looks like #565 didn't implement Nameprep (Unicode "lowercasing" and normalization), so that's not a complete implementation of IDN for some languages.

Implementing Nameprep means shipping a Unicode normalization table, which has significant impacts on binary size. Some platforms have ways of dealing with this (wasm32 targets can use JavaScript's Url object, Windows has its own IDN APIs and CoreFoundation has a CFURL type), but they all make things a bit complicated.

reqwest currently uses url:Url for its public APIs but http::Uri internally on non-wasm32 targets. Adding IDN support to http::Uri would mean that they no longer have to use url::Url, but they'd have to break API compatibility for non-wasm32 targets as well.

Having http::Request use a generic Url-like trait make things more flexible – on wasm32, one could use web_sys::Url which is much smaller than url::Url (with idna and unicode_normalization crates).

@kirawi
Copy link

kirawi commented Jun 17, 2024

@seanmonstar Would you be open to a PR for this?

@flisky
Copy link

flisky commented Jun 18, 2024

@kirawi actually, @micolous opened a PR servo/rust-url#887

@kirawi
Copy link

kirawi commented Jun 19, 2024

I don't think that's what this issue is about. AFAICT, this issue is about adding IDNA support for the Uri implementation in http. The url crate already implements it and that PR seems to be an optimization for webassembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-uri Area: Uri and parts B-rfc Blocked: request for comments. More discussion would help move this along. S-feature Severity: feature. This adds something new.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants