Re: bug#62444: [PATCH] erc: Fix "dcc get" flag parsing

"J.P." <[email protected]> Sat, 25 Mar 2023 21:10:26 -0700

Newsgroups	gmane.emacs.erc.general
Message-ID	<[email protected]>

Hi Daniel,

Thanks for submitting this report. I haven't gotten around to reviewing
your proposed changes properly but definitely will in the coming days.
For now, all I can offer are a few boring administrative notes.

Daniel Pettersson <[email protected]> writes:

> In erc mode when receiving a file with "/dcc get" if the nick or
> filename starts with a dash or the filename contains the following
> string " -", "/dcc get" is unable to download the file.

Indeed, sorry for any inconvenience this might have caused.

> Reproduce:
> As this is a bit cumbersome to reproduce without mocking files. I
> included a patch of erc-dcc-tests where the file name contains a the
> string " - ".
> Apply the following patch for erc-dcc-tests and run lisp-erc tests.
> ---
>
> diff --git a/test/lisp/erc/erc-dcc-tests.el b/test/lisp/erc/erc-dcc-tests.el
> index bd8a9fc7951..a487f9067cd 100644
> --- a/test/lisp/erc/erc-dcc-tests.el
> +++ b/test/lisp/erc/erc-dcc-tests.el
> @@ -109,7 +109,7 @@ erc-dcc-do-GET-command
[...]
>
> ---

This example makes sense, thanks. BTW, in the future, can you save out
your patches with git-format-patch and attach them somehow? This and
other conventions worth noting, such as formatting change-log style
commit messages, are detailed in CONTRIBUTE. As an example, I've
re-attached your patch with a reformatted commit message more along the
lines of what's expected (if you wouldn't mind taking a look).

Also, if you can remember, please add the header:

  X-Debbugs-CC: [email protected]

to any future bug reports (see also `erc-bug').

> Issue present since:
> df1e553688b * Accommodate nonstandard turbo file senders in erc-dcc
>
> Proposed patch:
> erc: Fix "dcc get" flag parsing
>
> When nick or filename starts with `?-' or filename contains the
> following string " -", "dcc get" is unable determine nick/filename and
> fails to download file.
>
> Flag parsing rules is kept as is:
> [flag] nick [flag] filename [flag]
>
> Flags have the highest priority when parsing the arguments to dcc
> get. This is not an complete fix as dcc will fail on:
>      - nicks "-s" and "-t"
>      - filenames starting with r"-s|t +"
>      - filenames with ending  with r" -s|t"

Guessing r"..." just means regexp? If not, please clarify.

> An more robust solution and cleaner implementation would be possible
> if flag position was limited to the end of the arguments list.
>
> This would also make it easier to implement pcomplete for flags as well.

I would tend to agree. Perhaps we ought to go ahead and only make these
-s and -t flags valid in the terminal position. Normally, that'd be a
hassle, likely involving the introduction of a "compat" user option. But
we've deliberately refrained from announcing these DCC features (other
than in the "usage" portion of the Commentary section atop the library
file). So I think a trivially breaking change in a patch version, like
5.5.1, might be doable.

Lastly, I have to mention the dreaded copyright thing because I couldn't
tell from the discussion for bug#57905 whether you ended up filing. If
not and we go with changes resembling those you've proposed, you'll
probably want to do so.

Thanks,
J.P.

0001-POC-Demo-broken-flags-parsing-in-erc-dcc-do-GET-comm.patch (text/x-patch, 3.8 KB)

From 7a349e180d1f99fecfb1179e5885e9aa9a7b0ac2 Mon Sep 17 00:00:00 2001
From: Daniel Pettersson <[email protected]>
Date: Sat, 25 Mar 2023 16:25:48 +0100
Subject: [PATCH 1/2] [POC] Demo broken flags parsing in erc-dcc-do-GET-command
 (bug#62444)

In erc mode when receiving a file with "/dcc get" if the nick or
filename starts with a dash or the filename contains the following
string " -", "/dcc get" is unable to download the file.

Reproduce:
As this is a bit cumbersome to reproduce without mocking files. I
included a patch of erc-dcc-tests where the file name contains a the
string " - ".
Apply the following patch for erc-dcc-tests and run lisp-erc tests.
---
 test/lisp/erc/erc-dcc-tests.el | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/test/lisp/erc/erc-dcc-tests.el b/test/lisp/erc/erc-dcc-tests.el
index bd8a9fc7951..a487f9067cd 100644
--- a/test/lisp/erc/erc-dcc-tests.el
+++ b/test/lisp/erc/erc-dcc-tests.el
@@ -109,7 +109,7 @@ erc-dcc-do-GET-command
                       :parent proc
                       :ip "127.0.0.1"
                       :port "9899"
-                      :file "foo.bin"
+                      :file "foo - .bin"
                       :size 1405135128))
            (erc-dcc-list (list elt))
            ;;
@@ -124,7 +124,7 @@ erc-dcc-do-GET-command
             erc-server-current-nick "dummy")
       (set-process-query-on-exit-flag proc nil)
       (cl-letf (((symbol-function 'read-file-name)
-                 (lambda (&rest _) "foo.bin"))
+                 (lambda (&rest _) "foo - .bin"))
                 ((symbol-function 'erc-dcc-get-file)
                  (lambda (&rest r) (push r calls))))
         (goto-char (point-max))
@@ -134,36 +134,36 @@ erc-dcc-do-GET-command
         (ert-info ("No turbo")
           (should-not (plist-member elt :turbo))
           (goto-char erc-input-marker)
-          (insert "/dcc GET tester foo.bin")
+          (insert "/dcc GET tester foo - .bin")
           (erc-send-current-line)
           (should-not (plist-member (car erc-dcc-list) :turbo))
-          (should (equal (pop calls) (list elt "foo.bin" proc))))
+          (should (equal (pop calls) (list elt "foo - .bin" proc))))
 
         (ert-info ("Arg turbo in pos 2")
           (should-not (plist-member elt :turbo))
           (goto-char erc-input-marker)
-          (insert "/dcc GET -t tester foo.bin")
+          (insert "/dcc GET -t tester foo - .bin")
           (erc-send-current-line)
           (should (eq t (plist-get (car erc-dcc-list) :turbo)))
-          (should (equal (pop calls) (list elt "foo.bin" proc))))
+          (should (equal (pop calls) (list elt "foo - .bin" proc))))
 
         (ert-info ("Arg turbo in pos 4")
           (setq elt (plist-put elt :turbo nil)
                 erc-dcc-list (list elt))
           (goto-char erc-input-marker)
-          (insert "/dcc GET tester -t foo.bin")
+          (insert "/dcc GET tester -t foo - .bin")
           (erc-send-current-line)
           (should (eq t (plist-get (car erc-dcc-list) :turbo)))
-          (should (equal (pop calls) (list elt "foo.bin" proc))))
+          (should (equal (pop calls) (list elt "foo - .bin" proc))))
 
         (ert-info ("Arg turbo in pos 6")
           (setq elt (plist-put elt :turbo nil)
                 erc-dcc-list (list elt))
           (goto-char erc-input-marker)
-          (insert "/dcc GET tester foo.bin -t")
+          (insert "/dcc GET tester foo - .bin -t")
           (erc-send-current-line)
           (should (eq t (plist-get (car erc-dcc-list) :turbo)))
-          (should (equal (pop calls) (list elt "foo.bin" proc))))))))
+          (should (equal (pop calls) (list elt "foo - .bin" proc))))))))
 
 (defun erc-dcc-tests--pcomplete-common (test-fn)
   (with-current-buffer (get-buffer-create "*erc-dcc-do-GET-command*")
-- 
2.39.2

0002-Fix-DCC-GET-flag-parsing-in-erc-dcc.patch (text/x-patch, 7.2 KB)

From 9917469aef02a54db9eec4b3f22f85e596e1fb52 Mon Sep 17 00:00:00 2001
From: Daniel Pettersson <[email protected]>
Date: Sat, 25 Mar 2023 16:25:48 +0100
Subject: [PATCH 2/2] Fix DCC GET flag parsing in erc-dcc

* lisp/erc/erc-dcc.el (erc-dcc-do-GET-command): Explain usage in doc
string.  Fix flag parsing so ERC better recognizes files that look
like flags.  This issue started with df1e553688b "Accommodate
nonstandard turbo file senders in erc-dcc".  When a nick or filename
starts with `?-' or contains a literal " -", "dcc get" is unable to
the determine the token type and fails to download the file.  This
change retains the existing parsing rules, namely, "FLAG nick FLAG
filename FLAG", where flags have the highest priority.  This is not a
complete fix as the command will still fail on things like nicks being
"-s" and "-t", filenames starting with r"-s|t +", and filenames ending
with " -s|t".  A more robust solution and a cleaner implementation
would be possible if flag position was limited to the end of the
arguments list.  This would also make it easier to implement pcomplete
for the command as well (bug#62444).
* test/lisp/erc/erc-dcc-tests.el (erc-dcc-do-GET-command): Call new
parameterized helper with various flags/file combinations.
(erc-dcc-tests--erc-dcc-do-GET-command): New fixture function.
---
 lisp/erc/erc-dcc.el            | 36 +++++++++++++++++++++++-----------
 test/lisp/erc/erc-dcc-tests.el | 27 ++++++++++++++-----------
 2 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/lisp/erc/erc-dcc.el b/lisp/erc/erc-dcc.el
index 4c557e0e0f9..d7c685e9413 100644
--- a/lisp/erc/erc-dcc.el
+++ b/lisp/erc/erc-dcc.el
@@ -504,18 +504,32 @@ erc-dcc-do-CLOSE-command
            ?n (erc-extract-nick (plist-get ret :nick))))))
     t))
 
-(defun erc-dcc-do-GET-command (proc nick &rest file)
-  "Do a DCC GET command.  NICK is the person who is sending the file.
-FILE is the filename.  If FILE is split into multiple arguments,
-re-join the arguments, separated by a space.
+(defun erc-dcc-do-GET-command (proc &rest args)
+  "Do a DCC GET command.
+ARGS are expected to contain:
+  nick     The person who is sending the file.
+  filename The filename to be downloaded. Can be split into multiple arguments
+           which is then joined by a space.
+  flags    \"-t\" sets `:turbo' see `erc-dcc-list'
+           \"-s\" sets `:secure' see `erc-dcc-list'
+ARGS are parsed as follows:
+  [flag] nick [flag] filename [flag]
 PROC is the server process."
-  (let* ((args (seq-group-by (lambda (s) (eq ?- (aref s 0))) (cons nick file)))
-         (flags (prog1 (cdr (assq t args))
-                  (setq args (cdr (assq nil args))
-                        nick (pop args)
-                        file (and args (mapconcat #'identity args " ")))))
-         (elt (erc-dcc-member :nick nick :type 'GET :file file))
-         (filename (or file (plist-get elt :file) "unknown")))
+  (let ((possible-flags '("-s" "-t"))
+        flags nick elt possible-files filename)
+    ;; Get flags between get and nick
+    (while (seq-contains-p possible-flags (car args) 'equal)
+      (setq flags (cons (pop args) flags)))
+    (setq nick (or (pop args) ""))
+    ;; Get flags between nick and filename
+    (while (seq-contains-p possible-flags (car args) 'equal)
+      (setq flags (cons (pop args) flags)))
+    ;; Get flags after filename
+    (setq args (reverse args))
+    (while (seq-contains-p possible-flags (car args) 'equal)
+      (setq flags (cons (pop args) flags)))
+    (setq filename (or (mapconcat #'identity (reverse args) " ") "")
+          elt (erc-dcc-member :nick nick :type 'GET :file filename))
     (if elt
         (let* ((file (read-file-name
                       (format-prompt "Local filename"
diff --git a/test/lisp/erc/erc-dcc-tests.el b/test/lisp/erc/erc-dcc-tests.el
index a487f9067cd..f21463bb5a0 100644
--- a/test/lisp/erc/erc-dcc-tests.el
+++ b/test/lisp/erc/erc-dcc-tests.el
@@ -100,7 +100,7 @@ erc-dcc-handle-ctcp-send--base
 (ert-deftest erc-dcc-handle-ctcp-send--turbo ()
   (erc-dcc-tests--dcc-handle-ctcp-send t))
 
-(ert-deftest erc-dcc-do-GET-command ()
+(defun erc-dcc-tests--erc-dcc-do-GET-command (file)
   (with-temp-buffer
     (let* ((proc (start-process "fake" (current-buffer) "sleep" "10"))
            (elt (list :nick "[email protected]"
@@ -109,7 +109,7 @@ erc-dcc-do-GET-command
                       :parent proc
                       :ip "127.0.0.1"
                       :port "9899"
-                      :file "foo - .bin"
+                      :file file
                       :size 1405135128))
            (erc-dcc-list (list elt))
            ;;
@@ -124,7 +124,7 @@ erc-dcc-do-GET-command
             erc-server-current-nick "dummy")
       (set-process-query-on-exit-flag proc nil)
       (cl-letf (((symbol-function 'read-file-name)
-                 (lambda (&rest _) "foo - .bin"))
+                 (lambda (&rest _) file))
                 ((symbol-function 'erc-dcc-get-file)
                  (lambda (&rest r) (push r calls))))
         (goto-char (point-max))
@@ -134,36 +134,41 @@ erc-dcc-do-GET-command
         (ert-info ("No turbo")
           (should-not (plist-member elt :turbo))
           (goto-char erc-input-marker)
-          (insert "/dcc GET tester foo - .bin")
+          (insert "/dcc GET tester " file)
           (erc-send-current-line)
           (should-not (plist-member (car erc-dcc-list) :turbo))
-          (should (equal (pop calls) (list elt "foo - .bin" proc))))
+          (should (equal (pop calls) (list elt file proc))))
 
         (ert-info ("Arg turbo in pos 2")
           (should-not (plist-member elt :turbo))
           (goto-char erc-input-marker)
-          (insert "/dcc GET -t tester foo - .bin")
+          (insert "/dcc GET -t tester " file)
           (erc-send-current-line)
           (should (eq t (plist-get (car erc-dcc-list) :turbo)))
-          (should (equal (pop calls) (list elt "foo - .bin" proc))))
+          (should (equal (pop calls) (list elt file proc))))
 
         (ert-info ("Arg turbo in pos 4")
           (setq elt (plist-put elt :turbo nil)
                 erc-dcc-list (list elt))
           (goto-char erc-input-marker)
-          (insert "/dcc GET tester -t foo - .bin")
+          (insert "/dcc GET tester -t " file)
           (erc-send-current-line)
           (should (eq t (plist-get (car erc-dcc-list) :turbo)))
-          (should (equal (pop calls) (list elt "foo - .bin" proc))))
+          (should (equal (pop calls) (list elt file proc))))
 
         (ert-info ("Arg turbo in pos 6")
           (setq elt (plist-put elt :turbo nil)
                 erc-dcc-list (list elt))
           (goto-char erc-input-marker)
-          (insert "/dcc GET tester foo - .bin -t")
+          (insert "/dcc GET tester " file " -t")
           (erc-send-current-line)
           (should (eq t (plist-get (car erc-dcc-list) :turbo)))
-          (should (equal (pop calls) (list elt "foo - .bin" proc))))))))
+          (should (equal (pop calls) (list elt file proc))))))))
+
+(ert-deftest erc-dcc-do-GET-command ()
+  (erc-dcc-tests--erc-dcc-do-GET-command "foo.bin")
+  (erc-dcc-tests--erc-dcc-do-GET-command "foo - file.bin")
+  (erc-dcc-tests--erc-dcc-do-GET-command "foo -t file.bin"))
 
 (defun erc-dcc-tests--pcomplete-common (test-fn)
   (with-current-buffer (get-buffer-create "*erc-dcc-do-GET-command*")
-- 
2.39.2