Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Bug Fix for https://github.com/joyent/node/issues/562 #1078

Closed
wants to merge 4 commits into from

Conversation

ryanpetrello
Copy link

Fixing a bug in url.parse: #562

The file:// protocol always has a hostname; it's frequently abbreviated as
an empty string, which represents 'localhost' implicitly.

According to RFC 1738 (http://tools.ietf.org/html/rfc1738):

A file URL takes the form:

file://<host>/<path>

where is the fully qualified domain name of the system on
which the is accessible...

As a special case, can be the string "localhost" or the empty
string; this is interpreted as `the machine from which the URL is
being interpreted'.

The file:// protocol *always* has a hostname; it's frequently abbreviated as
an empty string, which represents 'localhost' implicitly.

According to RFC 1738 (http://tools.ietf.org/html/rfc1738):

A file URL takes the form:

   file://<host>/<path>

where <host> is the fully qualified domain name of the system on
which the <path> is accessible...

As a special case, <host> can be the string "localhost" or the empty
string; this is interpreted as `the machine from which the URL is
being interpreted'.
@ry
Copy link

ry commented May 20, 2011

@isaacs, please review

@isaacs
Copy link

isaacs commented May 20, 2011

The change to url.js looks good to me. That bit of file-url incorrectness was to work around an issue that has since been refactored away.

Regarding 34b29b8: Please do not modify/remove an existing unit test (unless you believe that the behavior it verifies is actually incorrect.) Just create a new item in the list. (Or two, one with a trailing slash, and one without.)

Also, lest we be tempted to assign magic to "localhost", maybe also add a copy of those tests using "foo" as the hostname instead of "localhost".

@ryanpetrello
Copy link
Author

Done and done. Let me know if you need anything else.

@isaacs
Copy link

isaacs commented May 20, 2011

LGTM. Can you please sign the CLA at http://nodejs.org/cla.html? (You can just fill out the form online.)

@ryanpetrello
Copy link
Author

Working on getting contribution details from my employer. Thanks.

@ryanpetrello
Copy link
Author

@isaacs. Alright, I've signed the CLA. Sorry for the delay :).

isaacs pushed a commit to isaacs/node-v0.x-archive that referenced this pull request May 27, 2011
The file:// protocol *always* has a hostname; it's frequently
abbreviated as an empty string, which represents 'localhost'
implicitly.

According to RFC 1738 (http://tools.ietf.org/html/rfc1738):

A file URL takes the form:

   file://<host>/<path>

where <host> is the fully qualified domain name of the system on
which the <path> is accessible...

As a special case, <host> can be the string "localhost" or the empty
string; this is interpreted as 'the machine from which the URL is
being interpreted'.
@isaacs isaacs closed this in 58a1d7e May 27, 2011
@isaacs
Copy link

isaacs commented May 27, 2011

Landed on 58a1d7e.

Thanks!

@shaneosullivan
Copy link

Is there an ETA of when this will be in a new release? It would be great to get my hands on it.

@ryanpetrello
Copy link
Author

Also interested - I've got some feature development waiting on this bug fix :).

@ryanpetrello
Copy link
Author

Looks like this landed on http://blog.nodejs.org/2011/06/29/node-v0-4-9/.

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants