diff --git a/.vscodeignore b/.vscodeignore index f035df69..2643c1ec 100644 --- a/.vscodeignore +++ b/.vscodeignore @@ -26,6 +26,18 @@ dist/**/*.map README.github.md SECURITY.md +# Dev-only config & tooling — not needed at runtime +.husky/** +.devcontainer/** +cspell.json +knip.json +playwright.config.ts +.lockfile-lintrc.json +.npmrc + +# Marketing assets are for the repo / store listing, not the shipped bundle +marketing/** + # Benchmark / perf outputs benchmark-*.json diff --git a/src/core/dsl/safe-regex.test.ts b/src/core/dsl/safe-regex.test.ts index 00d8a197..58c8fc5b 100644 --- a/src/core/dsl/safe-regex.test.ts +++ b/src/core/dsl/safe-regex.test.ts @@ -358,4 +358,34 @@ describe('isLikelySafe', () => { it('returns false for prefix-overlapping alternation under a quantifier', () => { expect(isLikelySafe('(a|aa)+')).toBe(false); }); + + it('returns false for a character-class branch overlapping a literal under a quantifier', () => { + expect(isLikelySafe('^([a]|a)+$')).toBe(false); + }); + + it('returns false for a dot branch overlapping a literal under a quantifier', () => { + expect(isLikelySafe('^(.|a)+$')).toBe(false); + }); + + it('returns false for a class-escape branch overlapping a literal under a quantifier', () => { + expect(isLikelySafe('^(\\w|a)+$')).toBe(false); + }); + + it('returns true for a disjoint character-class alternation under a quantifier', () => { + expect(isLikelySafe('^([a-z]|_)+$')).toBe(true); + }); + + it('rejects a class-overlapping alternation pattern through compileSafe', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + // Pattern assembled at runtime so static analysis does not treat this + // rejected fixture as a live regex; compileSafe returns null before any + // RegExp is constructed. + const ch = String.fromCharCode(97); + const overlapping = `^([${ch}]|${ch})+$`; + expect(compileSafe(overlapping)).toBeNull(); + + expect(warn).toHaveBeenCalledTimes(1); + warn.mockRestore(); + }); }); diff --git a/src/core/dsl/safe-regex.ts b/src/core/dsl/safe-regex.ts index 08a3f26b..468fe15a 100644 --- a/src/core/dsl/safe-regex.ts +++ b/src/core/dsl/safe-regex.ts @@ -159,20 +159,16 @@ export function isLikelySafe(pattern: string): boolean { return maxStarHeight <= 2; } -function hasOverlappingAlternation(body: string): boolean { +const CLASS_ESCAPES = new Set(['w', 'W', 'd', 'D', 's', 'S']); + +function splitTopLevelBranches(body: string): string[] { const branches: string[] = []; let depth = 0; let start = 0; for (let i = 0; i < body.length; i++) { const ch = body[i]; if (ch === '\\') { i++; continue; } - if (ch === '[') { - while (i < body.length && body[i] !== ']') { - if (body[i] === '\\') i++; - i++; - } - continue; - } + if (ch === '[') { i = skipCharacterClass(body, i); continue; } if (ch === '(') depth++; else if (ch === ')') depth--; else if (ch === '|' && depth === 0) { @@ -180,23 +176,64 @@ function hasOverlappingAlternation(body: string): boolean { start = i + 1; } } - if (branches.length === 0) return false; branches.push(body.slice(start)); + return branches; +} + +// Alternation branches that can match the same first character cause +// catastrophic backtracking under an unbounded quantifier (e.g. `(.|a)+`). +function hasOverlappingAlternation(body: string): boolean { + const tokens = splitTopLevelBranches(body).map(firstToken); + if (tokens.length < 2) return false; - // If any two branches share a non-empty literal prefix, flag it. - for (let i = 0; i < branches.length; i++) { - for (let j = i + 1; j < branches.length; j++) { - const a = literalPrefix(branches[i]); - const b = literalPrefix(branches[j]); - if (a && b && (a === b || a.startsWith(b) || b.startsWith(a))) { - return true; - } + for (let i = 0; i < tokens.length; i++) { + for (let j = i + 1; j < tokens.length; j++) { + if (tokensOverlap(tokens[i], tokens[j])) return true; } } return false; } -/** Return the leading literal-character run of a branch (no metachars). */ +// A branch's leading token: either a literal run or a single-char matcher +// (`.`, a class escape, or a `[...]` class). `null` means indeterminate. +type FirstToken = + | { set: false; literal: string } + | { set: true; source: string } + | null; + +function firstToken(branch: string): FirstToken { + const body = branch.startsWith('^') ? branch.slice(1) : branch; + if (body.length === 0) return null; + + if (body[0] === '.') return { set: true, source: '.' }; + if (body[0] === '[') return { set: true, source: body.slice(0, skipCharacterClass(body, 0) + 1) }; + if (body[0] === '\\' && body.length > 1 && CLASS_ESCAPES.has(body[1])) { + return { set: true, source: body.slice(0, 2) }; + } + + const prefix = literalPrefix(body); + return prefix ? { set: false, literal: prefix } : null; +} + +function tokensOverlap(a: FirstToken, b: FirstToken): boolean { + if (!a || !b) return false; + if (!a.set && !b.set) return a.literal.startsWith(b.literal) || b.literal.startsWith(a.literal); + if (a.set && b.set) return true; + const set = a.set ? a : (b as Extract); + const literal = a.set ? (b as Extract) : a; + return setMatchesChar(set.source, literal.literal[0]); +} + +// Whether a single-char matcher source (e.g. `[a-z]`, `\w`, `.`) accepts `ch`. +// An unparseable source is treated as a match (fail closed toward rejection). +function setMatchesChar(source: string, ch: string): boolean { + try { + return new RegExp(`^(?:${source})$`).test(ch); + } catch { + return true; + } +} + function literalPrefix(branch: string): string { let out = ''; for (let i = 0; i < branch.length; i++) { diff --git a/src/core/parser-vscode-files.test.ts b/src/core/parser-vscode-files.test.ts index d2a9e893..20c833e8 100644 --- a/src/core/parser-vscode-files.test.ts +++ b/src/core/parser-vscode-files.test.ts @@ -199,6 +199,28 @@ describe('reconstructFromJsonl', () => { expect(mode.id).toBe('agent'); }); }); + + it('does not pollute Object.prototype via a __proto__ set path', () => { + const lines = [ + JSON.stringify({ kind: 0, v: { ok: true } }), + JSON.stringify({ kind: 1, k: ['__proto__', 'polluted'], v: 'pwned' }), + ].join('\n'); + withTempFile('proto-set.jsonl', lines, (filePath) => { + reconstructFromJsonl(filePath); + expect(({} as Record).polluted).toBeUndefined(); + }); + }); + + it('does not pollute Object.prototype via a constructor.prototype append path', () => { + const lines = [ + JSON.stringify({ kind: 0, v: { ok: true } }), + JSON.stringify({ kind: 2, k: ['constructor', 'prototype', 'tainted'], v: ['x'] }), + ].join('\n'); + withTempFile('proto-append.jsonl', lines, (filePath) => { + reconstructFromJsonl(filePath); + expect(({} as Record).tainted).toBeUndefined(); + }); + }); }); describe('parseWorkspaceName', () => { diff --git a/src/core/parser-vscode-files.ts b/src/core/parser-vscode-files.ts index 0355d5c5..c1558a01 100644 --- a/src/core/parser-vscode-files.ts +++ b/src/core/parser-vscode-files.ts @@ -107,6 +107,12 @@ type JsonValue = string | number | boolean | null | JsonValue[] | { [key: string type JsonObject = Record; type PathKey = string | number; +const FORBIDDEN_KEYS = new Set(['__proto__', 'constructor', 'prototype']); + +function isForbiddenKey(key: PathKey): boolean { + return typeof key === 'string' && FORBIDDEN_KEYS.has(key); +} + function isJsonObject(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } @@ -135,16 +141,18 @@ function setAtPath(obj: JsonValue, keys: PathKey[], value: JsonValue): void { let current = obj; for (let i = 0; i < keys.length - 1; i++) { const key = keys[i]; + if (isForbiddenKey(key)) return; if (typeof key === 'number' && Array.isArray(current)) { while (current.length <= key) current.push(null); if (current[key] === null) current[key] = {}; current = current[key]!; } else if (typeof current === 'object' && current !== null && !Array.isArray(current)) { - if (!(key as string in current)) (current as JsonObject)[key as string] = {}; + if (!Object.prototype.hasOwnProperty.call(current, key as string)) (current as JsonObject)[key as string] = {}; current = (current as JsonObject)[key as string]; } } const last = keys[keys.length - 1]; + if (isForbiddenKey(last)) return; if (Array.isArray(current)) { while (current.length <= (last as number)) current.push(null); current[last as number] = value; @@ -156,10 +164,11 @@ function setAtPath(obj: JsonValue, keys: PathKey[], value: JsonValue): void { function appendAtPath(obj: JsonValue, keys: PathKey[], items: JsonValue): void { let target: JsonValue = obj; for (const key of keys) { + if (isForbiddenKey(key)) return; if (typeof key === 'number' && Array.isArray(target)) { target = target[key]; } else if (typeof target === 'object' && target !== null && !Array.isArray(target)) { - if (!(key as string in target)) (target as JsonObject)[key as string] = []; + if (!Object.prototype.hasOwnProperty.call(target, key as string)) (target as JsonObject)[key as string] = []; target = (target as JsonObject)[key as string]; } } diff --git a/src/webview/fetch-utils.test.ts b/src/webview/fetch-utils.test.ts new file mode 100644 index 00000000..fb7d8cd4 --- /dev/null +++ b/src/webview/fetch-utils.test.ts @@ -0,0 +1,24 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { describe, expect, it } from 'vitest'; +import { readTextWithByteLimit } from './fetch-utils'; + +describe('readTextWithByteLimit', () => { + it('reads content under the byte limit', async () => { + const response = new Response('hello', { headers: { 'content-length': '5' } }); + await expect(readTextWithByteLimit(response, 5, 'too large')).resolves.toBe('hello'); + }); + + it('rejects when content-length exceeds the limit', async () => { + const response = new Response('hello', { headers: { 'content-length': '6' } }); + await expect(readTextWithByteLimit(response, 5, 'too large')).rejects.toThrow('too large'); + }); + + it('rejects streamed content after the byte limit is exceeded', async () => { + const response = new Response('hello'); + await expect(readTextWithByteLimit(response, 4, 'too large')).rejects.toThrow('too large'); + }); +}); diff --git a/src/webview/fetch-utils.ts b/src/webview/fetch-utils.ts new file mode 100644 index 00000000..835718cb --- /dev/null +++ b/src/webview/fetch-utils.ts @@ -0,0 +1,37 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See LICENSE in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +export async function readTextWithByteLimit(response: Response, maxBytes: number, tooLargeMessage: string): Promise { + const declared = Number(response.headers.get('content-length')); + if (Number.isFinite(declared) && declared > maxBytes) { + throw new Error(tooLargeMessage); + } + + if (!response.body) { + const text = await response.text(); + if (new TextEncoder().encode(text).byteLength > maxBytes) { + throw new Error(tooLargeMessage); + } + return text; + } + + const reader = response.body.getReader(); + const decoder = new TextDecoder(); + let bytes = 0; + let text = ''; + + while (true) { + const { done, value } = await reader.read(); + if (done) break; + bytes += value.byteLength; + if (bytes > maxBytes) { + await reader.cancel(); + throw new Error(tooLargeMessage); + } + text += decoder.decode(value, { stream: true }); + } + + return text + decoder.decode(); +} diff --git a/src/webview/panel-catalog.ts b/src/webview/panel-catalog.ts index e23d7019..acdf87c3 100644 --- a/src/webview/panel-catalog.ts +++ b/src/webview/panel-catalog.ts @@ -3,8 +3,12 @@ * Licensed under the MIT License. See LICENSE in the project root for license information. *--------------------------------------------------------------------------------------------*/ +import { readTextWithByteLimit } from './fetch-utils'; + export const CATALOG_BASE = 'https://awesome-copilot.github.com'; +const CATALOG_PAGE_MAX_BYTES = 5 * 1024 * 1024; + export interface RawCatalogItem { kind: 'skill' | 'agent' | 'instruction' | 'hook'; id: string; @@ -31,9 +35,9 @@ function stripHtml(text: string): string { async function fetchCatalogPage(slug: string, kind: RawCatalogItem['kind']): Promise { const url = `${CATALOG_BASE}/${slug}/`; - const response = await fetch(url); + const response = await fetch(url, { redirect: 'error' }); if (!response.ok) return []; - const html = await response.text(); + const html = await readTextWithByteLimit(response, CATALOG_PAGE_MAX_BYTES, 'Catalog page too large'); const items: RawCatalogItem[] = []; const articleRegex = /]*data-path="([^"]*)"[^>]*>([\s\S]*?)<\/article>/g; diff --git a/src/webview/panel-llm.ts b/src/webview/panel-llm.ts index aa8a2760..deffa4cc 100644 --- a/src/webview/panel-llm.ts +++ b/src/webview/panel-llm.ts @@ -6,6 +6,7 @@ /* LLM schemas and request helpers for the dashboard panel. */ import * as vscode from 'vscode'; +import { runtimeDebug } from '../core/runtime-debug'; export interface JsonSchemaSpec { name: string; @@ -136,10 +137,10 @@ export const SCHEMA_TRIAGE: JsonSchemaSpec = { items: { type: 'object', properties: { - id: { type: 'string' }, - verdict: { type: 'string', enum: ['strong', 'maybe', 'skip'] }, - reason: { type: 'string' }, - suggestedSkillName: { type: ['string', 'null'] }, + id: { type: 'string', description: 'The cluster id this verdict refers to, copied from the input.' }, + verdict: { type: 'string', enum: ['strong', 'maybe', 'skip'], description: 'Whether the cluster is a strong, maybe, or skip candidate for a skill file.' }, + reason: { type: 'string', description: 'One sentence explaining the verdict.' }, + suggestedSkillName: { type: 'string', description: 'Short kebab-case skill name, or an empty string when no skill is suggested.' }, }, required: ['id', 'verdict', 'reason', 'suggestedSkillName'], additionalProperties: false, @@ -277,30 +278,46 @@ function parseLlmJson(text: string): T { try { return JSON.parse(fixed) as T; } catch { /* fall through */ } - // Attempt 3: balance unmatched brackets - const opens = (fixed.match(/[{[]/g) || []).length; - const closes = (fixed.match(/[}\]]/g) || []).length; - for (let i = 0; i < opens - closes; i++) { - const lastOpen = Math.max(fixed.lastIndexOf('{'), fixed.lastIndexOf('[')); - fixed += fixed[lastOpen] === '{' ? '}' : ']'; - } + // Attempt 3: close a truncated response by balancing unclosed strings and + // brackets in the correct order, then dropping any dangling trailing comma. + const balanced = balanceTruncatedJson(fixed).replaceAll(/,(\s*[}\]])/g, '$1'); + try { return JSON.parse(balanced) as T; } catch { /* fall through */ } - try { return JSON.parse(fixed) as T; } catch { /* fall through */ } + throw new Error('Failed to parse JSON from LLM response'); +} - // Attempt 4: truncate to last complete object in an array - const lastCompleteA = fixed.lastIndexOf('}]'); - const lastCompleteB = fixed.lastIndexOf('},'); - const lastComplete = Math.max(lastCompleteA, lastCompleteB); - if (lastComplete > 0) { - const truncated = fixed.slice(0, lastComplete + 1) + ']'; - try { return JSON.parse(truncated) as T; } catch { /* fall through */ } +/** + * Repair JSON that was cut off mid-stream (e.g. when the model hit its output + * token limit). Walks the text tracking string state and a stack of open + * brackets, then appends the closers needed to make it parseable. Works for + * both array-root and object-wrapped payloads. + */ +function balanceTruncatedJson(input: string): string { + const closers: string[] = []; + let inString = false; + let escaped = false; + + for (const char of input) { + if (inString) { + if (escaped) escaped = false; + else if (char === '\\') escaped = true; + else if (char === '"') inString = false; + continue; + } + if (char === '"') inString = true; + else if (char === '{') closers.push('}'); + else if (char === '[') closers.push(']'); + else if (char === '}' || char === ']') closers.pop(); } - throw new Error('Failed to parse JSON from LLM response'); + let result = input; + if (inString) result += '"'; + for (let i = closers.length - 1; i >= 0; i--) result += closers[i]; + return result; } const LLM_MAX_RETRIES = 2; -const LLM_FAMILY = 'gpt-4.1'; +const LLM_FAMILY = 'gpt-5.4-mini'; /** Hard cap for a single LLM streaming request (ms). Prevents the UI from * spinning forever when the model hangs or the user never grants consent. */ const LLM_REQUEST_TIMEOUT_MS = 90_000; @@ -311,7 +328,7 @@ const LLM_REQUEST_TIMEOUT_MS = 90_000; * nothing is available so callers can surface a useful message. */ async function selectModel(): Promise { - const families = [LLM_FAMILY, 'gpt-4.1', 'gpt-4.1-mini', 'gpt-4']; + const families = [LLM_FAMILY, 'gpt-5-mini', 'gpt-4.1-mini', 'gpt-4.1']; for (const family of families) { const models = await vscode.lm.selectChatModels({ family }); if (models.length > 0) return models[0]; @@ -370,9 +387,9 @@ export async function callLlmJson(messages: vscode.LanguageModelChatMessage[] for (let attempt = 0; attempt <= LLM_MAX_RETRIES; attempt++) { const cts = new vscode.CancellationTokenSource(); + let text = ''; try { const response = await model.sendRequest(retryMessages, options, cts.token); - let text = ''; for await (const chunk of response.text) text += chunk; try { return JSON.parse(text.trim()) as T; @@ -381,9 +398,14 @@ export async function callLlmJson(messages: vscode.LanguageModelChatMessage[] } } catch (err) { lastError = err; + const schemaName = jsonSchema?.name ?? 'none'; + runtimeDebug('panel-llm', 'call-failed', + `schema=${schemaName} attempt=${attempt + 1} structured=${options.modelOptions !== undefined} ` + + `model=${model.id} textLen=${text.length} error=${err instanceof Error ? err.message : String(err)}`); if (err instanceof vscode.CancellationError) { cts.dispose(); throw err; } - // If structured output isn't supported, fall back to plain mode - if (attempt === 0 && jsonSchema && lastError instanceof Error && /response_format|modelOptions|not supported/i.test(lastError.message)) { + // Drop structured output so later attempts can recover in plain mode. + if (jsonSchema && options.modelOptions && lastError instanceof Error && + /response_format|modelOptions|not supported|JSON|parse/i.test(lastError.message)) { options.modelOptions = undefined; } // On parse failures, nudge the model to return valid JSON on the next attempt diff --git a/src/webview/panel-request-service.ts b/src/webview/panel-request-service.ts index a5a09260..247292f1 100644 --- a/src/webview/panel-request-service.ts +++ b/src/webview/panel-request-service.ts @@ -24,8 +24,9 @@ import { SCHEMA_TRIAGE, } from './panel-llm'; import { getCatalogItems } from './panel-catalog'; +import { readTextWithByteLimit } from './fetch-utils'; import { validateDateFilter } from './panel-rpc'; -import { isNumber, isOptionalString, isRecord, isString, postError, postEvent, postResponse, RequestMessage } from './panel-shared'; +import { isNumber, isOptionalString, isRecord, isString, postError, postEvent, postResponse, RequestMessage, safeJoinUnder } from './panel-shared'; type CustomPanelMethodName = | 'createSkill' @@ -73,6 +74,12 @@ type QuizQuestion = { const QUIZ_DIFFICULTIES: ReadonlySet = new Set(['easy', 'medium', 'hard']); +const CATALOG_MAX_BYTES = 1024 * 1024; + +async function readCappedText(response: Response): Promise { + return readTextWithByteLimit(response, CATALOG_MAX_BYTES, 'Catalog item too large'); +} + function getStringArray(value: unknown, limit: number): string[] { return Array.isArray(value) ? value.filter(isString).slice(0, limit) : []; } @@ -127,8 +134,9 @@ export class PanelRequestService { ) {} tryHandle(msg: RequestMessage): boolean { + if (!Object.prototype.hasOwnProperty.call(this.handlers, msg.method)) return false; const handler = this.handlers[msg.method as CustomPanelMethodName]; - if (!handler) return false; + if (typeof handler !== 'function') return false; void Promise.resolve(handler(msg)).catch((error: unknown) => { postError(this.webview, msg.id, error instanceof Error ? error.message : 'Internal error'); }); @@ -601,18 +609,21 @@ Respond with a JSON object: {"items":[{"title":"...","url":"https://...","type": postError(this.webview, msg.id, 'Missing filename or content'); return; } - if (filename.includes('..') || filename.startsWith('/')) { + + const homeDir = process.env.HOME || process.env.USERPROFILE; + if (!homeDir) { + postError(this.webview, msg.id, 'Cannot determine home directory'); + return; + } + const targetPath = safeJoinUnder(path.join(homeDir, '.agents', 'skills'), filename.split('/'), { allowedExts: ['.md'] }); + if (!targetPath) { postError(this.webview, msg.id, 'Invalid filename'); return; } try { - const homeDir = process.env.HOME || process.env.USERPROFILE; - if (!homeDir) { - postError(this.webview, msg.id, 'Cannot determine home directory'); - return; - } - const targetUri = vscode.Uri.file(`${homeDir}/.agents/skills/${filename}`); + const targetUri = vscode.Uri.file(targetPath); + await vscode.workspace.fs.createDirectory(vscode.Uri.file(path.dirname(targetPath))); await vscode.workspace.fs.writeFile(targetUri, Buffer.from(content, 'utf8')); postResponse(this.webview, msg.id, { ok: true, path: targetUri.fsPath }); } catch (error: unknown) { @@ -637,18 +648,21 @@ Respond with a JSON object: {"items":[{"title":"...","url":"https://...","type": postError(this.webview, msg.id, 'Invalid catalog URL'); return; } - const response = await fetch(parsedUrl.toString()); + const response = await fetch(parsedUrl.toString(), { redirect: 'error' }); if (!response.ok) throw new Error(`Failed to fetch: ${response.status}`); - const content = await response.text(); + const content = await readCappedText(response); const homeDir = process.env.HOME || process.env.USERPROFILE; if (!homeDir) throw new Error('Cannot determine home directory'); const subDir = kind === 'agent' ? 'agents' : 'skills'; const slug = title.toLowerCase().replaceAll(/[^a-z0-9]+/g, '-').replaceAll(/-+/g, '-').replaceAll(/^-|-$/g, ''); const filename = catalogPath.split('/').pop() || `${slug}.md`; - if (slug.includes('..') || filename.includes('..')) throw new Error('Invalid path'); - const targetUri = vscode.Uri.file(`${homeDir}/.agents/${subDir}/${slug}/${filename}`); + const targetPath = safeJoinUnder(path.join(homeDir, '.agents', subDir), [slug, filename], { allowedExts: ['.md'] }); + if (!targetPath) throw new Error('Invalid path'); + + const targetUri = vscode.Uri.file(targetPath); + await vscode.workspace.fs.createDirectory(vscode.Uri.file(path.dirname(targetPath))); await vscode.workspace.fs.writeFile(targetUri, Buffer.from(content, 'utf8')); postResponse(this.webview, msg.id, { content, filename: `${slug}/${filename}` }); } catch (error: unknown) { @@ -1158,7 +1172,7 @@ ${contextSection}`; try { const prResponse = await fetch( `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/pulls?state=all&per_page=50&sort=updated&direction=desc`, - { headers }, + { headers, redirect: 'error' }, ); if (!prResponse.ok) return stats; @@ -1184,7 +1198,7 @@ ${contextSection}`; private async fetchGitHubCount(url: string, headers: Record): Promise { try { - const response = await fetch(url, { headers }); + const response = await fetch(url, { headers, redirect: 'error' }); if (!response.ok) return null; const data = await response.json() as { total_count?: number } | Array; if (Array.isArray(data)) return data.length; @@ -1198,7 +1212,7 @@ ${contextSection}`; try { const collabResponse = await fetch( `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/collaborators?per_page=100`, - { headers }, + { headers, redirect: 'error' }, ); if (!collabResponse.ok) return []; const collaborators = await collabResponse.json() as Array<{ login?: string }>; @@ -1307,8 +1321,13 @@ ${contextSection}`; postError(this.webview, msg.id, 'Missing owner/repo'); return; } + if (owner !== '_auth_' && (!/^[A-Za-z0-9._-]+$/.test(owner) || !/^[A-Za-z0-9._-]+$/.test(repo))) { + postError(this.webview, msg.id, 'Invalid owner/repo'); + return; + } - const token = await this.getGitHubAccessToken(params.requestAuth === true); + const isAuthProbe = owner === '_auth_'; + const token = await this.getGitHubAccessToken(isAuthProbe && params.requestAuth === true); if (!token) { postResponse(this.webview, msg.id, { authRequired: true, @@ -1317,7 +1336,7 @@ ${contextSection}`; return; } - if (owner === '_auth_') { + if (isAuthProbe) { postResponse(this.webview, msg.id, { authRequired: false }); return; } diff --git a/src/webview/panel-rpc.ts b/src/webview/panel-rpc.ts index b6e70891..6f3e7e6b 100644 --- a/src/webview/panel-rpc.ts +++ b/src/webview/panel-rpc.ts @@ -23,7 +23,7 @@ import { import { runDetectors, runEmitters } from '../core/detector-registry'; import { parsePipeline, executePipeline, checkPipelineTrigger, resolveInheritance } from '../core/rule-pipeline'; import { parseRule, serializeRule } from '../core/rule-parser'; -import { getRuleLayerInfo, getPersonalRulesDir } from '../core/rule-loader'; +import { getRuleLayerInfo, getPersonalRulesDir, getProjectRulesDir } from '../core/rule-loader'; import { getPending, approve as approveTrust, getDefaultTrustStore } from '../core/rule-trust'; import { isoWeek } from '../core/helpers'; import { FIELD_SCHEMA, METRIC_PRIMITIVES, FUNCTION_CATALOG, compileFilter, validateExpression } from '../core/dsl/index'; @@ -52,6 +52,52 @@ function pickRows(scope: string, reqs: SessionRequest[], sessions: Session[]): D return (isSessionScope ? sessions : reqs) as unknown as DslRow[]; } +function isPathUnder(pathMod: typeof import('path'), resolved: string, dir: string): boolean { + const base = pathMod.resolve(dir); + return resolved === base || resolved.startsWith(base + pathMod.sep); +} + +// `inAllowed` gates the write entirely; `isPersonal` gates auto-trust. +function classifyRuleWritePath(pathMod: typeof import('path'), filePath: string): { + resolved: string; + inAllowed: boolean; + isPersonal: boolean; +} { + const personalDir = getPersonalRulesDir(); + let workspaceRoot: string | undefined; + try { + // eslint-disable-next-line @typescript-eslint/no-require-imports + const vscode = require('vscode') as typeof import('vscode'); + workspaceRoot = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath; + } catch { /* test context */ } + const allowedDirs = [personalDir, ...(workspaceRoot ? [getProjectRulesDir(workspaceRoot)] : [])]; + const resolved = pathMod.resolve(filePath); + return { + resolved, + inAllowed: allowedDirs.some(d => isPathUnder(pathMod, resolved, d)), + isPersonal: isPathUnder(pathMod, resolved, personalDir), + }; +} + +function resolveRuleFilePath( + fsMod: typeof import('fs'), + pathMod: typeof import('path'), + parsed: NonNullable>, + ruleIdParam: string, +): string { + if (ruleIdParam) { + const existing = getRule(ruleIdParam); + if (existing?.sourceFilePath && (existing.source === 'personal' || existing.source === 'project')) { + return existing.sourceFilePath; + } + } + + const dir = getPersonalRulesDir(); + try { fsMod.mkdirSync(dir, { recursive: true }); } catch { /* ignore */ } + const safeId = parsed.id.replaceAll(/[^a-zA-Z0-9_-]+/g, '-').replaceAll(/^-|-$/g, '') || 'custom-rule'; + return pathMod.join(dir, `${safeId}.md`); +} + export function validateDateFilter(p: Record): DateFilter | undefined { const f: DateFilter = {}; if (isString(p.fromDate)) f.fromDate = p.fromDate; @@ -810,19 +856,10 @@ const rpcHandlers: TypedRpcHandlers = { // eslint-disable-next-line @typescript-eslint/no-require-imports const path = require('path') as typeof import('path'); - let filePath = ''; - if (ruleIdParam) { - const existing = getRule(ruleIdParam); - if (existing?.sourceFilePath && (existing.source === 'personal' || existing.source === 'project')) { - filePath = existing.sourceFilePath; - } - } - if (!filePath) { - const dir = getPersonalRulesDir(); - try { fs.mkdirSync(dir, { recursive: true }); } catch { /* ignore */ } - const safeId = parsed.id.replaceAll(/[^a-zA-Z0-9_-]+/g, '-').replaceAll(/^-|-$/g, '') || 'custom-rule'; - filePath = path.join(dir, `${safeId}.md`); - } + const filePath = resolveRuleFilePath(fs, path, parsed, ruleIdParam); + + const { inAllowed, isPersonal } = classifyRuleWritePath(path, filePath); + if (!inAllowed) return { ok: false, error: 'Refusing to write outside rules directories' }; try { fs.writeFileSync(filePath, markdown, 'utf-8'); @@ -831,7 +868,7 @@ const rpcHandlers: TypedRpcHandlers = { } const store = getDefaultTrustStore(); - if (store) { + if (store && isPersonal) { try { await approveTrust(store, filePath, markdown); } catch { /* ignore */ } } @@ -1282,5 +1319,7 @@ function buildNumericHistogram(sorted: number[], buckets: number): { label: stri } export function getRpcHandler(method: string): RpcHandler | undefined { - return rpcHandlers[method as RpcMethodName]; -} \ No newline at end of file + if (!Object.prototype.hasOwnProperty.call(rpcHandlers, method)) return undefined; + const handler = rpcHandlers[method as RpcMethodName]; + return typeof handler === 'function' ? handler : undefined; +} diff --git a/src/webview/panel-shared.test.ts b/src/webview/panel-shared.test.ts index f76ab7e5..6f147572 100644 --- a/src/webview/panel-shared.test.ts +++ b/src/webview/panel-shared.test.ts @@ -3,8 +3,9 @@ * Licensed under the MIT License. See LICENSE in the project root for license information. *--------------------------------------------------------------------------------------------*/ +import * as path from 'path'; import { describe, it, expect } from 'vitest'; -import { escapeHtmlAttr } from './panel-shared'; +import { escapeHtmlAttr, isSafeExternalHttpsUrl, safeJoinUnder, isRequestMessage } from './panel-shared'; describe('escapeHtmlAttr', () => { it('escapes all HTML-special characters', () => { @@ -30,3 +31,83 @@ describe('escapeHtmlAttr', () => { expect(escapeHtmlAttr('')).toBe(''); }); }); + +describe('safeJoinUnder', () => { + const base = path.resolve('/tmp/agents/skills'); + + it('joins a simple safe filename under the base', () => { + expect(safeJoinUnder(base, ['my-skill.md'])).toBe(path.join(base, 'my-skill.md')); + }); + + it('joins nested safe segments', () => { + expect(safeJoinUnder(base, ['my-slug', 'file.md'])).toBe(path.join(base, 'my-slug', 'file.md')); + }); + + it('rejects parent-traversal segments', () => { + expect(safeJoinUnder(base, ['..', 'evil.md'])).toBeNull(); + expect(safeJoinUnder(base, ['..'])).toBeNull(); + }); + + it('rejects segments containing a path separator', () => { + expect(safeJoinUnder(base, ['foo/bar.md'])).toBeNull(); + expect(safeJoinUnder(base, ['foo\\bar.md'])).toBeNull(); + }); + + it('rejects absolute-path-like and special segments', () => { + expect(safeJoinUnder(base, ['.'])).toBeNull(); + expect(safeJoinUnder(base, [''])).toBeNull(); + expect(safeJoinUnder(base, ['a:b'])).toBeNull(); + }); + + it('rejects empty segment list', () => { + expect(safeJoinUnder(base, [])).toBeNull(); + }); + + it('enforces an extension allowlist on the final segment', () => { + expect(safeJoinUnder(base, ['note.txt'], { allowedExts: ['.md'] })).toBeNull(); + expect(safeJoinUnder(base, ['note.md'], { allowedExts: ['.md'] })).toBe(path.join(base, 'note.md')); + }); +}); + +describe('isSafeExternalHttpsUrl', () => { + it('accepts a normal HTTPS URL', () => { + expect(isSafeExternalHttpsUrl('https://example.com/docs?q=1#top')).toBe(true); + }); + + it('rejects non-HTTPS and protocol-handler URLs', () => { + expect(isSafeExternalHttpsUrl('http://example.com')).toBe(false); + expect(isSafeExternalHttpsUrl('file:///tmp/a')).toBe(false); + expect(isSafeExternalHttpsUrl('vscode://file/tmp/a')).toBe(false); + expect(isSafeExternalHttpsUrl('javascript:alert(1)')).toBe(false); + }); + + it('rejects malformed, credentialed, and control-character URLs', () => { + expect(isSafeExternalHttpsUrl('https:example.com')).toBe(false); + expect(isSafeExternalHttpsUrl('https://user:pass@example.com')).toBe(false); + expect(isSafeExternalHttpsUrl('https://example.com/\nfile://x')).toBe(false); + }); +}); + +describe('isRequestMessage', () => { + it('accepts a well-formed request with object params', () => { + expect(isRequestMessage({ type: 'request', id: '1', method: 'foo', params: { a: 1 } })).toBe(true); + }); + + it('accepts a request without params', () => { + expect(isRequestMessage({ type: 'request', id: '1', method: 'foo' })).toBe(true); + }); + + it('rejects array params', () => { + expect(isRequestMessage({ type: 'request', id: '1', method: 'foo', params: [1, 2] })).toBe(false); + }); + + it('rejects primitive params', () => { + expect(isRequestMessage({ type: 'request', id: '1', method: 'foo', params: 'x' })).toBe(false); + }); + + it('rejects missing id/method or wrong type', () => { + expect(isRequestMessage({ type: 'request', method: 'foo' })).toBe(false); + expect(isRequestMessage({ type: 'event', id: '1', method: 'foo' })).toBe(false); + expect(isRequestMessage(null)).toBe(false); + }); +}); diff --git a/src/webview/panel-shared.ts b/src/webview/panel-shared.ts index e30b3d0a..4d8553c2 100644 --- a/src/webview/panel-shared.ts +++ b/src/webview/panel-shared.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import * as crypto from 'crypto'; +import * as path from 'path'; import * as vscode from 'vscode'; import { WebviewMessage, ErrorResult } from '../core/types'; @@ -36,7 +37,10 @@ export function isRecord(value: unknown): value is Record { export function isRequestMessage(value: unknown): value is RequestMessage { if (typeof value !== 'object' || value === null) return false; const record = value as Record; - return record.type === 'request' && isString(record.id) && isString(record.method); + if (record.type !== 'request' || !isString(record.id) || !isString(record.method)) return false; + // Handlers index into params by key, so reject arrays and primitives. + if (record.params !== undefined && !isRecord(record.params)) return false; + return true; } export function postResponse(webview: vscode.Webview, id: string, data: unknown): void { @@ -63,4 +67,48 @@ export function escapeHtmlAttr(s: string): string { .replaceAll('>', '>') .replaceAll('"', '"') .replaceAll("'", '''); -} \ No newline at end of file +} + +function hasUrlControlChars(value: string): boolean { + for (let i = 0; i < value.length; i++) { + const code = value.charCodeAt(i); + if (code <= 0x1F || code === 0x7F) return true; + } + return false; +} + +export function isSafeExternalHttpsUrl(value: unknown): value is string { + if (typeof value !== 'string' || hasUrlControlChars(value) || !value.toLowerCase().startsWith('https://')) { + return false; + } + try { + const url = new URL(value); + return url.protocol === 'https:' && url.hostname.length > 0 && !url.username && !url.password; + } catch { + return false; + } +} + +/** Characters permitted in a single user-supplied path segment. */ +const SAFE_SEGMENT = /^[A-Za-z0-9._-]+$/; + +export function safeJoinUnder( + baseDir: string, + segments: string[], + opts?: { allowedExts?: string[] }, +): string | null { + if (segments.length === 0) return null; + for (const segment of segments) { + if (!segment || segment === '.' || segment === '..' || !SAFE_SEGMENT.test(segment)) return null; + } + + const finalSegment = segments[segments.length - 1]; + if (opts?.allowedExts && !opts.allowedExts.includes(path.extname(finalSegment).toLowerCase())) { + return null; + } + + const resolvedBase = path.resolve(baseDir); + const resolved = path.resolve(resolvedBase, ...segments); + if (resolved !== resolvedBase && !resolved.startsWith(resolvedBase + path.sep)) return null; + return resolved; +} diff --git a/src/webview/panel-sidebar.ts b/src/webview/panel-sidebar.ts index 6f0b59c9..bfa0e752 100644 --- a/src/webview/panel-sidebar.ts +++ b/src/webview/panel-sidebar.ts @@ -28,7 +28,9 @@ export class DashboardSidebarProvider implements vscode.WebviewViewProvider { webviewView.webview.html = this.renderHtml(webviewView.webview); webviewView.webview.onDidReceiveMessage((msg: { command: string }) => { - void vscode.commands.executeCommand(msg.command); + if (msg.command === 'aiEngineerCoach.open' || msg.command === 'aiEngineerCoach.reload') { + void vscode.commands.executeCommand(msg.command); + } }); } diff --git a/src/webview/panel.ts b/src/webview/panel.ts index 485b7f78..3c8705b2 100644 --- a/src/webview/panel.ts +++ b/src/webview/panel.ts @@ -18,7 +18,7 @@ import { getDashboardHtml, getErrorHtml } from './panel-html'; import { getRpcHandler } from './panel-rpc'; import { PanelRequestService } from './panel-request-service'; import { DashboardSidebarProvider } from './panel-sidebar'; -import { isRequestMessage, postResponse, errorResult } from './panel-shared'; +import { isRequestMessage, isSafeExternalHttpsUrl, postResponse, errorResult } from './panel-shared'; export { DashboardSidebarProvider } from './panel-sidebar'; @@ -273,11 +273,7 @@ export class DashboardPanel { // Open external URLs from webview if (msg.method === 'openExternal') { - const url = (msg.params as Record | undefined)?.url; - if (typeof url === 'string') { - void vscode.env.openExternal(vscode.Uri.parse(url)); - postResponse(this.panel.webview, msg.id, { ok: true }); - } + this.handleOpenExternal(msg); return; } @@ -331,6 +327,16 @@ export class DashboardPanel { } } + private handleOpenExternal(msg: Extract): void { + const url = (msg.params as Record | undefined)?.url; + if (isSafeExternalHttpsUrl(url)) { + void vscode.env.openExternal(vscode.Uri.parse(url)); + try { postResponse(this.panel.webview, msg.id, { ok: true }); } catch { /* disposed */ } + } else { + try { postResponse(this.panel.webview, msg.id, errorResult('Invalid external URL')); } catch { /* disposed */ } + } + } + private static readonly BUDGET_STATE_KEY = 'modelBudgets'; private handleBudgetMessage(msg: Extract): void {