[BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages

[BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages

[Örjan Ekeberg]

Hi all,

I have found what seems to be a bug, or at least a misbehaviour of the
"missing attachment warning" implemented by the otherwise so nice
notmuch-mua-attachment-check.

It works fine to detect the regexp for attachments in simple messages.
The problem is that it also triggers the warning if a matching string is
present inside a forwarded message.  This is particularly annoying when
forwarding messages originating from MS-Exchange since those seem to
always include a hidden header "X-MS-Has-Attach" where the word "Attach"
constantly leads to false missing-attachment warnings.

Would it be possible to somehow restrict the regexp search to the part
of the message actually being authored?  Would it be too simplistic to
end the search at the first occurrence of "\n\n<#" ?

/Örjan

Re: [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages

[Daniel Kahn Gillmor]

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

On Wed 2019-05-08 19:19:18 +0200, Örjan Ekeberg wrote:

> I have found what seems to be a bug, or at least a misbehaviour of the
> "missing attachment warning" implemented by the otherwise so nice
> notmuch-mua-attachment-check.
>
> It works fine to detect the regexp for attachments in simple messages.
> The problem is that it also triggers the warning if a matching string is
> present inside a forwarded message.  This is particularly annoying when
> forwarding messages originating from MS-Exchange since those seem to
> always include a hidden header "X-MS-Has-Attach" where the word "Attach"
> constantly leads to false missing-attachment warnings.

I've tagged this with notmuch::bug so that it shows up at
https://nmbug.notmuchmail.org/status/, and i'm directly cc'ing dme,
since i think he's the author of notmuch-mua-attachment-check.  

> Would it be possible to somehow restrict the regexp search to the part
> of the message actually being authored?  Would it be too simplistic to
> end the search at the first occurrence of "\n\n<#" ?

that's pretty simplistic, but does seem better than the status quo.  do
you have a proposed patch?

    --dkg

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

[PATCH 0/2] Limit attachment check to stop before forwarded message

[ekeberg]

From: Örjan Ekeberg <ekeberg@kth.se>

This is a simple patch to notmuch-mua-attachment-check to make it stop
searching for mathing "attachment"-words when reaching a forwarded
message (or anything starting with "\n<#").  This avoids the false
warnings that always occur when forwarding messages originating from
MS-Exchange, since they contain a header "MS-Has-Attach:".

Örjan Ekeberg (2):
  emacs: limit search for attachment to stop at first mime-part
  test: extend test of attachment warnings

 emacs/notmuch-mua.el              | 25 ++++++++++++++-----------
 test/emacs-attachment-warnings.el | 12 ++++++++++++
 2 files changed, 26 insertions(+), 11 deletions(-)

-- 
2.24.0

[PATCH 1/2] emacs: limit search for attachment to stop at first mime-part

[ekeberg]

From: Örjan Ekeberg <ekeberg@kth.se>

This commit changes the behaviour of notmuch-mua-attachment-check
so that it stops searching for notmuch-mua-attachment-regexp when a
new mime-part is reached.  This avoids false warnings when matching
words occur inside forwarded messages.
---
 emacs/notmuch-mua.el | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 7fdd76bc..76572b87 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -137,17 +137,20 @@ Typically this is added to `notmuch-mua-send-hook'."
 	 ;; When the message mentions attachment...
 	 (save-excursion
 	   (message-goto-body)
-	   (loop while (re-search-forward notmuch-mua-attachment-regexp (point-max) t)
-		 ;; For every instance of the "attachment" string
-		 ;; found, examine the text properties. If the text
-		 ;; has either a `face' or `syntax-table' property
-		 ;; then it is quoted text and should *not* cause the
-		 ;; user to be asked about a missing attachment.
-		 if (let ((props (text-properties-at (match-beginning 0))))
-		      (not (or (memq 'syntax-table props)
-			       (memq 'face props))))
-		 return t
-		 finally return nil))
+	   ;; Limit search from reaching other possible parts of the message
+	   (let ((search-limit (search-forward "\n<#" nil t)))
+	     (message-goto-body)
+	     (loop while (re-search-forward notmuch-mua-attachment-regexp search-limit t)
+		   ;; For every instance of the "attachment" string
+		   ;; found, examine the text properties. If the text
+		   ;; has either a `face' or `syntax-table' property
+		   ;; then it is quoted text and should *not* cause the
+		   ;; user to be asked about a missing attachment.
+		   if (let ((props (text-properties-at (match-beginning 0))))
+			(not (or (memq 'syntax-table props)
+				 (memq 'face props))))
+		   return t
+		   finally return nil)))
 	 ;; ...but doesn't have a part with a filename...
 	 (save-excursion
 	   (message-goto-body)
-- 
2.24.0

[PATCH 2/2] test: extend test of attachment warnings

[ekeberg]

From: Örjan Ekeberg <ekeberg@kth.se>

Check that attachment warnings are not raised when the word
"attach" only occurs in a forwarded message.
---
 test/emacs-attachment-warnings.el | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/test/emacs-attachment-warnings.el b/test/emacs-attachment-warnings.el
index 200ca7ba..a3067b14 100644
--- a/test/emacs-attachment-warnings.el
+++ b/test/emacs-attachment-warnings.el
@@ -36,6 +36,12 @@ Return `t' if the message would be sent, otherwise `nil'"
 	   ;; fontification properties. For fontification to happen we need to
 	   ;; allow some time for redisplay.
 	   (sit-for 0.01)))
+    (t . (lambda ()
+	   ;; "attach" is only mentioned in a forwarded message.
+	   (insert "Hello\n")
+	   (insert "<#mml type=message/rfc822 disposition=inline>\n")
+	   (insert "X-Has-Attach:\n")
+	   (insert "<#/mml>\n")))
 
     ;; These should not be okay:
     (nil . (lambda () (insert "Here is an attachment:\n")))
@@ -49,6 +55,12 @@ Return `t' if the message would be sent, otherwise `nil'"
 	     ;; looking at fontification properties. For fontification
 	     ;; to happen we need to allow some time for redisplay.
 	     (sit-for 0.01)))
+    (nil . (lambda ()
+	   ;; "attachment" is mentioned before a forwarded message.
+	   (insert "I also attach something.\n")
+	   (insert "<#mml type=message/rfc822 disposition=inline>\n")
+	   (insert "X-Has-Attach:\n")
+	   (insert "<#/mml>\n")))
     ))
 
 (defun notmuch-test-attachment-warning-1 ()
-- 
2.24.0

Re: [PATCH 0/2] Limit attachment check to stop before forwarded message

[David Edmondson]

On Friday, 2019-12-13 at 00:35:34 +01, ekeberg@kth.se wrote:

> From: Örjan Ekeberg <ekeberg@kth.se>
>
> This is a simple patch to notmuch-mua-attachment-check to make it stop
> searching for mathing "attachment"-words when reaching a forwarded
> message (or anything starting with "\n<#").  This avoids the false
> warnings that always occur when forwarding messages originating from
> MS-Exchange, since they contain a header "MS-Has-Attach:".
>
> Örjan Ekeberg (2):
>   emacs: limit search for attachment to stop at first mime-part
>   test: extend test of attachment warnings
>
>  emacs/notmuch-mua.el              | 25 ++++++++++++++-----------
>  test/emacs-attachment-warnings.el | 12 ++++++++++++
>  2 files changed, 26 insertions(+), 11 deletions(-)

Looks good - thanks for doing this.

(Apologies for the resend - hit 'r' not 'R' the first time.)

dme.
-- 
I'd come on over but I haven't got a raincoat.

Re: [PATCH 0/2] Limit attachment check to stop before forwarded message

[David Bremner]

ekeberg@kth.se writes:

> From: Örjan Ekeberg <ekeberg@kth.se>
>
> This is a simple patch to notmuch-mua-attachment-check to make it stop
> searching for mathing "attachment"-words when reaching a forwarded
> message (or anything starting with "\n<#").  This avoids the false
> warnings that always occur when forwarding messages originating from
> MS-Exchange, since they contain a header "MS-Has-Attach:".
>
> Örjan Ekeberg (2):
>   emacs: limit search for attachment to stop at first mime-part
>   test: extend test of attachment warnings
>
>  emacs/notmuch-mua.el              | 25 ++++++++++++++-----------
>  test/emacs-attachment-warnings.el | 12 ++++++++++++
>  2 files changed, 26 insertions(+), 11 deletions(-)
>

Merged to master,

d

Re: [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages

[David Bremner]

Örjan Ekeberg <ekeberg@kth.se> writes:

> Hi all,
>
> I have found what seems to be a bug, or at least a misbehaviour of the
> "missing attachment warning" implemented by the otherwise so nice
> notmuch-mua-attachment-check.
>
> It works fine to detect the regexp for attachments in simple messages.
> The problem is that it also triggers the warning if a matching string is
> present inside a forwarded message.  This is particularly annoying when
> forwarding messages originating from MS-Exchange since those seem to
> always include a hidden header "X-MS-Has-Attach" where the word "Attach"
> constantly leads to false missing-attachment warnings.
>
> Would it be possible to somehow restrict the regexp search to the part
> of the message actually being authored?  Would it be too simplistic to
> end the search at the first occurrence of "\n\n<#" ?

marking this as fixed as the proposed patches were applied several years
ago.

d\r