From 75daebb72ffa3ee8754512af3e43f2e64501290b Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 20 Oct 2026 15:10:40 +0000 Subject: [PATCH] Check nicks on the IRC listener with the HTTP API's rule (closes #142) The nick rule moves from internal/handlers to internal/service as ValidNick, and the HTTP API and both IRC listener NICK paths call it. The IRC listener used to cut a nick to 32 bytes and accept anything else, so a nick with spaces such as "bob PRIVMSG #inj :hi" made the lines built from it read as another user's PRIVMSG. It now answers a nick the rule refuses with ERR_ERRONEUSNICKNAME (432), before and after registration, and keeps the nick as it was. Model: opus-5-5 --- README.md | 8 ++-- internal/handlers/api.go | 8 +--- internal/ircserver/commands.go | 9 ++++- internal/ircserver/conn.go | 12 ++++-- internal/ircserver/server_test.go | 62 +++++++++++++++++++++++++++++++ internal/service/service.go | 12 ++++++ 6 files changed, 96 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 53fe709..cfb9eb1 100644 --- a/README.md +++ b/README.md @@ -249,9 +249,11 @@ what channel bans (`+b` mode) match against. hold the same nick simultaneously. - Nicks are **case-sensitive** (unlike traditional IRC). `Alice` and `alice` are different nicks. -- Nick length: 1–32 characters. Over the HTTP API a nick starts with a letter or - `_`, followed by letters, digits and `` _-[]\^{}|` ``. The IRC listener cuts a - longer nick to 32 bytes and checks nothing else. +- Nick length: 1–32 characters. A nick starts with a letter or `_`, followed by + letters, digits and `` _-[]\^{}|` ``. The HTTP API and the IRC listener both + apply this rule. On the IRC listener, a `NICK` with any other nick, before or + after registration, gets ERR_ERRONEUSNICKNAME (432) and leaves the nick as it + was. - Nicks are **released when a session is destroyed** (via `QUIT` command or session expiry). There is no nick registration or reservation system. - Nick changes are broadcast to all users sharing a channel with the changer, as diff --git a/internal/handlers/api.go b/internal/handlers/api.go index 8ca7480..a9471a9 100644 --- a/internal/handlers/api.go +++ b/internal/handlers/api.go @@ -19,10 +19,6 @@ import ( "sneak.berlin/go/neoirc/pkg/irc" ) -var validNickRe = regexp.MustCompile( - `^[a-zA-Z_][a-zA-Z0-9_\-\[\]\\^{}|` + "`" + `]{0,31}$`, -) - var validUsernameRe = regexp.MustCompile( `^[a-zA-Z0-9_\-\[\]\\^{}|` + "`" + `]{1,32}$`, ) @@ -211,7 +207,7 @@ func (hdlr *Handlers) handleCreateSession( payload.Nick = strings.TrimSpace(payload.Nick) - if !validNickRe.MatchString(payload.Nick) { + if !service.ValidNick(payload.Nick) { hdlr.respondError( writer, request, "invalid nick format", @@ -1551,7 +1547,7 @@ func (hdlr *Handlers) handleNick( newNick := strings.TrimSpace(lines[0]) - if !validNickRe.MatchString(newNick) { + if !service.ValidNick(newNick) { hdlr.respondIRCError( writer, request, clientID, sessionID, irc.ErrErroneusNickname, nick, []string{newNick}, diff --git a/internal/ircserver/commands.go b/internal/ircserver/commands.go index f7ffa04..639c52e 100644 --- a/internal/ircserver/commands.go +++ b/internal/ircserver/commands.go @@ -71,8 +71,13 @@ func (c *Conn) handleNick( } newNick := msg.Params[0] - if len(newNick) > maxNickLen { - newNick = newNick[:maxNickLen] + if !service.ValidNick(newNick) { + c.sendNumeric( + irc.ErrErroneusNickname, + newNick, "Erroneous nickname", + ) + + return } oldMask := c.hostmask() diff --git a/internal/ircserver/conn.go b/internal/ircserver/conn.go index bd2adcf..8fc9224 100644 --- a/internal/ircserver/conn.go +++ b/internal/ircserver/conn.go @@ -27,7 +27,6 @@ const ( pollInterval = 100 * time.Millisecond pingInterval = 90 * time.Second pongDeadline = 30 * time.Second - maxNickLen = 32 minPasswordLen = 8 maxHashcashBits = 40 ) @@ -426,11 +425,16 @@ func (c *Conn) handlePreRegistration( return } - c.nick = msg.Params[0] - if len(c.nick) > maxNickLen { - c.nick = c.nick[:maxNickLen] + if !service.ValidNick(msg.Params[0]) { + c.sendNumeric( + irc.ErrErroneusNickname, + msg.Params[0], "Erroneous nickname", + ) + + return } + c.nick = msg.Params[0] c.gotNick = true case irc.CmdUser: if len(msg.Params) < 4 { //nolint:mnd diff --git a/internal/ircserver/server_test.go b/internal/ircserver/server_test.go index e77a996..6aba3a8 100644 --- a/internal/ircserver/server_test.go +++ b/internal/ircserver/server_test.go @@ -519,6 +519,68 @@ func TestDuplicateNick(t *testing.T) { assertContains(t, lines, " 433 ", "ERR_NICKNAMEINUSE") } +// TestNickRefused checks that NICK, before and after +// registration, answers a nick the HTTP API refuses with +// ERR_ERRONEUSNICKNAME and keeps the nick as it was, and +// that another member of the channel sees no line from it. +func TestNickRefused(t *testing.T) { + t.Parallel() + + // Each is what follows "NICK " on the wire. + refused := []string{ + ":bob PRIVMSG #inj :hi", // contains spaces + ":", // empty + strings.Repeat("a", 33), // one character too long + "bob!x", + "bob@x", + "#bob", + } + + env := newTestEnv(t) + + // Before registration: the 432 is addressed to the nick + // sent first, and USER registers with that nick. + early := env.dial(t) + early.send("NICK early") + + for _, param := range refused { + lines := early.sendAndExpect("NICK "+param, " 432 ") + assertContains(t, lines, " 432 early ", "before: "+param) + } + + welcome := early.sendAndExpect("USER early 0 * :Test", " 001 ") + assertContains(t, welcome, " 001 early ", "registered as early") + + // After registration. + bob := env.dial(t) + bob.register("bob") + bob.joinAndDrain("#inj") + + mallory := env.dial(t) + mallory.register("mallory") + mallory.joinAndDrain("#inj") + + bob.readUntil(func(l string) bool { + return strings.Contains(l, "JOIN") && + strings.Contains(l, "mallory") + }) + + for _, param := range refused { + lines := mallory.sendAndExpect("NICK "+param, " 432 ") + assertContains(t, lines, " 432 mallory ", "after: "+param) + } + + mallory.send("PRIVMSG #inj :still mallory") + + bobLines := bob.readUntil(func(l string) bool { + return strings.Contains(l, "still mallory") + }) + assertNotContains(t, bobLines, " NICK ", "bob sees no NICK") + assertContains( + t, bobLines, ":mallory!", "bob sees mallory's message", + ) +} + func TestListChannels(t *testing.T) { t.Parallel() diff --git a/internal/service/service.go b/internal/service/service.go index 66206a8..dabadd7 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -44,6 +44,18 @@ var validChannelRe = regexp.MustCompile( `^#[a-zA-Z0-9_\-]{1,63}$`, ) +// validNickRe matches the nicks both transports accept: a +// letter or _, then up to 31 letters, digits or +// _-[]\^{}|`. +var validNickRe = regexp.MustCompile( + `^[a-zA-Z_][a-zA-Z0-9_\-\[\]\\^{}|` + "`" + `]{0,31}$`, +) + +// ValidNick reports whether validNickRe matches nick. +func ValidNick(nick string) bool { + return validNickRe.MatchString(nick) +} + // Params defines the dependencies for creating a Service. type Params struct { fx.In -- 2.54.0