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)