Skip invalid hostnames check in uribl

[dani]

For example the string my..my would be detected as a hostname and ends with a fatal plugin error

(data_post) uribl: URIBL: checking sub-host my..my
(data_post) uribl: URIBL: checking sub-host .my
FATAL PLUGIN ERROR [uribl]: unexpected null domain label at /usr/lib64/perl5/vendor_perl/Net/DNS/Question.pm line 80

Instead of creating too complex regex, add a global regex list and skip entries which match one of them.

For now, only .. are skiped, but it'll be easy to add more if needed

Note that this smart match operator requires perl 5.10 or later

[msimerson]

I'm thinking out loud here. I'm really not terribly fond of fixing one regex problem by throwing more regex at it. I think better and faster (runtime) would be to add a valid_hostname function that filters URIBL matches to just valid hostnames (RFC 2181). Off the top of my head, it should:

• assure hostnames do not exceed 255 octets
• split hostnames into labels (on dot boundaries)
• assure each label is at least one octet
• assure labels do not exceed 63 octets
• assure TLD label has at least 2 octets

[dani]

What about relying on a dedicated module like http://search.cpan.org/~drolsky/Data-Validate-Domain/ to do those checks ?

[msimerson]

Even better!

[dani]

While I'm looking at this, we might even replace the manual URI search with http://search.cpan.org/~mschwern/URI-Find/lib/URI/Find.pm

[dani]

What about https://github.com/smtpd/qpsmtpd/compare/master...dani:valid_domain?expand=1

It's a very unintrusive solution, just requires Data::Validate::Domain (which itself requires Net::Domain::TLD. All it does is check the extracted strings are valid domain names (including check the labels are all valid etc...)

[msimerson]

My preferred solution is one that:

1. adds tests which exercise the problem in this issue
2. solves the problem
3. prevents regression (see 1.)

Bonus points for replacing custom QP code with well documented and supported CPAN modules.

[dani]

Do you mean this solution would be OK if I add unit test ? I'm not very familiar with them, I could probably hack it but here are no test at all for uribl for now, so I'm not sure where to start

[msimerson]

Do you mean this solution would be OK if I add unit test

Exactly.

there are no test at all for uribl for now, so I'm not sure where to start

Take a look at the existing plugins that have unit tests. We've done a lot of work to make it not terribly difficult to write plugin tests. Also, you can run the unit tests for just one plugin with this syntax:

$ perl t/plugin_tests.t relay
# 14424 failed to load Mail::DMARC::PurePerl
# 14424 error, dspam CLI binary not found: install dspam and/or set dspam_bin
# 14424 unable to load ClamAV::Client
# relay  test_relay_only
ok 1 - relay_only -
ok 2 - relay_only +
# relay  test_is_octet_match
ok 3 - match, +
ok 4 - nope, -
ok 5 - nope, -
# relay  test_is_in_cidr_block
ok 6 - match, +
ok 7 - nope, -
ok 8 - match, +
ok 9 - nope, -
# relay  test_is_in_norelayclients
ok 10 - match, + (192.0.99.5)
ok 11 - match, + (192.0.98.1)
ok 12 - match, + (192.0.98.255)
ok 13 - match, - (192.0.99.7)
ok 14 - match, - (192.0.109.7)
1..14

That saves a ton of time.

[charliebrady]

On Tue, 5 Jul 2016, Matt Simerson wrote:

My preferred solution is one that:

1. adds tests which exercise the problem in this issue
2. solves the problem
3. prevents regression (see 1.)

Bonus points for replacing custom QP code with well documented and supported CPAN modules.

... which don't have a deep dependency tree.

[dani]

Which is the case: Data::Validate::Domain only requires Net::Domain::TLD. Unfortunately, I just can't get the logic for writing unit tests :-(

[msimerson]

@dani, did you make progress on using Data::Validate::Domain? If so, can you push that to this branch?

[msimerson]

closing as abandoned.

[unnilennium]

@msimerson , is uribl plugin abandonned or is this the lack of answer of @dani that makes this pull request abandoned ?

[msimerson]

the latter.