Skip to content

Commit d7384c4

Browse files
cocosheng-gcocosheng-g
authored
fix(core): centralize path validation to prevent crashes from malformed prompts (google-gemini#27211)
1 parent e440e02 commit d7384c4

13 files changed

Lines changed: 1258 additions & 187 deletions

‎packages/cli/src/acp/acpRpcDispatcher.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ describe('GeminiAgent - RPC Dispatcher', () => {
7171
validatePathAccess: vi.fn().mockReturnValue(null),
7272
getWorkspaceContext: vi.fn().mockReturnValue({
7373
addReadOnlyPath: vi.fn(),
74+
getDirectories: vi.fn().mockReturnValue(['/tmp']),
7475
}),
7576
getPolicyEngine: vi.fn().mockReturnValue({
7677
addRule: vi.fn(),

‎packages/cli/src/acp/acpSession.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ describe('Session', () => {
149149
validatePathAccess: vi.fn().mockReturnValue(null),
150150
getWorkspaceContext: vi.fn().mockReturnValue({
151151
addReadOnlyPath: vi.fn(),
152+
getDirectories: vi.fn().mockReturnValue(['/tmp']),
152153
}),
153154
waitForMcpInit: vi.fn(),
154155
getDisableAlwaysAllow: vi.fn().mockReturnValue(false),

‎packages/cli/src/acp/acpSession.ts‎

Lines changed: 110 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ import {
3737
MessageBusType,
3838
PolicyDecision,
3939
type ToolConfirmationRequest,
40+
resolveAtCommandPath,
41+
type ResolvedAtCommandPath,
4042
} from '@google/gemini-cli-core';
4143
import * as acp from '@agentclientprotocol/sdk';
4244
import type { Part, FunctionCall } from '@google/genai';
@@ -1023,99 +1025,120 @@ export class Session {
10231025
let currentPathSpec = pathName;
10241026
let resolvedSuccessfully = false;
10251027
let readDirectly = false;
1026-
try {
1027-
const absolutePath = path.resolve(
1028+
1029+
const result = await resolveAtCommandPath(
1030+
pathName,
1031+
this.context.config,
1032+
(msg) => this.debug(msg),
1033+
);
1034+
1035+
let validationError: string | null = null;
1036+
let absolutePath: string;
1037+
let resolved: ResolvedAtCommandPath | undefined;
1038+
1039+
if (result.status === 'resolved') {
1040+
resolved = result.resolved;
1041+
absolutePath = resolved.absolutePath;
1042+
} else if (result.status === 'unauthorized') {
1043+
absolutePath = result.absolutePath;
1044+
validationError = result.error;
1045+
} else if (result.status === 'invalid') {
1046+
// Already logged in resolveAtCommandPath
1047+
continue;
1048+
} else {
1049+
// Result is not_found.
1050+
// We still check if it's an unauthorized absolute path that we can ask permission for,
1051+
// specifically for paths that are completely outside the root and not even in any workspace directory.
1052+
// For relative paths not found anywhere, we resolve relative to targetDir for permission check.
1053+
absolutePath = path.resolve(
10281054
this.context.config.getTargetDir(),
10291055
pathName,
10301056
);
1057+
}
10311058

1032-
let validationError = this.context.config.validatePathAccess(
1033-
absolutePath,
1034-
'read',
1035-
);
1036-
1037-
// We ask the user for explicit permission to read them if outside sandboxed workspace boundaries (and not already authorized).
1038-
if (
1039-
validationError &&
1040-
!isWithinRoot(absolutePath, this.context.config.getTargetDir())
1041-
) {
1042-
try {
1043-
const stats = await fs.stat(absolutePath);
1044-
if (stats.isFile()) {
1045-
const syntheticCallId = `resolve-prompt-${pathName}-${randomUUID()}`;
1046-
const params = {
1047-
sessionId: this.id,
1048-
options: [
1049-
{
1050-
optionId: ToolConfirmationOutcome.ProceedOnce,
1051-
name: 'Allow once',
1052-
kind: 'allow_once',
1053-
},
1059+
if (
1060+
!resolved &&
1061+
validationError &&
1062+
!isWithinRoot(absolutePath, this.context.config.getTargetDir())
1063+
) {
1064+
try {
1065+
const stats = await fs.stat(absolutePath);
1066+
if (stats.isFile()) {
1067+
const syntheticCallId = `resolve-prompt-${pathName}-${randomUUID()}`;
1068+
const params = {
1069+
sessionId: this.id,
1070+
options: [
1071+
{
1072+
optionId: ToolConfirmationOutcome.ProceedOnce,
1073+
name: 'Allow once',
1074+
kind: 'allow_once',
1075+
},
1076+
{
1077+
optionId: ToolConfirmationOutcome.Cancel,
1078+
name: 'Deny',
1079+
kind: 'reject_once',
1080+
},
1081+
] as acp.PermissionOption[],
1082+
toolCall: {
1083+
toolCallId: syntheticCallId,
1084+
status: 'pending',
1085+
title: `Allow access to absolute path: ${pathName}`,
1086+
content: [
10541087
{
1055-
optionId: ToolConfirmationOutcome.Cancel,
1056-
name: 'Deny',
1057-
kind: 'reject_once',
1058-
},
1059-
] as acp.PermissionOption[],
1060-
toolCall: {
1061-
toolCallId: syntheticCallId,
1062-
status: 'pending',
1063-
title: `Allow access to absolute path: ${pathName}`,
1064-
content: [
1065-
{
1066-
type: 'content',
1067-
content: {
1068-
type: 'text',
1069-
text: `The Agent needs access to read an attached file outside your workspace: ${pathName}`,
1070-
},
1088+
type: 'content',
1089+
content: {
1090+
type: 'text',
1091+
text: `The Agent needs access to read an attached file outside your workspace: ${pathName}`,
10711092
},
1072-
],
1073-
locations: [],
1074-
kind: 'read',
1075-
},
1076-
};
1093+
},
1094+
],
1095+
locations: [],
1096+
kind: 'read',
1097+
},
1098+
};
10771099

1078-
const output = RequestPermissionResponseSchema.parse(
1079-
await this.connection.requestPermission(params),
1080-
);
1100+
const output = RequestPermissionResponseSchema.parse(
1101+
await this.connection.requestPermission(params),
1102+
);
10811103

1082-
const outcome =
1083-
output.outcome.outcome === 'cancelled'
1084-
? ToolConfirmationOutcome.Cancel
1085-
: z
1086-
.nativeEnum(ToolConfirmationOutcome)
1087-
.parse(output.outcome.optionId);
1088-
1089-
if (outcome === ToolConfirmationOutcome.ProceedOnce) {
1090-
this.context.config
1091-
.getWorkspaceContext()
1092-
.addReadOnlyPath(absolutePath);
1093-
validationError = null;
1094-
} else {
1095-
this.debug(
1096-
`Direct read authorization denied for absolute path ${pathName}`,
1097-
);
1098-
directContents.push({
1099-
spec: pathName,
1100-
content: `[Warning: Access to absolute path \`${pathName}\` denied by user.]`,
1101-
});
1102-
continue;
1103-
}
1104+
const outcome =
1105+
output.outcome.outcome === 'cancelled'
1106+
? ToolConfirmationOutcome.Cancel
1107+
: z
1108+
.nativeEnum(ToolConfirmationOutcome)
1109+
.parse(output.outcome.optionId);
1110+
1111+
if (outcome === ToolConfirmationOutcome.ProceedOnce) {
1112+
this.context.config
1113+
.getWorkspaceContext()
1114+
.addReadOnlyPath(absolutePath);
1115+
validationError = null;
1116+
} else {
1117+
this.debug(
1118+
`Direct read authorization denied for absolute path ${pathName}`,
1119+
);
1120+
directContents.push({
1121+
spec: pathName,
1122+
content: `[Warning: Access to absolute path \`${pathName}\` denied by user.]`,
1123+
});
1124+
continue;
11041125
}
1105-
} catch (error) {
1106-
this.debug(
1107-
`Failed to request permission for absolute attachment ${pathName}: ${getErrorMessage(error)}`,
1108-
);
1109-
await this.sendUpdate({
1110-
sessionUpdate: 'agent_thought_chunk',
1111-
content: {
1112-
type: 'text',
1113-
text: `Warning: Failed to display permission dialog for \`${absolutePath}\`. Error: ${getErrorMessage(error)}`,
1114-
},
1115-
});
11161126
}
1127+
} catch (error) {
1128+
this.debug(
1129+
`Failed to request permission for absolute attachment ${pathName}: ${getErrorMessage(error)}`,
1130+
);
1131+
await this.sendUpdate({
1132+
sessionUpdate: 'agent_thought_chunk',
1133+
content: {
1134+
type: 'text',
1135+
text: `Warning: Failed to display permission dialog for \`${absolutePath}\`. Error: ${getErrorMessage(error)}`,
1136+
},
1137+
});
11171138
}
1139+
}
11181140

1141+
try {
11191142
if (!validationError) {
11201143
// If it's an absolute path that is authorized (e.g. added via readOnlyPaths),
11211144
// read it directly to avoid ReadManyFilesTool absolute path resolution issues.
@@ -1128,7 +1151,9 @@ export class Session {
11281151
!readDirectly
11291152
) {
11301153
try {
1131-
const stats = await fs.stat(absolutePath);
1154+
const stats = resolved
1155+
? resolved.stats
1156+
: await fs.stat(absolutePath);
11321157
if (stats.isFile()) {
11331158
const fileReadResult = await processSingleFileContent(
11341159
absolutePath,
@@ -1187,7 +1212,9 @@ export class Session {
11871212
}
11881213

11891214
if (!readDirectly) {
1190-
const stats = await fs.stat(absolutePath);
1215+
const stats = resolved
1216+
? resolved.stats
1217+
: await fs.stat(absolutePath);
11911218
if (stats.isDirectory()) {
11921219
currentPathSpec = pathName.endsWith('/')
11931220
? `${pathName}**`

‎packages/cli/src/acp/acpSessionManager.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ describe('AcpSessionManager', () => {
7979
validatePathAccess: vi.fn().mockReturnValue(null),
8080
getWorkspaceContext: vi.fn().mockReturnValue({
8181
addReadOnlyPath: vi.fn(),
82+
getDirectories: vi.fn().mockReturnValue(['/tmp']),
8283
}),
8384
getPolicyEngine: vi.fn().mockReturnValue({
8485
addRule: vi.fn(),

0 commit comments

Comments
 (0)