v3 of legacy-display cleanup

v3 of legacy-display cleanup

[Daniel Kahn Gillmor]

This is the third 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 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.
----------

The main differences from version 2 are:

 * i've directly included the two-patch "repair" series (originally
   found at id:20190531074027.16337-1-dkg@fifthhorseman.net), and

 * i've rebased against the uncrustified master, as well as applying
   the uncrustify rules to my changes here.

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 1/7] 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.20.1

Re: [PATCH 1/7] mime-node: split out _mime_node_set_up_part

[William Casarin]

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

> 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);
> +

nit: Instead of a forward declaration, could _mime_node_create be moved after
_mime_node_set_up_part instead?

>  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.20.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

-- 
https://jb55.com

Re: [PATCH 1/7] mime-node: split out _mime_node_set_up_part

[Daniel Kahn Gillmor]

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

On Mon 2019-06-24 19:43:58 -0700, William Casarin wrote:
>> +static bool
>> +_mime_node_set_up_part (mime_node_t *node, GMimeObject *part, int numchild);
>> +
>
> nit: Instead of a forward declaration, could _mime_node_create be moved after
> _mime_node_set_up_part instead?

yep, we could definitely do that.  I did it this way because the diff
feels cleaner to me -- rather than moving a big chunk of code, i'm just
breaking one function in half.

If anyone feels strongly about it, i wouldn't object to doing it the
other way around, but i'd be unlikely to want to rerun the patch series
for this change on its own.

       --dkg

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

[PATCH 2/7] 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.20.1

[PATCH 3/7] 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 4/7] 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 db3dd568..8a3e2e09 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));
+	    _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..f2bab64e 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));
+	_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 225f537a..31cf056b 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -135,19 +135,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,
@@ -155,11 +152,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;
 	}
     }
 
@@ -169,7 +166,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: */
@@ -177,16 +174,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 */
     }
 
@@ -196,7 +193,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 11e8060a..f8bda0d1 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

Re: [PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

[David Bremner]

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

> 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.
>

Is a bit confusing that this patch introduces a new return value, and
then ignores. Perhaps cast the calls to (void) to make it clear you are
ignoring it on purpose.
> -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;
> -

what about leaving an assert or call to INTERNAL_ERROR here? I'm a bit
concerned this change is making the code less robust. I guess we'll see
a segfault, if either is NULL, but that seems bit icky to rely on.

> -	GMimeContentType *ct = g_mime_object_get_content_type (payload);
> +	GMimeContentType *ct = g_mime_object_get_content_type (part);

The purpose of this change is unclear to me from the diff. Can you explain?
It seems like there is a related s/payload/part/ in several places
below. Maybe this deserves a note in the commit message?

Re: [PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

[Daniel Kahn Gillmor]

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

Hi David--

Thanks for the review!

On Wed 2019-07-31 08:57:56 -0300, David Bremner wrote:
> Is a bit confusing that this patch introduces a new return value, and
> then ignores. Perhaps cast the calls to (void) to make it clear you are
> ignoring it on purpose.

hm, we ignore it only for the moment -- in patches 6 and 7 we take
action on the basis of the return value.  I'll cast them to void here in
this patch and update the commit message to explain the situation.

>> -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;
>> -
>
> what about leaving an assert or call to INTERNAL_ERROR here? I'm a bit
> concerned this change is making the code less robust. I guess we'll see
> a segfault, if either is NULL, but that seems bit icky to rely on.

Sure, INTERNAL_ERROR makes sense, i think.

>> -	GMimeContentType *ct = g_mime_object_get_content_type (payload);
>> +	GMimeContentType *ct = g_mime_object_get_content_type (part);
>
> The purpose of this change is unclear to me from the diff. Can you explain?
> It seems like there is a related s/payload/part/ in several places
> below. Maybe this deserves a note in the commit message?

The function is _notmuch_message_crypto_potential_payload, and it is
testing whether a given mime part (a GMimeObject) *is* the payload or
not.  the GMimeObject argument used to be named "payload", which is a
bit weird -- if we know it's the payload already, why call the function?
rather, we should call it "part".  i'll split that out into a separate
patch, though, since it's clearly intended as a non-functional change.

Do you have more review pending on this series or should i send the
updated series to the list with these changes?

      --dkg

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

Re: [PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

[Daniel Kahn Gillmor]

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

On Wed 2019-07-31 23:15:55 -0400, Daniel Kahn Gillmor wrote:
> On Wed 2019-07-31 08:57:56 -0300, David Bremner wrote:
>> what about leaving an assert or call to INTERNAL_ERROR here? I'm a bit
>> concerned this change is making the code less robust. I guess we'll see
>> a segfault, if either is NULL, but that seems bit icky to rely on.
>
> Sure, INTERNAL_ERROR makes sense, i think.

hm, INTERNAL_ERROR is only a valid symbol within libnotmuch, and
util/crypto.o is *not* within libnotmuch.  So i think i'll use assert,
by analogy with hex_encode() in util/hex_encode.c.  let me know if you
think that's a bad idea for some reason.

   --dkg

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

Re: [PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

[David Bremner]

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

> On Wed 2019-07-31 23:15:55 -0400, Daniel Kahn Gillmor wrote:
>> On Wed 2019-07-31 08:57:56 -0300, David Bremner wrote:
>>> what about leaving an assert or call to INTERNAL_ERROR here? I'm a bit
>>> concerned this change is making the code less robust. I guess we'll see
>>> a segfault, if either is NULL, but that seems bit icky to rely on.
>>
>> Sure, INTERNAL_ERROR makes sense, i think.
>
> hm, INTERNAL_ERROR is only a valid symbol within libnotmuch, and
> util/crypto.o is *not* within libnotmuch.  So i think i'll use assert,
> by analogy with hex_encode() in util/hex_encode.c.  let me know if you
> think that's a bad idea for some reason.
>

assert is OK, but INTERNAL_ERROR is definded in util/error_util.c, so
including that header is another sensible option.

Re: [PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

[Daniel Kahn Gillmor]

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

On Thu 2019-08-01 08:32:56 -0300, David Bremner wrote:
> assert is OK, but INTERNAL_ERROR is definded in util/error_util.c, so
> including that header is another sensible option.

gotcha, i think i was doing these updates with not-enough-sleep. I've
gone back to INTERNAL_ERROR for my internal branch now. :)

      --dkg

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

Re: [PATCH 4/7] util/crypto: _n_m_crypto_potential_payload returns whether part is the payload

[David Bremner]

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

>
> Do you have more review pending on this series or should i send the
> updated series to the list with these changes?
>

Give me a day or two, I should finish the series.

d

[PATCH 5/7] 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..0809f7b4 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

Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display

[William Casarin]

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

> 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..0809f7b4 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

This may be a noob question, but why the comma operators after the
assignment expressions? Wouldn't they evaluate to the same thing?

> +	 * 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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

-- 
https://jb55.com

Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display

[Daniel Kahn Gillmor]

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

On Mon 2019-06-24 20:02:13 -0700, William Casarin wrote:
> dkg wrote:
>> +    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
>
> This may be a noob question, but why the comma operators after the
> assignment expressions? Wouldn't they evaluate to the same thing?

Yes, they do evaluate to the same thing.

But A sensible compiler will warn when it sees "if (foo = bar)" because
most people who write that actually mean "if (foo == bar)".

So using the comma operator signals to the compiler "yes, I meant to
test the result of this particular assignment, and it is not just a
buggy attempt at a comparison".

     --dkg

PS if you leave out the comma, gcc's actual warning with -Wall is
   disappointingly opaque:

    warning: suggest parentheses around assignment used as truth value [-Wparentheses]

   This suggests that you could also work around it by using a construct
   like"if ((foo = bar))".  But i think that is harder to read, and
   doesn't really describe what the intent is as well.

   As Hal Abelson said, "Programs must be written for people to read,
   and only incidentally for machines to execute."

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

Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display

[David Bremner]

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

> +	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;
> +	    }

I can live with the use of break if you think it's superior, but I think
the idiom of "goto DONE" is more common in the notmuch codebase. I
personally always have think about the semantics of "break" and
"continue" in C pretty carefully.

> +	    if (strcmp (g_mime_header_get_value (dh), g_mime_header_get_value (ph))) {
> +		ret = false;
> +		break;
> +	    }

It's not really clear to me what kind of "invalid" causes
g_mime_header_get_value to return NULL. Maybe this strcmp should be
guarded against that?

Re: [PATCH 5/7] util/repair: add _notmuch_repair_crypto_payload_skip_legacy_display

[Daniel Kahn Gillmor]

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

Hi Bremner--

thanks for the review!

On Sat 2019-08-03 12:15:30 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> +	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;
>> +	    }
>
> I can live with the use of break if you think it's superior, but I think
> the idiom of "goto DONE" is more common in the notmuch codebase. I
> personally always have think about the semantics of "break" and
> "continue" in C pretty carefully.

i thought i was the only one who got confused between "break" and
"continue"!  I will convert to goto DONE, i agree it's more readable.

>> +	    if (strcmp (g_mime_header_get_value (dh), g_mime_header_get_value (ph))) {
>> +		ret = false;
>> +		break;
>> +	    }
>
> It's not really clear to me what kind of "invalid" causes
> g_mime_header_get_value to return NULL. Maybe this strcmp should be
> guarded against that?

i think it's impossible in the current implementation for this to go
wrong, since we've already got a GMimeHeader object from an existing
block of headers, but i'll add some protection just in case GMime
changes its implementation or some fuzzer constructs a truly devious
not-quite-RFC-822 input.

         --dkg

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

[PATCH 6/7] 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 f2bab64e..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 {
-	_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 7/7] 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 8a3e2e09..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);
-	    _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.20.1

Re: v3 of legacy-display cleanup

[William Casarin]

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

> This is the third 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 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.
> ----------
>
> The main differences from version 2 are:
>
>  * i've directly included the two-patch "repair" series (originally
>    found at id:20190531074027.16337-1-dkg@fifthhorseman.net), and
>
>  * i've rebased against the uncrustified master, as well as applying
>    the uncrustify rules to my changes here.
>
> 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!

Looks good to me, didn't get a chance to test it out yet. Thanks for
working on this!

Cheers,
Will

-- 
https://jb55.com