Skip to content

Commit a9343b4

Browse files
andris9claude
andris9
and
claude
committed
fix(mime): normalize an address so header and envelope agree
A local part is emitted bare, so anything that is not a valid dot-atom now goes out as a quoted-string. The previous check only looked at the first and last character, so a value that merely started and ended with a quote, such as '"a"@evil.com"@good.com', passed through untouched. Sender and receiver then split the domain off at a different '@' and the message goes somewhere other than the header shows. Along the same lines: * an address that carries a special is emitted inside angle brackets. A domain has no quoting construct, so without them a ',' or a ';' in a domain reads as a recipient separator and the header lists a recipient the envelope never had. * addressparser keeps the quotes on a local part it read out of a quoted string, so consumers of the standalone package stop receiving the ambiguous bare form. * a custom envelope is normalized on the sendmail, ses, json and stream transports as well, which read it raw before. The smtp transports already went through getEnvelope. * an explicit envelope keeps a bare local username such as 'root'. An envelope value is an addr-spec, not the display name a header would read it as. * the sendmail argv guard looks past an opening quote, so a quoted local part can no longer hide a leading dash from it. * _parseAddresses no longer rewrites the address object the caller passed in. Observable change: info.accepted, info.rejected and info.envelope now carry the quoted form for addresses that were previously emitted malformed. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_019EN8xhmtzZvxjFfBUuygVi
1 parent b7d772e commit a9343b4

11 files changed

Lines changed: 383 additions & 37 deletions

File tree

‎lib/addressparser/index.js‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,39 @@
11
'use strict';
22

3+
/**
4+
* Restores the quoting of a local part that was read out of a quoted string.
5+
*
6+
* RFC 5321 allows '@' inside a quoted local part, so handing '"[email protected]"@good.com'
7+
* on as the bare '[email protected]@good.com' leaves it to the consumer which '@' splits the
8+
* domain off. Getting that wrong is a misrouting vector, so the quotes go back on. The
9+
* same holds for the other specials: a ',' or a ';' that loses its quotes reads as a
10+
* recipient separator once the consumer puts the address back into a header.
11+
*
12+
* This module has no dependencies so that it can ship on its own, which is why the two
13+
* grammar tests below are spelled out here instead of shared with lib/mime-node. Keeping
14+
* only what is ambiguous quoted is deliberate, mime-node applies the stricter RFC 5321
15+
* dot-atom rule on top of this when it emits an address.
16+
*
17+
* @param {String} address Address with an unquoted local part
18+
* @return {String} Address with the local part as a quoted-string
19+
*/
20+
function _quoteLocalPart(address) {
21+
const lastAt = address.lastIndexOf('@');
22+
if (lastAt < 0) {
23+
// no domain to split off, nothing can be misrouted
24+
return address;
25+
}
26+
27+
const user = address.substr(0, lastAt);
28+
if (/^[^\s"(),:;<>@[\\\]]+$/.test(user) || /^"(?:[^"\\]|\\[\s\S])*"$/.test(user)) {
29+
// a local part that carries no special reads the same with or without the quotes,
30+
// and one that is already a complete quoted-string needs nothing either
31+
return address;
32+
}
33+
34+
return '"' + user.replace(/["\\]/g, '\\$&') + '"@' + address.substr(lastAt + 1);
35+
}
36+
337
/**
438
* Converts tokens for a single address into an address object
539
*
@@ -145,6 +179,10 @@ function _handleAddress(tokens, depth) {
145179
data.text = data.text.concat(data.address.splice(1));
146180
}
147181

182+
// An address is only taken from unquoted text, so anything left in the text at this
183+
// point that still has to serve as the address carries its quoting in this flag
184+
const addressFromQuotedText = !data.address.length && data.textWasQuoted.some(wasQuoted => wasQuoted);
185+
148186
// Join values with spaces
149187
data.text = data.text.join(' ');
150188
data.address = data.address.join(' ');
@@ -162,6 +200,10 @@ function _handleAddress(tokens, depth) {
162200
}
163201
}
164202

203+
if (addressFromQuotedText && address.address) {
204+
address.address = _quoteLocalPart(address.address);
205+
}
206+
165207
addresses.push(address);
166208
}
167209

‎lib/json-transport/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ class JSONTransport {
3333
// Sendmail strips this header line by itself
3434
mail.message.keepBcc = true;
3535

36-
const envelope = mail.data.envelope || mail.message.getEnvelope();
36+
const envelope = mail.message.getEnvelope();
3737
const messageId = mail.message.messageId();
3838

3939
const recipients = [].concat(envelope.to || []);

‎lib/mailer/mail-message.js‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ class MailMessage {
140140
}
141141

142142
normalize(callback) {
143-
const envelope = this.data.envelope || this.message.getEnvelope();
143+
const envelope = this.message.getEnvelope();
144144
const messageId = this.message.messageId();
145145

146146
this.resolveAll((err, data) => {
@@ -278,11 +278,9 @@ class MailMessage {
278278
const needsEncoding = !mimeFuncs.isPlainText(comment) || /\x7f/.test(comment);
279279

280280
if (key.toLowerCase().trim() === 'id') {
281-
// List-ID: "comment" <domain>
282-
// in the quoted-string a quote would end it early and a trailing backslash
283-
// would escape the closing quote and swallow the <domain> behind it, so
284-
// both go out as quoted-pairs
285-
comment = needsEncoding ? mimeFuncs.encodeWord(comment) : '"' + comment.replace(/["\\]/g, '\\$&') + '"';
281+
// List-ID: "comment" <domain>, where an unescaped quote or a trailing
282+
// backslash in the comment would swallow the <domain> behind it
283+
comment = needsEncoding ? mimeFuncs.encodeWord(comment) : mimeFuncs.quoteString(comment);
286284

287285
// List-ID expects a bare domain-like identifier, so strip the
288286
// scheme prefix that _formatListUrl adds or passes through

‎lib/mime-funcs/index.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,17 @@ module.exports = {
2626
return typeof value === 'string' && !re.test(value);
2727
},
2828

29+
/**
30+
* Wraps a value into a quoted-string. Inside one a quote would end the string early
31+
* and a backslash would escape whatever follows it, so both go out as quoted-pairs.
32+
*
33+
* @param {String} value String to be quoted
34+
* @returns {String} The value as a quoted-string, quotes included
35+
*/
36+
quoteString(value) {
37+
return '"' + (value || '').toString().replace(/["\\]/g, '\\$&') + '"';
38+
},
39+
2940
/**
3041
* Checks if a multi line string containes lines longer than the selected value.
3142
*

‎lib/mime-node/index.js‎

Lines changed: 83 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,20 @@ const LeUnix = require('./le-unix');
2121

2222
const FORMATTED_HEADERS = ['From', 'Sender', 'To', 'Cc', 'Bcc', 'Reply-To', 'Date', 'References'];
2323

24+
// RFC 5321 atext, plus the non-ascii bytes that SMTPUTF8 (RFC 6531) adds to it. A local part
25+
// built from these, with '.' as a separator, is a dot-atom and can be emitted bare
26+
const ATEXT = "[A-Za-z0-9!#$%&'*+\\-/=?^_`{|}~\\x80-\\uFFFF]";
27+
const DOT_ATOM = new RegExp('^' + ATEXT + '+(?:\\.' + ATEXT + '+)*$');
28+
29+
// A complete quoted-string: everything between the outer quotes is either a plain char or
30+
// a quoted-pair. Anchored, so a value that only starts and ends with a quote does not pass
31+
const QUOTED_STRING = /^"(?:[^"\\]|\\[\s\S])*"$/;
32+
33+
// An address that carries no special anywhere can be emitted bare in a header, everything
34+
// else goes into angle brackets so that the header can not be read as more addresses than
35+
// the envelope carries
36+
const PLAIN_ADDRESS = /^[^\s"(),:;<>@[\\\]]+@[^\s"(),:;<>@[\\\]]+$/;
37+
2438
/**
2539
* Creates a new mime tree node. Assumes 'multipart/*' as the content type
2640
* if it is a branch, anything else counts as leaf. If rootNode is missing from
@@ -853,15 +867,15 @@ class MimeNode {
853867

854868
if (envelope.from) {
855869
list = [];
856-
this._convertAddresses(this._parseAddresses(envelope.from), list);
870+
this._convertAddresses(this._parseEnvelopeAddresses(envelope.from), list);
857871
list = list.filter(address => address && address.address);
858872
if (list.length && list[0]) {
859873
this._envelope.from = list[0].address;
860874
}
861875
}
862876
['to', 'cc', 'bcc'].forEach(key => {
863877
if (envelope[key]) {
864-
this._convertAddresses(this._parseAddresses(envelope[key]), this._envelope.to);
878+
this._convertAddresses(this._parseEnvelopeAddresses(envelope[key]), this._envelope.to);
865879
}
866880
});
867881

@@ -1050,15 +1064,43 @@ class MimeNode {
10501064
[],
10511065
[].concat(addresses).map(address => {
10521066
if (address && address.address) {
1053-
address.address = this._normalizeAddress(address.address);
1054-
address.name = address.name || '';
1055-
return [address];
1067+
const normalized = this._normalizeAddress(address.address);
1068+
if (normalized === address.address && typeof address.name === 'string') {
1069+
// there is nothing to rewrite, so there is nothing to keep off the original
1070+
return [address];
1071+
}
1072+
1073+
// rewriting would land on the object the caller passed in and might
1074+
// still hold a reference to, so rewrite a copy of it instead
1075+
const copy = Object.assign({}, address);
1076+
copy.address = normalized;
1077+
copy.name = address.name || '';
1078+
return [copy];
10561079
}
10571080
return addressparser(address);
10581081
})
10591082
);
10601083
}
10611084

1085+
/**
1086+
* Parses the addresses of an explicitly set envelope.
1087+
*
1088+
* An envelope value is an addr-spec and never a display name, so a bare local username
1089+
* such as 'root' is the address here. Header parsing has to read the same value as a
1090+
* display name, as a value with no '@' in it can not be an addr-spec in a header.
1091+
*
1092+
* @param {Mixed} addresses Addresses to be parsed
1093+
* @return {Array} An array of address objects
1094+
*/
1095+
_parseEnvelopeAddresses(addresses) {
1096+
return this._parseAddresses(addresses).map(entry => {
1097+
if (entry.address || entry.group || !entry.name || /[\s@]/.test(entry.name)) {
1098+
return entry;
1099+
}
1100+
return { address: this._normalizeAddress(entry.name), name: '' };
1101+
});
1102+
}
1103+
10621104
/**
10631105
* Normalizes a header key, uses Camel-Case form, except for uppercase MIME-
10641106
*
@@ -1212,7 +1254,11 @@ class MimeNode {
12121254
address.address = this._normalizeAddress(address.address);
12131255

12141256
if (!address.name) {
1215-
values.push(address.address.indexOf(' ') >= 0 ? `<${address.address}>` : `${address.address}`);
1257+
// an address that carries a special, be it a quoted local part or a domain
1258+
// that could not be normalized, is only unambiguous inside angle brackets.
1259+
// Without them a ',' or a ';' anywhere in it reads as a recipient separator
1260+
// and the header would list more recipients than the envelope carries
1261+
values.push(PLAIN_ADDRESS.test(address.address) ? address.address : `<${address.address}>`);
12161262
} else {
12171263
values.push(`${this._encodeAddressName(address.name)} <${address.address}>`);
12181264
}
@@ -1241,16 +1287,23 @@ class MimeNode {
12411287
.replace(/[\x00-\x1F\x7F<>]+/g, ' ') // remove unallowed characters
12421288
.trim();
12431289

1290+
if (!address) {
1291+
// callers use an empty value to detect a missing address
1292+
return address;
1293+
}
1294+
12441295
const lastAt = address.lastIndexOf('@');
12451296
if (lastAt < 0) {
1246-
// Bare username
1247-
return address;
1297+
// Bare username, there is no domain to split off
1298+
return this._normalizeLocalPart(address);
12481299
}
12491300

1250-
let user = address.substr(0, lastAt);
1301+
const user = address.substr(0, lastAt);
12511302
const domain = address.substr(lastAt + 1);
12521303

1253-
// Usernames are not touched and are kept as is even if these include unicode.
1304+
// Unicode in the local part is kept as is, see _normalizeLocalPart for the rest of it.
1305+
// A domain has no quoting construct to fall back on, so whatever is not a valid domain
1306+
// is kept as supplied and it is _convertAddresses that keeps such an address unambiguous.
12541307
// Domains are punycoded when the local part is ASCII ('safe@jõgeva.ee' -> '[email protected]').
12551308
// When the local part contains non-ASCII bytes the address already requires SMTPUTF8,
12561309
// so the domain is kept (or decoded back) as UTF-8 for symmetry on both sides of '@'.
@@ -1267,16 +1320,27 @@ class MimeNode {
12671320
// keep domain as supplied
12681321
}
12691322

1270-
if (user.indexOf(' ') >= 0) {
1271-
if (user.charAt(0) !== '"') {
1272-
user = '"' + user;
1273-
}
1274-
if (user.substr(-1) !== '"') {
1275-
user = user + '"';
1276-
}
1323+
return `${this._normalizeLocalPart(user)}@${encodedDomain}`;
1324+
}
1325+
1326+
/**
1327+
* Normalizes the local part of an address into a form that can be emitted as is.
1328+
*
1329+
* A local part is either a dot-atom or a quoted-string, anything else is not a valid
1330+
* addr-spec. The quotes of a quoted local part get lost along the way, and a bare
1331+
* '[email protected]@good.com' leaves it to the receiver which '@' splits the domain off,
1332+
* while the split here is always at the last one. So whatever is not already one of
1333+
* the two valid forms goes back out as a quoted-string.
1334+
*
1335+
* @param {String} user Local part of an address
1336+
* @return {String} Local part as a dot-atom or as a quoted-string
1337+
*/
1338+
_normalizeLocalPart(user) {
1339+
if (DOT_ATOM.test(user) || QUOTED_STRING.test(user)) {
1340+
return user;
12771341
}
12781342

1279-
return `${user}@${encodedDomain}`;
1343+
return mimeFuncs.quoteString(user);
12801344
}
12811345

12821346
/**
@@ -1288,7 +1352,7 @@ class MimeNode {
12881352
_encodeAddressName(name) {
12891353
if (!/^[\w ]*$/.test(name)) {
12901354
if (/^[\x20-\x7e]*$/.test(name)) {
1291-
return '"' + name.replace(/([\\"])/g, '\\$1') + '"';
1355+
return mimeFuncs.quoteString(name);
12921356
} else {
12931357
return mimeFuncs.encodeWord(name, this._getTextEncoding(name), 52);
12941358
}

‎lib/sendmail-transport/index.js‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,14 +62,17 @@ class SendmailTransport {
6262
// Sendmail strips this header line by itself
6363
mail.message.keepBcc = true;
6464

65-
const envelope = mail.data.envelope || mail.message.getEnvelope();
65+
const envelope = mail.message.getEnvelope();
6666
const messageId = mail.message.messageId();
6767
let returned;
6868

6969
const hasInvalidAddresses = []
7070
.concat(envelope.from || [])
7171
.concat(envelope.to || [])
72-
.some(addr => /^-/.test(addr));
72+
// a local part is either a dot-atom or a quoted-string, so a leading dash sits at
73+
// offset 0 or, behind the opening quote, at offset 1. Only the first shape is read
74+
// as an option by sendmail, but both are the address this guard keeps out of argv
75+
.some(addr => /^"?-/.test(addr));
7376
if (hasInvalidAddresses) {
7477
const err = new Error('Can not send mail. Invalid envelope addresses.');
7578
err.code = errors.ESENDMAIL;

‎lib/ses-transport/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ class SESTransport extends EventEmitter {
6666
fromHeader = mimeNode._convertAddresses(mimeNode._parseAddresses(fromHeader.value));
6767
}
6868

69-
const envelope = mail.data.envelope || mail.message.getEnvelope();
69+
const envelope = mail.message.getEnvelope();
7070
const messageId = mail.message.messageId();
7171

7272
const recipients = [].concat(envelope.to || []);

‎lib/stream-transport/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class StreamTransport {
4242
// We probably need this in the output
4343
mail.message.keepBcc = true;
4444

45-
const envelope = mail.data.envelope || mail.message.getEnvelope();
45+
const envelope = mail.message.getEnvelope();
4646
const messageId = mail.message.messageId();
4747

4848
const recipients = [].concat(envelope.to || []);

‎test/addressparser/addressparser-test.js‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ describe('#addressparser', () => {
331331
let input = '"[email protected]"@example.com';
332332
let expected = [
333333
{
334-
address: '[email protected]@example.com',
334+
address: '"[email protected]"@example.com',
335335
name: ''
336336
}
337337
];
@@ -342,10 +342,11 @@ describe('#addressparser', () => {
342342
it('should not extract email from quoted local-part (security)', () => {
343343
let input = '"[email protected] x"@internal.domain';
344344
let result = addressparser(input);
345-
// Should preserve full address, NOT extract [email protected]
345+
// Should preserve full address, NOT extract [email protected]. The local part keeps
346+
// its quotes, without them the '@' that splits the domain off is ambiguous
346347
assert.strictEqual(result.length, 1);
347348
assert.strictEqual(result[0].address.includes('@internal.domain'), true);
348-
assert.strictEqual(result[0].address, '[email protected] [email protected]');
349+
assert.strictEqual(result[0].address, '"[email protected] x"@internal.domain');
349350
});
350351

351352
it('should handle quoted local-part with attacker domain (security)', () => {
@@ -354,15 +355,15 @@ describe('#addressparser', () => {
354355
// Should route to legitimate.com, not attacker.com
355356
assert.strictEqual(result.length, 1);
356357
assert.strictEqual(result[0].address.includes('@legitimate.com'), true);
357-
assert.strictEqual(result[0].address, '[email protected]@legitimate.com');
358+
assert.strictEqual(result[0].address, '"[email protected]"@legitimate.com');
358359
});
359360

360361
it('should handle multiple @ in quoted local-part (security)', () => {
361362
let input = '"a@b@c"@example.com';
362363
let result = addressparser(input);
363364
// Should not extract a@b or b@c
364365
assert.strictEqual(result.length, 1);
365-
assert.strictEqual(result[0].address, 'a@b@[email protected]');
366+
assert.strictEqual(result[0].address, '"a@b@c"@example.com');
366367
});
367368

368369
it('should handle quoted local-part with angle brackets', () => {
@@ -379,14 +380,14 @@ describe('#addressparser', () => {
379380
let input = '"test\\"quote"@example.com';
380381
let result = addressparser(input);
381382
assert.strictEqual(result.length, 1);
382-
assert.strictEqual(result[0].address, 'test"[email protected]');
383+
assert.strictEqual(result[0].address, '"test\\"quote"@example.com');
383384
});
384385

385386
it('should handle escaped backslashes', () => {
386387
let input = '"test\\\\backslash"@example.com';
387388
let result = addressparser(input);
388389
assert.strictEqual(result.length, 1);
389-
assert.strictEqual(result[0].address, 'test\\[email protected]');
390+
assert.strictEqual(result[0].address, '"test\\\\backslash"@example.com');
390391
});
391392

392393
it('should handle unclosed quote gracefully', () => {

0 commit comments

Comments
 (0)