fix(fqdn): reject domain labels with a trailing hyphen - #1592
Open
mahirhir wants to merge 2 commits into
Open
Conversation
fqdnRegexStringRFC1123 matched each label with
[a-zA-Z0-9]{1}[a-zA-Z0-9-]{0,62}, which allows a label to end in a
hyphen (e.g. "foo-.example.com" or "example.com-"). The hostname
validators were fixed for the same issue in go-playground#1565 (RFC 1123) and go-playground#1569
(RFC 952), but the fqdn pattern, whose comment says it should match
hostnameRegexStringRFC1123, was left behind.
Use the same label shape [a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])? so
a label must start and end with an alphanumeric. The non-numeric TLD,
the optional trailing dot, and hyphens inside the last label (go-playground#1548) are
preserved.
| {"24.example24.com", true}, | ||
| {"test.24.example.com", true}, | ||
| {"test-site-http.test-site", true}, | ||
| {"foo-.example.com", false}, |
There was a problem hiding this comment.
Perhaps some test cases confirming label length (63 characters) would be helpful as an addition to the test suite? I believe the regex seems correct, but it may be beneficial for future contributors, what do you think?
Author
|
Added cases for a 63-char label (passes, the RFC max a single label can be) and 64 (fails). TestFQDNValidation passes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes Or Enhances
The
fqdnvalidator accepts domain labels that end in a hyphen, even though those are not valid hostnames. All of these pass today but shouldn't:foo-.example.com(first label ends in a hyphen)foo.bar-.com(middle label ends in a hyphen)example.com-(TLD ends in a hyphen)fqdnRegexStringRFC1123matches each label with[a-zA-Z0-9]{1}[a-zA-Z0-9-]{0,62}, i.e. one alphanumeric followed by up to 62 of[a-zA-Z0-9-], so nothing forces the last character of a label to be alphanumeric.The hostname validators had the same issue and were fixed in #1565 (RFC 1123) and #1569 (RFC 952), which changed each label to
[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?. The fqdn pattern was not updated at the same time, even though its comment says it is "same as hostnameRegexStringRFC1123 but must contain a non numerical TLD". This PR applies that same label shape to the fqdn pattern, including the TLD (which still has to start with a letter).Behaviour that stays the same: the non-numeric TLD requirement, the optional trailing dot, and hyphens inside the last label added in #1548 (
test-site-http.test-sitestill validates). I diffed the new pattern against the old one over a large set of random label strings; the only inputs whose result changes are the ones with a label ending in a hyphen.Added regression cases to
TestFQDNValidationand ran the package tests locally.Make sure that you've checked the boxes below before you submit PR:
@go-playground/validator-maintainers