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

Add test case for ambiguous ipv6 #466

Closed
wants to merge 1 commit into from
Closed

Conversation

@jedahan
Copy link

jedahan commented Oct 30, 2018

I don't think this should be merged, because CI will fail, but it would be nice to start the conversation if this truly should be an error. Motivation is reducing false positives in alacritty/alacritty#1727 .

If we determine it should be an error, I'd like to tackle fixing it.


This change is Reviewable

@SimonSapin
Copy link
Member

SimonSapin commented Oct 31, 2018

The parsing algorithm that we implement only looks for an IPv6 address if it’s enclosed in [ and ] brakets: https://url.spec.whatwg.org/#host-parsing

@SimonSapin
Copy link
Member

SimonSapin commented Oct 31, 2018

Looking more closely, I think this is not related to IPv6 at all. The f::some::ambiguous::ipv6 input is parsed (IIRC) as an URL if the f scheme/protocol with a :some::ambiguous::ipv6 "path".

The parser is very flexible and accepts unknown protocols. For example, magnet:?xt=urn:btih:c12fe1c06bba254a9dc9f519b335aa7c1367a88a is an URL that some real applications know how to deal with. rust-url can parse it correctly but doesn’t know anything about magnet specifically.

This parser is designed to parse strings that are already delimited by some other markers, for example the value of the href attribute of a <a> HTML element. In plain text when trying to guess what substrings look like a "valid" URL you probably a different test of validity. Both more restrictive, possibly as much as only allowing http: and https: rather than arbitrary schemes; and more permissive, for example recognizing github.com as implicitly in HTTP.

@valenting
Copy link
Collaborator

valenting commented Dec 5, 2018

Also, this test shouldn't be in urltestdata.json.
I assume we want to track web-platform-tests as closely as possible, in which case this should either be submitted to that repo, or converted to a regular rust unit test.
But I agree with Simon, I think current parsing behaviour for f::some::ambiguous::ipv6 is the correct one.

@nox
Copy link
Member

nox commented Jul 18, 2019

The original alacritty issue was closed, I don't think we should land this anyway, closing.

@nox nox closed this Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.