bugfix: domain name validation. #951
Conversation
Updated regexp for domain validation Add testcases for domain validation fix issue: IdentityPython#950
tests/test_13_validate.py
Outdated
| assert valid_domain_name("lk.domain.com:12") | ||
| assert valid_domain_name("lk.domain.com:12") | ||
| assert valid_domain_name("static.domain.xyz:12345") |
There was a problem hiding this comment.
these are domains + ports; they are not just domains.
There was a problem hiding this comment.
yep but it can be as value in DNSName? or Not?
I think it depend of setup of IdentityProvider
tests/test_13_validate.py
Outdated
| assert valid_domain_name("domain.com") | ||
| assert valid_domain_name("domain.lu") | ||
| assert valid_domain_name("auth-domain.com") | ||
| assert valid_domain_name("domain.com:12345") |
tests/test_13_validate.py
Outdated
| valid_domain_name("") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("auth.domain.ljnjnfds") |
There was a problem hiding this comment.
Why should this result to an error?
There was a problem hiding this comment.
top-level domain cannot be longer than 5 characters
There was a problem hiding this comment.
top-level domain cannot be longer than 5 characters
According to the MDN, the longest a TLD can be is 63 characters. Cutting this down to a 5-character space would invalidate many top level domains, some that I own, some that I know others own.
tests/test_13_validate.py
Outdated
| valid_domain_name("exaple.c") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("123example.com") |
There was a problem hiding this comment.
domains can start with digits
There was a problem hiding this comment.
tryed to find it and I found,
I agree
src/saml2/validate.py
Outdated
| @@ -420,6 +420,7 @@ def valid_instance(instance): | |||
|
|
|||
|
|
|||
| def valid_domain_name(dns_name): | |||
| m = re.match(r"^[a-z0-9]+([-.]{ 1 }[a-z0-9]+).[a-z]{2,5}(:[0-9]{1,5})?(\/.)?$", dns_name, re.I) | |||
| m = re.match(r"^((?:[a-zA-Z](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?\.)+[a-zA-Z]{2,5})(?::\d+)?$", dns_name, re.I) | |||
There was a problem hiding this comment.
does the regex come from somewhere?
|
The original issue is due to the change from This PR introduces a new regex. I would be good to have an explanation about the regex itself. Further, we could also reuse existing packages for this purpose, like |
yep, but I found other problems with that regex. |
- Added validators library. - For domain validation uses validators.domain insted of regepx. - Updated tests according to reviews.
Here we go. |
Updated regexp for domain validation
Add testcases for domain validation
fix issue: #950
fix small bug with domain validation