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 MODE change reached IRC
listener members without its key, limit or mask, and "+o bob" went out
as one trailing parameter; each value is now its own parameter, in the
relay and in the echo to the IRC listener user who made the change.
CR, LF and NUL are now removed from each parameter of a line sent to an
IRC listener connection, before the leading ':' is decided, and from
the whole line, so a stored body line, key or mask cannot start a line
of its own. The channel name check moved from the HTTP handler into
service.JoinChannel, so the IRC listener's JOIN refuses the same names
with 403.

Model: opus-5-5

README.md

@@ -2583,7 +2583,9 @@ is also loaded automatically via [godotenv](https://github.com/joho/godotenv),
 and Viper also reads a YAML file `neoirc.yaml` from `/etc/neoirc/` or
 `$HOME/.config/neoirc/` if one exists, with the same names as keys; an
 environment variable wins over the file. An environment variable set to the
-empty string counts as unset, so the file's value or the default applies.
+empty string counts as unset, so the file's value or the default applies; the
+one exception is `IRC_LISTEN_ADDR`, where the empty string disables the IRC
+listener.
 
 | Variable               | Type   | Default                                                                                | Description                                                                                                                                                                                                                                                                             |
 | ---------------------- | ------ | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
@@ -2605,7 +2607,7 @@ empty string counts as unset, so the file's value or the default applies.
 | `NEOIRC_OPER_PASSWORD` | string | `""`                                                                                   | Server operator (o-line) password. Both name and password must be set to enable OPER.                                                                                                                                                                                                   |
 | `LOGIN_RATE_LIMIT`     | float  | `1`                                                                                    | Allowed login attempts per second per IP address.                                                                                                                                                                                                                                       |
 | `LOGIN_RATE_BURST`     | int    | `5`                                                                                    | Maximum burst of login attempts per IP before rate limiting kicks in.                                                                                                                                                                                                                   |
-| `IRC_LISTEN_ADDR`      | string | `:6667`                                                                                | TCP address for the traditional IRC protocol listener. Only an empty value in `neoirc.yaml` disables it; an empty environment variable does not.                                                                                                                                        |
+| `IRC_LISTEN_ADDR`      | string | `:6667`                                                                                | TCP address for the traditional IRC protocol listener. Set it to the empty string, in the environment or in `neoirc.yaml`, to disable the listener.                                                                                                                                     |
 | `MAINTENANCE_MODE`     | bool   | `false`                                                                                | Maintenance mode flag (reserved)                                                                                                                                                                                                                                                        |
 
 ### Example `.env` file
@@ -2632,9 +2634,14 @@ and others.
 ### Configuration
 
 The IRC listener is **enabled by default** on `:6667`. To disable it, set
-`IRC_LISTEN_ADDR` to an empty string in `neoirc.yaml` (see
-[Configuration](#configuration)); an empty environment variable counts as unset
-and leaves it on `:6667`:
+`IRC_LISTEN_ADDR` to an empty string (see [Configuration](#configuration)),
+either in the environment:
+
+```bash
+IRC_LISTEN_ADDR=
+```
+
+or in `neoirc.yaml`:
 
 ```yaml
 IRC_LISTEN_ADDR: ""

internal/config/config.go

@@ -4,6 +4,7 @@ package config
 import (
 	"errors"
 	"log/slog"
+	"os"
 
 	"github.com/spf13/viper"
 	"go.uber.org/fx"
@@ -129,6 +130,13 @@ func New(
 		params:             &params,
 	}
 
+	// Viper counts an empty environment variable as unset, but an
+	// empty IRC_LISTEN_ADDR must turn the IRC listener off.
+	ircListenAddr, set := os.LookupEnv("IRC_LISTEN_ADDR")
+	if set {
+		cfg.IRCListenAddr = ircListenAddr
+	}
+
 	if cfg.Debug {
 		params.Logger.EnableDebugLogging()
 		cfg.log = params.Logger.Get()

internal/config/config_test.go

@@ -0,0 +1,61 @@
+package config_test
+
+import (
+	"os"
+	"testing"
+	"time"
+
+	"sneak.berlin/go/neoirc/internal/config"
+	"sneak.berlin/go/neoirc/internal/globals"
+	"sneak.berlin/go/neoirc/internal/logger"
+)
+
+func newConfig(t *testing.T) *config.Config {
+	t.Helper()
+
+	globs := &globals.Globals{
+		Appname:   "neoirc-test",
+		Version:   "test",
+		StartTime: time.Now(),
+	}
+
+	log, err := logger.New(nil, logger.Params{Globals: globs})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cfg, err := config.New(
+		nil, config.Params{Globals: globs, Logger: log},
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return cfg
+}
+
+func TestIRCListenAddrUnsetKeepsDefault(t *testing.T) {
+	// t.Setenv puts back whatever value was there after the test.
+	t.Setenv("IRC_LISTEN_ADDR", "")
+
+	err := os.Unsetenv("IRC_LISTEN_ADDR")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	const want = ":6667"
+
+	cfg := newConfig(t)
+	if cfg.IRCListenAddr != want {
+		t.Errorf("IRCListenAddr = %q, want %q", cfg.IRCListenAddr, want)
+	}
+}
+
+func TestIRCListenAddrEmptyTurnsListenerOff(t *testing.T) {
+	t.Setenv("IRC_LISTEN_ADDR", "")
+
+	cfg := newConfig(t)
+	if cfg.IRCListenAddr != "" {
+		t.Errorf("IRCListenAddr = %q, want empty", cfg.IRCListenAddr)
+	}
+}

internal/ircserver/conn.go

@@ -297,11 +297,9 @@ func (c *Conn) send(line string) {
 }
 
 // sendWithin writes a formatted IRC line to the connection,
-// giving up after timeout. CR, LF and NUL are removed first:
-// stored text can hold them, and on the wire they would end
-// the line early. They are removed, not replaced with a
-// space, because the line is already formatted and a space
-// would split a parameter in two.
+// 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,

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

internal/ircserver/relay_test.go

@@ -142,7 +142,9 @@ func TestRelayModeParameters(t *testing.T) {
 
 // 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.
+// 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()
 
@@ -159,6 +161,8 @@ func TestRelayLineBreaks(t *testing.T) {
 		[]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"})
 
@@ -171,6 +175,8 @@ func TestRelayLineBreaks(t *testing.T) {
 		":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",
 	}