mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-17 18:14:13 +00:00
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
This commit is contained in:
1 parent
7ba3a5115a
commit
79cae648e4
8 files changed
+44
-20
No files matched your search
@@ -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]
|
||||
|
||||
@@ -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" {
|
||||
|
||||
@@ -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 != "" {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in new issue
Block a user