Skip to content

Commit b28181f

Browse files
lieserlieser
committed
Always sort ARH results (#534)
1 parent 41c2006 commit b28181f

File tree

4 files changed

+122
-7
lines changed

4 files changed

+122
-7
lines changed

‎CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ All notable changes to this project will be documented in this file.
66

77
## Unreleased
88

9+
### Enhancements
10+
11+
- Authentication-Results header: Sort DKIM, SPF and DMARC results from ARH, even when not replacing the add-ons verification (#534).
12+
913
### Fixes
1014

1115
- Libunbound resolver: Make unloading of libraries more robust in case the wrong one got loaded.

‎modules/arhVerifier.mjs.js

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,9 @@
1414

1515
// @ts-check
1616
/* eslint-disable camelcase */
17+
/* eslint-disable no-magic-numbers */
1718

18-
import { addrIsInDomain, copy, domainIsInDomain, getDomainFromAddr, stringEndsWith } from "./utils.mjs.js";
19+
import { addrIsInDomain, copy, domainIsInDomain, getDomainFromAddr, stringEndsWith, stringEqual } from "./utils.mjs.js";
1920
import ArhParser from "./arhParser.mjs.js";
2021
import Logging from "./logging.mjs.js";
2122
import { getBimiIndicator } from "./bimi.mjs.js";
@@ -241,6 +242,67 @@ function checkSignatureAlgorithm(dkimSigResult) {
241242
}
242243
}
243244
}
245+
246+
/**
247+
* Converts an ARH result Keyword to a sorted number.
248+
*
249+
* @param {string} result
250+
* @returns {number}
251+
*/
252+
function resultToNumber(result) {
253+
if (stringEqual(result, "pass")) {
254+
return 0;
255+
}
256+
257+
if (stringEqual(result, "neutral")) {
258+
return 10;
259+
}
260+
if (stringEqual(result, "declined")) {
261+
return 11;
262+
}
263+
if (stringEqual(result, "policy")) {
264+
return 12;
265+
}
266+
267+
if (stringEqual(result, "hardfail")) {
268+
return 20;
269+
}
270+
if (stringEqual(result, "fail")) {
271+
return 21;
272+
}
273+
if (stringEqual(result, "softfail")) {
274+
return 22;
275+
}
276+
277+
if (stringEqual(result, "permerror")) {
278+
return 30;
279+
}
280+
if (stringEqual(result, "temperror")) {
281+
return 31;
282+
}
283+
284+
if (stringEqual(result, "skipped")) {
285+
return 40;
286+
}
287+
if (stringEqual(result, "none")) {
288+
return 41;
289+
}
290+
291+
return 99;
292+
}
293+
294+
/**
295+
* Sort the given ArhResInfo.
296+
*
297+
* @param {ArhResInfo[]} arhResInfo
298+
* @returns {void}
299+
*/
300+
function sortResultKeyword(arhResInfo) {
301+
arhResInfo.sort((resInfo1, resInfo2) => {
302+
return resultToNumber(resInfo1.result) - resultToNumber(resInfo2.result);
303+
});
304+
}
305+
244306
/**
245307
* Get the Authentication-Results header as an SavedAuthResult.
246308
*
@@ -284,6 +346,8 @@ export default function getArhResult(headers, from, account) {
284346
dmarc: authenticationResults.dmarc,
285347
bimiIndicator: getBimiIndicator(headers, authenticationResults.bimi) ?? undefined,
286348
};
349+
sortResultKeyword(savedAuthResult.spf);
350+
sortResultKeyword(savedAuthResult.dmarc);
287351
log.debug("ARH result:", copy(savedAuthResult));
288352
return savedAuthResult;
289353
}

‎modules/authVerifier.mjs.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ export default class AuthVerifier {
181181
await checkSignRules(message, savedAuthResult.dkim, msg.from, listId, this._dmarc);
182182
sortSignatures(savedAuthResult.dkim, msg.from, listId);
183183
} else {
184+
sortSignatures(arhResult.dkim, msg.from, listId);
184185
savedAuthResult = {
185186
version: "3.1",
186187
dkim: [],

‎test/unittest/authVerifierSpec.mjs.js

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
// @ts-check
1111
/* eslint-disable camelcase */
12+
/* eslint-disable complexity */
1213

1314
import { FakeMessageHeader, fakeBrowser } from "../helpers/initWebExtensions.mjs.js";
1415
import AuthVerifier from "../../modules/authVerifier.mjs.js";
@@ -402,7 +403,6 @@ describe("AuthVerifier [unittest]", function () {
402403
expect(res.dkim[0]?.result_str).to.be.equal("Invalid (No Signature, should be signed by paypal.com)");
403404
});
404405

405-
// eslint-disable-next-line complexity
406406
it("Failure because of wrong SDID keeps signature meta data", async function () {
407407
const message = await fakeBrowser.messages.addMsg("rfc6376-A.2.eml");
408408

@@ -492,6 +492,30 @@ describe("AuthVerifier [unittest]", function () {
492492
expect((res.spf ?? [])[0]?.result).to.be.equal("pass");
493493
});
494494

495+
it("SPF and DMARC results should be sorted", async function () {
496+
await prefs.setValue("arh.read", true);
497+
498+
const message = await fakeBrowser.messages.addMsg("arh/multiple_spf_and_dmarc_results.eml");
499+
const res = await authVerifier.verify(message);
500+
const spf = res.spf ?? [];
501+
expect(spf.length).to.be.equal(8);
502+
expect(spf[0]?.result).to.be.equal("pass");
503+
expect(spf[1]?.result).to.be.equal("neutral");
504+
expect(spf[2]?.result).to.be.equal("policy");
505+
expect(spf[3]?.result).to.be.equal("fail");
506+
expect(spf[4]?.result).to.be.equal("softfail");
507+
expect(spf[5]?.result).to.be.equal("permerror");
508+
expect(spf[6]?.result).to.be.equal("temperror");
509+
expect(spf[7]?.result).to.be.equal("none");
510+
const dmarc = res.dmarc ?? [];
511+
expect(dmarc.length).to.be.equal(5);
512+
expect(dmarc[0]?.result).to.be.equal("pass");
513+
expect(dmarc[1]?.result).to.be.equal("fail");
514+
expect(dmarc[2]?.result).to.be.equal("permerror");
515+
expect(dmarc[3]?.result).to.be.equal("temperror");
516+
expect(dmarc[4]?.result).to.be.equal("none");
517+
});
518+
495519
describe("Converting of ARH result to DKIM result", function () {
496520
beforeEach(async function () {
497521
await prefs.setValue("dkim.enable", false);
@@ -644,7 +668,7 @@ describe("AuthVerifier [unittest]", function () {
644668

645669
it("DKIM results should be sorted", async function () {
646670
const message = await fakeBrowser.messages.addMsg("arh/multiple_dkim_results.eml");
647-
const res = await authVerifier.verify(message);
671+
let res = await authVerifier.verify(message);
648672
expect(res.dkim.length).to.be.equal(7);
649673
expect(res.dkim[0]?.result).to.be.equal("SUCCESS");
650674
expect(res.dkim[0]?.sdid).to.be.equal("example.com");
@@ -660,6 +684,28 @@ describe("AuthVerifier [unittest]", function () {
660684
expect(res.dkim[5]?.result_str).to.be.equal("Invalid");
661685
expect(res.dkim[6]?.result).to.be.equal("PERMFAIL");
662686
expect(res.dkim[6]?.result_str).to.be.equal("Invalid (test failure signed by unrelated)");
687+
688+
await prefs.setValue("arh.replaceAddonResult", false);
689+
690+
res = await authVerifier.verify(message);
691+
expect(res.dkim.length).to.be.equal(1);
692+
expect(res.dkim[0]?.result).to.be.equal("none");
693+
const arhDkim = res.arh?.dkim ?? [];
694+
expect(arhDkim.length).to.be.equal(7);
695+
expect(arhDkim[0]?.result).to.be.equal("SUCCESS");
696+
expect(arhDkim[0]?.sdid).to.be.equal("example.com");
697+
expect(arhDkim[1]?.result).to.be.equal("SUCCESS");
698+
expect(arhDkim[1]?.sdid).to.be.equal("example.org");
699+
expect(arhDkim[2]?.result).to.be.equal("SUCCESS");
700+
expect(arhDkim[2]?.sdid).to.be.equal("unrelated.org");
701+
expect(arhDkim[3]?.result).to.be.equal("PERMFAIL");
702+
expect(arhDkim[3]?.result_str).to.be.equal("Invalid (test failure)");
703+
expect(arhDkim[4]?.result).to.be.equal("PERMFAIL");
704+
expect(arhDkim[4]?.result_str).to.be.equal("Invalid");
705+
expect(arhDkim[5]?.result).to.be.equal("PERMFAIL");
706+
expect(arhDkim[5]?.result_str).to.be.equal("Invalid");
707+
expect(arhDkim[6]?.result).to.be.equal("PERMFAIL");
708+
expect(arhDkim[6]?.result_str).to.be.equal("Invalid (test failure signed by unrelated)");
663709
});
664710

665711
it("With secure signature algorithm", async function () {
@@ -751,9 +797,9 @@ describe("AuthVerifier [unittest]", function () {
751797
const arhDkim = res.arh?.dkim ?? [];
752798
expect(arhDkim.length).to.be.equal(3);
753799
expect(arhDkim[0]?.result).to.be.equal("SUCCESS");
754-
expect(arhDkim[0]?.sdid).to.be.equal("example.com");
800+
expect(arhDkim[0]?.sdid).to.be.equal("football.example.com");
755801
expect(arhDkim[1]?.result).to.be.equal("SUCCESS");
756-
expect(arhDkim[1]?.sdid).to.be.equal("football.example.com");
802+
expect(arhDkim[1]?.sdid).to.be.equal("example.com");
757803
expect(arhDkim[2]?.result).to.be.equal("SUCCESS");
758804
expect(arhDkim[2]?.sdid).to.be.equal("last.example.com");
759805
expect((res.spf ?? [])[0]?.result).to.be.equal("pass");
@@ -830,9 +876,9 @@ describe("AuthVerifier [unittest]", function () {
830876
const arhDkim = res.arh?.dkim ?? [];
831877
expect(arhDkim.length).to.be.equal(3);
832878
expect(arhDkim[0]?.result).to.be.equal("SUCCESS");
833-
expect(arhDkim[0]?.sdid).to.be.equal("example.com");
879+
expect(arhDkim[0]?.sdid).to.be.equal("football.example.com");
834880
expect(arhDkim[1]?.result).to.be.equal("SUCCESS");
835-
expect(arhDkim[1]?.sdid).to.be.equal("football.example.com");
881+
expect(arhDkim[1]?.sdid).to.be.equal("example.com");
836882
expect(arhDkim[2]?.result).to.be.equal("SUCCESS");
837883
expect(arhDkim[2]?.sdid).to.be.equal("last.example.com");
838884
expect((res.spf ?? [])[0]?.result).to.be.equal("pass");

0 commit comments

Comments
 (0)