Don’t remove leading dots in domain names #171

SimonSapin · 2016-02-22T16:42:17Z

Fixes #166, closes #170.

http://www.unicode.org/reports/tr46/#ToASCII has algorithms like:

Break the result [a domain] into labels at U+002E FULL STOP.

[…]

join the labels using U+002E FULL STOP as a separator

Before this PR, the IDNA code does "join" with code like:

     let mut result = String::new();
     for label in /* ... */ {
         if result.len() > 0 {
             result.push('.');
         }
         result.push_str(/* ... */)
     }

This writes a dot before every label, except the first. But this code seems to be buggy: if all labels so far are empty, it won’t push dots to separate empty labels. In other words, leading dots are stripped.

I’ve fixed this apparent bug by using a boolean, but a number of tests started failing. They apparently expect leading dots to be stripped. I don’t see any thing in the spec to support this behavior.

@valenting did you mean to strip leading dots in this code? Am I missing something in the spec?

valenting · 2016-02-22T17:19:19Z

I don't recall if my implementation happened to match the unit tests, or if I changed the implementation to match the unit tests.
But I do agree that the spec doesn't specify this, and your patch makes more sense.
We should raise this issue with the unicode group, to get the unit tests fixed.

bors-servo · 2016-02-26T14:33:58Z

☔ The latest upstream changes (presumably #172) made this pull request unmergeable. Please resolve the merge conflicts.

valenting · 2016-03-03T19:31:42Z

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

a discussion (no related file):
Sorry I never completed this review. As I said, this makes sense, and I think we should land it.

tests/tests.rs, line 119 [r1] (raw file):
Interestingly, not how Firefox parses this URL :)

Comments from the review on Reviewable.io

Manishearth · 2016-12-18T19:53:03Z

Just realized that this is the same as #256 .

Both chrome and firefox do not strip any dots, and the spec agrees with this. We should land either this or #256

We're not running the IDNA tests on CI, btw, so we already have a bunch that fail.

The tests should be changed and also CId.

Manishearth · 2016-12-18T19:54:16Z

But I do agree that the spec doesn't specify this

The spec says to "join using full stops", and the "join" operation usually means putting the given delimeter between the segments, even if they are empty. ["","",""].join('.') == ".."

SimonSapin · 2016-12-19T17:48:07Z

What bothers me about this change, and why this PR isn’t merged yet, is that it makes some IDNA tests fail. It looks like these tests might be incorrect? Still, just commenting them out (as done in the PR at the moment) feels wrong. But the Unicode Technical Committee has been completely unresponsive to reports of spec issues so far, so I don’t know what to do here.

Manishearth · 2016-12-19T17:50:09Z

The tests are wrong -- they don't agree with the spec or the implementations.

Manishearth · 2016-12-19T18:02:47Z

Anyway, r=me on this change

dtolnay · 2017-05-12T05:03:04Z

This is still waiting for a response from the Unicode Technical Committee right?

A retry of servo#171 This diff changes the behavior of ToASCII step to match the spec and prevent failures on some cases when a domain name starts with leading dots (FULL STOPs), as requested in servo#166. The change in the code results in a few failures for test cases of the Conformance Testing data provided with UTS servo#46. But, as the header of the test data file (IdnaTest.txt) says: "If the file does not indicate an error, then the implementation must either have an error, or must have a matching result." Therefore, failing on those test cases does not break conformance with UTS servo#46, and to some level, anticipated. As mentioned in servo#166, a feedback is submitted for this inconsistency and the test logic can be improved later if the data file addresses the comments. Until then, we can throw less errors and maintain passing conformance tests with this diff.

A retry of servo#171 This diff changes the behavior of ToASCII step to match the spec and prevent failures on some cases when a domain name starts with leading dots (FULL STOPs), as requested in servo#166. The change in the code results in a few failures for test cases of the Conformance Testing data provided with UTS servo#46. But, as the header of the test data file (IdnaTest.txt) says: "If the file does not indicate an error, then the implementation must either have an error, or must have a matching result." Therefore, failing on those test cases does not break conformance with UTS servo#46, and to some level, anticipated. As mentioned in servo#166, a feedback is submitted for this inconsistency and the test logic can be improved later if the data file addresses the comments. Until then, we can throw less errors and maintain passing conformance tests with this diff. To keep the side-effects of ignoring errors during test runs as minimum as possible, I have separated `TooShortForDns` error from `TooLongForDns`. The `Error` struct has been kept private, so the change won't affect any library users.

A retry of servo#171 This diff changes the behavior of ToASCII step to match the spec and prevent failures on some cases when a domain name starts with leading dots (FULL STOPs), as requested in servo#166. The change in the code results in a few failures for test cases of the Conformance Testing data provided with UTS servo#46. But, as the header of the test data file (IdnaTest.txt) says: "If the file does not indicate an error, then the implementation must either have an error, or must have a matching result." Therefore, failing on those test cases does not break conformance with UTS servo#46, and to some level, anticipated. As mentioned in servo#166, a feedback is submitted for this inconsistency and the test logic can be improved later if the data file addresses the comments. Until then, we can throw less errors and maintain passing conformance tests with this diff. To keep the side-effects of ignoring errors during test runs as minimum as possible, I have separated `TooShortForDns` error from `TooLongForDns`. The `Error` struct has been kept private, so the change won't affect any library users. Fix servo#166

behnam · 2017-05-12T21:44:19Z

I have submitted a new PR, based on this, which also has a fix for the conformance tests: #337

About the spec and test data issues, please see discussion on the issue: #166

With those, I recommend closing this old PR.

A retry of servo#171 This diff changes the behavior of ToASCII step to match the spec and prevent failures on some cases when a domain name starts with leading dots (FULL STOPs), as requested in servo#166. The change in the code results in a few failures for test cases of the Conformance Testing data provided with UTS servo#46. But, as the header of the test data file (IdnaTest.txt) says: "If the file does not indicate an error, then the implementation must either have an error, or must have a matching result." Therefore, failing on those test cases does not break conformance with UTS servo#46, and to some level, anticipated. As mentioned in servo#166, a feedback is submitted for this inconsistency and the test logic can be improved later if the data file addresses the comments. Until then, we can throw less errors and maintain passing conformance tests with this diff. To keep the side-effects of ignoring errors during test runs as minimum as possible, I have separated `TooShortForDns` error from `TooLongForDns`. The `Error` struct has been kept private, so the change won't affect any library users. Fix servo#166

Hoverbear · 2017-05-17T11:41:25Z

Closing this as requested.

A retry of servo#171 This diff changes the behavior of ToASCII step to match the spec and prevent failures on some cases when a domain name starts with leading dots (FULL STOPs), as requested in servo#166. The change in the code results in a few failures for test cases of the Conformance Testing data provided with UTS servo#46. But, as the header of the test data file (IdnaTest.txt) says: "If the file does not indicate an error, then the implementation must either have an error, or must have a matching result." Therefore, failing on those test cases does not break conformance with UTS servo#46, and to some level, anticipated. As mentioned in servo#166, a feedback is submitted for this inconsistency and the test logic can be improved later if the data file addresses the comments. Until then, we can throw less errors and maintain passing conformance tests with this diff. To keep the side-effects of ignoring errors during test runs as minimum as possible, I have separated `TooShortForDns` error from `TooLongForDns`. The `Error` struct has been kept private, so the change won't affect any library users. Fix servo#166

[idna] Update data to Unicode 10.0 and fix logic * Change the behavior of ToASCII step to match the spec and prevent failures on some cases when a domain name starts with leading dots (FULL STOPs), as requested in #166. (Another attempt on #337 and #171) * Update `IdnaTest.txt` file to UCD 10.0 and fix Validation Rules, specially Bidi Rules, for the tests to pass. * Add TODO marks for new flags introduced in Unicode 10.0 version of UTS#46. (http://www.unicode.org/reports/tr46/proposed.html) * Add integration test for `rust-url` crate for the new behavior. Fix #166  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/351)

Don’t remove leading dots in domain names

9d0fd21

This was referenced Feb 22, 2016

Do not panic when parsing '.' in URL host: #166 #170

Closed

Panic when parsing a . in file URLs #166

Closed

This was referenced Dec 19, 2016

"https://../foo" should not lose the dots when parsed #255

Closed

Handle empty domain segments correctly in idna #256

Closed

Manishearth added this to the rust-url oxidation milestone Dec 19, 2016

behnam mentioned this pull request May 12, 2017

[idna] Preserve leading dots in host #337

Closed

Hoverbear closed this May 17, 2017

behnam mentioned this pull request May 29, 2017

[idna] Update data to Unicode 10.0 and fix logic #351

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don’t remove leading dots in domain names #171

Don’t remove leading dots in domain names #171

SimonSapin commented Feb 22, 2016

valenting commented Feb 22, 2016

bors-servo commented Feb 26, 2016

valenting commented Mar 3, 2016

Manishearth commented Dec 18, 2016 •

edited

Loading

Manishearth commented Dec 18, 2016

SimonSapin commented Dec 19, 2016 •

edited

Loading

Manishearth commented Dec 19, 2016 •

edited

Loading

Manishearth commented Dec 19, 2016

dtolnay commented May 12, 2017

behnam commented May 12, 2017

Hoverbear commented May 17, 2017

Don’t remove leading dots in domain names #171

Don’t remove leading dots in domain names #171

Conversation

SimonSapin commented Feb 22, 2016

valenting commented Feb 22, 2016

bors-servo commented Feb 26, 2016

valenting commented Mar 3, 2016

Manishearth commented Dec 18, 2016 • edited Loading

Manishearth commented Dec 18, 2016

SimonSapin commented Dec 19, 2016 • edited Loading

Manishearth commented Dec 19, 2016 • edited Loading

Manishearth commented Dec 19, 2016

dtolnay commented May 12, 2017

behnam commented May 12, 2017

Hoverbear commented May 17, 2017

Manishearth commented Dec 18, 2016 •

edited

Loading

SimonSapin commented Dec 19, 2016 •

edited

Loading

Manishearth commented Dec 19, 2016 •

edited

Loading