diff --git a/platform/internal/channels/discord.go b/platform/internal/channels/discord.go index b7807724b..44957e397 100644 --- a/platform/internal/channels/discord.go +++ b/platform/internal/channels/discord.go @@ -84,7 +84,11 @@ func (d *DiscordAdapter) SendMessage(ctx context.Context, config map[string]inte resp, err := client.Do(req) if err != nil { - return fmt.Errorf("discord: send: %w", err) + // Do NOT wrap err — the *url.Error from http.Client.Do includes the + // full request URL, which contains the Discord webhook token + // (https://discord.com/api/webhooks/{id}/{token}). Wrapping with %w + // would propagate that token into logs and error responses (#659). + return fmt.Errorf("discord: HTTP request failed") } body, _ := io.ReadAll(resp.Body) resp.Body.Close() diff --git a/platform/internal/channels/discord_test.go b/platform/internal/channels/discord_test.go index cd184d17e..61b71a4c8 100644 --- a/platform/internal/channels/discord_test.go +++ b/platform/internal/channels/discord_test.go @@ -287,6 +287,34 @@ func TestSplitMessage_LongMessage(t *testing.T) { } } +// TestDiscordAdapter_SendMessage_ErrorDoesNotLeakToken verifies that when the +// HTTP call to the Discord webhook fails (e.g. DNS error), the returned error +// message does NOT contain the webhook URL — which embeds the Discord token. +// Regression test for the MEDIUM security finding in PR #659. +func TestDiscordAdapter_SendMessage_ErrorDoesNotLeakToken(t *testing.T) { + a := &DiscordAdapter{} + // Use a valid-looking webhook URL with a fake token so we can check it + // doesn't appear in the error string. + fakeToken := "SUPER_SECRET_DISCORD_TOKEN_12345" + webhookURL := discordWebhookPrefix + "123456789/" + fakeToken + + // Point at an unroutable address to force a dial error. + err := a.SendMessage( + context.Background(), + map[string]interface{}{"webhook_url": webhookURL}, + "ignored", + "hello", + ) + + if err == nil { + // In some environments the request might actually succeed; that's fine. + t.Skip("request unexpectedly succeeded — skipping token-leak check") + } + if strings.Contains(err.Error(), fakeToken) { + t.Errorf("error message leaks Discord webhook token: %q", err.Error()) + } +} + func TestSplitMessage_SplitsAtNewline(t *testing.T) { // Build a message where a newline falls within the split window. line1 := strings.Repeat("a", 1500) + "\n" diff --git a/platform/internal/handlers/channels.go b/platform/internal/handlers/channels.go index c2bb08909..0c7df94ce 100644 --- a/platform/internal/handlers/channels.go +++ b/platform/internal/handlers/channels.go @@ -1,12 +1,17 @@ package handlers import ( + "bytes" "context" + "crypto/ed25519" "crypto/subtle" "database/sql" + "encoding/hex" "encoding/json" + "io" "log" "net/http" + "os" "strings" "github.com/gin-gonic/gin" @@ -410,6 +415,22 @@ func (h *ChannelHandler) Webhook(c *gin.Context) { return } + // Discord: verify Ed25519 signature BEFORE the body is consumed by ParseWebhook. + // The app_public_key is the Discord application's public key (not a secret — + // it's a PUBLIC key and therefore stored in plaintext in channel_config). + // We look it up from the DB (first enabled Discord channel with the field set) + // and fall back to the DISCORD_APP_PUBLIC_KEY env var for self-hosted setups + // that prefer global configuration. Fail closed: no key configured → 401. + // verifyDiscordSignature restores r.Body after reading so ParseWebhook below + // can still read the payload. + if channelType == "discord" { + pubKey := discordPublicKey(ctx) + if pubKey == "" || !verifyDiscordSignature(c.Request, pubKey) { + c.JSON(http.StatusUnauthorized, gin.H{"error": "invalid signature"}) + return + } + } + // For webhooks, we need to find the channel by type and match by chat_id in the message // Parse the webhook first to get the chat_id msg, err := adapter.ParseWebhook(c, nil) @@ -489,3 +510,69 @@ func (h *ChannelHandler) Webhook(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"status": "accepted"}) } + +// discordPublicKey returns the Ed25519 public key to use for Discord request +// signature verification. It queries the DB for the first enabled Discord +// channel whose config contains a non-empty app_public_key (stored in +// plaintext — it is a PUBLIC key and is not in the sensitiveFields list), +// then falls back to the DISCORD_APP_PUBLIC_KEY environment variable. +// +// Returns "" when no key is configured, which causes the caller to reject +// the incoming request with 401 (fail-closed behaviour). +func discordPublicKey(ctx context.Context) string { + var pubKey string + row := db.DB.QueryRowContext(ctx, ` + SELECT COALESCE(channel_config->>'app_public_key', '') + FROM workspace_channels + WHERE channel_type = 'discord' AND enabled = true + AND channel_config->>'app_public_key' IS NOT NULL + AND channel_config->>'app_public_key' != '' + LIMIT 1 + `) + _ = row.Scan(&pubKey) + if pubKey != "" { + return pubKey + } + return os.Getenv("DISCORD_APP_PUBLIC_KEY") +} + +// verifyDiscordSignature verifies a Discord Interactions request using the +// Ed25519 signature scheme described in Discord's Interactions documentation. +// Discord signs the concatenation of the X-Signature-Timestamp header and the +// raw request body with the application's private key; we verify with the +// public key stored in channel_config or DISCORD_APP_PUBLIC_KEY. +// +// The function reads r.Body in full and then replaces it with a bytes.Reader +// over the same bytes so that subsequent callers (adapter.ParseWebhook) can +// still read the body. +// +// Returns false when any required header is missing, when pubKeyHex cannot +// be hex-decoded to a 32-byte Ed25519 public key, when the signature header +// cannot be decoded, or when the Ed25519 verification itself fails. +func verifyDiscordSignature(r *http.Request, pubKeyHex string) bool { + sig := r.Header.Get("X-Signature-Ed25519") + ts := r.Header.Get("X-Signature-Timestamp") + if sig == "" || ts == "" || pubKeyHex == "" { + return false + } + + pubKeyBytes, err := hex.DecodeString(pubKeyHex) + if err != nil || len(pubKeyBytes) != ed25519.PublicKeySize { + return false + } + + body, err := io.ReadAll(r.Body) + if err != nil { + return false + } + // Restore body so adapter.ParseWebhook can read it. + r.Body = io.NopCloser(bytes.NewReader(body)) + + sigBytes, err := hex.DecodeString(sig) + if err != nil { + return false + } + + msg := append([]byte(ts), body...) + return ed25519.Verify(pubKeyBytes, msg, sigBytes) +} diff --git a/platform/internal/handlers/channels_test.go b/platform/internal/handlers/channels_test.go index 88f0a5047..d05909ea5 100644 --- a/platform/internal/handlers/channels_test.go +++ b/platform/internal/handlers/channels_test.go @@ -3,12 +3,17 @@ package handlers import ( "bytes" "context" + "crypto/ed25519" + "crypto/rand" + "encoding/hex" "encoding/json" + "io" "net/http" "net/http/httptest" + "strings" "testing" - "github.com/DATA-DOG/go-sqlmock" + sqlmock "github.com/DATA-DOG/go-sqlmock" "github.com/Molecule-AI/molecule-monorepo/platform/internal/channels" "github.com/gin-gonic/gin" ) @@ -579,3 +584,238 @@ func TestChannelHandler_Send_BudgetNotYetReached_PassesThrough(t *testing.T) { t.Errorf("expected budget check to pass (under limit), but got 429") } } + +// ==================== Discord Ed25519 signature verification ==================== +// +// These tests cover verifyDiscordSignature and the Discord signature gate in +// the Webhook handler. They use real Ed25519 key pairs generated in-process so +// the cryptographic assertions are load-bearing (not hand-crafted hex strings). + +// genDiscordKey generates a fresh Ed25519 key pair for tests. +// Returns (pubKeyHex, privKey). +func genDiscordKey(t *testing.T) (string, ed25519.PrivateKey) { + t.Helper() + pub, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("ed25519.GenerateKey: %v", err) + } + return hex.EncodeToString(pub), priv +} + +// discordSignedRequest builds an *http.Request with the correct Discord +// Ed25519 headers signed by privKey. +func discordSignedRequest(t *testing.T, body string, ts string, privKey ed25519.PrivateKey) *http.Request { + t.Helper() + msg := append([]byte(ts), []byte(body)...) + sig := ed25519.Sign(privKey, msg) + req := httptest.NewRequest(http.MethodPost, "/webhooks/discord", strings.NewReader(body)) + req.Header.Set("X-Signature-Ed25519", hex.EncodeToString(sig)) + req.Header.Set("X-Signature-Timestamp", ts) + return req +} + +// TestVerifyDiscordSignature_Valid asserts that a correctly signed request +// passes verification. +func TestVerifyDiscordSignature_Valid(t *testing.T) { + pubHex, priv := genDiscordKey(t) + body := `{"type":1}` + req := discordSignedRequest(t, body, "1700000000", priv) + + if !verifyDiscordSignature(req, pubHex) { + t.Error("expected true for valid Discord signature, got false") + } + // Body must be restored so subsequent reads still work. + restored, _ := io.ReadAll(req.Body) + if string(restored) != body { + t.Errorf("body not restored: got %q, want %q", restored, body) + } +} + +// TestVerifyDiscordSignature_WrongKey asserts that a signature verified with +// a different public key returns false. +func TestVerifyDiscordSignature_WrongKey(t *testing.T) { + _, priv := genDiscordKey(t) + wrongPubHex, _ := genDiscordKey(t) // different key pair + req := discordSignedRequest(t, `{"type":1}`, "1700000000", priv) + + if verifyDiscordSignature(req, wrongPubHex) { + t.Error("expected false for signature verified with wrong public key") + } +} + +// TestVerifyDiscordSignature_TamperedBody asserts that modifying the body +// after signing invalidates the signature. +func TestVerifyDiscordSignature_TamperedBody(t *testing.T) { + pubHex, priv := genDiscordKey(t) + req := discordSignedRequest(t, `{"type":1}`, "1700000000", priv) + // Replace the body with different content after signing. + req.Body = io.NopCloser(strings.NewReader(`{"type":2,"tampered":true}`)) + + if verifyDiscordSignature(req, pubHex) { + t.Error("expected false for tampered body, got true") + } +} + +// TestVerifyDiscordSignature_MissingTimestamp asserts that a missing +// X-Signature-Timestamp header returns false. +func TestVerifyDiscordSignature_MissingTimestamp(t *testing.T) { + pubHex, priv := genDiscordKey(t) + req := discordSignedRequest(t, `{"type":1}`, "1700000000", priv) + req.Header.Del("X-Signature-Timestamp") + + if verifyDiscordSignature(req, pubHex) { + t.Error("expected false for missing X-Signature-Timestamp") + } +} + +// TestVerifyDiscordSignature_MissingSignature asserts that a missing +// X-Signature-Ed25519 header returns false. +func TestVerifyDiscordSignature_MissingSignature(t *testing.T) { + pubHex, priv := genDiscordKey(t) + req := discordSignedRequest(t, `{"type":1}`, "1700000000", priv) + req.Header.Del("X-Signature-Ed25519") + + if verifyDiscordSignature(req, pubHex) { + t.Error("expected false for missing X-Signature-Ed25519") + } +} + +// TestVerifyDiscordSignature_InvalidHexSignature asserts that a non-hex +// signature returns false. +func TestVerifyDiscordSignature_InvalidHexSignature(t *testing.T) { + pubHex, _ := genDiscordKey(t) + req := httptest.NewRequest(http.MethodPost, "/webhooks/discord", strings.NewReader(`{}`)) + req.Header.Set("X-Signature-Ed25519", "not-valid-hex!!!") + req.Header.Set("X-Signature-Timestamp", "1700000000") + + if verifyDiscordSignature(req, pubHex) { + t.Error("expected false for invalid hex signature") + } +} + +// TestVerifyDiscordSignature_InvalidHexPubKey asserts that a non-hex public +// key returns false. +func TestVerifyDiscordSignature_InvalidHexPubKey(t *testing.T) { + _, priv := genDiscordKey(t) + req := discordSignedRequest(t, `{}`, "1700000000", priv) + + if verifyDiscordSignature(req, "not-hex-at-all!!!") { + t.Error("expected false for non-hex public key") + } +} + +// TestVerifyDiscordSignature_WrongLengthPubKey asserts that a hex-encoded +// byte slice that is not 32 bytes returns false. +func TestVerifyDiscordSignature_WrongLengthPubKey(t *testing.T) { + _, priv := genDiscordKey(t) + req := discordSignedRequest(t, `{}`, "1700000000", priv) + // 16 bytes — too short for Ed25519. + shortKey := hex.EncodeToString(make([]byte, 16)) + + if verifyDiscordSignature(req, shortKey) { + t.Error("expected false for short public key") + } +} + +// TestChannelHandler_Webhook_Discord_NoKey_Returns401 verifies that a Discord +// webhook request is rejected with 401 when no public key is configured in the +// DB and DISCORD_APP_PUBLIC_KEY env var is not set. +func TestChannelHandler_Webhook_Discord_NoKey_Returns401(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewChannelHandler(newTestChannelManager()) + + // discordPublicKey: DB returns no rows (no Discord channels with app_public_key). + mock.ExpectQuery(`SELECT COALESCE\(channel_config->>'app_public_key'`). + WillReturnRows(sqlmock.NewRows([]string{"pubkey"})) + + // Ensure env var is not set. + t.Setenv("DISCORD_APP_PUBLIC_KEY", "") + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPost, "/webhooks/discord", strings.NewReader(`{"type":1}`)) + c.Request.Header.Set("X-Signature-Ed25519", "aabbcc") + c.Request.Header.Set("X-Signature-Timestamp", "1700000000") + c.Params = gin.Params{{Key: "type", Value: "discord"}} + + handler.Webhook(c) + + if w.Code != http.StatusUnauthorized { + t.Errorf("expected 401 (no public key), got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("unmet sqlmock expectations: %v", err) + } +} + +// TestChannelHandler_Webhook_Discord_InvalidSig_Returns401 verifies that a +// Discord webhook with an invalid signature is rejected with 401, even when a +// valid public key is configured. +func TestChannelHandler_Webhook_Discord_InvalidSig_Returns401(t *testing.T) { + pubHex, _ := genDiscordKey(t) // generate key but sign with a DIFFERENT key + _, wrongPriv := genDiscordKey(t) + + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewChannelHandler(newTestChannelManager()) + + // discordPublicKey: DB returns the correct pubHex. + mock.ExpectQuery(`SELECT COALESCE\(channel_config->>'app_public_key'`). + WillReturnRows(sqlmock.NewRows([]string{"pubkey"}).AddRow(pubHex)) + + // Build a request signed with the wrong private key. + req := discordSignedRequest(t, `{"type":1}`, "1700000000", wrongPriv) + req.URL.Path = "/webhooks/discord" + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = req + c.Params = gin.Params{{Key: "type", Value: "discord"}} + + handler.Webhook(c) + + if w.Code != http.StatusUnauthorized { + t.Errorf("expected 401 (invalid sig), got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("unmet sqlmock expectations: %v", err) + } +} + +// TestChannelHandler_Webhook_Discord_ValidSig_PingAccepted verifies that a +// correctly signed Discord PING (type=1) passes the signature gate and the +// handler returns 200 (PING returns nil msg → "ignored" status). +func TestChannelHandler_Webhook_Discord_ValidSig_PingAccepted(t *testing.T) { + pubHex, priv := genDiscordKey(t) + + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewChannelHandler(newTestChannelManager()) + + // discordPublicKey: DB returns pubHex. + mock.ExpectQuery(`SELECT COALESCE\(channel_config->>'app_public_key'`). + WillReturnRows(sqlmock.NewRows([]string{"pubkey"}).AddRow(pubHex)) + + body := `{"type":1}` + req := discordSignedRequest(t, body, "1700000000", priv) + req.URL.Path = "/webhooks/discord" + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = req + c.Params = gin.Params{{Key: "type", Value: "discord"}} + + handler.Webhook(c) + + // Discord PING → ParseWebhook returns nil, nil → handler responds "ignored" + if w.Code != http.StatusOK { + t.Errorf("expected 200 for valid PING, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "ignored") { + t.Errorf("expected body to contain 'ignored', got: %s", w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Fatalf("unmet sqlmock expectations: %v", err) + } +}