Allow @-includes outside of workspaces (with permission) (#18470)

This commit is contained in:
Tommaso Sciortino
2026-02-09 12:24:28 -08:00
committed by GitHub
parent e73288f25f
commit 262e8384d4
17 changed files with 250 additions and 64 deletions
@@ -152,6 +152,7 @@ export const createMockConfig = (overrides: Partial<Config> = {}): Config =>
getBlockedMcpServers: vi.fn().mockReturnValue([]),
getExperiments: vi.fn().mockReturnValue(undefined),
getHasAccessToPreviewModel: vi.fn().mockReturnValue(false),
validatePathAccess: vi.fn().mockReturnValue(null),
...overrides,
}) as unknown as Config;
+64
View File
@@ -145,6 +145,7 @@ vi.mock('./contexts/SessionContext.js');
vi.mock('./components/shared/text-buffer.js');
vi.mock('./hooks/useLogger.js');
vi.mock('./hooks/useInputHistoryStore.js');
vi.mock('./hooks/atCommandProcessor.js');
vi.mock('./hooks/useHookDisplayState.js');
vi.mock('./hooks/useTerminalTheme.js', () => ({
useTerminalTheme: vi.fn(),
@@ -2734,4 +2735,67 @@ describe('AppContainer State Management', () => {
compUnmount();
});
});
describe('Permission Handling', () => {
it('shows permission dialog when checkPermissions returns paths', async () => {
const { checkPermissions } = await import(
'./hooks/atCommandProcessor.js'
);
vi.mocked(checkPermissions).mockResolvedValue(['/test/file.txt']);
let unmount: () => void;
await act(async () => (unmount = renderAppContainer().unmount));
await waitFor(() => expect(capturedUIActions).toBeTruthy());
await act(async () =>
capturedUIActions.handleFinalSubmit('read @file.txt'),
);
expect(capturedUIState.permissionConfirmationRequest).not.toBeNull();
expect(capturedUIState.permissionConfirmationRequest?.files).toEqual([
'/test/file.txt',
]);
await act(async () => unmount!());
});
it.each([true, false])(
'handles permissions when allowed is %s',
async (allowed) => {
const { checkPermissions } = await import(
'./hooks/atCommandProcessor.js'
);
vi.mocked(checkPermissions).mockResolvedValue(['/test/file.txt']);
const addReadOnlyPathSpy = vi.spyOn(
mockConfig.getWorkspaceContext(),
'addReadOnlyPath',
);
const { submitQuery } = mockedUseGeminiStream();
let unmount: () => void;
await act(async () => (unmount = renderAppContainer().unmount));
await waitFor(() => expect(capturedUIActions).toBeTruthy());
await act(async () =>
capturedUIActions.handleFinalSubmit('read @file.txt'),
);
await act(async () =>
capturedUIState.permissionConfirmationRequest?.onComplete({
allowed,
}),
);
if (allowed) {
expect(addReadOnlyPathSpy).toHaveBeenCalledWith('/test/file.txt');
} else {
expect(addReadOnlyPathSpy).not.toHaveBeenCalled();
}
expect(submitQuery).toHaveBeenCalledWith('read @file.txt');
expect(capturedUIState.permissionConfirmationRequest).toBeNull();
await act(async () => unmount!());
},
);
});
});
+29 -2
View File
@@ -28,7 +28,9 @@ import {
type HistoryItemToolGroup,
AuthState,
type ConfirmationRequest,
type PermissionConfirmationRequest,
} from './types.js';
import { checkPermissions } from './hooks/atCommandProcessor.js';
import { MessageType, StreamingState } from './types.js';
import { ToolActionsProvider } from './contexts/ToolActionsContext.js';
import {
@@ -844,6 +846,8 @@ Logging in with Google... Restarting Gemini CLI to continue.
const [authConsentRequest, setAuthConsentRequest] =
useState<ConfirmationRequest | null>(null);
const [permissionConfirmationRequest, setPermissionConfirmationRequest] =
useState<PermissionConfirmationRequest | null>(null);
useEffect(() => {
const handleConsentRequest = (payload: ConsentRequestPayload) => {
@@ -1078,11 +1082,30 @@ Logging in with Google... Restarting Gemini CLI to continue.
);
const handleFinalSubmit = useCallback(
(submittedValue: string) => {
async (submittedValue: string) => {
const isSlash = isSlashCommand(submittedValue.trim());
const isIdle = streamingState === StreamingState.Idle;
if (isSlash || (isIdle && isMcpReady)) {
if (!isSlash) {
const permissions = await checkPermissions(submittedValue, config);
if (permissions.length > 0) {
setPermissionConfirmationRequest({
files: permissions,
onComplete: (result) => {
setPermissionConfirmationRequest(null);
if (result.allowed) {
permissions.forEach((p) =>
config.getWorkspaceContext().addReadOnlyPath(p),
);
}
void submitQuery(submittedValue);
},
});
addInput(submittedValue);
return;
}
}
void submitQuery(submittedValue);
} else {
// Check messageQueue.length === 0 to only notify on the first queued item
@@ -1103,6 +1126,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
isMcpReady,
streamingState,
messageQueue.length,
config,
],
);
@@ -1221,7 +1245,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
!showPrivacyNotice &&
geminiClient?.isInitialized?.()
) {
handleFinalSubmit(initialPrompt);
void handleFinalSubmit(initialPrompt);
initialPromptSubmitted.current = true;
}
}, [
@@ -1714,6 +1738,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
adminSettingsChanged ||
!!commandConfirmationRequest ||
!!authConsentRequest ||
!!permissionConfirmationRequest ||
!!customDialog ||
confirmUpdateExtensionRequests.length > 0 ||
!!loopDetectionConfirmationRequest ||
@@ -1819,6 +1844,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
authConsentRequest,
confirmUpdateExtensionRequests,
loopDetectionConfirmationRequest,
permissionConfirmationRequest,
geminiMdFileCount,
streamingState,
initError,
@@ -1925,6 +1951,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
authConsentRequest,
confirmUpdateExtensionRequests,
loopDetectionConfirmationRequest,
permissionConfirmationRequest,
geminiMdFileCount,
streamingState,
initError,
@@ -117,6 +117,20 @@ export const DialogManager = ({
);
}
if (uiState.permissionConfirmationRequest) {
const files = uiState.permissionConfirmationRequest.files;
const filesList = files.map((f) => `- ${f}`).join('\n');
return (
<ConsentPrompt
prompt={`The following files are outside your workspace:\n\n${filesList}\n\nDo you want to allow this read?`}
onConfirm={(allowed) => {
uiState.permissionConfirmationRequest?.onComplete({ allowed });
}}
terminalWidth={terminalWidth}
/>
);
}
// commandConfirmationRequest and authConsentRequest are kept separate
// to avoid focus deadlocks and state race conditions between the
// synchronous command loop and the asynchronous auth flow.
@@ -52,7 +52,7 @@ export interface UIActions {
setConstrainHeight: (value: boolean) => void;
onEscapePromptChange: (show: boolean) => void;
refreshStatic: () => void;
handleFinalSubmit: (value: string) => void;
handleFinalSubmit: (value: string) => Promise<void>;
handleClearScreen: () => void;
handleProQuotaChoice: (
choice: 'retry_later' | 'retry_once' | 'retry_always' | 'upgrade',
@@ -14,6 +14,7 @@ import type {
HistoryItemWithoutId,
StreamingState,
ActiveHook,
PermissionConfirmationRequest,
} from '../types.js';
import type { CommandContext, SlashCommand } from '../commands/types.js';
import type { TextBuffer } from '../components/shared/text-buffer.js';
@@ -85,6 +86,7 @@ export interface UIState {
authConsentRequest: ConfirmationRequest | null;
confirmUpdateExtensionRequests: ConfirmationRequest[];
loopDetectionConfirmationRequest: LoopDetectionConfirmationRequest | null;
permissionConfirmationRequest: PermissionConfirmationRequest | null;
geminiMdFileCount: number;
streamingState: StreamingState;
initError: string | null;
@@ -1188,40 +1188,6 @@ describe('handleAtCommand', () => {
expect.stringContaining(`using glob: ${path.join(subDirPath, '**')}`),
);
});
it('should skip absolute paths outside workspace', async () => {
const outsidePath = '/tmp/outside-workspace.txt';
const query = `Check @${outsidePath} please.`;
const mockWorkspaceContext = {
isPathWithinWorkspace: vi.fn((path: string) =>
path.startsWith(testRootDir),
),
getDirectories: () => [testRootDir],
addDirectory: vi.fn(),
getInitialDirectories: () => [testRootDir],
setDirectories: vi.fn(),
onDirectoriesChanged: vi.fn(() => () => {}),
} as unknown as ReturnType<typeof mockConfig.getWorkspaceContext>;
mockConfig.getWorkspaceContext = () => mockWorkspaceContext;
const result = await handleAtCommand({
query,
config: mockConfig,
addItem: mockAddItem,
onDebugMessage: mockOnDebugMessage,
messageId: 502,
signal: abortController.signal,
});
expect(result).toEqual({
processedQuery: [{ text: `Check @${outsidePath} please.` }],
});
expect(mockOnDebugMessage).toHaveBeenCalledWith(
`Path ${outsidePath} is not in the workspace and will be skipped.`,
);
});
});
it("should not add the user's turn to history, as that is the caller's responsibility", async () => {
+32 -14
View File
@@ -13,6 +13,8 @@ import {
getErrorMessage,
isNodeError,
unescapePath,
resolveToRealPath,
fileExists,
ReadManyFilesTool,
REFERENCE_CONTENT_START,
REFERENCE_CONTENT_END,
@@ -152,6 +154,35 @@ function categorizeAtCommands(
return { agentParts, resourceParts, fileParts };
}
/**
* Checks if the query contains any file paths that require read permission.
* Returns an array of such paths.
*/
export async function checkPermissions(
query: string,
config: Config,
): Promise<string[]> {
const commandParts = parseAllAtCommands(query);
const { fileParts } = categorizeAtCommands(commandParts, config);
const permissionsRequired: string[] = [];
for (const part of fileParts) {
const pathName = part.content.substring(1);
if (!pathName) continue;
const resolvedPathName = resolveToRealPath(
path.resolve(config.getTargetDir(), pathName),
);
if (config.validatePathAccess(resolvedPathName, 'read')) {
if (await fileExists(resolvedPathName)) {
permissionsRequired.push(resolvedPathName);
}
}
}
return permissionsRequired;
}
interface ResolvedFile {
part: AtCommandPart;
pathSpec: string;
@@ -189,17 +220,6 @@ async function resolveFilePaths(
continue;
}
const resolvedPathName = path.isAbsolute(pathName)
? pathName
: path.resolve(config.getTargetDir(), pathName);
if (!config.isPathAllowed(resolvedPathName)) {
onDebugMessage(
`Path ${pathName} is not in the workspace and will be skipped.`,
);
continue;
}
const gitIgnored =
respectFileIgnore.respectGitIgnore &&
fileDiscovery.shouldIgnoreFile(pathName, {
@@ -229,9 +249,7 @@ async function resolveFilePaths(
for (const dir of config.getWorkspaceContext().getDirectories()) {
try {
const absolutePath = path.isAbsolute(pathName)
? pathName
: path.resolve(dir, pathName);
const absolutePath = path.resolve(dir, pathName);
const stats = await fs.stat(absolutePath);
const relativePath = path.isAbsolute(pathName)
+5
View File
@@ -451,6 +451,11 @@ export interface LoopDetectionConfirmationRequest {
onComplete: (result: { userSelection: 'disable' | 'keep' }) => void;
}
export interface PermissionConfirmationRequest {
files: string[];
onComplete: (result: { allowed: boolean }) => void;
}
export interface ActiveHook {
name: string;
eventName: string;
+14 -1
View File
@@ -1880,9 +1880,22 @@ export class Config {
* Validates if a path is allowed and returns a detailed error message if not.
*
* @param absolutePath The absolute path to validate.
* @param checkType The type of access to check ('read' or 'write'). Defaults to 'write' for safety.
* @returns An error message string if the path is disallowed, null otherwise.
*/
validatePathAccess(absolutePath: string): string | null {
validatePathAccess(
absolutePath: string,
checkType: 'read' | 'write' = 'write',
): string | null {
// For read operations, check read-only paths first
if (checkType === 'read') {
if (this.getWorkspaceContext().isPathReadable(absolutePath)) {
return null;
}
}
// Then check standard allowed paths (Workspace + Temp)
// This covers 'write' checks and acts as a fallback/temp-dir check for 'read'
if (this.isPathAllowed(absolutePath)) {
return null;
}
+8 -3
View File
@@ -123,8 +123,10 @@ class GlobToolInvocation extends BaseToolInvocation<
this.config.getTargetDir(),
this.params.dir_path,
);
const validationError =
this.config.validatePathAccess(searchDirAbsolute);
const validationError = this.config.validatePathAccess(
searchDirAbsolute,
'read',
);
if (validationError) {
return {
llmContent: validationError,
@@ -318,7 +320,10 @@ export class GlobTool extends BaseDeclarativeTool<GlobToolParams, ToolResult> {
params.dir_path || '.',
);
const validationError = this.config.validatePathAccess(searchDirAbsolute);
const validationError = this.config.validatePathAccess(
searchDirAbsolute,
'read',
);
if (validationError) {
return validationError;
}
+8 -2
View File
@@ -123,7 +123,10 @@ class GrepToolInvocation extends BaseToolInvocation<
let searchDirAbs: string | null = null;
if (pathParam) {
searchDirAbs = path.resolve(this.config.getTargetDir(), pathParam);
const validationError = this.config.validatePathAccess(searchDirAbs);
const validationError = this.config.validatePathAccess(
searchDirAbs,
'read',
);
if (validationError) {
return {
llmContent: validationError,
@@ -623,7 +626,10 @@ export class GrepTool extends BaseDeclarativeTool<GrepToolParams, ToolResult> {
this.config.getTargetDir(),
params.dir_path,
);
const validationError = this.config.validatePathAccess(resolvedPath);
const validationError = this.config.validatePathAccess(
resolvedPath,
'read',
);
if (validationError) {
return validationError;
}
+5 -2
View File
@@ -143,7 +143,10 @@ class LSToolInvocation extends BaseToolInvocation<LSToolParams, ToolResult> {
this.params.dir_path,
);
const validationError = this.config.validatePathAccess(resolvedDirPath);
const validationError = this.config.validatePathAccess(
resolvedDirPath,
'read',
);
if (validationError) {
return {
llmContent: validationError,
@@ -331,7 +334,7 @@ export class LSTool extends BaseDeclarativeTool<LSToolParams, ToolResult> {
this.config.getTargetDir(),
params.dir_path,
);
return this.config.validatePathAccess(resolvedPath);
return this.config.validatePathAccess(resolvedPath, 'read');
}
protected createInvocation(
+8 -2
View File
@@ -76,7 +76,10 @@ class ReadFileToolInvocation extends BaseToolInvocation<
}
async execute(): Promise<ToolResult> {
const validationError = this.config.validatePathAccess(this.resolvedPath);
const validationError = this.config.validatePathAccess(
this.resolvedPath,
'read',
);
if (validationError) {
return {
llmContent: validationError,
@@ -213,7 +216,10 @@ export class ReadFileTool extends BaseDeclarativeTool<
params.file_path,
);
const validationError = this.config.validatePathAccess(resolvedPath);
const validationError = this.config.validatePathAccess(
resolvedPath,
'read',
);
if (validationError) {
return validationError;
}
+4 -1
View File
@@ -221,7 +221,10 @@ ${finalExclusionPatternsForDescription
const fullPath = path.resolve(this.config.getTargetDir(), relativePath);
const validationError = this.config.validatePathAccess(fullPath);
const validationError = this.config.validatePathAccess(
fullPath,
'read',
);
if (validationError) {
skippedFiles.push({
path: fullPath,
+8 -2
View File
@@ -164,7 +164,10 @@ class GrepToolInvocation extends BaseToolInvocation<
const pathParam = this.params.dir_path || '.';
const searchDirAbs = path.resolve(this.config.getTargetDir(), pathParam);
const validationError = this.config.validatePathAccess(searchDirAbs);
const validationError = this.config.validatePathAccess(
searchDirAbs,
'read',
);
if (validationError) {
return {
llmContent: validationError,
@@ -582,7 +585,10 @@ export class RipGrepTool extends BaseDeclarativeTool<
this.config.getTargetDir(),
params.dir_path,
);
const validationError = this.config.validatePathAccess(resolvedPath);
const validationError = this.config.validatePathAccess(
resolvedPath,
'read',
);
if (validationError) {
return validationError;
}
@@ -24,6 +24,7 @@ export interface AddDirectoriesResult {
export class WorkspaceContext {
private directories = new Set<string>();
private initialDirectories: Set<string>;
private readOnlyPaths = new Set<string>();
private onDirectoriesChangedListeners = new Set<() => void>();
/**
@@ -113,6 +114,24 @@ export class WorkspaceContext {
return result;
}
/**
* Adds a path to the read-only list.
* These paths are allowed for reading but not for writing (unless they are also in the workspace).
*/
addReadOnlyPath(pathToAdd: string): void {
try {
// Check if it exists
if (!fs.existsSync(pathToAdd)) {
return;
}
// Resolve symlinks
const resolved = fs.realpathSync(path.resolve(this.targetDir, pathToAdd));
this.readOnlyPaths.add(resolved);
} catch (e) {
debugLogger.warn(`Failed to add read-only path ${pathToAdd}:`, e);
}
}
private resolveAndValidateDir(directory: string): string {
const absolutePath = path.resolve(this.targetDir, directory);
@@ -174,6 +193,34 @@ export class WorkspaceContext {
}
}
/**
* Checks if a path is allowed to be read.
* This includes workspace paths and explicitly added read-only paths.
* @param pathToCheck The path to validate
* @returns True if the path is readable, false otherwise
*/
isPathReadable(pathToCheck: string): boolean {
if (this.isPathWithinWorkspace(pathToCheck)) {
return true;
}
try {
const fullyResolvedPath = this.fullyResolvedPath(pathToCheck);
for (const allowedPath of this.readOnlyPaths) {
// Allow exact matches or subpaths (if allowedPath is a directory)
if (
fullyResolvedPath === allowedPath ||
this.isPathWithinRoot(fullyResolvedPath, allowedPath)
) {
return true;
}
}
return false;
} catch (_error) {
return false;
}
}
/**
* Fully resolves a path, including symbolic links.
* If the path does not exist, it returns the fully resolved path as it would be