fix: resolve eight reported AI quality findings (#2463)

* fix: resolve eight reported AI quality findings

* test: keep invalid port fixtures outside both random ranges

* test: verify selected-view history and document OAuth fallback

* fix(mcp): probe the OAuth callback loopback address

* fix(mcp): preserve wildcard checks for automatic callback ports
This commit is contained in:
JATMN
2026-10-28 15:10:11 -07:00
committed by GitHub
parent 70fdbaf36e
commit 977cb53282
9 changed files with 296 additions and 132 deletions
+154 -3
View File
@@ -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<void>((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<void>((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<void>(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<void>(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<void>((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<void>((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<void>(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<void>((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<void>((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<void>(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')
})
+27 -18
View File
@@ -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<void> {
return new Promise<void>((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<number> {
// 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<number> {
const port = randomInt(min, max + 1)
try {
await new Promise<void>((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<number> {
// If random selection failed, try the fallback port
try {
await new Promise<void>((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`)
+1 -95
View File
@@ -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)
}
+4 -1
View File
@@ -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$/)
+3 -2
View File
@@ -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] : []
@@ -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 });
@@ -711,9 +711,6 @@ function renderChatHtml({ nonce, platform }) {
html = html.replace(/\\*\\*(.+?)\\*\\*/g, '<strong>$1</strong>');
html = html.replace(/\\*(.+?)\\*/g, '<em>$1</em>');
// links
html = html.replace(/\\[([^\\]]+)\\]\\(([^)]+)\\)/g, '<a href="$2" title="$2">$1</a>');
// unordered lists (simple)
html = html.replace(/^[\\-\\*] (.+)$/gm, '<li>$1</li>');
html = html.replace(/((?:<li>.*<\\/li>\\n?)+)/g, '<ul>$1</ul>');
@@ -736,6 +733,17 @@ function renderChatHtml({ nonce, platform }) {
html = html.replace(/<p>(<div class="code-wrapper">)/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, '&lt;').replace(/>/g, '&gt;')
.replace(/"/g, '&quot;').replace(/'/g, '&#39;');
return '<a href="' + attribute + '" title="' + attribute + '">' + label + '</a>';
});
return html;
}
@@ -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(/<script nonce="test">([\s\S]*?)<\/script>/)[1];
vm.runInNewContext(script, {
document: { getElementById(id) {
if (!elements.has(id)) elements.set(id, element());
return elements.get(id);
}, createElement: element, addEventListener() {} },
window: { addEventListener(_name, listener) { receive = listener; } },
acquireVsCodeApi: () => ({ postMessage() {}, getState: () => null }),
requestAnimationFrame: callback => callback(),
});
const render = text => {
receive({ data: { type: 'stream_delta', text } });
return created.find(el => el.className === 'md-content').innerHTML;
};
for (const href of ['https://example.com/?a=1&b=2', 'http://example.com', 'mailto:test@example.com', 'HTTPS://example.com']) {
assert.match(render('[safe](' + href + ')'), /<a href="[^\"]+" title="[^\"]+">safe<\/a>/);
}
for (const href of ['javascript:alert', 'JaVaScRiPt:alert', 'data:text/html,test', 'command:workbench.action', '//example.com', 'file:///tmp/test', 'java\nscript:alert', 'javascript&#58;alert']) {
const html = render('[unsafe](' + href + ')');
assert.ok(!html.includes('<a '), href);
assert.ok(html.includes('unsafe'), href);
}
const injected = render('[safe](https://example.com/" onmouseover="alert)');
assert.match(injected, /href="https:\/\/example.com\/&quot; onmouseover=&quot;alert"/);
assert.ok(!injected.includes('" onmouseover="'));
const formatted = render('[**safe**](https://example.com/?a=1&b=2)');
assert.ok(formatted.includes('<strong>safe</strong>'));
assert.ok(formatted.includes('href="https://example.com/?a=1&amp;b=2"'));
});
@@ -191,6 +191,46 @@ test('resume hands the selected transcript to the CLI, including reconnect and W
assert.deepEqual(fresh.getMessages(), []);
assert.ok(!updates.some(message => message.type === 'restore_messages'));
// Newer selections win in both views, even across a shared controller.
for (const OtherProvider of [OpenClaudeChatViewProvider, OpenClaudeChatPanelManager]) {
for (const olderFinishesFirst of [true, false]) {
let receiveOther;
const newerUpdates = [];
new OtherProvider(fresh)._attachMessageHandler({
onDidReceiveMessage: handler => { receiveOther = handler; },
postMessage: message => newerUpdates.push(message),
});
updates.length = 0;
const older = receive({ type: 'resume_session', sessionId: id });
const finishOlder = finishLoad;
const newerId = 'newer-session';
const newer = receiveOther({ type: 'resume_session', sessionId: newerId });
const finishNewer = finishLoad;
const before = calls.length;
if (olderFinishesFirst) {
finishOlder([{ role: 'user', text: 'stale history' }]);
await older;
assert.equal(calls.length, before, 'stale selection cannot launch while newer load is pending');
}
finishNewer([{ role: 'user', text: 'newer history' }]);
await newer;
if (!olderFinishesFirst) {
finishOlder([{ role: 'user', text: 'stale history' }]);
await older;
}
assert.equal(calls.length, before + 1, 'only the newest selection launches');
assert.equal(resumeValue(), newerId);
assert.equal(fresh.sessionId, newerId);
assert.deepEqual(fresh.getMessages(), [{ role: 'user', text: 'newer history' }]);
assert.ok(newerUpdates.some(message => message.type === 'restore_messages' &&
message.messages.some(item => item.text === 'newer history')));
assert.ok(![...updates, ...newerUpdates].some(message => message.type === 'restore_messages' &&
message.messages.some(item => item.text === 'stale history')));
fresh.stopSession();
}
}
fresh.resetForNewSession();
// An ordinary send must not cancel the selected history load.
const ordinaryResume = receive({ type: 'resume_session', sessionId: id });
const ordinarySend = fresh.sendMessage('sent while loading');