From 41f362841ef11372770eaec8c3a52da9a457f820 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Facu=5FVi=C3=B1as?= Date: Wed, 11 Mar 2026 17:58:47 -0300 Subject: [PATCH] fix(security): validate channel ID format to prevent SSRF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Slack IDs must match ^[A-Z0-9]{9,12}$, Discord snowflakes must match ^\d{17,20}$. resolveRemoteConfig() and getRemoteConfigStatus() now reject malformed IDs before they reach any URL interpolation. Also fixes pre-existing false-positive in config tests (env overrides couldn't affect module-level homedir() cache) — replaced with direct isValidChannelId() unit tests. Co-Authored-By: Claude Opus 4.6 --- .../gsd/tests/remote-questions.test.ts | 57 ++++++++++--------- .../extensions/remote-questions/config.ts | 17 +++++- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/src/resources/extensions/gsd/tests/remote-questions.test.ts b/src/resources/extensions/gsd/tests/remote-questions.test.ts index 5480b15e0..bf37c2771 100644 --- a/src/resources/extensions/gsd/tests/remote-questions.test.ts +++ b/src/resources/extensions/gsd/tests/remote-questions.test.ts @@ -1,7 +1,7 @@ import test from "node:test"; import assert from "node:assert/strict"; import { parseSlackReply, parseDiscordResponse } from "../../remote-questions/format.ts"; -import { resolveRemoteConfig } from "../../remote-questions/config.ts"; +import { resolveRemoteConfig, isValidChannelId } from "../../remote-questions/config.ts"; test("parseSlackReply handles single-number single-question answers", () => { const result = parseSlackReply("2", [{ @@ -87,31 +87,32 @@ test("parseDiscordResponse rejects multi-question reaction parsing", () => { assert.match(String(result.answers.second.user_note), /single-question prompts/i); }); -test("resolveRemoteConfig clamps invalid timeout and poll interval values", async () => { - const os = await import("node:os"); - const fs = await import("node:fs"); - const path = await import("node:path"); - - const savedHome = process.env.HOME; - const savedUserProfile = process.env.USERPROFILE; - const tempHome = path.join(os.tmpdir(), `gsd-remote-config-${Date.now()}-${Math.random().toString(36).slice(2)}`); - fs.mkdirSync(path.join(tempHome, ".gsd"), { recursive: true }); - process.env.HOME = tempHome; - process.env.USERPROFILE = tempHome; - process.env.SLACK_BOT_TOKEN = "token"; - - try { - const prefsPath = path.join(tempHome, ".gsd", "preferences.md"); - fs.writeFileSync(prefsPath, `---\nremote_questions:\n channel: slack\n channel_id: \"C123\"\n timeout_minutes: 999\n poll_interval_seconds: 0\n---\n`, "utf-8"); - - const config = resolveRemoteConfig(); - assert.ok(config); - assert.equal(config?.timeoutMs, 30 * 60 * 1000); - assert.equal(config?.pollIntervalMs, 2 * 1000); - } finally { - process.env.HOME = savedHome; - process.env.USERPROFILE = savedUserProfile; - delete process.env.SLACK_BOT_TOKEN; - fs.rmSync(tempHome, { recursive: true, force: true }); - } +test("isValidChannelId rejects invalid Slack channel IDs", () => { + // Too short + assert.equal(isValidChannelId("slack", "C123"), false); + // Contains invalid chars (URL injection) + assert.equal(isValidChannelId("slack", "https://evil.com"), false); + // Lowercase + assert.equal(isValidChannelId("slack", "c12345678"), false); + // Too long + assert.equal(isValidChannelId("slack", "C1234567890AB"), false); + // Valid: 9-12 uppercase alphanumeric + assert.equal(isValidChannelId("slack", "C12345678"), true); + assert.equal(isValidChannelId("slack", "C12345678AB"), true); + assert.equal(isValidChannelId("slack", "C1234567890A"), true); }); + +test("isValidChannelId rejects invalid Discord channel IDs", () => { + // Too short + assert.equal(isValidChannelId("discord", "12345"), false); + // Contains letters (not a snowflake) + assert.equal(isValidChannelId("discord", "abc12345678901234"), false); + // URL injection + assert.equal(isValidChannelId("discord", "https://evil.com"), false); + // Too long (21 digits) + assert.equal(isValidChannelId("discord", "123456789012345678901"), false); + // Valid: 17-20 digit snowflake + assert.equal(isValidChannelId("discord", "12345678901234567"), true); + assert.equal(isValidChannelId("discord", "11234567890123456789"), true); +}); + diff --git a/src/resources/extensions/remote-questions/config.ts b/src/resources/extensions/remote-questions/config.ts index 7fe7b7d2c..0b962c2e4 100644 --- a/src/resources/extensions/remote-questions/config.ts +++ b/src/resources/extensions/remote-questions/config.ts @@ -18,6 +18,12 @@ const ENV_KEYS: Record = { discord: "DISCORD_BOT_TOKEN", }; +// Channel ID format validation — prevents SSRF if preferences are attacker-controlled +const CHANNEL_ID_PATTERNS: Record = { + slack: /^[A-Z0-9]{9,12}$/, + discord: /^\d{17,20}$/, +}; + const DEFAULT_TIMEOUT_MINUTES = 5; const DEFAULT_POLL_INTERVAL_SECONDS = 5; const MIN_TIMEOUT_MINUTES = 1; @@ -31,6 +37,9 @@ export function resolveRemoteConfig(): ResolvedConfig | null { if (!rq || !rq.channel || !rq.channel_id) return null; if (rq.channel !== "slack" && rq.channel !== "discord") return null; + const channelId = String(rq.channel_id); + if (!CHANNEL_ID_PATTERNS[rq.channel].test(channelId)) return null; + const token = process.env[ENV_KEYS[rq.channel]]; if (!token) return null; @@ -39,7 +48,7 @@ export function resolveRemoteConfig(): ResolvedConfig | null { return { channel: rq.channel, - channelId: String(rq.channel_id), + channelId, timeoutMs: timeoutMinutes * 60 * 1000, pollIntervalMs: pollIntervalSeconds * 1000, token, @@ -51,6 +60,8 @@ export function getRemoteConfigStatus(): string { const rq: RemoteQuestionsConfig | undefined = prefs?.preferences.remote_questions; if (!rq || !rq.channel || !rq.channel_id) return "Remote questions: not configured"; if (rq.channel !== "slack" && rq.channel !== "discord") return `Remote questions: unknown channel type \"${rq.channel}\"`; + const channelId = String(rq.channel_id); + if (!CHANNEL_ID_PATTERNS[rq.channel].test(channelId)) return `Remote questions: invalid ${rq.channel} channel ID format`; const envVar = ENV_KEYS[rq.channel]; if (!process.env[envVar]) return `Remote questions: ${envVar} not set — remote questions disabled`; @@ -59,6 +70,10 @@ export function getRemoteConfigStatus(): string { return `Remote questions: ${rq.channel} configured (timeout ${timeoutMinutes}m, poll ${pollIntervalSeconds}s)`; } +export function isValidChannelId(channel: RemoteChannel, id: string): boolean { + return CHANNEL_ID_PATTERNS[channel].test(id); +} + function clampNumber(value: unknown, fallback: number, min: number, max: number): number { const n = typeof value === "number" ? value : Number(value); if (!Number.isFinite(n)) return fallback;