From 83c4e50fd385647dcb5ead8cb11756c7bb2c9269 Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Thu, 22 Jun 2023 13:36:41 -0700 Subject: [PATCH 1/2] check for DNS results befor iterating, fixes #13 --- lib/spf.js | 8 ++++++-- test/index.js | 8 ++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/spf.js b/lib/spf.js index 3f15168..efb70d8 100644 --- a/lib/spf.js +++ b/lib/spf.js @@ -413,6 +413,8 @@ class SPF { } } + if (!addrs) return self.SPF_NONE + for (const addr of addrs) { if (cidr) { // CIDR @@ -501,8 +503,10 @@ class SPF { } pending--; - self.log_debug(`mech_mx: mx=${mx} addresses=${addrs.join(',')}`); - addresses = addrs.concat(addresses); + if (addrs) { + self.log_debug(`mech_mx: mx=${mx} addresses=${addrs.join(',')}`); + addresses = addrs.concat(addresses); + } if (pending === 0) { if (!addresses.length) return self.SPF_NONE // All queries run; see if our IP matches diff --git a/test/index.js b/test/index.js index fa831fa..9f12052 100644 --- a/test/index.js +++ b/test/index.js @@ -160,6 +160,14 @@ describe('hook_helo', function () { done() }, this.connection, '[190.168.1.1]' ); }) + + it('MX with no A record', function (done) { + this.connection.set('remote.ip', '192.0.2.0'); + this.plugin.helo_spf(function next (rc) { + assert.equal(undefined, rc); + done() + }, this.connection, 'haraka-test.tnpi.net' ); + }) }) const test_addr = new Address(''); From 988a3175e6c0c509aec5b08205d4c9f1f69697ba Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Thu, 22 Jun 2023 13:41:04 -0700 Subject: [PATCH 2/2] es6(lib/spf): replace 'self' with 'this' --- Changes.md | 9 ++- lib/spf.js | 216 +++++++++++++++++++++++++-------------------------- package.json | 2 +- 3 files changed, 114 insertions(+), 113 deletions(-) diff --git a/Changes.md b/Changes.md index f0f8548..3f8e8f8 100644 --- a/Changes.md +++ b/Changes.md @@ -2,11 +2,17 @@ ### Unreleased +### [1.2.2] - 2023-06-22 + +- fix: check for DNS results befor iterating, fixes #13 +- es6(lib/spf): replace `self` with `this` + + ### [1.2.1] - 2023-06-19 - fix: call skip_hosts via 'this' instead of exports (#11) - skip configuration was being ignored -- es6: replace `plugin` with `this` +- es6(index): replace `plugin` with `this` - deps: bump versions to latest @@ -52,3 +58,4 @@ [1.1.3]: https://github.com/haraka/haraka-plugin-spf/releases/tag/1.1.3 [1.2.0]: https://github.com/haraka/haraka-plugin-spf/releases/tag/1.2.0 [1.2.1]: https://github.com/haraka/haraka-plugin-spf/releases/tag/1.2.1 +[1.3.0]: https://github.com/haraka/haraka-plugin-spf/releases/tag/1.3.0 diff --git a/lib/spf.js b/lib/spf.js index efb70d8..55738e8 100644 --- a/lib/spf.js +++ b/lib/spf.js @@ -158,7 +158,6 @@ class SPF { } async check_host (ip, domain, mail_from) { - const self = this; domain = domain.toLowerCase(); mail_from = mail_from ? mail_from.toLowerCase() : `postmaster@${domain}` this.ipaddr = ipaddr.parse(ip); @@ -183,12 +182,12 @@ class SPF { txt_rrs = await dns.resolveTxt(domain) } catch (err) { - self.log_debug(`error looking up TXT record: ${err.message}`); + this.log_debug(`error looking up TXT record: ${err.message}`); switch (err.code) { case dns.NOTFOUND: case dns.NODATA: - case dns.NXDOMAIN: return self.SPF_NONE - default: return self.SPF_TEMPERROR + case dns.NXDOMAIN: return this.SPF_NONE + default: return this.SPF_TEMPERROR } } @@ -198,24 +197,24 @@ class SPF { match = /^(v=spf1(?:$|\s.+$))/i.exec(txt_rr); if (!match) { - self.log_debug(`discarding TXT record: ${txt_rr}`); + this.log_debug(`discarding TXT record: ${txt_rr}`); continue; } if (!spf_record) { - self.log_debug(`found SPF record for domain ${domain}: ${match[1]}`); + this.log_debug(`found SPF record for domain ${domain}: ${match[1]}`); spf_record = match[1].replace(/\s+/, ' ').toLowerCase(); } else { - self.log_debug(`found additional SPF record for domain ${domain}: ${match[1]}`); - return self.SPF_PERMERROR + this.log_debug(`found additional SPF record for domain ${domain}: ${match[1]}`); + return this.SPF_PERMERROR } } - if (!spf_record) return self.SPF_NONE // No SPF record? + if (!spf_record) return this.SPF_NONE // No SPF record? // Store the SPF record used in the object - self.spf_record = spf_record; + this.spf_record = spf_record; // Validate SPF record and build call chain const mech_regexp1 = /^([-+~?])?(all|a|mx|ptr)$/; @@ -230,20 +229,20 @@ class SPF { if ((match = (mech_regexp1.exec(mechanism) || mech_regexp2.exec(mechanism)))) { // match: 1=qualifier, 2=mechanism, 3=optional args if (!match[1]) match[1] = '+'; - self.log_debug(`found mechanism: ${match}`); + this.log_debug(`found mechanism: ${match}`); if (match[2] === 'ip4' || match[2] === 'ip6') { - if (!this.valid_ip(match[3])) return self.SPF_PERMERROR + if (!this.valid_ip(match[3])) return this.SPF_PERMERROR } else { // Validate macro strings if (match[3] && /%[^{%+-]/.exec(match[3])) { - self.log_debug('invalid macro string'); - return self.SPF_PERMERROR + this.log_debug('invalid macro string'); + return this.SPF_PERMERROR } if (match[3]) { // Expand macros - match[3] = self.expand_macros(match[3]); + match[3] = this.expand_macros(match[3]); } } @@ -252,12 +251,12 @@ class SPF { // console.log(mech_array) } else if ((match = mod_regexp.exec(mechanism))) { - self.log_debug(`found modifier: ${match}`); + this.log_debug(`found modifier: ${match}`); // match[1] = modifier // match[2] = name // Make sure we have a method - if (!self[`mod_${match[1]}`]) { - self.log_debug(`skipping unknown modifier: ${match[1]}`); + if (!this[`mod_${match[1]}`]) { + this.log_debug(`skipping unknown modifier: ${match[1]}`); } else { obj[match[1]] = match[2]; @@ -267,31 +266,31 @@ class SPF { } else { // Syntax error - self.log_debug(`syntax error: ${mechanism}`); - return self.SPF_PERMERROR + this.log_debug(`syntax error: ${mechanism}`); + return this.SPF_PERMERROR } } - self.log_debug(`SPF record for '${self.domain}' validated OK`); + this.log_debug(`SPF record for '${this.domain}' validated OK`); // Run all the mechanisms first for (const mech of mech_array) { const func = Object.keys(mech); const args = mech[func]; - // console.log(`running mechanism: ${func} args=${args} domain=${self.domain}`); - self.log_debug(`running mechanism: ${func} args=${args} domain=${self.domain}`); + // console.log(`running mechanism: ${func} args=${args} domain=${this.domain}`); + this.log_debug(`running mechanism: ${func} args=${args} domain=${this.domain}`); - if (self.count > self.LIMIT) { - self.log_debug('lookup limit reached'); - return self.SPF_PERMERROR + if (this.count > this.LIMIT) { + this.log_debug('lookup limit reached'); + return this.SPF_PERMERROR } - const result = await self[`mech_${func}`](((args && args.length) ? args[0] : null), ((args && args.length) ? args[1] : null)); + const result = await this[`mech_${func}`](((args && args.length) ? args[0] : null), ((args && args.length) ? args[1] : null)); // console.log(result) // If we have a result other than SPF_NONE - if (result && result !== self.SPF_NONE) return result + if (result && result !== this.SPF_NONE) return result } // run any modifiers @@ -299,20 +298,20 @@ class SPF { const func = Object.keys(mod); const args = mod[func]; - self.log_debug(`running modifier: ${func} args=${args} domain=${self.domain}`); - const result = await self[`mod_${func}`](args); + this.log_debug(`running modifier: ${func} args=${args} domain=${this.domain}`); + const result = await this[`mod_${func}`](args); // Check limits - if (self.count > self.LIMIT) { - self.log_debug('lookup limit reached'); - return self.SPF_PERMERROR + if (this.count > this.LIMIT) { + this.log_debug('lookup limit reached'); + return this.SPF_PERMERROR } // Return any result that is not SPF_NONE - if (result && result !== self.SPF_NONE) return result + if (result && result !== this.SPF_NONE) return result } - return self.SPF_NEUTRAL // default if no more mechanisms + return this.SPF_NEUTRAL // default if no more mechanisms } async mech_all (qualifier, args) { @@ -320,27 +319,26 @@ class SPF { } async mech_include (qualifier, args) { - const self = this; const domain = args.substr(1); // Avoid circular references - if (self.been_there[domain]) { - self.log_debug(`circular reference detected: ${domain}`); - return self.SPF_NONE + if (this.been_there[domain]) { + this.log_debug(`circular reference detected: ${domain}`); + return this.SPF_NONE } - self.count++; - self.been_there[domain] = true; + this.count++; + this.been_there[domain] = true; // Recurse - const recurse = new SPF(self.count, self.been_there); + const recurse = new SPF(this.count, this.been_there); try { - const result = await recurse.check_host(self.ip, domain, self.mail_from) - self.log_debug(`mech_include: domain=${domain} returned=${self.const_translate(result)}`); + const result = await recurse.check_host(this.ip, domain, this.mail_from) + this.log_debug(`mech_include: domain=${domain} returned=${this.const_translate(result)}`); switch (result) { - case self.SPF_PASS: return self.SPF_PASS - case self.SPF_FAIL: - case self.SPF_SOFTFAIL: - case self.SPF_NEUTRAL: return self.SPF_NONE - case self.SPF_TEMPERROR: return self.SPF_TEMPERROR - default: return self.SPF_PERMERROR + case this.SPF_PASS: return this.SPF_PASS + case this.SPF_FAIL: + case this.SPF_SOFTFAIL: + case this.SPF_NEUTRAL: return this.SPF_NONE + case this.SPF_TEMPERROR: return this.SPF_TEMPERROR + default: return this.SPF_PERMERROR } } catch (err) { @@ -349,31 +347,29 @@ class SPF { } async mech_exists (qualifier, args) { - const self = this; - self.count++; + this.count++; const exists = args.substr(1); try { const addrs = await dns.resolve(exists) - self.log_debug(`mech_exists: ${exists} result=${addrs.join(',')}`); - return self.return_const(qualifier) + this.log_debug(`mech_exists: ${exists} result=${addrs.join(',')}`); + return this.return_const(qualifier) } catch (err) { - self.log_debug(`mech_exists: ${err}`); + this.log_debug(`mech_exists: ${err}`); switch (err.code) { case dns.NOTFOUND: case dns.NODATA: case dns.NXDOMAIN: - return self.SPF_NONE + return this.SPF_NONE default: - return self.SPF_TEMPERROR + return this.SPF_TEMPERROR } } } async mech_a (qualifier, args) { - const self = this; - self.count++; + this.count++; // Parse any arguments let cm; let cidr4; @@ -383,18 +379,18 @@ class SPF { cidr6 = cm[2]; } let dm; - let domain = self.domain; + let domain = this.domain; if (args && (dm = /^:([^/ ]+)/.exec(args))) { domain = dm[1]; } // Calculate with IP method to use let resolve_method; let cidr; - if (self.ip_ver === 'ipv4') { + if (this.ip_ver === 'ipv4') { cidr = cidr4; resolve_method = 'resolve4'; } - else if (self.ip_ver === 'ipv6') { + else if (this.ip_ver === 'ipv6') { cidr = cidr6; resolve_method = 'resolve6'; } @@ -404,43 +400,42 @@ class SPF { addrs = await dns[resolve_method](domain) } catch (err) { - self.log_debug(`mech_a: ${err}`); + this.log_debug(`mech_a: ${err}`); switch (err.code) { case dns.NOTFOUND: case dns.NODATA: - case dns.NXDOMAIN: return self.SPF_NONE - default: return self.SPF_TEMPERROR + case dns.NXDOMAIN: return this.SPF_NONE + default: return this.SPF_TEMPERROR } } - if (!addrs) return self.SPF_NONE + if (!addrs) return this.SPF_NONE for (const addr of addrs) { if (cidr) { // CIDR const range = ipaddr.parse(addr); - if (self.ipaddr.match(range, cidr)) { - self.log_debug(`mech_a: ${self.ip} => ${addr}/${cidr}: MATCH!`); - return self.return_const(qualifier) + if (this.ipaddr.match(range, cidr)) { + this.log_debug(`mech_a: ${this.ip} => ${addr}/${cidr}: MATCH!`); + return this.return_const(qualifier) } else { - self.log_debug(`mech_a: ${self.ip} => ${addr}/${cidr}: NO MATCH`); + this.log_debug(`mech_a: ${this.ip} => ${addr}/${cidr}: NO MATCH`); } } else { - if (addr === self.ip) { - return self.return_const(qualifier) + if (addr === this.ip) { + return this.return_const(qualifier) } else { - self.log_debug(`mech_a: ${self.ip} => ${addr}: NO MATCH`); + this.log_debug(`mech_a: ${this.ip} => ${addr}: NO MATCH`); } } } - return self.SPF_NONE + return this.SPF_NONE } async mech_mx (qualifier, args) { - const self = this; this.count++; // Parse any arguments let cm; @@ -464,15 +459,15 @@ class SPF { switch (err.code) { case dns.NOTFOUND: case dns.NODATA: - case dns.NXDOMAIN: return self.SPF_NONE - default: return self.SPF_TEMPERROR + case dns.NXDOMAIN: return this.SPF_NONE + default: return this.SPF_TEMPERROR } } let pending = 0; let addresses = []; // RFC 4408 Section 10.1 - if (mxes.length > self.LIMIT) return self.SPF_PERMERROR + if (mxes.length > this.LIMIT) return this.SPF_PERMERROR for (const element of mxes) { pending++; @@ -480,11 +475,11 @@ class SPF { // Calculate which IP method to use let resolve_method; let cidr; - if (self.ip_ver === 'ipv4') { + if (this.ip_ver === 'ipv4') { cidr = cidr4; resolve_method = 'resolve4'; } - else if (self.ip_ver === 'ipv6') { + else if (this.ip_ver === 'ipv6') { cidr = cidr6; resolve_method = 'resolve6'; } @@ -498,52 +493,51 @@ class SPF { case dns.NOTFOUND: case dns.NODATA: case dns.NXDOMAIN: break; - default: return self.SPF_TEMPERROR + default: return this.SPF_TEMPERROR } } pending--; if (addrs) { - self.log_debug(`mech_mx: mx=${mx} addresses=${addrs.join(',')}`); + this.log_debug(`mech_mx: mx=${mx} addresses=${addrs.join(',')}`); addresses = addrs.concat(addresses); } if (pending === 0) { - if (!addresses.length) return self.SPF_NONE + if (!addresses.length) return this.SPF_NONE // All queries run; see if our IP matches if (cidr) { // CIDR match type for (const address of addresses) { const range = ipaddr.parse(address); - if (self.ipaddr.match(range, cidr)) { - self.log_debug(`mech_mx: ${self.ip} => ${address}/${cidr}: MATCH!`); - return self.return_const(qualifier) + if (this.ipaddr.match(range, cidr)) { + this.log_debug(`mech_mx: ${this.ip} => ${address}/${cidr}: MATCH!`); + return this.return_const(qualifier) } else { - self.log_debug(`mech_mx: ${self.ip} => ${address}/${cidr}: NO MATCH`); + this.log_debug(`mech_mx: ${this.ip} => ${address}/${cidr}: NO MATCH`); } } // No matches - return self.SPF_NONE + return this.SPF_NONE } else { - if (addresses.includes(self.ip)) { - self.log_debug(`mech_mx: ${self.ip} => ${addresses.join(',')}: MATCH!`); - return self.return_const(qualifier) + if (addresses.includes(this.ip)) { + this.log_debug(`mech_mx: ${this.ip} => ${addresses.join(',')}: MATCH!`); + return this.return_const(qualifier) } else { - self.log_debug(`mech_mx: ${self.ip} => ${addresses.join(',')}: NO MATCH`); - return self.SPF_NONE + this.log_debug(`mech_mx: ${this.ip} => ${addresses.join(',')}: NO MATCH`); + return this.SPF_NONE } } } // In case we didn't run any queries... - if (pending === 0) return self.SPF_NONE + if (pending === 0) return this.SPF_NONE } - if (pending === 0) self.SPF_NONE + if (pending === 0) this.SPF_NONE } async mech_ptr (qualifier, args) { - const self = this; this.count++; let dm; let domain = this.domain; @@ -556,34 +550,34 @@ class SPF { ptrs = await dns.reverse(this.ip) } catch (err) { - self.log_debug(`mech_ptr: lookup=${self.ip} => ${err}`); - return self.SPF_NONE + this.log_debug(`mech_ptr: lookup=${this.ip} => ${err}`); + return this.SPF_NONE } let resolve_method; - if (self.ip_ver === 'ipv4') resolve_method = 'resolve4'; - if (self.ip_ver === 'ipv6') resolve_method = 'resolve6'; + if (this.ip_ver === 'ipv4') resolve_method = 'resolve4'; + if (this.ip_ver === 'ipv6') resolve_method = 'resolve6'; const names = []; // RFC 4408 Section 10.1 - if (ptrs.length > self.LIMIT) return self.SPF_PERMERROR + if (ptrs.length > this.LIMIT) return this.SPF_PERMERROR for (const ptr of ptrs) { try { const addrs = await dns[resolve_method](ptr) for (const addr of addrs) { - if (addr === self.ip) { - self.log_debug(`mech_ptr: ${self.ip} => ${ptr} => ${addr}: MATCH!`); + if (addr === this.ip) { + this.log_debug(`mech_ptr: ${this.ip} => ${ptr} => ${addr}: MATCH!`); names.push(ptr.toLowerCase()); } else { - self.log_debug(`mech_ptr: ${self.ip} => ${ptr} => ${addr}: NO MATCH`); + this.log_debug(`mech_ptr: ${this.ip} => ${ptr} => ${addr}: NO MATCH`); } } } catch (err) { // Skip on error - self.log_debug(`mech_ptr: lookup=${ptr} => ${err}`); + this.log_debug(`mech_ptr: lookup=${ptr} => ${err}`); continue } } @@ -595,18 +589,18 @@ class SPF { const re = new RegExp(`${domain.replace('.','\\.')}$`, 'i'); for (const name of names) { if (re.test(name)) { - self.log_debug(`mech_ptr: ${name} => ${domain}: MATCH!`); - return self.return_const(qualifier) + this.log_debug(`mech_ptr: ${name} => ${domain}: MATCH!`); + return this.return_const(qualifier) } else { - self.log_debug(`mech_ptr: ${name} => ${domain}: NO MATCH`); + this.log_debug(`mech_ptr: ${name} => ${domain}: NO MATCH`); } } - return self.SPF_NONE + return this.SPF_NONE } catch (e) { - self.log_debug('mech_ptr', { domain: self.domain, err: e.message }); - return self.SPF_PERMERROR + this.log_debug('mech_ptr', { domain: this.domain, err: e.message }); + return this.SPF_PERMERROR } } diff --git a/package.json b/package.json index f756373..b7cd9ed 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "haraka-plugin-spf", - "version": "1.2.1", + "version": "1.2.2", "description": "Sender Policy Framework (SPF) plugin for Haraka", "main": "index.js", "scripts": {