bug#74516: 31.0.50; ERC 5.6.1-git: [PATCH] allow port as string in auth-source params

["J.P." <jp@neverwas.me>]

[-- Attachment #1: Type: text/plain, Size: 3635 bytes --]

Trevor Arjeski <tmarjeski@gmail.com> writes:

> Reproduction steps:
>
> 1. Have an entry in .authinfo, such as
>         machine yourbouncer login nick password hunter2 port 7777
>
> 2. Using the following code in init.el, open emacs
> 3. M-x erc-connect
> 4. Notice ERC tries connecting without password
> 5. Change erc-port to be an integer (commented out below)
> 6. Retry steps 2 - 4
>
> [3. text/x-org]
> #+BEGIN_SRC emacs-lisp
> (use-package erc
>   :ensure nil
>   :preface
>   (defun erc-connect ()
>     (interactive)
>     (erc :server erc-server
>          :port erc-port
>          :user erc-nick))
>   :custom
>   (erc-server "yourbouncer")
>   (erc-port "7777") ;; (erc-port 7777) is working
>   (erc-nick "nick"))
> #+END_SRC

Thanks. I can reproduce this.

> From 9468a786fb8c0ef950117e78395592f2e11613c2 Mon Sep 17 00:00:00 2001
> From: Trevor Arjeski <tmarjeski@gmail.com>
> Date: Sun, 24 Nov 2024 23:35:41 +0300
> Subject: [PATCH] erc: allow port as string in auth-source params
>
> Checking the equality of the given `erc-session-port' with "irc" is
> unnecessary since:
>
> 1. "irc" is already added to the list of ports
> 2. /etc/services may contain "ircs-u" (or other) as the desired port

I think it makes sense to allow entries to specify well known service
names. And I suppose it couldn't hurt to also at least implicitly
support numeric strings, although that sounds like bad UX if anyone
should need to resort to that just to differentiate between entries.

> If the correct port/service is missing then the auth-source query will
> fail for a seemingly unknown reason.

IIRC, all params appearing in an auth-source entry are basically
required, and a successful query must therefore supply all of them.
However, such a query may specify additional, unmatched or partially
matched parameters.

> This also allows a user to `(setopt erc-port "1234")', intentionally or
> accidentally, and still be able to use .authinfo for password
> management.
> ---
>  lisp/erc/erc.el | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
> index 7028d0a68cc..81818a7227e 100644
> --- a/lisp/erc/erc.el
> +++ b/lisp/erc/erc.el
> @@ -4675,8 +4675,7 @@ erc--auth-source-determine-params-defaults
>                    (list net erc-server-announced-name erc-session-server)))
>           (ports (list (cl-typecase erc-session-port
>                          (integer (number-to-string erc-session-port))
> -                        (string (and (string= erc-session-port "irc")
> -                                     erc-session-port)) ; or nil
> +                        (string erc-session-port) ; or nil

I've changed this slightly to become nil if `erc-session-port' is the
empty string or "irc".

>                          (t erc-session-port))
>                        "irc")))
>      (list (cons :host (delq nil hosts))

I've also added a companion patch that changes some foundational
behavior so that `erc-session-port' can more easily be set to a string
to accommodate service names. With your example of

  machine mybouncer port ircs-u login mynick password hunter2

if someone tries to connect with

  (setopt auth-source-do-cache nil
          auth-source-debug t)
  (erc-tls :server "mybouncer" :nick "mynick")

they should still be denied. But adding

  :port "ircs-u"

to the invocation or doing something like

  (setopt erc-port "ircs-u")

beforehand should see them succeed, assuming ircs-u appears in their
/etc/services as a port listened on by their bouncer.

Please try these out when you get a chance and give feedback if
possible. Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6.1-Allow-querying-auth-source-with-port-as-string.patch --]
[-- Type: text/x-patch, Size: 1817 bytes --]

From 0b82f7665c81008a0a3683c459c492f454c1a2af Mon Sep 17 00:00:00 2001
From: Trevor Arjeski <tmarjeski@gmail.com>
Date: Sun, 24 Nov 2024 23:35:41 +0300
Subject: [PATCH 1/2] [5.6.1] Allow querying auth-source with port as string in
 ERC

* lisp/erc/erc.el (erc--auth-source-determine-params-defaults): Allow
arbitrary strings for `ers-session-port'.  Previously, if a port/service
was any string other than "irc", the auth-source query would fail for a
seemingly unknown reason.  Restricting the value to "irc" is unnecessary
since "irc" is already added to the list of ports, and
`make-network-process' already consults /etc/services for well-known
service names, like "ircs-u", etc.  This change allows a user to (setopt
erc-port "1234"), intentionally or accidentally, while still being able
to use .authinfo for password management.  (Bug#74516)

Copyright-paperwork-exempt: yes
---
 lisp/erc/erc.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 7028d0a68cc..60d84b2b20c 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -4675,8 +4675,9 @@ erc--auth-source-determine-params-defaults
                   (list net erc-server-announced-name erc-session-server)))
          (ports (list (cl-typecase erc-session-port
                         (integer (number-to-string erc-session-port))
-                        (string (and (string= erc-session-port "irc")
-                                     erc-session-port)) ; or nil
+                        (string (and (not (member erc-session-port
+                                                  '("" "irc")))
+                                     erc-session-port))
                         (t erc-session-port))
                       "irc")))
     (list (cons :host (delq nil hosts))
-- 
2.47.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-5.6.1-Only-normalize-ports-for-equality-comparisons-.patch --]
[-- Type: text/x-patch, Size: 12693 bytes --]

From 420463c7f89a708ab0cc80e5bb2f6715fc48a03c Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Mon, 25 Nov 2024 04:21:36 -0800
Subject: [PATCH 2/2] [5.6.1] Only normalize ports for equality comparisons in
 ERC

* etc/ERC-NEWS: Add entry explaining changes to entry point 'erc-tls'
and library functions, like `erc-normalize-port' and `erc-compute-port'.
* lisp/erc/erc.el (erc-normalize-port): Map "ircu" to 6665 instead of
6667.  Add other IANA mappings.  Return 0 for unknown nonempty strings.
(erc-open): Pass `erc-session-port' through
`erc-string-to-port' before handing it to `erc-server-connect'.  This
prevents the network-connection machinery from seeing a numeric string,
like "6667".
(erc-select-read-args): Use `erc-port-equal' instead of `eql'.
(erc-tls): Respect a configured non-nil `erc-port' option when the user
does not provide a :port argument.
(erc-determine-parameters): Use `erc-compute-port' for
`erc-session-port'.
(erc-compute-port): Don't pass the result through `erc-normalize-port',
which can convert it to an unintuitive form.
(erc-handle-irc-url): Use `erc-port-equal' for comparison.  (Bug#74516)
* test/lisp/erc/erc-scenarios-auth-source.el
(erc-scenarios-common--auth-source)
(erc-scenarios-base-auth-source-server--dialed): Use `erc-port' option
instead of passing a :port parameter to entry-point command.
* test/lisp/erc/erc-tests.el (erc-normalize-port): New test.
---
 etc/ERC-NEWS                               | 25 +++++++++++
 lisp/erc/erc.el                            | 52 +++++++++++++---------
 test/lisp/erc/erc-scenarios-auth-source.el | 29 +++++++-----
 test/lisp/erc/erc-tests.el                 | 16 +++++++
 4 files changed, 88 insertions(+), 34 deletions(-)

diff --git a/etc/ERC-NEWS b/etc/ERC-NEWS
index 3970f67d725..1c7c278338f 100644
--- a/etc/ERC-NEWS
+++ b/etc/ERC-NEWS
@@ -62,6 +62,31 @@ of concerns and the newer module's "experimental" status, the migration
 was deemed worth any potential disruption, despite this being a point
 release.  ERC appreciates your understanding in this matter.
 
+** Entry-point command 'erc-tls' now considers option 'erc-port'.
+When called in Lisp code, this command now respects a non-nil 'erc-port'
+if the ':port' keyword is absent.  This also means users with this
+option set to a non-TLS port should make sure to specify a ':port' from
+now on.
+
+** Changes in the library API.
+
+*** Function 'erc-normalize-port' may return 0 instead of nil.
+When given a nonempty, non-numeric string, this function now returns 0.
+ERC now officially requests that users not use its output for anything
+but comparing port equality, which was always its intended purpose.
+
+*** Function 'erc-compute-port' no longer uses 'erc-normalize-port'.
+An uninformed change in ERC 5.5 led to 'erc-compute-port' filtering its
+result through 'erc-normalize-port', which brought unwelcome type
+coercion and possible null return values.  This defied its purpose of
+ensuring a usable port.  Users reliant on the aberrant 5.5 behavior
+should wrap its return value in 'erc-normalize-port'.
+
+*** Local variable 'erc-session-port' may be a string.
+Although this has always been the case, string values are now more
+likely to be seen because ERC no longer coerces service names to port
+numbers.
+
 \f
 * Changes in ERC 5.6
 
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 60d84b2b20c..46f096142d6 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1987,13 +1987,11 @@ erc-reuse-buffers
                         "old behavior when t now permanent" "29.1")
 
 (defun erc-normalize-port (port)
-  "Normalize the port specification PORT to integer form.
-PORT may be an integer, a string or a symbol.  If it is a string or a
-symbol, it may have these values:
-* irc         -> 194
-* ircs        -> 994
-* ircd        -> 6667
-* ircd-dalnet -> 7000"
+  "Normalize known PORT specifications to an integer.
+Expect PORT to be an integer, a string, or a symbol to coerce into a
+standardized form for the express purpose of equality comparisons.  If
+PORT is an IANA recognized service, return its numeric mapping.  Do the
+same for a few traditional but nonstandard names."
   ;; These were updated somewhat in 2022 to reflect modern standards
   ;; and practices.  See also:
   ;;
@@ -2001,7 +1999,7 @@ erc-normalize-port
   ;; https://www.iana.org/assignments/service-names-port-numbers
   (cond
    ((symbolp port)
-    (erc-normalize-port (symbol-name port)))
+    (and port (erc-normalize-port (symbol-name port))))
    ((stringp port)
     (let ((port-nr (string-to-number port)))
       (cond
@@ -2011,14 +2009,19 @@ erc-normalize-port
         194)
        ((string-equal port "ircs")
         994)
-       ((string-equal port "ircu") 6667) ; 6665-6669
+       ((string-equal port "ircu") 6665)
+       ((string-equal port "ircu-2") 6666)
+       ((string-equal port "ircu-3") 6667)
+       ((string-equal port "ircu-4") 6668)
+       ((string-equal port "ircu-5") 6669)
        ((string-equal port "ircd") ; nonstandard (irc-serv is 529)
         6667)
        ((string-equal port "ircs-u") 6697)
        ((string-equal port "ircd-dalnet")
         7000)
+       ((string-empty-p port) nil)
        (t
-        nil))))
+        0))))
    ((numberp port)
     port)
    (t
@@ -2665,7 +2668,7 @@ erc-open
 
     (if connect
         (erc-server-connect erc-session-server
-                            erc-session-port
+                            (erc-string-to-port erc-session-port)
                             buffer
                             erc-session-client-certificate)
       (erc-update-mode-line))
@@ -2769,8 +2772,8 @@ erc-select-read-args
          (port (or (url-portspec url)
                    (erc-compute-port
                     (let ((d (erc-compute-port sp))) ; may be a string
-                      (read-string (format-prompt "Port" d)
-                                   nil nil d)))))
+                      (erc-string-to-port
+                       (read-string (format-prompt "Port" d) nil nil d))))))
          ;; Trust the user not to connect twice accidentally.  We
          ;; can't use `erc-already-logged-in' to check for an existing
          ;; connection without modifying it to consider USER and PASS.
@@ -2792,10 +2795,10 @@ erc-select-read-args
                                (format-prompt "Server password" p)
                              "Server password (optional): ")))
                    (if erc-prompt-for-password (read-passwd m nil p) p)))
-         (opener (and (or sp (eql port erc-default-port-tls)
+         (opener (and (or sp (erc-port-equal port erc-default-port-tls)
                           (and (equal server erc-default-server)
                                (not (string-prefix-p "irc://" input))
-                               (eql port erc-default-port)
+                               (erc-port-equal port erc-default-port)
                                (y-or-n-p "Connect using TLS instead? ")
                                (setq port erc-default-port-tls)))
                       #'erc-open-tls-stream))
@@ -2891,7 +2894,8 @@ 'erc-ssl
 
 ;;;###autoload
 (cl-defun erc-tls (&key (server (erc-compute-server))
-                        (port   (erc-compute-port 'ircs-u))
+                        (port   (let ((erc-default-port erc-default-port-tls))
+                                  (erc-compute-port)))
                         (nick   (erc-compute-nick))
                         (user   (erc-compute-user))
                         password
@@ -8841,7 +8845,7 @@ erc-determine-parameters
 - `erc-server-current-nick'"
   (setq erc-session-connector erc-server-connect-function
         erc-session-server (erc-compute-server server)
-        erc-session-port (or port erc-default-port)
+        erc-session-port (erc-compute-port port)
         erc-session-user-full-name (erc-compute-full-name name)
         erc-session-username (erc-compute-user user)
         erc-session-password (erc--compute-server-password passwd nick))
@@ -8914,8 +8918,12 @@ erc-compute-port
 
 - PORT (the argument passed to this function)
 - The `erc-port' option
-- The `erc-default-port' variable"
-  (erc-normalize-port (or port erc-port erc-default-port)))
+- The `erc-default-port' variable
+
+Note that between ERC 5.5 and 5.6.1, this function filtered its result
+through `erc-normalize-port', which introduced regrettable surprises,
+such as unwelcome, possibly null, type conversions."
+  (or (and port (not (equal "" port)) port) erc-port erc-default-port))
 
 ;; time routines
 
@@ -9909,9 +9917,9 @@ erc-handle-irc-url
                            (and (string-equal erc-session-server host)
                                 ;; Ports only matter when dialed hosts
                                 ;; match and we have sufficient info.
-                                (or (not port)
-                                    (= (erc-normalize-port erc-session-port)
-                                       port)))))))))
+                                (or (null port)
+                                    (erc-port-equal erc-session-port
+                                                    port)))))))))
          key deferred)
     (unless server-buffer
       (setq deferred t
diff --git a/test/lisp/erc/erc-scenarios-auth-source.el b/test/lisp/erc/erc-scenarios-auth-source.el
index f0a7a4cbaca..7e1e7c2f3ab 100644
--- a/test/lisp/erc/erc-scenarios-auth-source.el
+++ b/test/lisp/erc/erc-scenarios-auth-source.el
@@ -44,15 +44,19 @@ erc-scenarios-common--auth-source
                                    (string-join ents "\n")))
        (auth-sources (list netrc-file))
        (auth-source-do-cache nil)
+       (erc-port (and (eq erc-port 'test) (number-to-string port)))
        (erc-scenarios-common-extra-teardown (lambda ()
-                                              (delete-file netrc-file))))
+                                              (delete-file netrc-file)))
+       ;; With a `cl-defun', a keyword's presence prevents the default
+       ;; init form from being evaluated, even if its value is nil.
+       (args `( :server "127.0.0.1"
+                ,@(and (null erc-port) (list :port port))
+                :nick "tester"
+                :full-name "tester"
+                :id ,id)))
 
     (ert-info ("Connect")
-      (with-current-buffer (erc :server "127.0.0.1"
-                                :port port
-                                :nick "tester"
-                                :full-name "tester"
-                                :id id)
+      (with-current-buffer (apply #'erc args)
         (should (string= (buffer-name) (if id
                                            (symbol-name id)
                                          (format "127.0.0.1:%d" port))))
@@ -60,12 +64,13 @@ erc-scenarios-common--auth-source
 
 (ert-deftest erc-scenarios-base-auth-source-server--dialed ()
   :tags '(:expensive-test)
-  (erc-scenarios-common--auth-source
-   nil 'foonet
-   "machine GNU.chat port %d user tester password fake"
-   "machine FooNet port %d user tester password fake"
-   "machine 127.0.0.1 port %d user tester password changeme"
-   "machine 127.0.0.1 port %d user imposter password fake"))
+  (let ((erc-port 'test))
+    (erc-scenarios-common--auth-source
+     nil 'foonet
+     "machine GNU.chat port %d user tester password fake"
+     "machine FooNet port %d user tester password fake"
+     "machine 127.0.0.1 port \"%s\" user tester password changeme" ; correct
+     "machine 127.0.0.1 port %d user imposter password fake")))
 
 (ert-deftest erc-scenarios-base-auth-source-server--netid ()
   :tags '(:expensive-test)
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 4c5521721f0..c6936ba8dc0 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -2978,6 +2978,22 @@ erc--route-insertion
     (should-not (buffer-live-p spam-buffer))
     (kill-buffer chan-buffer)))
 
+(ert-deftest erc-normalize-port ()
+  ;; The empty string, nil, and unsupported types become nil.
+  (should-not (erc-normalize-port ""))
+  (should-not (erc-normalize-port nil))
+  (should-not (erc-normalize-port (current-buffer)))
+
+  ;; Unrecognized names are coerced to 0.
+  (should (equal 0 (erc-normalize-port "fake")))
+
+  ;; Numbers pass through, but numeric strings are coerced.
+  (should (equal 6667 (erc-normalize-port 6667)))
+  (should (equal 6697 (erc-normalize-port "6697")))
+
+  ;; Strange IANA mappings recognized.
+  (should (equal 6665 (erc-normalize-port "ircu"))))
+
 (defvar erc-tests--ipv6-examples
   '("1:2:3:4:5:6:7:8"
     "::ffff:10.0.0.1" "::ffff:1.2.3.4" "::ffff:0.0.0.0"
-- 
2.47.0