From c0eef92d23939501ee3195003659124cd81868fc Mon Sep 17 00:00:00 2001 From: clawbot Date: Thu, 8 Oct 2026 03:01:13 +0000 Subject: [PATCH] Share the ADMIN text and KILL reason, and test the review's gaps ADMIN's location and email lines come from service.AdminLines, and KillUser applies the default reason, so both transports send the same reply. The Disconnect test now allows a quarter of one write window, so a Disconnect that writes before returning fails it. QueryUserMode and UserhostReply are tested with a read that fails. The handler tests again cover USERHOST for a nick with no session, MODE +o on your own nick, and the status of a successful KILL. handleMode's comment says what it does. Model: opus-5-5 --- internal/handlers/api.go | 5 +++-- internal/handlers/utility.go | 12 +++++------- internal/handlers/utility_test.go | 15 +++++++++++++-- internal/ircserver/commands.go | 10 ++++++---- internal/ircserver/conn_test.go | 6 ++++-- internal/ircserver/export_test.go | 4 ++++ internal/service/service.go | 16 +++++++++++++++- internal/service/service_test.go | 20 ++++++++++++++++++++ 8 files changed, 70 insertions(+), 18 deletions(-) diff --git a/internal/handlers/api.go b/internal/handlers/api.go index 3d98775..cde58bd 100644 --- a/internal/handlers/api.go +++ b/internal/handlers/api.go @@ -1951,8 +1951,9 @@ func (hdlr *Handlers) handleQuit( http.StatusOK) } -// handleMode handles the MODE command for channels and -// users. Currently supports query-only (no mode changes). +// handleMode handles the MODE command: a target that starts +// with # is a channel, handled by handleChannelMode, and any +// other target a nick, handled by handleUserMode. func (hdlr *Handlers) handleMode( writer http.ResponseWriter, request *http.Request, diff --git a/internal/handlers/utility.go b/internal/handlers/utility.go index 0ae2242..30dcb02 100644 --- a/internal/handlers/utility.go +++ b/internal/handlers/utility.go @@ -106,22 +106,20 @@ func (hdlr *Handlers) handleAdmin( ) { ctx := request.Context() srvName := hdlr.serverName() + location1, location2, email := hdlr.svc.AdminLines(srvName) hdlr.enqueueNumeric( ctx, clientID, irc.RplAdminMe, nick, []string{srvName}, "Administrative info", ) hdlr.enqueueNumeric( - ctx, clientID, irc.RplAdminLoc1, nick, nil, - "neoirc server", + ctx, clientID, irc.RplAdminLoc1, nick, nil, location1, ) hdlr.enqueueNumeric( - ctx, clientID, irc.RplAdminLoc2, nick, nil, - "IRC over HTTP", + ctx, clientID, irc.RplAdminLoc2, nick, nil, location2, ) hdlr.enqueueNumeric( - ctx, clientID, irc.RplAdminEmail, nick, nil, - "admin@"+srvName, + ctx, clientID, irc.RplAdminEmail, nick, nil, email, ) hdlr.broker.Notify(sessionID) hdlr.respondJSON(writer, request, @@ -199,7 +197,7 @@ func (hdlr *Handlers) handleKill( return } - reason := "KILLed" + reason := "" if len(lines) > 1 { reason = lines[1] } diff --git a/internal/handlers/utility_test.go b/internal/handlers/utility_test.go index cb76075..7afa834 100644 --- a/internal/handlers/utility_test.go +++ b/internal/handlers/utility_test.go @@ -90,6 +90,16 @@ func TestUserhost(t *testing.T) { t.Errorf("expected alice and bob, got %q", body) } + msgs, lastID = sendAndPoll(tserver, token, lastID, map[string]any{ + commandKey: irc.CmdUserhost, + bodyKey: []string{"ghost"}, + }) + + body = numericBody(t, msgs, "302") + if body != "" { + t.Errorf("expected an empty reply for ghost, got %q", body) + } + msgs, _ = sendAndPoll(tserver, token, lastID, map[string]any{ commandKey: irc.CmdUserhost, }) @@ -187,8 +197,8 @@ func TestKill(t *testing.T) { commandKey: irc.CmdKill, bodyKey: []string{"victim", "go away"}, }) - if status != http.StatusOK { - t.Fatalf("expected 200, got %d: %v", status, result) + if status != http.StatusOK || result[statusKey] != "ok" { + t.Fatalf("expected 200 and status ok, got %d: %v", status, result) } msgs, _ = tserver.pollMessages(observerToken, observerLastID) @@ -283,6 +293,7 @@ func TestUserMode(t *testing.T) { {nick, "", "221", "+w"}, {strings.ToUpper(nick), "-w", "221", "+"}, {nick, "+z", "501", ""}, + {nick, "+o", "501", ""}, {other, "", "502", ""}, {other, "-w", "502", ""}, } { diff --git a/internal/ircserver/commands.go b/internal/ircserver/commands.go index 99785ae..b5b0d75 100644 --- a/internal/ircserver/commands.go +++ b/internal/ircserver/commands.go @@ -1313,12 +1313,14 @@ func (c *Conn) handleVersion() { // handleAdmin replies with the server's admin info. func (c *Conn) handleAdmin() { + location1, location2, email := c.svc.AdminLines(c.serverSfx) + c.sendNumeric( irc.RplAdminMe, c.serverSfx, "Administrative info", ) - c.sendNumeric(irc.RplAdminLoc1, "neoirc server") - c.sendNumeric(irc.RplAdminLoc2, "IRC over HTTP") - c.sendNumeric(irc.RplAdminEmail, "admin@"+c.serverSfx) + c.sendNumeric(irc.RplAdminLoc1, location1) + c.sendNumeric(irc.RplAdminLoc2, location2) + c.sendNumeric(irc.RplAdminEmail, email) } // handleInfo replies with the server's software info. @@ -1352,7 +1354,7 @@ func (c *Conn) handleKill( return } - reason := "KILLed" + reason := "" if len(msg.Params) > 1 { reason = msg.Params[1] } diff --git a/internal/ircserver/conn_test.go b/internal/ircserver/conn_test.go index 75601a1..d860d81 100644 --- a/internal/ircserver/conn_test.go +++ b/internal/ircserver/conn_test.go @@ -37,7 +37,9 @@ func newKillVictim(t *testing.T) (*ircserver.Conn, net.Conn) { // TestDisconnectDoesNotBlockOnUnresponsiveVictim checks // that Disconnect returns although the victim never reads, -// so a KILL cannot stall the operator who sent it. +// so a KILL cannot stall the operator who sent it. The +// limit is well below one KillWriteWindow, so a Disconnect +// that waits on any write to the victim fails. func TestDisconnectDoesNotBlockOnUnresponsiveVictim( t *testing.T, ) { @@ -53,7 +55,7 @@ func TestDisconnectDoesNotBlockOnUnresponsiveVictim( select { case <-returned: - case <-time.After(testTimeout): + case <-time.After(ircserver.KillWriteWindow / 4): t.Fatal("Disconnect waited for the victim to read") } } diff --git a/internal/ircserver/export_test.go b/internal/ircserver/export_test.go index 2b8e08c..4e665b2 100644 --- a/internal/ircserver/export_test.go +++ b/internal/ircserver/export_test.go @@ -56,6 +56,10 @@ func (s *Server) Listener() net.Listener { return s.listener } +// KillWriteWindow is how long Disconnect allows each write +// to a killed victim. +const KillWriteWindow = killWriteWindow + // NewTestConn returns a Conn for nick on tcpConn, without a // server, database or service behind it. func NewTestConn( diff --git a/internal/service/service.go b/internal/service/service.go index 16e2fa4..361bcd3 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -119,6 +119,15 @@ func (s *Service) InfoLines() []string { } } +// AdminLines returns the texts of the RPL_ADMINLOC1, +// RPL_ADMINLOC2 and RPL_ADMINEMAIL replies, in that order, +// that ADMIN sends on both transports. +func (s *Service) AdminLines( + serverName string, +) (string, string, string) { + return "neoirc server", "IRC over HTTP", "admin@" + serverName +} + // UserhostReply returns the RPL_USERHOST text for the first // five of the given nicks: "nick=+user@host" entries joined // by spaces, with * after the nick of an operator and - in @@ -199,7 +208,8 @@ func (s *Service) UnregisterWireConn( // KillUser carries out an operator's KILL: the target's // channel peers see it quit, its session is deleted, and -// its IRC connection, if it has one, is closed. +// its IRC connection, if it has one, is closed. An empty +// reason is given as "KILLed". func (s *Service) KillUser( ctx context.Context, sessionID int64, @@ -231,6 +241,10 @@ func (s *Service) KillUser( } } + if reason == "" { + reason = "KILLed" + } + quitReason := "Killed (" + nick + " (" + reason + "))" s.BroadcastQuit(ctx, targetSID, targetNick, quitReason) diff --git a/internal/service/service_test.go b/internal/service/service_test.go index 933235d..2cc9ef2 100644 --- a/internal/service/service_test.go +++ b/internal/service/service_test.go @@ -406,6 +406,16 @@ func TestQueryUserMode(t *testing.T) { if modes != "+ow" { t.Errorf("expected +ow, got %s", modes) } + + // A flag that cannot be read is an error, not a flag + // that is unset. + cancelled, cancel := context.WithCancel(ctx) + cancel() + + _, err = env.svc.QueryUserMode(cancelled, sid) + if !errors.Is(err, context.Canceled) { + t.Errorf("expected the read's error, got %v", err) + } } func TestUserhostReply(t *testing.T) { @@ -453,6 +463,16 @@ func TestUserhostReply(t *testing.T) { if strings.Count(reply, "bob=") != 5 { t.Errorf("want 5 entries for 6 nicks, got %q", reply) } + + // A nick that cannot be looked up is an error, not a nick + // with no session. + cancelled, cancel := context.WithCancel(ctx) + cancel() + + _, err = env.svc.UserhostReply(cancelled, []string{"alice"}, "srv") + if !errors.Is(err, context.Canceled) { + t.Errorf("want the lookup's error, got %v", err) + } } // userModes is the stored state of a session's user mode