Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

reject regexp

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

same here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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