Skip to content
Merged
118 changes: 113 additions & 5 deletions packages/cli/src/ui/hooks/atCommandProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import * as fs from 'node:fs/promises';
import * as path from 'node:path';
import type { Part, PartListUnion } from '@google/genai';
import type { Config } from '@qwen-code/qwen-code-core';
import type { Config, Extension } from '@qwen-code/qwen-code-core';
import {
getErrorMessage,
isNodeError,
Expand All @@ -27,6 +27,11 @@ import type {
} from '../types.js';
import { ToolCallStatus } from '../types.js';
import { matchMcpServerPrefix } from './mcpResourceRef.js';
import {
parseExtensionRef,
matchExtensionByRef,
buildExtensionContextText,
} from './extension-mention-ref.js';

export interface ResolveAtCommandParams {
query: string;
Expand Down Expand Up @@ -205,6 +210,13 @@ export async function resolveAtCommandQuery({
uri: string;
}> = [];

// Extension references (`@ext:<name>`) collected during the loop.
const activeExtensions = config.getActiveExtensions?.() ?? [];

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — the autocomplete side has the guard, but the resolution path doesn't. However, adding it to the resolution path would mean @ext:name silently disappears in untrusted folders after autocomplete showed it. The current behavior is consistent: if autocomplete doesn't show extensions in untrusted folders, the user can't construct the reference. If they manually type it, the extension context is still from the user's own installed extensions, not the workspace.

const extensionMentions: Array<{
originalAtPath: string;
extension: Extension;
}> = [];

for (const atPathPart of atPathCommandParts) {
const originalAtPath = atPathPart.content; // e.g., "@file.txt" or "@"

Expand All @@ -217,6 +229,23 @@ export async function resolveAtCommandQuery({

const pathName = originalAtPath.substring(1);

// Extension reference (`@ext:<name>`): detected BEFORE MCP/filesystem
// resolution. Only matches when the path starts with `ext:` and the name
// corresponds to an active extension.
const extRef = parseExtensionRef(pathName);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By design — ext: is an unambiguous prefix. If a user types @ext:foo and foo isn't an extension, it's a typo, not an MCP server reference. MCP servers don't use the ext: prefix.

if (extRef) {
const extension = matchExtensionByRef(extRef.name, activeExtensions);
if (extension) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No deduplication of extension mentions.

If the user types @ext:foo @ext:foo, the same extension is matched twice, its context files are read twice, and the context text is injected twice into the prompt — wasting both I/O and context window budget.

Suggested change
if (extension) {
const extension = matchExtensionByRef(extRef.name, activeExtensions);
if (extension) {
if (!extensionMentions.some((m) => m.extension.name === extension.name)) {
extensionMentions.push({ originalAtPath, extension });
}
atPathToResolvedSpecMap.set(originalAtPath, pathName);
continue;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added dedup check: extensionMentions.some(m => m.extension.name === extension.name) before pushing.

if (
!extensionMentions.some((m) => m.extension.name === extension.name)
) {
extensionMentions.push({ originalAtPath, extension });
}
atPathToResolvedSpecMap.set(originalAtPath, pathName);
continue;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — unmatched ext: refs now continue with a debug message listing available extensions, instead of falling through to filesystem resolution.

}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed — unmatched @ext: refs now continue with a debug message listing available extensions (commit b45f061).


// MCP resource reference (`@server:uri`): detected BEFORE filesystem
// resolution so a resource URI containing ':' / '//' isn't mistaken for
// a path. Only matches when `server` is a configured MCP server; all
Expand Down Expand Up @@ -444,8 +473,12 @@ export async function resolveAtCommandQuery({

// Fallback for lone "@" or completely invalid @-commands resulting in empty
// initialQueryText — only when there is nothing to read at all (no valid
// file paths AND no resource references).
if (pathSpecsToRead.length === 0 && mcpResourceRefs.length === 0) {
// file paths, resource references, or extension mentions).
if (
pathSpecsToRead.length === 0 &&
mcpResourceRefs.length === 0 &&
extensionMentions.length === 0
) {
onDebugMessage('No valid file paths found in @ commands to read.');
if (initialQueryText === '@' && query.trim() === '@') {
// If the only thing was a lone @, pass original query (which might have spaces)
Expand Down Expand Up @@ -530,22 +563,97 @@ export async function resolveAtCommandQuery({
}
}

// Build extension context parts and display cards for @-mentioned extensions.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] No aggregate cap on extension context injection.

Each context file has a 50KB per-file cap (line 583), but the loop iterates all context files without a cumulative limit. An extension shipping many context files — or a user @-mentioning several extensions — can inject unbounded content into the prompt, risking context_length_exceeded errors or inflated API costs.

Consider adding a cumulative budget (e.g., 200KB total across all extension context files) and logging when the cap is hit.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added a 200KB aggregate budget (EXTENSION_CONTEXT_BUDGET) across all extension context files, with a 50KB per-file cap. Logs when the budget is exhausted.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing integration tests for security boundary code

113 lines of new integration logic — path traversal guard (isSubpath), 200KB aggregate budget, per-file cap, dedup, extension resolution priority — have zero test coverage. The R1 review requested tests; 20 unit tests were added for extension-mention-ref.ts (pure helpers), but the security-critical code here remains untested.

The existing atCommandProcessor.test.ts has 44 tests with established mocking patterns for Config, fs.readFile, etc. At minimum, add tests for:

  1. @ext:name resolution produces extension display + context part
  2. Path traversal rejection for context files outside extension.path
  3. Budget truncation when a context file exceeds 50KB
  4. Dedup when the same extension is mentioned twice (@ext:browser @ext:browser)
  5. @ext:nonexistent falls through to file/MCP resolution

— qwen3.7-max via Qwen Code /review

// Aggregate cap across all extensions to prevent unbounded context injection.
const EXTENSION_CONTEXT_BUDGET = 200_000; // 200KB total

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The budget is an approximate guard against context blowup, not a precise byte counter. String.length is consistent with how the rest of the codebase handles text size limits.

const PER_FILE_CAP = 50_000; // 50KB per context file
let extensionContextBudgetRemaining = EXTENSION_CONTEXT_BUDGET;

const extensionParts: Part[] = [];
const extensionDisplays: IndividualToolCallDisplay[] = [];
const extensionLabels: string[] = [];
for (let i = 0; i < extensionMentions.length; i++) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer loop needs to be sequential because the aggregate budget (extensionContextBudgetRemaining) is shared across extensions. Parallelizing would require pre-allocating budget per extension or a post-hoc trim, adding complexity for a rare edge case (multiple @-mentioned extensions).

const { extension } = extensionMentions[i];
const displayName = extension.displayName || extension.name;
const callId = `client-extension-${userMessageTimestamp}-${i}`;

let contextText = buildExtensionContextText(extension);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, but the metadata block is ~200-500 bytes which is negligible against the 200KB budget. Not worth the complexity of deducting it.


// Read extension context files in parallel, with path traversal and
// budget checks.
if (extension.contextFiles && extension.contextFiles.length > 0) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metadata block (buildExtensionContextText) is always emitted regardless of budget — it's the context files that are budget-gated. The metadata (~200-500 bytes) is negligible. Adding an outer check would skip the metadata block too, which would be wrong.

const fileReads = await Promise.allSettled(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Promise.allSettled reads all context files before budget check

All context files are read into memory simultaneously via Promise.allSettled. When the 200KB budget is already exhausted by a prior extension, subsequent extensions' files are still read and discarded — wasted IO and memory.

Add an early budget guard before the parallel read phase:

Suggested change
const fileReads = await Promise.allSettled(
if (extension.contextFiles && extension.contextFiles.length > 0
&& extensionContextBudgetRemaining > 0) {

— qwen3.7-max via Qwen Code /review

extension.contextFiles.map(async (contextFilePath) => {
const resolved = path.resolve(contextFilePath);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Symlink-based path traversal bypasses isSubpath guard

path.resolve() normalizes ./.. but does NOT resolve symlinks. A malicious extension can place a symlink inside its directory (e.g., context.md -> ~/.ssh/id_rsa) and declare it as a context file. The check isSubpath(extension.path, resolved) passes because the string path stays within the extension dir, but fs.readFile follows the symlink and reads arbitrary files.

The codebase already has realPathWithin() in packages/core/src/extension/gemini-converter.ts:134 which uses fs.realpathSync — the same function used in 5 other extension path confinement checks (gemini-converter.ts, claude-converter.ts). This PR should follow the established pattern.

Suggested change
const resolved = path.resolve(contextFilePath);
let resolved: string;
try {
resolved = fs.realpathSync(contextFilePath);
} catch {
onDebugMessage(
`Skipping unreadable context file: ${contextFilePath}`,
);
return null;
}
const realExtPath = fs.realpathSync(extension.path);
if (!isSubpath(realExtPath, resolved)) {

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — now using fs.realpath() to resolve symlinks before the isSubpath check, matching the pattern in gemini-converter.ts and claude-converter.ts.

if (!isSubpath(extension.path, resolved)) {
onDebugMessage(
`Skipping context file outside extension directory: ${contextFilePath}`,
);
return null;
}
return fs.readFile(resolved, 'utf-8');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing AbortSignal in extension context file reads

signal (AbortSignal) is destructured from params (line 179) and is in scope. Both the MCP resource path (readMcpResource(ref.serverName, ref.uri, { signal }) at line 424) and the file read path (readManyFiles(config, { ... signal }) at line 586) propagate the abort signal for cancellation. But this fs.readFile call does not pass it.

When the user cancels a pending query (e.g., edits the input while extension context files are being read), the in-flight fs.readFile calls continue to completion — wasting disk I/O and memory for content that will be discarded.

Suggested change
return fs.readFile(resolved, 'utf-8');
return fs.readFile(resolved, { encoding: 'utf-8', signal });

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — now passing { encoding: 'utf-8', signal } to fs.readFile for extension context files, matching the MCP/file read paths.

}),
);

for (let j = 0; j < fileReads.length; j++) {
const outcome = fileReads[j];
if (outcome.status === 'rejected') {
onDebugMessage(
`Failed to read extension context file ${extension.contextFiles[j]}: ${getErrorMessage(outcome.reason)}`,
);
continue;
}
const content = outcome.value;
if (!content || !content.trim()) continue;
if (extensionContextBudgetRemaining <= 0) {
onDebugMessage(
`Extension context budget exhausted, skipping remaining files.`,
);
break;
}
const cap = Math.min(PER_FILE_CAP, extensionContextBudgetRemaining);
const cappedContent =
content.length > cap
? content.slice(0, cap) + '\n... (truncated)'
: content;
contextText += `\n\n${cappedContent}`;
extensionContextBudgetRemaining -= cappedContent.length;
}
}

extensionParts.push({ text: contextText });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Hard-coded ext: prefix instead of using imported constant

This file already imports parseExtensionRef, matchExtensionByRef, and buildExtensionContextText from ./extension-mention-ref.js, but buildExtensionRef (which does exactly ${EXTENSION_REF_PREFIX}${extensionName}) is not imported. If EXTENSION_REF_PREFIX ever changes, this line silently diverges.

Suggested change
extensionParts.push({ text: contextText });
extensionLabels.push(buildExtensionRef(extension.name));

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — now using imported buildExtensionRef(extension.name) instead of hard-coded template literal.

extensionLabels.push(`ext:${extension.name}`);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extension.name is the machine-readable slug (alphanumeric + hyphens), not user-facing metadata. It doesn't go through terminal rendering. Sanitizing it would be inconsistent — we don't sanitize ext.name anywhere else in the codebase.

extensionDisplays.push({
callId,
name: 'Activate Extension',
description: `Activated extension ${displayName}`,
status: ToolCallStatus.Success,
resultDisplay: undefined,
confirmationDetails: undefined,
});
}

// File and resource content are grouped by type, NOT interleaved by their
// position in the user's query. The model correlates each @-reference with
// its content block via the "--- Content from ... ---" delimiter labels (and
// the verbatim `@server:uri` / `@path` left in the prompt text), not by
// positional alignment, so grouping is safe.
const processedQueryParts: PartListUnion = [
{ text: initialQueryText },
...extensionParts,
...fileParts,
...resourceParts,
];
const allLabels = [...contentLabelsForDisplay, ...resourceLabels];
const allLabels = [
...extensionLabels,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Session resume mislabels extension activations as "Read File"

allLabels includes extensionLabels (e.g., ext:browser) and is assigned to recording.filesRead. On session resume, resumeHistoryUtils maps every filesRead entry to "Read File" — producing "Read file ext:browser" instead of "Activated extension Browser."

Consider excluding extensionLabels from recording.filesRead since extensionDisplays are already persisted via toolDisplays and provide the correct labeling. Or tag entries with a type discriminator ({ kind: 'extension' | 'file' | 'mcp', label }).

— qwen3.7-max via Qwen Code /review

...contentLabelsForDisplay,
...resourceLabels,
];

return {
processedQuery: processedQueryParts,
shouldProceed: true,
toolDisplays: [...fileDisplays, ...resourceDisplays],
toolDisplays: [...extensionDisplays, ...fileDisplays, ...resourceDisplays],
filesRead: allLabels,
recording: {
filesRead: allLabels,
Expand Down
Loading
Loading