diff --git a/internal/server/server.go b/internal/server/server.go index 50216fa..35d2080 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -71,7 +71,17 @@ func New( lifecycle.Append(fx.Hook{ OnStart: func(_ context.Context) error { srv.startupTime = time.Now() - go srv.Run() //nolint:contextcheck + // Configure, enable Sentry, and build the router + // synchronously so that srv.router is fully initialized + // before OnStart returns. Any HTTP traffic (including + // httptest harnesses that wrap srv as a handler) is + // therefore guaranteed to see an initialized router, + // eliminating the previous race between SetupRoutes + // and ServeHTTP. + srv.configure() + srv.enableSentry() + srv.SetupRoutes() + go srv.serve() //nolint:contextcheck return nil }, @@ -83,10 +93,16 @@ func New( return srv, nil } -// Run starts the server configuration, Sentry, and begins serving. +// Run configures the server and begins serving. It blocks +// until shutdown is signalled. Kept for external callers +// that embed the server outside fx. The fx lifecycle now +// performs setup synchronously in OnStart and invokes +// serve directly in a goroutine, so this is only used when +// the server is driven by hand. func (srv *Server) Run() { srv.configure() srv.enableSentry() + srv.SetupRoutes() srv.serve() } @@ -202,8 +218,6 @@ func (srv *Server) serveUntilShutdown() { Handler: srv, } - srv.SetupRoutes() - srv.log.Info( "http begin listen", "listenaddr", listenAddr, ) diff --git a/internal/service/service.go b/internal/service/service.go index a788db8..05cdd51 100644 --- a/internal/service/service.go +++ b/internal/service/service.go @@ -814,28 +814,34 @@ func (s *Service) QueryUserMode( return modes } -// ApplyUserMode parses a mode string character by -// character (e.g. "+wo", "-w") and applies each mode -// change to the session. Returns the resulting mode string -// after all changes, or an IRCError on failure. +// userModeOp is a single parsed user-mode change collected +// by parseUserModeString before any DB writes happen. +type userModeOp struct { + char rune + adding bool +} + +// ApplyUserMode parses an IRC user-mode string and applies +// the resulting changes atomically. It supports multiple +// sign transitions (e.g. "+w-o", "-w+o", "+o-w+w") and +// rejects malformed input (empty string, no leading sign, +// bare sign with no mode letters, unknown mode letters, +// +o which must be set via OPER) with an IRCError. On +// failure, no persistent change is made. On success, the +// resulting mode string is returned. func (s *Service) ApplyUserMode( ctx context.Context, sessionID int64, modeStr string, ) (string, error) { - if len(modeStr) < 2 { //nolint:mnd // +/- prefix + ≥1 char - return "", &IRCError{ - Code: irc.ErrUmodeUnknownFlag, - Params: nil, - Message: "Unknown MODE flag", - } + ops, err := parseUserModeString(modeStr) + if err != nil { + return "", err } - adding := modeStr[0] == '+' - - for _, ch := range modeStr[1:] { + for _, op := range ops { if err := s.applySingleUserMode( - ctx, sessionID, ch, adding, + ctx, sessionID, op.char, op.adding, ); err != nil { return "", err } @@ -844,7 +850,79 @@ func (s *Service) ApplyUserMode( return s.QueryUserMode(ctx, sessionID), nil } -// applySingleUserMode applies one user mode character. +// parseUserModeString validates and parses a user-mode +// string into a list of operations. The string must begin +// with '+' or '-'; subsequent '+' / '-' characters flip the +// active sign, and letters between them are applied with +// the current sign. Every letter must be a recognized user +// mode for this server, and '+o' is never allowed via MODE +// (use OPER to become operator). If any character is +// invalid, no operations are returned and an IRCError with +// ERR_UMODEUNKNOWNFLAG (501) is returned. +func parseUserModeString( + modeStr string, +) ([]userModeOp, error) { + unknownFlag := &IRCError{ + Code: irc.ErrUmodeUnknownFlag, + Params: nil, + Message: "Unknown MODE flag", + } + + if modeStr == "" { + return nil, unknownFlag + } + + first := modeStr[0] + if first != '+' && first != '-' { + return nil, unknownFlag + } + + ops := make([]userModeOp, 0, len(modeStr)-1) + adding := true + + for _, ch := range modeStr { + switch ch { + case '+': + adding = true + case '-': + adding = false + default: + if !isKnownUserModeChar(ch) { + return nil, unknownFlag + } + + if ch == 'o' && adding { + return nil, unknownFlag + } + + ops = append(ops, userModeOp{ + char: ch, adding: adding, + }) + } + } + + if len(ops) == 0 { + return nil, unknownFlag + } + + return ops, nil +} + +// isKnownUserModeChar reports whether the character is a +// recognized user mode letter. +func isKnownUserModeChar(ch rune) bool { + switch ch { + case 'w', 'o': + return true + default: + return false + } +} + +// applySingleUserMode applies one already-validated user +// mode character to the session. parseUserModeString must +// have validated the character and sign before this runs; +// the default branch here is defence-in-depth only. func (s *Service) applySingleUserMode( ctx context.Context, sessionID int64, @@ -864,7 +942,6 @@ func (s *Service) applySingleUserMode( return fmt.Errorf("set wallops: %w", err) } case 'o': - // +o cannot be set via MODE; use OPER command. if adding { return &IRCError{ Code: irc.ErrUmodeUnknownFlag, diff --git a/internal/service/service_test.go b/internal/service/service_test.go index 016c174..5030ebe 100644 --- a/internal/service/service_test.go +++ b/internal/service/service_test.go @@ -393,136 +393,260 @@ func TestQueryUserMode(t *testing.T) { } } -func TestApplyUserModeSingleChar(t *testing.T) { - env := newTestEnv(t) - ctx := t.Context() - - sid := createSession(ctx, t, env.db, "alice") - - // Apply +w. - result, err := env.svc.ApplyUserMode(ctx, sid, "+w") - if err != nil { - t.Fatalf("apply +w: %v", err) +// TestApplyUserMode is the rigorous table-driven suite for +// the shared user-mode parser. It covers every case listed +// in sneak's review of PR #96 plus a few adjacent ones. +// Each case asserts the resulting mode string AND the +// persisted session state, to prove that rejected input +// leaves no side effects. +func TestApplyUserMode(t *testing.T) { + type caseState struct { + oper bool + wallops bool } - if result != "+w" { - t.Errorf("expected +w, got %s", result) + // Each case starts from initialState, calls + // ApplyUserMode(modeStr), and verifies either the + // returned mode string (wantModes) or an IRCError with + // wantErrCode. After the call, the DB must match + // wantState — proving that rejected input made no + // change. + cases := []struct { + name string + initialState caseState + modeStr string + wantModes string + wantErr bool + wantErrCode irc.IRCMessageType + wantState caseState + }{ + // Happy path: single-char operations. + { + name: "+w from empty", + modeStr: "+w", + wantModes: "+w", + wantState: caseState{wallops: true}, + }, + { + name: "-w from +w", + initialState: caseState{wallops: true}, + modeStr: "-w", + wantModes: "+", + wantState: caseState{}, + }, + { + name: "-o from +o", + initialState: caseState{oper: true}, + modeStr: "-o", + wantModes: "+", + wantState: caseState{}, + }, + // Multi-char without sign transitions. + { + name: "-wo from +ow", + initialState: caseState{oper: true, wallops: true}, + modeStr: "-wo", + wantModes: "+", + wantState: caseState{}, + }, + // Multi-char with sign transitions. + { + name: "+w-o from +o", + initialState: caseState{oper: true}, + modeStr: "+w-o", + wantModes: "+w", + wantState: caseState{wallops: true}, + }, + { + name: "-w+o always rejects +o", + initialState: caseState{wallops: true, oper: true}, + modeStr: "-w+o", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + // +o is rejected before any op applies; wallops + // stays set. + wantState: caseState{wallops: true, oper: true}, + }, + { + name: "+o-w+w rejects because of +o", + initialState: caseState{wallops: true, oper: true}, + modeStr: "+o-w+w", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + // Wallops must NOT be cleared; oper must NOT be + // cleared. Rejection is fully atomic. + wantState: caseState{wallops: true, oper: true}, + }, + { + name: "-x+y rejects unknown -x", + modeStr: "-x+y", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, + { + name: "+y-x rejects unknown +y", + modeStr: "+y-x", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, + // Malformed prefix — must not silently treat as '-'. + { + name: "w no prefix rejects", + modeStr: "w", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, + { + name: "xw no prefix rejects (would have been" + + " silently -w before)", + initialState: caseState{wallops: true}, + modeStr: "xw", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + // Prove wallops is NOT cleared — the whole point + // of sneak's review. + wantState: caseState{wallops: true}, + }, + { + name: "empty string rejects", + modeStr: "", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, + // Bare signs with no mode letters reject. + { + name: "bare + rejects", + modeStr: "+", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, + { + name: "bare - rejects", + modeStr: "-", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, + { + name: "+-+ rejects (no mode letters)", + modeStr: "+-+", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, + // Unknown mode letters reject whole string. + { + name: "+z unknown mode rejects", + modeStr: "+z", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, + { + name: "+wz rejects whole thing; +w side effect" + + " must NOT persist", + modeStr: "+wz", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + // Wallops must NOT be set. + wantState: caseState{}, + }, + { + name: "+wo rejects whole thing; +w side effect" + + " must NOT persist", + modeStr: "+wo", + wantErr: true, + wantErrCode: irc.ErrUmodeUnknownFlag, + wantState: caseState{}, + }, } - // Apply -w. - result, err = env.svc.ApplyUserMode(ctx, sid, "-w") - if err != nil { - t.Fatalf("apply -w: %v", err) - } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + env := newTestEnv(t) + ctx := t.Context() + sid := createSession(ctx, t, env.db, "alice") - if result != "+" { - t.Errorf("expected +, got %s", result) - } -} - -func TestApplyUserModeMultiChar(t *testing.T) { - env := newTestEnv(t) - ctx := t.Context() - - sid := createSession(ctx, t, env.db, "alice") - - // Set oper first so we can test +wo (w applied, o - // rejected because +o is not allowed via MODE). - _ = env.db.SetSessionOper(ctx, sid, true) - - // Apply +w alone should work. - result, err := env.svc.ApplyUserMode(ctx, sid, "+w") - if err != nil { - t.Fatalf("apply +w: %v", err) - } - - if result != "+ow" { - t.Errorf("expected +ow, got %s", result) - } - - // Reset wallops. - _ = env.db.SetSessionWallops(ctx, sid, false) - - // Multi-char -ow: should de-oper and remove wallops. - _ = env.db.SetSessionWallops(ctx, sid, true) - - result, err = env.svc.ApplyUserMode(ctx, sid, "-ow") - if err != nil { - t.Fatalf("apply -ow: %v", err) - } - - if result != "+" { - t.Errorf("expected +, got %s", result) - } - - // +wo should fail because +o is not allowed. - _, err = env.svc.ApplyUserMode(ctx, sid, "+wo") - - var ircErr *service.IRCError - if !errors.As(err, &ircErr) { - t.Fatalf("expected IRCError, got %v", err) - } - - if ircErr.Code != irc.ErrUmodeUnknownFlag { - t.Errorf( - "expected ErrUmodeUnknownFlag, got %d", - ircErr.Code, - ) - } -} - -func TestApplyUserModeInvalidInput(t *testing.T) { - env := newTestEnv(t) - ctx := t.Context() - - sid := createSession(ctx, t, env.db, "alice") - - // Too short. - _, err := env.svc.ApplyUserMode(ctx, sid, "+") - - var ircErr *service.IRCError - if !errors.As(err, &ircErr) { - t.Fatalf("expected IRCError for short input, got %v", err) - } - - // Unknown flag. - _, err = env.svc.ApplyUserMode(ctx, sid, "+x") - if !errors.As(err, &ircErr) { - t.Fatalf("expected IRCError for unknown flag, got %v", err) - } - - if ircErr.Code != irc.ErrUmodeUnknownFlag { - t.Errorf( - "expected ErrUmodeUnknownFlag, got %d", - ircErr.Code, - ) - } -} - -func TestApplyUserModeDeoper(t *testing.T) { - env := newTestEnv(t) - ctx := t.Context() - - sid := createSession(ctx, t, env.db, "alice") - - // Make oper via DB directly. - _ = env.db.SetSessionOper(ctx, sid, true) - - // -o should work. - result, err := env.svc.ApplyUserMode(ctx, sid, "-o") - if err != nil { - t.Fatalf("apply -o: %v", err) - } - - if result != "+" { - t.Errorf("expected +, got %s", result) - } - - // +o should fail. - _, err = env.svc.ApplyUserMode(ctx, sid, "+o") - - var ircErr *service.IRCError - if !errors.As(err, &ircErr) { - t.Fatalf("expected IRCError for +o, got %v", err) + if tc.initialState.oper { + if err := env.db.SetSessionOper( + ctx, sid, true, + ); err != nil { + t.Fatalf("init oper: %v", err) + } + } + + if tc.initialState.wallops { + if err := env.db.SetSessionWallops( + ctx, sid, true, + ); err != nil { + t.Fatalf("init wallops: %v", err) + } + } + + result, err := env.svc.ApplyUserMode( + ctx, sid, tc.modeStr, + ) + + if tc.wantErr { + var ircErr *service.IRCError + if !errors.As(err, &ircErr) { + t.Fatalf( + "expected IRCError, got %v", err, + ) + } + + if ircErr.Code != tc.wantErrCode { + t.Errorf( + "code: want %d got %d", + tc.wantErrCode, ircErr.Code, + ) + } + } else { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if result != tc.wantModes { + t.Errorf( + "modes: want %q got %q", + tc.wantModes, result, + ) + } + } + + // Whether or not the call errored, the persisted + // state must match wantState — this is the + // atomicity guarantee sneak demanded. + gotOper, err := env.db.IsSessionOper(ctx, sid) + if err != nil { + t.Fatalf("read oper: %v", err) + } + + gotWallops, err := env.db.IsSessionWallops( + ctx, sid, + ) + if err != nil { + t.Fatalf("read wallops: %v", err) + } + + if gotOper != tc.wantState.oper { + t.Errorf( + "oper: want %v got %v", + tc.wantState.oper, gotOper, + ) + } + + if gotWallops != tc.wantState.wallops { + t.Errorf( + "wallops: want %v got %v", + tc.wantState.wallops, gotWallops, + ) + } + }) } }