From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: "J.P." Newsgroups: gmane.emacs.bugs Subject: bug#62444: [PATCH] erc: Fix "dcc get" flag parsing Date: Sat, 25 Mar 2023 21:10:26 -0700 Message-ID: <87a600xidp.fsf__8096.96602388925$1679803890$gmane$org@neverwas.me> References: Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="23157"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 62444@debbugs.gnu.org, emacs-erc@gnu.org To: Daniel Pettersson Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Mar 26 06:11:22 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pgHj0-0005lm-2j for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 26 Mar 2023 06:11:22 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pgHii-0007pq-99; Sun, 26 Mar 2023 00:11:04 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pgHig-0007pN-UH for bug-gnu-emacs@gnu.org; Sun, 26 Mar 2023 00:11:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pgHig-0002UG-8j for bug-gnu-emacs@gnu.org; Sun, 26 Mar 2023 00:11:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pgHif-0007ex-JP for bug-gnu-emacs@gnu.org; Sun, 26 Mar 2023 00:11:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: "J.P." Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 26 Mar 2023 04:11:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 62444 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 62444-submit@debbugs.gnu.org id=B62444.167980384529420 (code B ref 62444); Sun, 26 Mar 2023 04:11:01 +0000 Original-Received: (at 62444) by debbugs.gnu.org; 26 Mar 2023 04:10:45 +0000 Original-Received: from localhost ([127.0.0.1]:43779 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pgHiO-0007eR-Jr for submit@debbugs.gnu.org; Sun, 26 Mar 2023 00:10:45 -0400 Original-Received: from mail-108-mta51.mxroute.com ([136.175.108.51]:46351) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pgHiL-0007eD-9X for 62444@debbugs.gnu.org; Sun, 26 Mar 2023 00:10:42 -0400 Original-Received: from mail-111-mta2.mxroute.com ([136.175.111.2] filter006.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta51.mxroute.com (ZoneMTA) with ESMTPSA id 1871c1c5456000edb4.001 for <62444@debbugs.gnu.org> (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Sun, 26 Mar 2023 04:10:31 +0000 X-Zone-Loop: 1bcbac1e6ebe651dd5d73330386483dadc29bdfa5b26 X-Originating-IP: [136.175.111.2] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=neverwas.me ; s=x; h=Content-Type:MIME-Version:Message-ID:Date:References:In-Reply-To: Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=6V8u2QAPV9hoOBR9Du27IQ/vJADuHUOGJdqjw7YZ41w=; b=BLDrtJN9PaJCI+dX2gWPR4EAxq w91hd+h/JoW0sPyoh3vU7wM358B+dB95kE/0K+uYo/DY/7qeGQfm/+QNanIGPyaI81Cb6cSRd2MXZ GwTJ2mqUcIrecJ79j9LfuAAVdkkiWwydh6+51lIN7w8B2FlQNIynwA1HveRm862d7jZ15TeThb9J6 4us1F5W8e6E4vNv2BjCfUkYk3Png7VToNbKoq05qhefEpe+cFVIvuX7Q3csApBHLE+eqld6alWF15 2N2H9mSYPkJB6bzPkV2kqWaqdbYV4z83Ff5MzTN2HA5wpULsK9B6mBkt69r87Vqkd4g4yu+o/4Vn7 L2dzyLuQ==; In-Reply-To: (Daniel Pettersson's message of "Sat, 25 Mar 2023 16:25:48 +0100") X-Authenticated-Id: masked@neverwas.me X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:258643 Archived-At: --=-=-= Content-Type: text/plain 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 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: emacs-erc@gnu.org 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. --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-POC-Demo-broken-flags-parsing-in-erc-dcc-do-GET-comm.patch >From 7a349e180d1f99fecfb1179e5885e9aa9a7b0ac2 Mon Sep 17 00:00:00 2001 From: Daniel Pettersson 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 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0002-Fix-DCC-GET-flag-parsing-in-erc-dcc.patch >From 9917469aef02a54db9eec4b3f22f85e596e1fb52 Mon Sep 17 00:00:00 2001 From: Daniel Pettersson 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 "tester!~tester@fake.irc" @@ -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 --=-=-=--