unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages
@ 2019-05-08 17:19 Örjan Ekeberg
  2019-06-09 23:12 ` Daniel Kahn Gillmor
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Örjan Ekeberg @ 2019-05-08 17:19 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages
  2019-05-08 17:19 [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages Örjan Ekeberg
@ 2019-06-09 23:12 ` Daniel Kahn Gillmor
  2019-12-12 23:35 ` [PATCH 0/2] Limit attachment check to stop before forwarded message ekeberg
  2021-12-25 18:04 ` [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages David Bremner
  2 siblings, 0 replies; 8+ messages in thread
From: Daniel Kahn Gillmor @ 2019-06-09 23:12 UTC (permalink / raw)
  To: Örjan Ekeberg, notmuch, David Edmondson

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 0/2] Limit attachment check to stop before forwarded message
@ 2019-12-12 23:35 ` ekeberg
  2019-12-12 23:35   ` [PATCH 1/2] emacs: limit search for attachment to stop at first mime-part ekeberg
                     ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: ekeberg @ 2019-12-12 23:35 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] emacs: limit search for attachment to stop at first mime-part
  2019-12-12 23:35 ` [PATCH 0/2] Limit attachment check to stop before forwarded message ekeberg
@ 2019-12-12 23:35   ` ekeberg
  2019-12-12 23:35   ` [PATCH 2/2] test: extend test of attachment warnings ekeberg
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: ekeberg @ 2019-12-12 23:35 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] test: extend test of attachment warnings
  2019-12-12 23:35 ` [PATCH 0/2] Limit attachment check to stop before forwarded message ekeberg
  2019-12-12 23:35   ` [PATCH 1/2] emacs: limit search for attachment to stop at first mime-part ekeberg
@ 2019-12-12 23:35   ` ekeberg
  2019-12-13 10:26   ` [PATCH 0/2] Limit attachment check to stop before forwarded message David Edmondson
  2019-12-14 11:39   ` David Bremner
  3 siblings, 0 replies; 8+ messages in thread
From: ekeberg @ 2019-12-12 23:35 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] Limit attachment check to stop before forwarded message
  2019-12-12 23:35 ` [PATCH 0/2] Limit attachment check to stop before forwarded message ekeberg
  2019-12-12 23:35   ` [PATCH 1/2] emacs: limit search for attachment to stop at first mime-part ekeberg
  2019-12-12 23:35   ` [PATCH 2/2] test: extend test of attachment warnings ekeberg
@ 2019-12-13 10:26   ` David Edmondson
  2019-12-14 11:39   ` David Bremner
  3 siblings, 0 replies; 8+ messages in thread
From: David Edmondson @ 2019-12-13 10:26 UTC (permalink / raw)
  To: ekeberg, notmuch

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] Limit attachment check to stop before forwarded message
  2019-12-12 23:35 ` [PATCH 0/2] Limit attachment check to stop before forwarded message ekeberg
                     ` (2 preceding siblings ...)
  2019-12-13 10:26   ` [PATCH 0/2] Limit attachment check to stop before forwarded message David Edmondson
@ 2019-12-14 11:39   ` David Bremner
  3 siblings, 0 replies; 8+ messages in thread
From: David Bremner @ 2019-12-14 11:39 UTC (permalink / raw)
  To: ekeberg, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages
  2019-05-08 17:19 [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages Örjan Ekeberg
  2019-06-09 23:12 ` Daniel Kahn Gillmor
  2019-12-12 23:35 ` [PATCH 0/2] Limit attachment check to stop before forwarded message ekeberg
@ 2021-12-25 18:04 ` David Bremner
  2 siblings, 0 replies; 8+ messages in thread
From: David Bremner @ 2021-12-25 18:04 UTC (permalink / raw)
  To: Örjan Ekeberg, notmuch

Ö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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-12-25 18:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-08 17:19 [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages Örjan Ekeberg
2019-06-09 23:12 ` Daniel Kahn Gillmor
2019-12-12 23:35 ` [PATCH 0/2] Limit attachment check to stop before forwarded message ekeberg
2019-12-12 23:35   ` [PATCH 1/2] emacs: limit search for attachment to stop at first mime-part ekeberg
2019-12-12 23:35   ` [PATCH 2/2] test: extend test of attachment warnings ekeberg
2019-12-13 10:26   ` [PATCH 0/2] Limit attachment check to stop before forwarded message David Edmondson
2019-12-14 11:39   ` David Bremner
2021-12-25 18:04 ` [BUG] emacs: notmuch-mua-attachment-check finds triggering string inside forwarded messages David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).