From a85d4cd873f517cc573b05fe391f647538e161a1 Mon Sep 17 00:00:00 2001 From: Andris Reinman Date: Sat, 15 Aug 2026 19:27:50 +0300 Subject: [PATCH 1/2] feat(dns): accept a promise-based custom DNS resolver The only way to supply a resolver was dnsOptions.resolve, which takes a callback, and which omits the record type entirely for A lookups. Everything inside the library has been async/await for a while, so the callback was promisified straight back into a promise, and the arity quirk had to be preserved because resolvers written against it may only accept the two-argument shape. dnsOptions.resolveAsync is the same job without either wart. It receives a domain and a record type, A lookups included, and returns the records. Returning them directly is as acceptable as returning a promise, since awaiting covers both, so a resolver backed by a cache no longer has to pretend to be asynchronous; a synchronous throw surfaces as a rejection for the same reason. resolve keeps working exactly as before, including its two-argument A lookups. When both are set resolveAsync wins and the callback is not consulted, so a deployment can migrate one option at a time. getDnsResolver now takes the whole dnsOptions object rather than one function, since it has three sources to choose between and every call site already had the object to hand. It is internal, not exported from the package. The name is resolveAsync rather than resolver because the latter differs from resolve by one character, and a typo between them would silently select a different code path. --- CLAUDE.md | 4 +- README.md | 66 +++++++++++++++++++---- lib/mx-connect.js | 6 ++- lib/resolve-ip.js | 5 +- lib/resolve-mx.js | 5 +- lib/tools.js | 79 ++++++++++++++------------- test/mx-connect-test.js | 115 ++++++++++++++++++++++++++++++++++++++++ test/tools-test.js | 98 ++++++++++++++++++++++++++++++++-- 8 files changed, 321 insertions(+), 57 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 30ff0ad..e4df0b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,7 +75,9 @@ formatAddress -> resolvePolicy -> resolveMX -> validateMxPolicy -> resolveIP -> **MTA-STS integration:** Uses `mailauth` library for policy fetching and MX validation. Policies are cached via user-provided cache handlers. -**Custom DNS resolvers:** The library accepts callback-style custom resolvers via `dnsOptions.resolve`. These are automatically promisified internally. When no custom resolver is provided, native `dns.promises` is used. +**Custom DNS resolvers:** `tools.getDnsResolver(dnsOptions)` builds the one resolver the whole pipeline uses, choosing between three sources: `dnsOptions.resolveAsync` returns the records, either directly or as a promise; `dnsOptions.resolve` is the older callback form and is promisified; with neither set, native `dns.promises` is used. + +`resolveAsync` always receives an explicit record type. The callback form keeps its quirk of omitting the type for A lookups, since resolvers written against it may only accept the two-argument shape, and that wart is what `resolveAsync` exists to leave behind. When both options are set, `resolveAsync` wins. **Async conventions:** All asynchronous code uses async/await. Raw promise primitives are limited to justified boundaries: `new Promise` wrappers where callback-style user APIs (custom DNS resolvers, `connectHook`) or event-based APIs (`net.connect` with timeout race) meet async code, `Promise.all` for parallel fan-out, and the `.then()` bridge in `mx-connect.js` that feeds the public callback API. diff --git a/README.md b/README.md index 3f36337..139f66d 100644 --- a/README.md +++ b/README.md @@ -85,16 +85,62 @@ The family-specific options let you bind a different source address per family. ### dnsOptions -| Option | Type | Default | Description | -| ----------------------- | -------- | ------------- | ----------------------------------------------------------------------------------------------- | -| `ignoreIPv6` | boolean | `false` | Never use IPv6 for sending. See [below](#ignoreipv6) | -| `preferIPv6` | boolean | `false` | Try IPv6 addresses before IPv4 when a host has both | -| `blockLocalAddresses` | boolean | `false` | Refuse local and private scope addresses. See [Address validation](#address-validation) | -| `blockReservedNetworks` | boolean | `false` | Refuse IANA special-purpose addresses. See [Address validation](#address-validation) | -| `nat64Prefixes` | string[] | `[]` | NAT64 prefixes your own network runs. See [NAT64 on your own prefix](#nat64-on-your-own-prefix) | -| `resolve` | function | `dns.resolve` | Custom callback-style DNS resolver | - -A custom `resolve` function is called as `resolve(domain, type, callback)`, except for A record lookups which always use `resolve(domain, callback)`. Other record types (`MX`, `AAAA`, `TXT`) always pass the type. +| Option | Type | Default | Description | +| ----------------------- | -------- | ------- | ----------------------------------------------------------------------------------------------- | +| `ignoreIPv6` | boolean | `false` | Never use IPv6 for sending. See [below](#ignoreipv6) | +| `preferIPv6` | boolean | `false` | Try IPv6 addresses before IPv4 when a host has both | +| `blockLocalAddresses` | boolean | `false` | Refuse local and private scope addresses. See [Address validation](#address-validation) | +| `blockReservedNetworks` | boolean | `false` | Refuse IANA special-purpose addresses. See [Address validation](#address-validation) | +| `nat64Prefixes` | string[] | `[]` | NAT64 prefixes your own network runs. See [NAT64 on your own prefix](#nat64-on-your-own-prefix) | +| `resolveAsync` | function | | DNS resolver returning the records. See [Custom DNS resolver](#custom-dns-resolver) | +| `resolve` | function | | Callback-style DNS resolver. See [Custom DNS resolver](#custom-dns-resolver) | + +With neither set, native `dns.promises` is used. + +#### Custom DNS resolver + +`resolveAsync` receives a domain and a record type and returns the records: + +```javascript +const connection = await mxConnect({ + target: 'user@example.com', + dnsOptions: { + async resolveAsync(domain, type) { + // type is 'MX', 'A', 'AAAA' or 'TXT' + return myResolver.lookup(domain, type); + } + } +}); +``` + +Returning the records directly is fine too, so a resolver backed by a cache does not have to pretend to be asynchronous: + +```javascript +const dnsOptions = { + resolveAsync: (domain, type) => cache.get(`${domain}:${type}`) ?? [] +}; +``` + +Throwing, or returning a rejected promise, is how you report a lookup failure. Set `err.code` to `ENOTFOUND` or `ENODATA` to say "no records of this type", which lets resolution fall through to the next step; any other code is treated as a real DNS failure. + +> [!TIP] +> `resolveAsync` always receives an explicit record type, A lookups included. + +The older `resolve` option takes a callback and is still supported: + +```javascript +const dnsOptions = { + resolve(domain, type, callback) { + // A lookups arrive as resolve(domain, callback), with no type + myResolver.lookup(domain, type, callback); + } +}; +``` + +It is called as `resolve(domain, type, callback)`, except for A records where it is called as `resolve(domain, callback)` with no type at all. That quirk is why `resolveAsync` exists; prefer it for new code. + +> [!NOTE] +> If both are set, `resolveAsync` is used and `resolve` is ignored, so you can migrate one deployment at a time. #### ignoreIPv6 diff --git a/lib/mx-connect.js b/lib/mx-connect.js index ef4a0ff..3096600 100644 --- a/lib/mx-connect.js +++ b/lib/mx-connect.js @@ -56,7 +56,7 @@ const EMPTY_CACHE_HANDLER = { */ function createPolicyResolver(delivery) { const dnsOptions = delivery.dnsOptions || {}; - const dnsResolve = tools.getDnsResolver(dnsOptions.resolve); + const dnsResolve = tools.getDnsResolver(dnsOptions); return async (domain, type) => { // mailauth falls back to AAAA when the A lookup comes back empty, which would @@ -558,7 +558,9 @@ async function runPipeline(delivery) { * @param {boolean} [options.dnsOptions.blockReservedNetworks=false] - Block IANA special-purpose IPs * @param {Array} [options.dnsOptions.nat64Prefixes] - NAT64 prefixes this network runs, * in CIDR form, so the IPv4 address such an address carries is validated too - * @param {Function} [options.dnsOptions.resolve] - Custom DNS resolver + * @param {Function} [options.dnsOptions.resolveAsync] - DNS resolver returning records, or a + * promise for them: (domain, type) => records + * @param {Function} [options.dnsOptions.resolve] - Callback-based DNS resolver (legacy) * @param {Array} [options.mx] - Pre-resolved MX entries (skips DNS MX lookup) * @param {Array} [options.ignoreMXHosts] - IP addresses to skip * @param {Function} [options.connectHook] - Pre-connection hook: (delivery, options, callback) diff --git a/lib/resolve-ip.js b/lib/resolve-ip.js index a9088c7..888e0c6 100644 --- a/lib/resolve-ip.js +++ b/lib/resolve-ip.js @@ -100,7 +100,8 @@ async function diagnoseSkippedIPv6(skipped, dnsResolve, filterAddress) { * @param {Array} delivery.mx - Array of MX entries with exchange hostnames * @param {Object} [delivery.dnsOptions] - DNS configuration options * @param {boolean} [delivery.dnsOptions.ignoreIPv6=false] - Skip AAAA lookups - * @param {Function} [delivery.dnsOptions.resolve] - Custom DNS resolver + * @param {Function} [delivery.dnsOptions.resolveAsync] - Promise-based DNS resolver + * @param {Function} [delivery.dnsOptions.resolve] - Callback-based DNS resolver (legacy) * @returns {Promise} Delivery object with populated A/AAAA arrays * @throws {Error} If no valid IP addresses can be resolved (error.category = 'dns') */ @@ -142,7 +143,7 @@ async function resolveIP(delivery) { }; const dnsOptions = delivery.dnsOptions || {}; - const dnsResolve = tools.getDnsResolver(dnsOptions.resolve); + const dnsResolve = tools.getDnsResolver(dnsOptions); // Hosts whose AAAA lookup ignoreIPv6 skipped, kept only to explain a failed delivery const skippedIPv6 = []; diff --git a/lib/resolve-mx.js b/lib/resolve-mx.js index 603e12c..820cfd8 100644 --- a/lib/resolve-mx.js +++ b/lib/resolve-mx.js @@ -135,7 +135,8 @@ async function tryResolve(dnsResolve, domain, type) { * @param {boolean} delivery.isIp - True if target is already an IP address * @param {Object} [delivery.dnsOptions] - DNS configuration options * @param {boolean} [delivery.dnsOptions.ignoreIPv6=false] - Skip AAAA record lookup - * @param {Function} [delivery.dnsOptions.resolve] - Custom DNS resolver + * @param {Function} [delivery.dnsOptions.resolveAsync] - Promise-based DNS resolver + * @param {Function} [delivery.dnsOptions.resolve] - Callback-based DNS resolver (legacy) * @returns {Promise} Delivery object with populated mx array * @throws {Error} If no valid MX servers can be resolved (error.category = 'dns') */ @@ -189,7 +190,7 @@ async function resolveMX(delivery) { } const domain = delivery.decodedDomain; - const dnsResolve = tools.getDnsResolver(dnsOptions.resolve); + const dnsResolve = tools.getDnsResolver(dnsOptions); // Step 1: Try MX records (canonical mail server entries) const mxResult = await tryResolve(dnsResolve, domain, 'MX'); diff --git a/lib/tools.js b/lib/tools.js index 27d534f..e6cb4a1 100644 --- a/lib/tools.js +++ b/lib/tools.js @@ -16,52 +16,59 @@ const { promises: dnsPromises } = require('dns'); const LOCAL_ADDRESSES = collectLocalAddresses(os.networkInterfaces()); /** - * Creates a promise-based DNS resolver function. + * Builds the promise-based DNS resolver the pipeline uses. * - * When no custom resolver is provided, uses native dns.promises for optimal performance. - * When a custom callback-style resolver is provided (via dnsOptions.resolve), wraps it - * with promisification to maintain a consistent async interface. + * Three sources, in order of preference: * - * The returned resolver accepts an optional record type. An omitted type and an - * explicit 'A' are equivalent: both resolve A records (IPv4). For custom - * resolvers, A lookups always use the legacy two-argument form - * (domain, callback), so resolvers that only implement that form keep working. + * 1. `dnsOptions.resolveAsync`, which returns the records. It may return them directly or + * as a promise; awaiting covers both, so a resolver backed by a cache can answer + * synchronously without pretending to be asynchronous. + * 2. `dnsOptions.resolve`, the older callback form, promisified here. + * 3. Native `dns.promises`, which avoids the callback hop entirely. * - * @param {Function} [customResolver] - Optional callback-style DNS resolver with signature - * (domain, callback) or (domain, type, callback). If not provided, uses dns.promises. + * The record type is always passed on to `resolveAsync`, A records included. The callback + * form keeps its quirk of omitting the type for A lookups, because resolvers written + * against it may only accept the two-argument shape, and that is precisely the wart the + * newer option exists to leave behind. + * + * @param {Object} [dnsOptions] - DNS configuration + * @param {Function} [dnsOptions.resolveAsync] - (domain, type) => records or Promise + * @param {Function} [dnsOptions.resolve] - Callback resolver, (domain, type, callback) or + * (domain, callback) for A records * @returns {Function} Promise-based resolver: (domain, type?) => Promise * * @example - * // Using default resolver - * const resolve = getDnsResolver(); + * const resolve = getDnsResolver({ resolveAsync: (domain, type) => myCache.lookup(domain, type) }); * const mxRecords = await resolve('example.com', 'MX'); - * - * @example - * // Using custom resolver - * const resolve = getDnsResolver(myCustomDnsResolve); - * const ipAddresses = await resolve('example.com'); */ -function getDnsResolver(customResolver) { - // Use native dns.promises when no custom resolver - faster and avoids callback overhead - if (!customResolver) { - return (domain, type) => { - if (type === undefined || type === 'A') { - return dnsPromises.resolve4(domain); - } - return dnsPromises.resolve(domain, type); - }; +function getDnsResolver(dnsOptions) { + const options = dnsOptions || {}; + + if (typeof options.resolveAsync === 'function') { + // async so a resolver that returns records directly, or throws synchronously, still + // behaves like every other branch here + return async (domain, type) => options.resolveAsync(domain, type || 'A'); } - // Promisify custom callback-style resolver - return (domain, type) => - new Promise((resolve, reject) => { - const callback = (err, data) => (err ? reject(err) : resolve(data)); - if (type === undefined || type === 'A') { - customResolver(domain, callback); - } else { - customResolver(domain, type, callback); - } - }); + if (typeof options.resolve === 'function') { + return (domain, type) => + new Promise((resolve, reject) => { + const callback = (err, data) => (err ? reject(err) : resolve(data)); + if (type === undefined || type === 'A') { + options.resolve(domain, callback); + } else { + options.resolve(domain, type, callback); + } + }); + } + + // Native dns.promises - faster, and avoids the callback hop + return (domain, type) => { + if (type === undefined || type === 'A') { + return dnsPromises.resolve4(domain); + } + return dnsPromises.resolve(domain, type); + }; } /** diff --git a/test/mx-connect-test.js b/test/mx-connect-test.js index 4f3b262..1336d3e 100644 --- a/test/mx-connect-test.js +++ b/test/mx-connect-test.js @@ -853,3 +853,118 @@ test('endToEndOverRealSocket', async () => { await closeServer(server); }); + +test('resolveAsyncDrivesTheWholePipeline', async () => { + // The promise-based resolver has to serve every lookup the delivery makes, not just + // the MX one, and a synchronous return is as acceptable as a promise + const calls = []; + const records = { + 'async.example.com:MX': [{ exchange: 'mail.example.com', priority: 10 }], + 'mail.example.com:A': ['192.0.2.1'] + }; + + const connection = await mxConnect({ + target: 'async.example.com', + dnsOptions: { + resolveAsync(domain, type) { + calls.push(`${domain}:${type}`); + const answer = records[`${domain}:${type}`]; + if (!answer) { + const err = new Error('ENODATA'); + err.code = 'ENODATA'; + throw err; + } + return answer; + } + }, + connectHook(delivery, options, callback) { + options.socket = createMockSocket({ remoteAddress: options.host }); + return callback(); + } + }); + + assert.strictEqual(connection.host, '192.0.2.1'); + assert.ok(calls.includes('async.example.com:MX'), 'The MX lookup should go through resolveAsync'); + assert.ok(calls.includes('mail.example.com:A'), 'Address lookups should go through it too'); + assert.ok(!calls.some(call => call.endsWith(':undefined')), 'Every lookup should name its record type'); +}); + +test('resolveAsyncCoversTheMtaStsPolicyLookup', async () => { + // The policy host is resolved before any MX record is considered, and it must use the + // caller's resolver like everything else. Falling back to system DNS there would both + // ignore the configuration and reach a different answer than the rest of the delivery. + const calls = []; + const records = { + '_mta-sts.sts-async.example.com:TXT': [['v=STSv1; id=async1']], + 'mail.example.com:A': ['192.0.2.1'] + }; + + const connection = await mxConnect({ + target: 'sts-async.example.com', + mx: ['mail.example.com'], + mtaSts: { enabled: true }, + dnsOptions: { + resolveAsync(domain, type) { + calls.push(`${domain}:${type}`); + const answer = records[`${domain}:${type}`]; + if (!answer) { + const err = new Error('ENOTFOUND'); + err.code = 'ENOTFOUND'; + throw err; + } + return answer; + } + }, + connectHook(delivery, options, callback) { + options.socket = createMockSocket({ remoteAddress: options.host }); + return callback(); + } + }); + + assert.ok(connection.socket); + assert.ok(calls.includes('_mta-sts.sts-async.example.com:TXT'), 'The policy TXT lookup should use resolveAsync'); + assert.ok( + calls.some(call => call.startsWith('mta-sts.sts-async.example.com:')), + 'The policy host lookup should use resolveAsync' + ); +}); + +test('legacyResolveStillWorksAlongsideTheNewOption', async () => { + // The callback form keeps working untouched, including its two-argument A lookups + const arities = []; + const connection = await mxConnect({ + target: 'legacy.example.com', + dnsOptions: { + resolve(domain, typeOrCallback, maybeCallback) { + const twoArgForm = typeof typeOrCallback === 'function'; + const type = twoArgForm ? 'A' : typeOrCallback; + const callback = twoArgForm ? typeOrCallback : maybeCallback; + arities.push({ type, args: twoArgForm ? 2 : 3 }); + + if (type === 'MX') { + return setImmediate(() => callback(null, [{ exchange: 'mail.example.com', priority: 10 }])); + } + if (type === 'A') { + return setImmediate(() => callback(null, ['192.0.2.3'])); + } + const err = new Error('ENODATA'); + err.code = 'ENODATA'; + return setImmediate(() => callback(err)); + } + }, + connectHook(delivery, options, callback) { + options.socket = createMockSocket({ remoteAddress: options.host }); + return callback(); + } + }); + + assert.strictEqual(connection.host, '192.0.2.3'); + assert.ok( + arities.some(call => call.type === 'A' && call.args === 2), + 'A lookups must keep using the two-argument callback form' + ); + assert.ok( + arities.some(call => call.type === 'MX' && call.args === 3), + 'Other record types keep the three-argument form' + ); +}); diff --git a/test/tools-test.js b/test/tools-test.js index 982a821..3b27965 100644 --- a/test/tools-test.js +++ b/test/tools-test.js @@ -16,7 +16,7 @@ test('getDnsResolverWithCustomResolver', async () => { setImmediate(() => callback(null, ['192.0.2.1'])); }; - const resolver = tools.getDnsResolver(customResolver); + const resolver = tools.getDnsResolver({ resolve: customResolver }); try { // Test with type argument @@ -45,7 +45,7 @@ test('getDnsResolverWithCustomResolverError', async () => { setImmediate(() => callback(err)); }; - const resolver = tools.getDnsResolver(customResolver); + const resolver = tools.getDnsResolver({ resolve: customResolver }); try { await resolver('fail.example.com', 'MX'); @@ -57,7 +57,7 @@ test('getDnsResolverWithCustomResolverError', async () => { test('getDnsResolverWithoutCustomResolver', async () => { // When no custom resolver provided, should use native dns.promises - const resolver = tools.getDnsResolver(null); + const resolver = tools.getDnsResolver(); // Just verify it returns a function assert.strictEqual(typeof resolver, 'function'); @@ -285,7 +285,7 @@ test('getDnsResolverExplicitATypeUsesLegacyForm', async () => { setImmediate(() => callback(null, ['192.0.2.1'])); }; - const resolver = tools.getDnsResolver(customResolver); + const resolver = tools.getDnsResolver({ resolve: customResolver }); try { const aRecords = await resolver('example.com', 'A'); @@ -428,3 +428,93 @@ test('isInvalidNat64PrefixesNotAnArray', async () => { assert.ok(tools.isInvalid(options, '127.0.0.1'), 'ordinary validation must be undisturbed'); } }); + +test('getDnsResolverResolveAsyncReturningPromise', async () => { + // The newer option returns the records rather than taking a callback + const calls = []; + const resolver = tools.getDnsResolver({ + async resolveAsync(domain, type) { + calls.push({ domain, type }); + return ['192.0.2.1']; + } + }); + + assert.deepStrictEqual(await resolver('example.com', 'MX'), ['192.0.2.1']); + assert.deepStrictEqual(calls, [{ domain: 'example.com', type: 'MX' }]); +}); + +test('getDnsResolverResolveAsyncMayBeSynchronous', async () => { + // Returning the records directly is allowed, so a resolver backed by a cache does not + // have to pretend to be asynchronous + const resolver = tools.getDnsResolver({ + resolveAsync: () => ['192.0.2.2'] + }); + + assert.deepStrictEqual(await resolver('example.com', 'A'), ['192.0.2.2']); +}); + +test('getDnsResolverResolveAsyncAlwaysReceivesAType', async () => { + // The callback form omits the type for A lookups, which is the wart this option exists + // to leave behind, so an A lookup arrives here named like every other + const types = []; + const resolver = tools.getDnsResolver({ + resolveAsync: (domain, type) => { + types.push(type); + return []; + } + }); + + await resolver('example.com'); + await resolver('example.com', 'A'); + await resolver('example.com', 'AAAA'); + + assert.deepStrictEqual(types, ['A', 'A', 'AAAA'], 'An omitted type must reach the resolver as A'); +}); + +test('getDnsResolverResolveAsyncErrorsReject', async () => { + // Both a rejected promise and a synchronous throw have to surface as a rejection, or + // the resolution step cannot tell a failed lookup from an empty answer + const rejecting = tools.getDnsResolver({ + async resolveAsync() { + const err = new Error('SERVFAIL'); + err.code = 'SERVFAIL'; + throw err; + } + }); + await assert.rejects(() => rejecting('example.com', 'MX'), { code: 'SERVFAIL' }); + + const throwing = tools.getDnsResolver({ + resolveAsync() { + const err = new Error('SERVFAIL'); + err.code = 'SERVFAIL'; + throw err; + } + }); + await assert.rejects(() => throwing('example.com', 'MX'), { code: 'SERVFAIL' }, 'A synchronous throw must reject too'); +}); + +test('getDnsResolverPrefersResolveAsyncOverResolve', async () => { + // Both may be set while a caller migrates. The newer one wins, and the callback is + // left alone rather than being called as well. + let callbackUsed = false; + const resolver = tools.getDnsResolver({ + resolveAsync: () => ['from-async'], + resolve: (domain, typeOrCallback, maybeCallback) => { + callbackUsed = true; + const callback = typeof typeOrCallback === 'function' ? typeOrCallback : maybeCallback; + return setImmediate(() => callback(null, ['from-callback'])); + } + }); + + assert.deepStrictEqual(await resolver('example.com', 'MX'), ['from-async']); + assert.strictEqual(callbackUsed, false, 'The callback resolver must not be consulted as well'); +}); + +test('getDnsResolverIgnoresNonFunctionResolvers', async () => { + // A mistyped option must fall through to the next source rather than throwing from + // inside every lookup + for (const bad of [null, 'nope', 42, {}, []]) { + const resolver = tools.getDnsResolver({ resolveAsync: bad, resolve: undefined }); + assert.strictEqual(typeof resolver, 'function', `resolveAsync of type ${typeof bad} should fall through`); + } +}); From 44e35987476456f90280e6a16a271cf6d353da90 Mon Sep 17 00:00:00 2001 From: Andris Reinman Date: Sat, 15 Aug 2026 19:44:42 +0300 Subject: [PATCH 2/2] refactor(dns): rename resolveAsync to resolveRecords, refuse uncallable resolvers Renamed before it ships, on review, so nothing released is affected. "Async" named the wrong axis: both resolver options are asynchronous, and ironically the suffixed one is the only one allowed to answer synchronously. It also collided with the meaning the suffix already carries in this codebase, where resolveTlsaAsync is an internally promisified callback while the public promise-based options, dane.resolveTlsa and dane.checkDnssecSecure, take no suffix at all. resolveRecords follows those, and says what the function does. Two findings from the security review are fixed with it. A resolver option set to something uncallable used to throw from inside every lookup; the typeof guard added with the new option turned that into a silent fall back to the system resolver. For an MTA that deliberately configures a resolver, say a DNSSEC-validating one, a typo would have quietly sent mail through a resolver nobody chose, with nothing to notice. Both options are now refused up front with a TypeError naming the offender. Unset, or explicitly null, still falls through. The MTA-STS policy filter called .filter on whatever the resolver returned, so an answer supplying its own filter method chose what survived validation, and mailauth connects to whatever comes back. That was the only unvalidated address the review could get near a socket. The filter now runs only over a real array. Also folded the "an omitted type means A" rule, which had spread to four places, into a single default parameter, so the legacy two-argument A lookup lives on one line of one branch. tryResolve no longer needs its ternary and names A explicitly. --- CLAUDE.md | 6 +- README.md | 18 +++-- lib/mx-connect.js | 12 ++- lib/resolve-ip.js | 2 +- lib/resolve-mx.js | 8 +- lib/tools.js | 38 +++++---- test/mx-connect-test.js | 171 +++++++++++++++++++++------------------- test/test-utils.js | 30 +++++++ test/tools-test.js | 45 ++++++++--- 9 files changed, 201 insertions(+), 129 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e4df0b5..5f6d5e3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,7 +64,7 @@ formatAddress -> resolvePolicy -> resolveMX -> validateMxPolicy -> resolveIP -> - `resolve-mx.js` - Async DNS MX record resolution with fallback to A/AAAA records - `resolve-ip.js` - Async resolution of MX hostnames to IPv4/IPv6 addresses (parallel) - `get-connection.js` - Async iteration through MX hosts attempting TCP connections -- `tools.js` - Shared utilities: `getDnsResolver` (promisifies custom DNS resolvers or uses native `dns.promises`), `isNotFoundError`, IP validation (`isLocal`, `isInvalid`) +- `tools.js` - Shared utilities: `getDnsResolver` (selects the DNS resolver, see below), `isNotFoundError`, IP validation (`isLocal`, `isInvalid`, `checkAddress`) - `dns-errors.js` / `net-errors.js` - Error code to message mappings **Key data structure:** The `delivery` object flows through the pipeline, accumulating: @@ -75,9 +75,7 @@ formatAddress -> resolvePolicy -> resolveMX -> validateMxPolicy -> resolveIP -> **MTA-STS integration:** Uses `mailauth` library for policy fetching and MX validation. Policies are cached via user-provided cache handlers. -**Custom DNS resolvers:** `tools.getDnsResolver(dnsOptions)` builds the one resolver the whole pipeline uses, choosing between three sources: `dnsOptions.resolveAsync` returns the records, either directly or as a promise; `dnsOptions.resolve` is the older callback form and is promisified; with neither set, native `dns.promises` is used. - -`resolveAsync` always receives an explicit record type. The callback form keeps its quirk of omitting the type for A lookups, since resolvers written against it may only accept the two-argument shape, and that wart is what `resolveAsync` exists to leave behind. When both options are set, `resolveAsync` wins. +**Custom DNS resolvers:** `tools.getDnsResolver(dnsOptions)` builds the one resolver the whole pipeline uses, choosing between `dnsOptions.resolveRecords`, the older callback-style `dnsOptions.resolve`, and native `dns.promises`. The user-facing contract for both options, including why the callback form still omits the record type for A lookups, is documented in the README under Custom DNS resolver. TLSA lookups do not go through here; they have their own `dane.resolveTlsa`. **Async conventions:** All asynchronous code uses async/await. Raw promise primitives are limited to justified boundaries: `new Promise` wrappers where callback-style user APIs (custom DNS resolvers, `connectHook`) or event-based APIs (`net.connect` with timeout race) meet async code, `Promise.all` for parallel fan-out, and the `.then()` bridge in `mx-connect.js` that feeds the public callback API. diff --git a/README.md b/README.md index 139f66d..65cd11b 100644 --- a/README.md +++ b/README.md @@ -92,20 +92,20 @@ The family-specific options let you bind a different source address per family. | `blockLocalAddresses` | boolean | `false` | Refuse local and private scope addresses. See [Address validation](#address-validation) | | `blockReservedNetworks` | boolean | `false` | Refuse IANA special-purpose addresses. See [Address validation](#address-validation) | | `nat64Prefixes` | string[] | `[]` | NAT64 prefixes your own network runs. See [NAT64 on your own prefix](#nat64-on-your-own-prefix) | -| `resolveAsync` | function | | DNS resolver returning the records. See [Custom DNS resolver](#custom-dns-resolver) | +| `resolveRecords` | function | | DNS resolver returning the records. See [Custom DNS resolver](#custom-dns-resolver) | | `resolve` | function | | Callback-style DNS resolver. See [Custom DNS resolver](#custom-dns-resolver) | With neither set, native `dns.promises` is used. #### Custom DNS resolver -`resolveAsync` receives a domain and a record type and returns the records: +`resolveRecords` receives a domain and a record type and returns the records: ```javascript const connection = await mxConnect({ target: 'user@example.com', dnsOptions: { - async resolveAsync(domain, type) { + async resolveRecords(domain, type) { // type is 'MX', 'A', 'AAAA' or 'TXT' return myResolver.lookup(domain, type); } @@ -117,14 +117,14 @@ Returning the records directly is fine too, so a resolver backed by a cache does ```javascript const dnsOptions = { - resolveAsync: (domain, type) => cache.get(`${domain}:${type}`) ?? [] + resolveRecords: (domain, type) => cache.get(`${domain}:${type}`) ?? [] }; ``` Throwing, or returning a rejected promise, is how you report a lookup failure. Set `err.code` to `ENOTFOUND` or `ENODATA` to say "no records of this type", which lets resolution fall through to the next step; any other code is treated as a real DNS failure. > [!TIP] -> `resolveAsync` always receives an explicit record type, A lookups included. +> `resolveRecords` always receives an explicit record type, A lookups included. The older `resolve` option takes a callback and is still supported: @@ -137,10 +137,14 @@ const dnsOptions = { }; ``` -It is called as `resolve(domain, type, callback)`, except for A records where it is called as `resolve(domain, callback)` with no type at all. That quirk is why `resolveAsync` exists; prefer it for new code. +It is called as `resolve(domain, type, callback)`, except for A records where it is called as `resolve(domain, callback)` with no type at all. That quirk is why `resolveRecords` exists; prefer it for new code. > [!NOTE] -> If both are set, `resolveAsync` is used and `resolve` is ignored, so you can migrate one deployment at a time. +> If both are set, `resolveRecords` is used and `resolve` is ignored, so you can migrate one deployment at a time. + +Setting either option to something that is not a function throws, rather than quietly falling back to the system resolver. A mistyped option would otherwise send mail through a resolver you did not choose, losing whatever yours was there for, and nothing would say so. Leaving an option unset, or setting it to `null`, is not a mistake and falls through as normal. + +Neither option covers TLSA lookups for DANE, which have their own [`dane.resolveTlsa`](#custom-tlsa-resolver). Wiring up a single resolver here does not route those through it. #### ignoreIPv6 diff --git a/lib/mx-connect.js b/lib/mx-connect.js index 3096600..861cbb7 100644 --- a/lib/mx-connect.js +++ b/lib/mx-connect.js @@ -72,7 +72,15 @@ function createPolicyResolver(delivery) { return list; } - return (list || []).filter(ip => { + // Anything but a real array is not an answer this can vet. A custom resolver + // returning an object with its own filter method would otherwise choose what + // survives, and mailauth connects to whatever comes back, so the check has to run + // over an array we recognise rather than one the value supplies. + if (!Array.isArray(list)) { + return []; + } + + return list.filter(ip => { const invalid = tools.checkAddress(delivery, domain, ip); if (!invalid) { return true; @@ -558,7 +566,7 @@ async function runPipeline(delivery) { * @param {boolean} [options.dnsOptions.blockReservedNetworks=false] - Block IANA special-purpose IPs * @param {Array} [options.dnsOptions.nat64Prefixes] - NAT64 prefixes this network runs, * in CIDR form, so the IPv4 address such an address carries is validated too - * @param {Function} [options.dnsOptions.resolveAsync] - DNS resolver returning records, or a + * @param {Function} [options.dnsOptions.resolveRecords] - DNS resolver returning records, or a * promise for them: (domain, type) => records * @param {Function} [options.dnsOptions.resolve] - Callback-based DNS resolver (legacy) * @param {Array} [options.mx] - Pre-resolved MX entries (skips DNS MX lookup) diff --git a/lib/resolve-ip.js b/lib/resolve-ip.js index 888e0c6..67bd76f 100644 --- a/lib/resolve-ip.js +++ b/lib/resolve-ip.js @@ -100,7 +100,7 @@ async function diagnoseSkippedIPv6(skipped, dnsResolve, filterAddress) { * @param {Array} delivery.mx - Array of MX entries with exchange hostnames * @param {Object} [delivery.dnsOptions] - DNS configuration options * @param {boolean} [delivery.dnsOptions.ignoreIPv6=false] - Skip AAAA lookups - * @param {Function} [delivery.dnsOptions.resolveAsync] - Promise-based DNS resolver + * @param {Function} [delivery.dnsOptions.resolveRecords] - Promise-based DNS resolver * @param {Function} [delivery.dnsOptions.resolve] - Callback-based DNS resolver (legacy) * @returns {Promise} Delivery object with populated A/AAAA arrays * @throws {Error} If no valid IP addresses can be resolved (error.category = 'dns') diff --git a/lib/resolve-mx.js b/lib/resolve-mx.js index 820cfd8..65c2140 100644 --- a/lib/resolve-mx.js +++ b/lib/resolve-mx.js @@ -105,13 +105,13 @@ function isRecoverableError(err) { * * @param {Function} dnsResolve - Promise-based DNS resolver function * @param {string} domain - Domain to resolve - * @param {string} [type] - Record type (MX, AAAA). Omit for A records. + * @param {string} type - Record type (MX, A, AAAA) * @returns {Promise<{list: Array, error: Error|null}>} Resolution result with list and error * @private */ async function tryResolve(dnsResolve, domain, type) { try { - const list = type !== undefined ? await dnsResolve(domain, type) : await dnsResolve(domain); + const list = await dnsResolve(domain, type); return { list: list || [], error: null }; } catch (err) { return { list: [], error: err }; @@ -135,7 +135,7 @@ async function tryResolve(dnsResolve, domain, type) { * @param {boolean} delivery.isIp - True if target is already an IP address * @param {Object} [delivery.dnsOptions] - DNS configuration options * @param {boolean} [delivery.dnsOptions.ignoreIPv6=false] - Skip AAAA record lookup - * @param {Function} [delivery.dnsOptions.resolveAsync] - Promise-based DNS resolver + * @param {Function} [delivery.dnsOptions.resolveRecords] - Promise-based DNS resolver * @param {Function} [delivery.dnsOptions.resolve] - Callback-based DNS resolver (legacy) * @returns {Promise} Delivery object with populated mx array * @throws {Error} If no valid MX servers can be resolved (error.category = 'dns') @@ -226,7 +226,7 @@ async function resolveMX(delivery) { } // Step 2: Fallback to A records (RFC 5321 Section 5.1 implicit MX) - const aResult = await tryResolve(dnsResolve, domain); + const aResult = await tryResolve(dnsResolve, domain, 'A'); if (aResult.list.length) { // RFC 5321 Section 5.1: the domain itself is a single implicit mail exchanger, so all // of its addresses belong to one entry. Giving each address an entry of its own would diff --git a/lib/tools.js b/lib/tools.js index e6cb4a1..c8ff933 100644 --- a/lib/tools.js +++ b/lib/tools.js @@ -20,41 +20,52 @@ const LOCAL_ADDRESSES = collectLocalAddresses(os.networkInterfaces()); * * Three sources, in order of preference: * - * 1. `dnsOptions.resolveAsync`, which returns the records. It may return them directly or + * 1. `dnsOptions.resolveRecords`, which returns the records. It may return them directly or * as a promise; awaiting covers both, so a resolver backed by a cache can answer * synchronously without pretending to be asynchronous. * 2. `dnsOptions.resolve`, the older callback form, promisified here. * 3. Native `dns.promises`, which avoids the callback hop entirely. * - * The record type is always passed on to `resolveAsync`, A records included. The callback - * form keeps its quirk of omitting the type for A lookups, because resolvers written - * against it may only accept the two-argument shape, and that is precisely the wart the + * An omitted record type means A, which is defaulted once here so each branch below can + * just read it. Only the callback form still omits the type for A lookups, because + * resolvers written against it may accept nothing else, and that is precisely the wart the * newer option exists to leave behind. * * @param {Object} [dnsOptions] - DNS configuration - * @param {Function} [dnsOptions.resolveAsync] - (domain, type) => records or Promise + * @param {Function} [dnsOptions.resolveRecords] - (domain, type) => records or Promise * @param {Function} [dnsOptions.resolve] - Callback resolver, (domain, type, callback) or * (domain, callback) for A records * @returns {Function} Promise-based resolver: (domain, type?) => Promise + * @throws {TypeError} If a resolver option is set to something that cannot be called * * @example - * const resolve = getDnsResolver({ resolveAsync: (domain, type) => myCache.lookup(domain, type) }); + * const resolve = getDnsResolver({ resolveRecords: (domain, type) => myCache.lookup(domain, type) }); * const mxRecords = await resolve('example.com', 'MX'); */ function getDnsResolver(dnsOptions) { const options = dnsOptions || {}; - if (typeof options.resolveAsync === 'function') { + // Falling back to the system resolver because an option was mistyped would quietly send + // mail through a resolver the operator did not choose, losing whatever the configured + // one was there for. Say so instead. + for (const name of ['resolveRecords', 'resolve']) { + if (options[name] !== undefined && options[name] !== null && typeof options[name] !== 'function') { + throw new TypeError(`dnsOptions.${name} must be a function, got ${typeof options[name]}`); + } + } + + if (typeof options.resolveRecords === 'function') { // async so a resolver that returns records directly, or throws synchronously, still // behaves like every other branch here - return async (domain, type) => options.resolveAsync(domain, type || 'A'); + return async (domain, type = 'A') => options.resolveRecords(domain, type); } if (typeof options.resolve === 'function') { - return (domain, type) => + return (domain, type = 'A') => new Promise((resolve, reject) => { const callback = (err, data) => (err ? reject(err) : resolve(data)); - if (type === undefined || type === 'A') { + // The legacy quirk, contained to this one line + if (type === 'A') { options.resolve(domain, callback); } else { options.resolve(domain, type, callback); @@ -63,12 +74,7 @@ function getDnsResolver(dnsOptions) { } // Native dns.promises - faster, and avoids the callback hop - return (domain, type) => { - if (type === undefined || type === 'A') { - return dnsPromises.resolve4(domain); - } - return dnsPromises.resolve(domain, type); - }; + return (domain, type = 'A') => (type === 'A' ? dnsPromises.resolve4(domain) : dnsPromises.resolve(domain, type)); } /** diff --git a/test/mx-connect-test.js b/test/mx-connect-test.js index 1336d3e..87e85aa 100644 --- a/test/mx-connect-test.js +++ b/test/mx-connect-test.js @@ -4,7 +4,15 @@ const { test } = require('node:test'); const assert = require('node:assert'); const mxConnect = require('../lib/mx-connect'); -const { createMockDnsResolver, createTrackingDnsResolver, createMockSocket, startGreetingServer, closeServer } = require('./test-utils'); +const { + createMockDnsResolver, + createTrackingDnsResolver, + createMockRecordResolver, + createMockConnectHook, + createMockSocket, + startGreetingServer, + closeServer +} = require('./test-utils'); test('basicWithMock', (t, done) => { const mockResolver = createMockDnsResolver({ @@ -854,117 +862,114 @@ test('endToEndOverRealSocket', async () => { await closeServer(server); }); -test('resolveAsyncDrivesTheWholePipeline', async () => { - // The promise-based resolver has to serve every lookup the delivery makes, not just - // the MX one, and a synchronous return is as acceptable as a promise - const calls = []; - const records = { - 'async.example.com:MX': [{ exchange: 'mail.example.com', priority: 10 }], - 'mail.example.com:A': ['192.0.2.1'] - }; +test('resolveRecordsDrivesTheWholePipeline', async () => { + // The promise-based resolver has to serve every lookup the delivery makes, not just the + // MX one, and each has to arrive with its record type named + const { resolver, calls } = createMockRecordResolver({ + 'async.example.com:MX': { data: [{ exchange: 'mail.example.com', priority: 10 }] }, + 'mail.example.com:A': { data: ['192.0.2.1'] } + }); const connection = await mxConnect({ target: 'async.example.com', - dnsOptions: { - resolveAsync(domain, type) { - calls.push(`${domain}:${type}`); - const answer = records[`${domain}:${type}`]; - if (!answer) { - const err = new Error('ENODATA'); - err.code = 'ENODATA'; - throw err; - } - return answer; - } - }, - connectHook(delivery, options, callback) { - options.socket = createMockSocket({ remoteAddress: options.host }); - return callback(); - } + dnsOptions: { resolveRecords: resolver }, + connectHook: createMockConnectHook() }); assert.strictEqual(connection.host, '192.0.2.1'); - assert.ok(calls.includes('async.example.com:MX'), 'The MX lookup should go through resolveAsync'); + assert.ok(calls.includes('async.example.com:MX'), 'The MX lookup should go through resolveRecords'); assert.ok(calls.includes('mail.example.com:A'), 'Address lookups should go through it too'); assert.ok(!calls.some(call => call.endsWith(':undefined')), 'Every lookup should name its record type'); }); -test('resolveAsyncCoversTheMtaStsPolicyLookup', async () => { +test('resolveRecordsCoversTheMtaStsPolicyLookup', async () => { // The policy host is resolved before any MX record is considered, and it must use the // caller's resolver like everything else. Falling back to system DNS there would both // ignore the configuration and reach a different answer than the rest of the delivery. - const calls = []; - const records = { - '_mta-sts.sts-async.example.com:TXT': [['v=STSv1; id=async1']], - 'mail.example.com:A': ['192.0.2.1'] - }; + const { resolver, calls } = createMockRecordResolver({ + '_mta-sts.sts-async.example.com:TXT': { data: [['v=STSv1; id=async1']] }, + 'mail.example.com:A': { data: ['192.0.2.1'] } + }); const connection = await mxConnect({ target: 'sts-async.example.com', mx: ['mail.example.com'], mtaSts: { enabled: true }, - dnsOptions: { - resolveAsync(domain, type) { - calls.push(`${domain}:${type}`); - const answer = records[`${domain}:${type}`]; - if (!answer) { - const err = new Error('ENOTFOUND'); - err.code = 'ENOTFOUND'; - throw err; - } - return answer; - } - }, - connectHook(delivery, options, callback) { - options.socket = createMockSocket({ remoteAddress: options.host }); - return callback(); - } + dnsOptions: { resolveRecords: resolver }, + connectHook: createMockConnectHook() }); assert.ok(connection.socket); - assert.ok(calls.includes('_mta-sts.sts-async.example.com:TXT'), 'The policy TXT lookup should use resolveAsync'); + assert.ok(calls.includes('_mta-sts.sts-async.example.com:TXT'), 'The policy TXT lookup should use resolveRecords'); assert.ok( calls.some(call => call.startsWith('mta-sts.sts-async.example.com:')), - 'The policy host lookup should use resolveAsync' + 'The policy host lookup should use resolveRecords' ); }); -test('legacyResolveStillWorksAlongsideTheNewOption', async () => { - // The callback form keeps working untouched, including its two-argument A lookups - const arities = []; +test('legacyResolveStillHonouredAlongsideTheNewOption', async () => { + // Adding resolveRecords changed how the resolver is selected, so the callback option has + // to keep being picked up when it is the only one set. Its arity contract is pinned by + // mtaStsPolicyResolverUsesTwoArgumentAForm; this only guards the selection. + const { resolver, calls } = createTrackingDnsResolver({ + 'legacy.example.com:MX': { data: [{ exchange: 'mail.example.com', priority: 10 }] }, + 'mail.example.com:A': { data: ['192.0.2.3'] } + }); + const connection = await mxConnect({ target: 'legacy.example.com', - dnsOptions: { - resolve(domain, typeOrCallback, maybeCallback) { - const twoArgForm = typeof typeOrCallback === 'function'; - const type = twoArgForm ? 'A' : typeOrCallback; - const callback = twoArgForm ? typeOrCallback : maybeCallback; - arities.push({ type, args: twoArgForm ? 2 : 3 }); - - if (type === 'MX') { - return setImmediate(() => callback(null, [{ exchange: 'mail.example.com', priority: 10 }])); - } - if (type === 'A') { - return setImmediate(() => callback(null, ['192.0.2.3'])); - } - const err = new Error('ENODATA'); - err.code = 'ENODATA'; - return setImmediate(() => callback(err)); - } - }, - connectHook(delivery, options, callback) { - options.socket = createMockSocket({ remoteAddress: options.host }); - return callback(); - } + dnsOptions: { resolve: resolver }, + connectHook: createMockConnectHook() }); assert.strictEqual(connection.host, '192.0.2.3'); - assert.ok( - arities.some(call => call.type === 'A' && call.args === 2), - 'A lookups must keep using the two-argument callback form' - ); - assert.ok( - arities.some(call => call.type === 'MX' && call.args === 3), - 'Other record types keep the three-argument form' - ); + assert.ok(calls.length > 0, 'The callback resolver must still be used when it is the only one set'); +}); + +test('mtaStsPolicyFilterOnlyTrustsRealArrays', async () => { + // mailauth connects to whatever the resolver returned, so the validation in between has + // to run over an array we recognise. An answer that supplies its own filter method would + // otherwise choose what survives and hand back an address nothing checked. + const https = require('https'); + const originalRequest = https.request; + const requestedHosts = []; + + https.request = options => { + requestedHosts.push(options.host); + throw new Error('no request should be made in this test'); + }; + + try { + const connection = await mxConnect({ + target: 'duck.example.com', + mx: ['mail.example.com'], + mtaSts: { enabled: true }, + dnsOptions: { + blockLocalAddresses: true, + resolveRecords(domain, type) { + if (type === 'TXT') { + return [['v=STSv1; id=duck1']]; + } + if (domain.startsWith('mta-sts.')) { + // Array-like, with a filter that would wave anything through + return { length: 1, 0: '127.0.0.1', filter: () => ['127.0.0.1'] }; + } + if (type === 'A') { + return ['192.0.2.1']; + } + const err = new Error('ENODATA'); + err.code = 'ENODATA'; + throw err; + } + }, + connectHook: createMockConnectHook() + }); + + assert.ok(connection.socket, 'Delivery should still proceed over the legitimate MX'); + assert.strictEqual(connection.host, '192.0.2.1'); + } finally { + https.request = originalRequest; + } + + assert.deepStrictEqual(requestedHosts, [], 'No policy fetch may be made to an address the filter never saw'); }); diff --git a/test/test-utils.js b/test/test-utils.js index ca8213b..ae298d1 100644 --- a/test/test-utils.js +++ b/test/test-utils.js @@ -94,6 +94,35 @@ function createTrackingDnsResolver(responses) { return { resolver, calls }; } +/** + * Creates a promise-based mock resolver for the dnsOptions.resolveRecords option, using the + * same response map as createMockDnsResolver. + * + * Returns { resolver, calls }, where calls holds `domain:type` strings in order. The + * resolver returns the records rather than taking a callback, and throws for a miss, which + * is how that option reports a lookup failure. + * + * @param {Object} responses - Map of `domain:type` (or bare domain) keys to { data } or { error } + */ +function createMockRecordResolver(responses) { + const calls = []; + + const resolver = (domain, type) => { + calls.push(`${domain}:${type}`); + + const response = responses[`${domain}:${type}`] || responses[domain]; + if (!response) { + throw createDnsError('ENOTFOUND'); + } + if (response.error) { + throw response.error; + } + return response.data; + }; + + return { resolver, calls }; +} + /** * Creates a DNS error with the specified code. */ @@ -178,6 +207,7 @@ module.exports = { getFreePort, createMockDnsResolver, createTrackingDnsResolver, + createMockRecordResolver, createDnsError, createMockSocket, createMockConnectHook, diff --git a/test/tools-test.js b/test/tools-test.js index 3b27965..007a719 100644 --- a/test/tools-test.js +++ b/test/tools-test.js @@ -433,7 +433,7 @@ test('getDnsResolverResolveAsyncReturningPromise', async () => { // The newer option returns the records rather than taking a callback const calls = []; const resolver = tools.getDnsResolver({ - async resolveAsync(domain, type) { + async resolveRecords(domain, type) { calls.push({ domain, type }); return ['192.0.2.1']; } @@ -447,7 +447,7 @@ test('getDnsResolverResolveAsyncMayBeSynchronous', async () => { // Returning the records directly is allowed, so a resolver backed by a cache does not // have to pretend to be asynchronous const resolver = tools.getDnsResolver({ - resolveAsync: () => ['192.0.2.2'] + resolveRecords: () => ['192.0.2.2'] }); assert.deepStrictEqual(await resolver('example.com', 'A'), ['192.0.2.2']); @@ -458,7 +458,7 @@ test('getDnsResolverResolveAsyncAlwaysReceivesAType', async () => { // to leave behind, so an A lookup arrives here named like every other const types = []; const resolver = tools.getDnsResolver({ - resolveAsync: (domain, type) => { + resolveRecords: (domain, type) => { types.push(type); return []; } @@ -475,7 +475,7 @@ test('getDnsResolverResolveAsyncErrorsReject', async () => { // Both a rejected promise and a synchronous throw have to surface as a rejection, or // the resolution step cannot tell a failed lookup from an empty answer const rejecting = tools.getDnsResolver({ - async resolveAsync() { + async resolveRecords() { const err = new Error('SERVFAIL'); err.code = 'SERVFAIL'; throw err; @@ -484,7 +484,7 @@ test('getDnsResolverResolveAsyncErrorsReject', async () => { await assert.rejects(() => rejecting('example.com', 'MX'), { code: 'SERVFAIL' }); const throwing = tools.getDnsResolver({ - resolveAsync() { + resolveRecords() { const err = new Error('SERVFAIL'); err.code = 'SERVFAIL'; throw err; @@ -498,7 +498,7 @@ test('getDnsResolverPrefersResolveAsyncOverResolve', async () => { // left alone rather than being called as well. let callbackUsed = false; const resolver = tools.getDnsResolver({ - resolveAsync: () => ['from-async'], + resolveRecords: () => ['from-async'], resolve: (domain, typeOrCallback, maybeCallback) => { callbackUsed = true; const callback = typeof typeOrCallback === 'function' ? typeOrCallback : maybeCallback; @@ -510,11 +510,32 @@ test('getDnsResolverPrefersResolveAsyncOverResolve', async () => { assert.strictEqual(callbackUsed, false, 'The callback resolver must not be consulted as well'); }); -test('getDnsResolverIgnoresNonFunctionResolvers', async () => { - // A mistyped option must fall through to the next source rather than throwing from - // inside every lookup - for (const bad of [null, 'nope', 42, {}, []]) { - const resolver = tools.getDnsResolver({ resolveAsync: bad, resolve: undefined }); - assert.strictEqual(typeof resolver, 'function', `resolveAsync of type ${typeof bad} should fall through`); +test('getDnsResolverRejectsUncallableResolvers', async () => { + // Quietly falling back to the system resolver because an option was mistyped would send + // mail through a resolver the operator did not choose, losing whatever theirs was there + // for, and nothing would say so. Both options are checked, so the mistake surfaces + // whichever one carries it. + for (const bad of ['nope', 42, {}, [], true]) { + assert.throws( + () => tools.getDnsResolver({ resolveRecords: bad }), + { name: 'TypeError', message: /resolveRecords must be a function/ }, + `resolveRecords of type ${typeof bad} must be refused` + ); + assert.throws( + () => tools.getDnsResolver({ resolve: bad }), + { name: 'TypeError', message: /resolve must be a function/ }, + `resolve of type ${typeof bad} must be refused` + ); } + + // An option left explicitly empty is not a mistake, it just means "not set" + for (const empty of [undefined, null]) { + const resolver = tools.getDnsResolver({ + resolveRecords: empty, + resolve: (domain, callback) => setImmediate(() => callback(null, ['fell-through'])) + }); + assert.deepStrictEqual(await resolver('example.com'), ['fell-through'], 'An unset resolveRecords must fall through to resolve'); + } + + assert.strictEqual(typeof tools.getDnsResolver({ resolveRecords: null, resolve: null }), 'function', 'Both unset falls through to the native resolver'); });