From 54b6ce0d57ac8d353c452127d605f2a2d630bce4 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer A (Kimi)" Date: Sun, 24 May 2026 04:00:29 +0000 Subject: [PATCH] fix(channels): make discord token-leak test deterministic (RCA #1763 Finding 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestDiscordAdapter_SendMessage_ErrorDoesNotLeakToken previously relied on a real network dial to an unroutable address and t.Skip'd when the request unexpectedly succeeded. This made the test flaky and caused silent loss of security-coverage in environments where the dial did not fail. - Add optional httpClient field to DiscordAdapter so tests can inject a fake http.RoundTripper. - Replace the skip-based test with a deterministic fake-transport that returns an error containing the webhook URL, then assert the adapter still redacts the token. - Zero behavioral change when httpClient is nil (production path). Fixes RCA #1763 Finding 2 — webhook token redaction skip. Co-Authored-By: Claude Opus 4.7 --- workspace-server/internal/channels/discord.go | 12 +++++-- .../internal/channels/discord_test.go | 31 ++++++++++++++----- 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/workspace-server/internal/channels/discord.go b/workspace-server/internal/channels/discord.go index 453007d9e..d4eb1d0c5 100644 --- a/workspace-server/internal/channels/discord.go +++ b/workspace-server/internal/channels/discord.go @@ -33,7 +33,12 @@ const ( // // StartPolling returns nil immediately — Discord does not support long-polling; // use the Interactions webhook route instead. -type DiscordAdapter struct{} +type DiscordAdapter struct { + // httpClient overrides the default HTTP client used by SendMessage. + // Used in tests to inject a fake transport so token-redaction checks + // are deterministic and do not depend on real network behavior. + httpClient *http.Client +} func (d *DiscordAdapter) Type() string { return "discord" } func (d *DiscordAdapter) DisplayName() string { return "Discord" } @@ -95,7 +100,10 @@ func (d *DiscordAdapter) SendMessage(ctx context.Context, config map[string]inte // Split long messages into chunks at word boundaries where possible. chunks := splitMessage(text, maxLen) - client := &http.Client{Timeout: discordHTTPTimeout} + client := d.httpClient + if client == nil { + client = &http.Client{Timeout: discordHTTPTimeout} + } for _, chunk := range chunks { payload, err := json.Marshal(map[string]string{"content": chunk}) if err != nil { diff --git a/workspace-server/internal/channels/discord_test.go b/workspace-server/internal/channels/discord_test.go index d2b79ff39..61c89f4df 100644 --- a/workspace-server/internal/channels/discord_test.go +++ b/workspace-server/internal/channels/discord_test.go @@ -3,6 +3,7 @@ package channels import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "strings" @@ -288,17 +289,25 @@ 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. +// HTTP call to the Discord webhook fails, the returned error message does NOT +// contain the webhook URL — which embeds the Discord token. Uses a fake +// http.RoundTripper so the test is deterministic and does not depend on real +// network behavior (fixes RCA #1763 Finding 2). // 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. + // Fake transport that returns an error containing the full webhook URL, + // mimicking the real behavior of net/http when a dial fails. + fakeTransport := roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return nil, fmt.Errorf("dial tcp: connect to %s: connection refused", req.URL.String()) + }) + + a := &DiscordAdapter{ + httpClient: &http.Client{Transport: fakeTransport}, + } + err := a.SendMessage( context.Background(), map[string]interface{}{"webhook_url": webhookURL}, @@ -307,14 +316,20 @@ func TestDiscordAdapter_SendMessage_ErrorDoesNotLeakToken(t *testing.T) { ) if err == nil { - // In some environments the request might actually succeed; that's fine. - t.Skip("request unexpectedly succeeded — skipping token-leak check") + t.Fatal("expected error from fake transport, got nil") } if strings.Contains(err.Error(), fakeToken) { t.Errorf("error message leaks Discord webhook token: %q", err.Error()) } } +// roundTripperFunc adapts a function to the http.RoundTripper interface. +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + func TestSplitMessage_SplitsAtNewline(t *testing.T) { // Build a message where a newline falls within the split window. line1 := strings.Repeat("a", 1500) + "\n" -- 2.52.0