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.

Sign up for GitHub

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

Jump to bottom

Host parser UTF-8 decoding should fail, not U+FFFD #216

Merged
merged 2 commits into from
Jan 29, 2017
Merged

Host parser UTF-8 decoding should fail, not U+FFFD #216

merged 2 commits into from
Jan 29, 2017

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 24, 2017

Tests: …

Fixes #215.

@annevk
Copy link
Member Author

annevk commented Jan 24, 2017

Tests: web-platform-tests/wpt#4617.

@annevk annevk requested a review from domenic January 24, 2017 20:27
@domenic
Copy link
Member

domenic commented Jan 24, 2017

This appears to be unnecessary. ToASCII applied to something with replacement characters will generate an error result, because Processing step 1 ends up in "disallowed" for the replacement character.

@annevk
Copy link
Member Author

annevk commented Jan 25, 2017

@achristensen07 it seems your implementation of ToASCII might be buggy.

@domenic thanks, I'll update this PR to instead make a note that implementations could fail early if they desire.

@annevk
Copy link
Member Author

annevk commented Jan 25, 2017

I should probably add an explicit test for https://�/ since Chrome and Firefox (haven't tested Edge) still turn that into something. But in general we need more IDNA tests.

@jasnell jasnell mentioned this pull request Jan 25, 2017
Closed
@annevk
Copy link
Member Author

annevk commented Jan 27, 2017

Test for U+FFFD: web-platform-tests/wpt#4639. I also updated the PR to make it a note instead.

@annevk annevk merged commit cf9854c into master Jan 29, 2017
@annevk annevk deleted the annevk/fail-on-non-utf-8-hosts branch January 29, 2017 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@annevk @domenic