fix(security): validate channel ID format to prevent SSRF
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 <noreply@anthropic.com>
This commit is contained in:
parent
9b80c221ce
commit
41f362841e
2 changed files with 45 additions and 29 deletions
|
|
@ -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);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -18,6 +18,12 @@ const ENV_KEYS: Record<RemoteChannel, string> = {
|
|||
discord: "DISCORD_BOT_TOKEN",
|
||||
};
|
||||
|
||||
// Channel ID format validation — prevents SSRF if preferences are attacker-controlled
|
||||
const CHANNEL_ID_PATTERNS: Record<RemoteChannel, RegExp> = {
|
||||
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;
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue