From 4e75694ac6fd107764ec4f3a5447b59e0dc62e0e Mon Sep 17 00:00:00 2001
From: Girish Ramakrishnan <girish@cloudron.io>
Date: Sun, 11 Sep 2022 11:15:21 +0200
Subject: [PATCH] mail: require catch all to be absolute

---
 CHANGES                                        |  1 +
 ...220911090713-mail-catchAll-make-absolute.js | 18 ++++++++++++++++++
 src/mail.js                                    |  4 ++++
 src/routes/test/mail-test.js                   | 13 +++++++++++--
 src/test/mail-test.js                          | 11 ++++++++---
 5 files changed, 42 insertions(+), 5 deletions(-)
 create mode 100644 migrations/20220911090713-mail-catchAll-make-absolute.js

diff --git a/CHANGES b/CHANGES
index 31ef97260..e44fd907e 100644
--- a/CHANGES
+++ b/CHANGES
@@ -2514,4 +2514,5 @@
 * eventlog: distinguish ghost/impersonate logins from others
 * ldap: remove virtual user and admin groups to ldap user records
 * Randomize certificate generation cronjob to lighten load on Let's Encrypt servers
+* mail: catch all address can be any domain
 
diff --git a/migrations/20220911090713-mail-catchAll-make-absolute.js b/migrations/20220911090713-mail-catchAll-make-absolute.js
new file mode 100644
index 000000000..fbd261ec9
--- /dev/null
+++ b/migrations/20220911090713-mail-catchAll-make-absolute.js
@@ -0,0 +1,18 @@
+'use strict';
+
+const safe = require('safetydance');
+
+exports.up = async function (db) {
+    const mailDomains = await db.runSql('SELECT * FROM mail', []);
+
+    for (const mailDomain of mailDomains) {
+        let catchAll = safe.JSON.parse(mailDomain.catchAllJson) || [];
+        if (catchAll.length === 0) continue;
+
+        catchAll = catchAll.map(a => `${a}@${mailDomain.domain}`);
+        await db.runSql('UPDATE mail SET catchAllJson = ? WHERE domain = ?', [ JSON.stringify(catchAll), mailDomain.domain ]);
+    }
+};
+
+exports.down = async function( /* db */) {
+};
diff --git a/src/mail.js b/src/mail.js
index e140b046a..7959bad65 100644
--- a/src/mail.js
+++ b/src/mail.js
@@ -1042,6 +1042,10 @@ async function setCatchAllAddress(domain, addresses) {
     assert.strictEqual(typeof domain, 'string');
     assert(Array.isArray(addresses));
 
+    for (const address of addresses) {
+        if (!validator.isEmail(address)) throw new BoxError(BoxError.BAD_FIELD, `Invalid catch all address: ${address}`);
+    }
+
     await updateDomain(domain, { catchAll: addresses });
 
     safe(restartMail(), { debug }); // have to restart mail container since haraka cannot watch symlinked config files (mail.ini)
diff --git a/src/routes/test/mail-test.js b/src/routes/test/mail-test.js
index 59fd614a9..a5e34a754 100644
--- a/src/routes/test/mail-test.js
+++ b/src/routes/test/mail-test.js
@@ -352,10 +352,19 @@ describe('Mail API', function () {
             expect(response.statusCode).to.equal(400);
         });
 
+        it('cannot set with bad addresses field', async function () {
+            const response = await superagent.post(`${serverUrl}/api/v1/mail/${dashboardDomain}/catch_all`)
+                .query({ access_token: owner.token })
+                .send({ addresses: [ 'user1' ] })
+                .ok(() => true);
+
+            expect(response.statusCode).to.equal(400);
+        });
+
         it('set succeeds', async function () {
             const response = await superagent.post(`${serverUrl}/api/v1/mail/${dashboardDomain}/catch_all`)
                 .query({ access_token: owner.token })
-                .send({ addresses: [ 'user1' ] });
+                .send({ addresses: [ `user1@${dashboardDomain}` ] });
 
             expect(response.statusCode).to.equal(202);
         });
@@ -365,7 +374,7 @@ describe('Mail API', function () {
                 .query({ access_token: owner.token });
 
             expect(response.statusCode).to.equal(200);
-            expect(response.body.catchAll).to.eql([ 'user1' ]);
+            expect(response.body.catchAll).to.eql([ `user1@${dashboardDomain}` ]);
         });
     });
 
diff --git a/src/test/mail-test.js b/src/test/mail-test.js
index 353549a6b..4e736c77b 100644
--- a/src/test/mail-test.js
+++ b/src/test/mail-test.js
@@ -40,11 +40,16 @@ describe('Mail', function () {
             expect(mailConfig.mailFromValidation).to.be(false);
         });
 
-        it('can set catch all address', async function () {
-            await mail.setCatchAllAddress(domain.domain, [ 'user1', 'user2' ]);
+        it('cannot set invalid catch all address', async function () {
+            const [error] = await safe(mail.setCatchAllAddress(domain.domain, [ 'user1' ]));
+            expect(error.reason).to.be(BoxError.BAD_FIELD);
+        });
+
+        it('can set invalid catch all address', async function () {
+            await mail.setCatchAllAddress(domain.domain, [ `user1@${domain.domain}`, `user2@${domain.domain}` ]);
 
             const mailConfig = await mail.getDomain(domain.domain);
-            expect(mailConfig.catchAll).to.eql([ 'user1', 'user2' ]);
+            expect(mailConfig.catchAll).to.eql([ `user1@${domain.domain}`, `user2@${domain.domain}` ]);
         });
 
         it('can set mail relay', async function () {