fix(security): Ed25519 signature verification for Discord webhooks + strip token from error chain

HIGH (#659-1): POST /webhooks/discord had no signature verification, allowing any attacker to POST forged Discord slash-command payloads. Add Ed25519 verification via verifyDiscordSignature() before adapter.ParseWebhook() is called. The function reads r.Body, verifies Ed25519(pubKey, timestamp+body, X-Signature-Ed25519), then restores r.Body with io.NopCloser so ParseWebhook can still read the payload. The public key is resolved from the first enabled Discord channel's app_public_key config (plaintext — it is a public key and not in sensitiveFields) with a fallback to DISCORD_APP_PUBLIC_KEY env var; no key configured -> 401 (fail-closed). discordPublicKey() is the DB helper. MEDIUM (#659-2): discord.go SendMessage() wrapped http.Client.Do errors with %w, propagating the *url.Error which includes the full webhook URL (https://discord.com/api/webhooks/{id}/{token}) into logs and error responses. Replace with a static "discord: HTTP request failed" string. Tests added (11 new): - TestVerifyDiscordSignature_Valid / _WrongKey / _TamperedBody / _MissingTimestamp / _MissingSignature / _InvalidHexSignature / _InvalidHexPubKey / _WrongLengthPubKey (real Ed25519 key pairs) - TestChannelHandler_Webhook_Discord_NoKey_Returns401 - TestChannelHandler_Webhook_Discord_InvalidSig_Returns401 - TestChannelHandler_Webhook_Discord_ValidSig_PingAccepted - TestDiscordAdapter_SendMessage_ErrorDoesNotLeakToken go test ./... green. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-17 10:36:51 +00:00 · 2026-04-17 10:36:51 +00:00 · 470704416e
commit 470704416e
parent 06938e8335
4 changed files with 361 additions and 2 deletions
--- 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()
--- 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"
--- 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)
+}
--- 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)
+	}
+}