Tighten hostname validation in HostInfo constructor#470
Tighten hostname validation in HostInfo constructor#470
Conversation
Replace lenient regex with stricter validation that properly handles: - IPv6 addresses (hex digits and colons) - IPv4 addresses (digits with dot separators) - DNS names (labels must start/end with alphanumeric, hyphens mid-label only) Rejects invalid patterns like leading/trailing dots, consecutive dots, and labels starting or ending with hyphens.
unprolix
force-pushed
the
jjb-stricter-hostname-validation
branch
from
ac92bf4 to
720f93e
Compare
src/net-util/export/HostUtil.js
Outdated
| } | ||
|
|
||
| // Reject bracketed IPv6 addresses. The EndpointAddress canonicalizer | ||
| // accepts them, but HostInfo callers should provide bracket-free addresses. |
There was a problem hiding this comment.
This comment refers to HostInfo, but we're in HostUtil here, whose general behavior is to accept bracketed IPv6 addresses. But also, I think it's actually okay to accept bracketed IPv6 addresses in the HostInfo constructor (though I am open to opinions to the contrary).
There was a problem hiding this comment.
i believe i have addressed all your comments in the current revision of this PR. (if not then plmk!)
src/net-util/export/HostUtil.js
Outdated
| /** | ||
| * Validates that the given value is a valid hostname string, returning it if | ||
| * so. This accepts both DNS names and IP addresses (but not wildcards). | ||
| * Unlike other hostname methods, this does NOT accept bracketed IPv6 |
| * @throws {Error} Thrown if `value` is not a valid hostname string. | ||
| */ | ||
| static mustBeHostname(value) { | ||
| if (typeof value !== 'string') { |
There was a problem hiding this comment.
More apologies (same reason as stated in HostInfo): There is already canonicalizeHostnameElseNull(), which would have been a better starting point for mustBeHostname() than parse...(). Specifically, given that this is a mustBe...() method the pattern in the system is that it returns the given value per se, and so there's no need to actually split up the string etc. etc.
| // guarantee that there are no uppercase letters. TODO: Maybe it should be | ||
| // more restrictive? | ||
| this.#nameString = MustBe.string(nameString, /^[-_.:a-z0-9]+$/); | ||
| this.#nameString = HostUtil.mustBeHostname(nameString); |
There was a problem hiding this comment.
Ok, super-duper sorry about this (I am still paging this code back into my head):
The word "maybe" in the TODO comment is pretty load-bearing in this case, and the point of the note wasn't to say "this should definitely happen, but with low priority," but rather, "I am really not sure if it should be done at all." Specifically, the only way this constructor ever gets invoked in practice is through the static methods on the class, and all of those do canonicalization and validation of some sort or another. To the extent that we add more here, it shouldn't be duplicative of anything already done. So, the right path forward will look more like doing stuff here (including canonicalization, not just error checking) and also getting rid of the stuff in the clients of this constructor which is thereby rendered moot.
As such, arguably we don't need the new method in HostUtil.
I'm leaving the comments I made in HostUtil (which I made before writing this one), more for reference/advice about general stuff than necessarily to be acted on in this PR.
Simplify mustBeHostname() to use canonicalizeHostnameElseNull() as base, per code review feedback. Now accepts bracketed IPv6 addresses (which are canonicalized to bracket-free form).
unprolix
force-pushed
the
jjb-stricter-hostname-validation
branch
from
720f93e to
d1ec36f
Compare
| * @returns {?{pathArray: string[], wildcard: boolean}} The path array and | ||
| * wildcard flag, or `null` if invalid. | ||
| */ | ||
| static #pathArrayFromHostnameElseNull(name, allowWildcard) { |
There was a problem hiding this comment.
This no longer needs to be extracted.
2 participants
Summary
--bad,.leading,trailing.,double..dot,-start,end-Test plan
example.com,sub.example.com,localhost,my-host,a1.b2.c3--bad,.leading,trailing.,double..dot,-start,end-localhoststill work