From 070870be2b00759fa8caa95c81df88a5ee1dc0d3 Mon Sep 17 00:00:00 2001 From: ak Date: Fri, 6 Nov 2026 17:44:23 -0800 Subject: [PATCH 1/3] Harden account isolation, public fetchers, and IRC resource bounds --- bot/bot.go | 64 ++++++++++++++++++++-- bot/security_test.go | 89 ++++++++++++++++++++++++++++++ docs/configuration.md | 9 ++- docs/plugins.md | 21 +++++-- plugins/ask.go | 10 +++- plugins/note.go | 28 +++++++--- plugins/note_security_test.go | 100 ++++++++++++++++++++++++++++++++++ plugins/note_test.go | 10 +++- plugins/paste.go | 5 +- plugins/public_http.go | 34 ++++++++++++ plugins/public_http_test.go | 95 ++++++++++++++++++++++++++++++++ plugins/status_http.go | 35 +----------- plugins/status_http_test.go | 4 +- plugins/urltitle.go | 47 +++++++++++----- 14 files changed, 475 insertions(+), 76 deletions(-) create mode 100644 bot/security_test.go create mode 100644 plugins/note_security_test.go create mode 100644 plugins/public_http.go create mode 100644 plugins/public_http_test.go diff --git a/bot/bot.go b/bot/bot.go index 915f8d1..6c10874 100644 --- a/bot/bot.go +++ b/bot/bot.go @@ -208,7 +208,13 @@ func (b *Bot) sendNow(target, text string) { c := b.client b.mu.RUnlock() if c != nil { - c.WriteMessage(&irc.Message{Command: "PRIVMSG", Params: []string{target, text}}) + if err := c.WriteMessage(&irc.Message{Command: "PRIVMSG", Params: []string{target, text}}); err != nil { + b.recordDroppedOutput() + if b.Log != nil { + b.Log.Warn("IRC message write failed") + } + return + } b.Stats.sent.Add(1) if b.networkStats != nil { b.networkStats.sent.Add(1) @@ -260,14 +266,15 @@ func (b *Bot) connect(ctx context.Context) error { } } var conn net.Conn + dialer := &net.Dialer{Timeout: 15 * time.Second, KeepAlive: 30 * time.Second} if b.Config.Server.TLS { - tlsConfig := &tls.Config{ServerName: b.Config.Server.Host, InsecureSkipVerify: !b.Config.Server.VerifyCert} + tlsConfig := &tls.Config{ServerName: b.Config.Server.Host, MinVersion: tls.VersionTLS12, InsecureSkipVerify: !b.Config.Server.VerifyCert} if b.Config.Server.ClientCert != "" { tlsConfig.Certificates = []tls.Certificate{clientCertificate} } - conn, err = tls.Dial("tcp", server, tlsConfig) + conn, err = (&tls.Dialer{NetDialer: dialer, Config: tlsConfig}).DialContext(ctx, "tcp", server) } else { - conn, err = net.Dial("tcp", server) + conn, err = dialer.DialContext(ctx, "tcp", server) } if err != nil { return err @@ -535,7 +542,29 @@ func (b *Bot) IsOwner(m Message) bool { return false } for _, owner := range b.Config.OwnerAccounts { - if strings.EqualFold(strings.TrimSpace(owner), account) { + owner = strings.TrimSpace(owner) + if network, name, qualified := strings.Cut(owner, "/"); qualified { + network, name = strings.TrimSpace(network), strings.TrimSpace(name) + if network != "" && name != "" && strings.EqualFold(network, strings.TrimSpace(b.Config.NetworkName)) && strings.EqualFold(name, account) { + return true + } + continue + } + // Bare legacy account names grant access only on the first configured + // network. A same-named account elsewhere is a different principal. + primary := true + if len(b.Config.Networks) > 1 { + first := b.Config.Networks[0] + name := strings.TrimSpace(first.Name) + if name == "" { + name = strings.TrimSpace(first.Server.Host) + } + if name == "" { + name = "network-1" + } + primary = strings.EqualFold(name, strings.TrimSpace(b.Config.NetworkName)) + } + if primary && strings.EqualFold(owner, account) { return true } } @@ -570,6 +599,11 @@ func (b *Bot) AllowCommand(m Message) bool { if exists && now.Sub(last) < cooldown { lastWarning, warned := b.lastWarnings[key] if !warned || now.Sub(lastWarning) >= warningCooldown { + pruneCommandHistory(b.lastWarnings, now, warningCooldown) + if _, known := b.lastWarnings[key]; !known && len(b.lastWarnings) >= maxCommandIdentities { + b.commandMu.Unlock() + return false + } b.lastWarnings[key] = now b.commandMu.Unlock() b.Send(m.ReplyTarget(), "command cooldown—please wait a moment") @@ -578,11 +612,31 @@ func (b *Bot) AllowCommand(m Message) bool { b.commandMu.Unlock() return false } + if !exists { + pruneCommandHistory(b.lastCommands, now, cooldown) + if len(b.lastCommands) >= maxCommandIdentities { + b.commandMu.Unlock() + return false + } + } b.lastCommands[key] = now b.commandMu.Unlock() return true } +const maxCommandIdentities = 10000 + +func pruneCommandHistory(history map[string]time.Time, now time.Time, lifetime time.Duration) { + if len(history) < maxCommandIdentities { + return + } + for key, last := range history { + if now.Sub(last) >= lifetime { + delete(history, key) + } + } +} + func commandBypassesCooldown(m Message, prefix string) bool { cmd, arg, ok := IsCommand(m, prefix) if !ok { diff --git a/bot/security_test.go b/bot/security_test.go new file mode 100644 index 0000000..d60ecce --- /dev/null +++ b/bot/security_test.go @@ -0,0 +1,89 @@ +package bot + +import ( + "context" + "errors" + "fmt" + "net" + "testing" + "time" + + "gopkg.in/irc.v3" +) + +func TestOwnerPrivilegesAreNetworkScoped(t *testing.T) { + cfg := Config{Networks: []NetworkConfig{{Name: "alpha"}, {Name: "beta"}}, OwnerAccounts: []string{"alice", "beta/bob"}} + for _, test := range []struct { + network, account string + want bool + }{ + {"alpha", "ALICE", true}, {"beta", "alice", false}, {"beta", "bob", true}, {"alpha", "bob", false}, {"beta", "*", false}, + } { + cfg.NetworkName = test.network + b := &Bot{Config: cfg} + if got := b.IsOwner(Message{Account: test.account, Nick: "alice"}); got != test.want { + t.Errorf("owner check on %s/%s = %t, want %t", test.network, test.account, got, test.want) + } + } + cfg.NetworkName = "beta" + called := false + b := &Bot{Config: cfg, reloadHandler: func(Message) { called = true }} + if b.handlePrivateReload(Message{Command: "PRIVMSG", Account: "alice", Text: "reload"}) || called { + t.Fatal("same-named account on a secondary network triggered owner reload") + } + cfg.Networks[0] = NetworkConfig{Server: ServerConfig{Host: "irc.example"}} + cfg.NetworkName = "irc.example" + if !(&Bot{Config: cfg}).IsOwner(Message{Account: "alice"}) { + t.Fatal("unnamed primary network lost its legacy owner") + } +} + +func TestCommandIdentityHistoriesAreBoundedAndExpire(t *testing.T) { + b := &Bot{lastCommands: make(map[string]time.Time), lastWarnings: make(map[string]time.Time)} + now := time.Now() + for i := 0; i < maxCommandIdentities; i++ { + b.lastCommands[fmt.Sprint(i)] = now + } + message := Message{Command: "PRIVMSG", Nick: "new-user", Text: "!status"} + b.Config.CommandPrefix = "!" + if b.AllowCommand(message) || len(b.lastCommands) > maxCommandIdentities { + t.Fatal("new identities bypassed the command-history bound") + } + for key := range b.lastCommands { + b.lastCommands[key] = now.Add(-time.Hour) + } + if !b.AllowCommand(message) || len(b.lastCommands) != 1 { + t.Fatal("expired command identities did not free capacity") + } + for i := 0; i < maxCommandIdentities; i++ { + b.lastWarnings[fmt.Sprint(i)] = now + } + if b.AllowCommand(message) || len(b.lastWarnings) > maxCommandIdentities { + t.Fatal("warning history exceeded its bound") + } +} + +func TestFailedIRCWriteCountsAsDroppedNotSent(t *testing.T) { + left, right := net.Pipe() + defer left.Close() + right.Close() + stats := NewStats() + b := &Bot{Stats: stats, client: irc.NewClient(left, irc.ClientConfig{})} + b.networkStats = stats.registerNetwork("test", 0, nil) + b.sendNow("#test", "synthetic test message") + if stats.sent.Load() != 0 || stats.dropped.Load() != 1 || b.networkStats.sent.Load() != 0 || b.networkStats.dropped.Load() != 1 { + t.Fatal("failed IRC write was counted as a successful send") + } +} + +func TestIRCConnectHonorsCancellation(t *testing.T) { + for _, useTLS := range []bool{false, true} { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + b := &Bot{Config: Config{Server: ServerConfig{Host: "example.invalid", Port: 6697, TLS: useTLS, VerifyCert: true}}} + started := time.Now() + if err := b.connect(ctx); !errors.Is(err, context.Canceled) || time.Since(started) > time.Second { + t.Fatalf("IRC connection ignored cancellation: %v", err) + } + } +} diff --git a/docs/configuration.md b/docs/configuration.md index b1e7a68..71e075e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -288,12 +288,19 @@ alone are never treated as proof of ownership: ```yaml owner_accounts: - "your-account" - - "another-owner" + - "secondary-network/another-owner" ``` GoBot records the IRCv3 `account` tag when the network provides it. There is intentionally no ownership-claim command. +Account names are local to their IRC network. With multiple networks, bare +legacy names such as `your-account` grant owner access only on the **first** +configured network. Use an explicit `network-name/account-name` entry for each +additional grant. A same-named account on another network is not an owner by +default. Single-network bare-account configurations retain their existing +behavior. Owner configuration changes require a restart. + ### Owner-only private reload An owner can reload file-backed plugin settings without disconnecting GoBot: diff --git a/docs/plugins.md b/docs/plugins.md index 1c68098..28cd7e2 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -341,15 +341,28 @@ The `note` plugin stores small, user-invoked notes by authenticated IRC account: !note clear ~~~ -`!notes` is an alias for `!note`. Notes are not announced automatically and are -stored separately for each user. A note response sent in a channel is visible -there, so use a private message when the note is sensitive. When account tags -are unavailable, GoBot falls back to the network and nickname. Notes are limited +`!notes` is an alias for `!note`. Send all note commands in a private message +to GoBot while identified to an IRC account. The network must provide account +tags; unverified nicknames cannot read or modify notes. Channel commands only +receive a private usage notice and do not read, write, or expose notes. +Records are scoped to both network and account, so a same-named account on +another network cannot claim them. Legacy unscoped/nickname records remain in +the old `notes` bucket, untouched and inaccessible to commands; new records use +`notes_v2`. An operator must verify legacy ownership before any offline recovery; +there is deliberately no automatic ownership claim or destructive migration. +Notes are limited to 50 per user by default, with 400 characters per note, and unused notes expire after 180 days. Operators can change these limits with `plugins.note.max_notes`, `plugins.note.max_note_length`, and `plugins.note.expiry_days`; set `expiry_days: 0` to disable expiry. +User-controlled HTTP fetchers (`urltitle`, paste-source fetches, Ask excerpts, +and website status checks) share a strict public-IP policy. Shared/CGNAT, +benchmark, documentation, reserved, local, and transition addresses are not +valid targets. Redirects and response headers are bounded, proxies cannot +bypass destination validation, and URL previews use at most four background +workers across networks. URL-preview logs omit query strings and paths. + ## Join greetings The optional `welcome` plugin posts a short, playful line when another user diff --git a/plugins/ask.go b/plugins/ask.go index ba6a947..35c7a4b 100644 --- a/plugins/ask.go +++ b/plugins/ask.go @@ -1979,11 +1979,13 @@ func isDuckDuckGoHost(host string) bool { } func isPublicIP(address netip.Addr) bool { - return address.IsGlobalUnicast() && !address.IsPrivate() && !address.IsLoopback() && !address.IsLinkLocalUnicast() && !address.IsLinkLocalMulticast() && !address.IsMulticast() && !address.IsUnspecified() + return publicInternetIP(address) } func newAskWebHTTPClient() *http.Client { transport := http.DefaultTransport.(*http.Transport).Clone() + transport.Proxy = nil + transport.MaxResponseHeaderBytes = 64 << 10 transport.DialContext = askPublicDialContext return &http.Client{ Timeout: 6 * time.Second, @@ -2016,11 +2018,15 @@ func askPublicDialContext(ctx context.Context, network, address string) (net.Con return nil, err } dialer := &net.Dialer{Timeout: 3 * time.Second} + // Reject mixed public/private answers before dialing any member. for _, ip := range ips { parsed, parseErr := netip.ParseAddr(ip.String()) if parseErr != nil || !isPublicIP(parsed) { - continue + return nil, fmt.Errorf("refusing non-public address") } + } + for _, ip := range ips { + parsed, _ := netip.ParseAddr(ip.String()) conn, dialErr := dialer.DialContext(ctx, network, net.JoinHostPort(parsed.String(), port)) if dialErr == nil { return conn, nil diff --git a/plugins/note.go b/plugins/note.go index 72e88d7..f6cda4d 100644 --- a/plugins/note.go +++ b/plugins/note.go @@ -16,7 +16,7 @@ import ( ) const ( - noteBucket = "notes" + noteBucket = "notes_v2" defaultMaxNotes = 50 defaultMaxNoteLength = 400 defaultNoteExpiryDays = 180 @@ -43,7 +43,7 @@ type Note struct { func (p *Note) Name() string { return "note" } func (p *Note) Commands() []string { return []string{"note", "notes"} } func (p *Note) Help() string { - return "!note add — save a note; !note — read it; !note list; !note delete ; !note clear" + return "In a private message, with an authenticated account: !note add ; !note ; !note list; !note delete ; !note clear (notes are network-scoped)" } func (p *Note) Init(c bot.PluginConfig, db *storage.DB) error { @@ -60,6 +60,7 @@ func (p *Note) Init(c bot.PluginConfig, db *storage.DB) error { if expiryDays < 0 || expiryDays > 3650 { expiryDays = defaultNoteExpiryDays } + p.expiry = 0 if expiryDays > 0 { p.expiry = time.Duration(expiryDays) * 24 * time.Hour } @@ -72,8 +73,16 @@ func (p *Note) Handle(b *bot.Bot, m bot.Message) bool { if !ok || (cmd != "note" && cmd != "notes") { return false } + if m.IsChannel { + b.Send(m.Nick, ircColor(ircYellow, "Use notes in a private message to GoBot; no note contents or names are returned to channels.")) + return true + } identity := noteIdentity(b, m) - if identity == "" || p.db == nil { + if identity == "" { + b.Send(m.ReplyTarget(), ircColor(ircYellow, "Notes require an authenticated IRC account and account-tag support on this network.")) + return true + } + if p.db == nil { b.Send(m.ReplyTarget(), ircColor(ircRed, "note storage is unavailable")) return true } @@ -265,15 +274,16 @@ func noteUsage() string { func noteIdentity(b *bot.Bot, m bot.Message) string { account := strings.TrimSpace(m.Account) - if account != "" && account != "*" { - return "account:" + strings.ToLower(account) + network := strings.TrimSpace(b.Config.NetworkName) + if network == "" { + network = strings.TrimSpace(b.Config.Server.Host) } - nick := strings.ToLower(strings.TrimSpace(m.Nick)) - if nick == "" { + if account == "" || account == "*" || network == "" { return "" } - network := strings.ToLower(strings.TrimSpace(b.Config.NetworkName)) - return "nick:" + network + "\x00" + nick + // Account names are local to an IRC network. Never claim old unscoped or + // nickname-only records automatically; their ownership is ambiguous. + return scopedKey(network, "notes", account) } func noteStorageKey(identity string) string { diff --git a/plugins/note_security_test.go b/plugins/note_security_test.go new file mode 100644 index 0000000..71a78a8 --- /dev/null +++ b/plugins/note_security_test.go @@ -0,0 +1,100 @@ +package plugins + +import ( + "context" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/variablenix/GoBot/bot" + "github.com/variablenix/GoBot/storage" +) + +func noteTestCommand(t *testing.T, p *Note, network, account, command string, channel bool) string { + t.Helper() + sent := make(chan bot.Outgoing, 10) + b := &bot.Bot{Config: bot.Config{NetworkName: network, CommandPrefix: "!"}, Queue: bot.NewQueue(1000, 10, func(out bot.Outgoing) { sent <- out })} + defer b.Queue.Drain(context.Background()) + m := bot.Message{Command: "PRIVMSG", Nick: "tester", Target: "GoBot", Account: account, Text: command, IsChannel: channel} + if channel { + m.Target = "#test" + } + if !p.Handle(b, m) { + t.Fatal("note command not handled") + } + b.Queue.Drain(context.Background()) + var replies []string + for len(sent) > 0 { + out := <-sent + if out.Target != "tester" { + t.Fatal("private note output sent to a channel") + } + replies = append(replies, stripPluginIRC(out.Text)) + } + return strings.Join(replies, " ") +} + +func TestNotesRejectGuestAndChannelAccessAndIsolateNetworks(t *testing.T) { + db, err := storage.Open(filepath.Join(t.TempDir(), "notes.db")) + if err != nil { + t.Fatal(err) + } + defer db.Close() + p := &Note{} + if err := p.Init(nil, db); err != nil { + t.Fatal(err) + } + if got := noteTestCommand(t, p, "alpha", "alice", "!note add sample synthetic-private-value", false); !strings.Contains(got, "saved") { + t.Fatal(got) + } + for _, account := range []string{"", "*", "bob"} { + if got := noteTestCommand(t, p, "alpha", account, "!note sample", false); strings.Contains(got, "synthetic-private-value") { + t.Fatal("guest or another account read private notes") + } + } + if got := noteTestCommand(t, p, "beta", "alice", "!note sample", false); strings.Contains(got, "synthetic-private-value") { + t.Fatal("same-named account on another network read private notes") + } + if got := noteTestCommand(t, p, "alpha", "alice", "!note sample", true); strings.Contains(got, "synthetic-private-value") || strings.Contains(got, "sample:") { + t.Fatal("channel command revealed note content") + } + if got := noteTestCommand(t, p, "alpha", "alice", "!note delete sample", true); !strings.Contains(got, "private message") { + t.Fatal("channel mutation was not rejected") + } + if got := noteTestCommand(t, p, "alpha", "alice", "!note sample", false); !strings.Contains(got, "synthetic-private-value") { + t.Fatal("legitimate private note was changed or inaccessible") + } + if err := p.Init(bot.PluginConfig{"expiry_days": 0}, db); err != nil || p.expiry != 0 { + t.Fatal("reinitialization did not disable expiry") + } +} + +func TestLegacyNotesPreservedWithoutAutomaticOwnershipClaim(t *testing.T) { + db, err := storage.Open(filepath.Join(t.TempDir(), "notes.db")) + if err != nil { + t.Fatal(err) + } + defer db.Close() + key := noteStorageKey("account:alice") + old := time.Now().Add(-365 * 24 * time.Hour) + if err := db.Set("notes", key, map[string]noteRecord{"legacy": {Name: "legacy", Text: "synthetic-legacy-value", CreatedAt: old}}); err != nil { + t.Fatal(err) + } + p := &Note{} + if err := p.Init(nil, db); err != nil { + t.Fatal(err) + } + if got := noteTestCommand(t, p, "alpha", "alice", "!note legacy", false); strings.Contains(got, "synthetic-legacy-value") { + t.Fatal("legacy records were claimed by an unverified network identity") + } + if raw, err := db.Get("notes", key); err != nil || !strings.Contains(string(raw), "synthetic-legacy-value") { + t.Fatal("legacy data was removed or modified") + } + if err := db.Close(); err != nil { + t.Fatal(err) + } + if got := noteTestCommand(t, p, "alpha", "alice", "!note add sample text", false); !strings.Contains(got, "couldn't save") { + t.Fatal("storage failure was reported as success") + } +} diff --git a/plugins/note_test.go b/plugins/note_test.go index c4fa6d4..c71ed9e 100644 --- a/plugins/note_test.go +++ b/plugins/note_test.go @@ -33,14 +33,18 @@ func TestSplitAndParseNoteCommands(t *testing.T) { } } -func TestNoteIdentityUsesAccountThenNetworkAndNick(t *testing.T) { +func TestNoteIdentityRequiresAccountAndScopesItToNetwork(t *testing.T) { b := &bot.Bot{Config: bot.Config{NetworkName: "Ouch"}} - if got := noteIdentity(b, bot.Message{Account: "Alice", Nick: "Different"}); got != "account:alice" { + if got := noteIdentity(b, bot.Message{Account: "Alice", Nick: "Different"}); got != scopedKey("ouch", "notes", "alice") { t.Fatalf("account identity = %q", got) } - if got := noteIdentity(b, bot.Message{Account: "*", Nick: "Alice"}); got != "nick:ouch\x00alice" { + if got := noteIdentity(b, bot.Message{Account: "*", Nick: "Alice"}); got != "" { t.Fatalf("fallback identity = %q", got) } + other := &bot.Bot{Config: bot.Config{NetworkName: "other"}} + if noteIdentity(b, bot.Message{Account: "alice"}) == noteIdentity(other, bot.Message{Account: "alice"}) { + t.Fatal("same-named accounts on different networks share note access") + } } func TestNotesPersistAndExpireInactiveRecords(t *testing.T) { diff --git a/plugins/paste.go b/plugins/paste.go index 302f9b0..8cf036e 100644 --- a/plugins/paste.go +++ b/plugins/paste.go @@ -41,11 +41,12 @@ func newPasteFetchHTTPClient() *http.Client { } transport.Proxy = nil transport.DialContext = safePublicDialContext + transport.MaxResponseHeaderBytes = 64 << 10 return &http.Client{ Timeout: 8 * time.Second, Transport: transport, - CheckRedirect: func(req *http.Request, _ []*http.Request) error { - if !publicHTTPURL(req.URL) { + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= 10 || !publicHTTPURLContext(req.Context(), req.URL) { return fmt.Errorf("redirect target is not a public HTTP URL") } return nil diff --git a/plugins/public_http.go b/plugins/public_http.go new file mode 100644 index 0000000..1d3e340 --- /dev/null +++ b/plugins/public_http.go @@ -0,0 +1,34 @@ +package plugins + +import "net/netip" + +// Shared by all fetchers of user-controlled URLs, not only website checks. +// Local, shared, documentation, benchmark, reserved, and transition address +// space must never be reachable through a public IRC lookup command. +var nonPublicIPPrefixes = []netip.Prefix{ + netip.MustParsePrefix("0.0.0.0/8"), netip.MustParsePrefix("100.64.0.0/10"), + netip.MustParsePrefix("192.0.0.0/24"), netip.MustParsePrefix("192.0.2.0/24"), + netip.MustParsePrefix("192.88.99.0/24"), netip.MustParsePrefix("198.18.0.0/15"), + netip.MustParsePrefix("198.51.100.0/24"), netip.MustParsePrefix("203.0.113.0/24"), + netip.MustParsePrefix("240.0.0.0/4"), netip.MustParsePrefix("2001::/23"), + netip.MustParsePrefix("2001:db8::/32"), netip.MustParsePrefix("2002::/16"), + netip.MustParsePrefix("3fff::/20"), +} + +var internetIPv6Global = netip.MustParsePrefix("2000::/3") + +func publicInternetIP(ip netip.Addr) bool { + if !ip.IsValid() || ip.Zone() != "" { + return false + } + ip = ip.Unmap() + if !ip.IsGlobalUnicast() || ip.IsPrivate() || ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.Is6() && !internetIPv6Global.Contains(ip) { + return false + } + for _, prefix := range nonPublicIPPrefixes { + if prefix.Contains(ip) { + return false + } + } + return true +} diff --git a/plugins/public_http_test.go b/plugins/public_http_test.go new file mode 100644 index 0000000..b1cc819 --- /dev/null +++ b/plugins/public_http_test.go @@ -0,0 +1,95 @@ +package plugins + +import ( + "context" + "net" + "net/http" + "net/netip" + "strings" + "testing" + "time" + + "github.com/variablenix/GoBot/bot" +) + +func TestPublicFetchersShareStrictAddressPolicy(t *testing.T) { + for _, raw := range []string{"100.64.0.1", "100.100.100.200", "0.1.2.3", "240.0.0.1", "198.18.0.1", "2001:db8::1", "::ffff:127.0.0.1"} { + ip := netip.MustParseAddr(raw) + if publicIP(net.ParseIP(raw)) || isPublicIP(ip) || publicInternetIP(ip) { + t.Fatalf("a user-controlled fetcher still accepts %s", raw) + } + if _, err := askPublicDialContext(t.Context(), "tcp", net.JoinHostPort(raw, "443")); err == nil { + t.Fatal("Ask dialer accepted a special-purpose literal") + } + if _, err := safePublicDialContext(t.Context(), "tcp", net.JoinHostPort(raw, "443")); err == nil { + t.Fatal("shared URL/paste dialer accepted a special-purpose literal") + } + } + if publicIP(nil) { + t.Fatal("empty address accepted") + } + for _, client := range []*http.Client{newAskWebHTTPClient(), newPasteFetchHTTPClient()} { + transport := client.Transport.(*http.Transport) + if transport.Proxy != nil || transport.MaxResponseHeaderBytes != 64<<10 { + t.Fatal("public fetcher lacks proxy/header protections") + } + req, _ := http.NewRequest(http.MethodGet, "https://1.1.1.1/", nil) + if client.CheckRedirect(req, make([]*http.Request, 10)) == nil { + t.Fatal("public fetcher follows unbounded redirects") + } + } +} + +func TestURLTitleBoundsWorkersAcrossNetworksAndSnapshotsSettings(t *testing.T) { + started := make(chan struct{}, 20) + release := make(chan struct{}) + replies := make(chan string, 20) + client := &http.Client{Timeout: time.Second, Transport: newPluginRoundTripper(func(*http.Request) (*http.Response, error) { + started <- struct{}{} + <-release + return newPluginResponse(200, "Synthetic original title"), nil + })} + p := &URLTitle{} + if err := p.Init(nil, nil); err != nil { + t.Fatal(err) + } + p.client = client + b := &bot.Bot{Config: bot.Config{NetworkName: "alpha"}, Queue: bot.NewQueue(1000, 20, func(out bot.Outgoing) { replies <- out.Text })} + defer b.Queue.Drain(context.Background()) + for i := 0; i < 20; i++ { + b.Config.NetworkName = []string{"alpha", "beta"}[i%2] + p.Handle(b, bot.Message{IsChannel: true, Target: "#test", Text: "https://1.1.1.1/"}) + } + deadline := time.After(2 * time.Second) + for i := 0; i < cap(urlTitleSlots); i++ { + select { + case <-started: + case <-deadline: + t.Fatal("bounded URL workers did not start") + } + } + // The router allows reinitialization only after Handle returns, but old + // asynchronous workers can still be running and must use captured settings. + if err := p.Init(bot.PluginConfig{"max_title_length": 1}, nil); err != nil { + t.Fatal(err) + } + close(release) + ticker := time.NewTicker(time.Millisecond) + defer ticker.Stop() + for len(urlTitleSlots) != 0 { + select { + case <-ticker.C: + case <-deadline: + t.Fatal("URL workers did not release their capacity") + } + } + b.Queue.Drain(context.Background()) + if len(started) != 0 || len(replies) != cap(urlTitleSlots) { + t.Fatal("URL worker limit exceeded or results were lost") + } + for len(replies) > 0 { + if text := <-replies; !strings.Contains(text, "Synthetic original title") { + t.Fatal("old URL fetch used reloaded settings") + } + } +} diff --git a/plugins/status_http.go b/plugins/status_http.go index ec40b50..a75fa5d 100644 --- a/plugins/status_http.go +++ b/plugins/status_http.go @@ -42,7 +42,7 @@ func parseStatusTarget(raw string) (*url.URL, error) { } host := strings.ToLower(strings.TrimSuffix(u.Hostname(), ".")) if ip, err := netip.ParseAddr(host); err == nil { - if !statusPublicIP(ip) { + if !publicInternetIP(ip) { return nil, errStatusTargetBlocked } if ip.Is6() { @@ -79,37 +79,6 @@ func parseStatusTarget(raw string) (*url.URL, error) { return u, nil } -// Restrict checks to ordinary globally routed addresses. In addition to local -// ranges, exclude shared, documentation, benchmark, and transition space from -// IANA's special-purpose registries (including IPv4-mapped private addresses). -var statusBlockedPrefixes = []netip.Prefix{ - netip.MustParsePrefix("0.0.0.0/8"), netip.MustParsePrefix("100.64.0.0/10"), - netip.MustParsePrefix("192.0.0.0/24"), netip.MustParsePrefix("192.0.2.0/24"), - netip.MustParsePrefix("192.88.99.0/24"), netip.MustParsePrefix("198.18.0.0/15"), - netip.MustParsePrefix("198.51.100.0/24"), netip.MustParsePrefix("203.0.113.0/24"), - netip.MustParsePrefix("240.0.0.0/4"), netip.MustParsePrefix("2001::/23"), - netip.MustParsePrefix("2001:db8::/32"), netip.MustParsePrefix("2002::/16"), - netip.MustParsePrefix("3fff::/20"), -} - -var statusIPv6Global = netip.MustParsePrefix("2000::/3") - -func statusPublicIP(ip netip.Addr) bool { - if !ip.IsValid() || ip.Zone() != "" { - return false - } - ip = ip.Unmap() - if !ip.IsGlobalUnicast() || ip.IsPrivate() || ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.Is6() && !statusIPv6Global.Contains(ip) { - return false - } - for _, prefix := range statusBlockedPrefixes { - if prefix.Contains(ip) { - return false - } - } - return true -} - func newStatusHTTPClient(timeout time.Duration) *http.Client { transport := http.DefaultTransport.(*http.Transport).Clone() transport.Proxy = nil // Never let an environment proxy bypass IP validation. @@ -155,7 +124,7 @@ func dialStatusPublic(ctx context.Context, network, address string, // Validate the entire answer before dialing even one address. Pin each // connection to a validated IP; never resolve the hostname a second time. for _, ip := range ips { - if !statusPublicIP(ip) { + if !publicInternetIP(ip) { return nil, errStatusTargetBlocked } } diff --git a/plugins/status_http_test.go b/plugins/status_http_test.go index e2a3e56..b887b76 100644 --- a/plugins/status_http_test.go +++ b/plugins/status_http_test.go @@ -52,7 +52,7 @@ func TestStatusPublicIP(t *testing.T) { "::ffff:10.0.0.1", "64:ff9b::a00:1", "2001::1", "2001:db8::1", "2002:7f00:1::1", "3fff::1", } { ip := netip.MustParseAddr(raw) - if statusPublicIP(ip) { + if publicInternetIP(ip) { t.Errorf("non-public address accepted: %s", raw) } if _, err := parseStatusTarget("https://" + net.JoinHostPort(raw, "443")); err == nil { @@ -60,7 +60,7 @@ func TestStatusPublicIP(t *testing.T) { } } for _, raw := range []string{"1.1.1.1", "8.8.8.8", "2606:4700:4700::1111", "::ffff:8.8.8.8"} { - if !statusPublicIP(netip.MustParseAddr(raw)) { + if !publicInternetIP(netip.MustParseAddr(raw)) { t.Errorf("public address rejected: %s", raw) } } diff --git a/plugins/urltitle.go b/plugins/urltitle.go index 9d1665a..d0e714b 100644 --- a/plugins/urltitle.go +++ b/plugins/urltitle.go @@ -7,6 +7,7 @@ import ( "io" "net" "net/http" + "net/netip" "net/url" "regexp" "strconv" @@ -26,6 +27,8 @@ type URLTitle struct { youtubeAPIKey string } +var urlTitleSlots = make(chan struct{}, 4) + func (p *URLTitle) Name() string { return "urltitle" } func (p *URLTitle) Commands() []string { return nil } func (p *URLTitle) Help() string { return "posts the title of URLs shared in a channel" } @@ -40,11 +43,12 @@ func (p *URLTitle) Init(c bot.PluginConfig, _ *storage.DB) error { // request to a loopback, private, or link-local service. transport.Proxy = nil transport.DialContext = safePublicDialContext + transport.MaxResponseHeaderBytes = 64 << 10 p.client = &http.Client{ Timeout: time.Duration(c.Int("timeout_seconds", 5)) * time.Second, Transport: transport, - CheckRedirect: func(req *http.Request, _ []*http.Request) error { - if !publicHTTPURL(req.URL) { + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= 10 || !publicHTTPURLContext(req.Context(), req.URL) { return fmt.Errorf("redirect target is not a public HTTP URL") } return nil @@ -62,12 +66,24 @@ func (p *URLTitle) Handle(b *bot.Bot, m bot.Message) bool { return false } parsed, err := url.Parse(u) - if err != nil || !publicHTTPURL(parsed) { + if err != nil || (parsed.Scheme != "http" && parsed.Scheme != "https") || parsed.Hostname() == "" || parsed.User != nil { + return false + } + select { + case urlTitleSlots <- struct{}{}: + default: return false } + // Capture immutable request settings before leaving the router's reload + // lock. Re-enabling the plugin must not race with an old background fetch. + client, apiKey, maxTitle := p.client, p.youtubeAPIKey, p.cfg.Int("max_title_length", 120) go func() { - requestCtx, cancel := context.WithTimeout(context.Background(), p.client.Timeout) + defer func() { <-urlTitleSlots }() + requestCtx, cancel := context.WithTimeout(context.Background(), client.Timeout) defer cancel() + if !publicHTTPURLContext(requestCtx, parsed) { + return + } req, err := http.NewRequestWithContext(requestCtx, http.MethodGet, parsed.String(), nil) if err != nil { return @@ -75,16 +91,16 @@ func (p *URLTitle) Handle(b *bot.Bot, m bot.Message) bool { req.Header.Set("User-Agent", "GoBot/1.0 (+IRC URL title fetcher)") req.Header.Set("Accept", "text/html,application/xhtml+xml") start := time.Now() - res, err := p.client.Do(req) + res, err := client.Do(req) if err != nil { - if title, ok := redditTitle(requestCtx, p.client, parsed); ok { - b.Send(m.Target, formatURLTitle(title, parsed, p.cfg.Int("max_title_length", 120))) + if title, ok := redditTitle(requestCtx, client, parsed); ok { + b.Send(m.Target, formatURLTitle(title, parsed, maxTitle)) } return } defer res.Body.Close() - if time.Since(start) > 2*time.Second { - b.Log.Warn("slow URL fetch", zap.String("url", u), zap.Duration("latency", time.Since(start))) + if time.Since(start) > 2*time.Second && b.Log != nil { + b.Log.Warn("slow URL fetch", zap.String("host", parsed.Hostname()), zap.Duration("latency", time.Since(start))) } body, err := io.ReadAll(io.LimitReader(res.Body, 2<<20)) if err != nil { @@ -96,7 +112,7 @@ func (p *URLTitle) Handle(b *bot.Bot, m bot.Message) bool { // actual post title even when the normal HTML page is a shell, a // bot-check page, or an access-denied document. if isRedditHost(parsed.Hostname()) { - if reddit, ok := redditTitle(requestCtx, p.client, parsed); ok { + if reddit, ok := redditTitle(requestCtx, client, parsed); ok { title = reddit } } @@ -107,8 +123,8 @@ func (p *URLTitle) Handle(b *bot.Bot, m bot.Message) bool { // Keep metadata lookup independent from the page-fetch timeout. Some // YouTube pages are slow enough that oEmbed/player requests would // otherwise inherit an already-expired context after the title fetch. - metadataCtx, metadataCancel := context.WithTimeout(context.Background(), p.client.Timeout) - metadata, ok := youtubeMetadata(metadataCtx, p.client, parsed, body, p.youtubeAPIKey) + metadataCtx, metadataCancel := context.WithTimeout(context.Background(), client.Timeout) + metadata, ok := youtubeMetadata(metadataCtx, client, parsed, body, apiKey) metadataCancel() if ok { title = metadata.title @@ -120,10 +136,10 @@ func (p *URLTitle) Handle(b *bot.Bot, m bot.Message) bool { return } if isYouTubeHost(parsed.Hostname()) { - b.Send(m.Target, formatYouTubeTitle(title, youtubeChannel, youtubeDuration, p.cfg.Int("max_title_length", 120))) + b.Send(m.Target, formatYouTubeTitle(title, youtubeChannel, youtubeDuration, maxTitle)) return } - b.Send(m.Target, formatURLTitle(title, parsed, p.cfg.Int("max_title_length", 120))) + b.Send(m.Target, formatURLTitle(title, parsed, maxTitle)) }() return false } @@ -653,5 +669,6 @@ func safePublicDialContext(ctx context.Context, network, address string) (net.Co } func publicIP(ip net.IP) bool { - return !ip.IsLoopback() && !ip.IsPrivate() && !ip.IsLinkLocalUnicast() && !ip.IsLinkLocalMulticast() && !ip.IsUnspecified() && !ip.IsMulticast() + address, ok := netip.AddrFromSlice(ip) + return ok && publicInternetIP(address) } From 91bbafcefa7656a875b3a43d6273eed3424d5e04 Mon Sep 17 00:00:00 2001 From: ak Date: Fri, 6 Nov 2026 17:45:47 -0800 Subject: [PATCH 2/3] Include dictionary fallback in live provider regression checks --- plugins/provider_live_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/provider_live_test.go b/plugins/provider_live_test.go index c2bfb69..382186a 100644 --- a/plugins/provider_live_test.go +++ b/plugins/provider_live_test.go @@ -24,6 +24,7 @@ func TestLivePluginCommands(t *testing.T) { command, want, token string }{ {&Wikipedia{}, "!wiki Linux", "wikipedia.org", ""}, + {&Define{}, "!define Linux", "Linux", ""}, {&Weather{}, "!weather London", "London", ""}, {&IMDb{}, "!imdb The Matrix", "imdb.com/title/", ""}, {&Linux{}, "!linux", "kernel", ""}, From b4889dd2b6628940486059888dc6ecfba5ca3418 Mon Sep 17 00:00:00 2001 From: ak Date: Fri, 6 Nov 2026 17:52:58 -0800 Subject: [PATCH 3/3] Reject one-sided comparison excerpts and sanitize title formatting --- docs/plugins.md | 6 ++++ plugins/ask.go | 10 ++++-- plugins/ask_relevance.go | 62 +++++++++++++++++++++++++++++++++++ plugins/ask_relevance_test.go | 50 ++++++++++++++++++++++++++++ plugins/urltitle.go | 2 ++ plugins/urltitle_test.go | 13 ++++++++ 6 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 plugins/ask_relevance.go create mode 100644 plugins/ask_relevance_test.go diff --git a/docs/plugins.md b/docs/plugins.md index 28cd7e2..ce50fd5 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -1430,6 +1430,12 @@ with an existing snippet gets at most 600 ms to refresh from its origin before returning the attributed snippet. Sources without snippets retain their normal bounded fetch budget. +For explicit `X vs Y` / `X versus Y` comparisons, a search-result fallback must +mention both distinct subjects; a fast one-sided result cannot preempt an +in-flight Assist answer. If an origin refresh loses that context, a complete +attributed snippet is retained instead. This lexical safeguard applies to +search excerpts, not to generated Assist paraphrases. + Lightweight attempts still retain the 300 ms normalized retry and a final original-question recovery request after 500 ms. Their original budget reserve is confined to that lane; it no longer delays independent providers. All waits diff --git a/plugins/ask.go b/plugins/ask.go index 35c7a4b..908625c 100644 --- a/plugins/ask.go +++ b/plugins/ask.go @@ -1098,6 +1098,9 @@ func selectAskSearchResult(question string, results []askRenderedSearchResult) ( if result.Title == "" || !validPublicHTTPURL(result.URL) { continue } + if !askComparisonRelevant(question, result.Title+" "+result.Snippet) { + continue + } parsedURL, _ := url.Parse(result.URL) if parsedURL == nil || isDuckDuckGoHost(parsedURL.Hostname()) { continue @@ -1309,12 +1312,15 @@ func fetchAskSearchResult(ctx context.Context, result askRenderedSearchResult, q body, readErr := io.ReadAll(io.LimitReader(res.Body, 512<<10)) if readErr == nil { if excerpt := extractAskHTMLExcerpt(body, result.Snippet, result.Title, strings.Fields(askFocusedTerm(question))); excerpt != "" { - return askSource{Title: result.Title, Summary: formatAskSearchResultSummary(result.Title, excerpt), URL: result.URL, Provider: "search_result"}, true + summary := formatAskSearchResultSummary(result.Title, excerpt) + if askComparisonRelevant(question, summary) { + return askSource{Title: result.Title, Summary: summary, URL: result.URL, Provider: "search_result"}, true + } } } } } - if result.Snippet != "" { + if result.Snippet != "" && askComparisonRelevant(question, result.Title+" "+result.Snippet) { return askSource{Title: result.Title, Summary: formatAskSearchResultSummary(result.Title, result.Snippet), URL: result.URL, Provider: "search_result"}, true } return askSource{}, false diff --git a/plugins/ask_relevance.go b/plugins/ask_relevance.go new file mode 100644 index 0000000..42a604d --- /dev/null +++ b/plugins/ask_relevance.go @@ -0,0 +1,62 @@ +package plugins + +import ( + "strings" + "unicode" +) + +// A search excerpt about only one side must not win a comparison query over +// an in-flight Assist answer. This is a conservative lexical check for weak +// search-result fallbacks, not a filter on generated Assist paraphrases. +func askComparisonRelevant(question, excerpt string) bool { + fields := strings.Fields(strings.ToLower(question)) + for i, field := range fields { + if token := strings.Trim(field, ".,?!:;"); token != "vs" && token != "versus" { + continue + } + left := askComparisonWords(strings.Join(fields[:i], " ")) + right := askComparisonWords(strings.Join(fields[i+1:], " ")) + for word := range left { + if _, shared := right[word]; shared { + delete(left, word) + delete(right, word) + } + } + if len(left) == 0 || len(right) == 0 { + return true // Too little subject information to reject a source. + } + compact := strings.Map(func(r rune) rune { + if unicode.IsLetter(r) || unicode.IsDigit(r) { + return unicode.ToLower(r) + } + return -1 + }, excerpt) + matches := func(words map[string]struct{}) bool { + for word := range words { + if strings.Contains(compact, word) { + return true + } + } + return false + } + return matches(left) && matches(right) + } + return true +} + +func askComparisonWords(text string) map[string]struct{} { + words := make(map[string]struct{}) + for _, word := range strings.FieldsFunc(text, func(r rune) bool { return !unicode.IsLetter(r) && !unicode.IsDigit(r) }) { + switch word { + case "how", "much", "do", "does", "what", "which", "is", "are", "the", "a", "an", "of", "to", "in", "for", "at", "cost", "costs", "price", "prices", "compare", "comparison", "between", "new", "used": + continue + } + if len(word) > 3 { + word = strings.TrimSuffix(word, "s") + } + if len(word) >= 2 { + words[word] = struct{}{} + } + } + return words +} diff --git a/plugins/ask_relevance_test.go b/plugins/ask_relevance_test.go new file mode 100644 index 0000000..63c4229 --- /dev/null +++ b/plugins/ask_relevance_test.go @@ -0,0 +1,50 @@ +package plugins + +import ( + "net/http" + "strings" + "testing" +) + +func TestAskComparisonRejectsOneSidedSearchFallbacks(t *testing.T) { + question := "how much do Alpha plans cost vs Beta plans?" + for _, text := range []string{"Alpha plans cost $10.", "Beta plans start at $20.", "Plans have different prices."} { + if askComparisonRelevant(question, text) { + t.Fatalf("one-sided or generic excerpt accepted: %s", text) + } + } + if !askComparisonRelevant(question, "Alpha plans start at $10, while Beta plans start at $20.") { + t.Fatal("complete comparison was rejected") + } + results := []askRenderedSearchResult{ + {Title: "Alpha plan prices", Snippet: "Alpha plans cost $10.", URL: "https://example.org/alpha"}, + {Title: "Alpha vs Beta pricing", Snippet: "A comparison of their plans.", URL: "https://example.org/comparison"}, + } + selected, ok := selectAskSearchResult(question, results) + if !ok || !strings.HasSuffix(selected.URL, "/comparison") { + t.Fatal("first one-sided result won over a comparison") + } + if _, ok := selectAskSearchResult(question, results[:1]); ok { + t.Fatal("one-sided source should leave other providers available") + } + if !askComparisonRelevant("what is Alpha?", "Alpha is an example service.") { + t.Fatal("non-comparison lookup changed") + } + if !askComparisonRelevant("Alpha vs. Beta", "Alpha and Beta compared.") { + t.Fatal("punctuated comparison rejected") + } +} + +func TestAskComparisonKeepsRelevantSnippetWhenOriginLosesContext(t *testing.T) { + old := askWebHTTPClient + t.Cleanup(func() { askWebHTTPClient = old }) + askWebHTTPClient = &http.Client{Transport: newPluginRoundTripper(func(*http.Request) (*http.Response, error) { + return newPluginResponse(200, ``), nil + })} + source, found := fetchAskSearchResult(t.Context(), askRenderedSearchResult{ + Title: "Plan pricing", Snippet: "Alpha starts at $10, Beta at $20.", URL: "https://example.org/prices", + }, "Alpha vs Beta prices") + if !found || !strings.Contains(source.Summary, "Beta") { + t.Fatal("origin refresh discarded the complete comparison snippet") + } +} diff --git a/plugins/urltitle.go b/plugins/urltitle.go index d0e714b..fabe427 100644 --- a/plugins/urltitle.go +++ b/plugins/urltitle.go @@ -145,6 +145,7 @@ func (p *URLTitle) Handle(b *bot.Bot, m bot.Message) bool { } func formatURLTitle(title string, parsed *url.URL, maxLength int) string { + title = cleanExternalText(title) displaySource := parsed.Host if isYouTubeHost(parsed.Hostname()) { if shortURL, ok := shortYouTubeDisplayURL(parsed); ok { @@ -155,6 +156,7 @@ func formatURLTitle(title string, parsed *url.URL, maxLength int) string { } func formatYouTubeTitle(title, channel, duration string, maxLength int) string { + title, channel, duration = cleanExternalText(title), cleanExternalText(channel), cleanExternalText(duration) result := "[YouTube] " + truncateRunes(title, maxLength) if channel != "" { result += " | Channel: " + truncateRunes(channel, 80) diff --git a/plugins/urltitle_test.go b/plugins/urltitle_test.go index d1ca7c1..350ff0f 100644 --- a/plugins/urltitle_test.go +++ b/plugins/urltitle_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "net/url" "regexp" + "strings" "testing" ) @@ -17,6 +18,18 @@ func TestURLExtraction(t *testing.T) { } } +func TestURLTitleFormatsStripUpstreamIRCAndTerminalControls(t *testing.T) { + parsed, _ := url.Parse("https://example.org/") + for _, text := range []string{ + formatURLTitle("safe\x0304 title\x01\x1b\r\n", parsed, 120), + formatYouTubeTitle("title\x0304", "channel\x01", "3:00\x1b", 120), + } { + if strings.ContainsAny(text, "\x01\x03\x1b\r\n") { + t.Fatal("untrusted metadata changed IRC or terminal control state") + } + } +} + func TestPageTitlePrefersOpenGraph(t *testing.T) { body := []byte(`