diff --git a/packages/cli/src/ui/commands/planCommand.ts b/packages/cli/src/ui/commands/planCommand.ts index 3575561b52..f439b8535b 100644 --- a/packages/cli/src/ui/commands/planCommand.ts +++ b/packages/cli/src/ui/commands/planCommand.ts @@ -6,6 +6,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; +import { isDirectorySecure } from '@google/gemini-cli-core'; import type { SlashCommand } from './types.js'; import { CommandKind } from './types.js'; import { MessageType } from '../types.js'; @@ -26,6 +27,18 @@ export const planCommand: SlashCommand = { const plansDir = config.storage.getProjectTempPlansDir(); try { + const securityCheck = await isDirectorySecure(plansDir); + if (!securityCheck.secure) { + context.ui.addItem( + { + type: MessageType.ERROR, + text: `Security check failed for plans directory: ${securityCheck.reason}`, + }, + Date.now(), + ); + return; + } + if (fs.existsSync(plansDir)) { const files = await fs.promises.readdir(plansDir); const mdFiles = files.filter((f) => f.endsWith('.md')); diff --git a/packages/cli/src/ui/components/messages/Todo.test.tsx b/packages/cli/src/ui/components/messages/Todo.test.tsx index 6d2906dfcb..9b37fc39bd 100644 --- a/packages/cli/src/ui/components/messages/Todo.test.tsx +++ b/packages/cli/src/ui/components/messages/Todo.test.tsx @@ -4,15 +4,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { render } from '../../../test-utils/render.js'; +import { renderWithProviders as render } from '../../../test-utils/render.js'; import { describe, it, expect } from 'vitest'; -import { Box } from 'ink'; import { TodoTray } from './Todo.js'; import type { Todo } from '@google/gemini-cli-core'; -import type { UIState } from '../../contexts/UIStateContext.js'; -import { UIStateContext } from '../../contexts/UIStateContext.js'; import type { HistoryItem } from '../../types.js'; import { ToolCallStatus } from '../../types.js'; +import type { UIState } from '../../contexts/UIStateContext.js'; const createTodoHistoryItem = (todos: Todo[]): HistoryItem => ({ @@ -34,21 +32,18 @@ describe.each([true, false])( ' (showFullTodos: %s)', (showFullTodos: boolean) => { const renderWithUiState = (uiState: Partial) => - render( - - - , - ); + render(, { + uiState: { ...uiState, showFullTodos }, + }); it('renders null when no todos are in the history', () => { - const { lastFrame } = renderWithUiState({ history: [], showFullTodos }); + const { lastFrame } = renderWithUiState({ history: [] }); expect(lastFrame()).toMatchSnapshot(); }); it('renders null when todo list is empty', () => { const { lastFrame } = renderWithUiState({ history: [createTodoHistoryItem([])], - showFullTodos, }); expect(lastFrame()).toMatchSnapshot(); }); @@ -62,7 +57,6 @@ describe.each([true, false])( { description: 'Completed Task', status: 'completed' }, ]), ], - showFullTodos, }); expect(lastFrame()).toMatchSnapshot(); }); @@ -77,39 +71,31 @@ describe.each([true, false])( { description: 'Completed Task', status: 'completed' }, ]), ], - showFullTodos, }); expect(lastFrame()).toMatchSnapshot(); }); it('renders a todo list with long descriptions that wrap when full view is on', () => { - const { lastFrame } = render( - - , { + width: 50, + uiState: { + history: [ + createTodoHistoryItem([ { - history: [ - createTodoHistoryItem([ - { - description: - 'This is a very long description for a pending task that should wrap around multiple lines when the terminal width is constrained.', - status: 'in_progress', - }, - { - description: - 'Another completed task with an equally verbose description to test wrapping behavior.', - status: 'completed', - }, - ]), - ], - showFullTodos, - } as UIState - } - > - - - , - ); + description: + 'This is a very long description for a pending task that should wrap around multiple lines when the terminal width is constrained.', + status: 'in_progress', + }, + { + description: + 'Another completed task with an equally verbose description to test wrapping behavior.', + status: 'completed', + }, + ]), + ], + showFullTodos, + }, + }); expect(lastFrame()).toMatchSnapshot(); }); @@ -125,7 +111,6 @@ describe.each([true, false])( { description: 'Newer Task 2', status: 'in_progress' }, ]), ], - showFullTodos, }); expect(lastFrame()).toMatchSnapshot(); }); @@ -138,9 +123,34 @@ describe.each([true, false])( { description: 'Task 2', status: 'cancelled' }, ]), ], - showFullTodos, }); expect(lastFrame()).toMatchSnapshot(); }); + + it('renders planTodos instead of history todos when available', () => { + const { lastFrame } = renderWithUiState({ + history: [ + createTodoHistoryItem([ + { description: 'History Task', status: 'pending' }, + ]), + ], + planTodos: [ + { description: 'Plan Task 1', status: 'in_progress' }, + { description: 'Plan Task 2', status: 'pending' }, + ], + planFileName: 'implementation.md', + }); + expect(lastFrame()).toMatchSnapshot(); + }); + + it('renders planTodos with the correct filename in the header', () => { + const { lastFrame } = renderWithUiState({ + planTodos: [{ description: 'Only Task', status: 'pending' }], + planFileName: 'my-feature.md', + showFullTodos: true, + }); + expect(lastFrame()).toContain('Plan: my-feature.md'); + expect(lastFrame()).toMatchSnapshot(); + }); }, ); diff --git a/packages/cli/src/ui/components/messages/Todo.tsx b/packages/cli/src/ui/components/messages/Todo.tsx index f6680573e2..ffae91d087 100644 --- a/packages/cli/src/ui/components/messages/Todo.tsx +++ b/packages/cli/src/ui/components/messages/Todo.tsx @@ -6,6 +6,7 @@ import type React from 'react'; import { Box, Text } from 'ink'; +import stripAnsi from 'strip-ansi'; import { type Todo, type TodoList, @@ -37,7 +38,7 @@ const TodoTitleDisplay: React.FC<{ return ( - {fileName ? `Plan: ${fileName}` : 'Todo'} + {fileName ? `Plan: ${stripAnsi(fileName)}` : 'Todo'} {score} (ctrl+t to toggle) diff --git a/packages/cli/src/ui/components/messages/__snapshots__/Todo.test.tsx.snap b/packages/cli/src/ui/components/messages/__snapshots__/Todo.test.tsx.snap index 04e9663011..9801a947f4 100644 --- a/packages/cli/src/ui/components/messages/__snapshots__/Todo.test.tsx.snap +++ b/packages/cli/src/ui/components/messages/__snapshots__/Todo.test.tsx.snap @@ -11,18 +11,28 @@ exports[` (showFullTodos: false) > renders null when no todos are in exports[` (showFullTodos: false) > renders null when todo list is empty 1`] = `""`; +exports[` (showFullTodos: false) > renders planTodos instead of history todos when available 1`] = ` +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + Plan: implementation.md 0/2 completed (ctrl+t to toggle) » Plan Task 1" +`; + +exports[` (showFullTodos: false) > renders planTodos with the correct filename in the header 1`] = ` +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + Plan: my-feature.md 0/1 completed (ctrl+t to toggle)" +`; + exports[` (showFullTodos: false) > renders the most recent todo list when multiple write_todos calls are in history 1`] = ` -"──────────────────────────────────────────────────────────────────────────────────────────────────── +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Todo 0/2 completed (ctrl+t to toggle) » Newer Task 2" `; exports[` (showFullTodos: false) > renders when todos exist and one is in progress 1`] = ` -"──────────────────────────────────────────────────────────────────────────────────────────────────── +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Todo 1/3 completed (ctrl+t to toggle) » Task 2" `; exports[` (showFullTodos: false) > renders when todos exist but none are in progress 1`] = ` -"──────────────────────────────────────────────────────────────────────────────────────────────────── +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Todo 1/2 completed (ctrl+t to toggle)" `; @@ -38,7 +48,7 @@ exports[` (showFullTodos: true) > renders a todo list with long desc `; exports[` (showFullTodos: true) > renders full list when all todos are inactive 1`] = ` -"──────────────────────────────────────────────────────────────────────────────────────────────────── +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Todo 1/1 completed (ctrl+t to toggle) ✓ Task 1 @@ -49,8 +59,23 @@ exports[` (showFullTodos: true) > renders null when no todos are in exports[` (showFullTodos: true) > renders null when todo list is empty 1`] = `""`; +exports[` (showFullTodos: true) > renders planTodos instead of history todos when available 1`] = ` +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + Plan: implementation.md 0/2 completed (ctrl+t to toggle) + + » Plan Task 1 + ☐ Plan Task 2" +`; + +exports[` (showFullTodos: true) > renders planTodos with the correct filename in the header 1`] = ` +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── + Plan: my-feature.md 0/1 completed (ctrl+t to toggle) + + ☐ Only Task" +`; + exports[` (showFullTodos: true) > renders the most recent todo list when multiple write_todos calls are in history 1`] = ` -"──────────────────────────────────────────────────────────────────────────────────────────────────── +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Todo 0/2 completed (ctrl+t to toggle) ☐ Newer Task 1 @@ -58,7 +83,7 @@ exports[` (showFullTodos: true) > renders the most recent todo list `; exports[` (showFullTodos: true) > renders when todos exist and one is in progress 1`] = ` -"──────────────────────────────────────────────────────────────────────────────────────────────────── +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Todo 1/3 completed (ctrl+t to toggle) ☐ Pending Task @@ -68,7 +93,7 @@ exports[` (showFullTodos: true) > renders when todos exist and one i `; exports[` (showFullTodos: true) > renders when todos exist but none are in progress 1`] = ` -"──────────────────────────────────────────────────────────────────────────────────────────────────── +"──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Todo 1/2 completed (ctrl+t to toggle) ☐ Pending Task diff --git a/packages/cli/src/ui/hooks/usePlanMonitoring.test.ts b/packages/cli/src/ui/hooks/usePlanMonitoring.test.ts new file mode 100644 index 0000000000..abf2b903a4 --- /dev/null +++ b/packages/cli/src/ui/hooks/usePlanMonitoring.test.ts @@ -0,0 +1,176 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { usePlanMonitoring } from './usePlanMonitoring.js'; +import { renderHook } from '../../test-utils/render.js'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; +import { makeFakeConfig, type Config } from '@google/gemini-cli-core'; +import { waitFor } from '../../test-utils/async.js'; +import { act } from 'react'; + +const { mockFsPromises } = vi.hoisted(() => ({ + mockFsPromises: { + readdir: vi.fn(), + lstat: vi.fn(), + readFile: vi.fn(), + stat: vi.fn(), + }, +})); + +vi.mock('node:fs', () => ({ + existsSync: vi.fn(), + lstatSync: vi.fn(), + statSync: vi.fn(), + realpathSync: vi.fn(), + promises: mockFsPromises, + default: { + existsSync: vi.fn(), + lstatSync: vi.fn(), + statSync: vi.fn(), + realpathSync: vi.fn(), + promises: mockFsPromises, + }, +})); + +vi.mock('node:fs/promises', () => mockFsPromises); + +describe('usePlanMonitoring', () => { + let mockConfig: Config; + const plansDir = path.join(os.tmpdir(), 'test-plans'); + + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + + vi.mocked(fs.existsSync).mockImplementation((p) => { + const pathStr = p.toString(); + if (pathStr === plansDir || pathStr === os.tmpdir()) return true; + return false; + }); + vi.mocked(fs.lstatSync).mockImplementation((p) => { + const pathStr = p.toString(); + if (pathStr === plansDir || pathStr === os.tmpdir()) { + return { isDirectory: () => true } as fs.Stats; + } + throw new Error('ENOENT'); + }); + vi.mocked(fs.statSync).mockImplementation((p) => { + const pathStr = p.toString(); + if (pathStr === plansDir || pathStr === os.tmpdir()) { + return { isDirectory: () => true } as fs.Stats; + } + throw new Error('ENOENT'); + }); + vi.mocked(fs.realpathSync).mockImplementation((p) => p.toString()); + + mockConfig = makeFakeConfig({ + targetDir: os.tmpdir(), + }); + vi.spyOn(mockConfig.storage, 'getProjectTempPlansDir').mockReturnValue( + plansDir, + ); + mockFsPromises.readdir.mockResolvedValue([]); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); + }); + + it('initially has null todos and planFileName', async () => { + mockFsPromises.readdir.mockResolvedValue([]); + const { result } = renderHook(() => usePlanMonitoring(mockConfig)); + + expect(result.current.planTodos).toBeNull(); + expect(result.current.planFileName).toBeNull(); + }); + + it('reads the most recently modified markdown file', async () => { + const file1 = 'old.md'; + const file2 = 'new.md'; + + mockFsPromises.readdir.mockResolvedValue([file1, file2]); + mockFsPromises.lstat.mockImplementation(async (p) => { + const pathStr = p.toString(); + if (pathStr.endsWith(file1)) + return { isFile: () => true, mtimeMs: 100 } as fs.Stats; + if (pathStr.endsWith(file2)) + return { isFile: () => true, mtimeMs: 200 } as fs.Stats; + return { isFile: () => false } as fs.Stats; + }); + mockFsPromises.readFile.mockResolvedValue('- [ ] Task from new.md'); + + const { result } = renderHook(() => usePlanMonitoring(mockConfig)); + + await waitFor(() => { + expect(result.current.planFileName).toBe(file2); + expect(result.current.planTodos).toEqual([ + { description: 'Task from new.md', status: 'pending' }, + ]); + }); + }); + + it('updates todos when the file content changes', async () => { + mockFsPromises.readdir.mockResolvedValue(['plan.md']); + mockFsPromises.lstat.mockResolvedValue({ + isFile: () => true, + mtimeMs: 100, + } as fs.Stats); + + mockFsPromises.readFile.mockResolvedValueOnce('- [ ] Initial Task'); + + const { result } = renderHook(() => usePlanMonitoring(mockConfig)); + + await waitFor(() => { + expect(result.current.planTodos?.[0].description).toBe('Initial Task'); + }); + + // Change mtime and content + mockFsPromises.lstat.mockResolvedValue({ + isFile: () => true, + mtimeMs: 200, + } as fs.Stats); + mockFsPromises.readFile.mockResolvedValueOnce('- [x] Completed Task'); + + // Advance timers to trigger interval + await act(async () => { + await vi.advanceTimersByTimeAsync(2000); + }); + + await waitFor(() => { + expect(result.current.planTodos?.[0].description).toBe('Completed Task'); + expect(result.current.planTodos?.[0].status).toBe('completed'); + }); + }); + + it('clears todos when no markdown files are found', async () => { + mockFsPromises.readdir.mockResolvedValueOnce(['plan.md']); + mockFsPromises.lstat.mockResolvedValue({ + isFile: () => true, + mtimeMs: 100, + } as fs.Stats); + mockFsPromises.readFile.mockResolvedValue('- [ ] Task'); + + const { result } = renderHook(() => usePlanMonitoring(mockConfig)); + + await waitFor(() => { + expect(result.current.planTodos).not.toBeNull(); + }); + + mockFsPromises.readdir.mockResolvedValueOnce([]); + + await act(async () => { + await vi.advanceTimersByTimeAsync(2000); + }); + + await waitFor(() => { + expect(result.current.planTodos).toBeNull(); + expect(result.current.planFileName).toBeNull(); + }); + }); +}); diff --git a/packages/cli/src/ui/hooks/usePlanMonitoring.ts b/packages/cli/src/ui/hooks/usePlanMonitoring.ts index 6aa4414487..75e231f86a 100644 --- a/packages/cli/src/ui/hooks/usePlanMonitoring.ts +++ b/packages/cli/src/ui/hooks/usePlanMonitoring.ts @@ -12,11 +12,13 @@ import { type Todo, type Config, debugLogger, + isDirectorySecure, } from '@google/gemini-cli-core'; export function usePlanMonitoring(config: Config) { const [planTodos, setPlanTodos] = useState(null); const [planFileName, setPlanFileName] = useState(null); + const planFileNameRef = useRef(null); const lastModifiedRef = useRef(0); useEffect(() => { @@ -24,6 +26,15 @@ export function usePlanMonitoring(config: Config) { const updatePlan = async () => { try { + const securityCheck = await isDirectorySecure(plansDir); + if (!securityCheck.secure) { + debugLogger.warn( + 'Security check failed for plans directory', + securityCheck.reason, + ); + return; + } + if (!fs.existsSync(plansDir)) { return; } @@ -32,8 +43,11 @@ export function usePlanMonitoring(config: Config) { const mdFiles = files.filter((f) => f.endsWith('.md')); if (mdFiles.length === 0) { - setPlanTodos(null); - setPlanFileName(null); + if (planFileNameRef.current !== null) { + setPlanTodos(null); + setPlanFileName(null); + planFileNameRef.current = null; + } return; } @@ -43,8 +57,8 @@ export function usePlanMonitoring(config: Config) { for (const file of mdFiles) { const filePath = path.join(plansDir, file); - const stats = await fs.promises.stat(filePath); - if (stats.mtimeMs > latestMtime) { + const stats = await fs.promises.lstat(filePath); + if (stats.isFile() && stats.mtimeMs > latestMtime) { latestMtime = stats.mtimeMs; latestFile = file; } @@ -52,7 +66,7 @@ export function usePlanMonitoring(config: Config) { if ( latestMtime > lastModifiedRef.current || - latestFile !== planFileName + latestFile !== planFileNameRef.current ) { const content = await fs.promises.readFile( path.join(plansDir, latestFile), @@ -61,6 +75,7 @@ export function usePlanMonitoring(config: Config) { const todos = parseMarkdownTodos(content); setPlanTodos(todos); setPlanFileName(latestFile); + planFileNameRef.current = latestFile; lastModifiedRef.current = latestMtime; } } catch (error) { @@ -77,7 +92,7 @@ export function usePlanMonitoring(config: Config) { }, 2000); return () => clearInterval(interval); - }, [config, planFileName]); + }, [config]); return { planTodos, planFileName }; } diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index d72c36a4bb..cd9d779696 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -219,7 +219,7 @@ import { import { McpClientManager } from '../tools/mcp-client-manager.js'; import type { EnvironmentSanitizationConfig } from '../services/environmentSanitization.js'; import { getErrorMessage } from '../utils/errors.js'; -import { WriteTodosTool } from 'src/tools/write-todos.js'; +import { WriteTodosTool } from '../tools/write-todos.js'; export type { FileFilteringOptions }; export { diff --git a/packages/core/src/core/prompts.ts b/packages/core/src/core/prompts.ts index 9df1cc3354..3cad6454aa 100644 --- a/packages/core/src/core/prompts.ts +++ b/packages/core/src/core/prompts.ts @@ -162,7 +162,7 @@ ${planModeToolsList} - Save your plans as Markdown (.md) files directly to: \`${plansDir}/\` - Use descriptive filenames: \`feature-name.md\` or \`bugfix-description.md\` - **Source of Truth:** The UI is based on these markdown files. To show progress in the CLI, you MUST update the plan file with the status markers below. -- **Task Status Markers:** Use the following markers in your task lists to update the UI: +- **Task Status Markers:** Use the following markers in your task lists to update the UI (note that these are case-sensitive): - \`- [ ] Task\` : Pending - \`- [/] Task\` : In Progress - \`- [x] Task\` : Completed @@ -280,7 +280,7 @@ When requested to perform tasks like fixing bugs, adding features, refactoring, **Monitoring Progress:** - **Plans Directory:** \`${config.storage.getProjectTempPlansDir()}/\` - If a plan file exists in this directory, you MUST update it to reflect your progress using the \`write_file\` or \`replace\` tools. -- Use markers: \`- [ ]\` (Pending), \`- [/]\` (In Progress), \`- [x]\` (Completed), \`- [-]\` (Cancelled). +- Use markers (case-sensitive): \`- [ ]\` (Pending), \`- [/]\` (In Progress), \`- [x]\` (Completed), \`- [-]\` (Cancelled). ## New Applications diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 90896c8dcb..1c7fcbff38 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -97,6 +97,7 @@ export * from './utils/secure-browser-launcher.js'; export * from './utils/apiConversionUtils.js'; export * from './utils/channel.js'; export * from './utils/constants.js'; +export * from './utils/security.js'; // Export services export * from './services/fileDiscoveryService.js'; diff --git a/packages/core/src/utils/planUtils.test.ts b/packages/core/src/utils/planUtils.test.ts index a6a7408d93..d3b382a9c5 100644 --- a/packages/core/src/utils/planUtils.test.ts +++ b/packages/core/src/utils/planUtils.test.ts @@ -57,4 +57,16 @@ describe('parseMarkdownTodos', () => { const todos = parseMarkdownTodos(markdown); expect(todos).toEqual([{ description: 'A task', status: 'pending' }]); }); + + it('is case-sensitive for completed marker', () => { + const markdown = ` +- [x] lowercase +- [X] uppercase + `; + const todos = parseMarkdownTodos(markdown); + expect(todos).toEqual([ + { description: 'lowercase', status: 'completed' }, + { description: 'uppercase', status: 'pending' }, + ]); + }); }); diff --git a/packages/core/src/utils/planUtils.ts b/packages/core/src/utils/planUtils.ts index 897619bd51..ba4d251046 100644 --- a/packages/core/src/utils/planUtils.ts +++ b/packages/core/src/utils/planUtils.ts @@ -40,7 +40,7 @@ export function parseMarkdownTodos(content: string): Todo[] { ); if (taskMarkerMatch) { - const marker = taskMarkerMatch[1].toLowerCase(); + const marker = taskMarkerMatch[1]; const description = taskMarkerMatch[2].split('\n')[0].trim(); // Take only the first line as description let status: TodoStatus = 'pending'; @@ -50,7 +50,7 @@ export function parseMarkdownTodos(content: string): Todo[] { status = 'in_progress'; } else if (marker === '-') { status = 'cancelled'; - } else if (marker === '') { + } else if (marker === '' || marker === ' ') { status = 'pending'; } diff --git a/packages/core/src/utils/security.ts b/packages/core/src/utils/security.ts index cd08a34dac..c8b017acae 100644 --- a/packages/core/src/utils/security.ts +++ b/packages/core/src/utils/security.ts @@ -14,81 +14,132 @@ export interface SecurityCheckResult { reason?: string; } +export interface SecurityCheckOptions { + /** + * The expected owner of the directory. + * - 'root': Owned by root (uid 0) on POSIX. + * - 'user': Owned by the current user. + * Defaults to 'root'. + */ + owner?: 'root' | 'user'; + /** + * Whether to allow symbolic links. + * Defaults to false. + */ + allowSymlinks?: boolean; +} + /** - * Verifies if a directory is secure (owned by root and not writable by others). + * Verifies if a directory is secure. + * + * For 'root' owner (default): + * - POSIX: Owned by root (uid 0) and not writable by group or others. + * - Windows: ACLs checked to ensure standard users/groups don't have write access. + * + * For 'user' owner: + * - POSIX: Owned by current user and has restrictive permissions (0700). + * - Windows: Verified as a directory and not a symbolic link. * * @param dirPath The path to the directory to check. + * @param options Security check options. * @returns A promise that resolves to a SecurityCheckResult. */ export async function isDirectorySecure( dirPath: string, + options: SecurityCheckOptions = {}, ): Promise { + const { owner = 'root', allowSymlinks = false } = options; + try { - const stats = await fs.stat(dirPath); + const stats = await fs.lstat(dirPath); + + if (!allowSymlinks && stats.isSymbolicLink()) { + return { secure: false, reason: 'Directory is a symbolic link' }; + } if (!stats.isDirectory()) { return { secure: false, reason: 'Not a directory' }; } if (os.platform() === 'win32') { - try { - // Check ACLs using PowerShell to ensure standard users don't have write access - const escapedPath = dirPath.replace(/'/g, "''"); - const script = ` - $path = '${escapedPath}'; - $acl = Get-Acl -LiteralPath $path; - $rules = $acl.Access | Where-Object { - $_.AccessControlType -eq 'Allow' -and - (($_.FileSystemRights -match 'Write') -or ($_.FileSystemRights -match 'Modify') -or ($_.FileSystemRights -match 'FullControl')) - }; - $insecureIdentity = $rules | Where-Object { - $_.IdentityReference.Value -match 'Users' -or $_.IdentityReference.Value -eq 'Everyone' - } | Select-Object -ExpandProperty IdentityReference; - Write-Output ($insecureIdentity -join ', '); - `; + if (owner === 'root') { + try { + // Check ACLs using PowerShell to ensure standard users don't have write access + const escapedPath = dirPath.replace(/'/g, "''"); + const script = ` + $path = '${escapedPath}'; + $acl = Get-Acl -LiteralPath $path; + $rules = $acl.Access | Where-Object { + $_.AccessControlType -eq 'Allow' -and + (($_.FileSystemRights -match 'Write') -or ($_.FileSystemRights -match 'Modify') -or ($_.FileSystemRights -match 'FullControl')) + }; + $insecureIdentity = $rules | Where-Object { + $_.IdentityReference.Value -match 'Users' -or $_.IdentityReference.Value -eq 'Everyone' + } | Select-Object -ExpandProperty IdentityReference; + Write-Output ($insecureIdentity -join ', '); + `; - const { stdout } = await spawnAsync('powershell', [ - '-NoProfile', - '-NonInteractive', - '-Command', - script, - ]); + const { stdout } = await spawnAsync('powershell', [ + '-NoProfile', + '-NonInteractive', + '-Command', + script, + ]); - const insecureGroups = stdout.trim(); - if (insecureGroups) { + const insecureGroups = stdout.trim(); + if (insecureGroups) { + return { + secure: false, + reason: `Directory '${dirPath}' is insecure. The following user groups have write permissions: ${insecureGroups}. To fix this, remove Write and Modify permissions for these groups from the directory's ACLs.`, + }; + } + } catch (error) { return { secure: false, - reason: `Directory '${dirPath}' is insecure. The following user groups have write permissions: ${insecureGroups}. To fix this, remove Write and Modify permissions for these groups from the directory's ACLs.`, + reason: `A security check for the system policy directory '${dirPath}' failed and could not be completed. Please file a bug report. Original error: ${(error as Error).message}`, }; } - - return { secure: true }; - } catch (error) { - return { - secure: false, - reason: `A security check for the system policy directory '${dirPath}' failed and could not be completed. Please file a bug report. Original error: ${(error as Error).message}`, - }; } + return { secure: true }; } // POSIX checks - // Check ownership: must be root (uid 0) - if (stats.uid !== 0) { - return { - secure: false, - reason: `Directory '${dirPath}' is not owned by root (uid 0). Current uid: ${stats.uid}. To fix this, run: sudo chown root:root "${dirPath}"`, - }; - } + if (owner === 'root') { + // Check ownership: must be root (uid 0) + if (stats.uid !== 0) { + return { + secure: false, + reason: `Directory '${dirPath}' is not owned by root (uid 0). Current uid: ${stats.uid}. To fix this, run: sudo chown root:root "${dirPath}"`, + }; + } - // Check permissions: not writable by group (S_IWGRP) or others (S_IWOTH) - const mode = stats.mode; - if ((mode & (constants.S_IWGRP | constants.S_IWOTH)) !== 0) { - return { - secure: false, - reason: `Directory '${dirPath}' is writable by group or others (mode: ${mode.toString( - 8, - )}). To fix this, run: sudo chmod g-w,o-w "${dirPath}"`, - }; + // Check permissions: not writable by group (S_IWGRP) or others (S_IWOTH) + const mode = stats.mode; + if ((mode & (constants.S_IWGRP | constants.S_IWOTH)) !== 0) { + return { + secure: false, + reason: `Directory '${dirPath}' is writable by group or others (mode: ${mode.toString( + 8, + )}). To fix this, run: sudo chmod g-w,o-w "${dirPath}"`, + }; + } + } else { + const userInfo = os.userInfo(); + if (stats.uid !== userInfo.uid) { + return { + secure: false, + reason: `Directory is not owned by the current user (uid ${userInfo.uid})`, + }; + } + + // Check for restrictive permissions (0700) + const mode = stats.mode & 0o777; + if (mode !== 0o700) { + return { + secure: false, + reason: `Directory has insecure permissions: ${mode.toString(8)} (expected 0700)`, + }; + } } return { secure: true };