Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cli/src/test-utils/mockConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
64 changes: 64 additions & 0 deletions packages/cli/src/ui/AppContainer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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!());
},
);
});
});
31 changes: 29 additions & 2 deletions packages/cli/src/ui/AppContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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);
Comment thread
scidomino marked this conversation as resolved.
},
});
addInput(submittedValue);
return;
}
}
void submitQuery(submittedValue);
} else {
// Check messageQueue.length === 0 to only notify on the first queued item
Expand All @@ -1103,6 +1126,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
isMcpReady,
streamingState,
messageQueue.length,
config,
],
);

Expand Down Expand Up @@ -1221,7 +1245,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
!showPrivacyNotice &&
geminiClient?.isInitialized?.()
) {
handleFinalSubmit(initialPrompt);
void handleFinalSubmit(initialPrompt);
initialPromptSubmitted.current = true;
}
}, [
Expand Down Expand Up @@ -1714,6 +1738,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
adminSettingsChanged ||
!!commandConfirmationRequest ||
!!authConsentRequest ||
!!permissionConfirmationRequest ||
!!customDialog ||
confirmUpdateExtensionRequests.length > 0 ||
!!loopDetectionConfirmationRequest ||
Expand Down Expand Up @@ -1819,6 +1844,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
authConsentRequest,
confirmUpdateExtensionRequests,
loopDetectionConfirmationRequest,
permissionConfirmationRequest,
geminiMdFileCount,
streamingState,
initError,
Expand Down Expand Up @@ -1925,6 +1951,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
authConsentRequest,
confirmUpdateExtensionRequests,
loopDetectionConfirmationRequest,
permissionConfirmationRequest,
geminiMdFileCount,
streamingState,
initError,
Expand Down
14 changes: 14 additions & 0 deletions packages/cli/src/ui/components/DialogManager.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/ui/contexts/UIActionsContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/ui/contexts/UIStateContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
34 changes: 0 additions & 34 deletions packages/cli/src/ui/hooks/atCommandProcessor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
46 changes: 32 additions & 14 deletions packages/cli/src/ui/hooks/atCommandProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
getErrorMessage,
isNodeError,
unescapePath,
resolveToRealPath,
fileExists,
ReadManyFilesTool,
REFERENCE_CONTENT_START,
REFERENCE_CONTENT_END,
Expand Down Expand Up @@ -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;
Comment thread
scidomino marked this conversation as resolved.
}

interface ResolvedFile {
part: AtCommandPart;
pathSpec: string;
Expand Down Expand Up @@ -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, {
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions packages/cli/src/ui/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 14 additions & 1 deletion packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
11 changes: 8 additions & 3 deletions packages/core/src/tools/glob.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down
Loading
Loading