Skip to content

Commit 39e0155

Browse files
NickOvtNickOvt
authored
fix: ZMS-28: filters, return full http forwarding target (#1027)
* tools.js, replace usage of urlib with new URL * ZMS-28: filters, return full http forwarding target * adjust tests, add correct options * new URL can throw on error, add try catch * on ipv6 hostname remove brackets for correct getRelayData response object * add getRelayData tests
1 parent df2f69d commit 39e0155

File tree

4 files changed

+180
-26
lines changed

4 files changed

+180
-26
lines changed

‎lib/api/filters.js‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
const log = require('npmlog');
44
const Joi = require('joi');
55
const ObjectId = require('mongodb').ObjectId;
6-
const urllib = require('url');
76
const tools = require('../tools');
87
const roles = require('../roles');
98
const { nextPageCursorSchema, previousPageCursorSchema, sessSchema, sessIPSchema, booleanSchema, metaDataSchema } = require('../schemas');
@@ -173,7 +172,7 @@ module.exports = (db, server, userHandler, settingsHandler) => {
173172
previousCursor: listingWrapper.previousCursor,
174173
nextCursor: listingWrapper.nextCursor,
175174
results: (listingWrapper.listing.results || []).map(filterData => {
176-
let descriptions = getFilterStrings(filterData, mailboxes);
175+
let descriptions = getFilterStrings(filterData, mailboxes, { preserveTargetUrls: true });
177176

178177
let values = {
179178
id: filterData._id.toString(),
@@ -381,7 +380,7 @@ module.exports = (db, server, userHandler, settingsHandler) => {
381380
},
382381

383382
results: filters.map(filterData => {
384-
let descriptions = getFilterStrings(filterData, mailboxes);
383+
let descriptions = getFilterStrings(filterData, mailboxes, { preserveTargetUrls: true });
385384

386385
const values = {
387386
id: filterData._id.toString(),
@@ -1283,7 +1282,9 @@ module.exports = (db, server, userHandler, settingsHandler) => {
12831282
);
12841283
};
12851284

1286-
function getFilterStrings(filter, mailboxes) {
1285+
function getFilterStrings(filter, mailboxes, options) {
1286+
options = options || {};
1287+
12871288
let query = Object.keys(filter.query.headers || {}).map(key => [key, '(' + filter.query.headers[key] + ')']);
12881289

12891290
if (filter.query.ha && filter.query.ha > 0) {
@@ -1347,15 +1348,16 @@ function getFilterStrings(filter, mailboxes) {
13471348
'forward to',
13481349
filter.action[key]
13491350
.map(target => {
1350-
switch (target.type) {
1351-
case 'http': {
1352-
let parsed = urllib.parse(target.value);
1353-
return parsed.hostname || parsed.host;
1354-
}
1355-
1356-
default:
1351+
if (target.type === 'http' && !options.preserveTargetUrls) {
1352+
try {
1353+
let parsed = new URL(target.value);
1354+
return parsed.hostname || parsed.host || target.value;
1355+
} catch (err) {
13571356
return target.value;
1357+
}
13581358
}
1359+
1360+
return target.value;
13591361
})
13601362
.join(', ')
13611363
];

‎lib/tools.js‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ const fs = require('fs');
1010
const he = require('he');
1111
const pathlib = require('path');
1212
const crypto = require('crypto');
13-
const urllib = require('url');
1413
const net = require('net');
1514
const ipaddr = require('ipaddr.js');
1615
const ObjectId = require('mongodb').ObjectId;
@@ -385,22 +384,29 @@ function escapeRegexStr(string) {
385384
}
386385

387386
function getRelayData(url) {
388-
let urlparts = urllib.parse(url);
387+
let urlparts;
388+
try {
389+
urlparts = new URL(url);
390+
} catch {
391+
urlparts = {};
392+
}
393+
let hostname = urlparts.hostname || '';
394+
if (/^\[[^\]]+\]$/.test(hostname)) {
395+
hostname = hostname.slice(1, -1);
396+
}
389397
let targetMx = {
390-
host: urlparts.hostname,
398+
host: hostname,
391399
port: urlparts.port || 25,
392-
auth: urlparts.auth
393-
? [urlparts.auth].map(auth => {
394-
let parts = auth.split(':');
395-
return {
396-
user: decodeURIComponent(parts[0] || ''),
397-
pass: decodeURIComponent(parts[1] || '')
398-
};
399-
})[0]
400-
: false,
400+
auth:
401+
urlparts.username || urlparts.password
402+
? {
403+
user: decodeURIComponent(urlparts.username || ''),
404+
pass: decodeURIComponent(urlparts.password || '')
405+
}
406+
: false,
401407
secure: urlparts.protocol === 'smtps:',
402-
A: [].concat(net.isIPv4(urlparts.hostname) ? urlparts.hostname : []),
403-
AAAA: [].concat(net.isIPv6(urlparts.hostname) ? urlparts.hostname : [])
408+
A: [].concat(net.isIPv4(hostname) ? hostname : []),
409+
AAAA: [].concat(net.isIPv6(hostname) ? hostname : [])
404410
};
405411
let data = {
406412
mx: [

‎test/api/filters-test.js‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,61 @@ describe('API Filters', function () {
207207
expect(responseGet.body.action.mailbox).to.be.equal(inbox);
208208
});
209209

210+
it('should preserve full HTTP target URL when returning filter listings', async () => {
211+
const targetUrl = 'https://example.com/inbound/email?token=abc123';
212+
const createResponse = await server
213+
.post(`/users/${user}/filters`)
214+
.send({
215+
name: 'http target filter',
216+
query: {
217+
from: 'http-target'
218+
},
219+
action: {
220+
targets: [targetUrl]
221+
}
222+
})
223+
.expect(200);
224+
expect(createResponse.body.success).to.be.true;
225+
226+
const filter = createResponse.body.id;
227+
228+
const authResponse = await server
229+
.post('/authenticate')
230+
.send({
231+
username: 'filteruser',
232+
password: 'secretvalue',
233+
token: true
234+
})
235+
.expect(200);
236+
expect(authResponse.body.success).to.be.true;
237+
238+
const userListResponse = await server.get(`/users/${user}/filters`).expect(200);
239+
expect(userListResponse.body.success).to.be.true;
240+
241+
const userListFilter = userListResponse.body.results.find(entry => entry.id === filter);
242+
expect(userListFilter).to.exist;
243+
expect(userListFilter.action).to.deep.equal([['forward to', targetUrl]]);
244+
expect(userListFilter.originalAction.targets).to.deep.equal([targetUrl]);
245+
246+
const ownFiltersResponse = await server.get(`/filters?accessToken=${authResponse.body.token}`).expect(200);
247+
expect(ownFiltersResponse.body.success).to.be.true;
248+
249+
const ownFiltersEntry = ownFiltersResponse.body.results.find(entry => entry.id === filter);
250+
expect(ownFiltersEntry).to.exist;
251+
expect(ownFiltersEntry.action).to.deep.equal([['forward to', targetUrl]]);
252+
expect(ownFiltersEntry.originalAction.targets).to.deep.equal([targetUrl]);
253+
expect(ownFiltersEntry.targets).to.deep.equal([targetUrl]);
254+
255+
const allFiltersResponse = await server.get(`/filters`).expect(200);
256+
expect(allFiltersResponse.body.success).to.be.true;
257+
258+
const allFiltersEntry = allFiltersResponse.body.results.find(entry => entry.id === filter);
259+
expect(allFiltersEntry).to.exist;
260+
expect(allFiltersEntry.action).to.deep.equal([['forward to', targetUrl]]);
261+
expect(allFiltersEntry.originalAction.targets).to.deep.equal([targetUrl]);
262+
expect(allFiltersEntry.targets).to.deep.equal([targetUrl]);
263+
});
264+
210265
describe('Filter spam action', function () {
211266
let spamFilter;
212267

‎test/filtering-tools-test.js‎

Lines changed: 92 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
'use strict';
33

44
const { expect } = require('chai');
5-
const { extractQuotedPhrases, parseFilterQueryText, filterQueryTermMatches } = require('../lib/tools');
5+
const { extractQuotedPhrases, parseFilterQueryText, filterQueryTermMatches, getRelayData } = require('../lib/tools');
66

77
describe('Email Filtering helper functions', () => {
88
describe('extractQuotedPhrases', () => {
@@ -371,4 +371,95 @@ describe('Email Filtering helper functions', () => {
371371
expect(parsed.exactPhrases).to.deep.equal(['first phrase', 'second phrase']);
372372
});
373373
});
374+
375+
describe('getRelayData', () => {
376+
[
377+
{
378+
title: 'should return expected structure for a hostname relay without auth',
379+
input: 'smtp://mx.example.com',
380+
expected: {
381+
mx: [
382+
{
383+
priority: 0,
384+
mx: true,
385+
exchange: 'mx.example.com',
386+
A: [],
387+
AAAA: []
388+
}
389+
],
390+
mxPort: 25,
391+
mxAuth: false,
392+
mxSecure: false,
393+
url: 'smtp://mx.example.com'
394+
}
395+
},
396+
{
397+
title: 'should return expected structure for a hostname relay with auth and port',
398+
input: 'smtp://user:pass@mx.example.com:2525',
399+
expected: {
400+
mx: [
401+
{
402+
priority: 0,
403+
mx: true,
404+
exchange: 'mx.example.com',
405+
A: [],
406+
AAAA: []
407+
}
408+
],
409+
mxPort: '2525',
410+
mxAuth: {
411+
user: 'user',
412+
pass: 'pass'
413+
},
414+
mxSecure: false,
415+
url: 'smtp://user:pass@mx.example.com:2525'
416+
}
417+
},
418+
{
419+
title: 'should return expected structure for an IPv4 relay',
420+
input: 'smtp://192.0.2.15:25',
421+
expected: {
422+
mx: [
423+
{
424+
priority: 0,
425+
mx: true,
426+
exchange: '192.0.2.15',
427+
A: ['192.0.2.15'],
428+
AAAA: []
429+
}
430+
],
431+
mxPort: '25',
432+
mxAuth: false,
433+
mxSecure: false,
434+
url: 'smtp://192.0.2.15:25'
435+
}
436+
},
437+
{
438+
title: 'should return expected structure for an IPv6 relay',
439+
input: 'smtps://user:p%40ss@[2001:db8::1]:465',
440+
expected: {
441+
mx: [
442+
{
443+
priority: 0,
444+
mx: true,
445+
exchange: '2001:db8::1',
446+
A: [],
447+
AAAA: ['2001:db8::1']
448+
}
449+
],
450+
mxPort: '465',
451+
mxAuth: {
452+
user: 'user',
453+
pass: 'p@ss'
454+
},
455+
mxSecure: true,
456+
url: 'smtps://user:p%40ss@[2001:db8::1]:465'
457+
}
458+
}
459+
].forEach(testCase => {
460+
it(testCase.title, () => {
461+
expect(getRelayData(testCase.input)).to.deep.equal(testCase.expected);
462+
});
463+
});
464+
});
374465
});

0 commit comments

Comments
 (0)