diff --git a/src/services/mcp/oauthPort.test.ts b/src/services/mcp/oauthPort.test.ts index 78cd80a6e..c6d578991 100644 --- a/src/services/mcp/oauthPort.test.ts +++ b/src/services/mcp/oauthPort.test.ts @@ -1,6 +1,8 @@ import assert from 'node:assert/strict' import { createServer } from 'node:http' +import { networkInterfaces } from 'node:os' import test from 'node:test' +import { fileURLToPath } from 'node:url' import { snapshotEnv } from '../../test/envSnapshot.js' import { getPlatform } from '../../utils/platform.js' @@ -22,7 +24,7 @@ test('OAuth callback port selection works without weak randomness and releases i assert.equal(buildRedirectUri(port), `http://localhost:${port}/callback`) await new Promise((resolve, reject) => { server.once('error', reject) - server.listen(port, resolve) + server.listen(port, '127.0.0.1', resolve) }) } finally { Math.random = originalRandom @@ -37,10 +39,159 @@ test('OAuth callback port selection works without weak randomness and releases i test('OAuth callback port selection preserves the configured port override', async () => { const restoreEnv = snapshotEnv(['MCP_OAUTH_CALLBACK_PORT']) - process.env.MCP_OAUTH_CALLBACK_PORT = '43210' + const server = createServer() try { - assert.equal(await findAvailablePort(), 43210) + await new Promise((resolve, reject) => { + server.once('error', reject) + server.listen(0, '127.0.0.1', resolve) + }) + const address = server.address() + assert.ok(address && typeof address === 'object') + await new Promise(resolve => server.close(() => resolve())) + process.env.MCP_OAUTH_CALLBACK_PORT = String(address.port) + assert.equal(await findAvailablePort(), address.port) + } finally { + restoreEnv() + if (server.listening) await new Promise(resolve => server.close(() => resolve())) + } +}) + + +test('OAuth callback port overrides reject incomplete and out-of-range values', async () => { + const restoreEnv = snapshotEnv(['MCP_OAUTH_CALLBACK_PORT']) + try { + for (const value of ['70000', '12345abc', '12345.5', '-1', '0', ' 12345', '12345\n']) { + process.env.MCP_OAUTH_CALLBACK_PORT = value + const port = await findAvailablePort() + const windows = getPlatform() === 'windows' + assert.ok(port === 3118 || (port >= (windows ? 39152 : 49152) && port <= (windows ? 49151 : 65535)), value) + } } finally { restoreEnv() } }) + +test('OAuth callback port selection skips an occupied override and releases the selected socket', async () => { + const restoreEnv = snapshotEnv(['MCP_OAUTH_CALLBACK_PORT']) + const occupied = createServer() + const selected = createServer() + try { + await new Promise((resolve, reject) => { + occupied.once('error', reject) + occupied.listen(0, '127.0.0.1', resolve) + }) + const address = occupied.address() + assert.ok(address && typeof address === 'object') + process.env.MCP_OAUTH_CALLBACK_PORT = String(address.port) + const port = await findAvailablePort() + assert.notEqual(port, address.port) + await new Promise((resolve, reject) => { + selected.once('error', reject) + selected.listen(port, '127.0.0.1', resolve) + }) + } finally { + restoreEnv() + for (const server of [occupied, selected]) { + if (server.listening) await new Promise(resolve => server.close(() => resolve())) + } + } +}) + + +test('OAuth callback override remains usable when another interface owns the port', async t => { + const otherAddress = Object.values(networkInterfaces()).flat() + .find(address => address?.family === 'IPv4' && !address.internal)?.address + if (!otherAddress) { + t.skip('No secondary IPv4 interface available') + return + } + const restoreEnv = snapshotEnv(['MCP_OAUTH_CALLBACK_PORT']) + const other = createServer() + const callback = createServer() + try { + await new Promise((resolve, reject) => { + other.once('error', reject) + other.listen(0, otherAddress, resolve) + }) + const address = other.address() + assert.ok(address && typeof address === 'object') + process.env.MCP_OAUTH_CALLBACK_PORT = String(address.port) + assert.equal(await findAvailablePort(), address.port) + await new Promise((resolve, reject) => { + callback.once('error', reject) + callback.listen(address.port, '127.0.0.1', resolve) + }) + } finally { + restoreEnv() + for (const server of [other, callback]) { + if (server.listening) await new Promise(resolve => server.close(() => resolve())) + } + } +}) + + +// Exercise Node's dual-stack binding; Bun's HTTP listener defaults differ. +// Leave five seconds beyond the child deadline for assertions and cleanup. +test('random OAuth callback selection rejects IPv6-occupied ports in Node', { timeout: 15000 }, t => { + const child = Bun.spawnSync(['node', '--input-type=module', '--eval', String.raw` + import assert from 'node:assert/strict' + import { readFileSync } from 'node:fs' + import { createServer } from 'node:http' + import crypto from 'node:crypto' + import { registerHooks, stripTypeScriptTypes, syncBuiltinESMExports } from 'node:module' + import { pathToFileURL } from 'node:url' + const sourceUrl = pathToFileURL(process.env.OAUTH_PORT_SOURCE).href + registerHooks({ + resolve(specifier, context, next) { + // Only platform detection is isolated; execute the actual port selector + // and native Node socket operations without bundling the whole CLI. + if (specifier === '../../utils/platform.js') return { + url: 'data:text/javascript,export const getPlatform = () => ' + + JSON.stringify(process.platform === 'win32' ? 'windows' : 'linux'), + shortCircuit: true, + } + return next(specifier, context) + }, + load(url, context, next) { + if (url === sourceUrl) return { format: 'module', shortCircuit: true, + source: stripTypeScriptTypes(readFileSync(process.env.OAUTH_PORT_SOURCE, 'utf8')) } + return next(url, context) + }, + }) + delete process.env.MCP_OAUTH_CALLBACK_PORT + const { findAvailablePort } = await import(sourceUrl) + const ipv6 = createServer() + const originalRandomInt = crypto.randomInt + try { + const port = await findAvailablePort() + try { + await new Promise((resolve, reject) => { + ipv6.once('error', reject) + ipv6.listen(port, '::1', resolve) + }) + } catch (error) { + if (['EAFNOSUPPORT', 'EADDRNOTAVAIL'].includes(error.code)) { + console.log('SKIP: IPv6 loopback unavailable') + process.exit(0) + } + throw error + } + let calls = 0 + crypto.randomInt = (...args) => ++calls === 1 ? port : originalRandomInt(...args) + syncBuiltinESMExports() + assert.notEqual(await findAvailablePort(), port) + assert.ok(calls >= 2, 'selection must reject the held port and try another') + } finally { + crypto.randomInt = originalRandomInt + syncBuiltinESMExports() + if (ipv6.listening) await new Promise(resolve => ipv6.close(resolve)) + } + `], { + cwd: fileURLToPath(new URL('./', import.meta.url)), + stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', + env: { ...process.env, OAUTH_PORT_SOURCE: fileURLToPath(new URL('./oauthPort.ts', import.meta.url)) }, + timeout: 10000, + }) + assert.equal(child.exitCode, 0, child.stderr.toString()) + if (child.stdout.toString().includes('SKIP:')) t.skip('IPv6 loopback unavailable') +}) diff --git a/src/services/mcp/oauthPort.ts b/src/services/mcp/oauthPort.ts index ff94311ab..fd69c11e2 100644 --- a/src/services/mcp/oauthPort.ts +++ b/src/services/mcp/oauthPort.ts @@ -26,21 +26,42 @@ export function buildRedirectUri( } function getMcpOAuthCallbackPort(): number | undefined { - const port = parseInt(process.env.MCP_OAUTH_CALLBACK_PORT || '', 10) - return port > 0 ? port : undefined + const rawPort = process.env.MCP_OAUTH_CALLBACK_PORT + if (!rawPort || /[^0-9]/.test(rawPort)) return undefined + const port = Number(rawPort) + return Number.isInteger(port) && port >= 1 && port <= 65535 ? port : undefined +} + +function probePort(port: number, host?: string): Promise { + return new Promise((resolve, reject) => { + const testServer = createServer() + testServer.once('error', reject) + testServer.listen(port, host, () => { + testServer.close(error => error ? reject(error) : resolve()) + }) + }) } /** * Finds an available port in the specified range for OAuth redirect - * Uses random selection for better security + * Uses random selection for better security. + * An invalid or unavailable MCP_OAUTH_CALLBACK_PORT falls back to random/fallback + * selection, so the redirect URI can change. Providers requiring an exact + * pre-registered redirect URI may reject that flow; keep their configured port free. */ export async function findAvailablePort(): Promise { // First, try the configured port if specified const configuredPort = getMcpOAuthCallbackPort() if (configuredPort) { - return configuredPort + try { + await probePort(configuredPort, '127.0.0.1') + return configuredPort + } catch { + // The override is unavailable; use the usual random/fallback selection. + } } + // Preserve wildcard probes for random/fallback ports advertised as localhost. const { min, max } = REDIRECT_PORT_RANGE const range = max - min + 1 const maxAttempts = Math.min(range, 100) // Don't try forever @@ -49,13 +70,7 @@ export async function findAvailablePort(): Promise { const port = randomInt(min, max + 1) try { - await new Promise((resolve, reject) => { - const testServer = createServer() - testServer.once('error', reject) - testServer.listen(port, () => { - testServer.close(() => resolve()) - }) - }) + await probePort(port) return port } catch { // Port in use, try another random port @@ -65,13 +80,7 @@ export async function findAvailablePort(): Promise { // If random selection failed, try the fallback port try { - await new Promise((resolve, reject) => { - const testServer = createServer() - testServer.once('error', reject) - testServer.listen(REDIRECT_PORT_FALLBACK, () => { - testServer.close(() => resolve()) - }) - }) + await probePort(REDIRECT_PORT_FALLBACK) return REDIRECT_PORT_FALLBACK } catch { throw new Error(`No available ports for OAuth redirect`) diff --git a/src/services/tools/toolExecution.ts b/src/services/tools/toolExecution.ts index cbefd37db..8a0a9ff7b 100644 --- a/src/services/tools/toolExecution.ts +++ b/src/services/tools/toolExecution.ts @@ -88,7 +88,6 @@ import { withMemoryCorrectionHint, } from '../../utils/messages.js' import type { - PermissionDecisionReason, PermissionResult, } from '../../utils/permissions/PermissionResult.js' import { @@ -242,85 +241,6 @@ export function classifyToolError(error: unknown): string { return 'UnknownError' } -/** - * Map a rule's origin to the documented OTel `source` vocabulary, matching - * the interactive path's semantics (permissionLogging.ts:81): session-scoped - * grants are temporary, on-disk grants are permanent, and user-authored - * denies are user_reject regardless of persistence. Everything the user - * didn't write (cliArg, policySettings, projectSettings, flagSettings) is - * config. - */ -function ruleSourceToOTelSource( - ruleSource: string, - behavior: 'allow' | 'deny', -): string { - switch (ruleSource) { - case 'session': - return behavior === 'allow' ? 'user_temporary' : 'user_reject' - case 'localSettings': - case 'userSettings': - return behavior === 'allow' ? 'user_permanent' : 'user_reject' - default: - return 'config' - } -} - -/** - * Map a PermissionDecisionReason to the OTel `source` label for the - * non-interactive tool_decision path, staying within the documented - * vocabulary (config, hook, user_permanent, user_temporary, user_reject). - * - * For permissionPromptTool, the SDK host may set decisionClassification on - * the PermissionResult to tell us exactly what happened (once vs always vs - * cache hit — the host knows, we can't tell from {behavior:'allow'} alone). - * Without it, we fall back conservatively: allow → user_temporary, - * deny → user_reject. - */ -function decisionReasonToOTelSource( - reason: PermissionDecisionReason | undefined, - behavior: 'allow' | 'deny', -): string { - if (!reason) { - return 'config' - } - switch (reason.type) { - case 'permissionPromptTool': { - // toolResult is typed `unknown` on PermissionDecisionReason but carries - // the parsed Output from PermissionPromptToolResultSchema. Narrow at - // runtime rather than widen the cross-file type. - const toolResult = reason.toolResult as - | { decisionClassification?: string } - | undefined - const classified = toolResult?.decisionClassification - if ( - classified === 'user_temporary' || - classified === 'user_permanent' || - classified === 'user_reject' - ) { - return classified - } - return behavior === 'allow' ? 'user_temporary' : 'user_reject' - } - case 'rule': - return ruleSourceToOTelSource(reason.rule.source, behavior) - case 'hook': - return 'hook' - case 'mode': - case 'classifier': - case 'subcommandResults': - case 'asyncAgent': - case 'sandboxOverride': - case 'workingDir': - case 'safetyCheck': - case 'other': - return 'config' - default: { - const _exhaustive: never = reason - return 'config' - } - } -} - function getNextImagePasteId(messages: Message[]): number { let maxId = 0 for (const message of messages) { @@ -1234,20 +1154,6 @@ export async function checkPermissionsAndCallTool( ) } - // Emit tool_decision OTel event and code-edit counter if the interactive - // permission path didn't already log it (headless mode bypasses permission - // logging, so we need to emit both the generic event and the code-edit - // counter here) - if ( - permissionDecision.behavior !== 'ask' && - !toolUseContext.toolDecisions?.has(toolUseID) - ) { - decisionReasonToOTelSource( - permissionDecision.decisionReason, - permissionDecision.behavior, - ) - } - // Add message if permission was granted/denied by PermissionRequest hook if ( permissionDecision.decisionReason?.type === 'hook' && @@ -1711,7 +1617,7 @@ export async function checkPermissionsAndCallTool( }) } - // TOOD(hackyon): refactor so we don't have different experiences for MCP tools + // TODO(hackyon): refactor so we don't have different experiences for MCP tools if (!isMcpTool(tool)) { await addToolResult(toolOutput, mappedToolResultBlock) } diff --git a/src/utils/agenticSessionSearch.test.ts b/src/utils/agenticSessionSearch.test.ts index e1002f7f3..ead8c8d40 100644 --- a/src/utils/agenticSessionSearch.test.ts +++ b/src/utils/agenticSessionSearch.test.ts @@ -42,11 +42,14 @@ test.each(['indices', 'diagnostics'] as const)('deep search validates %s at the assert.ok(prompt.includes(text), 'search input must retain ' + text) } if (${JSON.stringify(mode)} === 'indices') { - for (const field of [[], [0], [0, 0.5, null, false, true, '0', '0.0', [], [0], {}, -1, 1], null, 0, '0', {}, undefined]) { + for (const field of [[], [0], [0, 0, 0], [0, 0.5, null, false, true, '0', '0.0', [], [0], {}, -1, 1], null, 0, '0', {}, undefined]) { responseText = JSON.stringify({ relevant_indices: field }) const result = await agenticSessionSearch('private-query', [log]) assert.deepEqual(result, Array.isArray(field) && field.includes(0) ? [log] : []) } + const other = { ...log, firstPrompt: 'second session' } + responseText = '{"relevant_indices":[1,0,1,-1,"0",0]}' + assert.deepEqual(await agenticSessionSearch('private-query', [log, other]), [other, log]) } else { responseText = '{"relevant_indices":[private-transcript]}' await assert.rejects(agenticSessionSearch('private-query', [log]), /^Error: Agentic session search failed$/) diff --git a/src/utils/agenticSessionSearch.ts b/src/utils/agenticSessionSearch.ts index a7b409443..e7b38812f 100644 --- a/src/utils/agenticSessionSearch.ts +++ b/src/utils/agenticSessionSearch.ts @@ -280,9 +280,10 @@ Find the sessions that are most relevant to this query.` ? result.relevant_indices : [] // Map indices back to logs (indices are relative to logsWithTranscripts) - const relevantLogs = relevantIndices + const uniqueIndices = new Set(relevantIndices .filter((index): index is number => typeof index === 'number' && - Number.isInteger(index) && index >= 0 && index < logsWithTranscripts.length) + Number.isInteger(index) && index >= 0 && index < logsWithTranscripts.length)) + const relevantLogs = Array.from(uniqueIndices) .flatMap(index => { const log = logsWithTranscripts[index] return log ? [log] : [] diff --git a/vscode-extension/openclaude-vscode/src/chat/chatProvider.js b/vscode-extension/openclaude-vscode/src/chat/chatProvider.js index b2025b829..9fe5eec01 100644 --- a/vscode-extension/openclaude-vscode/src/chat/chatProvider.js +++ b/vscode-extension/openclaude-vscode/src/chat/chatProvider.js @@ -41,7 +41,8 @@ class ChatController { this._sessionManager = sessionManager; this._process = null; this._startGeneration = 0; - this._resetGeneration = 0; + // Transcript selections and New Chat invalidate pending history loads. + this._selectionGeneration = 0; this._pendingStart = null; this._webviews = new Set(); this._accumulatedText = ''; @@ -169,7 +170,7 @@ class ChatController { } resetForNewSession() { - this._resetGeneration++; + this._selectionGeneration++; this.stopSession(); this._currentSessionId = null; this._messages = []; @@ -542,12 +543,13 @@ class OpenClaudeChatViewProvider { this._chatController.resetForNewSession(); break; case 'resume_session': + this._chatController._selectionGeneration++; this._chatController.stopSession(); webview.postMessage({ type: 'session_cleared' }); { - const generation = this._chatController._resetGeneration; + const generation = this._chatController._selectionGeneration; await this._loadAndDisplaySession(webview, msg.sessionId); - if (generation !== this._chatController._resetGeneration) break; + if (generation !== this._chatController._selectionGeneration) break; await this._chatController.startSession({ sessionId: msg.sessionId }); } break; @@ -592,9 +594,9 @@ class OpenClaudeChatViewProvider { async _loadAndDisplaySession(webview, sessionId) { if (!this._chatController.sessionManager) return; try { - const generation = this._chatController._resetGeneration; + const generation = this._chatController._selectionGeneration; const messages = await this._chatController.sessionManager.loadSession(sessionId); - if (generation !== this._chatController._resetGeneration) return; + if (generation !== this._chatController._selectionGeneration) return; if (messages && messages.length > 0) { this._chatController._messages = messages; webview.postMessage({ type: 'restore_messages', messages }); @@ -656,12 +658,13 @@ class OpenClaudeChatPanelManager { this._chatController.resetForNewSession(); break; case 'resume_session': + this._chatController._selectionGeneration++; this._chatController.stopSession(); webview.postMessage({ type: 'session_cleared' }); { - const generation = this._chatController._resetGeneration; + const generation = this._chatController._selectionGeneration; await this._loadAndDisplaySession(webview, msg.sessionId); - if (generation !== this._chatController._resetGeneration) break; + if (generation !== this._chatController._selectionGeneration) break; await this._chatController.startSession({ sessionId: msg.sessionId }); } break; @@ -706,9 +709,9 @@ class OpenClaudeChatPanelManager { async _loadAndDisplaySession(webview, sessionId) { if (!this._chatController.sessionManager) return; try { - const generation = this._chatController._resetGeneration; + const generation = this._chatController._selectionGeneration; const messages = await this._chatController.sessionManager.loadSession(sessionId); - if (generation !== this._chatController._resetGeneration) return; + if (generation !== this._chatController._selectionGeneration) return; if (messages && messages.length > 0) { this._chatController._messages = messages; webview.postMessage({ type: 'restore_messages', messages }); diff --git a/vscode-extension/openclaude-vscode/src/chat/chatRenderer.js b/vscode-extension/openclaude-vscode/src/chat/chatRenderer.js index b270e1c82..65d671153 100644 --- a/vscode-extension/openclaude-vscode/src/chat/chatRenderer.js +++ b/vscode-extension/openclaude-vscode/src/chat/chatRenderer.js @@ -711,9 +711,6 @@ function renderChatHtml({ nonce, platform }) { html = html.replace(/\\*\\*(.+?)\\*\\*/g, '$1'); html = html.replace(/\\*(.+?)\\*/g, '$1'); - // links - html = html.replace(/\\[([^\\]]+)\\]\\(([^)]+)\\)/g, '$1'); - // unordered lists (simple) html = html.replace(/^[\\-\\*] (.+)$/gm, '
  • $1
    • '); html = html.replace(/((?:
  • .*<\\/li>\\n?)+)/g, '
      $1
    '); @@ -736,6 +733,17 @@ function renderChatHtml({ nonce, platform }) { html = html.replace(/

    (

    )/g, '$1'); html = html.replace(/(<\\/div>)<\\/p>/g, '$1'); + // Render links last so later Markdown substitutions cannot alter attributes. + // Input entities were already escaped by escapeForMd; escape only the + // remaining attribute delimiters (including markup generated above). + html = html.replace(/\\[([^\\]]+)\\]\\(([^)]+)\\)/g, (_, label, destination) => { + const href = destination.trim(); + if (!/^(https?:|mailto:)/i.test(href)) return label; + const attribute = href.replace(//g, '>') + .replace(/"/g, '"').replace(/'/g, '''); + return '' + label + ''; + }); + return html; } diff --git a/vscode-extension/openclaude-vscode/src/chat/chatRenderer.test.js b/vscode-extension/openclaude-vscode/src/chat/chatRenderer.test.js index b5f87204b..faa5ceb40 100644 --- a/vscode-extension/openclaude-vscode/src/chat/chatRenderer.test.js +++ b/vscode-extension/openclaude-vscode/src/chat/chatRenderer.test.js @@ -44,3 +44,46 @@ test('New Chat during streaming enables sending the next prompt', () => { assert.equal(sent.at(-1).text, 'fresh prompt'); assert.equal(getElement('sendBtn').disabled, true); }); + +test('assistant Markdown links allow safe schemes without injecting attributes or markup', () => { + const created = []; + const element = () => { + const el = { + style: {}, value: '', textContent: '', scrollHeight: 20, + classList: { toggle() {} }, addEventListener() {}, appendChild() {}, + }; + created.push(el); + return el; + }; + const elements = new Map(); + let receive; + const script = renderChatHtml({ nonce: 'test', platform: 'linux' }) + .match(/