From 0b5124a8f167db91943203932dfdf92d9530d926 Mon Sep 17 00:00:00 2001 From: viettranx Date: Mon, 16 Mar 2026 19:55:08 +0700 Subject: [PATCH] =?UTF-8?q?fix(security):=20harden=20pairing=20auth=20?= =?UTF-8?q?=E2=80=94=20fail-closed,=20rate-limit,=20TTL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Change WS pairing check from fail-open to fail-closed on DB error (router.go: previously granted RoleOperator on any IsPaired() error) - Add "browser" to InternalChannels so it's properly excluded from outbound dispatch without ad-hoc helpers - Rate-limit browser.pairing.status endpoint to prevent sender_id enumeration (reuses server RateLimiter via PairingMethods injection) - Add expires_at column to paired_devices with 30-day TTL for defense-in-depth; IsPaired() now checks expiry, ListPaired() prunes - Add confidence_score column to team_tasks, team_messages, team_task_comments - Bump RequiredSchemaVersion to 21 --- cmd/gateway_methods.go | 2 +- internal/channels/channel.go | 2 ++ internal/gateway/methods/pairing.go | 10 ++++++++-- internal/gateway/router.go | 8 ++++++-- internal/store/pg/pairing.go | 18 +++++++++++++----- internal/upgrade/version.go | 2 +- .../000021_paired_devices_expiry.down.sql | 4 ++++ migrations/000021_paired_devices_expiry.up.sql | 8 ++++++++ 8 files changed, 43 insertions(+), 11 deletions(-) create mode 100644 migrations/000021_paired_devices_expiry.down.sql create mode 100644 migrations/000021_paired_devices_expiry.up.sql diff --git a/cmd/gateway_methods.go b/cmd/gateway_methods.go index 2883a176..4cf71cee 100644 --- a/cmd/gateway_methods.go +++ b/cmd/gateway_methods.go @@ -29,7 +29,7 @@ func registerAllMethods(server *gateway.Server, agents *agent.Router, sessStore // Phase 2: Pairing (store created externally, shared with channel manager). // OnApprove callback is set later by the caller after channel manager is created. - pairingMethods := methods.NewPairingMethods(pairingStore, msgBus) + pairingMethods := methods.NewPairingMethods(pairingStore, msgBus, server.RateLimiter()) pairingMethods.Register(router) // Phase 2: Usage (queries SessionStore for real token data) diff --git a/internal/channels/channel.go b/internal/channels/channel.go index a867ba7f..28e350ba 100644 --- a/internal/channels/channel.go +++ b/internal/channels/channel.go @@ -19,10 +19,12 @@ import ( ) // InternalChannels are system channels excluded from outbound dispatch. +// "browser" uses WebSocket directly — no outbound channel routing needed. var InternalChannels = map[string]bool{ "cli": true, "system": true, "subagent": true, + "browser": true, } // IsInternalChannel checks if a channel name is internal. diff --git a/internal/gateway/methods/pairing.go b/internal/gateway/methods/pairing.go index 99c9bf90..69641730 100644 --- a/internal/gateway/methods/pairing.go +++ b/internal/gateway/methods/pairing.go @@ -23,10 +23,11 @@ type PairingMethods struct { msgBus *bus.MessageBus onApprove PairingApproveCallback broadcaster func(protocol.EventFrame) + rateLimiter *gateway.RateLimiter } -func NewPairingMethods(service store.PairingStore, msgBus *bus.MessageBus) *PairingMethods { - return &PairingMethods{service: service, msgBus: msgBus} +func NewPairingMethods(service store.PairingStore, msgBus *bus.MessageBus, rateLimiter *gateway.RateLimiter) *PairingMethods { + return &PairingMethods{service: service, msgBus: msgBus, rateLimiter: rateLimiter} } // SetOnApprove sets a callback that fires after a pairing is approved. @@ -203,6 +204,11 @@ func (m *PairingMethods) handleRevoke(ctx context.Context, client *gateway.Clien // handleBrowserPairingStatus lets a pending browser client check if its pairing code has been approved. // Called by unauthenticated clients during the browser pairing flow. func (m *PairingMethods) handleBrowserPairingStatus(ctx context.Context, client *gateway.Client, req *protocol.RequestFrame) { + // Rate-limit unauthenticated polling to prevent sender_id enumeration. + if m.rateLimiter != nil && m.rateLimiter.Enabled() && !m.rateLimiter.Allow("pairing:"+client.RemoteAddr()) { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrResourceExhausted, "rate limited")) + return + } locale := store.LocaleFromContext(ctx) var params struct { SenderID string `json:"sender_id"` diff --git a/internal/gateway/router.go b/internal/gateway/router.go index dd49f720..334b1d44 100644 --- a/internal/gateway/router.go +++ b/internal/gateway/router.go @@ -142,9 +142,13 @@ func (r *MethodRouter) handleConnect(ctx context.Context, client *Client, req *p if ps != nil && params.SenderID != "" { paired, pairErr := ps.IsPaired(params.SenderID, "browser") if pairErr != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed", "sender_id", params.SenderID, "error", pairErr) - paired = true + // Fail-closed: deny access on DB error instead of granting operator role. + locale := i18n.Normalize(client.locale) + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInternal, + i18n.T(locale, i18n.MsgInternalError, pairErr.Error()))) + return } if paired { client.role = permissions.RoleOperator diff --git a/internal/store/pg/pairing.go b/internal/store/pg/pairing.go index cb6ea6fe..dc040b3b 100644 --- a/internal/store/pg/pairing.go +++ b/internal/store/pg/pairing.go @@ -16,6 +16,7 @@ const ( codeAlphabet = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789" codeLength = 8 codeTTL = 60 * time.Minute + pairedDeviceTTL = 30 * 24 * time.Hour // 30 days maxPendingPerAccount = 3 ) @@ -90,12 +91,13 @@ func (s *PGPairingStore) ApprovePairing(code, approvedBy string) (*store.PairedD // Remove from pending s.db.Exec("DELETE FROM pairing_requests WHERE id = $1", reqID) - // Add to paired + // Add to paired (with expiry for defense-in-depth) now := time.Now() + expiresAt := now.Add(pairedDeviceTTL) _, err = s.db.Exec( - `INSERT INTO paired_devices (id, sender_id, channel, chat_id, paired_by, paired_at, metadata) - VALUES ($1, $2, $3, $4, $5, $6, $7)`, - uuid.Must(uuid.NewV7()), senderID, channel, chatID, approvedBy, now, metaJSON, + `INSERT INTO paired_devices (id, sender_id, channel, chat_id, paired_by, paired_at, metadata, expires_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8)`, + uuid.Must(uuid.NewV7()), senderID, channel, chatID, approvedBy, now, metaJSON, expiresAt, ) if err != nil { return nil, fmt.Errorf("create paired device: %w", err) @@ -142,7 +144,10 @@ func (s *PGPairingStore) RevokePairing(senderID, channel string) error { func (s *PGPairingStore) IsPaired(senderID, channel string) (bool, error) { var count int64 - err := s.db.QueryRow("SELECT COUNT(*) FROM paired_devices WHERE sender_id = $1 AND channel = $2", senderID, channel).Scan(&count) + err := s.db.QueryRow( + "SELECT COUNT(*) FROM paired_devices WHERE sender_id = $1 AND channel = $2 AND (expires_at IS NULL OR expires_at > NOW())", + senderID, channel, + ).Scan(&count) if err != nil { return false, fmt.Errorf("pairing check query: %w", err) } @@ -182,6 +187,9 @@ func (s *PGPairingStore) ListPending() []store.PairingRequestData { } func (s *PGPairingStore) ListPaired() []store.PairedDeviceData { + // Prune expired paired devices + s.db.Exec("DELETE FROM paired_devices WHERE expires_at IS NOT NULL AND expires_at < NOW()") + rows, err := s.db.Query("SELECT sender_id, channel, chat_id, paired_by, paired_at, COALESCE(metadata, '{}') FROM paired_devices ORDER BY paired_at DESC") if err != nil { return nil diff --git a/internal/upgrade/version.go b/internal/upgrade/version.go index 0e6b69fc..fb56bf40 100644 --- a/internal/upgrade/version.go +++ b/internal/upgrade/version.go @@ -2,4 +2,4 @@ package upgrade // RequiredSchemaVersion is the schema migration version this binary requires. // Bump this whenever adding a new SQL migration file. -const RequiredSchemaVersion uint = 20 +const RequiredSchemaVersion uint = 21 diff --git a/migrations/000021_paired_devices_expiry.down.sql b/migrations/000021_paired_devices_expiry.down.sql new file mode 100644 index 00000000..71213b23 --- /dev/null +++ b/migrations/000021_paired_devices_expiry.down.sql @@ -0,0 +1,4 @@ +ALTER TABLE paired_devices DROP COLUMN IF EXISTS expires_at; +ALTER TABLE team_tasks DROP COLUMN IF EXISTS confidence_score; +ALTER TABLE team_messages DROP COLUMN IF EXISTS confidence_score; +ALTER TABLE team_task_comments DROP COLUMN IF EXISTS confidence_score; diff --git a/migrations/000021_paired_devices_expiry.up.sql b/migrations/000021_paired_devices_expiry.up.sql new file mode 100644 index 00000000..b9a5ffaf --- /dev/null +++ b/migrations/000021_paired_devices_expiry.up.sql @@ -0,0 +1,8 @@ +-- Add expiry to paired devices for defense-in-depth. +-- NULL means no expiry (backward compat for existing rows). +ALTER TABLE paired_devices ADD COLUMN IF NOT EXISTS expires_at TIMESTAMPTZ; + +-- Add confidence_score to team tables for agent self-assessment. +ALTER TABLE team_tasks ADD COLUMN IF NOT EXISTS confidence_score FLOAT; +ALTER TABLE team_messages ADD COLUMN IF NOT EXISTS confidence_score FLOAT; +ALTER TABLE team_task_comments ADD COLUMN IF NOT EXISTS confidence_score FLOAT;