-
Notifications
You must be signed in to change notification settings - Fork 475
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
Conversation
src/URI.js
Outdated
@@ -575,6 +575,10 @@ | |||
string = '/' + string; | |||
} | |||
|
|||
if (parts.port) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
src/URI.js
Outdated
@@ -524,6 +524,8 @@ | |||
// what's left must be the path | |||
parts.path = string; | |||
|
|||
URI.basicValidation(parts); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/URI.js
Outdated
@@ -575,6 +575,10 @@ | |||
string = '/' + string; | |||
} | |||
|
|||
if (parts.port) { |
There was a problem hiding this comment.
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 :)
src/URI.js
Outdated
} | ||
}; | ||
|
||
URI.basicValidation = function(parts) { |
There was a problem hiding this comment.
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()
?
There was a problem hiding this comment.
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.
src/URI.js
Outdated
var hasHostname = !parts.hostname; | ||
|
||
if (isHttpOrHttps && !hasHostname) { | ||
throw new TypeError('Hostname cannot be empty, if protocol is http(s)'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This makes the behaviour consistent with port().
Looks like rewording/editing/amending commits isn't a good idea for implementing requested changes :( It's probably better to add new commits and do the squashing/merging when it's ready to merge? |
src/URI.js
Outdated
} | ||
}; | ||
|
||
URI.basicValidation = function(parts) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
src/URI.js
Outdated
} | ||
}; | ||
|
||
URI.ensureValidPort = function(v) { |
There was a problem hiding this comment.
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');
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Protocols listed in this array will require the user to specify a hostname. Oherwise URI.js' factory/constructor will throw a TypeError.
This reverts commit 0657ad8.
thanks for your effort! released as v1.18.11 :) |
Hey @konstantinblaesi, since the introduction of hostname verification in v1.18.11 we've seen a few compatibility problems: #352, #354, #355. I'm considering the rollback of your change - unless you have a better idea? |
As the hostname validation caused some kerfuffle I've decided to make this feature opt-in, thereby returning to the previous default behavior of v1.18.10. As of v1.19.0 you can activate hostname validation globally by setting I'm sorry for the confusion, inconvenience caused by the original change and the delayed rectification. |
This makes the behaviour consistent with port().
Fixes #344