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

Fix parsing of IPv6 Addresses in vibe.inet.url #2308

Merged
merged 3 commits into from Jun 15, 2019

Conversation

@GallaFrancesco
Copy link
Contributor

commented May 15, 2019

The current implementation of URL.parse fails when provided with an IPv6 Address. Using the latest master branch:

import vibe.inet.url;

void main()
{
	auto url = URL.parse("localhost:8080");
	auto ipv6url = URL.parse("[1080:0:0:0:8:800:200C:417A]:8888");
}

The program gives the following result:

std.conv.ConvException@/opt/dmd-2.085/import/std/conv.d(1865): Unexpected ':' when converting from type string to type ushort

The code responsible for this is in url.d when the parser looks for the index of the first : occurrence. The presence of : inside IPv6 addresses is not taken into account, causing a failure when trying to convert the expected port to ushort.

The proposed fix consider both cases by setting the index accordingly.

Edit: this can also be seen here: https://run.dlang.io/is/SxRV9e

@GallaFrancesco GallaFrancesco force-pushed the GallaFrancesco:master branch from 94aa142 to 6975c66 May 15, 2019
@s-ludwig

This comment has been minimized.

Copy link
Member

commented May 16, 2019

The indexOf at that place is supposed to find the colon following the schema, with a schema present, IPv6 URLs should be parsed correctly (unit test at line 430).

But besides the question whether any kind of relative URL should be supported by URL (users would now potentially have to verify that they got an absolute URL), is [::1]:1/foo actually a valid URL? AFAIK, only these forms of relative URLs are allowed: //[::1]:1/foo, /foo and foo

@GallaFrancesco

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

While I agree that the [::1]:8080 might not be considered a valid URL (lack of schema, lack of path), my problem with this is the inconsistency of the parser when provided with IPv4 or IPv6 addresses:

  • IPv4: localhost:8080 is accepted (and 127.0.0.1:8080 is as well)
  • IPv6: [::1]:8080 is accepted while e.g. [2a00:1450:4013:c01::65]:8080 is not (by throwing the exception above), which is not in line with the expected behavior.

The actual issue with that is that when provided with such a URL the parser gets the parameters wrong without failing, e.g. in case of localhost:8080:

  • req.schema: "localhost"
  • req.host: "8080"
  • req.port: 0
  • req.path: ""

see: https://run.dlang.io/is/pCfsqr

It is true that this behavior could be detected by checking a posteriori wether e.g. url.schema == "http" || url.schema == "https" and so on, yet I don't see why this task should be passed on to the user (it could cause subtle bugs).

Therefore a sound approach could be to introduce a check which forbids the invalid host:port URL format for both IPv4 and IPv6 addresses and let the users make sure that a correct URL is provided, or to introduce a specific case for this type of URLs which assigns the parameters correctly.

The first approach seems safer while the second one is probably simpler to introduce since it allows for a corner case whithout requiring additional checks such as string traversing, looking for : or enforcing a particular URL format (which could break existing code).

@s-ludwig

This comment has been minimized.

Copy link
Member

commented May 19, 2019

The problem is that "localhost:8080" may actually be a valid URL, depending on how the "localhost" schema is defined. There are a number of schemas that don't have the "//" to denote the start of the hiearchical part of the URL.

As for actual IPv4 addresses, it would make sense to disallow "127.0.0.1:8080", as the schema must start with an alphabetical character.

But in any case, only "//[::1]:8080/" can be a valid URL - "[::1]:8080/" is just invalid AFAICS.

@GallaFrancesco

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I've added a check which doesn't allow schemas starting with non-alphabetic characters. This should also detect [::1]:8080/ as invalid, since it doesn't start with /.

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

Makes sense, I'll merge that for now. Would be nice to have something like #2311 on top to support parsing relative (and maybe also fuzzy) URLs.

@s-ludwig s-ludwig merged commit 09e7565 into vibe-d:master Jun 15, 2019
3 checks passed
3 checks passed
codecov/patch 50% of diff hit (target 36.067%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.