fix: KILL actually disconnects the victim; unify HTTP/IRC divergences
All checks were successful
check / check (push) Successful in 1m12s

Addresses the 2026-08-10 FAIL review (findings 1-7, 9, 10).

KILL never terminated the victim's connection on either transport: both
paths called BroadcastQuit, which deletes the session row and tells the
victim's peers it quit, but leaves the victim holding a socket that looks
alive and silently delivers nothing while its nick is freed for reuse.
Service now owns a session-ID keyed registry of live wire connections that
ircserver populates at registration, and both KILL paths go through the new
Service.KillSession, which broadcasts the QUIT and then sends the victim a
KILL and ERROR :Closing Link before closing its socket. The victim's relay
goroutine is cancelled and its cleanup no longer re-broadcasts a QUIT for
an already-deleted session. TestIntegrationKill now asserts the victim
reads to EOF and is gone from NAMES and WHO, not just that an observer saw
the QUIT relay.

HTTP MODE <othernick> with no body answered with the requester's own modes,
because the target check sat inside the mode-change branch. The check is
hoisted above the query/change split, and both transports now compare nicks
with EqualFold since IRC nicks are case-insensitive.

Service.QueryUserMode returned "+" for a database failure, making an
unreadable mode indistinguishable from an unset one; it now returns an
error, and both callers surface it. db.GetUserhostInfo likewise treated
every scan error as "nick not found"; only sql.ErrNoRows is skipped now.

The four new unsynchronized c.nick reads this branch introduced are read
through currentNick() under c.mu, and c.closed is now guarded everywhere
because KILL writes it from another client's goroutine. Conn.send takes a
write mutex, as a connection is now written to by three goroutines.

server.Server.Run was left with no in-tree callers when its body was
inlined into the fx OnStart hook; it is deleted rather than left to drift.

INFO and VERSION had two implementations that had already diverged: the
version string is now Service.ServerVersion and the INFO body is
Service.InfoLines, used verbatim by both transports. The ctx parameters on
handleVersion/handleAdmin/handleInfo/handleTime existed only to be
discarded and are gone.
This commit is contained in:
clawbot
2026-09-03 15:17:43 +00:00
parent f24e33a310
commit c20ad88dfe
13 changed files with 600 additions and 158 deletions

View File

@@ -10,11 +10,14 @@ import (
"log/slog"
"strconv"
"strings"
"sync"
"time"
"go.uber.org/fx"
"sneak.berlin/go/neoirc/internal/broker"
"sneak.berlin/go/neoirc/internal/config"
"sneak.berlin/go/neoirc/internal/db"
"sneak.berlin/go/neoirc/internal/globals"
"sneak.berlin/go/neoirc/internal/logger"
"sneak.berlin/go/neoirc/pkg/irc"
)
@@ -27,23 +30,41 @@ type Params struct {
Config *config.Config
Database *db.Database
Broker *broker.Broker
Globals *globals.Globals
}
// WireConn is a live client connection that a transport
// registers with the service so that commands such as KILL
// can reach it. The IRC wire server registers one per
// registered connection; the HTTP transport has no
// long-lived socket and registers nothing.
type WireConn interface {
// Disconnect terminates the connection, telling the
// client why before closing the socket.
Disconnect(reason string)
}
// Service provides shared business logic for IRC commands.
type Service struct {
db *db.Database
broker *broker.Broker
config *config.Config
log *slog.Logger
db *db.Database
broker *broker.Broker
config *config.Config
globals *globals.Globals
log *slog.Logger
wireMu sync.Mutex
wireConns map[int64]WireConn
}
// New creates a new Service.
func New(params Params) *Service {
return &Service{
db: params.Database,
broker: params.Broker,
config: params.Config,
log: params.Logger.Get(),
return &Service{ //nolint:exhaustruct // mutex zero value
db: params.Database,
broker: params.Broker,
config: params.Config,
globals: params.Globals,
log: params.Logger.Get(),
wireConns: make(map[int64]WireConn),
}
}
@@ -53,13 +74,105 @@ func NewTestService(
database *db.Database,
brk *broker.Broker,
cfg *config.Config,
globs *globals.Globals,
log *slog.Logger,
) *Service {
return &Service{
db: database,
broker: brk,
config: cfg,
log: log,
return &Service{ //nolint:exhaustruct // mutex zero value
db: database,
broker: brk,
config: cfg,
globals: globs,
log: log,
wireConns: make(map[int64]WireConn),
}
}
// ServerVersion returns the canonical server version string
// used by every transport, e.g. "neoirc-1.2.3". Both the
// IRC wire protocol and the HTTP API must report the same
// string, so this is the only place it is built.
func (s *Service) ServerVersion() string {
name := "neoirc"
ver := "dev"
if s.globals != nil {
if s.globals.Appname != "" {
name = s.globals.Appname
}
if s.globals.Version != "" {
ver = s.globals.Version
}
}
return name + "-" + ver
}
// InfoLines returns the RPL_INFO body. Both transports
// send exactly these lines so that INFO does not diverge
// between the wire protocol and the HTTP API.
func (s *Service) InfoLines() []string {
started := "unknown"
if s.globals != nil && !s.globals.StartTime.IsZero() {
started = s.globals.StartTime.Format(time.RFC1123)
}
return []string{
"neoirc — IRC semantics over HTTP",
"Version: " + s.ServerVersion(),
"Written in Go",
"Started: " + started,
}
}
// RegisterWireConn associates a live wire connection with
// its session ID so that KillSession can reach it.
func (s *Service) RegisterWireConn(
sessionID int64,
conn WireConn,
) {
s.wireMu.Lock()
defer s.wireMu.Unlock()
s.wireConns[sessionID] = conn
}
// UnregisterWireConn removes the association created by
// RegisterWireConn. It is a no-op if the session has
// already been rebound to a different connection.
func (s *Service) UnregisterWireConn(
sessionID int64,
conn WireConn,
) {
s.wireMu.Lock()
defer s.wireMu.Unlock()
if s.wireConns[sessionID] == conn {
delete(s.wireConns, sessionID)
}
}
// KillSession forcibly removes a user from the server: the
// victim's channel peers are told via QUIT, the victim's
// session is deleted, and any live wire connection it holds
// is disconnected. Both the IRC KILL command and the HTTP
// KILL endpoint route through here so the two transports
// cannot diverge.
func (s *Service) KillSession(
ctx context.Context,
sessionID int64,
nick, reason string,
) {
s.BroadcastQuit(ctx, sessionID, nick, reason)
// A session with no registered wire connection (an
// HTTP-only client) has nothing left to disconnect.
s.wireMu.Lock()
conn := s.wireConns[sessionID]
s.wireMu.Unlock()
if conn != nil {
conn.Disconnect(reason)
}
}
@@ -792,26 +905,42 @@ func (s *Service) QueryChannelMode(
}
// QueryUserMode returns the current user mode string for
// the given session (e.g. "+ow", "+w", "+").
// the given session (e.g. "+ow", "+w", "+"). A database
// failure is returned rather than being reported as an
// unset flag: an unreadable mode is not the same as an
// absent one, and reporting "+" for either would tell the
// user they are de-opered when the truth is unknown.
func (s *Service) QueryUserMode(
ctx context.Context,
sessionID int64,
) string {
) (string, error) {
modes := "+"
isOper, err := s.db.IsSessionOper(ctx, sessionID)
if err == nil && isOper {
if err != nil {
return "", fmt.Errorf(
"query oper flag: %w", err,
)
}
if isOper {
modes += "o"
}
isWallops, err := s.db.IsSessionWallops(
ctx, sessionID,
)
if err == nil && isWallops {
if err != nil {
return "", fmt.Errorf(
"query wallops flag: %w", err,
)
}
if isWallops {
modes += "w"
}
return modes
return modes, nil
}
// userModeOp is a single parsed user-mode change collected
@@ -847,7 +976,7 @@ func (s *Service) ApplyUserMode(
}
}
return s.QueryUserMode(ctx, sessionID), nil
return s.QueryUserMode(ctx, sessionID)
}
// parseUserModeString validates and parses a user-mode

View File

@@ -11,6 +11,7 @@ import (
"fmt"
"os"
"testing"
"time"
"go.uber.org/fx"
"go.uber.org/fx/fxtest"
@@ -55,9 +56,10 @@ func newTestEnv(t *testing.T) *testEnv {
app := fxtest.New(t,
fx.Provide(
func() *globals.Globals {
return &globals.Globals{ //nolint:exhaustruct
Appname: "neoirc-test",
Version: "test",
return &globals.Globals{
Appname: "neoirc-test",
Version: "test",
StartTime: time.Now(),
}
},
logger.New,
@@ -371,7 +373,11 @@ func TestQueryUserMode(t *testing.T) {
sid := createSession(ctx, t, env.db, "alice")
// Fresh session has no modes.
modes := env.svc.QueryUserMode(ctx, sid)
modes, err := env.svc.QueryUserMode(ctx, sid)
if err != nil {
t.Fatalf("query user mode: %v", err)
}
if modes != "+" {
t.Errorf("expected +, got %s", modes)
}
@@ -379,7 +385,11 @@ func TestQueryUserMode(t *testing.T) {
// Set wallops.
_ = env.db.SetSessionWallops(ctx, sid, true)
modes = env.svc.QueryUserMode(ctx, sid)
modes, err = env.svc.QueryUserMode(ctx, sid)
if err != nil {
t.Fatalf("query user mode: %v", err)
}
if modes != "+w" {
t.Errorf("expected +w, got %s", modes)
}
@@ -387,7 +397,11 @@ func TestQueryUserMode(t *testing.T) {
// Set oper.
_ = env.db.SetSessionOper(ctx, sid, true)
modes = env.svc.QueryUserMode(ctx, sid)
modes, err = env.svc.QueryUserMode(ctx, sid)
if err != nil {
t.Fatalf("query user mode: %v", err)
}
if modes != "+ow" {
t.Errorf("expected +ow, got %s", modes)
}