Re: master eaf224f: Repad the Face header in Gnus

[Alex Bochannek]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Robert Pluim <rpluim@gmail.com> writes:
>
>> This signals an error because at this point 'substr' is a line of the
>> Face header with a leading space. Any of Lars' messages will trigger it.
>
> I pushed a fix a couple of hours ago...

Thanks Robert for finding the failure and thanks Lars for responding so
quickly!

This is my bad for not properly testing for the folding whitespace
continuation in long header fields. While spaces are illegal characters
in base 64, which is what I was testing for, it makes sense to deal with
potential FWSP sequences first since this function is specifically for
Gnus.

diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index 0e15ebce6c..1cf6bb7053 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -1368,7 +1368,11 @@ gnus-base64-repad
   ;;   input (3.1, 3.3)
   ;; - if line-length is set, error on input exceeding the limit (3.1)
   ;; - reject characters outside base encoding (3.3, also section 12)
-  (let ((splitstr (split-string str "[\n\r \t]+" t)))
+  ;;
+  ;; RFC 5322 section 2.2.3 consideration:
+  ;; Because base 64-encoded strings can appear in long header fields, remove
+  ;; folding whitespace while still observing the RFC 4648 decisions above.
+  (let ((splitstr (split-string str "[ \t]*[\r\n]+[ \t]?" t)))
     (when (and reject-newlines (> (length splitstr) 1))
       (error "Invalid Base64 string"))
     (dolist (substr splitstr)

The above is a bit stricter than the latest commit by Lars: SP and HTAB
can occur before a CRLF and there can only be one SP or HTAB on the next
line. I am not entirely sure how end-of-line works here cross-platform
and whether the string will still use the convention of the message
format or the local convention on the receiving end, so [\r\n]+ seems
the safest.

The fix Lars submitted is more lenient and I am OK with that one too, of
course. I just felt since I made the original contribution, it is my
responsibility to offer a fix as well.

I also updated the tests accordingly. Please note that this
implementation /will/ fail a whitespace if it isn't followed by an
end-of-line. While updating that test, I cleaned up the other ones a
bit.

diff --git a/test/lisp/gnus/gnus-util-tests.el 
b/test/lisp/gnus/gnus-util-tests.el
index ec58032e84..47f0a9cf76 100644
--- a/test/lisp/gnus/gnus-util-tests.el
+++ b/test/lisp/gnus/gnus-util-tests.el
@@ -151,8 +151,10 @@ gnus-base64-repad
   (should (equal "Zg==" (gnus-base64-repad "Zg")))
   (should (equal "Zg==" (gnus-base64-repad "Zg====")))
 
-  (should (equal (gnus-base64-repad " ") ""))
-  (should (equal (gnus-base64-repad "Zg== ") "Zg=="))
+  (should-error (gnus-base64-repad " ")
+                :type 'error)
+  (should-error (gnus-base64-repad "Zg== ")
+                :type 'error)
   (should-error (gnus-base64-repad "Z?\x00g==")
                 :type 'error)
   ;; line-length
@@ -162,9 +164,10 @@ gnus-base64-repad
   (should-error (gnus-base64-repad "Zm9v\r\nYmFy" t)
                 :type 'error)
   (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9vYmFy" t)))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy" nil)))
-  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy\n" nil)))
-  (should (equal (gnus-base64-repad "Zm9v\r\n YmFy\r\n" nil) "Zm9vYmFy"))
+  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy")))
+  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\nYmFy\n")))
+  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v\r\n YmFy\r\n")))
+  (should (equal "Zm9vYmFy" (gnus-base64-repad "Zm9v \r\n\tYmFy")))
   (should-error (gnus-base64-repad "Zm9v\r\nYmFy" nil 3)
                 :type 'error))
 

Let me know if you have any concerns about this or want me to make any
additional changes. Thanks again for finding the failure, Robert!

-- 
Alex.