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

bugfix: domain name validation. #951

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/saml2/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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?

Copy link
Author

Choose a reason for hiding this comment

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

combined with previous regex.

if not m:
raise ValueError("Not a proper domain name")
return True
59 changes: 59 additions & 0 deletions tests/test_13_validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Comment on lines +157 to +159
Copy link
Member

Choose a reason for hiding this comment

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

these are domains + ports; they are not just domains.

Copy link
Author

@StuBz211 StuBz211 Apr 23, 2024

Choose a reason for hiding this comment

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

yep but it can be as value in DNSName? or Not?
I think it depend of setup of IdentityProvider

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")
Copy link
Member

Choose a reason for hiding this comment

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

same here

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")
Copy link
Member

Choose a reason for hiding this comment

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

Why should this result to an error?

Copy link
Author

Choose a reason for hiding this comment

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

top-level domain cannot be longer than 5 characters

Copy link

Choose a reason for hiding this comment

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

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.


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("[email protected]")

with raises(ValueError):
valid_domain_name("exaple.c")

with raises(ValueError):
valid_domain_name("123example.com")
Copy link
Member

Choose a reason for hiding this comment

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

domains can start with digits

Copy link
Author

Choose a reason for hiding this comment

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

tryed to find it and I found,
I agree


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.")