Skip to content

Commit 5e232bf

Browse files
InterLinked1InterLinked1
committed
mod_smtp_mailing_lists: Further fix list reply behavior.
* Ignore existing Reply-To headers in submitted messages. * Ensure the list is always in the To or Cc headers, so that reply all will go to the list if replyto=sender. * Actually load the 'name' setting when parsing config. * Add tests for list name and reply behavior.
1 parent a76f4e3 commit 5e232bf

5 files changed

Lines changed: 94 additions & 15 deletions

File tree

‎include/net_smtp.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ int smtp_is_exempt_relay(struct smtp_session *smtp);
7272
* \brief Get a timestamp string appropriate for the Received header
7373
* \param received Received time
7474
* \param[out] buf
75-
* \param len Length of buf
75+
* \param len Length of buf. Should be at least 32
7676
*/
7777
void smtp_timestamp(time_t received, char *buf, size_t len);
7878

‎modules/mod_smtp_mailing_lists.c‎

Lines changed: 69 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,12 @@ static int listify(struct mailing_list *l, struct smtp_response *resp, FILE *fp,
273273
char delivered_hdr[256];
274274
char from_hdr[256] = "";
275275
int skipping = 0;
276+
int list_in_to = 0, list_in_cc = 0, got_to = 0, got_cc = 0;
277+
char baselistaddr[256], listaddr[256];
278+
const char *rdomain = S_OR(l->domain, smtp_hostname());
279+
280+
snprintf(baselistaddr, sizeof(baselistaddr), "%s@%s", l->user, rdomain);
281+
snprintf(listaddr, sizeof(listaddr), "%s%s%s<%s@%s>", l->name ? "\"" : "", S_IF(l->name), l->name ? "\" " : "", l->user, rdomain);
276282

277283
snprintf(delivered_hdr, sizeof(delivered_hdr), "Delivered-To: mailing list %s@%s", l->user, S_OR(l->domain, smtp_hostname()));
278284

@@ -385,13 +391,13 @@ static int listify(struct mailing_list *l, struct smtp_response *resp, FILE *fp,
385391
/* Munge the address portion (excluding name) */
386392
snprintf(munged_address, sizeof(munged_address), "%s%s%s", user, host ? "@" : "", S_IF(host));
387393
bbs_strreplace(munged_address, '@', '=');
388-
bbs_debug(5, "Munged From address '%s' -> '%s@%s'\n", from, munged_address, S_OR(l->domain, smtp_hostname()));
394+
bbs_debug(5, "Munged From address '%s' -> '%s@%s'\n", from, munged_address, rdomain);
389395
/* e.g. bob@dmarc.example.com should now be bob=dmarc.example.com@example.com */
390396

391397
if (name) {
392398
/* If we originally had a name, include it here
393399
* If it was already quoted, retain the quotes, but we don't add them if they weren't present. */
394-
fprintf(fp, "From: %s <%s@%s>\r\n", name, munged_address, S_OR(l->domain, smtp_hostname()));
400+
fprintf(fp, "From: %s <%s@%s>\r\n", name, munged_address, rdomain);
395401
} else {
396402
fprintf(fp, "From: %s@%s\r\n", munged_address, S_OR(l->domain, smtp_hostname()));
397403
}
@@ -401,10 +407,69 @@ static int listify(struct mailing_list *l, struct smtp_response *resp, FILE *fp,
401407
fprintf(fp, "From: %s\r\n", from); /* Use the original From header */
402408
}
403409
}
410+
} else if (STARTS_WITH(buf, "Reply-To:")) {
411+
/* We add our own Reply-To header, so if the message came in with one, ignore it */
412+
continue;
413+
} else if (STARTS_WITH(buf, "To:")) {
414+
/*
415+
* The Reply-To header is easy, as that simply follows the replyto setting.
416+
*
417+
* The To and Cc headers are a bit tricky for us.
418+
* We need to largely preserve what is already in these headers, but:
419+
* - If the list is not present in either of them, add it (so that reply all works)
420+
*
421+
* We'll try adding the list to 'To' by default, but if it's in the Cc for some reason, we'll leave that alone, doesn't matter for reply all.
422+
* If the list is in both the To and Cc headers, that should be okay, proper mail clients should discard the Cc and just keep the To (at least Mozilla does)
423+
*
424+
* For example, if a user sends an email with the list in Bcc, so it's not originally in either header, we'll add it to the To header.
425+
*
426+
* Note that addresses can appear in both To/Cc and Reply-To. If this happens, Mozilla picks the name from the To/Cc headers (but follows Reply-To for reply).
427+
*
428+
* Testing Note: If you send a message to the list in Mozilla and the Reply-To is your address in the current mailbox, with replyto=sender, "Reply" will yield no recipients,
429+
* while Reply To only includes the list - it's smart enough to prevent you from just emailing yourself when you click Reply.
430+
*/
431+
got_to = 1;
432+
if (strstr(buf, baselistaddr)) {
433+
list_in_to = 1;
434+
fprintf(fp, "%s\r\n", buf);
435+
} else {
436+
/* We've gotten both headers and the list isn't in either. Add it to 'To' now. */
437+
if (got_cc && !list_in_cc) {
438+
char *hdrval = buf + STRLEN("To:");
439+
ltrim(hdrval);
440+
fprintf(fp, "To: %s,%s\r\n", hdrval, listaddr); /* no space between comma-separated recipients */
441+
} else {
442+
fprintf(fp, "%s\r\n", buf);
443+
}
444+
}
445+
} else if (STARTS_WITH(buf, "Cc:")) {
446+
got_cc = 1;
447+
if (strstr(buf, baselistaddr)) {
448+
list_in_cc = 1;
449+
fprintf(fp, "%s\r\n", buf);
450+
} else {
451+
/* We've gotten both headers and the list isn't in either. Add it to 'Cc' now, since it's too late to add it to 'To' */
452+
if (got_to && !list_in_to) {
453+
char *hdrval = buf + STRLEN("Cc:");
454+
ltrim(hdrval);
455+
fprintf(fp, "Cc: %s,%s\r\n", hdrval, listaddr); /* no space between comma-separated recipients */
456+
} else {
457+
fprintf(fp, "%s\r\n", buf);
458+
}
459+
}
404460
} else {
405461
fprintf(fp, "%s\r\n", buf); /* Just copy it over */
406462
}
407463
}
464+
if (!list_in_to && !list_in_cc) {
465+
/* If this happens, then we only got one of the To and Cc headers or we would have put it in the second one encountered.
466+
* So, add the missing header, containing the list. */
467+
if (got_to) {
468+
fprintf(fp, "Cc: %s\r\n", listaddr);
469+
} else {
470+
fprintf(fp, "To: %s\r\n", listaddr);
471+
}
472+
}
408473
if (s_strlen_zero(from_hdr)) {
409474
bbs_warning("Message has no 'From' header?\n");
410475
return -1;
@@ -831,6 +896,8 @@ static int load_config(void)
831896
archive = S_TRUE(value);
832897
} else if (!strcmp(key, "maxsize")) {
833898
maxsize = (size_t) atol(value);
899+
} else if (!strcmp(key, "name")) {
900+
name = value;
834901
} else if (!strcmp(key, "tag")) {
835902
tag = value;
836903
} else if (!strcmp(key, "replyto")) {

‎nets/net_smtp.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1437,7 +1437,7 @@ void smtp_timestamp(time_t received, char *buf, size_t len)
14371437
struct tm smtpdate;
14381438

14391439
/* Timestamp is something like Wed, 22 Feb 2023 03:02:22 +0300 */
1440-
localtime_r(&received, &smtpdate);
1440+
localtime_r(&received, &smtpdate);
14411441
strftime(buf, len, "%a, %b %e %Y %H:%M:%S %z", &smtpdate);
14421442
}
14431443

‎tests/configs/mod_smtp_mailing_lists.conf‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ recipients = testuser
2525
senders = *
2626
replyto = sender
2727

28+
[replysendername]
29+
name = Sender Reply List
30+
recipients = testuser
31+
senders = *
32+
replyto = sender
33+
2834
[replylist]
2935
recipients = testuser
3036
senders = *

‎tests/test_smtp_mailing_lists.c‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -216,16 +216,23 @@ static int run(void)
216216

217217
/* Test reply behavior of individual lists */
218218
POST_TO_LIST("<replysender>", TEST_EMAIL, TEST_EMAIL);
219-
SWRITE(client1, "a4 FETCH 3 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
219+
SWRITE(client1, "b1 FETCH 3 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
220220
CLIENT_EXPECT_EVENTUALLY(client1, "Reply-To: " TEST_EMAIL);
221221

222222
POST_TO_LIST("<replylist>", TEST_EMAIL, TEST_EMAIL);
223-
SWRITE(client1, "a5 FETCH 4 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
223+
SWRITE(client1, "c1 FETCH 4 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
224224
CLIENT_EXPECT_EVENTUALLY(client1, "Reply-To: <replylist@bbs.example.com>");
225225

226226
POST_TO_LIST("<replyboth>", TEST_EMAIL, TEST_EMAIL);
227-
SWRITE(client1, "a6 FETCH 5 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
228-
CLIENT_EXPECT_EVENTUALLY(client1, "Reply-To: <replyboth@bbs.example.com>");
227+
SWRITE(client1, "c2 FETCH 5 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
228+
CLIENT_EXPECT_EVENTUALLY(client1, "Reply-To: <replyboth@bbs.example.com>," TEST_EMAIL);
229+
230+
/* Test that the list name comes through properly */
231+
POST_TO_LIST("<replysendername>", TEST_EMAIL, TEST_EMAIL);
232+
SWRITE(client1, "d1 FETCH 6 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
233+
CLIENT_EXPECT_EVENTUALLY(client1, "Reply-To: " TEST_EMAIL);
234+
235+
/* Past this point, all messages are from an external sender, only the dmarcmungetest list: */
229236

230237
/* Test an external email address, which should munge the From address
231238
* If TEST_WITH_REAL_DMARC_POLICIES isn't defined, mod_smtp_filter_dmarc isn't loaded, so it should fail safe with BBS_DMARC_POLICY_ERROR and munge anyways
@@ -238,25 +245,24 @@ static int run(void)
238245
CLIENT_EXPECT_EVENTUALLY(clientfd, "250 ");
239246

240247
POST_TO_LIST("<dmarcmungetest>", TEST_EMAIL_EXTERNAL, TEST_EMAIL_EXTERNAL);
241-
SWRITE(client1, "a7 FETCH 6 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
248+
SWRITE(client1, "e1 FETCH 7 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
242249
CLIENT_EXPECT_EVENTUALLY(client1, "Reply-To: " TEST_EMAIL_EXTERNAL);
243-
SWRITE(client1, "a8 FETCH 6 (BODY.PEEK[HEADER.FIELDS (From)])" ENDL);
250+
SWRITE(client1, "e2 FETCH 7 (BODY.PEEK[HEADER.FIELDS (From)])" ENDL);
244251
CLIENT_EXPECT_EVENTUALLY(client1, "From: external=" TEST_EXTERNAL_DOMAIN "@" TEST_HOSTNAME);
245252

246-
247253
POST_TO_LIST("<dmarcmungetest>", TEST_EMAIL_EXTERNAL, "External Sender <" TEST_EMAIL_EXTERNAL ">");
248-
SWRITE(client1, "b1 FETCH 7 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
254+
SWRITE(client1, "f1 FETCH 8 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
249255
CLIENT_EXPECT_EVENTUALLY(client1, "Reply-To: External Sender <" TEST_EMAIL_EXTERNAL ">");
250256
/* Ensure that if there was a name in the address, the munged From header still includes it
251257
* If TEST_WITH_REAL_DMARC_POLICIES is defined, we'll still munge since TEST_EMAIL_EXTERNAL uses a domain with p=reject and sp=reject */
252-
SWRITE(client1, "b2 FETCH 7 (BODY.PEEK[HEADER.FIELDS (From)])" ENDL);
258+
SWRITE(client1, "f2 FETCH 8 (BODY.PEEK[HEADER.FIELDS (From)])" ENDL);
253259
CLIENT_EXPECT_EVENTUALLY(client1, "From: External Sender <external=" TEST_EXTERNAL_DOMAIN "@" TEST_HOSTNAME ">");
254260

255261
/* Repeat, with a quoted name */
256262
POST_TO_LIST("<dmarcmungetest>", TEST_EMAIL_EXTERNAL, "\"External Sender\" <" TEST_EMAIL_EXTERNAL ">");
257-
SWRITE(client1, "b3 FETCH 8 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
263+
SWRITE(client1, "g1 FETCH 9 (BODY.PEEK[HEADER.FIELDS (Reply-To)])" ENDL);
258264
CLIENT_EXPECT_EVENTUALLY(client1, "Reply-To: \"External Sender\" <" TEST_EMAIL_EXTERNAL ">");
259-
SWRITE(client1, "b4 FETCH 8 (BODY.PEEK[HEADER.FIELDS (From)])" ENDL);
265+
SWRITE(client1, "g2 FETCH 9 (BODY.PEEK[HEADER.FIELDS (From)])" ENDL);
260266
CLIENT_EXPECT_EVENTUALLY(client1, "From: \"External Sender\" <external=" TEST_EXTERNAL_DOMAIN "@" TEST_HOSTNAME ">");
261267

262268
res = 0;

0 commit comments

Comments
 (0)