From 79cae648e45024aa77a149f526b8aa09509b909d Mon Sep 17 00:00:00 2001 From: viettranx Date: Thu, 2 Apr 2026 20:29:44 +0700 Subject: [PATCH] fix(security): pairing expiry race, sender-ID validation, Slack senderID cleanup - Add expires_at check to ApprovePairing SELECT (PG + SQLite) to close race between prune DELETE and code lookup - Add isValidSenderID regex validation to handleRequest, handleRevoke, and handleBrowserPairingStatus (prevents log injection / bus poisoning) - Add slog.Warn on decrypt fallback paths for downgrade detection - Remove Slack compound senderID|displayName format; all channels now pass plain senderID with displayName in metadata --- internal/channels/channel.go | 4 ++-- internal/channels/slack/handlers.go | 10 ++++----- internal/channels/slack/handlers_mention.go | 5 ++--- internal/channels/slack/utils.go | 4 ---- internal/crypto/aes.go | 10 +++++++-- internal/gateway/methods/pairing.go | 24 +++++++++++++++++++++ internal/store/pg/pairing.go | 5 +++-- internal/store/sqlitestore/pairing.go | 2 +- 8 files changed, 44 insertions(+), 20 deletions(-) diff --git a/internal/channels/channel.go b/internal/channels/channel.go index d0aec0b8..d482dc43 100644 --- a/internal/channels/channel.go +++ b/internal/channels/channel.go @@ -432,8 +432,8 @@ func (c *BaseChannel) HandleMessage(senderID, chatID, content string, media []st return } - // Derive userID from senderID: strip "|username" suffix if present (Telegram format). - // For most channels, senderID == userID (platform user ID). + // Derive userID from senderID: strip "|username" suffix if present (legacy Slack compound format). + // All channels now pass plain senderID; kept for backward compat with stored compound IDs. userID := senderID if idx := strings.IndexByte(senderID, '|'); idx > 0 { userID = senderID[:idx] diff --git a/internal/channels/slack/handlers.go b/internal/channels/slack/handlers.go index 5f750bde..4d769d89 100644 --- a/internal/channels/slack/handlers.go +++ b/internal/channels/slack/handlers.go @@ -86,9 +86,7 @@ func (c *Channel) handleMessage(ev *slackevents.MessageEvent) { peerKind = "direct" } - // Resolve display name; strip "|" to prevent compound senderID corruption - displayName := strings.ReplaceAll(c.resolveDisplayName(senderID), "|", "_") - compoundSenderID := fmt.Sprintf("%s|%s", senderID, displayName) + displayName := c.resolveDisplayName(senderID) // Policy check if isDM { @@ -103,7 +101,7 @@ func (c *Channel) handleMessage(ev *slackevents.MessageEvent) { // For DMs, apply global allowlist filter (allow_from contains user IDs). // For groups, skip — group policy already handles channel/user filtering. - if isDM && !c.IsAllowed(compoundSenderID) { + if isDM && !c.IsAllowed(senderID) { slog.Debug("slack message rejected by allowlist", "user_id", senderID, "display_name", displayName) return @@ -265,7 +263,7 @@ func (c *Channel) handleMessage(ev *slackevents.MessageEvent) { // Message debounce: batch rapid messages per-thread if c.debounceDelay > 0 { - if c.debounceMessage(localKey, compoundSenderID, channelID, finalContent, mediaPaths, metadata, peerKind) { + if c.debounceMessage(localKey, senderID, channelID, finalContent, mediaPaths, metadata, peerKind) { // Record thread participation even when debounced if peerKind == "group" && replyThreadTS != "" { participKey := channelID + ":particip:" + replyThreadTS @@ -275,7 +273,7 @@ func (c *Channel) handleMessage(ev *slackevents.MessageEvent) { } } - c.HandleMessage(compoundSenderID, channelID, finalContent, mediaPaths, metadata, peerKind) + c.HandleMessage(senderID, channelID, finalContent, mediaPaths, metadata, peerKind) // Record thread participation for auto-reply cache if peerKind == "group" { diff --git a/internal/channels/slack/handlers_mention.go b/internal/channels/slack/handlers_mention.go index 8991adb5..7d4883f7 100644 --- a/internal/channels/slack/handlers_mention.go +++ b/internal/channels/slack/handlers_mention.go @@ -39,8 +39,7 @@ func (c *Channel) handleAppMention(ev *slackevents.AppMentionEvent) { channelID := ev.Channel content := ev.Text - displayName := strings.ReplaceAll(c.resolveDisplayName(senderID), "|", "_") - compoundSenderID := fmt.Sprintf("%s|%s", senderID, displayName) + displayName := c.resolveDisplayName(senderID) if !c.checkGroupPolicy(ctx, senderID, channelID) { return @@ -99,7 +98,7 @@ func (c *Channel) handleAppMention(ev *slackevents.AppMentionEvent) { metadata["message_thread_id"] = replyThreadTS } - c.HandleMessage(compoundSenderID, channelID, finalContent, nil, metadata, "group") + c.HandleMessage(senderID, channelID, finalContent, nil, metadata, "group") // Record thread participation if replyThreadTS != "" { diff --git a/internal/channels/slack/utils.go b/internal/channels/slack/utils.go index 7990f174..0c128f14 100644 --- a/internal/channels/slack/utils.go +++ b/internal/channels/slack/utils.go @@ -5,7 +5,6 @@ import ( "fmt" "log/slog" "regexp" - "strings" "time" "github.com/nextlevelbuilder/goclaw/internal/bus" @@ -22,9 +21,6 @@ func (c *Channel) HandleMessage(senderID, chatID, content string, mediaPaths []s } userID := senderID - if idx := strings.IndexByte(senderID, '|'); idx > 0 { - userID = senderID[:idx] - } var mediaFiles []bus.MediaFile for _, p := range mediaPaths { diff --git a/internal/crypto/aes.go b/internal/crypto/aes.go index 67d35f7b..c1718239 100644 --- a/internal/crypto/aes.go +++ b/internal/crypto/aes.go @@ -8,6 +8,7 @@ import ( "encoding/base64" "encoding/hex" "errors" + "log/slog" "strings" ) @@ -55,6 +56,9 @@ func Decrypt(ciphertext, key string) (string, error) { } if !IsEncrypted(ciphertext) { + slog.Warn("crypto.unencrypted_value_read", + "hint", "value lacks aes-gcm: prefix — may be legacy plaintext or tampered", + ) return ciphertext, nil } @@ -65,7 +69,8 @@ func Decrypt(ciphertext, key string) (string, error) { data, err := base64.StdEncoding.DecodeString(strings.TrimPrefix(ciphertext, prefix)) if err != nil { - return ciphertext, nil // not valid base64 → treat as plain text + slog.Warn("crypto.invalid_base64_in_encrypted_value") + return ciphertext, nil } block, err := aes.NewCipher(keyBytes) @@ -80,7 +85,8 @@ func Decrypt(ciphertext, key string) (string, error) { nonceSize := gcm.NonceSize() if len(data) < nonceSize { - return ciphertext, nil // too short → treat as plain text + slog.Warn("crypto.encrypted_value_too_short", "len", len(data), "min", nonceSize) + return ciphertext, nil } plaintext, err := gcm.Open(nil, data[:nonceSize], data[nonceSize:], nil) diff --git a/internal/gateway/methods/pairing.go b/internal/gateway/methods/pairing.go index d0611828..5374f5b4 100644 --- a/internal/gateway/methods/pairing.go +++ b/internal/gateway/methods/pairing.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "log/slog" + "regexp" "github.com/nextlevelbuilder/goclaw/internal/bus" "github.com/nextlevelbuilder/goclaw/internal/gateway" @@ -12,6 +13,14 @@ import ( "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) +var validSenderIDRe = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._:@-]*$`) + +// isValidSenderID checks that a sender ID contains only safe characters. +// Prevents log injection and bus event poisoning. +func isValidSenderID(id string) bool { + return len(id) <= 128 && validSenderIDRe.MatchString(id) +} + // PairingApproveCallback is called after a pairing is approved. // channel is the channel name (e.g., "telegram"), chatID is the chat to notify, // senderID identifies the paired entity (e.g., "group:XXXX" for group pairings). @@ -66,6 +75,11 @@ func (m *PairingMethods) handleRequest(ctx context.Context, client *gateway.Clie return } + if !isValidSenderID(params.SenderID) { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, "invalid sender_id format")) + return + } + if params.AccountID == "" { params.AccountID = "default" } @@ -175,6 +189,11 @@ func (m *PairingMethods) handleRevoke(ctx context.Context, client *gateway.Clien return } + if !isValidSenderID(params.SenderID) { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, "invalid sender_id format")) + return + } + if err := m.service.RevokePairing(ctx, params.SenderID, params.Channel); err != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrNotFound, err.Error())) return @@ -222,6 +241,11 @@ func (m *PairingMethods) handleBrowserPairingStatus(ctx context.Context, client return } + if !isValidSenderID(params.SenderID) { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, "invalid sender_id format")) + return + } + paired, pairErr := m.service.IsPaired(ctx, params.SenderID, "browser") if pairErr != nil { slog.Warn("security.pairing_check_failed", "sender_id", params.SenderID, "error", pairErr) diff --git a/internal/store/pg/pairing.go b/internal/store/pg/pairing.go index aedde459..1aeca1c3 100644 --- a/internal/store/pg/pairing.go +++ b/internal/store/pg/pairing.go @@ -87,9 +87,10 @@ func (s *PGPairingStore) ApprovePairing(ctx context.Context, code, approvedBy st var senderID, channel, chatID string var metaJSON []byte var reqTenantID uuid.UUID - // Code lookup is cross-tenant (random token, approver is WS/HTTP user) + // Code lookup is cross-tenant (random token, approver is WS/HTTP user). + // Also check expires_at to close race between prune DELETE and this SELECT. err := s.db.QueryRowContext(ctx, - "SELECT id, sender_id, channel, chat_id, COALESCE(metadata, '{}'), tenant_id FROM pairing_requests WHERE code = $1", code, + "SELECT id, sender_id, channel, chat_id, COALESCE(metadata, '{}'), tenant_id FROM pairing_requests WHERE code = $1 AND expires_at > NOW()", code, ).Scan(&reqID, &senderID, &channel, &chatID, &metaJSON, &reqTenantID) if err != nil { return nil, fmt.Errorf("pairing code %s not found or expired", code) diff --git a/internal/store/sqlitestore/pairing.go b/internal/store/sqlitestore/pairing.go index ba8a3491..0019f51c 100644 --- a/internal/store/sqlitestore/pairing.go +++ b/internal/store/sqlitestore/pairing.go @@ -87,7 +87,7 @@ func (s *SQLitePairingStore) ApprovePairing(ctx context.Context, code, approvedB var reqTenantID uuid.UUID err := s.db.QueryRowContext(ctx, - "SELECT id, sender_id, channel, chat_id, COALESCE(metadata, '{}'), tenant_id FROM pairing_requests WHERE code = ?", code, + "SELECT id, sender_id, channel, chat_id, COALESCE(metadata, '{}'), tenant_id FROM pairing_requests WHERE code = ? AND expires_at > ?", code, now, ).Scan(&reqID, &senderID, &channel, &chatID, &metaJSON, &reqTenantID) if err != nil { return nil, fmt.Errorf("pairing code %s not found or expired", code)