Hiding legacy-display parts

Hiding legacy-display parts

[Daniel Kahn Gillmor]

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.

This series starts off with known-broken tests and ends by fixing the
problem by "fast-forwarding" through those MIME parts that are
reliably detectable as legacy-display parts.

Comments and feedback welcome!

         --dkg

[PATCH 1/6] 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                | 28 +++++++++++++
 .../protected-with-legacy-display.eml         | 40 +++++++++++++++++++
 2 files changed, 68 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..aeee3493 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -136,4 +136,32 @@ 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" ''
+
+# 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 2/6] util/crypto: make _n_m_crypto_potential_payload whether part is a payload

[Daniel Kahn Gillmor]

_notmuch_message_crypto_potential_payload 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 future changes to that function suggest 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 in that case.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 lib/index.cc  |  9 ++-------
 mime-node.c   |  5 +----
 util/crypto.c | 13 +++++--------
 util/crypto.h |  5 ++++-
 4 files changed, 12 insertions(+), 20 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 3492bcd0..a48a2119 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -268,7 +268,6 @@ static mime_node_t *
 _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
-    notmuch_status_t status;
 
     /* Set basic node properties */
     node->part = part;
@@ -321,9 +320,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
 	    node_verify (node, part);
 	}
     } else {
-	status = _notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, parent ? 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, parent ? parent->part : NULL, numchild);
     }
 
     return node;
diff --git a/util/crypto.c b/util/crypto.c
index 9e185e03..04dd91d4 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
+bool
 _notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, 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,
@@ -156,7 +153,7 @@ _notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto
 	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: */
@@ -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..8e1e6bbd 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -90,8 +90,11 @@ _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
+bool
 _notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum);
 
 
-- 
2.20.1

[PATCH 3/6] util/crypto: add _notmuch_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/crypto.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
 util/crypto.h | 17 +++++++++
 2 files changed, 113 insertions(+)

diff --git a/util/crypto.c b/util/crypto.c
index 04dd91d4..6c533f8d 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -209,3 +209,99 @@ _notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t *msg_cr
 
     return NOTMUCH_STATUS_SUCCESS;
 }
+
+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_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/crypto.h b/util/crypto.h
index 8e1e6bbd..5ea4fed2 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -97,6 +97,23 @@ _notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t *msg_cr
 bool
 _notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum);
 
+/* If part 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 takes a MIME object that is a cryptographic payload
+ * and either returns it 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 removed.
+ *
+ * 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_crypto_payload_skip_legacy_display (GMimeObject *part);
 
 #ifdef __cplusplus
 }
-- 
2.20.1

[PATCH 4/6] mime-node: split out _mime_node_set_up_part

[Daniel Kahn Gillmor]

This is a code reorganization that should have no functional effect,
but will make future changes simpler, because a future commit will
reuse the _mime_node_set_up_part functionality.

In the course of splitting out this function, I noticed a comment in
the codebase that referred to the original name of _mime_node_create,
where this functionality originally resided.  I've fixed that comment
to refer to the new function instead.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 mime-node.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index a48a2119..b8143b27 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -264,13 +264,36 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
 	g_error_free (err);
 }
 
+static bool
+_mime_node_set_up_part (mime_node_t *node, GMimeObject *part) {
+    /* Deal with the different types of parts */
+    if (GMIME_IS_PART (part)) {
+	node->part = part;
+	node->nchildren = 0;
+    } else if (GMIME_IS_MULTIPART (part)) {
+	node->part = part;
+	node->nchildren = g_mime_multipart_get_count (GMIME_MULTIPART (part));
+    } else if (GMIME_IS_MESSAGE_PART (part)) {
+	/* Promote part to an envelope and open it */
+	GMimeMessagePart *message_part = GMIME_MESSAGE_PART (part);
+	GMimeMessage *message = g_mime_message_part_get_message (message_part);
+	node->envelope_part = message_part;
+	node->part = GMIME_OBJECT (message);
+	node->nchildren = 1;
+    } else {
+	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
+		 g_type_name (G_OBJECT_TYPE (part)));
+	return false;
+    }
+    return true;
+}
+
 static mime_node_t *
 _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
 
     /* Set basic node properties */
-    node->part = part;
     node->ctx = parent->ctx;
     if (!talloc_reference (node, node->ctx)) {
 	fprintf (stderr, "Out of memory.\n");
@@ -281,21 +304,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
     node->part_num = node->next_part_num = -1;
     node->next_child = 0;
 
-    /* Deal with the different types of parts */
-    if (GMIME_IS_PART (part)) {
-	node->nchildren = 0;
-    } else if (GMIME_IS_MULTIPART (part)) {
-	node->nchildren = g_mime_multipart_get_count (GMIME_MULTIPART (part));
-    } else if (GMIME_IS_MESSAGE_PART (part)) {
-	/* Promote part to an envelope and open it */
-	GMimeMessagePart *message_part = GMIME_MESSAGE_PART (part);
-	GMimeMessage *message = g_mime_message_part_get_message (message_part);
-	node->envelope_part = message_part;
-	node->part = GMIME_OBJECT (message);
-	node->nchildren = 1;
-    } else {
-	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
-		 g_type_name (G_OBJECT_TYPE (part)));
+    if (! _mime_node_set_up_part (node, part)) {
 	talloc_free (node);
 	return NULL;
     }
@@ -344,7 +353,7 @@ mime_node_child (mime_node_t *parent, int child)
     } else if (GMIME_IS_MESSAGE (parent->part)) {
 	sub = g_mime_message_get_mime_part (GMIME_MESSAGE (parent->part));
     } else {
-	/* This should have been caught by message_part_create */
+	/* This should have been caught by _mime_node_set_up_part */
 	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
-- 
2.20.1

Re: [PATCH 4/6] mime-node: split out _mime_node_set_up_part

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]

On Fri 2019-05-31 00:28:23 -0400, Daniel Kahn Gillmor wrote:
> This is a code reorganization that should have no functional effect,
> but will make future changes simpler, because a future commit will
> reuse the _mime_node_set_up_part functionality.

This particular re-organization has a slight conflict with the patch
proposed in id:20190530172707.10378-5-dkg@fifthhorseman.net (patch 4/4
from the "mixed-up mangling repair" series).

in particular that, patch inserts some code when handling
GMIME_IS_MULTIPART in _mime_node_create, but if this patch is applied
first, that same hunk should amend _mime_node_set_up_part instead.

Whichever series you decide to apply series first, it should be
straightforward for me to re-base the other one to avoid merge
conflicts.  Let me know!

            --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 4/6] mime-node: split out _mime_node_set_up_part

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

On Fri 2019-05-31 00:41:54 -0400, Daniel Kahn Gillmor wrote:
> This particular re-organization has a slight conflict with the patch
> proposed in id:20190530172707.10378-5-dkg@fifthhorseman.net (patch 4/4
> from the "mixed-up mangling repair" series).

looking in more detail, i found some subtle differences that made
merging them trickier than expected.

please instead consider v3 of "mixed-up messages" and v2 of this "skip
protected header legacy display parts" series, both of which depend on
the "setup for message repair" series (found at
id:20190531074027.16337-1-dkg@fifthhorseman.net).

          --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[PATCH 5/6] 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                    | 9 ++++++++-
 test/T356-protected-headers.sh | 2 --
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index b8143b27..a1a9db29 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -329,7 +329,14 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
 	    node_verify (node, part);
 	}
     } else {
-	_notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, parent ? parent->part : NULL, numchild);
+	if (_notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, parent ? parent->part : NULL, numchild) &&
+	    node->ctx->msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL) {
+	    GMimeObject *clean_payload = _notmuch_crypto_payload_skip_legacy_display (part);
+	    if (clean_payload != part) {
+		if (! _mime_node_set_up_part (node, clean_payload))
+		    fprintf (stderr, "Error: failed to skip legacy display part\n");
+	    }
+	}
     }
 
     return node;
diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index aeee3493..af0b686b 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 6/6] 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.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 lib/index.cc                   | 14 ++++++++++----
 test/T356-protected-headers.sh |  1 -
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index deb76f6f..d3f21f91 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -433,8 +433,11 @@ _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_crypto_payload_skip_legacy_display (child);
+	    _index_mime_part (message, indexopts, toindex, msg_crypto);
 	}
 	return;
     }
@@ -572,8 +575,11 @@ _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_crypto_payload_skip_legacy_display (clear);
+    _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 af0b686b..295e3750 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -148,7 +148,6 @@ 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" ''
 
-- 
2.20.1