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

Public suffix check is case sensitive #251

Open
abeverley opened this issue Dec 9, 2024 · 6 comments · May be fixed by #252
Open

Public suffix check is case sensitive #251

abeverley opened this issue Dec 9, 2024 · 6 comments · May be fixed by #252

Comments

@abeverley
Copy link

The public suffix check in Mail::DMARC::Base is case sensitive, which means that when is_valid_domain() is called from within Mail::Dmarc that it returns false if the domain has a capitalized suffix. This causes croaks such as invalid header_from to be thrown.

I think it just needs a simple lc added in the function, which I have tested and seems to work:

- return 1 if $public_suffixes->{$zone};
+ return 1 if $public_suffixes->{lc $zone};

Module version 1.20211209
Perl version 5.36.0

Many thanks.

@msimerson
Copy link
Owner

The changes in #212 and #249 should have addressed this. Updating to the latest version should solve your issue.

@abeverley
Copy link
Author

Thanks for the quick reply Matt. I think those are slightly different issues though (regarding just SPF). This happens when I call Mail::DMARC::PurePerl->new with the header_from parameter. I've tried the latest version of the module and the same problem happens.

The problem is slightly more subtle than I thought. It only happens with public domains that consist of 2 suffixes (with no single suffix equivalent). E.g. .CO.ZA. This is because the single suffix check includes lc whereas the double-suffix does not:

my $tld = ( split /\./, lc $domain )[-1];
return 1 if $self->is_public_suffix($tld);
$tld = join( '.', ( split /\./, $domain )[ -2, -1 ] );
return 1 if $self->is_public_suffix($tld);

@msimerson
Copy link
Owner

msimerson commented Dec 9, 2024

Ahhhhhh, I can see that. I think this is the better way to solve that:

diff --git a/lib/Mail/DMARC.pm b/lib/Mail/DMARC.pm
index 8cd57c7..9a3a3bb 100644
--- a/lib/Mail/DMARC.pm
+++ b/lib/Mail/DMARC.pm
@@ -45,19 +45,19 @@ sub source_ip {
 
 sub envelope_to {
     return $_[0]->{envelope_to} if 1 == scalar @_;
-    croak "invalid envelope_to" if !$_[0]->is_valid_domain( $_[1] );
+    croak "invalid envelope_to" if !$_[0]->is_valid_domain( lc $_[1] );
     return $_[0]->{envelope_to} = $_[1];
 }
 
 sub envelope_from {
     return $_[0]->{envelope_from} if 1 == scalar @_;
-    croak "invalid envelope_from" if !$_[0]->is_valid_domain( $_[1] );
+    croak "invalid envelope_from" if !$_[0]->is_valid_domain( lc $_[1] );
     return $_[0]->{envelope_from} = $_[1];
 }
 
 sub header_from {
     return $_[0]->{header_from} if 1 == scalar @_;
-    croak "invalid header_from" if !$_[0]->is_valid_domain( $_[1] );
+    croak "invalid header_from" if !$_[0]->is_valid_domain( lc $_[1] );
     return $_[0]->{header_from} = lc $_[1];
 }
 

Thoughts?

@abeverley
Copy link
Author

Thanks Matt, that will definitely work and I'd be happy with that. Given that a domain is always (?) case-insensitive though, I would have thought it might be better to add it to the is_valid_domain function in case it gets used elsewhere in the future?

@msimerson
Copy link
Owner

msimerson commented Dec 9, 2024

In a world with unicode characters and IDN, where a naive case change could be lossy, I feel reluctant to be altering domain names at all. I almost feel that passing in valid domain names should be the purview of the calling application (normalizing them with toASCII or punycode). OTOH there's Postel's Law. If we're going to be liberal in what we accept, then I think any case changes should be at the boundaries of the public API, so they're predictable.

@abeverley
Copy link
Author

Fair enough, makes sense :)

@msimerson msimerson linked a pull request Dec 9, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants