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

Fixes #36 Hostname matching doesn't conform to modern hostname patterns #37

Merged
merged 1 commit into from Oct 30, 2018

Conversation

jimmystewpot
Copy link

This PR introduces a change to the regex which allows for Hostnames to start with a number.

I have added a test for two valid domains which currently pass.

Signed-off-by: James Lamb jlamb@atlassian.com

…ame patterns

Signed-off-by: James Lamb <jlamb@atlassian.com>
@codecov
Copy link

codecov bot commented Oct 30, 2018

Codecov Report

Merging #37 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #37   +/-   ##
=======================================
  Coverage   87.97%   87.97%           
=======================================
  Files          10       10           
  Lines        1422     1422           
=======================================
  Hits         1251     1251           
  Misses        144      144           
  Partials       27       27
Impacted Files Coverage Δ
default.go 92.81% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81f5045...6232591. Read the comment docs.

@casualjim casualjim merged commit b7bbaf9 into go-openapi:master Oct 30, 2018
@casualjim
Copy link
Member

thanks!

@fredbi
Copy link
Member

fredbi commented Oct 30, 2018

This update now lets the #.#.#.# form as a valid hostname, which should not be valid.

@casualjim
Copy link
Member

for an outdated specification because numbers as first character are definitely valid now.
I tried to create a few in dns servers and it works just fine which is where the limitation comes from.
So in this case we can ignore the spec and be real instead of follow outdated rules.

#36

@casualjim
Copy link
Member

The way this pattern is written will allow 9999.com but not 9999.88 or 9999.88.com. So to make this more correct it would need to match 9999.888.com, it's only the tld's that can't start with a number for now.

@fredbi
Copy link
Member

fredbi commented Oct 30, 2018

Yes correct. And that for digits.

There are so many errata to this set of RFCs that is very difficult to set out for an implementation...

A few days ago, I was also wondering if [A-Za-z] would not be better with unicode support, like \p{L}.
But the json spec states that "hostname" follows the "puny code" encoding, so apparently we only end up here with ascii and dashes. Is my assumption correct here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants