[PATCH WIP] emacs: Sanitize authors and subjects in search and show

[PATCH WIP] emacs: Sanitize authors and subjects in search and show

[Austin Clements]

Authors and subjects can contain embedded, encoded control characters
like "\n" and "\t" that mess up display.  Transform control characters
into spaces everywhere we display them in search and show.
---

This could obviously use some tests, but I thought I'd get it out
there to see what people thought or if the behavior should be tweaked.

Of course, I can't guarantee that this is all of the places we display
untrusted header text.  I'm really not sure how to make that guarantee
(suggestions welcome).

 emacs/notmuch-lib.el  | 6 ++++++
 emacs/notmuch-show.el | 7 ++++---
 emacs/notmuch.el      | 6 ++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 58f3313..6541282 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -243,6 +243,12 @@ depending on the value of `notmuch-poll-script'."
 	"[No Subject]"
       subject)))
 
+(defun notmuch-sanitize (str)
+  "Sanitize control character in STR.
+
+This includes newlines, tabs, and other funny characters."
+  (replace-regexp-in-string "[[:cntrl:]\x7f\u2028\u2029]+" " " str))
+
 (defun notmuch-escape-boolean-term (term)
   "Escape a boolean term for use in a query.
 
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 7325792..fa11d98 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -407,7 +407,8 @@ unchanged ADDRESS if parsing fails."
 message at DEPTH in the current thread."
   (let ((start (point)))
     (insert (notmuch-show-spaces-n (* notmuch-show-indent-messages-width depth))
-	    (notmuch-show-clean-address (plist-get headers :From))
+	    (notmuch-sanitize
+	     (notmuch-show-clean-address (plist-get headers :From)))
 	    " ("
 	    date
 	    ") ("
@@ -417,7 +418,7 @@ message at DEPTH in the current thread."
 
 (defun notmuch-show-insert-header (header header-value)
   "Insert a single header."
-  (insert header ": " header-value "\n"))
+  (insert header ": " (notmuch-sanitize header-value) "\n"))
 
 (defun notmuch-show-insert-headers (headers)
   "Insert the headers of the current message."
@@ -1154,7 +1155,7 @@ function is used."
       (jit-lock-register #'notmuch-show-buttonise-links)
 
       ;; Set the header line to the subject of the first message.
-      (setq header-line-format (notmuch-show-strip-re (notmuch-show-get-subject)))
+      (setq header-line-format (notmuch-sanitize (notmuch-show-strip-re (notmuch-show-get-subject))))
 
       (run-hooks 'notmuch-show-hook))))
 
diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index c47c6b5..44cd2fd 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -791,11 +791,13 @@ non-authors is found, assume that all of the authors match."
 					(plist-get result :total)))
 			'face 'notmuch-search-count)))
    ((string-equal field "subject")
-    (insert (propertize (format format-string (plist-get result :subject))
+    (insert (propertize (format format-string
+				(notmuch-sanitize (plist-get result :subject)))
 			'face 'notmuch-search-subject)))
 
    ((string-equal field "authors")
-    (notmuch-search-insert-authors format-string (plist-get result :authors)))
+    (notmuch-search-insert-authors
+     format-string (notmuch-sanitize (plist-get result :authors))))
 
    ((string-equal field "tags")
     (let ((tags (plist-get result :tags)))
-- 
1.8.4.rc3

Re: [PATCH WIP] emacs: Sanitize authors and subjects in search and show

[Jani Nikula]

On Fri, 11 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> Authors and subjects can contain embedded, encoded control characters
> like "\n" and "\t" that mess up display.  Transform control characters
> into spaces everywhere we display them in search and show.
> ---
>
> This could obviously use some tests, but I thought I'd get it out
> there to see what people thought or if the behavior should be tweaked.

I like it. Seems to work as advertized with some crappy Subject: lines
in my mail.

BR,
Jani.


>
> Of course, I can't guarantee that this is all of the places we display
> untrusted header text.  I'm really not sure how to make that guarantee
> (suggestions welcome).
>
>  emacs/notmuch-lib.el  | 6 ++++++
>  emacs/notmuch-show.el | 7 ++++---
>  emacs/notmuch.el      | 6 ++++--
>  3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 58f3313..6541282 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -243,6 +243,12 @@ depending on the value of `notmuch-poll-script'."
>  	"[No Subject]"
>        subject)))
>  
> +(defun notmuch-sanitize (str)
> +  "Sanitize control character in STR.
> +
> +This includes newlines, tabs, and other funny characters."
> +  (replace-regexp-in-string "[[:cntrl:]\x7f\u2028\u2029]+" " " str))
> +
>  (defun notmuch-escape-boolean-term (term)
>    "Escape a boolean term for use in a query.
>  
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 7325792..fa11d98 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -407,7 +407,8 @@ unchanged ADDRESS if parsing fails."
>  message at DEPTH in the current thread."
>    (let ((start (point)))
>      (insert (notmuch-show-spaces-n (* notmuch-show-indent-messages-width depth))
> -	    (notmuch-show-clean-address (plist-get headers :From))
> +	    (notmuch-sanitize
> +	     (notmuch-show-clean-address (plist-get headers :From)))
>  	    " ("
>  	    date
>  	    ") ("
> @@ -417,7 +418,7 @@ message at DEPTH in the current thread."
>  
>  (defun notmuch-show-insert-header (header header-value)
>    "Insert a single header."
> -  (insert header ": " header-value "\n"))
> +  (insert header ": " (notmuch-sanitize header-value) "\n"))
>  
>  (defun notmuch-show-insert-headers (headers)
>    "Insert the headers of the current message."
> @@ -1154,7 +1155,7 @@ function is used."
>        (jit-lock-register #'notmuch-show-buttonise-links)
>  
>        ;; Set the header line to the subject of the first message.
> -      (setq header-line-format (notmuch-show-strip-re (notmuch-show-get-subject)))
> +      (setq header-line-format (notmuch-sanitize (notmuch-show-strip-re (notmuch-show-get-subject))))
>  
>        (run-hooks 'notmuch-show-hook))))
>  
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index c47c6b5..44cd2fd 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -791,11 +791,13 @@ non-authors is found, assume that all of the authors match."
>  					(plist-get result :total)))
>  			'face 'notmuch-search-count)))
>     ((string-equal field "subject")
> -    (insert (propertize (format format-string (plist-get result :subject))
> +    (insert (propertize (format format-string
> +				(notmuch-sanitize (plist-get result :subject)))
>  			'face 'notmuch-search-subject)))
>  
>     ((string-equal field "authors")
> -    (notmuch-search-insert-authors format-string (plist-get result :authors)))
> +    (notmuch-search-insert-authors
> +     format-string (notmuch-sanitize (plist-get result :authors))))
>  
>     ((string-equal field "tags")
>      (let ((tags (plist-get result :tags)))
> -- 
> 1.8.4.rc3

Re: [PATCH WIP] emacs: Sanitize authors and subjects in search and show

[Tomi Ollila]

On Fri, Oct 11 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> Authors and subjects can contain embedded, encoded control characters
> like "\n" and "\t" that mess up display.  Transform control characters
> into spaces everywhere we display them in search and show.
> ---

LGTM.

Tomi


>
> This could obviously use some tests, but I thought I'd get it out
> there to see what people thought or if the behavior should be tweaked.
>
> Of course, I can't guarantee that this is all of the places we display
> untrusted header text.  I'm really not sure how to make that guarantee
> (suggestions welcome).
>
>  emacs/notmuch-lib.el  | 6 ++++++
>  emacs/notmuch-show.el | 7 ++++---
>  emacs/notmuch.el      | 6 ++++--
>  3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 58f3313..6541282 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -243,6 +243,12 @@ depending on the value of `notmuch-poll-script'."
>  	"[No Subject]"
>        subject)))
>  
> +(defun notmuch-sanitize (str)
> +  "Sanitize control character in STR.
> +
> +This includes newlines, tabs, and other funny characters."
> +  (replace-regexp-in-string "[[:cntrl:]\x7f\u2028\u2029]+" " " str))
> +
>  (defun notmuch-escape-boolean-term (term)
>    "Escape a boolean term for use in a query.
>  
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 7325792..fa11d98 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -407,7 +407,8 @@ unchanged ADDRESS if parsing fails."
>  message at DEPTH in the current thread."
>    (let ((start (point)))
>      (insert (notmuch-show-spaces-n (* notmuch-show-indent-messages-width depth))
> -	    (notmuch-show-clean-address (plist-get headers :From))
> +	    (notmuch-sanitize
> +	     (notmuch-show-clean-address (plist-get headers :From)))
>  	    " ("
>  	    date
>  	    ") ("
> @@ -417,7 +418,7 @@ message at DEPTH in the current thread."
>  
>  (defun notmuch-show-insert-header (header header-value)
>    "Insert a single header."
> -  (insert header ": " header-value "\n"))
> +  (insert header ": " (notmuch-sanitize header-value) "\n"))
>  
>  (defun notmuch-show-insert-headers (headers)
>    "Insert the headers of the current message."
> @@ -1154,7 +1155,7 @@ function is used."
>        (jit-lock-register #'notmuch-show-buttonise-links)
>  
>        ;; Set the header line to the subject of the first message.
> -      (setq header-line-format (notmuch-show-strip-re (notmuch-show-get-subject)))
> +      (setq header-line-format (notmuch-sanitize (notmuch-show-strip-re (notmuch-show-get-subject))))
>  
>        (run-hooks 'notmuch-show-hook))))
>  
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index c47c6b5..44cd2fd 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -791,11 +791,13 @@ non-authors is found, assume that all of the authors match."
>  					(plist-get result :total)))
>  			'face 'notmuch-search-count)))
>     ((string-equal field "subject")
> -    (insert (propertize (format format-string (plist-get result :subject))
> +    (insert (propertize (format format-string
> +				(notmuch-sanitize (plist-get result :subject)))
>  			'face 'notmuch-search-subject)))
>  
>     ((string-equal field "authors")
> -    (notmuch-search-insert-authors format-string (plist-get result :authors)))
> +    (notmuch-search-insert-authors
> +     format-string (notmuch-sanitize (plist-get result :authors))))
>  
>     ((string-equal field "tags")
>      (let ((tags (plist-get result :tags)))
> -- 
> 1.8.4.rc3
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH WIP] emacs: Sanitize authors and subjects in search and show

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> Authors and subjects can contain embedded, encoded control characters
> like "\n" and "\t" that mess up display.  Transform control characters
> into spaces everywhere we display them in search and show.
> ---
>
> This could obviously use some tests, but I thought I'd get it out
> there to see what people thought or if the behavior should be tweaked.

Pushed. I guess now would be a good time to think about tests.

d