~emersion/soju-dev

Remove CR and NUL in PRIVMSG/NOTICE from upstream v1 PROPOSED

Hubert Hirtz: 1
 Remove CR and NUL in PRIVMSG/NOTICE from upstream

 2 files changed, 8 insertions(+), 0 deletions(-)
Why I chose not to drop the connection and instead silently remove them:
1. we'd lose a message
1. it wouldn't solve the issue and instead just cause spurious
   disconnections (or in some cases a connect/disconnect infinite loop),
   since CR and NUL may appear in next messages,
1. they are 0-width chars and have no use in an IM context... they just
   might break scripts that operate on message stores, and some
   downstreams

Why I chose not to handle on all content:  if such bytes appear in
nicks, channel names or modes, then either we remove them and
downstreams end up corrupt (e.g. merged users and channels, wrong mode
strings), or we break the specs and send them.

Though there is a lot of other places they can end up like AWAY, TOPIC,
RPL_CREATIONTIME, RPL_TOPICWHOTIME, LIST, WHO, and others where they
can be safely removed.

In the end it's a of low priority issue since most upstreams don't send
CR nor NUL, I'm just not really confortable with letting them slip in
logs and downstreams.
Export patchset (mbox)
How do I use this?

Copy & paste the following snippet into your terminal to import this patchset into git:

curl -s https://lists.sr.ht/~emersion/soju-dev/patches/25809/mbox | git am -3
Learn more about email & git
View this thread in the archives

[PATCH] Remove CR and NUL in PRIVMSG/NOTICE from upstream Export this patch

They don't need to be removed from all upstream messages, since it would desync
downstreams' state.

They also don't need to be removed from downstream messages since
upstreams will reject them anyway.  Catching them early and sending back
nice error replies may be done in another patch.
---
 irc.go      | 7 +++++++
 upstream.go | 1 +
 2 files changed, 8 insertions(+)

diff --git a/irc.go b/irc.go
index 3c1d5cf..1d09437 100644
--- a/irc.go
+++ b/irc.go
@@ -25,6 +25,13 @@ const (
	maxMessageParams = 15
)

var cleanTextReplacer = strings.NewReplacer("\r", "", "\x00", "")

// cleanText removes bytes that cannot appear in IRC messages.
func cleanText(unclean string) string {
	return cleanTextReplacer.Replace(unclean)
}

// The server-time layout, as defined in the IRCv3 spec.
const serverTimeLayout = "2006-01-02T15:04:05.000Z"

diff --git a/upstream.go b/upstream.go
index 1c73059..6acd9d4 100644
--- a/upstream.go
+++ b/upstream.go
@@ -372,6 +372,7 @@ func (uc *upstreamConn) handleMessage(msg *irc.Message) error {
			if err := parseMessageParams(msg, &entity, &text); err != nil {
				return err
			}
			text = cleanText(text)
		} else {
			if err := parseMessageParams(msg, &entity); err != nil {
				return err
-- 
2.33.0
Not sure how I feel about this… This only handles PRIVMSG/NOTICE and not the
rest, and I don't like silently dropping the chars. Returning a proper error
would be nicer, but would disconnect us from upstream.