bug#59976: ERC 5.4.1: erc-networks--id gets clobbered in erc server buffer on /query name conflict

[Mike Kazantsev]

On Mon, 12 Dec 2022 06:35:00 -0800
"J.P." <jp@neverwas.me> wrote:

> Mike Kazantsev <mk.fraggod@gmail.com> writes:

> > I've needed to use case-sensitive channel names (with matrix2051 ad-hoc
> > ircd), wanted to test updated ERC client for that, and stumbled upon
> > what looks like a bug there:
> >
> >   When IRC server is named "slack", and then it sends you a message from
> >   user named "slack", ERC tries to open query-buffer "slack", ends up
> >   running erc--open-target("slack") which does kill-all-local-variables()
> >   when enabling erc-mode in the buffer.
> >
> >   That removes erc-networks--id value there and from then on ERC keeps
> >   spamming "error in process filter" about erc-networks--id being nil.
> >
> >
> > - What I'm trying to do:
> >
> >   - (erc-tls ...)  ;; cleates "slack" server buffer and some channels.
> >   - Open "slack" buffer  
> 
> You mean opened manually, like by issuing a /query slack, right? So this
> is in addition to the observation above where "it sends you a message
> from user named 'slack'"?

Sorry, no, I meant as in (switch-to-buffer "slack"), to type command into.

No manual /query command is implied anywhere, but maybe it can also be
used to reproduce the issue, given how it should do roughly same thing.


> >   - Send "/ping myuser" IRC-command there
> 
> You also mention "case-sensitive channel names." It'd be nice to know
> what those look like when they first arrive off the wire. (BTW, if
> you're expecting ERC to interoperate with a case-sensitive network, like
> one for which "#FOO" and "#foo" are distinct channels, then that may
> need a separate bug report.)

It's not related to this issue at all, and I probably shouldn't have
mentioned it, to avoid adding extra confusion, sorry.
I.e. "slack" buffer in case of this issue is all-lowercase everywhere.

But as a separate unrelated thing, here is specific problem that I was
thinking to try solving with updated ERC, as it seem to have some new
code added, related to casemapping:

  (to stress this - it's a separate issue from this one, only here as a
  curiosity, should be ignored entirely in context of this bug report)

  Detailed description:
  https://github.com/progval/matrix2051/issues/41

  I thought at first that issue might be CASEMAPPING=rfc3454 used
  there, which I was not familiar with, and assumed that it doesn't
  behave like ascii/rfc1459 casemapping.

  So I was thinking to try adding it to new erc-downcase func locally,
  and fix the issue with case-sensitive channels that way.

  But after looking at RFC 3454 I've found that it includes casemapping
  for ASCII letters and pretty much just extends casemapping to larger
  unicode character set, and should be compatible with ERC, at least
  for the case of simple ASCII letters.

  So it seem to be a bug in progval/matrix2051 ad-hoc ircd
  implementation, which I've reported there at the URL above, with a
  workaround I've used in ERC mentioned in the first comment there,
  just in case someone might find it via google when bumping into same
  issue.

So, again, it's not really related to this issue, and not an ERC issue
at all, and I probably shouldn't have mentioned it here.


> > - Workaround: changing announced name of the server to something else
> >   helps - "slack" query-buffer gets created and is separate from server 
> > buffer.  
> 
> You mean by changing `erc-server-announced-name'?

Yes... sort of.

As I understand it, this name ends up being converted via
erc-networks-alist from erc-server-announced-name to a network symbol
(e.g. 'OFTC or 'slack) in erc-network value.

So guess my description above is incorrect or somewhat incomplete -
what you'd actually want is have symbol in that alist not conflict with
query-channel name, not just whatever erc-server-announced-name ends up
being.

Only slightly related to description above, and should likely be
ignored in context of this issue:

  To set erc-network to 'slack value I ended up replacing that alist
  lookup with a much simplier local solution:
  https://github.com/mk-fg/emacs-setup/blob/4dd532b/core/fg_erc.el#L72-L95
  (current version uses '🖧-slack symbol with that unicode prefix
  exactly to avoid any name conflicts)

  It's partly needed because ad-hoc protocol-bridge ircd's don't
  announce network names, and partly because just seemed simplier
  to derive them this way, but I didn't think about :id solution that
  you've mentioned below, which might work even better here.

  But pretty sure if one did use the alist-to-symbol lookup, they'd 
  end up with same result, e.g. 'slack in erc-network, just like it
  works with 'OFTC in there.


> > Was able to understand what seem to be the issue here by enabling
> > logging for erc-networks--id changes to *Messages* like this:
> >
> >   (defun debug-watch-log (sym newval op where)
> >     (message "Variable update: %s = %S -> %S [%S %S]"
> >       sym (symbol-value sym) newval op where)
> >     (backtrace))
> >   (add-variable-watcher 'erc-networks--id #'debug-watch-log)
> >
> > (run M-x cancel-debug-on-variable-change afterwards to disable this)  
> 
> Thanks for the detective work. In this case, I'm pretty sure the root
> cause is not directly related to that variable, despite all appearances.
> 
> BTW, the most helpful insights are usually to be found in the contents
> of the *erc-protocol* buffer, which can be generated by calling
> `erc-toggle-debug-irc-protocol' before connecting for the first time in
> an Emacs session. If you're not comfortable sharing such info on the
> tracker, please consider doing so out of band.

I think issue should be easily reproducible without needing the
protocol logs, by setting erc-network to your own nickname and then
sending /msg to yourself, or maybe even e.g. "/q OFTC".

Will try it out a bit later and report on whether such trivial ways to
do it work.


> > I think some kind of disambiguation or conflict-detection for
> > query-channel names might be either missing or not working atm,
> > but is needed to avoid this happening unintentionally, or maybe
> > even on purpose (e.g. to annoy someone by cutting their IRC connection).  
> 
> Agreed.
> 
> Upon detecting the collision, perhaps the query buffer needs to become
> "slack!~slack@example.com" (or "slack@slack/me") and/or the server
> buffer "slack/me". Additionally, we could have an option that says to
> always use full n!u@h's when naming query buffers instead of adapting
> dynamically.
> 
> That said, at the moment, we only support unambiguous "network IDs" via
> the ":id" keyword parameter of `erc' and `erc-tls'. IOW, calling these
> with args like
> 
>   :server "127.0.0.1"
>   :port 2051
>   ...
>   :id 'Slack
> 
> is supposed to always work. But, I can definitely see how that doesn't
> in some cases (e.g., a lowercase `:id'). As you say, you may have no
> control over who sends you a query, which is still pertinent in the case
> of an explicit `:id' so long as it consists entirely of valid nick
> characters.

That looks like it might be a good solution to my other issue with
deriving names for these ad-hoc protocol-bridge networks mentioned
above, thanks for mentioning it.


> Unfortunately, the best we can do for ERC 5.5 (Emacs 29) is to mention
> somewhere, like in (info "(erc) Network Identifier"), that users really
> worried about this issue should choose an `:id' containing characters
> disallowed in nicks by their network (or just something improbable and
> unlikely to be guessed). But, hopefully, we can address this in a more
> DWIM-like fashion in an upcoming ELPA release, such as ERC 5.6.

Hopefully won't be an issue for most people, although it might be not
that uncommon for this kind of service/bot/proxy ircd's to also use
their name for messaging user with any kind of service-related info/issues.


> > I'm running ERC 5.4.1 from current 0e5d059 git master on Emacs 28.2
> > (replacing "erc" dir in /usr/share/emacs), so it is also possible that
> > maybe some change in Emacs 29+ prevents this behavior, but it seems 
> > unlikely.  
> 
> Unlikely, yes. But I sometimes worry about ERC's loaddefs when people
> manually shadow/replace instead of building alongside Emacs or
> installing from devel:
> 
>   (push '("devel" . "https://elpa.gnu.org/devel/";) package-archives)

I didn't consider that newer ERC can be installed this way, given that
it seem to be part of the emacs tree, but good to know, thanks, will
probably use this instead.

(although tbf it seems more complicated than replacing a dir, but if
it's cleaner then yeah, probably best to do it via package-archives)


> (Not saying you need to do either, of course.) Thanks again for the very
> nice bug report.

Thank you for looking into this and work on ERC in general.

As someone who spends most of the day looking at emacs, it's by far the
most convenient and configurable client for me, that I end up using for
pretty much all modern IM protocols because of that.


> 
> J.P.
> 
> P.S. I've attached some patches for Emacs 29 that address issues
> discovered while briefly looking into this bug; the first is totally
> unrelated and the latter two only tangentially so (in case anyone wants
> to take a gander).


-- 
Mike Kazantsev // fraggod.net