-
Notifications
You must be signed in to change notification settings - Fork 455
bugfix: domain name validation. #951
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| from saml2.validate import valid_any_uri | ||
| from saml2.validate import valid_anytype | ||
| from saml2.validate import valid_duration | ||
| from saml2.validate import valid_domain_name | ||
| from saml2.validate import valid_instance | ||
| from saml2.validate import valid_non_negative_integer | ||
| from saml2.validate import valid_string | ||
|
|
@@ -146,3 +147,61 @@ def test_valid_address(): | |
| assert valid_address("[2001:8003:5555:9999:555a:5555:c77:d5c5") | ||
| with raises(NotValid): | ||
| assert valid_address("[[2001:8003:5555:9999:555a:5555:c77:d5c5]") | ||
|
|
||
|
|
||
| def test_valid_domain_name(): | ||
| assert valid_domain_name("api.my-domain.com") | ||
| assert valid_domain_name("auth.admin.domain.com") | ||
| assert valid_domain_name("auth.domain.com") | ||
| assert valid_domain_name("auth.domain.com") | ||
| assert valid_domain_name("lk.domain.com:12") | ||
| assert valid_domain_name("lk.domain.com:12") | ||
| assert valid_domain_name("static.domain.xyz:12345") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these are domains + ports; they are not just domains.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep but it can be as value in DNSName? or Not?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
| 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") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
| assert valid_domain_name("auth-admin.domain-uero.xyz") | ||
| assert valid_domain_name("auth.lk.d.sr.mydomain.com") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("auth.domain.ljnjnfds") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why should this result to an error?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. top-level domain cannot be longer than 5 characters There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("123.123.123.123") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("123.123.123.123:80") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("123.123.123.123:8000") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("auth_domain.com") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("example-.com") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("exa@ple.com") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("exaple.c") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("123example.com") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. domains can start with digits
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tryed to find it and I found,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("example.com:") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("example..com") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("example.com123") | ||
|
|
||
| with raises(ValueError): | ||
| valid_domain_name("example.com.") | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the regex come from somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
combined with previous regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reject regexp