Bug in rspamd plugin with private_ip?

[swerter]

Haraka version

Haraka.js — Version: 2.7.2

Expected behavior

According to the docs:

check.private_ip

Default: false

If true, messages from private IPs will not be scanned by Rspamd.

https://github.com/haraka/Haraka/blob/master/plugins/rspamd.js#L104 should be

if (cfg.check.private_ip &&

instead of

if (!cfg.check.private_ip &&

Or the docs should have the "not" removed.

[msimerson]

if (!cfg.check.private_ip &&
        net_utils.is_private_ip(connection.remote_ip)) {
        return next();
    }

I think you are reading that incorrectly. The conditions are inverted because the conditional block is an early exit of the function. Then the conditions are true, the plugin does not check. Rough translation:

if (check.private_ip=false AND remoteIpIsPrivate=true) {
    // do not check
}

It is confusing logic, with the inverse negation. It would be more readable if it were written this way:

if (net_utils.is_private_ip(connection.remote_ip)) {    // it's a private IP
    if (cfg.check.private_ip === false) return next();    // don't check
}

[msimerson]

./plugins/auth/flat_file.js:    if (!net_utils.is_private_ip(connection.remote_ip) && !connection.using_tls) {
./plugins/bounce.js:        if (net_utils.is_private_ip(ip)) continue;
./plugins/connect.asn.js:    if (net_utils.is_private_ip(ip)) return next();
./plugins/connect.fcrdns.js:    if (net_utils.is_private_ip(rip)) {
./plugins/connect.fcrdns.js:                if (net_utils.is_private_ip(rip)) continue;
./plugins/connect.geoip.js:    if (net_utils.is_private_ip(ip)) return;
./plugins/connect.geoip.js:        if (net_utils.is_private_ip(match[1])) continue;  // exclude private IP
./plugins/connect.geoip.js:    if (net_utils.is_private_ip(found_ip)) return;
./plugins/data.uribl.js:                if (net_utils.is_private_ip(host)) {
./plugins/data.uribl.js:                if (net_utils.is_private_ip(host)) {
./plugins/dnsbl.js:    if (net_utils.is_private_ip(rip)) {
./plugins/greylist.js:    if (net_utils.is_private_ip(connection.remote_ip)) {
./plugins/helo.checks.js:    if (plugin.cfg.skip.private_ip && net_utils.is_private_ip(connection.remote_ip)) {
./plugins/helo.checks.js:    if (lmm_mode > 2 && net_utils.is_private_ip(helo_ip)) {
./plugins/messagesniffer.js:    if (net_utils.is_private_ip(connection.remote_ip)) return next();
./plugins/rspamd.js:        net_utils.is_private_ip(connection.remote_ip)) {
./plugins/spf.js:    if (net_utils.is_private_ip(connection.remote_ip)) { return next(); }
./plugins/spf.js:         net_utils.is_private_ip(connection.remote_ip)) {

With this many plugins using net_utils.is_private_ip(), I think it's no longer a premature optimization to just run that command once in connection.js and let all the plugins have a connection.is_private_ip boolean to check.

@haraka/core thoughts?

[swerter]

I am sorry, but I still think the docs are incorrect. Here, have a look: If I add to the docs the inverse:

If true, messages from private IPs will not be scanned by Rspamd.
If false, messages from private IPs will be scanned by Rspamd.

In the code it's exactly the contrary, if you set to false it will not be scanned (cfg.check.private_ip === false resulting in return next(), thus skip scanning). So the docs should be:

If true, messages from private IPs will be scanned by Rspamd.
If false, messages from private IPs will not be scanned by Rspamd.

Ahh these double negations :)

[msimerson]

I am sorry, but I still think the docs are incorrect.

They might be, didn't check. PRs that improve upon them are welcome. Especially if they avoid double negatives.

[swerter]

Here the pull request: https://github.com/haraka/Haraka/pull/1383

[smfreegard]

With this many plugins using net_utils.is_private_ip(), I think it's no longer a premature optimization to just run that command once in connection.js and let all the plugins have a connection.is_private_ip boolean to check.

@haraka/core thoughts?

+1

[baudehlo]

Yup, +1

Maybe not required for 2.8, but I don't care if it goes in (with docs).

On Thu, Mar 10, 2016 at 7:29 PM, Steve Freegard notifications@github.com wrote:

With this many plugins using net_utils.is_private_ip(), I think it's no longer a premature optimization to just run that command once in connection.js and let all the plugins have a connection.is_private_ip boolean to check.

@haraka/core https://github.com/orgs/haraka/teams/core thoughts?

+1

— Reply to this email directly or view it on GitHub https://github.com/haraka/Haraka/issues/1382#issuecomment-195116554.