From 1308769a2582303760194335b996395451558f3c Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 20 Oct 2026 13:48:16 +0200 Subject: [PATCH] Fix IRC listener relay of bodies and modes, and JOIN names (closes #127) The IRC listener relayed only the first line of a PRIVMSG or NOTICE body; it now sends one line per body line. A relayed MODE change now carries its key, limit or mask, each value its own parameter, also in the echo to the user who made it. CR, LF and NUL are removed from each parameter and from each whole line sent to an IRC listener connection, so no stored value can start a line of its own. The channel name check moved into service.JoinChannel, so the IRC listener's JOIN refuses the same names with 403. Model: opus-5-5 Co-authored-by: clawbot --- README.md | 29 ++--- internal/handlers/api.go | 14 --- internal/ircserver/commands.go | 9 +- internal/ircserver/conn.go | 8 +- internal/ircserver/conn_test.go | 46 ++++++++ internal/ircserver/parser.go | 9 +- internal/ircserver/relay.go | 40 +++++-- internal/ircserver/relay_test.go | 187 ++++++++++++++++++++++++++++++ internal/ircserver/server_test.go | 31 +++++ internal/service/service.go | 18 ++- 10 files changed, 345 insertions(+), 46 deletions(-) create mode 100644 internal/ircserver/relay_test.go diff --git a/README.md b/README.md index d81f1c7..1d68680 100644 --- a/README.md +++ b/README.md @@ -2667,7 +2667,9 @@ IRC_LISTEN_ADDR: "" - **CAP negotiation**: `CAP LS` gets an empty capability list and `CAP END` is ignored, for compatibility with modern clients. - **Channel prefixes**: Channels must start with `#`. `JOIN` prepends it if - omitted; other commands use the name as given. + omitted; other commands use the name as given. As over the HTTP API, `JOIN` + answers a name that is not `#` followed by 1–63 letters, digits, `_` or `-` + with ERR_NOSUCHCHANNEL (403). - **First joiner**: The first user to join a channel is automatically granted operator status (`@`). - **Channel modes**: `+i` (invite-only), `+m` (moderated), `+n` (no external), @@ -2689,21 +2691,20 @@ IRC_LISTEN_ADDR: "" Messages sent by IRC clients appear in channels visible to HTTP/JSON API clients and vice versa. The IRC listener and HTTP API share the same database, broker, and session infrastructure, so a user connected via IRC and a user connected via -the HTTP API can talk in the same channels, with these gaps today: +the HTTP API can talk in the same channels. -- An IRC listener user receives only the first line of a multi-line `body`. -- A `+k`, `+l` or `+b` change made over the HTTP API reaches IRC listener - members without its key, limit or mask. -- The IRC listener's `JOIN` accepts any channel name, while over the HTTP API a - name that is not `#` followed by 1–63 letters, digits, `_` or `-` gets - ERR_NOSUCHCHANNEL (403), so HTTP API users cannot join such a channel. -- The IRC listener does not pass on a `PRIVMSG`, `NOTICE`, `JOIN`, `PART`, - `NICK` or `QUIT` whose sender's nick matches the user's own nick ignoring - case, so an IRC listener user `alice` never sees these from `Alice`, although - nicks are case-sensitive. +Each line of a multi-line `body` reaches an IRC listener user as its own +`PRIVMSG` or `NOTICE` line. A CR, LF or NUL inside a line is removed before it +is sent, so it cannot start a line of its own. A mode change made over the HTTP +API reaches IRC listener members as an IRC `MODE` line with its value as a +separate parameter, e.g. `MODE #general +k secretpass` or +`MODE #general +o bob`. -https://git.eeqj.de/sneak/neoirc/issues/127 tracks the first three and -https://git.eeqj.de/sneak/neoirc/issues/128 the last. +One gap remains: the IRC listener does not pass on a `PRIVMSG`, `NOTICE`, +`JOIN`, `PART`, `NICK` or `QUIT` whose sender's nick matches the user's own nick +ignoring case, so an IRC listener user `alice` never sees these from `Alice`, +although nicks are case-sensitive. https://git.eeqj.de/sneak/neoirc/issues/128 +tracks it. ### Docker Usage diff --git a/internal/handlers/api.go b/internal/handlers/api.go index 917eb36..76da66f 100644 --- a/internal/handlers/api.go +++ b/internal/handlers/api.go @@ -29,10 +29,6 @@ var validNickRe = regexp.MustCompile( `^[a-zA-Z_][a-zA-Z0-9_\-\[\]\\^{}|` + "`" + `]{0,31}$`, ) -var validChannelRe = regexp.MustCompile( - `^#[a-zA-Z0-9_\-]{1,63}$`, -) - var validUsernameRe = regexp.MustCompile( `^[a-zA-Z0-9_\-\[\]\\^{}|` + "`" + `]{1,32}$`, ) @@ -1489,16 +1485,6 @@ func (hdlr *Handlers) handleJoin( channel = "#" + channel } - if !validChannelRe.MatchString(channel) { - hdlr.respondIRCError( - writer, request, clientID, sessionID, - irc.ErrNoSuchChannel, nick, []string{channel}, - "No such channel", - ) - - return - } - // Extract key from body lines (JOIN #channel key). var suppliedKey string diff --git a/internal/ircserver/commands.go b/internal/ircserver/commands.go index b5b0d75..40d83a9 100644 --- a/internal/ircserver/commands.go +++ b/internal/ircserver/commands.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "slices" "strconv" "strings" "time" @@ -683,9 +684,11 @@ func (c *Conn) applyChannelModes( if applied.Len() > 0 { modeReply := applied.String() + appliedArgs.String() - c.send(FormatMessage( - c.hostmask(), "MODE", channel, modeReply, - )) + params := slices.Concat( + []string{channel}, strings.Fields(modeReply), + ) + + c.send(FormatMessage(c.hostmask(), "MODE", params...)) c.svc.BroadcastMode( ctx, c.nick, channel, chID, modeReply, diff --git a/internal/ircserver/conn.go b/internal/ircserver/conn.go index f8024f0..bd2adcf 100644 --- a/internal/ircserver/conn.go +++ b/internal/ircserver/conn.go @@ -297,11 +297,17 @@ func (c *Conn) send(line string) { } // sendWithin writes a formatted IRC line to the connection, -// giving up after timeout. +// giving up after timeout. CR, LF and NUL are removed first, +// as FormatMessage does for each parameter, so that a line +// built without it cannot end early either. func (c *Conn) sendWithin( timeout time.Duration, line string, ) { + line = strings.ReplaceAll(line, "\r", "") + line = strings.ReplaceAll(line, "\n", "") + line = strings.ReplaceAll(line, "\x00", "") + c.writeMu.Lock() defer c.writeMu.Unlock() diff --git a/internal/ircserver/conn_test.go b/internal/ircserver/conn_test.go index d860d81..0598a63 100644 --- a/internal/ircserver/conn_test.go +++ b/internal/ircserver/conn_test.go @@ -5,6 +5,7 @@ import ( "log/slog" "net" "os" + "slices" "strings" "testing" "time" @@ -100,3 +101,48 @@ func TestDisconnectNotifiesAndClosesVictim(t *testing.T) { t.Errorf("missing ERROR line, got: %q", joined) } } + +// TestDisconnectReasonCannotStartLine checks that CR, LF and +// NUL in a KILL reason are removed from both the KILL line +// and the ERROR line, so the victim gets exactly those two +// lines. The ERROR line is not built with FormatMessage, so +// only the removal in sendWithin protects it. +func TestDisconnectReasonCannotStartLine(t *testing.T) { + t.Parallel() + + victim, clientSide := newKillVictim(t) + received := make(chan []string, 1) + + go func() { + var lines []string + + scanner := bufio.NewScanner(clientSide) + for scanner.Scan() { + lines = append(lines, scanner.Text()) + } + + received <- lines + }() + + victim.Disconnect("bye\r\nPRIVMSG victim :forged\x00") + + var lines []string + + select { + case lines = <-received: + case <-time.After(testTimeout): + t.Fatal("the victim's connection was not closed") + } + + // The test connection has no host name, hence the two + // spaces after "Closing Link:". + want := []string{ + ":" + testServerName + + " KILL victim :byePRIVMSG victim :forged", + "ERROR :Closing Link: (byePRIVMSG victim :forged)", + } + + if !slices.Equal(lines, want) { + t.Errorf("got lines %q, want %q", lines, want) + } +} diff --git a/internal/ircserver/parser.go b/internal/ircserver/parser.go index 5374bc0..17e4546 100644 --- a/internal/ircserver/parser.go +++ b/internal/ircserver/parser.go @@ -90,7 +90,10 @@ func ParseMessage(line string) *Message { } // FormatMessage formats an IRC message into wire protocol -// format (without the trailing CR-LF). +// format (without the trailing CR-LF). CR, LF and NUL are +// removed from each parameter, because on the wire they would +// end the line early; this happens before deciding whether +// the last parameter needs a leading ':'. func FormatMessage( prefix, command string, params ...string, @@ -106,6 +109,10 @@ func FormatMessage( buf.WriteString(command) for i, param := range params { + param = strings.ReplaceAll(param, "\r", "") + param = strings.ReplaceAll(param, "\n", "") + param = strings.ReplaceAll(param, "\x00", "") + buf.WriteByte(' ') isLast := i == len(params)-1 diff --git a/internal/ircserver/relay.go b/internal/ircserver/relay.go index 60ccb74..844e705 100644 --- a/internal/ircserver/relay.go +++ b/internal/ircserver/relay.go @@ -3,6 +3,7 @@ package ircserver import ( "context" "encoding/json" + "slices" "strings" "time" @@ -105,7 +106,7 @@ func (c *Conn) deliverIRCMessage( case isNumeric(command): c.deliverNumeric(msg, text) case command == irc.CmdPrivmsg || command == irc.CmdNotice: - c.deliverTextMessage(msg, command, text) + c.deliverTextMessage(msg, command, bodyLines) case command == irc.CmdJoin: c.deliverJoin(msg) case command == irc.CmdPart: @@ -123,7 +124,7 @@ func (c *Conn) deliverIRCMessage( case command == irc.CmdWallops: c.deliverWallops(msg, text) case command == irc.CmdMode: - c.deliverMode(msg, text) + c.deliverMode(msg, bodyLines) case command == irc.CmdPing: // Server-originated PING — reply with PONG. c.sendFromServer("PING", c.serverSfx) @@ -179,10 +180,12 @@ func (c *Conn) deliverNumeric( c.send(FormatMessage(from, msg.Command, allParams...)) } -// deliverTextMessage sends PRIVMSG or NOTICE. +// deliverTextMessage sends PRIVMSG or NOTICE, one line for +// each line of the body. func (c *Conn) deliverTextMessage( msg *db.IRCMessage, - command, text string, + command string, + bodyLines []string, ) { from := msg.From target := msg.To @@ -201,7 +204,9 @@ func (c *Conn) deliverTextMessage( prefix = from + "!" + from + "@*" } - c.send(FormatMessage(prefix, command, target, text)) + for _, line := range bodyLines { + c.send(FormatMessage(prefix, command, target, line)) + } } // deliverJoin sends a JOIN notification. @@ -351,15 +356,26 @@ func (c *Conn) deliverWallops( )) } -// deliverMode sends a MODE change notification. +// deliverMode sends a MODE change notification. The first +// body line is the change, possibly followed by its values +// separated by spaces ("+o bob"); any further lines are +// values ("+k", "secret"). Each goes out as its own +// parameter, as IRC sends them. func (c *Conn) deliverMode( msg *db.IRCMessage, - text string, + bodyLines []string, ) { - prefix := msg.From + "!" + msg.From + "@*" - target := msg.To - - if text != "" { - c.send(FormatMessage(prefix, "MODE", target, text)) + if len(bodyLines) == 0 || bodyLines[0] == "" { + return } + + prefix := msg.From + "!" + msg.From + "@*" + + params := slices.Concat( + []string{msg.To}, + strings.Fields(bodyLines[0]), + bodyLines[1:], + ) + + c.send(FormatMessage(prefix, "MODE", params...)) } diff --git a/internal/ircserver/relay_test.go b/internal/ircserver/relay_test.go new file mode 100644 index 0000000..579d422 --- /dev/null +++ b/internal/ircserver/relay_test.go @@ -0,0 +1,187 @@ +package ircserver_test + +import ( + "encoding/json" + "slices" + "strings" + "testing" +) + +// queueMessage queues a message for the user with nick +// recipient the way the HTTP API sends one: stored once, then +// queued to the recipient's session. +func (env *testEnv) queueMessage( + t *testing.T, + recipient, command, from, to string, + body []string, +) { + t.Helper() + + ctx := t.Context() + + bodyJSON, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal body: %v", err) + } + + msgID, _, err := env.database.InsertMessage( + ctx, command, from, to, nil, bodyJSON, nil, + ) + if err != nil { + t.Fatalf("insert message: %v", err) + } + + sessionID, err := env.database.GetSessionByNick(ctx, recipient) + if err != nil { + t.Fatalf("get session of %s: %v", recipient, err) + } + + err = env.database.EnqueueToSession(ctx, sessionID, msgID) + if err != nil { + t.Fatalf("enqueue: %v", err) + } + + env.brk.Notify(sessionID) +} + +// TestRelayMultiLineBody checks that each line of a PRIVMSG +// or NOTICE body sent over the HTTP API reaches an IRC +// listener user as its own line. +func TestRelayMultiLineBody(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + alice := env.dial(t) + alice.register("alice") + + for _, command := range []string{"PRIVMSG", "NOTICE"} { + env.queueMessage( + t, "alice", command, "carol", "alice", + []string{"first line", "second line"}, + ) + + lines := alice.readUntil(func(l string) bool { + return strings.Contains(l, "second line") + }) + + want := []string{ + ":carol!carol@* " + command + " alice :first line", + ":carol!carol@* " + command + " alice :second line", + } + + got := lines[max(0, len(lines)-len(want)):] + if !slices.Equal(got, want) { + t.Errorf("%s: got %q, want %q", command, got, want) + } + } +} + +// TestRelayModeParameters checks that a MODE change reaches +// IRC listener members with its values as separate +// parameters, both for the bodies the HTTP API stores and +// for a change made on the IRC listener. +func TestRelayModeParameters(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + alice := env.dial(t) + alice.register("alice") + + for _, step := range []struct { + body []string + want string + }{ + {[]string{"+k", "secret"}, "+k secret"}, + {[]string{"-k", "*"}, "-k *"}, + {[]string{"+l", "50"}, "+l 50"}, + {[]string{"-l"}, "-l"}, + {[]string{"+b", "*!*@*.example.com"}, "+b *!*@*.example.com"}, + {[]string{"-b", "*!*@*.example.com"}, "-b *!*@*.example.com"}, + {[]string{"+o bob"}, "+o bob"}, + {[]string{"+m"}, "+m"}, + } { + env.queueMessage(t, "alice", "MODE", "op", "#modes", step.body) + + lines := alice.readUntil(func(l string) bool { + return strings.Contains(l, " MODE ") + }) + + got := lines[len(lines)-1] + want := ":op!op@* MODE #modes " + step.want + + if got != want { + t.Errorf("body %q: got %q, want %q", step.body, got, want) + } + } + + bob := env.dial(t) + bob.register("bob") + + alice.joinAndDrain("#modes") + bob.joinAndDrain("#modes") + + alice.send("MODE #modes +v bob") + + const change = " MODE #modes +v bob" + + for name, client := range map[string]*testClient{ + "alice": alice, "bob": bob, + } { + lines := client.readUntil(func(l string) bool { + return strings.Contains(l, " MODE ") + }) + + got := lines[len(lines)-1] + if !strings.HasSuffix(got, change) { + t.Errorf("%s: got %q, want it to end in %q", name, got, change) + } + } +} + +// TestRelayLineBreaks checks that a CR, LF or NUL in a stored +// body line, key or mask never reaches an IRC listener user +// as a line break, so no extra line arrives, and that a body +// line keeps its parameter and leading ':' once they are +// removed. +func TestRelayLineBreaks(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + bob := env.dial(t) + bob.register("bob") + + env.queueMessage(t, "bob", "PRIVMSG", "carol", "bob", []string{ + "one\r\n:admin!a@b PRIVMSG bob :hi", + "two\n:admin!a@b NOTICE bob :hi\x00", + }) + env.queueMessage(t, "bob", "MODE", "op", "#breaks", + []string{"+k", "key\r\nERROR :Closing link"}) + env.queueMessage(t, "bob", "MODE", "op", "#breaks", + []string{"+b", "*!*@*\r\n:admin!a@b KICK #breaks bob"}) + env.queueMessage(t, "bob", "PRIVMSG", "carol", "bob", + []string{"\r\n", "\x00:hi"}) + env.queueMessage(t, "bob", "PRIVMSG", "carol", "bob", + []string{"the end"}) + + lines := bob.readUntil(func(l string) bool { + return strings.HasSuffix(l, " :the end") + }) + + want := []string{ + ":carol!carol@* PRIVMSG bob :one:admin!a@b PRIVMSG bob :hi", + ":carol!carol@* PRIVMSG bob :two:admin!a@b NOTICE bob :hi", + ":op!op@* MODE #breaks +k :keyERROR :Closing link", + ":op!op@* MODE #breaks +b :*!*@*:admin!a@b KICK #breaks bob", + ":carol!carol@* PRIVMSG bob :", + ":carol!carol@* PRIVMSG bob ::hi", + ":carol!carol@* PRIVMSG bob :the end", + } + + got := lines[max(0, len(lines)-len(want)):] + if !slices.Equal(got, want) { + t.Errorf("got %q, want %q", got, want) + } +} diff --git a/internal/ircserver/server_test.go b/internal/ircserver/server_test.go index 35ca0a6..e77a996 100644 --- a/internal/ircserver/server_test.go +++ b/internal/ircserver/server_test.go @@ -438,6 +438,37 @@ func TestJoinChannel(t *testing.T) { assertContains(t, lines, " 366 ", "RPL_ENDOFNAMES") } +// TestJoinInvalidChannelName checks that JOIN refuses a name +// that is not # followed by 1 to 63 letters, digits, _ or - +// with ERR_NOSUCHCHANNEL, and creates no such channel. +func TestJoinInvalidChannelName(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + client := env.dial(t) + + client.register("joiner") + + for _, channel := range []string{ + "#bad.name", + "#", + "#" + strings.Repeat("a", 64), + } { + lines := client.sendAndExpect("JOIN "+channel, " 403 ") + + assertContains( + t, lines, + " 403 joiner "+channel+" :No such channel", + channel, + ) + + _, err := env.database.GetChannelByName(t.Context(), channel) + if err == nil { + t.Errorf("channel %q was created", channel) + } + } +} + func TestPrivmsgBetweenClients(t *testing.T) { t.Parallel() diff --git a/internal/service/service.go b/internal/service/service.go index 361bcd3..d6edb06 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "log/slog" + "regexp" "strconv" "strings" "sync" @@ -35,6 +36,12 @@ const ( // for (RFC 2812). const maxUserhostNicks = 5 +// validChannelRe matches the channel names JOIN accepts: # +// followed by 1 to 63 letters, digits, _ or -. +var validChannelRe = regexp.MustCompile( + `^#[a-zA-Z0-9_\-]{1,63}$`, +) + // Params defines the dependencies for creating a Service. type Params struct { fx.In @@ -473,12 +480,21 @@ func (s *Service) SendDirectMessage( // JoinChannel creates or joins a channel, making the // first joiner the operator. Fans out the JOIN to all -// channel members. +// channel members. A channel name that validChannelRe +// does not match gets ERR_NOSUCHCHANNEL. func (s *Service) JoinChannel( ctx context.Context, sessionID int64, nick, channel, suppliedKey string, ) (*JoinResult, error) { + if !validChannelRe.MatchString(channel) { + return nil, &IRCError{ + irc.ErrNoSuchChannel, + []string{channel}, + msgNoSuchChannel, + } + } + chID, err := s.db.GetOrCreateChannel(ctx, channel) if err != nil { return nil, fmt.Errorf("get/create channel: %w", err)