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

Allow underscores in wtforms.validators.HostnameValidation. #463

Merged
merged 1 commit into from Jun 17, 2019

Conversation

camillser
Copy link
Contributor

@camillser camillser commented Feb 2, 2019

Example patch for #462

@@ -609,7 +609,7 @@ class HostnameValidation(object):
This is not a validator in and of itself, and as such is not exported.
"""

hostname_part = re.compile(r"^(xn-|[a-z0-9]+)(-[a-z0-9]+)*$", re.IGNORECASE)
hostname_part = re.compile(r"^(xn-|[a-z0-9_]+)(-[a-z0-9_]+)*$", re.IGNORECASE)
Copy link
Member

@davidism davidism Feb 2, 2019

Choose a reason for hiding this comment

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

Is a leading _ valid, even though a leading - isn't?

Copy link
Contributor Author

@camillser camillser Feb 4, 2019

Choose a reason for hiding this comment

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

From some casual research it looks like leading and trailing underscores are valid for domain names [1], but underscores are not valid in hostnames [2]. I think the problem here is that the URL validator is using HostnameValidation to verify the domain name when it should be using a more permissive validator. I see that HostnameValidation is not exported so I think the intention of that validator was to be a domain name validator and to be used exclusively in URL validator. I hope that makes sense. Let me know if it would be a good idea to change the name of HostnameValidation to make the intentions of the class clearer.

[1] https://tools.ietf.org/html/rfc2181#section-11
[2] https://www.ietf.org/rfc/rfc1123.txt

Copy link
Member

@davidism davidism Feb 4, 2019

Choose a reason for hiding this comment

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

Yeah, I'm fine limiting it to domain names.

@ftm
Copy link
Contributor

@ftm ftm commented Jun 15, 2019

Was it decided what the outcome of this was going to be? Should we rename HostnameValidation to something like DomainNameValidation and make it more permissive like the PR is doing?

@ftm ftm added enhancement onhold labels Jun 15, 2019
@davidism
Copy link
Member

@davidism davidism commented Jun 17, 2019

I'm fine with making the validation more permissive. It's not a public API, and it's only used by URL validator, so maybe we should just deprecate the class and inline the code. Related to #474, although whatwg-url doesn't validate the domain.

@ftm
Copy link
Contributor

@ftm ftm commented Jun 17, 2019

I'll merge it for now then, even if later we end up inlining it.

@ftm ftm merged commit b967fb1 into wtforms:master Jun 17, 2019
1 check passed
@davidism
Copy link
Member

@davidism davidism commented Jun 17, 2019

@ftm thanks! Remember to add a changelog entry

ftm added a commit that referenced this issue Jun 17, 2019
ftm added a commit that referenced this issue Jun 17, 2019
azmeuk added a commit to azmeuk/wtforms that referenced this issue Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement onhold
Development

Successfully merging this pull request may close these issues.

None yet

3 participants