From a270f4686eb2d5318f6026712230810640ab58f9 Mon Sep 17 00:00:00 2001 From: Matt Simerson Date: Sun, 14 Apr 2024 13:41:11 -0700 Subject: [PATCH] transaction init consistency (#3315) - transaction: always use conn.init_transaction, always pass in cfg #3315 - NOTICE: remove a handful of 3.0 sunset property names #3315 - outbound: pass in config when initiating txn - outbound: minor es6 updates --- .release | 2 +- Changes.md | 18 +++++++++++---- connection.js | 22 ++---------------- outbound/index.js | 35 +++++++++++----------------- package.json | 2 +- plugins.js | 4 ++-- tests/connection.js | 36 ++++------------------------- tests/fixtures/util_hmailitem.js | 8 ++----- tests/plugins/queue/smtp_forward.js | 2 +- tests/plugins/spamassassin.js | 2 +- tests/transaction.js | 12 +++------- transaction.js | 29 ++++++----------------- 12 files changed, 51 insertions(+), 121 deletions(-) diff --git a/.release b/.release index 7cd5707f7..36bb27a93 160000 --- a/.release +++ b/.release @@ -1 +1 @@ -Subproject commit 7cd5707f7d69f8d4dca1ec407ada911890e59d0a +Subproject commit 36bb27a93862517943e04f24fd67b0df2da6cbbe diff --git a/Changes.md b/Changes.md index 92848fee2..b2132fec4 100644 --- a/Changes.md +++ b/Changes.md @@ -1,26 +1,34 @@ ### Unreleased -#### Changed +#### Added - doc: add CONTRIBUTORS #3312 + +#### Changed + +- transaction: init with conn.init_transaction, always pass in cfg #3315 - check for local_mx only when default route is used #3307 - test: add a connection.response test case with DSN #3305 - deps: bump all versions to latest #3303 - auth_base: enable disabling constrain_sender at runtime #3298 - connection: support IPv6 when setting remote.is_private #3295 + - in setTLS, replace forEach with for...of + - NOTE: remove a handful of 3.0 sunset property names #3315 - outbound/mx_lookup: make it async/await -- outbound: emit log message when ignoring local MX +- outbound: emit log message when ignoring local MX #3285 +- outbound: pass in config when initiating txn #3315 +- outbound: minor es6 updates #3315 #### Fixed - fix(bin/haraka): list NPM installed plugin #3310 - fix(bin/haraka): get hook list from doc/Plugins #3306 -- fix(outbound): call cb even if no MX is found -- fix(helo.checks): declare reject.literal_mismatch as boolean +- fix(outbound): call cb even if no MX is found #3294 +- fix(helo.checks): declare reject.literal_mismatch as boolean #3293 - fix(outbound): allow LHLO over insecure socket if TLS is forced #3278 - fix(outbound): include return path param SMTPUTF8 when required #3289 -- fix(outbound): replace empty Message-ID header +- fix(outbound): replace empty Message-ID header #3288 - fix(outbound): don't send SNI servername when connecting to an IP - fix(outbound): chown queue dir after creation #3291 diff --git a/connection.js b/connection.js index f15541bf9..af631c7fc 100644 --- a/connection.js +++ b/connection.js @@ -223,11 +223,10 @@ class Connection { setTLS (obj) { this.set('hello', 'host', undefined); this.set('tls', 'enabled', true); - const options = ['cipher','verified','verifyError','peerCertificate']; - options.forEach(t => { + for (const t of ['cipher','verified','verifyError','peerCertificate']) { if (obj[t] === undefined) return; this.set('tls', t, obj[t]); - }) + } // prior to 2017-07, authorized and verified were both used. Verified // seems to be the more common and has the property updated in the // tls object. However, authorized has been up-to-date in the notes. Store @@ -272,22 +271,6 @@ class Connection { this.set('remote.is_private', net_utils.is_private_ip(this.remote.ip)); } } - - // sunset 3.0.0 - if (prop_str === 'hello.verb') { - this.greeting = val; - } - else if (prop_str === 'tls.enabled') { - this.using_tls = val; - } - else if (prop_str === 'proxy.ip') { - this.haproxy_ip = val; - } - else { - const legacy_name = prop_str.split('.').join('_'); - this[legacy_name] = val; - } - // /sunset } get (prop_str) { return prop_str.split('.').reduce((prev, curr) => { @@ -638,7 +621,6 @@ class Connection { this.client.end(); } get_capabilities () { - return []; } tran_uuid () { diff --git a/outbound/index.js b/outbound/index.js index c50f8bc54..d00f38170 100644 --- a/outbound/index.js +++ b/outbound/index.js @@ -24,21 +24,20 @@ const _qfile = exports.qfile = require('./qfile'); const { queue_dir, temp_fail_queue, delivery_queue } = queuelib; +const smtp_ini = config.get('smtp.ini', { booleans: [ '+headers.add_received' ] }) + exports.temp_fail_queue = temp_fail_queue; exports.delivery_queue = delivery_queue; exports.net_utils = net_utils; exports.config = config; -exports.get_stats = queuelib.get_stats; -exports.list_queue = queuelib.list_queue; -exports.stat_queue = queuelib.stat_queue; -exports.scan_queue_pids = queuelib.scan_queue_pids; -exports.flush_queue = queuelib.flush_queue; -exports.load_pid_queue = queuelib.load_pid_queue; -exports.ensure_queue_dir = queuelib.ensure_queue_dir; -exports.load_queue = queuelib.load_queue; -exports.stats = queuelib.stats; +const qlfns = ['get_stats', 'list_queue', 'stat_queue', 'scan_queue_pids', 'flush_queue', + 'load_pid_queue', 'ensure_queue_dir', 'load_queue', 'stats' +] +for (const n of qlfns) { + exports[n] = queuelib[n]; +} process.on('message', msg => { if (!msg.event) return @@ -59,30 +58,24 @@ process.on('message', msg => { // ignores the message }); -exports.send_email = function () { +exports.send_email = function (from, to, contents, next, options = {}) { if (arguments.length === 2) { logger.logdebug("[outbound] Sending email as a transaction"); return this.send_trans_email(arguments[0], arguments[1]); } - let from = arguments[0]; - let to = arguments[1]; - let contents = arguments[2]; - const next = arguments[3]; - const options = arguments[4] || {}; - - const dot_stuffed = options.dot_stuffed ? options.dot_stuffed : false; - const notes = options.notes ? options.notes : null; - const origin = options.origin ? options.origin : null; + const dot_stuffed = options.dot_stuffed ?? false; + const notes = options.notes ?? null; + const origin = options.origin ?? null; logger.loginfo("[outbound] Sending email via params", origin); - const transaction = trans.createTransaction(); + const transaction = trans.createTransaction(null, smtp_ini); logger.loginfo(`[outbound] Created transaction: ${transaction.uuid}`, origin); - //Adding notes passed as parameter + // Adding notes passed as parameter if (notes) { transaction.notes = notes; } diff --git a/package.json b/package.json index e3406a7d3..59b6dc077 100644 --- a/package.json +++ b/package.json @@ -74,7 +74,7 @@ }, "devDependencies": { "nodeunit-x": "^0.16.0", - "haraka-test-fixtures": "^1.3.3", + "haraka-test-fixtures": "^1.3.7", "mock-require": "^3.0.3", "eslint-plugin-haraka": "^1.0.15", "nodemailer": "^6.9.13" diff --git a/plugins.js b/plugins.js index 261fe926a..34dce5920 100644 --- a/plugins.js +++ b/plugins.js @@ -312,7 +312,7 @@ plugins.load_plugins = override => { plugin_list = exports.config.get('plugins', 'list'); } - plugin_list.forEach(plugin => { + for (let plugin of plugin_list) { if (plugin.startsWith('haraka-plugin-')) plugin = plugin.substr(14) if (plugins.deprecated[plugin]) { logger.lognotice(`the plugin ${plugin} has been replaced by '${plugins.deprecated[plugin]}'. Please update config/plugins`) @@ -321,7 +321,7 @@ plugins.load_plugins = override => { else { plugins.load_plugin(plugin); } - }); + } plugins.plugin_list = Object.keys(plugins.registered_plugins); diff --git a/tests/connection.js b/tests/connection.js index d37ca0272..d2037a594 100644 --- a/tests/connection.js +++ b/tests/connection.js @@ -28,7 +28,7 @@ function _set_up (done) { exports.connectionRaw = { setUp : _set_up, 'has remote object' (test) { - test.expect(5); + test.expect(1); test.deepEqual(this.connection.remote, { ip: null, port: null, @@ -38,33 +38,23 @@ exports.connectionRaw = { is_private: false, is_local: false }); - // backwards compat, sunset v3.0.0 - test.equal(this.connection.remote_ip, null); - test.equal(this.connection.remote_port, null); - test.equal(this.connection.remote_host, null); - test.equal(this.connection.remote_info, null); test.done(); }, 'has local object' (test) { - test.expect(5); + test.expect(3); test.equal(this.connection.local.ip, null); test.equal(this.connection.local.port, null); test.ok(this.connection.local.host, this.connection.local.host); - // backwards compat, sunset v3.0.0 - test.equal(this.connection.local_ip, null); - test.equal(this.connection.local_port, null); test.done(); }, 'has tls object' (test) { - test.expect(2); + test.expect(1); test.deepEqual(this.connection.tls, { enabled: false, advertised: false, verified: false, cipher: {}, }); - // backwards compat, sunset v3.0.0 - test.equal(this.connection.using_tls, false); test.done(); }, 'get_capabilities' (test) { @@ -109,18 +99,6 @@ exports.connectionRaw = { test.equal('', this.connection.queue_msg('hello')); test.done(); }, - 'has legacy connection properties' (test) { - test.expect(4); - this.connection.set('remote', 'ip', '172.16.15.1'); - this.connection.set('hello', 'verb', 'EHLO'); - this.connection.set('tls', 'enabled', true); - - test.equal('172.16.15.1', this.connection.remote_ip); - test.equal(null, this.connection.remote_port); - test.equal('EHLO', this.connection.greeting); - test.equal(true, this.connection.using_tls); - test.done(); - }, 'has normalized connection properties' (test) { test.expect(5); this.connection.set('remote', 'ip', '172.16.15.1'); @@ -140,12 +118,6 @@ exports.connectionRaw = { test.equal(false, this.connection.remote.is_local); test.done(); }, - 'has legacy proxy property set' (test) { - test.expect(1); - this.connection.set('proxy', 'ip', '172.16.15.1'); - test.equal('172.16.15.1', this.connection.haproxy_ip); - test.done(); - }, 'has normalized proxy properties, default' (test) { test.expect(4); test.equal(false, this.connection.proxy.allowed); @@ -355,4 +327,4 @@ exports.respond = { ); test.done(); }, -} \ No newline at end of file +} diff --git a/tests/fixtures/util_hmailitem.js b/tests/fixtures/util_hmailitem.js index 403345121..659a9d718 100644 --- a/tests/fixtures/util_hmailitem.js +++ b/tests/fixtures/util_hmailitem.js @@ -2,9 +2,6 @@ const { Address } = require('address-rfc2821'); const fixtures = require('haraka-test-fixtures'); -const stub_connection = fixtures.connection; -// const transaction = fixtures.transaction; // not yet sufficient -const transaction = require('../../transaction'); /** @@ -51,8 +48,8 @@ exports.createHMailItem = (outbound_context, options, callback) => { const delivery_domain = options.delivery_domain || 'domain'; const mail_recipients = options.mail_recipients || [new Address('recipient@domain')]; - const conn = stub_connection.createConnection(); - conn.transaction = transaction.createTransaction('someuuid'); + const conn = fixtures.connection.createConnection(); + conn.init_transaction() conn.transaction.mail_from = new Address(mail_from); const todo = new outbound_context.TODOItem(delivery_domain, mail_recipients, conn.transaction); @@ -144,7 +141,6 @@ exports.playTestSmtpConversation = (hmail, socket, test, playbook, callback) => const welcome = getNextEntryFromPlaybook('remote', playbook); socket.emit('line', welcome.line); - } function getNextEntryFromPlaybook (ofType, playbook) { diff --git a/tests/plugins/queue/smtp_forward.js b/tests/plugins/queue/smtp_forward.js index ab488c853..237594421 100644 --- a/tests/plugins/queue/smtp_forward.js +++ b/tests/plugins/queue/smtp_forward.js @@ -18,7 +18,7 @@ function _setup (done) { this.hmail = { todo: { notes: new Notes() } }; this.connection = new fixtures.connection.createConnection(); - this.connection.transaction = new fixtures.transaction.createTransaction(); + this.connection.init_transaction(); done(); } diff --git a/tests/plugins/spamassassin.js b/tests/plugins/spamassassin.js index b4ef87bcb..8cea9a3e6 100644 --- a/tests/plugins/spamassassin.js +++ b/tests/plugins/spamassassin.js @@ -14,7 +14,7 @@ function _set_up (done) { }; this.connection = fixtures.connection.createConnection(); - this.connection.transaction = fixtures.transaction.createTransaction() + this.connection.init_transaction() done(); } diff --git a/tests/transaction.js b/tests/transaction.js index 7c87eb304..f3e513004 100644 --- a/tests/transaction.js +++ b/tests/transaction.js @@ -1,20 +1,16 @@ const fs = require('fs'); const path = require('path'); +const config = require('haraka-config') const transaction = require('../transaction'); function _set_up (done) { - this.transaction = transaction.createTransaction(); - done(); -} - -function _tear_down (done) { - done(); + this.transaction = transaction.createTransaction(undefined, config.get('smtp.ini')); + done() } exports.transaction = { setUp : _set_up, - tearDown : _tear_down, 'add_body_filter' (test) { @@ -180,7 +176,6 @@ function write_file_data_to_transaction (test_transaction, filename) { exports.base64_handling = { setUp : _set_up, - tearDown: _tear_down, 'varied-base64-fold-lengths-preserve-data' (test) { @@ -230,7 +225,6 @@ exports.base64_handling = { // Expected: --abcd exports.boundarymarkercorrupt_test = { setUp : _set_up, - tearDown: _tear_down, // populate the same email data in transaction (self.transaction.add_data()) and // in raw buffer, then compare diff --git a/transaction.js b/transaction.js index f924e12b5..b02ce06b3 100644 --- a/transaction.js +++ b/transaction.js @@ -1,20 +1,16 @@ 'use strict'; // An SMTP Transaction -// node.js built-in modules const util = require('util'); -// haraka npm modules const Notes = require('haraka-notes'); const utils = require('haraka-utils'); - -// Haraka modules const message = require('haraka-email-message') class Transaction { - constructor (uuid, cfg) { + constructor (uuid, cfg = {}) { this.uuid = uuid || utils.uuid(); - this.cfg = cfg || load_smtp_ini(); + this.cfg = cfg; this.mail_from = null; this.rcpt_to = []; this.header_lines = []; @@ -50,14 +46,13 @@ class Transaction { this.body = new message.Body(this.header); this.body.on('mime_boundary', m => this.incr_mime_count()); - this.attachment_start_hooks.forEach(h => { - this.body.on('attachment_start', h); - }); - if (this.banner) { - this.body.set_banner(this.banner); + for (const hook of this.attachment_start_hooks) { + this.body.on('attachment_start', hook); } + if (this.banner) this.body.set_banner(this.banner); + for (const o of this.body_filters) { this.body.add_filter((ct, enc, buf) => { const re_match = (util.types.isRegExp(o.ct_match) && o.ct_match.test(ct.toLowerCase())); @@ -147,7 +142,7 @@ class Transaction { } else if (this.header_pos === 0) { // Build up headers - if (this.header_lines.length < this.cfg.headers.max_lines) { + if (this.header_lines.length < (this.cfg?.headers?.max_lines || 1000)) { if (line[0] === 0x2E) line = line.slice(1); // Strip leading '.' this.header_lines.push(line.toString(this.encoding).replace(/\r\n$/, '\n')); } @@ -255,13 +250,3 @@ exports.Transaction = Transaction; exports.createTransaction = (uuid, cfg) => { return new Transaction(uuid, cfg); } - -// sunset after test-fixtures createTransaction() is updated to pass in cfg -function load_smtp_ini () { - const config = require('haraka-config'); - const cfg = config.get('smtp.ini', { booleans: [ '+headers.add_received' ] }); - if (!cfg.headers.max_lines) { - cfg.headers.max_lines = parseInt(config.get('max_header_lines')) || 1000; - } - return cfg; -}