v4 of legacy-display cleanup

v4 of legacy-display cleanup

[Daniel Kahn Gillmor]

This is the fourth revision of the series that cleans up legacy-display
protected headers parts so that notmuch users only have to look at one
subject line.

version 3 can be found at id:20190625014107.12452-1-dkg@fifthhorseman.net
version 2 can be found at id:20190531075907.17035-1-dkg@fifthhorseman.net
version 1 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.
----------

This series addresses the concerns raised by David Bremner on the
mailing list.

The differences from version 3 are:

 * clearer semantics within the patch series, both code and commit
   messages (e.g. indicating that a new return value is temporarily
   unused in patch 5, before using it in subsequent patches)

 * separating out the non-functional change in argument name
   ("payload" to "part") into its own patch (4/8) for clarity

 * using "goto DONE" instead of "break" in patch 6.

 * using INTERNAL_ERROR in patch 5 to catch potential future internal
   misuse of _notmuch_message_crypto_potential_payload.

If we can get this merged, i'll send a subsequent revision of the
series that repairs "mixed-up MIME" mangled messages.

I would appreciate any feedback!

  --dkg

[PATCH v4 1/8] 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 without touching
_mime_node_create.

In the course of splitting out this function, I noticed a comment in
the codebase that referred to an older name of _mime_node_create
(message_part_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 | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 3133ca44..d2125f90 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -264,14 +264,15 @@ 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, int numchild);
+
 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;
     node->ctx = parent->ctx;
     if (! talloc_reference (node, node->ctx)) {
 	fprintf (stderr, "Out of memory.\n");
@@ -282,10 +283,24 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
     node->part_num = node->next_part_num = -1;
     node->next_child = 0;
 
+    if (_mime_node_set_up_part (node, part, numchild))
+	return node;
+    talloc_free (node);
+    return NULL;
+}
+
+/* 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;
 	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 */
@@ -297,11 +312,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
     } else {
 	fprintf (stderr, "Warning: Unknown mime part type: %s.\n",
 		 g_type_name (G_OBJECT_TYPE (part)));
-	talloc_free (node);
-	return NULL;
+	return false;
     }
 
-    /* Handle PGP/MIME parts */
+    /* Handle PGP/MIME parts (by definition not cryptographic payload parts) */
     if (GMIME_IS_MULTIPART_ENCRYPTED (part) && (node->ctx->crypto->decrypt != NOTMUCH_DECRYPT_FALSE)) {
 	if (node->nchildren != 2) {
 	    /* this violates RFC 3156 section 4, so we won't bother with it. */
@@ -321,12 +335,12 @@ _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);
+	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));
     }
 
-    return node;
+    return true;
 }
 
 mime_node_t *
@@ -347,7 +361,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.23.0.rc1

[PATCH v4 2/8] repair: set up codebase for repair functionality

[Daniel Kahn Gillmor]

This adds no functionality directly, but is a useful starting point
for adding new repair functionality.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 doc/man7/notmuch-properties.rst | 12 ++++++++++++
 lib/notmuch-private.h           |  1 +
 notmuch-client.h                |  1 +
 util/Makefile.local             |  1 +
 util/repair.c                   | 21 +++++++++++++++++++++
 util/repair.h                   | 17 +++++++++++++++++
 6 files changed, 53 insertions(+)
 create mode 100644 util/repair.c
 create mode 100644 util/repair.h

diff --git a/doc/man7/notmuch-properties.rst b/doc/man7/notmuch-properties.rst
index 802e6763..2e610683 100644
--- a/doc/man7/notmuch-properties.rst
+++ b/doc/man7/notmuch-properties.rst
@@ -109,6 +109,18 @@ of its normal activity.
     example, an AES-128 key might be stashed in a notmuch property as:
     ``session-key=7:14B16AF65536C28AF209828DFE34C9E0``.
 
+**index.repaired**
+
+    Some messages arrive in forms that are confusing to view; they can
+    be mangled by mail transport agents, or the sending mail user
+    agent may structure them in a way that is confusing.  If notmuch
+    knows how to both detect and repair such a problematic message, it
+    will do so during indexing.
+
+    If it applies a message repair during indexing, it will use the
+    ``index.repaired`` property to note the type of repair(s) it
+    performed.
+
 SEE ALSO
 ========
 
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 9bdb68ab..28ced3a2 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -53,6 +53,7 @@ NOTMUCH_BEGIN_DECLS
 #include "error_util.h"
 #include "string-util.h"
 #include "crypto.h"
+#include "repair.h"
 
 #ifdef DEBUG
 # define DEBUG_DATABASE_SANITY 1
diff --git a/notmuch-client.h b/notmuch-client.h
index d1b78017..74690054 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -52,6 +52,7 @@
 
 #include "talloc-extra.h"
 #include "crypto.h"
+#include "repair.h"
 
 #define unused(x) x ## _unused __attribute__ ((unused))
 
diff --git a/util/Makefile.local b/util/Makefile.local
index 46f8af3a..f5d72f79 100644
--- a/util/Makefile.local
+++ b/util/Makefile.local
@@ -6,6 +6,7 @@ extra_cflags += -I$(srcdir)/$(dir)
 libnotmuch_util_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
 		  $(dir)/string-util.c $(dir)/talloc-extra.c $(dir)/zlib-extra.c \
 		$(dir)/util.c $(dir)/gmime-extra.c $(dir)/crypto.c \
+		$(dir)/repair.c \
 		$(dir)/unicode-util.c
 
 libnotmuch_util_modules := $(libnotmuch_util_c_srcs:.c=.o)
diff --git a/util/repair.c b/util/repair.c
new file mode 100644
index 00000000..f91c1244
--- /dev/null
+++ b/util/repair.c
@@ -0,0 +1,21 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2019 Daniel Kahn Gillmor
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see https://www.gnu.org/licenses/ .
+ *
+ * Authors: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
+ */
+
+#include "repair.h"
diff --git a/util/repair.h b/util/repair.h
new file mode 100644
index 00000000..70e2b7bc
--- /dev/null
+++ b/util/repair.h
@@ -0,0 +1,17 @@
+#ifndef _REPAIR_H
+#define _REPAIR_H
+
+#include "gmime-extra.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* This is a collection of message structure and message format repair
+ * techniques that are designed to improve the user experience of
+ * notmuch */
+
+#ifdef __cplusplus
+}
+#endif
+#endif
-- 
2.23.0.rc1

[PATCH v4 3/8] 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.23.0.rc1

[PATCH v4 4/8] util/crypto: _n_m_crypto_potential_payload: rename "payload" arg to "part"

[Daniel Kahn Gillmor]

_notmuch_message_crypto_potential_payload is called on a GMimeObject
while walking the MIME tree of a message to determine whether that
object is the payload.  It doesn't make sense to name the argument
"payload" if it might not be the payload, so we rename it to "part"
for clarity.

This is a non-functional change, just semantic cleanup.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 util/crypto.c | 16 ++++++++--------
 util/crypto.h |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/util/crypto.c b/util/crypto.c
index 225f537a..3845ade8 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -136,13 +136,13 @@ _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)
+_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)
+    if (! msg_crypto || ! part)
 	return NOTMUCH_STATUS_NULL_POINTER;
 
     /* only fire on the first payload part encountered */
@@ -155,7 +155,7 @@ _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)
@@ -177,16 +177,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 */
     }
 
diff --git a/util/crypto.h b/util/crypto.h
index 11e8060a..c4411246 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -92,7 +92,7 @@ _notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t *msg_cr
  * when encountering a message part that is not part of the envelope.
  */
 notmuch_status_t
-_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum);
+_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *part, GMimeObject *parent, int childnum);
 
 
 #ifdef __cplusplus
-- 
2.23.0.rc1

[PATCH v4 5/8] 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.

We will use this return value in future patches, to make different
decisions based on whether a part is the cryptographic payload or not.
But for now, we just leave the places where it gets invoked marked
with (void) to show that the result is ignored.

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

diff --git a/lib/index.cc b/lib/index.cc
index db3dd568..08cc84e2 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -407,7 +407,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. */
@@ -436,11 +435,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));
+	    (void) _notmuch_message_crypto_potential_payload (msg_crypto, child, part, i);
 	    _index_mime_part (message, indexopts, child, msg_crypto);
 	}
 	return;
@@ -578,7 +573,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 d2125f90..abb6dd84 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -293,8 +293,6 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
 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;
@@ -335,9 +333,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));
+	(void) _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 3845ade8..0bb6f526 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -20,6 +20,7 @@
 
 #include "crypto.h"
 #include <strings.h>
+#include "error_util.h"
 #define unused(x) x __attribute__ ((unused))
 
 #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
@@ -135,19 +136,20 @@ _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 *part, GMimeObject *parent, int childnum)
 {
     const char *protected_headers = NULL;
     const char *forwarded = NULL;
     const char *subject = NULL;
 
-    if (! msg_crypto || ! part)
-	return NOTMUCH_STATUS_NULL_POINTER;
+    if ((! msg_crypto) || (! part))
+	INTERNAL_ERROR ("_notmuch_message_crypto_potential_payload() got NULL for %s\n",
+			msg_crypto? "part" : "msg_crypto");
 
     /* 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,
@@ -159,7 +161,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;
 	}
     }
 
@@ -169,7 +171,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: */
@@ -196,7 +198,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 c4411246..f8bda0d1 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 *part, GMimeObject *parent, int childnum);
 
 
-- 
2.23.0.rc1

[PATCH v4 6/8] 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 | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++
 util/repair.h |  17 +++++++++
 2 files changed, 118 insertions(+)

diff --git a/util/repair.c b/util/repair.c
index f91c1244..629e6f23 100644
--- a/util/repair.c
+++ b/util/repair.c
@@ -18,4 +18,105 @@
  * 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;
+		goto DONE;
+	    }
+	    GMimeHeader *ph = g_mime_header_list_get_header (protected_headers, g_mime_header_get_name (dh));
+	    if (ph == NULL) {
+		ret = false;
+		goto DONE;
+	    }
+	    const char *dhv = g_mime_header_get_value (dh);
+	    const char *phv = g_mime_header_get_value (ph);
+	    if (dhv == NULL || phv == NULL || strcmp (dhv, phv)) {
+		ret = false;
+		goto DONE;
+	    }
+	}
+    }
+
+ DONE:
+    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.23.0.rc1

[PATCH v4 7/8] 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 abb6dd84..599d3b65 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -333,7 +333,16 @@ _mime_node_set_up_part (mime_node_t *node, GMimeObject *part, int numchild)
 	    node_verify (node, part);
 	}
     } else {
-	(void) _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.23.0.rc1

[PATCH v4 8/8] 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 08cc84e2..1301d78a 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -435,8 +435,14 @@ _index_mime_part (notmuch_message_t *message,
 		continue;
 	    }
 	    child = g_mime_multipart_get_part (multipart, i);
-	    (void) _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;
     }
@@ -573,8 +579,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.23.0.rc1

Re: v4 of legacy-display cleanup

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> This is the fourth revision of the series that cleans up legacy-display
> protected headers parts so that notmuch users only have to look at one
> subject line.
>
> version 3 can be found at id:20190625014107.12452-1-dkg@fifthhorseman.net
> version 2 can be found at id:20190531075907.17035-1-dkg@fifthhorseman.net
> version 1 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.
> ----------

series pushed to master

d