show proper error message on ill-formed from (#238)

CHANGELOG.txt

@@ -1,3 +1,5 @@
+ - show proper error message on ill-formed from (#238)
+
 4.0.0 [2021-04-18]
 ------------------
  - now requires at least Thunderbird 78

_locales/de/messages.json

@@ -34,6 +34,9 @@
 	"DKIM_INTERNALERROR_INCORRECT_EMAIL_FORMAT": {
 		"message": "E-Mail ist nicht richtig formatiert"
 	},
+	"DKIM_INTERNALERROR_INCORRECT_FROM": {
+		"message": "Absenderadresse ist falsch formatiert"
+	},
 	// DKIM_INTERNALERROR - DNS
 	"DKIM_DNSERROR_UNKNOWN": {
 		"message": "Fehler im DNS-Resolver"

_locales/en_US/messages.json

@@ -34,6 +34,9 @@
 	"DKIM_INTERNALERROR_INCORRECT_EMAIL_FORMAT": {
 		"message": "E-mail is not correctly formatted"
 	},
+	"DKIM_INTERNALERROR_INCORRECT_FROM": {
+		"message": "From address is ill-formed"
+	},
 	// DKIM_INTERNALERROR - DNS
 	"DKIM_DNSERROR_UNKNOWN": {
 		"message": "Error in the DNS resolver"

modules/authVerifier.mjs.js

@@ -124,10 +124,25 @@ export default class AuthVerifier {
 		if (!fromHeader) {
 			throw new Error("message does not contain a from header");
 		}
+		let from;
+		try {
+			from = MsgParser.parseFromHeader(fromHeader[0]);
+		} catch (error) {
+			log.error("Parsing of from header failed", error);
+			return Promise.resolve({
+				version: "2.1",
+				dkim: [{
+					version: "2.0",
+					result: "PERMFAIL",
+					res_num: 30,
+					result_str: browser.i18n.getMessage("DKIM_INTERNALERROR_INCORRECT_FROM"),
+				}],
+			});
+		}
 		const msg = {
 			headerFields: msgParsed.headers,
 			bodyPlain: msgParsed.body,
-			from: MsgParser.parseFromHeader(fromHeader[0]),
+			from: from,
 		};
 		const listIdHeader = msgParsed.headers.get("list-id");
 		let listId = "";
@@ -774,7 +789,7 @@ function SavedAuthResult_to_AuthResult(savedAuthResult) {
 }
 
 /**
- * Convert AuthResultV2 to dkimSigResultV2
+ * Convert AuthResultDKIMV2 to dkimSigResultV2
  *
  * @param {AuthResultDKIMV2} authResultDKIM
  * @return {VerifierModule.dkimSigResultV2} dkimSigResultV2

test/data/ill_formed-from.eml

@@ -0,0 +1,14 @@
+Received: from client1.football.example.com  [192.0.2.1]
+      by submitserver.example.com with SUBMISSION;
+      Fri, 11 Jul 2003 21:01:54 -0700 (PDT)
+From: Joe SixPack <joe]@football.example.com>
+To: Suzie Q <suzie@shopping.example.net>
+Subject: Is dinner ready? (ill-formed from)
+Date: Fri, 11 Jul 2003 21:00:37 -0700 (PDT)
+Message-ID: <20030712040037.46341.5F8J-ill_formed-from@football.example.com>
+
+Hi.
+
+We lost the game. Are you hungry yet?
+
+Joe.

test/unittest/authVerifierSpec.mjs.js

@@ -45,6 +45,20 @@ function createFakeMessageHeader() {
 	};
 }
 
+/**
+ * @param {Map<string, string[]>} headers
+ * @param {string} name
+ * @returns {string[]}
+ */
+function extractHeaderValue(headers, name) {
+	const completeHeaders = headers.get(name);
+	if (completeHeaders === undefined) {
+		return [];
+	}
+	return completeHeaders.map(header =>
+		header.substr(name.length + ": ".length).slice(0, -"\r\n".length));
+}
+
 /**
  * @param {string} file - path to file relative to test data directory
  * @returns {Promise<browser.messageDisplay.MessageHeader>}
@@ -53,12 +67,12 @@ async function createMessageHeader(file) {
 	const fakeMessageHeader = createFakeMessageHeader();
 	const msgPlain = await readTestFile(file);
 	const msgParsed = MsgParser.parseMsg(msgPlain);
-	fakeMessageHeader.author = (msgParsed.headers.get("from") ?? [])[0];
-	fakeMessageHeader.recipients = msgParsed.headers.get("to") ?? [];
-	fakeMessageHeader.subject = (msgParsed.headers.get("subject") ?? [])[0];
+	fakeMessageHeader.author = extractHeaderValue(msgParsed.headers, "from")[0];
+	fakeMessageHeader.recipients = extractHeaderValue(msgParsed.headers, "to");
+	fakeMessageHeader.subject = extractHeaderValue(msgParsed.headers, "subject")[0];
 	browser.messages = {
 		getRaw: sinon.fake.resolves(msgPlain),
-		getFull: sinon.fake.throws("no fake for browser.messages.messages"),
+		getFull: sinon.fake.throws("no fake for browser.messages.getFull"),
 	};
 	return fakeMessageHeader;
 }
@@ -73,14 +87,17 @@ describe("AuthVerifier [unittest]", function () {
 			this.skip();
 		}
 		await prefs.init();
+	});
+
+	beforeEach(async function () {
 		await prefs.clear();
 	});
 
 	describe("saving of results", function () {
 		/** @type {import("../../modules/dkim/verifier.mjs.js").dkimResultV1|import("../../modules/authVerifier.mjs.js").AuthResultV2|import("../../modules/authVerifier.mjs.js").SavedAuthResultV3} */
 		let storedData;
 
-		before(async function () {
+		beforeEach(async function () {
 			await prefs.setValue("saveResult", true);
 
 			browser.storageMessage = {
@@ -192,11 +209,8 @@ describe("AuthVerifier [unittest]", function () {
 			expect(res.dmarc && res.dmarc[0].result).to.be.equal("pass");
 		});
 	});
-	describe("sign rules", function () {
-		beforeEach(async function () {
-			await prefs.clear();
-		});
 
+	describe("sign rules", function () {
 		it("unsigned PayPal message", async function () {
 			const fakePayPalMessage = await createMessageHeader("fakePayPal.eml");
 			let res = await authVerifier.verify(fakePayPalMessage);
@@ -239,11 +253,8 @@ describe("AuthVerifier [unittest]", function () {
 			expect(res.dkim[0].result_str).to.be.equal("Invalid (Should be signed by paypal.com)");
 		});
 	});
-	describe("ARH header", function () {
-		beforeEach(async function () {
-			await prefs.clear();
-		});
 
+	describe("ARH header", function () {
 		it("spf and dkim result", async function () {
 			const message = await createMessageHeader("rfc6376-A.2-arh-valid.eml");
 			let res = await authVerifier.verify(message);
@@ -327,4 +338,13 @@ describe("AuthVerifier [unittest]", function () {
 			});
 		});
 	});
+
+	describe("invalid messages", function () {
+		it("ill-formed from shows proper error message", async function () {
+			const message = await createMessageHeader("ill_formed-from.eml");
+			const res = await authVerifier.verify(message);
+			expect(res.dkim[0].result).to.be.equal("PERMFAIL");
+			expect(res.dkim[0].result_str).to.be.equal("From address is ill-formed");
+		});
+	});
 });