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

Explain why valid domain needs to run ToUnicode #817

Closed
hsivonen opened this issue Feb 2, 2024 · 5 comments · Fixed by #840
Closed

Explain why valid domain needs to run ToUnicode #817

hsivonen opened this issue Feb 2, 2024 · 5 comments · Fixed by #840
Labels
editorial Changes that do not affect how the standard is understood topic: idna

Comments

@hsivonen
Copy link
Member

hsivonen commented Feb 2, 2024

What is the issue with the URL Standard?

https://url.spec.whatwg.org/#valid-domain could use an informative note that states the implications of the two-step (both ToASCII and ToUnicode) check. Given the how both use UTS 46 "Processing" and "ToASCII" does more stuff after "Processing", it would be helpful to call out what the second run of "Processing" (as part of "ToUnicode") catches still.

@hsivonen hsivonen added topic: idna editorial Changes that do not affect how the standard is understood labels Feb 5, 2024
@hsivonen
Copy link
Member Author

hsivonen commented Mar 1, 2024

FWIW, after more progressi with writing code, I'm even more puzzled about what the second run of "Processing" is meant to catch here.

@annevk
Copy link
Member

annevk commented Mar 3, 2024

I wonder if the difference has disappeared over time. It does seem weird that ToUnicode can now fail apparently, but there's no explicit mention of this.

@zacknewman
Copy link

zacknewman commented Apr 25, 2024

Glad I saw this as I too am skeptical about the need to perform the domain-to-unicode algorithm. I've tried generating inputs that fail on step 3 using the below code in Rust using the idna crate, but I have been unable to find such an input:

use core::{ops::ControlFlow, str};
use idna::Config;
fn main() {
    match ('\0'..=char::MAX).try_fold(String::with_capacity(8), |mut input, c| {
        input.clear();
        input.push(c);
        if let Err(val) = idna_transform(input.as_str()) {
            println!("{val}");
            ControlFlow::Break(())
        } else {
            ControlFlow::Continue(input)
        }
    }) {
        ControlFlow::Continue(input) => {
            let mut utf8 = input.into_bytes();
            utf8.clear();
            utf8.extend_from_slice(b"xn--");
            punycode_inputs(&mut utf8, 0);
        }
        ControlFlow::Break(()) => (),
    }
}
fn punycode_inputs(utf8: &mut Vec<u8>, count: u8) -> bool {
    if count < 4 {
        for i in [
            b'-', b'0', b'1', b'2', b'3', b'4', b'5', b'6', b'7', b'8', b'9', b'a', b'b', b'c',
            b'd', b'e', b'f', b'g', b'h', b'i', b'j', b'k', b'l', b'm', b'n', b'o', b'p', b'q',
            b'r', b's', b't', b'u', b'v', b'w', b'x', b'y', b'z',
        ] {
            utf8.push(i);
            if let Err(val) =
                idna_transform(str::from_utf8(utf8.as_slice()).unwrap_or_else(|_| {
                    unreachable!("ASCII is a subset of UTF-8, so this is fine")
                }))
            {
                println!("{val}");
                return true;
            } else if punycode_inputs(utf8, count + 1) {
                return true;
            } else {
                utf8.pop();
            }
        }
    }
    false
}
fn idna_transform(input: &str) -> Result<(), &str> {
    idna::domain_to_ascii_strict(input).map_or_else(
        |_| Ok(()),
        |ascii| {
            Config::default()
                .use_std3_ascii_rules(true)
                .to_unicode(ascii.as_str())
                .1
                .map_err(|_e| input)
        },
    )
}

Consequently I believe steps 3 and 4 can be removed, but I haven't mathematically proven the domain-to-ascii algorithm is sufficient. I've used these examples as well.

annevk added a commit that referenced this issue Nov 29, 2024
I'm not entirely sure why this redundant check existed. Either because there was a difference back when this definition was introduced in 3bec3b8 or (more likely) I wasn't sure if there was a difference.

Fixes #817.
@annevk
Copy link
Member

annevk commented Nov 29, 2024

I've put up #840 to fix this. I'm somewhat curious why you all implemented the "valid domain" definition. At least within the web platform there's no known caller for it and it's really meant to be more of a syntax explanation as to how to write a domain.

@hsivonen
Copy link
Member Author

I'm somewhat curious why you all implemented the "valid domain" definition.

  1. The idna crate had configurability on the points that beStrict affects before my time.
  2. Being able to flip options closer to beStrict=true is useful for being able to use the upstream IdnaTestV2.txt test suite fully. (I say "closer", because prior to Unicode 16.0, the test suite assumed verifyDnsLength=true with the quirk of allowing the trailing dot.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Changes that do not affect how the standard is understood topic: idna
Development

Successfully merging a pull request may close this issue.

3 participants