v2 of skipping over legacy-display protected header parts

v2 of skipping over legacy-display protected header parts

[Daniel Kahn Gillmor]

This is the second revision of the series that skips over
"legacy-display" protected header parts.

v1 can be found at id:20190531042825.27774-1-dkg@fifthhorseman.net

----------
Now that notmuch can handle and interpret protected subject lines, it
should also avoid forcing the user to look at "legacy display" parts
that some MUAs (notably enigmail) copies of the protected headers that
are intended to be rendered only by legacy clients -- clients capable
of decryption but which don't understand how to handle protected
headers.
----------

The main difference from version 1 is that this series now depends on
the two-part series "Setup for message repair", found at
id:20190531074027.16337-1-dkg@fifthhorseman.net, and should be
somewhat easier to merge with v3 of the "mixed-up message mangling
repair" series.  (previous versions of the "legacy display" and "mixed
up" series could fail in subtle ways when merged in the wrong order)

Comments welcome!

         --dkg

[PATCH v2 1/5] test: avoid showing legacy-display parts

[Daniel Kahn Gillmor]

Enigmail generates a "legacy-display" part when it sends encrypted
mail with a protected Subject: header.  This part is intended to
display the Subject for mail user agents that are capable of
decryption, but do not know how to deal with embedded protected
headers.

This part is the first child of a two-part multipart/mixed
cryptographic payload within a cryptographic envelope that includes
encryption (that is, it is not just a cleartext signed message).  It
uses Content-Type: text/rfc822-headers.

That is:

A └┬╴multipart/encrypted
B  ├─╴application/pgp-encrypted
C  └┬╴application/octet-stream
*   ╤ <decryption>
D   └┬╴multipart/mixed; protected-headers=v1 (cryptographic payload)
E    ├─╴text/rfc822-headers; protected-headers=v1 (legacy-display part)
F    └─╴… (actual message body)

In discussions with jrollins, i've come to the conclusion that a
legacy-display part should be stripped entirely from "notmuch show"
and "notmuch reply" now that these tools can understand and interpret
protected headers.

You can tell when a message part is a protected header part this way:

 * is the payload (D) multipart/mixed with exactly two children?
 * is its first child (E) Content-Type: text/rfc822-headers?
 * does the first child (E) have the property protected-headers=v1?
 * do all the headers in the body of the first child (E) match
   the protected headers in the payload part (D) itself?

If this is the case, and we already know how to deal with the
protected header, then there is no reason to try to render the
legacy-display part itself for the user.

Furthermore, when indexing, if we are indexing properly, we should
avoid indexing the text in E as part of the message body.

'notmuch reply' is an interesting case: the standard use of 'notmuch
reply' will end up omitting all mention of protected Subject:.

The right fix is for the replying MUA to be able to protect its
headers, and for it to set them appropriately based on headers found
in the original message.

If a replying MUA is unable to protect headers, but still wants the
user to be able to see the original header, a replying MUA that
notices that the original message's subject differs from the proposed
reply subject may choose to include the original's subject in the
quoted/attributed text. (this would be a stopgap measure; it's not
even clear that there is user demand for it)

This test suite change indicates what we want to happen for this case
(the tests are currently broken), and includes three additional TODO
suggestions of subtle cases for anyone who wants to flesh out the test
suite even further.  (i believe all these cases should be already
fixed by the rest of this series, but haven't had time to write the
tests for the unusual cases)

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 test/T356-protected-headers.sh                | 33 +++++++++++++++
 .../protected-with-legacy-display.eml         | 40 +++++++++++++++++++
 2 files changed, 73 insertions(+)
 create mode 100644 test/corpora/protected-headers/protected-with-legacy-display.eml

diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index 4af018f3..43dfffe6 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -136,4 +136,37 @@ id:nested-rfc822-message@crypto.notmuchmail.org
 id:protected-header@crypto.notmuchmail.org
 id:subjectless-protected-header@crypto.notmuchmail.org'
 
+test_begin_subtest "when rendering protected headers, avoid rendering legacy-display part"
+test_subtest_known_broken
+output=$(notmuch show --format=json id:protected-with-legacy-display@crypto.notmuchmail.org)
+test_json_nodes <<<"$output" \
+                'subject:[0][0][0]["headers"]["Subject"]="Interrupting Cow"' \
+                'no_legacy_display:[0][0][0]["body"][0]["content"][1]["content-type"]="text/plain"'
+
+test_begin_subtest "when replying, avoid rendering legacy-display part"
+test_subtest_known_broken
+output=$(notmuch reply --format=json id:protected-with-legacy-display@crypto.notmuchmail.org)
+test_json_nodes <<<"$output" \
+                'no_legacy_display:["original"]["body"][0]["content"][1]["content-type"]="text/plain"'
+
+test_begin_subtest "do not treat legacy-display part as body when indexing"
+test_subtest_known_broken
+output=$(notmuch search --output=messages body:interrupting)
+test_expect_equal "$output" ''
+
+test_begin_subtest "identify message that had a legacy display part skipped during indexing"
+test_subtest_known_broken
+output=$(notmuch search --output=messages property:index.repaired=skip-protected-headers-legacy-display)
+test_expect_equal "$output" id:protected-with-legacy-display@crypto.notmuchmail.org
+
+# TODO: test that a part that looks like a legacy-display in
+# multipart/signed, but not encrypted, is indexed and not stripped.
+
+# TODO: test that a legacy-display in a decrypted subpart (not in the
+# cryptographic payload) is indexed and not stripped.
+
+# TODO: test that a legacy-display inside multiple MIME layers that
+# include an encryption layer (e.g. multipart/encrypted around
+# multipart/signed) is stripped and not indexed.
+
 test_done
diff --git a/test/corpora/protected-headers/protected-with-legacy-display.eml b/test/corpora/protected-headers/protected-with-legacy-display.eml
new file mode 100644
index 00000000..8c5dd251
--- /dev/null
+++ b/test/corpora/protected-headers/protected-with-legacy-display.eml
@@ -0,0 +1,40 @@
+From: test_suite@notmuchmail.org
+To: test_suite@notmuchmail.org
+Subject: Subject Unavailable
+Date: Sat, 01 Jan 2000 12:00:00 +0000
+Message-Id: <protected-with-legacy-display@crypto.notmuchmail.org>
+MIME-Version: 1.0
+Content-Type: multipart/encrypted; boundary="=-=-=";
+	protocol="application/pgp-encrypted"
+
+--=-=-=
+Content-Type: application/pgp-encrypted
+
+Version: 1
+
+--=-=-=
+Content-Type: application/octet-stream
+
+-----BEGIN PGP MESSAGE-----
+
+hIwDxE023q1UqxYBBACkgwtKOAP+UlKYDzYkZY+gDuMNKnHjWIvv2Cdnovy40QzL
+5sbuib40y7orO+MqYMCWpoFtgBVsGiOUE3bZAg8n3Ji38/zVGwQveu6sh7SAy0Q9
+zFEvLhtajw17nPe+QH2UmIyfVikA57Mot13THq4i6C4ozVCyhyIltx+sNJkmw9Lp
+AdQd+cgCMRSMbi++eRwIi4zgxKrfAoGOmdMiVzBrh3yZqnbI0rCxJIKu7gEWuQLT
+7BuvN2bJUkPGLAUhUanFararVoD7WWOl67IlWFkyncES0PRskUf9coV68WZnYjsR
+Y3LdLnha1sdMwUNeBKQ44XBd2e7mXbDSp1cSjTDf9euwB4m7uQFTLwoQ8Of+LmQD
+KMHzjmucbkNAIpfAjcDusTA/oaaqUiEgGIgYYMDqG1CaaxdT55S7tMjW5yJryQmo
+pg65jrUMgEn5XHZ+KI2OsCmwGdoBYNau8p1a2hsiKhHJmLUeEAu34gFI3hylIOC0
+0KC40d0zTSb0s7SZuTrD6vYgiXG9aFktHvAWFH0ATCts7qyiRN7k5jt7yWfRntE2
+UCexTGE3TH7aju+IqDPC1XsaKF4T3CVhdr8WmKCa+0VOaw7xHRGYnzq9y91GcaCx
+8AcoZ3kYs+f2LIn+T667A0KKP4Z6OmLjCx3b1RvRUQYR9taruEMAQbIuAajiyTe9
+KfUrsUULZfInE50x+OneYvDhzoSgSJoHIK+18X/wo6YcyleJ9fZxCQ/vaXTDkAeF
+ve7TFcbIqmJ4MHygXILHUuDwp7P4t/tIL7SZwja70P3digjsgoNZY29VTnU8uyIb
+d6eOjgpeNVhRjDWxbUvhFD7i4rHCi/bbXFlW0cCXoiaVQBtYmiNysRoRZOv0h3TW
+q/+/UmqkaQFnF3zp5sr87y+ValItgPWmb9Ds0lyAoSvQx35zVh8DFfH04m7hmsb7
+gcvemlPTAnQWkIMC3c/bZWgt8tNcG7tQeUMWd9n4281y/hApbm90x2NLzEqvVcRq
+K0iIgVxbCHSKqGh4TtbIwpNhzSP+KHYkZ8h6+QUDRwGEV9QqZKg=
+=2O0V
+-----END PGP MESSAGE-----
+
+--=-=-=--
-- 
2.20.1

[PATCH v2 2/5] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

[Daniel Kahn Gillmor]

Our _notmuch_message_crypto_potential_payload implementation could
only return a failure if bad arguments were passed to it.  It is an
internal function, so if that happens it's an entirely internal bug
for notmuch.

It will be more useful for this function to return whether or not the
part is in fact a cryptographic payload, so we dispense with the
status return.

If some future change suggests adding a status return back, there are
only a handful of call sites, and no pressure to retain a stable API,
so it could be changed easily. But for now, go with the simpler
function.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 lib/index.cc  |  9 ++-------
 mime-node.c   |  6 +-----
 util/crypto.c | 27 ++++++++++++---------------
 util/crypto.h |  7 +++++--
 4 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index 1fd9e67e..deb76f6f 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -405,7 +405,6 @@ _index_mime_part (notmuch_message_t *message,
 	  _notmuch_message_add_term (message, "tag", "encrypted");
 
 	for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
-	    notmuch_status_t status;
 	    GMimeObject *child;
 	    if (GMIME_IS_MULTIPART_SIGNED (multipart)) {
 		/* Don't index the signature, but index its content type. */
@@ -434,11 +433,7 @@ _index_mime_part (notmuch_message_t *message,
 		continue;
 	    }
 	    child = g_mime_multipart_get_part (multipart, i);
-	    status = _notmuch_message_crypto_potential_payload (msg_crypto, child, part, i);
-	    if (status)
-		_notmuch_database_log (notmuch_message_get_database (message),
-				       "Warning: failed to mark the potential cryptographic payload (%s).\n",
-				       notmuch_status_to_string (status));
+	    _notmuch_message_crypto_potential_payload (msg_crypto, child, part, i);
 	    _index_mime_part (message, indexopts, child, msg_crypto);
 	}
 	return;
@@ -577,7 +572,7 @@ _index_encrypted_mime_part (notmuch_message_t *message,
 	}
 	g_object_unref (decrypt_result);
     }
-    status = _notmuch_message_crypto_potential_payload (msg_crypto, clear, GMIME_OBJECT (encrypted_data), GMIME_MULTIPART_ENCRYPTED_CONTENT);
+    _notmuch_message_crypto_potential_payload (msg_crypto, clear, GMIME_OBJECT (encrypted_data), GMIME_MULTIPART_ENCRYPTED_CONTENT);
     _index_mime_part (message, indexopts, clear, msg_crypto);
     g_object_unref (clear);
 
diff --git a/mime-node.c b/mime-node.c
index 0be03de7..33c51156 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -292,8 +292,6 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
 /* associate a MIME part with a node. */
 static bool
 _mime_node_set_up_part (mime_node_t *node, GMimeObject *part, int numchild) {
-    notmuch_status_t status;
-
     /* Deal with the different types of parts */
     if (GMIME_IS_PART (part)) {
 	node->part = part;
@@ -334,9 +332,7 @@ _mime_node_set_up_part (mime_node_t *node, GMimeObject *part, int numchild) {
 	    node_verify (node, part);
 	}
     } else {
-	status = _notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, node->parent ? node->parent->part : NULL, numchild);
-	if (status)
-	    fprintf (stderr, "Warning: failed to record potential crypto payload (%s).\n", notmuch_status_to_string (status));
+	_notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, node->parent ? node->parent->part : NULL, numchild);
     }
 
     return true;
diff --git a/util/crypto.c b/util/crypto.c
index 9e185e03..743c751a 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -132,19 +132,16 @@ _notmuch_message_crypto_potential_sig_list (_notmuch_message_crypto_t *msg_crypt
 }
 
 
-notmuch_status_t
-_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum)
+bool
+_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *part, GMimeObject *parent, int childnum)
 {
     const char *protected_headers = NULL;
     const char *forwarded = NULL;
     const char *subject = NULL;
 
-    if (!msg_crypto || !payload)
-	return NOTMUCH_STATUS_NULL_POINTER;
-
     /* only fire on the first payload part encountered */
     if (msg_crypto->payload_encountered)
-	return NOTMUCH_STATUS_SUCCESS;
+	return false;
 
     /* the first child of multipart/encrypted that matches the
      * encryption protocol should be "control information" metadata,
@@ -152,11 +149,11 @@ _notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto
      * https://tools.ietf.org/html/rfc1847#page-8) */
     if (parent && GMIME_IS_MULTIPART_ENCRYPTED (parent) && childnum == GMIME_MULTIPART_ENCRYPTED_VERSION) {
 	const char *enc_type = g_mime_object_get_content_type_parameter (parent, "protocol");
-	GMimeContentType *ct = g_mime_object_get_content_type (payload);
+	GMimeContentType *ct = g_mime_object_get_content_type (part);
 	if (ct && enc_type) {
 	    const char *part_type = g_mime_content_type_get_mime_type (ct);
 	    if (part_type && strcmp (part_type, enc_type) == 0)
-		return NOTMUCH_STATUS_SUCCESS;
+		return false;
 	}
     }
 
@@ -166,7 +163,7 @@ _notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto
      * envelope: */
     if ((msg_crypto->decryption_status != NOTMUCH_MESSAGE_DECRYPTED_FULL) &&
 	(msg_crypto->sig_list == NULL))
-	return NOTMUCH_STATUS_SUCCESS;
+	return false;
 
     /* Verify that this payload has headers that are intended to be
      * exported to the larger message: */
@@ -174,16 +171,16 @@ _notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto
     /* Consider a payload that uses Alexei Melinkov's forwarded="no" for
      * message/global or message/rfc822:
      * https://tools.ietf.org/html/draft-melnikov-smime-header-signing-05#section-4 */
-    forwarded = g_mime_object_get_content_type_parameter (payload, "forwarded");
-    if (GMIME_IS_MESSAGE_PART (payload) && forwarded && strcmp (forwarded, "no") == 0) {
-	GMimeMessage *message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (payload));
+    forwarded = g_mime_object_get_content_type_parameter (part, "forwarded");
+    if (GMIME_IS_MESSAGE_PART (part) && forwarded && strcmp (forwarded, "no") == 0) {
+	GMimeMessage *message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (part));
 	subject = g_mime_message_get_subject (message);
 	/* FIXME: handle more than just Subject: at some point */
     } else {
 	/* Consider "memoryhole"-style protected headers as practiced by Enigmail and K-9 */
-	protected_headers = g_mime_object_get_content_type_parameter (payload, "protected-headers");
+	protected_headers = g_mime_object_get_content_type_parameter (part, "protected-headers");
 	if (protected_headers && strcasecmp("v1", protected_headers) == 0)
-	    subject = g_mime_object_get_header (payload, "Subject");
+	    subject = g_mime_object_get_header (part, "Subject");
 	/* FIXME: handle more than just Subject: at some point */
     }
 
@@ -193,7 +190,7 @@ _notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto
 	msg_crypto->payload_subject = talloc_strdup (msg_crypto, subject);
     }
 
-    return NOTMUCH_STATUS_SUCCESS;
+    return true;
 }
 
 
diff --git a/util/crypto.h b/util/crypto.h
index fdbb5da5..ba1bf905 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -90,9 +90,12 @@ _notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t *msg_cr
 
 /* call potential_payload during a depth-first-search on a message
  * when encountering a message part that is not part of the envelope.
+ *
+ * Returns true if part is the root of the cryptographic payload of
+ * this message.
  */
-notmuch_status_t
-_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum);
+bool
+_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *part, GMimeObject *parent, int childnum);
 
 
 #ifdef __cplusplus
-- 
2.20.1

[PATCH v2 3/5] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display

[Daniel Kahn Gillmor]

This is a utility function designed to make it easier to
"fast-forward" past a legacy-display part associated with a
cryptographic envelope, and show the user the intended message body.

The bulk of the ugliness in here is in the test function
_notmuch_crypto_payload_has_legacy_display, which tests all of the
things we'd expect to be true in a a cryptographic payload that
contains a legacy display part.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 util/repair.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
 util/repair.h | 17 +++++++++
 2 files changed, 115 insertions(+)

diff --git a/util/repair.c b/util/repair.c
index f91c1244..040f2c02 100644
--- a/util/repair.c
+++ b/util/repair.c
@@ -18,4 +18,102 @@
  * Authors: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
  */
 
+#include <stdbool.h>
 #include "repair.h"
+
+
+static bool
+_notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
+{
+    GMimeMultipart *mpayload;
+    const char *protected_header_parameter;
+    GMimeTextPart *legacy_display;
+    char *legacy_display_header_text = NULL;
+    GMimeStream *stream = NULL;
+    GMimeParser *parser = NULL;
+    GMimeObject *legacy_header_object = NULL, *first;
+    GMimeHeaderList *legacy_display_headers = NULL, *protected_headers = NULL;
+    bool ret = false;
+
+    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (payload),
+				       "multipart", "mixed"))
+	return false;
+    protected_header_parameter = g_mime_object_get_content_type_parameter (payload, "protected-headers");
+    if ((! protected_header_parameter) || strcmp (protected_header_parameter, "v1"))
+	return false;
+    if (! GMIME_IS_MULTIPART (payload))
+	return false;
+    mpayload = GMIME_MULTIPART (payload);
+    if (mpayload == NULL)
+	return false;
+    if (g_mime_multipart_get_count (mpayload) != 2)
+	return false;
+    first = g_mime_multipart_get_part (mpayload, 0);
+    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (first),
+				       "text", "rfc822-headers"))
+	return false;
+    protected_header_parameter = g_mime_object_get_content_type_parameter (first, "protected-headers");
+    if ((! protected_header_parameter) || strcmp (protected_header_parameter, "v1"))
+	return false;
+    if (! GMIME_IS_TEXT_PART (first))
+	return false;
+
+    /* ensure that the headers in the first part all match the values
+     * found in the payload's own protected headers!  if they don't,
+     * we should not treat this as a valid "legacy-display" part.
+     *
+     * Crafting a GMimeHeaderList object from the content of the
+     * text/rfc822-headers part is pretty clumsy; we should probably
+     * push something into GMime that makes this a one-shot
+     * operation. */
+    if ((protected_headers = g_mime_object_get_header_list (payload), protected_headers) &&
+	(legacy_display = GMIME_TEXT_PART (first), legacy_display) &&
+	(legacy_display_header_text = g_mime_text_part_get_text (legacy_display), legacy_display_header_text) &&
+	(stream = g_mime_stream_mem_new_with_buffer (legacy_display_header_text, strlen (legacy_display_header_text)), stream) &&
+	(g_mime_stream_write (stream, "\r\n\r\n", 4) == 4) &&
+	(g_mime_stream_seek (stream, 0, GMIME_STREAM_SEEK_SET) == 0) &&
+	(parser = g_mime_parser_new_with_stream (stream), parser) &&
+	(legacy_header_object = g_mime_parser_construct_part (parser, NULL), legacy_header_object) &&
+	(legacy_display_headers = g_mime_object_get_header_list (legacy_header_object), legacy_display_headers)) {
+	/* walk through legacy_display_headers, comparing them against
+	 * their values in the protected_headers: */
+	ret = true;
+	for (int i = 0; i < g_mime_header_list_get_count (legacy_display_headers); i++) {
+	    GMimeHeader *dh = g_mime_header_list_get_header_at (legacy_display_headers, i);
+	    if (dh == NULL) {
+		ret = false;
+		break;
+	    }
+	    GMimeHeader *ph = g_mime_header_list_get_header (protected_headers, g_mime_header_get_name (dh));
+	    if (ph == NULL) {
+		ret = false;
+		break;
+	    }
+	    if (strcmp (g_mime_header_get_value (dh), g_mime_header_get_value (ph))) {
+		ret = false;
+		break;
+	    }
+	}
+    }
+
+    if (legacy_display_header_text)
+	g_free (legacy_display_header_text);
+    if (stream)
+	g_object_unref (stream);
+    if (parser)
+	g_object_unref (parser);
+    if (legacy_header_object)
+	g_object_unref (legacy_header_object);
+
+   return ret;
+}
+
+GMimeObject *
+_notmuch_repair_crypto_payload_skip_legacy_display (GMimeObject *payload)
+{
+    if (_notmuch_crypto_payload_has_legacy_display (payload)) {
+	return g_mime_multipart_get_part (GMIME_MULTIPART (payload), 1);
+    } else {
+	return payload;
+    }
+}
diff --git a/util/repair.h b/util/repair.h
index 70e2b7bc..9974d693 100644
--- a/util/repair.h
+++ b/util/repair.h
@@ -11,6 +11,23 @@ extern "C" {
  * techniques that are designed to improve the user experience of
  * notmuch */
 
+/* If payload is a cryptographic payload within an encrypted message, and
+ * it has a "legacy display" part, then we can skip over it and jump
+ * to the actual content, because notmuch already handles protected
+ * headers appropriately.
+ *
+ * This function either returns payload directly (if it does not have
+ * a "legacy display" part), or it returns a pointer to its
+ * content-bearing subpart, with the "legacy display" part and the
+ * surrounding multipart/mixed object bypassed.
+ *
+ * No new objects are created by calling this function, and the
+ * returned object will only be released when the original part is
+ * disposed of.
+ */
+GMimeObject *
+_notmuch_repair_crypto_payload_skip_legacy_display (GMimeObject *payload);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.20.1

[PATCH v2 4/5] cli/{show,reply}: skip over legacy-display parts

[Daniel Kahn Gillmor]

Make use of the previous changes to fast-forward past any
legacy-display parts during "notmuch show" and "notmuch reply".

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 mime-node.c                    | 11 ++++++++++-
 test/T356-protected-headers.sh |  2 --
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 33c51156..41cc7581 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -332,7 +332,16 @@ _mime_node_set_up_part (mime_node_t *node, GMimeObject *part, int numchild) {
 	    node_verify (node, part);
 	}
     } else {
-	_notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, node->parent ? node->parent->part : NULL, numchild);
+	if (_notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, node->parent ? node->parent->part : NULL, numchild) &&
+	    node->ctx->msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL) {
+	    GMimeObject *clean_payload = _notmuch_repair_crypto_payload_skip_legacy_display (part);
+	    if (clean_payload != part) {
+		/* only one layer of recursion is possible here
+		 * because there can be only a single cryptographic
+		 * payload: */
+		return _mime_node_set_up_part (node, clean_payload, numchild);
+	    }
+	}
     }
 
     return true;
diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index 43dfffe6..867b8722 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -137,14 +137,12 @@ id:protected-header@crypto.notmuchmail.org
 id:subjectless-protected-header@crypto.notmuchmail.org'
 
 test_begin_subtest "when rendering protected headers, avoid rendering legacy-display part"
-test_subtest_known_broken
 output=$(notmuch show --format=json id:protected-with-legacy-display@crypto.notmuchmail.org)
 test_json_nodes <<<"$output" \
                 'subject:[0][0][0]["headers"]["Subject"]="Interrupting Cow"' \
                 'no_legacy_display:[0][0][0]["body"][0]["content"][1]["content-type"]="text/plain"'
 
 test_begin_subtest "when replying, avoid rendering legacy-display part"
-test_subtest_known_broken
 output=$(notmuch reply --format=json id:protected-with-legacy-display@crypto.notmuchmail.org)
 test_json_nodes <<<"$output" \
                 'no_legacy_display:["original"]["body"][0]["content"][1]["content-type"]="text/plain"'
-- 
2.20.1

[PATCH v2 5/5] index: avoid indexing legacy-display parts

[Daniel Kahn Gillmor]

When we notice a legacy-display part during indexing, it makes more
sense to avoid indexing it as part of the message body.

Given that the protected subject will already be indexed, there is no
need to index this part at all, so we skip over it.

If this happens during indexing, we set a property on the message:
index.repaired=skip-protected-headers-legacy-display

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 doc/man7/notmuch-properties.rst |  6 ++++++
 lib/index.cc                    | 20 ++++++++++++++++----
 test/T356-protected-headers.sh  |  2 --
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/doc/man7/notmuch-properties.rst b/doc/man7/notmuch-properties.rst
index 2e610683..e2db2ef5 100644
--- a/doc/man7/notmuch-properties.rst
+++ b/doc/man7/notmuch-properties.rst
@@ -121,6 +121,12 @@ of its normal activity.
     ``index.repaired`` property to note the type of repair(s) it
     performed.
 
+    ``index.repaired=skip-protected-headers-legacy-display`` indicates
+    that when indexing the cleartext of an encrypted message, notmuch
+    skipped over a "legacy-display" text/rfc822-headers part that it
+    found in that message, since it was able to index the built-in
+    protected headers directly.
+
 SEE ALSO
 ========
 
diff --git a/lib/index.cc b/lib/index.cc
index deb76f6f..769e29a9 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -433,8 +433,14 @@ _index_mime_part (notmuch_message_t *message,
 		continue;
 	    }
 	    child = g_mime_multipart_get_part (multipart, i);
-	    _notmuch_message_crypto_potential_payload (msg_crypto, child, part, i);
-	    _index_mime_part (message, indexopts, child, msg_crypto);
+	    GMimeObject *toindex = child;
+	    if (_notmuch_message_crypto_potential_payload (msg_crypto, child, part, i) &&
+		msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL) {
+		toindex = _notmuch_repair_crypto_payload_skip_legacy_display (child);
+		if (toindex != child)
+		    notmuch_message_add_property (message, "index.repaired", "skip-protected-headers-legacy-display");
+	    }
+	    _index_mime_part (message, indexopts, toindex, msg_crypto);
 	}
 	return;
     }
@@ -572,8 +578,14 @@ _index_encrypted_mime_part (notmuch_message_t *message,
 	}
 	g_object_unref (decrypt_result);
     }
-    _notmuch_message_crypto_potential_payload (msg_crypto, clear, GMIME_OBJECT (encrypted_data), GMIME_MULTIPART_ENCRYPTED_CONTENT);
-    _index_mime_part (message, indexopts, clear, msg_crypto);
+    GMimeObject *toindex = clear;
+    if (_notmuch_message_crypto_potential_payload (msg_crypto, clear, GMIME_OBJECT (encrypted_data), GMIME_MULTIPART_ENCRYPTED_CONTENT) &&
+	msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL) {
+	toindex = _notmuch_repair_crypto_payload_skip_legacy_display (clear);
+	if (toindex != clear)
+	    notmuch_message_add_property (message, "index.repaired", "skip-protected-headers-legacy-display");
+    }
+    _index_mime_part (message, indexopts, toindex, msg_crypto);
     g_object_unref (clear);
 
     status = notmuch_message_add_property (message, "index.decryption", "success");
diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index 867b8722..925805df 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -148,12 +148,10 @@ test_json_nodes <<<"$output" \
                 'no_legacy_display:["original"]["body"][0]["content"][1]["content-type"]="text/plain"'
 
 test_begin_subtest "do not treat legacy-display part as body when indexing"
-test_subtest_known_broken
 output=$(notmuch search --output=messages body:interrupting)
 test_expect_equal "$output" ''
 
 test_begin_subtest "identify message that had a legacy display part skipped during indexing"
-test_subtest_known_broken
 output=$(notmuch search --output=messages property:index.repaired=skip-protected-headers-legacy-display)
 test_expect_equal "$output" id:protected-with-legacy-display@crypto.notmuchmail.org
 
-- 
2.20.1