From f4c46516a6055f807e898b89908ad09c3be9533b Mon Sep 17 00:00:00 2001 From: Lex Christopherson Date: Wed, 11 Mar 2026 17:32:01 -0600 Subject: [PATCH] =?UTF-8?q?fix:=20harden=20remote=20questions=20=E2=80=94?= =?UTF-8?q?=20validate=20IDs=20before=20test-send,=20remove=20dead=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Validate channel IDs via isValidChannelId() before URL interpolation in setup wizard, preventing SSRF during test-send - Add 15s fetch timeout to setup API calls (fetchJson, Discord test-send) - Sanitize Discord error responses before surfacing to user - Remove dead send.ts + channels.ts (unused parallel implementation) - Add poll retry tolerance in manager.ts (1 transient error before fail) Co-Authored-By: Claude Opus 4.6 --- .../extensions/remote-questions/channels.ts | 36 --- .../extensions/remote-questions/manager.ts | 9 +- .../remote-questions/remote-command.ts | 10 +- .../extensions/remote-questions/send.ts | 213 ------------------ 4 files changed, 14 insertions(+), 254 deletions(-) delete mode 100644 src/resources/extensions/remote-questions/channels.ts delete mode 100644 src/resources/extensions/remote-questions/send.ts diff --git a/src/resources/extensions/remote-questions/channels.ts b/src/resources/extensions/remote-questions/channels.ts deleted file mode 100644 index 7360c00a3..000000000 --- a/src/resources/extensions/remote-questions/channels.ts +++ /dev/null @@ -1,36 +0,0 @@ -/** - * Remote Questions — Adapter pattern interfaces - * - * Defines the contract for Slack/Discord (or any future) channel adapters. - */ - -export interface ChannelAdapter { - readonly name: string; - sendQuestions(questions: FormattedQuestion[]): Promise; - pollResponse(ref: PollReference): Promise; - validate(): Promise; -} - -export interface FormattedQuestion { - id: string; - header: string; - question: string; - options: Array<{ label: string; description: string }>; - allowMultiple: boolean; -} - -export interface SendResult { - ref: PollReference; - threadUrl?: string; -} - -export interface PollReference { - channelType: "slack" | "discord"; - messageId: string; - threadTs?: string; - channelId: string; -} - -export interface RemoteAnswer { - answers: Record; -} diff --git a/src/resources/extensions/remote-questions/manager.ts b/src/resources/extensions/remote-questions/manager.ts index 511668deb..f965a657c 100644 --- a/src/resources/extensions/remote-questions/manager.ts +++ b/src/resources/extensions/remote-questions/manager.ts @@ -122,14 +122,19 @@ async function pollUntilDone( ref: import("./types.js").RemotePromptRef, signal?: AbortSignal, ): Promise { + let retryCount = 0; while (Date.now() < prompt.timeoutAt && !signal?.aborted) { try { const answer = await adapter.pollAnswer(prompt, ref); updatePromptRecord(prompt.id, { lastPollAt: Date.now() }); + retryCount = 0; if (answer) return answer; } catch (err) { - markPromptStatus(prompt.id, "failed", sanitizeError(String((err as Error).message))); - return null; + retryCount++; + if (retryCount > 1) { + markPromptStatus(prompt.id, "failed", sanitizeError(String((err as Error).message))); + return null; + } } await sleep(prompt.pollIntervalMs, signal); diff --git a/src/resources/extensions/remote-questions/remote-command.ts b/src/resources/extensions/remote-questions/remote-command.ts index be5796ff2..356cab9ab 100644 --- a/src/resources/extensions/remote-questions/remote-command.ts +++ b/src/resources/extensions/remote-questions/remote-command.ts @@ -8,7 +8,8 @@ import { CURSOR_MARKER, Editor, type EditorTheme, Key, matchesKey, truncateToWid import { existsSync, readFileSync, writeFileSync, mkdirSync } from "node:fs"; import { dirname, join } from "node:path"; import { getGlobalGSDPreferencesPath, loadEffectiveGSDPreferences } from "../gsd/preferences.js"; -import { getRemoteConfigStatus, resolveRemoteConfig } from "./config.js"; +import { getRemoteConfigStatus, isValidChannelId, resolveRemoteConfig } from "./config.js"; +import { sanitizeError } from "./manager.js"; import { getLatestPromptSummary } from "./status.js"; export async function handleRemote( @@ -37,6 +38,7 @@ async function handleSetupSlack(ctx: ExtensionCommandContext): Promise { const channelId = await promptInput(ctx, "Channel ID", "Paste the Slack channel ID (e.g. C0123456789)"); if (!channelId) return void ctx.ui.notify("Slack setup cancelled.", "info"); + if (!isValidChannelId("slack", channelId)) return void ctx.ui.notify("Invalid Slack channel ID format — expected 9-12 uppercase alphanumeric characters.", "error"); const send = await fetchJson("https://slack.com/api/chat.postMessage", { method: "POST", @@ -61,15 +63,17 @@ async function handleSetupDiscord(ctx: ExtensionCommandContext): Promise { const channelId = await promptInput(ctx, "Channel ID", "Paste the Discord channel ID (e.g. 1234567890123456789)"); if (!channelId) return void ctx.ui.notify("Discord setup cancelled.", "info"); + if (!isValidChannelId("discord", channelId)) return void ctx.ui.notify("Invalid Discord channel ID format — expected 17-20 digit numeric ID.", "error"); const sendResponse = await fetch(`https://discord.com/api/v10/channels/${channelId}/messages`, { method: "POST", headers: { Authorization: `Bot ${token}`, "Content-Type": "application/json" }, body: JSON.stringify({ content: "GSD remote questions connected." }), + signal: AbortSignal.timeout(15_000), }); if (!sendResponse.ok) { const body = await sendResponse.text().catch(() => ""); - return void ctx.ui.notify(`Could not send to channel (HTTP ${sendResponse.status}): ${body}`, "error"); + return void ctx.ui.notify(`Could not send to channel (HTTP ${sendResponse.status}): ${sanitizeError(body).slice(0, 200)}`, "error"); } saveProviderToken("discord_bot", token); @@ -138,7 +142,7 @@ async function handleRemoteMenu(ctx: ExtensionCommandContext): Promise { async function fetchJson(url: string, init?: RequestInit): Promise { try { - const response = await fetch(url, init); + const response = await fetch(url, { ...init, signal: AbortSignal.timeout(15_000) }); return await response.json(); } catch { return null; diff --git a/src/resources/extensions/remote-questions/send.ts b/src/resources/extensions/remote-questions/send.ts deleted file mode 100644 index 90d6e293b..000000000 --- a/src/resources/extensions/remote-questions/send.ts +++ /dev/null @@ -1,213 +0,0 @@ -/** - * Remote Questions — Entry point - * - * Transparent routing: when ctx.hasUI is false and a remote channel is - * configured, sends questions via Slack/Discord and polls for the response. - * - * The LLM keeps calling `ask_user_questions` as normal — this module - * intercepts the non-interactive branch. - */ - -import type { FormattedQuestion, ChannelAdapter, RemoteAnswer } from "./channels.js"; -import { resolveRemoteConfig, type ResolvedConfig } from "./config.js"; -import { SlackAdapter } from "./slack-adapter.js"; -import { DiscordAdapter } from "./discord-adapter.js"; - -// ─── Types ─────────────────────────────────────────────────────────────────── - -interface Question { - id: string; - header: string; - question: string; - options: Array<{ label: string; description: string }>; - allowMultiple?: boolean; -} - -interface ToolResult { - content: Array<{ type: "text"; text: string }>; - details?: Record; -} - -// ─── Public API ────────────────────────────────────────────────────────────── - -/** - * Try to send questions via a remote channel (Slack/Discord). - * Returns a formatted ToolResult if successful, or null if no remote - * channel is configured (caller falls back to the original error). - */ -export async function tryRemoteQuestions( - questions: Question[], - signal?: AbortSignal, -): Promise { - const config = resolveRemoteConfig(); - if (!config) return null; - - const adapter = createAdapter(config); - const formatted = questionsToFormatted(questions); - - try { - await adapter.validate(); - } catch (err) { - return errorToolResult(`Remote auth failed (${config.channel}): ${(err as Error).message}`); - } - - let sendResult; - try { - sendResult = await adapter.sendQuestions(formatted); - } catch (err) { - return errorToolResult(`Failed to send questions via ${config.channel}: ${(err as Error).message}`); - } - - const threadInfo = sendResult.threadUrl - ? ` Thread: ${sendResult.threadUrl}` - : ""; - - // Poll for response - const answer = await pollWithTimeout(adapter, sendResult.ref, formatted, signal, config); - - if (!answer) { - // Timeout — return structured result so the LLM knows - return { - content: [ - { - type: "text", - text: JSON.stringify({ - timed_out: true, - channel: config.channel, - timeout_minutes: config.timeoutMs / 60000, - thread_url: sendResult.threadUrl ?? null, - message: `User did not respond within ${config.timeoutMs / 60000} minutes.${threadInfo}`, - }), - }, - ], - details: { - remote: true, - channel: config.channel, - timed_out: true, - threadUrl: sendResult.threadUrl, - }, - }; - } - - // Format the answer in the same structure as formatForLLM - const formattedAnswer = formatRemoteAnswerForLLM(answer); - - return { - content: [{ type: "text", text: formattedAnswer }], - details: { - remote: true, - channel: config.channel, - timed_out: false, - threadUrl: sendResult.threadUrl, - questions, - response: answer, - }, - }; -} - -// ─── Internal ──────────────────────────────────────────────────────────────── - -function createAdapter(config: ResolvedConfig): ChannelAdapter & { - pollResponseWithQuestions?: ( - ref: import("./channels.js").PollReference, - questions: FormattedQuestion[], - ) => Promise; -} { - switch (config.channel) { - case "slack": - return new SlackAdapter(config.token, config.channelId); - case "discord": - return new DiscordAdapter(config.token, config.channelId); - default: - throw new Error(`Unknown channel type: ${config.channel}`); - } -} - -async function pollWithTimeout( - adapter: ReturnType, - ref: import("./channels.js").PollReference, - questions: FormattedQuestion[], - signal: AbortSignal | undefined, - config: ResolvedConfig, -): Promise { - const deadline = Date.now() + config.timeoutMs; - let retries = 0; - const maxNetworkRetries = 1; - - while (Date.now() < deadline && !signal?.aborted) { - try { - // Use the question-aware poll if available - const answer = adapter.pollResponseWithQuestions - ? await adapter.pollResponseWithQuestions(ref, questions) - : await adapter.pollResponse(ref); - - if (answer) return answer; - retries = 0; // Reset on successful poll - } catch { - retries++; - if (retries > maxNetworkRetries) return null; - } - - await sleep(config.pollIntervalMs, signal); - } - - return null; -} - -function sleep(ms: number, signal?: AbortSignal): Promise { - return new Promise((resolve) => { - if (signal?.aborted) { - resolve(); - return; - } - - let settled = false; - const settle = () => { - if (settled) return; - settled = true; - clearTimeout(timer); - if (signal) signal.removeEventListener("abort", onAbort); - resolve(); - }; - - const onAbort = () => settle(); - const timer = setTimeout(() => settle(), ms); - - if (signal) { - signal.addEventListener("abort", onAbort, { once: true }); - } - }); -} - -function questionsToFormatted(questions: Question[]): FormattedQuestion[] { - return questions.map((q) => ({ - id: q.id, - header: q.header, - question: q.question, - options: q.options, - allowMultiple: q.allowMultiple ?? false, - })); -} - -/** - * Format RemoteAnswer into the same JSON structure as the local formatForLLM. - * Structure: { answers: { [id]: { answers: string[] } } } - */ -function formatRemoteAnswerForLLM(answer: RemoteAnswer): string { - const formatted: Record = {}; - for (const [id, data] of Object.entries(answer.answers)) { - const list = [...data.answers]; - if (data.user_note) { - list.push(`user_note: ${data.user_note}`); - } - formatted[id] = { answers: list }; - } - return JSON.stringify({ answers: formatted }); -} - -function errorToolResult(message: string): ToolResult { - return { - content: [{ type: "text", text: message }], - details: { remote: true, error: true }, - }; -}