ARH: improve sorting of failed results (#247)

lieser · Jul 16, 2022 · a92be02 · a92be02
1 parent 35e0e75
commit a92be02
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file.
 - Added heuristic to detect maliciously added unsigned headers (#102).
 - Warn about unsigned headers that are recommended to be signed (#102, #277).
 - Improved theming of header icon in Thunderbird 102.
+- Authentication-Results header: Prefer to show failure results that include a reason and are related to the sending domain (#247).
 
 ### Fixes
 

diff --git a/modules/authVerifier.mjs.js b/modules/authVerifier.mjs.js
@@ -552,6 +552,35 @@ function sortSignatures(signatures, from, listId) {
 		return 0;
 	}
 
+	/**
+	 * Compare the error reason of two signatures.
+	 *
+	 * @param {VerifierModule.dkimSigResultV2} sig1
+	 * @param {VerifierModule.dkimSigResultV2} sig2
+	 * @returns {number}
+	 */
+	function error_compare(sig1, sig2) {
+		if (sig1.result !== "PERMFAIL") {
+			return 0;
+		}
+		if (sig1.errorType) {
+			// sig1 has an error type
+			if (sig2.errorType) {
+				// both signatures have an error type
+				return 0;
+			}
+			// sig2 has no error type
+			return -1;
+		}
+		// sig1 has no error type
+		if (sig2.errorType) {
+			// sig2 has an error type
+			return 1;
+		}
+		// both signatures have no error type
+		return 0;
+	}
+
 	signatures.sort(function (sig1, sig2) {
 		let cmp;
 		cmp = result_compare(sig1, sig2);
@@ -566,6 +595,10 @@ function sortSignatures(signatures, from, listId) {
 		if (cmp !== 0) {
 			return cmp;
 		}
+		cmp = error_compare(sig1, sig2);
+		if (cmp !== 0) {
+			return cmp;
+		}
 		return 0;
 	});
 }
@@ -587,18 +620,6 @@ function arhDKIM_to_dkimSigResultV2(arhDKIM) {
 		case "pass": {
 			dkimSigResult.result = "SUCCESS";
 			dkimSigResult.warnings = [];
-			let sdid = arhDKIM.propertys.header.d;
-			let auid = arhDKIM.propertys.header.i;
-			if (sdid || auid) {
-				if (!sdid) {
-					// @ts-expect-error
-					sdid = getDomainFromAddr(auid);
-				} else if (!auid) {
-					auid = `@${sdid}`;
-				}
-				dkimSigResult.sdid = sdid;
-				dkimSigResult.auid = auid;
-			}
 			break;
 		}
 		case "fail":
@@ -623,6 +644,18 @@ function arhDKIM_to_dkimSigResultV2(arhDKIM) {
 		default:
 			throw new DKIM_InternalError(`invalid dkim result in arh: ${arhDKIM.result}`);
 	}
+	let sdid = arhDKIM.propertys.header.d;
+	let auid = arhDKIM.propertys.header.i;
+	if (sdid || auid) {
+		if (!sdid) {
+			// @ts-expect-error
+			sdid = getDomainFromAddr(auid);
+		} else if (!auid) {
+			auid = `@${sdid}`;
+		}
+		dkimSigResult.sdid = sdid;
+		dkimSigResult.auid = auid;
+	}
 	return dkimSigResult;
 }
 

diff --git a/test/data/arh-multiple_dkim_results.eml b/test/data/arh-multiple_dkim_results.eml
@@ -1,6 +1,10 @@
 Authentication-Results: example.net;
+  dkim=fail header.d=example.com;
   dkim=pass header.d=unrelated.org;
   dkim=pass header.d=example.com;
+  dkim=fail reason="test failure" header.d=example.com;
+  dkim=fail reason="test failure signed by unrelated" header.d=unrelated.org;
+  dkim=fail header.d=example.com;
   dkim=pass header.d=example.org
 Received: from client1.football.example.com  [192.0.2.1]
       by submitserver.example.com with SUBMISSION;

diff --git a/test/unittest/authVerifierSpec.mjs.js b/test/unittest/authVerifierSpec.mjs.js
@@ -335,6 +335,14 @@ describe("AuthVerifier [unittest]", function () {
 				expect(res.dkim[1]?.sdid).to.be.equal("example.org");
 				expect(res.dkim[2]?.result).to.be.equal("SUCCESS");
 				expect(res.dkim[2]?.sdid).to.be.equal("unrelated.org");
+				expect(res.dkim[3]?.result).to.be.equal("PERMFAIL");
+				expect(res.dkim[3]?.result_str).to.be.equal("Invalid (test failure)");
+				expect(res.dkim[4]?.result).to.be.equal("PERMFAIL");
+				expect(res.dkim[4]?.result_str).to.be.equal("Invalid");
+				expect(res.dkim[5]?.result).to.be.equal("PERMFAIL");
+				expect(res.dkim[5]?.result_str).to.be.equal("Invalid");
+				expect(res.dkim[6]?.result).to.be.equal("PERMFAIL");
+				expect(res.dkim[6]?.result_str).to.be.equal("Invalid (test failure signed by unrelated)");
 			});
 			it("With secure signature algorithm", async function () {
 				const message = await createMessageHeader("rfc6376-A.2-arh-valid-a_tag.eml");