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

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants

Fixing a bug in url.parse: joyent#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'.

ryanpetrello added some commits May 20, 2011

@ryanpetrello ryanpetrello Fixing a bug in url.parse: nodejs#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 <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'.
2c96e2d
@ryanpetrello ryanpetrello Improving a unit test for url.parse. 34b29b8

ry commented May 20, 2011

@isaacs, please review

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".

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

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.)

Working on getting contribution details from my employer. Thanks.

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

@isaacs isaacs added a commit to isaacs/node-v0.x-archive that referenced this pull request May 27, 2011

@ryanpetrello @isaacs ryanpetrello + isaacs Close #562 Close #1078 Parse file:// urls properly
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'.
4e66ff5

isaacs closed this in 58a1d7e May 27, 2011

isaacs commented May 27, 2011

Landed on 58a1d7e.

Thanks!

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

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

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment