From b3bbbcffade21b31ed4ccaa4e4d9c268007fc251 Mon Sep 17 00:00:00 2001 From: unohee Date: Mon, 28 Sep 2026 15:16:14 +0900 Subject: [PATCH] fix(boundaries): harden outbound DNS pinning, GraphQL cost limits, and CLI/provider validation Salvages the boundary work from draft PRs #764, #774 and #778 onto current main. From #764: - support/outboundUrl: resolvePublicHttpUrl returns the validated addresses, createPinnedPublicLookup answers the connect hook from that same set (no second DNS round-trip), and publicFetch installs a per-request pinned dispatcher instead of a shared unpinned agent. - issues/graphql/server: GRAPHQL_MAX_DEPTH/FIELD_COUNT/ALIAS_COUNT/COST plus createGraphQLCostRule, and exact-path '/graphql' matching. - mcp/mcpClient: MAX_INPUT_SCHEMA_BYTES/PROPERTIES with countSchemaProperties. - auth/oauthPkce: isLoopbackRemote guard on the callback server (+ re-export). - verify/runner: buildVerifyToolchainPath replaces the inherited sandbox PATH. - adapters/webTools: cancel the redirect body before the next hop. From #774: - issues/graphql/costAnalysis: registry CRUD costs (registerEntity 100, updateEntity/removeEntity 80, addEntityRelation/removeEntityRelation 60). - issues/graphql/server: applyCors returns early without an Origin header. - bulkRegisterEntities alias/fragment-multiplication tests. From #778: - adapters/rateLimitError: parseRetryAfterSeconds handles HTTP-date Retry-After; classifyLimitResponse falls back to the codex reset epoch. - cli/mcpCommand: preset/url/command validation before persisting the registry. - cli/prCreate: fail closed on a dirty tree or a branch with no upstream. - adapters/webTools: refuse non-http(s) redirect destinations. Kept main's newer clone-timeout/resource-budget code in verify/runner and its onValidate cost plugin; dropped the already-in-main #774 files and all scratch probe files. --- src/adapters/rateLimitError.test.ts | 57 +++++++++- src/adapters/rateLimitError.ts | 45 +++++++- src/adapters/webTools.test.ts | 63 +++++++++++ src/adapters/webTools.ts | 25 ++++- src/auth/index.ts | 1 + src/auth/oauthPkce.test.ts | 27 +++++ src/auth/oauthPkce.ts | 31 ++++++ src/cli/mcpCommand.test.ts | 25 +++++ src/cli/mcpCommand.ts | 27 ++++- src/cli/prCreate.test.ts | 45 +++++--- src/cli/prCreate.ts | 47 ++++++-- src/issues/graphql/costAnalysis.ts | 13 +++ src/issues/graphql/server.test.ts | 165 +++++++++++++++++++++++++++- src/issues/graphql/server.ts | 104 +++++++++++++++++- src/mcp/mcpClient.test.ts | 33 ++++++ src/mcp/mcpClient.ts | 43 +++++++- src/support/outboundUrl.test.ts | 44 +++++++- src/support/outboundUrl.ts | 71 ++++++++---- src/verify/runner.test.ts | 54 ++++++++- src/verify/runner.ts | 94 ++++++++++++++-- 20 files changed, 935 insertions(+), 79 deletions(-) create mode 100644 src/auth/oauthPkce.test.ts diff --git a/src/adapters/rateLimitError.test.ts b/src/adapters/rateLimitError.test.ts index afd9563a..863aebcf 100644 --- a/src/adapters/rateLimitError.test.ts +++ b/src/adapters/rateLimitError.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { classifyLimitResponse, detectRateLimit, rateLimitFromCodexHeaders, rateLimitFromHttpResponse, matchesRateLimitMessage, RateLimitError } from './rateLimitError.js'; +import { classifyLimitResponse, detectRateLimit, parseRetryAfterSeconds, rateLimitFromCodexHeaders, rateLimitFromHttpResponse, matchesRateLimitMessage, RateLimitError } from './rateLimitError.js'; import { resolveLimitResponse, throttleWaitMs } from './throttleRetry.js'; import { isInfraError } from './errorClassification.js'; import { runAgenticLoop } from './agenticLoop.js'; @@ -21,6 +21,9 @@ describe('per-provider usage-limit recognition (INT-2520 audit)', () => { ['OpenRouter 402 relaying upstream BYOK balance', '{"error":{"message":"Provider returned error","code":402,"metadata":{"raw":"{\\"code\\":402,\\"msg\\":\\"insufficient balance\\"}","provider_name":"AtlasCloud","is_byok":true}}}'], ['HTTP 429 too many requests (local)', 'Local API error (429): Too Many Requests'], + ['OpenRouter 429 prose', 'Rate limit exceeded: 1000 requests per 1 day'], + ['local 429 overloaded', '{"error":"Too Many Requests: server is overloaded"}'], + ['local overloaded body', '{"error":"server is overloaded"}'], ]; for (const [name, output] of REAL_LIMIT_OUTPUTS) { it(`detects: ${name}`, () => { @@ -42,6 +45,9 @@ describe('per-provider usage-limit recognition (INT-2520 audit)', () => { // anchored on the provider's JSON key rather than added as a bare substring. "throw new Error('Insufficient balance') // wallet guard", 'if (res.status === 402) throw new Error("Insufficient balance for this transfer");', + // A 529 capacity blip is a single word, and it must stay an infra backoff: + // matching the bare word here would make every overload pause the scheduler. + 'Anthropic API error: overloaded', ]; for (const b of benign) { expect(matchesRateLimitMessage(b)).toBe(false); @@ -379,6 +385,55 @@ describe('resolveLimitResponse gating (INT-2907)', () => { }); }); +describe('Retry-After parsing (AGT-3442)', () => { + it('parses delta-seconds and HTTP-date Retry-After values', () => { + expect(parseRetryAfterSeconds('120')).toBe(120); + expect(parseRetryAfterSeconds(' 45 ')).toBe(45); + // Prefix digits must not silently win over a malformed token. + expect(parseRetryAfterSeconds('60xyz')).toBeUndefined(); + expect(parseRetryAfterSeconds('not-a-date')).toBeUndefined(); + + const future = new Date(Date.now() + 180_000); + const before = Math.floor(Date.now() / 1000); + const seconds = parseRetryAfterSeconds(future.toUTCString()); + const after = Math.floor(Date.now() / 1000); + expect(seconds).toBeDefined(); + const expected = Math.floor(future.getTime() / 1000); + expect(seconds!).toBeGreaterThanOrEqual(expected - after); + expect(seconds!).toBeLessThanOrEqual(expected - before); + }); + + it('surfaces an HTTP-date Retry-After as seconds-from-now via classifyLimitResponse', () => { + // Relative future date so the assertion does not rot as wall-clock moves. + const future = new Date(Date.now() + 120_000); + const headers = new Headers({ 'retry-after': future.toUTCString() }); + const before = Math.floor(Date.now() / 1000); + const result = classifyLimitResponse(headers, '{}'); + const after = Math.floor(Date.now() / 1000); + expect(result.quota).toBe(false); // no quota-exhausted body signature + const expected = Math.floor(future.getTime() / 1000); + expect(result.retryAfterSeconds!).toBeGreaterThanOrEqual(expected - after); + expect(result.retryAfterSeconds!).toBeLessThanOrEqual(expected - before); + }); + + it('rejects an unusable Retry-After instead of inventing a wait', () => { + // A date-shaped-but-invalid token must not become a NaN/0-second pause. + expect(classifyLimitResponse(new Headers({ 'retry-after': 'Fri, 99 Foo 9999' }), '').retryAfterSeconds) + .toBeUndefined(); + }); + + it('sets RateLimitError.resetsAt from an HTTP-date Retry-After on a 429', () => { + const future = new Date(Date.now() + 300_000); + const headers = new Headers({ 'retry-after': future.toUTCString() }); + const err = rateLimitFromHttpResponse(429, headers, '{"error":"rate limit"}'); + expect(err).toBeInstanceOf(RateLimitError); + const expected = Math.floor(future.getTime() / 1000); + // ±1s: parseRetryAfterSeconds and extractResetsAt each sample Date.now(). + expect(err!.resetsAt).toBeGreaterThanOrEqual(expected - 1); + expect(err!.resetsAt).toBeLessThanOrEqual(expected + 1); + }); +}); + describe('throttle backoff + downstream classification (INT-2907)', () => { it('honors Retry-After, caps it, and otherwise escalates the backoff', () => { // Backoff carries up to 1s of jitter so concurrent subagents don't retry in lockstep. diff --git a/src/adapters/rateLimitError.ts b/src/adapters/rateLimitError.ts index eb1b27d2..313927cf 100644 --- a/src/adapters/rateLimitError.ts +++ b/src/adapters/rateLimitError.ts @@ -60,6 +60,12 @@ const RATE_LIMIT_SUBSTRINGS: readonly string[] = [ 'purchase more credits', // codex CLI stdout error event 'exceeded your current quota',// OpenAI insufficient_quota human message 'too many requests', // HTTP 429 standard reason (local/lmstudio/others) + 'rate limit exceeded', // OpenRouter/OpenAI 429 prose ("Rate limit exceeded: 1000 requests per 1 day") + // local/lmstudio 429 body ("server is overloaded"). Deliberately NOT the bare + // word "overloaded": Anthropic/OpenRouter report a 529 capacity blip with that + // exact single word, and that is an infra backoff, not a scheduler pause — + // matching it here would re-bucket every 529 ahead of isInfraError. + 'server is overloaded', ]; // Regex signatures that need structure (co-occurrence / numeric context) to stay @@ -110,6 +116,23 @@ export function parseResetsAtFromBody(text: string): number | undefined { return m ? parseInt(m[1], 10) : undefined; } +/** + * RFC 7231 §7.1.3: Retry-After is either 1*DIGIT delta-seconds or an HTTP-date. + * Returns seconds-from-now when parseable; undefined when the value is unusable. + */ +export function parseRetryAfterSeconds(value: string): number | undefined { + const trimmed = value.trim(); + // Delta-seconds must be the entire token — parseInt("Fri, …") is NaN, but + // parseInt("60xyz") would silently accept a prefix, so require /^\d+$/. + if (/^\d+$/.test(trimmed)) { + const delta = parseInt(trimmed, 10); + return Number.isFinite(delta) ? delta : undefined; + } + const dateMs = Date.parse(trimmed); + if (!Number.isFinite(dateMs)) return undefined; + return Math.max(0, Math.floor(dateMs / 1000) - Math.floor(Date.now() / 1000)); +} + /** Pull a unix reset timestamp (seconds) out of headers or a JSON body, if present. */ function extractResetsAt(headers: Headers | undefined, body: string): number | undefined { const fromHeader = (k: string): number | undefined => { @@ -119,7 +142,7 @@ function extractResetsAt(headers: Headers | undefined, body: string): number | u }; // Only headers/fields that are genuinely UNIX-epoch seconds or seconds-from-now: // - x-codex-primary-reset-at: epoch seconds - // - Retry-After: seconds-from-now (→ convert to epoch) + // - Retry-After: seconds-from-now (→ convert to epoch) OR an HTTP-date // - body "resets_at": epoch seconds // Deliberately NOT x-ratelimit-reset-requests/-tokens: OpenAI returns those as // DURATION strings ("1s", "6ms", "2m59s"), not epoch — parseInt would yield a @@ -127,8 +150,13 @@ function extractResetsAt(headers: Headers | undefined, body: string): number | u // 60s default, which is correct rather than wrong. (INT-2520 review) const codexReset = fromHeader('x-codex-primary-reset-at'); if (codexReset != null) return codexReset; - const retryAfter = fromHeader('retry-after'); - if (retryAfter != null) return Math.floor(Date.now() / 1000) + retryAfter; + const retryAfter = headers?.get('retry-after'); + if (retryAfter != null) { + // RFC 7231 §7.1.3 via parseRetryAfterSeconds (delta-seconds or HTTP-date). + // Convert seconds-from-now → absolute epoch for RateLimitError.resetsAt. + const seconds = parseRetryAfterSeconds(retryAfter); + if (seconds != null) return Math.floor(Date.now() / 1000) + seconds; + } return parseResetsAtFromBody(body); } @@ -201,7 +229,16 @@ export function classifyLimitResponse(headers: Headers | undefined, body: string return Number.isFinite(n) ? n : undefined; }; const usedPercent = num('x-codex-primary-used-percent'); - const retryAfterSeconds = num('retry-after'); + // Retry-After is RFC 7231 delta-seconds OR an HTTP-date; parseInt on a date + // yields NaN and silently drops the server's wait. Fall back to the codex + // absolute reset epoch, converted to seconds-from-now. (AGT-3442) + let retryAfterSeconds: number | undefined; + const retryAfterHeader = headers?.get('retry-after'); + if (retryAfterHeader != null) retryAfterSeconds = parseRetryAfterSeconds(retryAfterHeader); + if (retryAfterSeconds == null) { + const resetAt = num('x-codex-primary-reset-at'); + if (resetAt != null) retryAfterSeconds = Math.max(0, resetAt - Math.floor(Date.now() / 1000)); + } const lower = body.toLowerCase(); const quota = QUOTA_EXHAUSTED_SUBSTRINGS.some((s) => lower.includes(s)) || diff --git a/src/adapters/webTools.test.ts b/src/adapters/webTools.test.ts index 2bad1d45..aa793f9f 100644 --- a/src/adapters/webTools.test.ts +++ b/src/adapters/webTools.test.ts @@ -58,6 +58,69 @@ describe('redirect method rewriting', () => { expect(second.method).toBe('POST'); expect(second.body).toBeTruthy(); }); + + it('cancels the redirect response body before following the next hop', async () => { + const cancel = vi.fn(async () => {}); + let calls = 0; + const f = vi.fn(async () => { + calls += 1; + if (calls === 1) { + return { + status: 302, + ok: false, + statusText: 'Found', + headers: new Headers({ location: 'https://example.com/next' }), + body: { cancel }, + arrayBuffer: async () => new ArrayBuffer(0), + } as unknown as Response; + } + return new Response('ok', { status: 200, headers: { 'content-type': 'text/plain' } }); + }); + vi.stubGlobal('fetch', f); + await webFetch('https://example.com/start'); + expect(cancel).toHaveBeenCalledOnce(); + expect(f).toHaveBeenCalledTimes(2); + }); +}); + +describe('redirect destination validation', () => { + it('refuses to forward credentials or a request body across origins', async () => { + const f = vi.fn(async () => + new Response(null, { status: 302, headers: { location: 'https://evil.example/collect' } }), + ); + vi.stubGlobal('fetch', f); + vi.stubEnv('TAVILY_KEY', 'secret-key'); + const out = await webSearch('q', 1); + expect(out).toContain('Search failed'); + expect(out).toMatch(/credentials or request body across origins/i); + // Only the first hop — never followed the cross-origin Location. + expect(f).toHaveBeenCalledTimes(1); + }); + + it('refuses a non-http(s) redirect Location before following', async () => { + const f = vi.fn(async () => + new Response(null, { status: 302, headers: { location: 'file:///etc/passwd' } }), + ); + vi.stubGlobal('fetch', f); + const out = await webFetch('https://example.com/start'); + expect(out).toMatch(/Refusing redirect to non-http/i); + expect(f).toHaveBeenCalledTimes(1); + }); + + it('allows a same-origin redirect that carries a request body', async () => { + let calls = 0; + const f = vi.fn(async () => { + calls += 1; + return calls === 1 + ? new Response(null, { status: 307, headers: { location: 'https://api.tavily.com/next' } }) + : new Response(JSON.stringify({ results: [] }), { status: 200 }); + }); + vi.stubGlobal('fetch', f); + vi.stubEnv('TAVILY_KEY', 'k'); + const out = await webSearch('q', 1); + expect(out).toContain('No results'); + expect(f).toHaveBeenCalledTimes(2); + }); }); describe('webFetch', () => { diff --git a/src/adapters/webTools.ts b/src/adapters/webTools.ts index ed9e7155..8ca73281 100644 --- a/src/adapters/webTools.ts +++ b/src/adapters/webTools.ts @@ -128,8 +128,29 @@ async function fetchWithTimeout( if ([301, 302, 303, 307, 308].includes(response.status)) { const location = response.headers.get('location'); if (!location) throw new Error('Redirect response has no location'); - await response.body?.cancel(); - const next = new URL(location, current); + // Release the redirect body before the next hop so undici does not keep + // the prior socket/buffer alive across a chain of Location responses. + if (response.body) { + try { + await response.body.cancel(); + } catch { + // A body that cannot be cancelled (already locked/consumed) must not + // abort the hop; draining it releases the socket the same way. + await response.arrayBuffer().catch(() => undefined); + } + } + let next: URL; + try { + next = new URL(location, current); + } catch { + throw new Error(`Invalid redirect location: ${location}`); + } + // Validate the hop before following: only http(s) destinations are + // eligible (publicFetch would also reject, but fail closed here so a + // credentialed request never even attempts a file:/javascript: target). + if (next.protocol !== 'http:' && next.protocol !== 'https:') { + throw new Error(`Refusing redirect to non-http(s) URL (${next.protocol})`); + } if (carriesSensitiveRequestData && next.origin !== initialOrigin) { throw new Error('Refusing to forward credentials or request body across origins'); } diff --git a/src/auth/index.ts b/src/auth/index.ts index 2fc942b9..fb60f681 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -7,6 +7,7 @@ export { runOAuthPkceFlow, loginAndSaveProfile, DEFAULT_OPENAI_CLIENT_ID, + isLoopbackRemote, type OAuthFlowResult, type OAuthFlowOptions, } from './oauthPkce.js'; diff --git a/src/auth/oauthPkce.test.ts b/src/auth/oauthPkce.test.ts new file mode 100644 index 00000000..268499f7 --- /dev/null +++ b/src/auth/oauthPkce.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from 'vitest'; +import { isLoopbackRemote } from './oauthPkce.js'; + +/** + * The OpenAI redirect_uri is always the advertised localhost form. Callbacks may + * still arrive over IPv4 127.0.0.1 or IPv6 ::1, and Node reports an IPv4 client + * on a dual-stack socket in the IPv4-mapped form. + */ +describe('isLoopbackRemote (OpenAI PKCE callback)', () => { + it('accepts the IPv4 and IPv6 loopback remotes Node reports', () => { + expect(isLoopbackRemote('127.0.0.1')).toBe(true); + expect(isLoopbackRemote('::1')).toBe(true); + expect(isLoopbackRemote('::ffff:127.0.0.1')).toBe(true); + }); + + it('rejects non-loopback remotes that must never drive the callback', () => { + expect(isLoopbackRemote(undefined)).toBe(false); + expect(isLoopbackRemote('')).toBe(false); + expect(isLoopbackRemote('10.0.0.1')).toBe(false); + expect(isLoopbackRemote('192.168.1.1')).toBe(false); + expect(isLoopbackRemote('8.8.8.8')).toBe(false); + expect(isLoopbackRemote('fe80::1')).toBe(false); + expect(isLoopbackRemote('2001:db8::1')).toBe(false); + // A mapped non-loopback address is still a remote host. + expect(isLoopbackRemote('::ffff:10.0.0.1')).toBe(false); + }); +}); diff --git a/src/auth/oauthPkce.ts b/src/auth/oauthPkce.ts index c5f22d4c..86a765c1 100644 --- a/src/auth/oauthPkce.ts +++ b/src/auth/oauthPkce.ts @@ -4,6 +4,7 @@ // ============================================ import { createServer, type IncomingMessage, type ServerResponse } from 'node:http'; +import { isIP } from 'node:net'; import { randomBytes, createHash } from 'node:crypto'; import { AuthProfileStore, type AuthProfile } from './oauthStore.js'; import { openBrowser } from './openBrowser.js'; @@ -26,6 +27,26 @@ const PROFILE_KEY = 'openai-gpt:default'; export const DEFAULT_OPENAI_CLIENT_ID = 'app_EMoamEEZ73f0CkXaXp7hrann'; const OAUTH_ORIGINATOR = 'openswarm'; +/** + * Accept only the advertised loopback callback forms: IPv4 127.0.0.1 and IPv6 + * ::1 (including the IPv4-mapped ::ffff:127.0.0.1 form Node reports when a + * dual-stack socket receives an IPv4 connection). Any other remote address is + * rejected so the callback server cannot be driven from off-host. + * + * The OpenAI redirect_uri stays `http://localhost:/auth/callback` (the + * registered client value); browsers may still connect via ::1 or 127.0.0.1. + */ +export function isLoopbackRemote(address: string | undefined): boolean { + if (!address) return false; + const family = isIP(address); + if (family === 4) return address === '127.0.0.1'; + if (family === 6) { + const lower = address.toLowerCase(); + return lower === '::1' || lower === '::ffff:127.0.0.1'; + } + return false; +} + // PKCE helpers function generateCodeVerifier(): string { @@ -113,6 +134,12 @@ export async function runOAuthPkceFlow(options: OAuthFlowOptions = {}): Promise< return; } + if (!isLoopbackRemote(req.socket.remoteAddress)) { + res.writeHead(403); + res.end('Forbidden'); + return; + } + const url = new URL(req.url ?? '/', `http://127.0.0.1:${port}`); if (url.pathname !== '/auth/callback') { @@ -236,6 +263,10 @@ export async function runOAuthPkceFlow(options: OAuthFlowOptions = {}): Promise< } }); + // Bound to the IPv4 loopback, not 'localhost': listen('localhost') resolves + // the name ONCE and binds whichever family wins (measured on macOS: ::1 + // only, so 127.0.0.1 clients get ECONNREFUSED). isLoopbackRemote still + // accepts the IPv6 and IPv4-mapped forms so a dual-stack bind stays correct. server.listen(port, '127.0.0.1', () => { console.log(`[Auth] Callback server listening on http://127.0.0.1:${port}`); console.log(`[Auth] 브라우저에서 OpenAI 로그인 페이지를 엽니다...`); diff --git a/src/cli/mcpCommand.test.ts b/src/cli/mcpCommand.test.ts index a0a3d50a..9351dc3b 100644 --- a/src/cli/mcpCommand.test.ts +++ b/src/cli/mcpCommand.test.ts @@ -28,6 +28,31 @@ describe('parseServerSpec (INT-1953)', () => { it('throws when nothing usable is given', () => { expect(() => parseServerSpec(undefined, [])).toThrow(); }); + it('rejects an unknown preset name before persistence (AGT-3442)', () => { + expect(() => parseServerSpec(undefined, [], 'not-a-real-preset')).toThrow(/unknown preset/); + }); + it('rejects a malformed http(s) URL before persistence (AGT-3442)', () => { + expect(() => parseServerSpec('https://exa mple.com/mcp', [])).toThrow(/invalid server URL/); + }); + it('rejects an empty server name before persistence (AGT-3442)', () => { + expect(() => addServer({ mcpServers: {} }, ' ', { command: 'x' })).toThrow(/non-empty server name/); + }); + it('does not write the registry when add validation fails (AGT-3442)', () => { + const dir = mkdtempSync(join(tmpdir(), 'mcpcmd-')); + const path = join(dir, 'mcp.json'); + try { + expect(() => + runMcpCommand('add', 'bad', ['https://exa mple.com/mcp'], { path }), + ).toThrow(/invalid server URL/); + expect(existsSync(path)).toBe(false); + expect(() => + runMcpCommand('add', 'bad', [], { preset: 'not-a-real-preset', path }), + ).toThrow(/unknown preset/); + expect(existsSync(path)).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); }); describe('addServer / removeServer / formatServerList', () => { diff --git a/src/cli/mcpCommand.ts b/src/cli/mcpCommand.ts index 6638b8ba..ff690da0 100644 --- a/src/cli/mcpCommand.ts +++ b/src/cli/mcpCommand.ts @@ -45,13 +45,34 @@ export function writeMcpJson(json: McpJson, path = MCP_JSON_PATH): void { * or a command + args (stdio). */ export function parseServerSpec(target: string | undefined, args: string[], preset?: string): McpServerConfig { - if (preset) return { preset }; - if (target && /^https?:\/\//.test(target)) return { url: target }; - if (target) return { command: target, ...(args.length ? { args } : {}) }; + if (preset) { + if (!BUILTIN_MCP_SERVERS[preset]) { + const known = Object.keys(BUILTIN_MCP_SERVERS).join(', '); + throw new Error(`mcp add: unknown preset "${preset}" (known: ${known})`); + } + return { preset }; + } + if (target && /^https?:\/\//.test(target)) { + // Validate before it is persisted: a malformed URL would only fail at connect + // time, long after the registry entry looked accepted. + const parsed = McpServerSchema.safeParse({ url: target }); + if (!parsed.success) { + throw new Error(`mcp add: invalid server URL "${target}": ${parsed.error.issues[0]?.message ?? 'malformed'}`); + } + return parsed.data as McpServerConfig; + } + if (target) { + const parsed = McpServerSchema.safeParse({ command: target, ...(args.length ? { args } : {}) }); + if (!parsed.success) { + throw new Error(`mcp add: invalid command spec: ${parsed.error.issues[0]?.message ?? 'malformed'}`); + } + return parsed.data as McpServerConfig; + } throw new Error('mcp add: provide a --preset, a URL, or a command'); } export function addServer(json: McpJson, name: string, spec: McpServerConfig): McpJson { + if (!name.trim()) throw new Error('mcp add: a non-empty server name is required'); return { mcpServers: { ...json.mcpServers, [name]: spec } }; } diff --git a/src/cli/prCreate.test.ts b/src/cli/prCreate.test.ts index ae3f28c2..7bc5ef7f 100644 --- a/src/cli/prCreate.test.ts +++ b/src/cli/prCreate.test.ts @@ -107,26 +107,22 @@ describe('createPrFromCwd (INT-3282)', () => { expect(title).toBe('fix: the thing'); }); - it('default currentBranch/hasDirtyOrAhead shell out to git when not injected', async () => { + it('default currentBranch refuses a dirty working tree before publishing', async () => { execImpl .mockResolvedValueOnce({ stdout: 'feat/ship\n', stderr: '' }) // rev-parse --abbrev-ref HEAD - .mockResolvedValueOnce({ stdout: ' M src/x.ts\n', stderr: '' }) // status --porcelain (dirty) - .mockResolvedValueOnce({ stdout: 'chore: wip\n', stderr: '' }); // log -1 --pretty=%s + .mockResolvedValueOnce({ stdout: ' M src/x.ts\n', stderr: '' }); // status --porcelain (dirty) const commitAndCreate = vi.fn(async () => 'https://example.com/pr/2'); - const result = await createPrFromCwd({ fix: false }, { commitAndCreate }); - expect(result.url).toContain('/pr/2'); - expect(commitAndCreate).toHaveBeenCalledWith( - expect.objectContaining({ branchName: 'feat/ship' }), - 'chore: wip', - 'local', - expect.any(String), + await expect(createPrFromCwd({ fix: false }, { commitAndCreate })).rejects.toThrow( + /Uncommitted changes/, ); + expect(commitAndCreate).not.toHaveBeenCalled(); }); - it('default hasDirtyOrAhead falls back to rev-list ahead-count when the tree is clean', async () => { + it('default path counts commits when the tree is clean and the branch is published', async () => { execImpl - .mockResolvedValueOnce({ stdout: 'feat/ship\n', stderr: '' }) // rev-parse + .mockResolvedValueOnce({ stdout: 'feat/ship\n', stderr: '' }) // rev-parse --abbrev-ref HEAD .mockResolvedValueOnce({ stdout: '', stderr: '' }) // status --porcelain (clean) + .mockResolvedValueOnce({ stdout: 'origin/feat/ship\n', stderr: '' }) // rev-parse @{u} .mockResolvedValueOnce({ stdout: '2\n', stderr: '' }) // rev-list --count @{u}..HEAD .mockResolvedValueOnce({ stdout: 'chore: wip\n', stderr: '' }); // log -1 --pretty=%s const commitAndCreate = vi.fn(async () => 'https://example.com/pr/3'); @@ -134,13 +130,28 @@ describe('createPrFromCwd (INT-3282)', () => { expect(result.url).toContain('/pr/3'); }); - it('default hasDirtyOrAhead treats a clean tree with no upstream and no commits as nothing to publish', async () => { + it('fails clearly when the feature branch has no upstream', async () => { execImpl - .mockResolvedValueOnce({ stdout: 'feat/ship\n', stderr: '' }) // rev-parse + .mockResolvedValueOnce({ stdout: 'feat/ship\n', stderr: '' }) // rev-parse --abbrev-ref HEAD .mockResolvedValueOnce({ stdout: '', stderr: '' }) // status --porcelain (clean) - .mockRejectedValueOnce(new Error('no upstream')) // rev-list fails (no @{u}) - .mockResolvedValueOnce({ stdout: '', stderr: '' }); // log --oneline -1 (nothing) + .mockRejectedValueOnce(new Error('no upstream')); // rev-parse @{u} const commitAndCreate = vi.fn(async () => 'https://example.com/pr/4'); - await expect(createPrFromCwd({ fix: false }, { commitAndCreate })).rejects.toThrow(/Nothing to publish/); + await expect(createPrFromCwd({ fix: false }, { commitAndCreate })).rejects.toThrow( + /no upstream/i, + ); + expect(commitAndCreate).not.toHaveBeenCalled(); + }); + + it('fails clearly when the tree is clean but not ahead of upstream', async () => { + execImpl + .mockResolvedValueOnce({ stdout: 'feat/ship\n', stderr: '' }) // rev-parse --abbrev-ref HEAD + .mockResolvedValueOnce({ stdout: '', stderr: '' }) // status --porcelain (clean) + .mockResolvedValueOnce({ stdout: 'origin/feat/ship\n', stderr: '' }) // rev-parse @{u} + .mockResolvedValueOnce({ stdout: '0\n', stderr: '' }); // rev-list --count @{u}..HEAD + const commitAndCreate = vi.fn(async () => 'https://example.com/pr/5'); + await expect(createPrFromCwd({ fix: false }, { commitAndCreate })).rejects.toThrow( + /Nothing to publish/, + ); + expect(commitAndCreate).not.toHaveBeenCalled(); }); }); diff --git a/src/cli/prCreate.ts b/src/cli/prCreate.ts index 1de49369..d26ee051 100644 --- a/src/cli/prCreate.ts +++ b/src/cli/prCreate.ts @@ -37,6 +37,11 @@ export interface PrCreateDeps { description: string, ) => Promise; currentBranch?: (cwd: string) => Promise; + /** + * When injected, replaces both the dirty/upstream gate and the ahead-of-upstream + * check (tests). The default path validates dirty + upstream separately, then + * returns whether HEAD is ahead of `@{u}`. + */ hasDirtyOrAhead?: (cwd: string) => Promise; } @@ -44,20 +49,31 @@ async function defaultCurrentBranch(cwd: string): Promise { return (await git(cwd, 'rev-parse', '--abbrev-ref', 'HEAD')).trim(); } -async function defaultHasDirtyOrAhead(cwd: string): Promise { +/** + * Fail closed on a dirty tree and on a feature branch with no upstream. + * + * Both used to be read as "there is work to publish", so `pr create` on an + * uncommitted tree pushed whatever was already committed and silently left the + * edits behind, and a never-pushed branch counted as publishable on the strength + * of any commit existing. + */ +async function assertPublishableFeatureBranch(cwd: string): Promise { const dirty = (await git(cwd, 'status', '--porcelain')).trim(); - if (dirty) return true; - // Anything ahead of upstream, or unpushed commits on a new branch. + if (dirty) { + throw new Error('Uncommitted changes in the working tree — commit or stash before creating a PR'); + } try { - const ahead = (await git(cwd, 'rev-list', '--count', '@{u}..HEAD')).trim(); - return parseInt(ahead, 10) > 0; + await git(cwd, 'rev-parse', '--abbrev-ref', '@{u}'); } catch { - // No upstream — check commits vs default base via commitAndCreatePR itself. - const log = (await git(cwd, 'log', '--oneline', '-1')).trim(); - return log.length > 0; + throw new Error('Current branch has no upstream — push the feature branch before creating a PR'); } } +async function defaultHasAheadOfUpstream(cwd: string): Promise { + const ahead = (await git(cwd, 'rev-list', '--count', '@{u}..HEAD')).trim(); + return parseInt(ahead, 10) > 0; +} + /** * Publish the current working tree as a PR. * Optionally runs local `openswarm fix` first so we don't push a red tree. @@ -97,9 +113,18 @@ export async function createPrFromCwd( ); } - const hasWork = deps.hasDirtyOrAhead ?? defaultHasDirtyOrAhead; - if (!(await hasWork(cwd))) { - throw new Error('Nothing to publish — working tree clean and no commits ahead of upstream'); + const hasWork = deps.hasDirtyOrAhead; + if (hasWork) { + if (!(await hasWork(cwd))) { + throw new Error('Nothing to publish — working tree clean and no commits ahead of upstream'); + } + } else { + // Default path: refuse a dirty tree or a branch with no upstream outright, + // then require at least one commit ahead of @{u}. (AGT-3442) + await assertPublishableFeatureBranch(cwd); + if (!(await defaultHasAheadOfUpstream(cwd))) { + throw new Error('Nothing to publish — working tree clean and no commits ahead of upstream'); + } } const title = diff --git a/src/issues/graphql/costAnalysis.ts b/src/issues/graphql/costAnalysis.ts index a27e1b11..15ade33b 100644 --- a/src/issues/graphql/costAnalysis.ts +++ b/src/issues/graphql/costAnalysis.ts @@ -21,6 +21,14 @@ import type { Plugin } from 'graphql-yoga'; export const BULK_REGISTER_ENTITIES_COST = 500; export const AUTO_LINK_MEMORIES_COST = 500; +/** + * Registry CRUD mutations: bounded single-row writes. Priced below a bulk + * register so a single-entity edit is never mistaken for a bulk write, but + * high enough that aliasing them multiplies past the limit quickly — alias and + * fragment spread are the multiplication vector this table defends against. + */ +export const REGISTER_ENTITY_COST = 100; + /** * 뮤테이션 루트 최상위 필드의 실행 대표 비용 매핑. * (DoD 명칭: FIELD_COSTS — 뮤테이션/쿼리 루트 필드 비용 테이블) @@ -28,6 +36,11 @@ export const AUTO_LINK_MEMORIES_COST = 500; export const FIELD_COSTS: Record = { bulkRegisterEntities: BULK_REGISTER_ENTITIES_COST, autoLinkMemories: AUTO_LINK_MEMORIES_COST, + registerEntity: REGISTER_ENTITY_COST, + updateEntity: 80, + removeEntity: 80, + addEntityRelation: 60, + removeEntityRelation: 60, }; /** 기본 쿼리 비용 상한 */ diff --git a/src/issues/graphql/server.test.ts b/src/issues/graphql/server.test.ts index 1566adcf..58216809 100644 --- a/src/issues/graphql/server.test.ts +++ b/src/issues/graphql/server.test.ts @@ -1,12 +1,21 @@ import { afterEach, describe, expect, it } from 'vitest'; import { createServer, type IncomingMessage } from 'node:http'; -import { parse } from 'graphql'; -import { handleGraphQL, isGraphQLTransportAuthorized } from './server.js'; +import { buildSchema, getIntrospectionQuery, parse, validate } from 'graphql'; +import { + createGraphQLCostRule, + handleGraphQL, + isGraphQLRequest, + isGraphQLTransportAuthorized, +} from './server.js'; import { AUTO_LINK_MEMORIES_COST, + BULK_REGISTER_ENTITIES_COST, DEFAULT_QUERY_COST_LIMIT, + REGISTER_ENTITY_COST, calculateOperationCost, } from './costAnalysis.js'; +import { typeDefs } from './typeDefs.js'; +import { registryTypeDefs } from '../../registry/graphql/typeDefs.js'; function request(address: string | undefined, headers: Record = {}): IncomingMessage { return { socket: { remoteAddress: address }, headers } as unknown as IncomingMessage; @@ -124,3 +133,155 @@ describe('GraphQL transport authorization', () => { } }); }); + +describe('isGraphQLRequest exact endpoint matching', () => { + it('accepts only the exact /graphql path (with or without query/hash)', () => { + expect(isGraphQLRequest('/graphql')).toBe(true); + expect(isGraphQLRequest('/graphql?query={__typename}')).toBe(true); + expect(isGraphQLRequest('/graphql#section')).toBe(true); + }); + + it('rejects path-prefix lookalikes that previously matched startsWith', () => { + expect(isGraphQLRequest('/graphql/')).toBe(false); + expect(isGraphQLRequest('/graphql/admin')).toBe(false); + expect(isGraphQLRequest('/graphqlfoo')).toBe(false); + expect(isGraphQLRequest('/api/graphql')).toBe(false); + expect(isGraphQLRequest(undefined)).toBe(false); + expect(isGraphQLRequest('')).toBe(false); + }); +}); + +describe('GraphQL query cost limits', () => { + const schema = buildSchema(` + type Query { + a: Query + b: String + } + `); + + it('rejects a document that exceeds max depth', () => { + const query = '{ a { a { a { a { b } } } } }'; + const errors = validate(schema, parse(query), [createGraphQLCostRule({ maxDepth: 3 })]); + expect(errors.some((e) => /maximum depth/i.test(e.message))).toBe(true); + }); + + it('rejects a document that exceeds field or alias counts', () => { + const query = '{ x: b y: b z: b }'; + const errors = validate(schema, parse(query), [ + createGraphQLCostRule({ maxFieldCount: 2, maxAliasCount: 2, maxCost: 100 }), + ]); + expect(errors.some((e) => /maximum (field|alias) count/i.test(e.message))).toBe(true); + }); + + it('rejects a document that exceeds weighted execution cost', () => { + // cost accumulates as the depth at each field enter — a wide shallow fan-out + // of depth-1 fields exceeds a tight budget without tripping depth/alias caps. + const query = '{ a1: b a2: b a3: b a4: b }'; + const errors = validate(schema, parse(query), [ + createGraphQLCostRule({ maxDepth: 10, maxFieldCount: 20, maxAliasCount: 20, maxCost: 3 }), + ]); + expect(errors.some((e) => /maximum execution cost/i.test(e.message))).toBe(true); + }); + + it('allows a shallow bounded query', () => { + const errors = validate(schema, parse('{ b }'), [createGraphQLCostRule()]); + expect(errors).toEqual([]); + }); +}); + +describe('GraphQL introspection headroom (GraphiQL)', () => { + it('leaves room for the full introspection query under the shipped caps', () => { + // Built with this module's own graphql rather than yoga's createSchema: two + // copies of graphql in the module graph make validate() reject the foreign + // schema outright, and only the SDL shape matters here. + const liveSchema = buildSchema(`${typeDefs}\n${registryTypeDefs}`); + + // GraphiQL issues this on load. The shape caps are sized from the mutation + // surface, not from introspection, so this is the query whose headroom the + // caps must be checked against: 237 of the 250 cost budget today. + const errors = validate(liveSchema, parse(getIntrospectionQuery({ + descriptions: true, + inputValueDeprecation: true, + schemaDescription: true, + directiveIsRepeatable: true, + specifiedByUrl: true, + })), [createGraphQLCostRule()]); + expect(errors).toEqual([]); + }); +}); + +describe('registry mutation costs', () => { + it('prices single-entity CRUD below a bulk register', () => { + const doc = parse(` + mutation { + registerEntity(input: { qualifiedName: "x", kind: CLASS }) { id } + } + `); + expect(calculateOperationCost(doc)).toBe(REGISTER_ENTITY_COST); + expect(REGISTER_ENTITY_COST).toBeLessThan(BULK_REGISTER_ENTITIES_COST); + }); + + it('multiplies the cost of every alias of an expensive registry mutation', () => { + const doc = parse(` + mutation { + a: registerEntity(input: { qualifiedName: "a", kind: CLASS }) { id } + b: registerEntity(input: { qualifiedName: "b", kind: CLASS }) { id } + c: updateEntity(id: "1", input: {}) { id } + d: removeEntity(id: "2") { id } + } + `); + expect(calculateOperationCost(doc)).toBe(REGISTER_ENTITY_COST * 2 + 80 + 80); + }); + + it('multiplies cost for fragment spreads containing registry mutations', () => { + const doc = parse(` + mutation { + ...RegistrationA + ...RegistrationB + } + fragment RegistrationA on Mutation { + a: bulkRegisterEntities(input: [{ qualifiedName: "x", kind: CLASS }]) { id } + } + fragment RegistrationB on Mutation { + b: bulkRegisterEntities(input: [{ qualifiedName: "y", kind: CLASS }]) { id } + } + `); + expect(calculateOperationCost(doc)).toBe(BULK_REGISTER_ENTITIES_COST * 2); + }); + + it('rejects aliased bulkRegisterEntities mutations over HTTP as exceeding the cost limit', async () => { + process.env.OPENSWARM_GRAPHQL_TOKEN = 'secret'; + const query = ` + mutation { + a: bulkRegisterEntities(input: [{ qualifiedName: "x", kind: CLASS }]) { id } + b: bulkRegisterEntities(input: [{ qualifiedName: "y", kind: CLASS }]) { id } + } + `; + expect(calculateOperationCost(parse(query))).toBe(BULK_REGISTER_ENTITIES_COST * 2); + expect(BULK_REGISTER_ENTITIES_COST * 2).toBeGreaterThan(DEFAULT_QUERY_COST_LIMIT); + + const httpServer = createServer(async (req, res) => { + if (req.url?.startsWith('/graphql')) { + await handleGraphQL(req, res); + } else { + res.writeHead(404); + res.end(); + } + }); + try { + await new Promise((resolve) => httpServer.listen(0, '127.0.0.1', resolve)); + const address = httpServer.address(); + if (!address || typeof address === 'string') throw new Error('missing test server address'); + const response = await fetch(`http://127.0.0.1:${address.port}/graphql`, { + method: 'POST', + headers: { 'content-type': 'application/json', authorization: 'Bearer secret' }, + body: JSON.stringify({ query }), + }); + expect(response.status).toBe(400); + const body = await response.json() as { errors?: Array<{ extensions?: { code?: string } }> }; + expect(body.errors?.[0]?.extensions?.code).toBe('GRAPHQL_COST_LIMIT_EXCEEDED'); + } finally { + await new Promise((resolve, reject) => httpServer.close((error) => error ? reject(error) : resolve())); + } + }); +}); diff --git a/src/issues/graphql/server.ts b/src/issues/graphql/server.ts index f64ae1bb..dfd3bffe 100644 --- a/src/issues/graphql/server.ts +++ b/src/issues/graphql/server.ts @@ -4,7 +4,14 @@ // Purpose: graphql-yoga 서버, 기존 HTTP 서버에 통합 // ============================================ -import { createSchema, createYoga } from 'graphql-yoga'; +import { createSchema, createYoga, type Plugin } from 'graphql-yoga'; +import { + GraphQLError, + type ASTVisitor, + type ValidationContext, + type ValidationRule, +} from 'graphql'; +import type { OnValidateEventPayload } from '@envelop/types'; import { typeDefs } from './typeDefs.js'; import { resolvers } from './resolvers.js'; import { registryTypeDefs } from '../../registry/graphql/typeDefs.js'; @@ -16,6 +23,12 @@ import { useQueryCostAnalysis } from './costAnalysis.js'; const CORS_METHODS = 'GET, POST, OPTIONS'; const CORS_HEADERS = 'Content-Type, Authorization, X-OpenSwarm-GraphQL-Token'; +/** Depth / fan-out / alias / weighted-cost caps for untrusted GraphQL documents. */ +export const GRAPHQL_MAX_DEPTH = 12; +export const GRAPHQL_MAX_FIELD_COUNT = 120; +export const GRAPHQL_MAX_ALIAS_COUNT = 40; +export const GRAPHQL_MAX_COST = 250; + function isAllowedOrigin(origin: string): boolean { let url: URL; try { @@ -38,7 +51,9 @@ function isAllowedOrigin(origin: string): boolean { function applyCors(req: IncomingMessage, res: ServerResponse): boolean { const origin = req.headers.origin; - if (origin && isAllowedOrigin(origin)) { + if (!origin) return false; + + if (isAllowedOrigin(origin)) { res.setHeader('Access-Control-Allow-Origin', origin); res.setHeader('Vary', 'Origin'); res.setHeader('Access-Control-Allow-Methods', CORS_METHODS); @@ -47,7 +62,7 @@ function applyCors(req: IncomingMessage, res: ServerResponse): boolean { if (req.method !== 'OPTIONS') return false; - res.writeHead(origin && !isAllowedOrigin(origin) ? 403 : 204); + res.writeHead(isAllowedOrigin(origin) ? 204 : 403); res.end(); return true; } @@ -98,13 +113,88 @@ export function isGraphQLTransportAuthorized(req: IncomingMessage): boolean { return hasValidToken({ authorization, token }); } +/** + * Reject documents that nest, fan out, or alias themselves into an unbounded + * execution cost before any resolver runs. Complements the mutation-priced + * check in costAnalysis: that one multiplies alias/fragment spreads of known + * expensive mutations, this one caps the shape itself (depth, field count, + * alias count, depth-weighted cost) so an unknown-but-deep document fails too. + */ +export function createGraphQLCostRule(limits: { + maxDepth?: number; + maxFieldCount?: number; + maxAliasCount?: number; + maxCost?: number; +} = {}): ValidationRule { + const maxDepth = limits.maxDepth ?? GRAPHQL_MAX_DEPTH; + const maxFieldCount = limits.maxFieldCount ?? GRAPHQL_MAX_FIELD_COUNT; + const maxAliasCount = limits.maxAliasCount ?? GRAPHQL_MAX_ALIAS_COUNT; + const maxCost = limits.maxCost ?? GRAPHQL_MAX_COST; + + return function GraphQLCostRule(context: ValidationContext): ASTVisitor { + let depth = 0; + let fieldCount = 0; + let aliasCount = 0; + let cost = 0; + + return { + Field: { + enter(node) { + depth += 1; + fieldCount += 1; + if (node.alias) aliasCount += 1; + // Deeper fields cost more so nested selection sets hit the budget first. + cost += depth; + + if (depth > maxDepth) { + context.reportError(new GraphQLError(`Query exceeds maximum depth of ${maxDepth}`, { nodes: [node] })); + } + if (fieldCount > maxFieldCount) { + context.reportError(new GraphQLError(`Query exceeds maximum field count of ${maxFieldCount}`, { nodes: [node] })); + } + if (aliasCount > maxAliasCount) { + context.reportError(new GraphQLError(`Query exceeds maximum alias count of ${maxAliasCount}`, { nodes: [node] })); + } + if (cost > maxCost) { + context.reportError(new GraphQLError(`Query exceeds maximum execution cost of ${maxCost}`, { nodes: [node] })); + } + }, + leave() { + depth -= 1; + }, + }, + // Fragment fields are counted when the fragment definition itself is walked. + FragmentDefinition: { + enter() { + depth = 0; + }, + }, + }; + }; +} + +/** + * Yoga plugin that installs the shape caps on every validated document. Typed + * against Yoga's own Plugin so the `onValidate` payload stays checked: the + * `addValidationRule` argument is `any` upstream, and an untyped destructure + * would let a rename in the hook payload pass silently. + */ +const costRulePlugin: Plugin = { + onValidate({ addValidationRule }: OnValidateEventPayload>) { + addValidationRule(createGraphQLCostRule()); + }, +}; + // GraphQL Yoga 인스턴스 생성 (이슈 + 코드 레지스트리 스키마 머지) const yoga = createYoga({ schema: createSchema({ typeDefs: [typeDefs, registryTypeDefs], resolvers: [resolvers, registryResolvers], }), - plugins: [useQueryCostAnalysis()], + plugins: [ + useQueryCostAnalysis(), + costRulePlugin, + ], graphqlEndpoint: '/graphql', cors: false, logging: { @@ -138,11 +228,13 @@ export async function handleGraphQL( } /** - * GraphQL 경로 매칭 여부 + * Exact GraphQL endpoint match — path-prefix forms like `/graphql/admin` or + * `/graphqlfoo` must not reach the Yoga handler. */ export function isGraphQLRequest(url: string | undefined): boolean { if (!url) return false; - return url.startsWith('/graphql'); + const path = url.split(/[?#]/, 1)[0] ?? ''; + return path === '/graphql'; } export { yoga }; diff --git a/src/mcp/mcpClient.test.ts b/src/mcp/mcpClient.test.ts index cc9f8132..86cc3217 100644 --- a/src/mcp/mcpClient.test.ts +++ b/src/mcp/mcpClient.test.ts @@ -11,6 +11,7 @@ import { initMcpTools, callMcpTool, withDeadline, + sanitizeInputSchema, } from './mcpClient.js'; import type { ToolDefinition } from '../adapters/tools.js'; @@ -416,3 +417,35 @@ describe('withDeadline', () => { vi.useRealTimers(); }); }); + +describe('sanitizeInputSchema bounds', () => { + it('keeps a small object schema', () => { + const schema = { type: 'object', properties: { q: { type: 'string' } }, required: ['q'] }; + expect(sanitizeInputSchema(schema)).toEqual(schema); + }); + + it('replaces an oversized serialized schema with the empty object schema', () => { + const properties: Record = {}; + for (let i = 0; i < 40; i++) { + properties[`field_${i}`] = { type: 'string', description: 'x'.repeat(500) }; + } + const out = sanitizeInputSchema({ type: 'object', properties }); + expect(out).toEqual({ type: 'object', properties: {} }); + }); + + it('counts nested declarations, not just top-level keys', () => { + const properties: Record = {}; + for (let i = 0; i < 60; i++) properties[`p${i}`] = { type: 'string' }; + const deep: Record = {}; + for (let i = 0; i < 5; i++) deep[`d${i}`] = { type: 'string' }; + properties.nested = { type: 'object', properties: deep }; + // 61 top-level keys are inside the cap on their own; the nested declarations + // take the total past it, which a top-level-keys-only count would miss. + expect(sanitizeInputSchema({ type: 'object', properties })).toEqual({ type: 'object', properties: {} }); + }); + + it('rejects non-object root types', () => { + expect(sanitizeInputSchema({ type: 'string' })).toEqual({ type: 'object', properties: {} }); + expect(sanitizeInputSchema(null)).toEqual({ type: 'object', properties: {} }); + }); +}); diff --git a/src/mcp/mcpClient.ts b/src/mcp/mcpClient.ts index e7bcc0f7..baa9e0da 100644 --- a/src/mcp/mcpClient.ts +++ b/src/mcp/mcpClient.ts @@ -34,6 +34,9 @@ const MCP_JSON_PATH = join(homedir(), '.openswarm', 'mcp.json'); const MAX_MCP_TOOL_RESULT_CHARS = 20_000; const MCP_CONNECT_TIMEOUT_MS = 15_000; const MCP_OPERATION_TIMEOUT_MS = 30_000; +/** Bound remote tool input schemas before they are cached or exposed to the model. */ +const MAX_INPUT_SCHEMA_BYTES = 16_384; +const MAX_INPUT_SCHEMA_PROPERTIES = 64; const EMPTY_INPUT_SCHEMA: Record = { type: 'object', properties: {} }; interface ServerConfig { @@ -123,9 +126,47 @@ function isJsonSchemaObject(schema: unknown, depth = 0): schema is Record { +/** Count every declared property in a schema tree, including nested/composed shapes. */ +function countSchemaProperties(schema: Record, depth = 0): number { + if (depth > 8) return Number.POSITIVE_INFINITY; + let count = 0; + if (isRecord(schema.properties)) { + count += Object.keys(schema.properties).length; + for (const value of Object.values(schema.properties)) { + if (isRecord(value)) count += countSchemaProperties(value, depth + 1); + } + } + for (const keyword of ['anyOf', 'oneOf', 'allOf'] as const) { + const value = schema[keyword]; + if (!Array.isArray(value)) continue; + for (const entry of value) { + if (isRecord(entry)) count += countSchemaProperties(entry, depth + 1); + } + } + if (isRecord(schema.items)) count += countSchemaProperties(schema.items, depth + 1); + else if (Array.isArray(schema.items)) { + for (const entry of schema.items) { + if (isRecord(entry)) count += countSchemaProperties(entry, depth + 1); + } + } + if (isRecord(schema.additionalProperties)) { + count += countSchemaProperties(schema.additionalProperties, depth + 1); + } + return count; +} + +/** Drop oversized or non-object remote schemas so tool defs cannot balloon the cache. */ +export function sanitizeInputSchema(schema: unknown): Record { if (!isJsonSchemaObject(schema)) return EMPTY_INPUT_SCHEMA; if (schema.type !== undefined && schema.type !== 'object') return EMPTY_INPUT_SCHEMA; + if (countSchemaProperties(schema) > MAX_INPUT_SCHEMA_PROPERTIES) return EMPTY_INPUT_SCHEMA; + let serialized: string; + try { + serialized = JSON.stringify(schema); + } catch { + return EMPTY_INPUT_SCHEMA; + } + if (serialized.length > MAX_INPUT_SCHEMA_BYTES) return EMPTY_INPUT_SCHEMA; return schema; } diff --git a/src/support/outboundUrl.test.ts b/src/support/outboundUrl.test.ts index 813a66cd..ea158704 100644 --- a/src/support/outboundUrl.test.ts +++ b/src/support/outboundUrl.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it } from 'vitest'; -import { createPublicLookup, isPrivateIp, type DnsAllResolver } from './outboundUrl.js'; +import { + createPinnedPublicLookup, + createPublicLookup, + isPrivateIp, + type DnsAllResolver, +} from './outboundUrl.js'; function resolverReturning(addresses: Array<{ address: string; family: number }>): DnsAllResolver { return (_hostname, _options, callback) => callback(null, addresses); @@ -46,6 +51,43 @@ describe('createPublicLookup callback contract', () => { }); }); +/** + * The connect-time hook publicFetch installs. It answers from the addresses + * resolvePublicHttpUrl already validated instead of re-resolving, so a second + * DNS answer cannot move the socket after the check (rebinding). + */ +describe('createPinnedPublicLookup', () => { + it('answers both callback shapes from the pinned set without a DNS round-trip', () => { + const addresses = [{ address: '93.184.216.34', family: 4 }]; + const lookup = createPinnedPublicLookup(addresses); + const seen: unknown[] = []; + lookup('example.com', { all: true }, (...args) => seen.push(args)); + lookup('example.com', {}, (...args) => seen.push(args)); + expect(seen).toEqual([ + [null, addresses], + [null, '93.184.216.34', 4], + ]); + }); + + it('refuses when the pinned set includes a private address', () => { + const lookup = createPinnedPublicLookup([ + { address: '93.184.216.34', family: 4 }, + { address: '127.0.0.1', family: 4 }, + ]); + let error: Error | null = null; + lookup('rebind.example', { all: true }, (err) => { error = err; }); + expect(error).toBeInstanceOf(Error); + expect((error as unknown as Error).message).toBe('Private network destinations are not allowed'); + }); + + it('refuses an empty pinned set', () => { + const lookup = createPinnedPublicLookup([]); + let error: Error | null = null; + lookup('empty.example', { all: true }, (err) => { error = err; }); + expect(error).toBeInstanceOf(Error); + }); +}); + describe('isPrivateIp IPv6 canonicalization', () => { it.each([ '0:0:0:0:0:0:0:1', diff --git a/src/support/outboundUrl.ts b/src/support/outboundUrl.ts index a195e5cc..6c114e6b 100644 --- a/src/support/outboundUrl.ts +++ b/src/support/outboundUrl.ts @@ -74,8 +74,16 @@ export function isPrivateIp(address: string): boolean { return false; } +interface ResolvedAddress { address: string; family: number } + +export interface ResolvedPublicHttpUrl { + url: URL; + /** Addresses validated in the same lookup the socket must use — prevents DNS rebinding. */ + addresses: ResolvedAddress[]; +} + /** Resolve and reject destinations that can reach the local machine or a private network. */ -export async function assertPublicHttpUrl(value: string): Promise { +export async function resolvePublicHttpUrl(value: string): Promise { const url = new URL(value); if (url.protocol !== 'https:' && url.protocol !== 'http:') throw new Error('Only HTTP(S) URLs are allowed'); if (url.username || url.password) throw new Error('Webhook URLs must not contain userinfo'); @@ -83,18 +91,37 @@ export async function assertPublicHttpUrl(value: string): Promise { if (hostname === 'localhost' || hostname.endsWith('.localhost') || hostname.endsWith('.local') || hostname.endsWith('.internal')) { throw new Error('Private network destinations are not allowed'); } - if (isIP(hostname)) { + const literalFamily = isIP(hostname); + if (literalFamily) { if (isPrivateIp(hostname)) throw new Error('Private network destinations are not allowed'); - return url; + return { url, addresses: [{ address: hostname, family: literalFamily }] }; } const addresses = await lookup(hostname, { all: true, verbatim: true }); if (addresses.length === 0 || addresses.some(({ address }) => isPrivateIp(address))) { throw new Error('Private network destinations are not allowed'); } - return url; + return { url, addresses }; } -interface ResolvedAddress { address: string; family: number } +/** Resolve and reject destinations that can reach the local machine or a private network. */ +export async function assertPublicHttpUrl(value: string): Promise { + return (await resolvePublicHttpUrl(value)).url; +} + +/** Lookup hook that returns only the pre-validated addresses — no second DNS round-trip. */ +export function createPinnedPublicLookup(addresses: readonly ResolvedAddress[]) { + return function pinnedPublicLookup( + _hostname: string, + options: { all?: boolean } | undefined, + callback: (error: Error | null, addresses: ResolvedAddress[] | string, family?: number) => void, + ): void { + if (addresses.length === 0 || addresses.some(({ address }) => isPrivateIp(address))) { + return callback(new Error('Private network destinations are not allowed'), '', 0); + } + if (options?.all) return callback(null, [...addresses]); + callback(null, addresses[0].address, addresses[0].family); + }; +} /** Injectable so the callback contract below can be tested without real DNS. */ export type DnsAllResolver = ( @@ -129,13 +156,6 @@ export function createPublicLookup(resolve: DnsAllResolver = lookupCallback as u }; } -let sharedPublicAgent: Agent | undefined; - -function publicNetworkAgent(): Agent { - sharedPublicAgent ??= new Agent({ connect: { lookup: createPublicLookup() } }); - return sharedPublicAgent; -} - /** * Fetch that refuses private destinations before connecting and pins the * resolved address, so a rebinding answer cannot move the socket after the @@ -144,15 +164,28 @@ function publicNetworkAgent(): Agent { * undici's own `fetch` is required: a dispatcher built from the npm `undici` * package is rejected by the copy of undici bundled inside Node's global * `fetch` ("invalid onError method"), which would fail every request. + * + * The connect-time lookup returns the same addresses `resolvePublicHttpUrl` + * already validated — a second DNS answer cannot steer the socket onto a + * different (still-"public") host after the check. Notification webhooks and + * other outbound callers therefore connect only to the validated public address. */ export async function publicFetch(url: string | URL, init: RequestInit = {}): Promise { - const validated = await assertPublicHttpUrl(String(url)); - // undici's Response is spec-compatible with the global one; the DOM lib types - // are structurally distinct, so cross the boundary once, here. - return undiciFetch(validated.toString(), { - ...(init as Record), - dispatcher: publicNetworkAgent(), - } as never) as unknown as Response; + const { url: validated, addresses } = await resolvePublicHttpUrl(String(url)); + // Per-request dispatcher, not a shared one: the pinned lookup is bound to this + // call's validated addresses and must not outlive it. + const agent = new Agent({ connect: { lookup: createPinnedPublicLookup(addresses) } }); + try { + // undici's Response is spec-compatible with the global one; the DOM lib types + // are structurally distinct, so cross the boundary once, here. `close()` is + // graceful — it lets the body finish streaming before the socket goes away. + return await undiciFetch(validated.toString(), { + ...(init as Record), + dispatcher: agent, + } as never) as unknown as Response; + } finally { + void agent.close(); + } } /** Cheap configuration-time check; send-time validation also resolves DNS. */ diff --git a/src/verify/runner.test.ts b/src/verify/runner.test.ts index cb3d1b71..ed29beb4 100644 --- a/src/verify/runner.test.ts +++ b/src/verify/runner.test.ts @@ -1,10 +1,17 @@ import { execFileSync } from 'node:child_process'; import { chmod, mkdir, mkdtemp, readFile, realpath, rm, symlink, unlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join, dirname } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { VerifyCommand } from './manifest.js'; -import { runVerify } from './runner.js'; +import { + buildVerifyToolchainPath, + CLONE_TIMEOUT_MS, + gitTimeoutMsFor, + runVerify, + VERIFY_ALLOWED_DEPENDENCY_DIRS, + VERIFY_TOOLCHAIN_PATH_PREFIXES, +} from './runner.js'; let root: string; let repo: string; @@ -675,8 +682,7 @@ describe('runVerify', () => { }); describe('git timeout budgets (AGT-4416)', () => { - it('gives the sandbox clone ten minutes and every other git call the 30 s default', async () => { - const { CLONE_TIMEOUT_MS, gitTimeoutMsFor } = await import('./runner.js'); + it('gives the sandbox clone ten minutes and every other git call the 30 s default', () => { expect(CLONE_TIMEOUT_MS).toBe(10 * 60_000); expect(gitTimeoutMsFor(['clone', '--quiet', '--no-hardlinks', '--no-checkout', '/src', '/dst'])).toBe(CLONE_TIMEOUT_MS); for (const args of [['rev-parse', 'HEAD'], ['checkout', '--quiet', '--detach', 'abc'], ['worktree', 'add', '--detach', '/x', 'abc'], ['merge-base', 'HEAD', 'main']]) { @@ -684,3 +690,43 @@ describe('git timeout budgets (AGT-4416)', () => { } }); }); + +describe('buildVerifyToolchainPath', () => { + it('keeps project bins and the read-only toolchain prefixes, dropping the rest of the host PATH', () => { + const path = buildVerifyToolchainPath( + ['/usr/bin', '/home/user/.local/bin', '/opt/homebrew/bin', '/tmp/evil/bin'].join(':'), + '/repo', + '/repo/packages/api', + ); + const parts = path.split(':'); + for (const prefix of VERIFY_TOOLCHAIN_PATH_PREFIXES) expect(parts).toContain(prefix); + expect(parts).toContain('/repo/node_modules/.bin'); + expect(parts).toContain('/repo/packages/api/node_modules/.bin'); + expect(parts).not.toContain('/home/user/.local/bin'); + expect(parts).not.toContain('/tmp/evil/bin'); + expect(VERIFY_ALLOWED_DEPENDENCY_DIRS.has('node_modules')).toBe(true); + }); + + it('puts project dependency bins ahead of the system prefixes', () => { + const parts = buildVerifyToolchainPath('/usr/bin', '/repo').split(':'); + expect(parts.indexOf('/repo/node_modules/.bin')).toBeLessThan(parts.indexOf('/usr/bin')); + }); + + it('preserves nvm/fnm-style version-manager bin directories', () => { + const path = buildVerifyToolchainPath('/home/user/.nvm/versions/node/v22.0.0/bin:/usr/bin', '/repo'); + expect(path.split(':')).toContain('/home/user/.nvm/versions/node/v22.0.0/bin'); + }); + + it('keeps the running interpreter bin ahead of the generic system prefixes', () => { + const parts = buildVerifyToolchainPath('/usr/bin', '/repo').split(':'); + expect(parts).toContain(dirname(process.execPath)); + // A different major node lives in /usr/local/bin on this host; the sandbox + // must reach the daemon's own interpreter first. + expect(parts.indexOf(dirname(process.execPath))).toBeLessThan(parts.indexOf('/usr/bin')); + }); + + it('keeps a project venv bin directory', () => { + const path = buildVerifyToolchainPath('/repo/.venv-verify/bin:/usr/bin', '/repo'); + expect(path.split(':')).toContain('/repo/.venv-verify/bin'); + }); +}); diff --git a/src/verify/runner.ts b/src/verify/runner.ts index 1dd26452..3cabfcad 100644 --- a/src/verify/runner.ts +++ b/src/verify/runner.ts @@ -39,12 +39,31 @@ export function gitTimeoutMsFor(args: readonly string[]): number { return args[0] === 'clone' ? CLONE_TIMEOUT_MS : GIT_TIMEOUT_MS; } const execFileAsync = promisify(execFile); -const DEPENDENCY_INPUTS = new Set([ + +/** Dependency manifests the sandbox may observe when comparing base vs head. */ +export const VERIFY_DEPENDENCY_INPUTS = new Set([ 'package.json', 'package-lock.json', 'npm-shrinkwrap.json', 'pnpm-lock.yaml', 'yarn.lock', 'Cargo.toml', 'Cargo.lock', 'go.mod', 'go.sum', 'requirements.txt', 'pyproject.toml', 'uv.lock', 'poetry.lock', ]); +/** Shared dependency directories the verify sandbox may bind read-only from the live tree. */ +export const VERIFY_ALLOWED_DEPENDENCY_DIRS = new Set([ + 'node_modules', '.venv-verify', '.venv', 'venv', +]); + +/** + * Read-only toolchain PATH prefixes verification may inherit. Anything else on + * the host PATH (writable home dirs, arbitrary tool installs) stays out of the + * sandbox so a malicious checkout cannot pick an unexpected binary via PATH. + */ +export const VERIFY_TOOLCHAIN_PATH_PREFIXES = [ + '/usr/bin', + '/bin', + '/usr/local/bin', + '/opt/homebrew/bin', +] as const; + export interface VerifyEvidence { command: VerifyCommand; baseStatus: 'pass' | 'fail' | 'infra' | 'skipped'; @@ -94,7 +113,7 @@ async function verificationSharedPaths(projectPath: string, commands: VerifyComm if (localDirectory === '..' || localDirectory.startsWith(`..${sep}`) || isAbsolute(localDirectory)) { throw new Error(`[security] verify cwd escapes project root: ${directory}`); } - for (const name of ['node_modules', '.venv-verify', '.venv', 'venv']) { + for (const name of VERIFY_ALLOWED_DEPENDENCY_DIRS) { const dependency = join(localDirectory, name); try { await access(join(projectPath, dependency)); @@ -115,7 +134,7 @@ function isPrivateEnvironmentPath(path: string): boolean { function sharedPathSecretFilter(sharedPath: string): (path: string) => boolean { // Like the companion's secret scan, dependency payloads retain packaged // certificates (e.g. certifi/cacert.pem). Local configuration stays excluded. - return ['node_modules', '.venv', '.venv-verify', 'venv'].includes(basename(sharedPath)) + return VERIFY_ALLOWED_DEPENDENCY_DIRS.has(basename(sharedPath)) ? (path) => path.split(sep).some(isPrivateConfigurationFile) : isPrivateEnvironmentPath; } @@ -280,6 +299,64 @@ async function terminateVerificationProcesses(processGroupId: number | undefined await terminateProcessesWithEnvMarker(marker); } +/** + * Build the sandbox PATH from project-local dependency bins plus an explicit + * read-only toolchain allowlist. Host PATH entries outside those prefixes are + * dropped, so a writable tool install or an attacker-planted directory on the + * host PATH cannot shadow a toolchain binary inside the verify sandbox. + * + * Project bins come FIRST: the sandbox must run the checkout's own + * `node_modules/.bin/vitest`, not a system-wide one. + */ +export function buildVerifyToolchainPath( + envPath: string | undefined, + root: string, + cwd: string = root, +): string { + const entries: string[] = []; + const seen = new Set(); + const add = (candidate: string): void => { + if (!candidate || seen.has(candidate)) return; + seen.add(candidate); + entries.push(candidate); + }; + + for (const base of [cwd, root]) { + add(join(base, 'node_modules', '.bin')); + for (const venv of VERIFY_ALLOWED_DEPENDENCY_DIRS) { + if (venv === 'node_modules') continue; + add(join(base, venv, process.platform === 'win32' ? 'Scripts' : 'bin')); + } + } + + // The interpreter running this verification comes right after the project bins. + // Its directory is trustworthy by construction — the daemon chose it — and it + // is the entry that covers every layout (/opt/hostedtoolcache/node//bin, + // Homebrew, a version manager) without opening $HOME. Ahead of the generic + // prefixes on purpose: this host's /usr/local/bin/node is a different major + // version, and the sandbox must run the interpreter the daemon runs. + add(dirname(process.execPath)); + + for (const prefix of VERIFY_TOOLCHAIN_PATH_PREFIXES) add(prefix); + + for (const part of (envPath ?? '').split(delimiter)) { + if (!part) continue; + const normalized = part.replace(/\\/g, '/').replace(/\/+$/, ''); + const allowedPrefix = VERIFY_TOOLCHAIN_PATH_PREFIXES.some( + (prefix) => normalized === prefix || normalized.startsWith(`${prefix}/`), + ); + const projectLocal = /(?:^|\/)(?:node_modules\/\.bin|(?:\.venv-verify|\.venv|venv)\/(?:bin|Scripts))$/ + .test(normalized); + // Node version managers install under */bin — keep those so `node`/`npm` + // remain reachable without opening the entire home directory PATH. + const versionManagerBin = /(?:^|\/)(?:\.?nvm|fnm|asdf|volta|n)(?:\/|$)/.test(normalized) + && normalized.endsWith('/bin'); + if (allowedPrefix || projectLocal || versionManagerBin) add(part); + } + + return entries.join(delimiter); +} + /** Working sandbox memoized, broken one re-probed — see makeSandboxCache. */ const linuxSandbox = makeSandboxCache(() => describeLinuxSandbox(makeSystemProbe({ exists: existsSync, @@ -315,13 +392,14 @@ async function runWithSandboxExecutor( const timeoutMs = command.timeoutMs ?? 300_000; try { const session = await createSession(root); - const cwdBin = join(cwd, 'node_modules', '.bin'); - const rootBin = join(root, 'node_modules', '.bin'); const relativeCwd = relative(root, cwd) || '.'; const vegaWorkspace = vegaVerifyWorkspaceRoot(root); + // The sandbox PATH is rebuilt from project bins + the read-only toolchain + // allowlist rather than inherited, so the host PATH no longer rides along. + const toolchainPath = buildVerifyToolchainPath(process.env.PATH, root, cwd); const result = await session.execute([ `cd -- ${shellQuote(relativeCwd)}`, - `export PATH=${shellQuote(`${cwdBin}${delimiter}${rootBin}`)}:"$PATH"`, + `export PATH=${shellQuote(toolchainPath)}`, ...(vegaWorkspace ? [`export VEGA_EXTRA_PATHS=${shellQuote(vegaWorkspace)}`] : []), // Bundled VEGA toolsets intentionally use the narrower headless-workspace // contract instead of VEGA_EXTRA_PATHS. Both settings name this same @@ -394,7 +472,7 @@ async function runCommand( await Promise.all([mkdir(isolatedHome, { recursive: true }), mkdir(isolatedTmp, { recursive: true })]); const processMarker = `openswarm-verify-${randomUUID()}`; const safeEnv: NodeJS.ProcessEnv = withTestResourceBudget({ - PATH: env.PATH, + PATH: buildVerifyToolchainPath(env.PATH, root, cwd), HOME: isolatedHome, USERPROFILE: isolatedHome, XDG_CONFIG_HOME: join(isolatedHome, '.config'), @@ -781,7 +859,7 @@ async function runAtBase( const changedFiles = await git(projectPath, ['diff', '--name-only', baseCommit, '--']); const untrackedFiles = await git(projectPath, ['ls-files', '--others', '--exclude-standard']); const dependencyChanges = `${changedFiles}\n${untrackedFiles}`.split('\n') - .some((file) => DEPENDENCY_INPUTS.has(file.split('/').pop() ?? '')); + .some((file) => VERIFY_DEPENDENCY_INPUTS.has(file.split('/').pop() ?? '')); root = await createVerifySandboxRoot('.openswarm-verify-base-', scratchRoot); worktreePath = join(root, 'worktree'); await git(projectPath, ['worktree', 'add', '--detach', worktreePath, baseCommit]);