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

Make constructor throw on invalid ports. #345

Merged
merged 6 commits into from Aug 8, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 52 additions & 6 deletions src/URI.js
Expand Up @@ -243,7 +243,7 @@
// allowed hostname characters according to RFC 3986
// ALPHA DIGIT "-" "." "_" "~" "!" "$" "&" "'" "(" ")" "*" "+" "," ";" "=" %encoded
// I've never seen a (non-IDN) hostname other than: ALPHA DIGIT . -
URI.invalid_hostname_characters = /[^a-zA-Z0-9\.-]/;
URI.invalid_hostname_characters = /[^a-zA-Z0-9\.\-:]/;
// map DOM Elements to their URI attribute
URI.domAttributes = {
'a': 'href',
Expand Down Expand Up @@ -524,6 +524,8 @@
// what's left must be the path
parts.path = string;

URI.basicValidation(parts);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation should probably be done in URI.parseHost()? maybe URI#hostname()'s validation can be moved there, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created and called the method this way because I wanted to put some validation there that isn't just about a specific part of the url but about a combination such as protocol, hostname, path.


// and we're done
return parts;
};
Expand Down Expand Up @@ -575,6 +577,14 @@
string = '/' + string;
}

if (parts.hostname) {
URI.ensureValidHostname(parts.hostname);
}

if (parts.port) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change should be covered by a test for the constructor (test/test.js, line 132)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a negative and a positive test, one for each case. Is that enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a positive test case, there are plenty of those :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've removed the positive testcase, improved the port validation code and 3 negative tests in total.

URI.ensureValidPort(parts.port);
}

return string.substring(pos) || '/';
};
URI.parseAuthority = function(string, parts) {
Expand Down Expand Up @@ -1024,11 +1034,41 @@
if (!punycode) {
throw new TypeError('Hostname "' + v + '" contains characters other than [A-Z0-9.-] and Punycode.js is not available');
}

if (punycode.toASCII(v).match(URI.invalid_hostname_characters)) {
throw new TypeError('Hostname "' + v + '" contains characters other than [A-Z0-9.-]');
throw new TypeError('Hostname "' + v + '" contains characters other than [A-Z0-9.:-]');
}
}
};

URI.ensureValidPort = function(v) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be simplified to

URI.ensureValidPort = function(v) {
  if (!v) {
    return;
  }

  var port = Number(v);
  if (Number.isInteger(port) && (port > 0) && (port < 65536)) {
    return;
  }

  throw new TypeError('Port "' + v + '" is not a valid port');
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

var valid = false;

if (!v || !v.length) {
// no custom port specified
valid = true;
} else {
var port = Number(v);

// verify type and range
if (Number.isInteger(port) && (port > 0) && (port < 65536)
) {
valid = true;
}
}

if (!valid) {
throw new TypeError('Port "' + v + '" is not a valid port');
}
};

URI.basicValidation = function(parts) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be called ensureValidHostname()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it here because I just wanted to prevent "http:///" being a valid input for the constructor. Since I am not only checking the hostname, but also protocol and path I extracted it into it's own function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not limited to HTTP/S, but also applies to ssh, ftp, and many more. Maybe we should create URI.hostProtocols analoguous to URI.defaultPorts? The name of this function should be similar to the property, so URI.ensureHostProtocol() or something like that? either way, basic is not a thing in URLs, so the term is really irritating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting to reject all URLs of the form protocol:/// no matter what the protocol is? Would you use URI.hostProtocols to store different regexes for the validation of the individual protocols?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay… no, I'm suggesting putting the protocols for which we require a hostname into a map on URI, like it's done for URI.defaultPorts. That would allow anyone to easily add the protocols they want validated the same way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodneyrehm What do you think, is this ready to merge?

var hasProtocol = !!parts.protocol; // not null and not empty an empty string
var isHttpOrHttps = hasProtocol && (parts.protocol.indexOf('http') !== -1);
var hasHostname = !!parts.hostname; // not null and not an empty string

if (isHttpOrHttps && !hasHostname) {
throw new TypeError('Hostname cannot be empty, if protocol is http(s)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is true of any protocol, even the empty protocol ://hostname/path

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code, because apparently my single negation was wrong anyways.

}
};

// noConflict
Expand Down Expand Up @@ -1288,9 +1328,7 @@
v = v.substring(1);
}

if (v.match(/[^0-9]/)) {
throw new TypeError('Port "' + v + '" contains characters other than [0-9]');
}
URI.ensureValidPort(v);
}
}
return _port.call(this, v, build);
Expand Down Expand Up @@ -1426,6 +1464,10 @@
v += '.';
}

if (v.indexOf(':') !== -1) {
throw new TypeError('Domains cannot contain colons');
}

if (v) {
URI.ensureValidHostname(v);
}
Expand Down Expand Up @@ -1466,6 +1508,10 @@
throw new TypeError('cannot set domain empty');
}

if (v.indexOf(':') !== -1) {
throw new TypeError('Domains cannot contain colons');
}

URI.ensureValidHostname(v);

if (!this._parts.hostname || this.is('IP')) {
Expand Down
25 changes: 20 additions & 5 deletions test/test.js
Expand Up @@ -124,6 +124,26 @@
ok(u instanceof URI, 'instanceof URI');
ok(u._parts.hostname !== undefined, 'host undefined');
});
test('function URI(string) with invalid port "port" throws', function () {
raises(function () {
new URI('http://example.org:port');
}, TypeError, "throws TypeError");
});
test('function URI(string) with invalid port "0" throws', function () {
raises(function () {
new URI('http://example.org:0');
}, TypeError, "throws TypeError");
});
test('function URI(string) with invalid port "65536" throws', function () {
raises(function () {
new URI('http://example.org:65536');
}, TypeError, "throws TypeError");
});
test('function URI(string) with protocol and without hostname should throw', function () {
raises(function () {
new URI('http://');
}, TypeError, "throws TypeError");
});
test('new URI(string, string)', function() {
// see http://dvcs.w3.org/hg/url/raw-file/tip/Overview.html#constructor
var u = new URI('../foobar.html', 'http://example.org/hello/world.html');
Expand Down Expand Up @@ -1338,11 +1358,6 @@
url: 'file:///C:/skyclan/snipkit',
base: 'http://example.com/foo/bar',
result: 'file:///C:/skyclan/snipkit'
}, {
name: 'absolute passthru - generic empty-hostname - urljoin (#328)',
url: 'http:///foo',
base: 'http://example.com/foo/bar',
result: 'http:///foo'
}, {
name: 'file paths - urljoin',
url: 'anotherFile',
Expand Down