From ee8d00be7ccecff18e11d523cac8d2258f235f97 Mon Sep 17 00:00:00 2001 From: Petr Malimanek Date: Fri, 30 Dec 2016 00:47:25 +0100 Subject: [PATCH 1/6] Add utf7 decoding of labels --- lib/Connection.js | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Connection.js b/lib/Connection.js index 619862ff4..46ce06af1 100644 --- a/lib/Connection.js +++ b/lib/Connection.js @@ -1448,7 +1448,7 @@ Connection.prototype._resUntagged = function(info) { if (toget[i] === 'X-GM-LABELS') { var labels = info.text[keys[j]]; for (var k = 0, lenk = labels.length; k < lenk; ++k) - labels[k] = (''+labels[k]).replace(RE_ESCAPE, '\\'); + labels[k] = utf7.decode((''+labels[k]).replace(RE_ESCAPE, '\\')); } key = FETCH_ATTR_MAP[toget[i]]; if (!key) diff --git a/package.json b/package.json index 3f1445855..e2c528a90 100644 --- a/package.json +++ b/package.json @@ -1,5 +1,5 @@ { "name": "imap", - "version": "0.8.19", + "version": "0.8.20", "author": "Brian White ", "description": "An IMAP module for node.js that makes communicating with IMAP servers easy", "main": "./lib/Connection", From 852283c9aa0490ec0c091335ba3c7bfabd7814b9 Mon Sep 17 00:00:00 2001 From: Petr Malimanek Date: Sun, 15 Mar 2020 10:23:59 +0100 Subject: [PATCH 2/6] Make @integromat/imap npm package --- .idea/$CACHE_FILE$ | 17 +++++++++++++++++ .idea/.gitignore | 2 ++ .idea/misc.xml | 6 ++++++ .idea/modules.xml | 8 ++++++++ .idea/node-imap.iml | 8 ++++++++ .idea/scopes | 6 ++++++ .idea/vagrant.xml | 7 +++++++ .idea/vcs.xml | 28 ++++++++++++++++++++++++++++ package.json | 6 +++--- 9 files changed, 85 insertions(+), 3 deletions(-) create mode 100644 .idea/$CACHE_FILE$ create mode 100644 .idea/.gitignore create mode 100644 .idea/misc.xml create mode 100644 .idea/modules.xml create mode 100644 .idea/node-imap.iml create mode 100644 .idea/scopes create mode 100644 .idea/vagrant.xml create mode 100644 .idea/vcs.xml diff --git a/.idea/$CACHE_FILE$ b/.idea/$CACHE_FILE$ new file mode 100644 index 000000000..3a1e22c78 --- /dev/null +++ b/.idea/$CACHE_FILE$ @@ -0,0 +1,17 @@ + + + + + + + + + + + Angular + + + + + + \ No newline at end of file diff --git a/.idea/.gitignore b/.idea/.gitignore new file mode 100644 index 000000000..5c98b4288 --- /dev/null +++ b/.idea/.gitignore @@ -0,0 +1,2 @@ +# Default ignored files +/workspace.xml \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml new file mode 100644 index 000000000..28a804d89 --- /dev/null +++ b/.idea/misc.xml @@ -0,0 +1,6 @@ + + + + + \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml new file mode 100644 index 000000000..5c0e0f485 --- /dev/null +++ b/.idea/modules.xml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/node-imap.iml b/.idea/node-imap.iml new file mode 100644 index 000000000..c956989b2 --- /dev/null +++ b/.idea/node-imap.iml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/scopes b/.idea/scopes new file mode 100644 index 000000000..a2df5f7cb --- /dev/null +++ b/.idea/scopes @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/.idea/vagrant.xml b/.idea/vagrant.xml new file mode 100644 index 000000000..a5aa78680 --- /dev/null +++ b/.idea/vagrant.xml @@ -0,0 +1,7 @@ + + + + + \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml new file mode 100644 index 000000000..f95c87a02 --- /dev/null +++ b/.idea/vcs.xml @@ -0,0 +1,28 @@ + + + + + + + + + \ No newline at end of file diff --git a/package.json b/package.json index e2c528a90..e2fb1f75c 100644 --- a/package.json +++ b/package.json @@ -1,5 +1,5 @@ -{ "name": "imap", - "version": "0.8.20", +{ "name": "@integromat/imap", + "version": "0.8.21", "author": "Brian White ", "description": "An IMAP module for node.js that makes communicating with IMAP servers easy", "main": "./lib/Connection", @@ -13,5 +13,5 @@ "engines": { "node": ">=0.8.0" }, "keywords": [ "imap", "mail", "email", "reader", "client" ], "licenses": [ { "type": "MIT", "url": "http://github.com/mscdex/node-imap/raw/master/LICENSE" } ], - "repository": { "type": "git", "url": "http://github.com/mscdex/node-imap.git" } + "repository": { "type": "git", "url": "https://github.com/integromat/node-imap.git" } } From f20a2bc4e7a62d8bc30a9b22ecf2db2f4513e480 Mon Sep 17 00:00:00 2001 From: Petr Malimanek Date: Sun, 15 Mar 2020 10:31:20 +0100 Subject: [PATCH 3/6] Add .gitignore --- .gitignore | 3 +++ .idea/$CACHE_FILE$ | 17 ----------------- .idea/.gitignore | 2 -- .idea/misc.xml | 6 ------ .idea/modules.xml | 8 -------- .idea/node-imap.iml | 8 -------- .idea/scopes | 6 ------ .idea/vagrant.xml | 7 ------- .idea/vcs.xml | 28 ---------------------------- 9 files changed, 3 insertions(+), 82 deletions(-) create mode 100644 .gitignore delete mode 100644 .idea/$CACHE_FILE$ delete mode 100644 .idea/.gitignore delete mode 100644 .idea/misc.xml delete mode 100644 .idea/modules.xml delete mode 100644 .idea/node-imap.iml delete mode 100644 .idea/scopes delete mode 100644 .idea/vagrant.xml delete mode 100644 .idea/vcs.xml diff --git a/.gitignore b/.gitignore new file mode 100644 index 000000000..84fae5498 --- /dev/null +++ b/.gitignore @@ -0,0 +1,3 @@ +.idea +.vscode +node_modules diff --git a/.idea/$CACHE_FILE$ b/.idea/$CACHE_FILE$ deleted file mode 100644 index 3a1e22c78..000000000 --- a/.idea/$CACHE_FILE$ +++ /dev/null @@ -1,17 +0,0 @@ - - - - - - - - - - - Angular - - - - - - \ No newline at end of file diff --git a/.idea/.gitignore b/.idea/.gitignore deleted file mode 100644 index 5c98b4288..000000000 --- a/.idea/.gitignore +++ /dev/null @@ -1,2 +0,0 @@ -# Default ignored files -/workspace.xml \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml deleted file mode 100644 index 28a804d89..000000000 --- a/.idea/misc.xml +++ /dev/null @@ -1,6 +0,0 @@ - - - - - \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml deleted file mode 100644 index 5c0e0f485..000000000 --- a/.idea/modules.xml +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - \ No newline at end of file diff --git a/.idea/node-imap.iml b/.idea/node-imap.iml deleted file mode 100644 index c956989b2..000000000 --- a/.idea/node-imap.iml +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - \ No newline at end of file diff --git a/.idea/scopes b/.idea/scopes deleted file mode 100644 index a2df5f7cb..000000000 --- a/.idea/scopes +++ /dev/null @@ -1,6 +0,0 @@ - - - - - - \ No newline at end of file diff --git a/.idea/vagrant.xml b/.idea/vagrant.xml deleted file mode 100644 index a5aa78680..000000000 --- a/.idea/vagrant.xml +++ /dev/null @@ -1,7 +0,0 @@ - - - - - \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml deleted file mode 100644 index f95c87a02..000000000 --- a/.idea/vcs.xml +++ /dev/null @@ -1,28 +0,0 @@ - - - - - - - - - \ No newline at end of file From 55f99c4422a7843beafafd9b2232381bd12b7e85 Mon Sep 17 00:00:00 2001 From: o-hushcha Date: Wed, 27 May 2026 00:44:55 +0300 Subject: [PATCH 4/6] Fix TypeError in parseFetch for integer-shaped key tokens Some IMAP servers emit FETCH responses where an even-indexed atom (key position) is an integer-shaped token (e.g. "* 1 FETCH (12345 NIL)" or as observed in the wild on the @integromat/imap consumer side via extension/quirk responses). convStr returns a JS number for such atoms, and parseFetch then calls .toLowerCase() on it, throwing: TypeError: list[i].toLowerCase is not a function at Parser.parseFetch (lib/Parser.js:431) Coerce the key token to a string at the extraction site. Value-position numeric atoms (RFC822.SIZE, MODSEQ, UIDNEXT, etc.) are untouched, so existing consumers that read those as numbers see no behavior change. Bump to 0.8.22. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/Parser.js | 7 ++++++- package.json | 2 +- test/test-parser.js | 9 +++++++++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/Parser.js b/lib/Parser.js index 16297d366..44740ea76 100644 --- a/lib/Parser.js +++ b/lib/Parser.js @@ -428,7 +428,12 @@ function parseFetch(text, literals, seqno) { var list = parseExpr(text, literals)[0], attrs = {}, m, body; // list is [KEY1, VAL1, KEY2, VAL2, .... KEYn, VALn] for (var i = 0, len = list.length, key, val; i < len; i += 2) { - key = list[i].toLowerCase(); + // Coerce the key token to a string before lowercasing. Some servers send + // integer-shaped atoms at key positions in malformed FETCH responses; + // convStr returns a JS number for those, and Number has no .toLowerCase. + // Coercing here keeps value-position numeric atoms (RFC822.SIZE, MODSEQ, + // etc.) untouched. + key = String(list[i]).toLowerCase(); val = list[i + 1]; if (key === 'envelope') val = parseFetchEnvelope(val); diff --git a/package.json b/package.json index e2fb1f75c..98368648c 100644 --- a/package.json +++ b/package.json @@ -1,5 +1,5 @@ { "name": "@integromat/imap", - "version": "0.8.21", + "version": "0.8.22", "author": "Brian White ", "description": "An IMAP module for node.js that makes communicating with IMAP servers easy", "main": "./lib/Connection", diff --git a/test/test-parser.js b/test/test-parser.js index 39d1723d1..01236a749 100644 --- a/test/test-parser.js +++ b/test/test-parser.js @@ -310,6 +310,15 @@ var CR = '\r', LF = '\n', CRLF = CR + LF; ], what: 'Untagged FETCH with non-body literal' }, + { source: ['* 1 FETCH (12345 NIL)', CRLF], + expected: [ { type: 'fetch', + num: 1, + textCode: undefined, + text: { '12345': null } + } + ], + what: 'Untagged FETCH with integer-shaped key token (regression: no TypeError on .toLowerCase)' + }, { source: ['* 12 FETCH (INTERNALDATE {2', '6}' + CRLF + '17-Jul-1996 02:44:25 -0700)' + CRLF], expected: [ { type: 'fetch', From 70f48f579e5672a647c160d0a42fcd4ccfc5271f Mon Sep 17 00:00:00 2001 From: Martin Zaloudek Date: Tue, 25 Aug 2026 12:05:54 +0200 Subject: [PATCH 5/6] fix: don't abort on LOGINDISABLED when XOAUTH2 is configured (#2) * fix: don't abort on LOGINDISABLED when XOAUTH2 is configured --- CHANGELOG.md | 45 +++++++++ lib/Connection.js | 18 ++-- package.json | 2 +- test/test-connection-logindisabled-login.js | 69 +++++++++++++ test/test-connection-logindisabled-xoauth2.js | 99 +++++++++++++++++++ 5 files changed, 226 insertions(+), 7 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 test/test-connection-logindisabled-login.js create mode 100644 test/test-connection-logindisabled-xoauth2.js diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 000000000..c50b98583 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,45 @@ +# Changelog + +Changes made in this fork. Versions up to and including 0.8.19 come from +upstream [mscdex/node-imap](https://github.com/mscdex/node-imap) and are not +listed here. + +## 0.8.23 + +- Fixed `_login()` aborting with `Logging in is disabled on this server` whenever + the server advertised `LOGINDISABLED`, even when an XOAUTH/XOAUTH2 token was + configured. Per RFC 3501, `LOGINDISABLED` only forbids the `LOGIN` command, so + the check now runs in the `LOGIN` branch only and the XOAUTH/XOAUTH2 + `AUTHENTICATE` branches are attempted first. This unblocks Exchange Online + mailboxes, which advertise `LOGINDISABLED` together with `AUTH=XOAUTH2` once + basic auth is disabled. + + References: + - [RFC 3501 §6.2.3](https://www.rfc-editor.org/rfc/rfc3501#section-6.2.3) — + "A client implementation MUST NOT send a LOGIN command if the LOGINDISABLED + capability is advertised." The restriction is scoped to `LOGIN`; nothing in + it applies to `AUTHENTICATE`. + - [Deprecation of Basic authentication in Exchange Online](https://learn.microsoft.com/en-us/exchange/clients-and-mobile-in-exchange-online/deprecation-of-basic-authentication-exchange-online) + — Microsoft "removed the ability to use Basic authentication in Exchange + Online for […] POP, IMAP […]" and it "is now disabled in all tenants", + with no way for admins or support to re-enable it. Such mailboxes answer + `CAPABILITY` with `LOGINDISABLED` and without `AUTH=PLAIN`, leaving + `AUTH=XOAUTH2` as the only usable mechanism. + - [Authenticate an IMAP, POP, or SMTP connection using OAuth](https://learn.microsoft.com/en-us/exchange/client-developer/legacy-protocols/how-to-authenticate-an-imap-pop-smtp-application-by-using-oauth) + — the XOAUTH2 flow Exchange Online expects instead. + - [mscdex/node-imap#685](https://github.com/mscdex/node-imap/issues/685) — + the same symptom reported upstream against Microsoft Exchange IMAP4. + +## 0.8.22 + +- Fixed `TypeError: list[i].toLowerCase is not a function` in `parseFetch()` when + a server emitted an integer-shaped token in a key position. Numeric values + (`RFC822.SIZE`, `MODSEQ`, `UIDNEXT`, …) are still parsed as numbers. + +## 0.8.21 + +- Published the fork as the scoped npm package `@integromat/imap`. + +## 0.8.20 + +- Added UTF-7 decoding of Gmail labels (`X-GM-LABELS`). diff --git a/lib/Connection.js b/lib/Connection.js index 46ce06af1..33a19cd70 100644 --- a/lib/Connection.js +++ b/lib/Connection.js @@ -1651,12 +1651,6 @@ Connection.prototype._login = function() { return; } - if (self.serverSupports('LOGINDISABLED')) { - err = new Error('Logging in is disabled on this server'); - err.source = 'authentication'; - return reentry(err); - } - var cmd; if (self.serverSupports('AUTH=XOAUTH') && self._config.xoauth) { self._caps = undefined; @@ -1672,6 +1666,18 @@ Connection.prototype._login = function() { cmd += ' ' + escape(self._config.xoauth2); self._enqueue(cmd, checkCaps); } else if (self._config.user && self._config.password) { + if (self.serverSupports('LOGINDISABLED')) { + // LOGINDISABLED (RFC 3501) only forbids the LOGIN command -- SASL + // mechanisms advertised as AUTH=* stay available. Servers that disable + // basic auth (e.g. Exchange Online) advertise LOGINDISABLED together + // with AUTH=XOAUTH2, so this check must not run before the AUTHENTICATE + // branches above. + + err = new Error('Logging in is disabled on this server'); + err.source = 'authentication'; + return reentry(err); + } + self._caps = undefined; self._enqueue('LOGIN "' + escape(self._config.user) + '" "' + escape(self._config.password) + '"', checkCaps); diff --git a/package.json b/package.json index 98368648c..c80631968 100644 --- a/package.json +++ b/package.json @@ -1,5 +1,5 @@ { "name": "@integromat/imap", - "version": "0.8.22", + "version": "0.8.23", "author": "Brian White ", "description": "An IMAP module for node.js that makes communicating with IMAP servers easy", "main": "./lib/Connection", diff --git a/test/test-connection-logindisabled-login.js b/test/test-connection-logindisabled-login.js new file mode 100644 index 000000000..2bce48294 --- /dev/null +++ b/test/test-connection-logindisabled-login.js @@ -0,0 +1,69 @@ +var assert = require('assert'), + net = require('net'), + Imap = require('../lib/Connection'); + +// When LOGIN is the only method the client can use, LOGINDISABLED must still +// abort the login sequence before any credentials are sent. + +var error; + +var CRLF = '\r\n'; + +var RESPONSES = [ + ['* CAPABILITY IMAP4rev1 LOGINDISABLED AUTH=XOAUTH2 NAMESPACE', + 'A0 OK CAPABILITY completed.', + '' + ].join(CRLF) +]; +var EXPECTED = [ + 'A0 CAPABILITY' +]; + +var exp = -1, + res = -1; + +var srv = net.createServer(function(sock) { + sock.write('* OK asdf\r\n'); + var buf = '', lines; + sock.on('data', function(data) { + buf += data.toString('utf8'); + if (buf.indexOf(CRLF) > -1) { + lines = buf.split(CRLF); + buf = lines.pop(); + lines.forEach(function(l) { + assert(l === EXPECTED[++exp], 'Unexpected client request: ' + l); + assert(RESPONSES[++res], 'No response for client request: ' + l); + sock.write(RESPONSES[res]); + }); + } + }); +}); +srv.listen(0, '127.0.0.1', function() { + var port = srv.address().port; + var imap = new Imap({ + user: 'foo', + password: 'bar', + host: '127.0.0.1', + port: port + }); + var timeout = setTimeout(function() { + assert(false, 'Timed out waiting for error'); + }, 2000); + imap.once('ready', function() { + clearTimeout(timeout); + assert(false, 'Unexpected successful login'); + }); + imap.once('error', function(err) { + clearTimeout(timeout); + error = err; + srv.close(); + }); + imap.connect(); +}); + +process.once('exit', function() { + assert(error, 'Expected an authentication error'); + assert.equal(error.message, 'Logging in is disabled on this server'); + assert.equal(error.source, 'authentication'); + assert.equal(exp, EXPECTED.length - 1, 'Credentials were sent to the server'); +}); diff --git a/test/test-connection-logindisabled-xoauth2.js b/test/test-connection-logindisabled-xoauth2.js new file mode 100644 index 000000000..a33e55e12 --- /dev/null +++ b/test/test-connection-logindisabled-xoauth2.js @@ -0,0 +1,99 @@ +var assert = require('assert'), + net = require('net'), + Imap = require('../lib/Connection'); + +// Servers that disable basic auth (e.g. Exchange Online) advertise +// LOGINDISABLED together with AUTH=XOAUTH2. LOGINDISABLED only forbids the +// LOGIN command (RFC 3501), so SASL authentication must still be attempted. + +var ready = false; + +var CRLF = '\r\n'; + +var XOAUTH2 = 'dXNlcj1mb29AZXhhbXBsZS5jb20BYXV0aD1CZWFyZXIgdG9rZW4BAQ=='; + +var CAPS = '* CAPABILITY IMAP4 IMAP4rev1 AUTH=XOAUTH2 LOGINDISABLED SASL-IR ' + + 'UIDPLUS MOVE ID UNSELECT CHILDREN IDLE NAMESPACE LITERAL+'; + +var RESPONSES = [ + [CAPS, + 'A0 OK CAPABILITY completed.', + '' + ].join(CRLF), + // Exchange sends no untagged CAPABILITY after AUTHENTICATE, which forces the + // client to re-fetch the capabilities before continuing. + ['A1 OK AUTHENTICATE completed.', + '' + ].join(CRLF), + [CAPS, + 'A2 OK CAPABILITY completed.', + '' + ].join(CRLF), + ['* NAMESPACE (("" "/")) NIL NIL', + 'A3 OK NAMESPACE completed.', + '' + ].join(CRLF), + ['* LIST (\\Noselect) "/" ""', + 'A4 OK LIST completed.', + '' + ].join(CRLF), + ['* BYE Microsoft Exchange Server IMAP4 server signing off.', + 'A5 OK LOGOUT completed.', + '' + ].join(CRLF) +]; +var EXPECTED = [ + 'A0 CAPABILITY', + 'A1 AUTHENTICATE XOAUTH2 ' + XOAUTH2, + 'A2 CAPABILITY', + 'A3 NAMESPACE', + 'A4 LIST "" ""', + 'A5 LOGOUT' +]; + +var exp = -1, + res = -1; + +var srv = net.createServer(function(sock) { + sock.write('* OK Microsoft Exchange IMAP4 service ready.\r\n'); + var buf = '', lines; + sock.on('data', function(data) { + buf += data.toString('utf8'); + if (buf.indexOf(CRLF) > -1) { + lines = buf.split(CRLF); + buf = lines.pop(); + lines.forEach(function(l) { + assert(l === EXPECTED[++exp], 'Unexpected client request: ' + l); + assert(RESPONSES[++res], 'No response for client request: ' + l); + sock.write(RESPONSES[res]); + }); + } + }); +}); +srv.listen(0, '127.0.0.1', function() { + var port = srv.address().port; + var imap = new Imap({ + xoauth2: XOAUTH2, + host: '127.0.0.1', + port: port + }); + var timeout = setTimeout(function() { + assert(false, 'Timed out waiting for ready'); + }, 2000); + imap.on('error', function(err) { + clearTimeout(timeout); + assert(false, 'Unexpected error: ' + err.message); + }); + imap.once('ready', function() { + clearTimeout(timeout); + ready = true; + srv.close(); + imap.end(); + }); + imap.connect(); +}); + +process.once('exit', function() { + assert(ready, 'Connection was not authenticated via XOAUTH2'); + assert.equal(exp, EXPECTED.length - 1, 'Not all expected commands were sent'); +}); From b479c5222ab22420230e8b77422cae8c412ef049 Mon Sep 17 00:00:00 2001 From: Martin Zaloudek Date: Wed, 26 Aug 2026 15:37:15 +0200 Subject: [PATCH 6/6] fix: send SNI on STARTTLS upgrade, drop dead polyfill import (#4) * fix: send SNI on STARTTLS upgrade, drop dead polyfill import --- CHANGELOG.md | 58 +++++++++++++ lib/Connection.js | 20 ++++- lib/Parser.js | 3 +- package.json | 4 +- test/clienthello.js | 66 +++++++++++++++ test/sni-harness.js | 117 +++++++++++++++++++++++++++ test/test-connection-starttls-sni.js | 36 +++++++++ test/test-connection-tls-sni-ip.js | 51 ++++++++++++ test/test-connection-tls-sni.js | 46 +++++++++++ 9 files changed, 393 insertions(+), 8 deletions(-) create mode 100644 test/clienthello.js create mode 100644 test/sni-harness.js create mode 100644 test/test-connection-starttls-sni.js create mode 100644 test/test-connection-tls-sni-ip.js create mode 100644 test/test-connection-tls-sni.js diff --git a/CHANGELOG.md b/CHANGELOG.md index c50b98583..f8d399696 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,64 @@ Changes made in this fork. Versions up to and including 0.8.19 come from upstream [mscdex/node-imap](https://github.com/mscdex/node-imap) and are not listed here. +## 0.8.24 + +Merged upstream [mscdex/node-imap](https://github.com/mscdex/node-imap) up to +`9918f08`, which is the last commit there (February 2022), and followed up on +the two gaps it left behind. + +- Fixed TLS connections not sending SNI at all. `tls.connect()` sends the + `server_name` extension only when `servername` is set explicitly — node never + derives it from `host`, which is used just to dial the connection and to check + the certificate identity. Servers that serve several domains from one address + (Gmail, Exchange Online, anything behind a load balancer) therefore answered + with a default certificate that does not match the requested host, surfacing as + `Error: self signed certificate` / `DEPTH_ZERO_SELF_SIGNED_CERT` and forcing + callers to pass `tlsOptions.servername` by hand. + + Imported from upstream `9918f08` for `connect()` (implicit TLS) and extended to + `_starttls()`, which builds its own `tlsOptions` and stayed uncovered upstream. + In both paths the assignment happens before the caller's `tlsOptions` are + copied over, so an explicit `tlsOptions.servername` still wins. + + The default is only derived from `host` when `host` is a name. + [RFC 6066 §3](https://www.rfc-editor.org/rfc/rfc6066#section-3) does not permit + IP addresses in `server_name`, and node warns about them (`DEP0123`, "This will + be ignored in a future version"), so defaulting one in would only trade a + missing extension for a deprecated one. An explicit `tlsOptions.servername` is + still passed through unchanged even when it is an IP — that call belongs to the + caller, not to this library. + + References: + - [mscdex/node-imap#724](https://github.com/mscdex/node-imap/issues/724) — + "Servername option is mandatory with gmail and Openssl 1.1.1", reporting + `Error: self signed certificate` against `imap.gmail.com` and the manual + `servername` workaround. + - [mscdex/node-imap#866](https://github.com/mscdex/node-imap/issues/866) — the + same `DEPTH_ZERO_SELF_SIGNED_CERT` symptom, misattributed to a MITM in the + upstream discussion. + - [RFC 6066 §3](https://www.rfc-editor.org/rfc/rfc6066#section-3) — defines + `server_name` and restricts it to host names, which is why the connection + host must be a name and not an IP for SNI to be meaningful. + +- Removed the dead `require('readable-stream')` fallback in `Parser.js`. Upstream + `7dbc664` dropped `readable-stream` from the dependencies but left the + `require('stream').Readable || require('readable-stream').Readable` fallback in + place. At runtime the `||` short-circuits, but bundlers (webpack, esbuild, ncc) + resolve `require()` statically and fail on the missing module. + +- Replaced the deprecated `Buffer` constructor with `Buffer.from()` / + `Buffer.allocUnsafe()` and the `'binary'` encoding alias with `'latin1'` + (upstream `7dbc664`). Both `allocUnsafe()` call sites overwrite the whole + buffer with `copy()` before reading it. The same commit dropped the + `readable-stream` dependency and raised `engines.node` to `>=10.0.0`. + +- Added regression tests that read the TLS handshake off the wire and assert the + client really announces the configured host, independent of the TLS version and + without certificate fixtures. They cover both the implicit TLS and the STARTTLS + path, and pin the IP behaviour down as well: no default for an IP host, but an + explicitly configured IP `servername` is still sent. + ## 0.8.23 - Fixed `_login()` aborting with `Logging in is disabled on this server` whenever diff --git a/lib/Connection.js b/lib/Connection.js index 3d2a98bbd..be9ea1540 100644 --- a/lib/Connection.js +++ b/lib/Connection.js @@ -1,5 +1,6 @@ var tls = require('tls'), Socket = require('net').Socket, + isIP = require('net').isIP, EventEmitter = require('events').EventEmitter, inherits = require('util').inherits, inspect = require('util').inspect, @@ -117,10 +118,18 @@ Connection.prototype.connect = function() { if (config.tls) { tlsOptions = {}; - // servername must be set to prevent issues with some imap server and openssl 1.1.1 - tlsOptions.servername = config.host; + if (!isIP(config.host)) { + // tls.connect() sends SNI only when servername is set -- it never derives it + // from host. Without it, servers that answer for several domains on one + // address hand back a default certificate that does not match the requested + // host. See mscdex/node-imap#724. + // + // Note: RFC 6066 does not permit IP addresses in SNI, so only default it for host + // names -- an explicit tlsOptions.servername below still wins either way. + tlsOptions.servername = config.host; + } tlsOptions.host = config.host; - // Host name may be overridden the tlsOptions + // Host name may be overridden by the tlsOptions for (var k in config.tlsOptions) tlsOptions[k] = config.tlsOptions[k]; tlsOptions.socket = socket; @@ -1708,8 +1717,11 @@ Connection.prototype._starttls = function() { var tlsOptions = {}; + if (!isIP(this._config.host)) { + tlsOptions.servername = this._config.host; + } tlsOptions.host = this._config.host; - // Host name may be overridden the tlsOptions + // Host name may be overridden by the tlsOptions for (var k in this._config.tlsOptions) tlsOptions[k] = this._config.tlsOptions[k]; tlsOptions.socket = self._sock; diff --git a/lib/Parser.js b/lib/Parser.js index d4379013e..857291eaa 100644 --- a/lib/Parser.js +++ b/lib/Parser.js @@ -1,6 +1,5 @@ var EventEmitter = require('events').EventEmitter, - ReadableStream = require('stream').Readable - || require('readable-stream').Readable, + ReadableStream = require('stream').Readable, inherits = require('util').inherits, inspect = require('util').inspect; diff --git a/package.json b/package.json index b121acaae..fb73fd92a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@integromat/imap", - "version": "0.8.23", - "author": "Brian White ", + "version": "0.8.24", + "author": "Brian White , Petr Malimanek, Oleksandr Hushcha, Martin Zaloudek", "description": "An IMAP module for node.js that makes communicating with IMAP servers easy", "main": "./lib/Connection", "dependencies": { diff --git a/test/clienthello.js b/test/clienthello.js new file mode 100644 index 000000000..2b05ad6dd --- /dev/null +++ b/test/clienthello.js @@ -0,0 +1,66 @@ +// Minimal TLS ClientHello reader used by the SNI tests. Working on the raw +// handshake bytes keeps the tests independent of the TLS/OpenSSL version the +// running node happens to be linked against: the server_name extension is sent +// in the clear by every TLS version, so a plain TCP server can observe it. +// +// Returns the requested host name, or null when the buffer holds no readable +// server_name extension -- including when it is truncated or malformed, which a +// reader of network data has to survive without throwing. + +// Bounds-checked reads. Both yield -1 instead of reading past the end, so a +// short buffer ends the walk instead of raising RangeError. +function u8(buf, p) { + return (p >= 0 && p < buf.length) ? buf[p] : -1; +} + +function u16(buf, p) { + return (p >= 0 && p + 2 <= buf.length) ? buf.readUInt16BE(p) : -1; +} + +function parseSNI(buf) { + // TLSPlaintext: type(1) version(2) length(2), then Handshake: type(1) len(3) + if (buf.length < 45 || buf[0] !== 0x16 || buf[5] !== 0x01) + return null; + + let p = 5 + 4 + 2 + 32; // handshake header + client_version + random + + const sessionId = u8(buf, p); // legacy_session_id + if (sessionId < 0) + return null; + p += 1 + sessionId; + + const ciphers = u16(buf, p); // cipher_suites + if (ciphers < 0) + return null; + p += 2 + ciphers; + + const compression = u8(buf, p); // legacy_compression_methods + if (compression < 0) + return null; + p += 1 + compression; + + const extensions = u16(buf, p); + if (extensions < 0) + return null; + const end = Math.min(p + 2 + extensions, buf.length); + p += 2; + + while (p + 4 <= end) { + const type = buf.readUInt16BE(p); + const len = buf.readUInt16BE(p + 2); + if (type === 0x0000) { // server_name + // ServerNameList: list length(2), then name_type(1) + length(2) + name + const q = p + 4 + 2; + if (u8(buf, q) !== 0x00) // not a host_name entry, or out of bounds + return null; + const nlen = u16(buf, q + 1); + if (nlen < 0 || q + 3 + nlen > buf.length) + return null; + return buf.toString('ascii', q + 3, q + 3 + nlen); + } + p += 4 + len; + } + return null; +} + +module.exports = { parseSNI }; diff --git a/test/sni-harness.js b/test/sni-harness.js new file mode 100644 index 000000000..0c3c2e553 --- /dev/null +++ b/test/sni-harness.js @@ -0,0 +1,117 @@ +const net = require('node:net'); + +const { parseSNI } = require('./clienthello'); +const Imap = require('../lib/Connection'); + +const CRLF = '\r\n'; + +const CAPS = '* CAPABILITY IMAP4rev1 STARTTLS'; + +// Connects one Imap client to a bare TCP server and resolves with what the +// client put on the wire: +// +// sni -- host name from the ClientHello server_name extension, or null when +// the client sent no such extension +// cmds -- IMAP commands received before the upgrade (STARTTLS runs only) +// +// The handshake is never completed: the server only records the offer, so no +// certificates are involved and the result does not depend on the TLS version. +// +// options.host host to listen on and to configure the client with +// options.starttls drive an IMAP session and upgrade via STARTTLS +// options.imapOptions extra Imap() options, merged over the defaults +function captureSNI(options = {}) { + const host = options.host || 'localhost'; + const cmds = []; + + return new Promise((resolve, reject) => { + let imap; + let timeout; + + const srv = net.createServer((sock) => { + let upgrading = false; + let done = false; + + const finish = (data) => { + done = true; + sock.destroy(); + srv.close(); + clearTimeout(timeout); + imap.destroy(); + resolve({ sni: parseSNI(data), cmds }); + }; + + // A ClientHello can in principle be split across several TCP segments, + // so collect bytes until the first TLS record is complete rather than + // parsing whatever the first 'data' event happened to carry. + let hello = Buffer.alloc(0); + const onHandshake = (data) => { + if (done) + return; + hello = Buffer.concat([hello, data]); + if (hello.length < 5) + return; + if (hello.length < 5 + hello.readUInt16BE(3)) + return; + finish(hello); + }; + + if (!options.starttls) { + sock.on('data', onHandshake); + return; + } + + let buf = ''; + sock.write(`* OK IMAP4rev1 service ready.${CRLF}`); + sock.on('data', (data) => { + if (upgrading) { + // everything after our STARTTLS response is handshake data + onHandshake(data); + return; + } + + buf += data.toString('latin1'); + let idx; + while ((idx = buf.indexOf(CRLF)) > -1) { + const line = buf.substring(0, idx); + const tag = line.substring(0, line.indexOf(' ')); + buf = buf.substring(idx + 2); + cmds.push(line); + if (/ CAPABILITY$/.test(line)) { + sock.write(`${CAPS}${CRLF}${tag} OK CAPABILITY completed.${CRLF}`); + } else if (/ STARTTLS$/.test(line)) { + upgrading = true; + sock.write(`${tag} OK Begin TLS negotiation now.${CRLF}`); + return; + } else { + sock.destroy(); + srv.close(); + clearTimeout(timeout); + imap.destroy(); + reject(new Error(`Unexpected command: ${line}`)); + return; + } + } + }); + }); + + srv.listen(0, host, () => { + imap = new Imap(Object.assign({ + host, + port: srv.address().port, + user: 'foo', + password: 'bar' + }, options.imapOptions)); + timeout = setTimeout(() => { + srv.close(); + imap.destroy(); + reject(new Error('Timed out waiting for the TLS ClientHello')); + }, 2000); + // the aborted handshake surfaces as a socket error -- expected here + imap.on('error', () => {}); + imap.connect(); + }); + }); +} + +module.exports = { captureSNI }; diff --git a/test/test-connection-starttls-sni.js b/test/test-connection-starttls-sni.js new file mode 100644 index 000000000..cde524a94 --- /dev/null +++ b/test/test-connection-starttls-sni.js @@ -0,0 +1,36 @@ +const assert = require('node:assert'); + +const { captureSNI } = require('./sni-harness'); + +// Same as test-connection-tls-sni.js, but for the upgrade performed by +// _starttls(): it builds its own tlsOptions, so it needs `servername` of its +// own. + +const HOST = 'localhost'; + +const EXPECTED = ['A0 CAPABILITY', 'A1 STARTTLS']; + +const results = {}; + +(async () => { + const { sni, cmds } = await captureSNI({ + host: HOST, + starttls: true, + imapOptions: { tls: false, autotls: 'always' } + }); + results.sni = sni; + results.cmds = cmds; +})().catch((err) => { + results.error = err; +}); + +process.once('exit', () => { + assert.ifError(results.error); + assert.deepStrictEqual(results.cmds, EXPECTED, + `Unexpected command sequence: ${results.cmds}`); + assert.strictEqual( + results.sni, + HOST, + `Expected the ClientHello to request SNI for ${HOST}, got: ${JSON.stringify(results.sni)}` + ); +}); diff --git a/test/test-connection-tls-sni-ip.js b/test/test-connection-tls-sni-ip.js new file mode 100644 index 000000000..8a0f2c66b --- /dev/null +++ b/test/test-connection-tls-sni-ip.js @@ -0,0 +1,51 @@ +const assert = require('node:assert'); + +const { captureSNI } = require('./sni-harness'); + +// RFC 6066 does not permit IP addresses in the server_name extension, and node +// warns about them (DEP0123: "Setting the TLS ServerName to an IP address is +// not permitted by RFC 6066. This will be ignored in a future version."), so +// the default is only derived from `host` when it is a name. +// +// An explicit tlsOptions.servername is passed through untouched even when it is +// an IP -- that is the caller's decision to make, not ours. + +const HOST = '127.0.0.1'; + +const results = {}; + +(async () => { + results.tls = (await captureSNI({ + host: HOST, + imapOptions: { tls: true } + })).sni; + + results.starttls = (await captureSNI({ + host: HOST, + starttls: true, + imapOptions: { tls: false, autotls: 'always' } + })).sni; + + // This asserts what node currently does with an IP servername. Once DEP0123 + // turns into an actual ignore, this one has to be relaxed -- the pass-through + // is what we promise, not what node makes of it. + results.ipOverride = (await captureSNI({ + host: HOST, + imapOptions: { tls: true, tlsOptions: { servername: HOST } } + })).sni; +})().catch((err) => { + results.error = err; +}); + +process.once('exit', () => { + assert.ifError(results.error); + assert.strictEqual(results.tls, null, + 'Expected no SNI for an IP host over implicit TLS, got: ' + + JSON.stringify(results.tls)); + assert.strictEqual(results.starttls, null, + 'Expected no SNI for an IP host over STARTTLS, got: ' + + JSON.stringify(results.starttls)); + assert.strictEqual(results.ipOverride, HOST, + 'Expected an explicit IP servername to be sent anyway, got: ' + + JSON.stringify(results.ipOverride)); +}); diff --git a/test/test-connection-tls-sni.js b/test/test-connection-tls-sni.js new file mode 100644 index 000000000..d6736ae5a --- /dev/null +++ b/test/test-connection-tls-sni.js @@ -0,0 +1,46 @@ +const assert = require('node:assert'); + +const { captureSNI } = require('./sni-harness'); + +// tls.connect() only sends the SNI extension when `servername` is set +// explicitly -- node never derives it from `host`. Without it, servers that +// serve several domains from one address (gmail, Exchange Online, anything +// behind a load balancer) answer with a default certificate that does not match +// the requested host, which surfaces as a "self signed certificate" error. +// See mscdex/node-imap#724. + +const HOST = 'localhost'; + +const results = {}; + +(async () => { + results.sni = (await captureSNI({ + host: HOST, + imapOptions: { tls: true } + })).sni; + + // The default is assigned before the caller's tlsOptions are copied over it, + // so a configured servername has to win. Consumers that already pass one + // (and point it somewhere other than `host`) depend on that. + results.override = (await captureSNI({ + host: HOST, + imapOptions: { tls: true, tlsOptions: { servername: 'imap.example.com' } } + })).sni; +})().catch((err) => { + results.error = err; +}); + +process.once('exit', () => { + assert.ifError(results.error); + assert.strictEqual( + results.sni, + HOST, + `Expected the ClientHello to request SNI for ${HOST}, got: ${JSON.stringify(results.sni)}` + ); + assert.strictEqual( + results.override, + 'imap.example.com', + 'Expected the configured servername to override the default, got: ' + + JSON.stringify(results.override) + ); +});