diff --git a/packages/core/src/commands/memory.test.ts b/packages/core/src/commands/memory.test.ts index 00c8a2f324..ee9b083a1b 100644 --- a/packages/core/src/commands/memory.test.ts +++ b/packages/core/src/commands/memory.test.ts @@ -325,6 +325,8 @@ describe('memory commands', () => { let projectRoot: string; let globalMemoryDir: string; let patchConfig: Config; + const isCaseInsensitivePathPlatform = + process.platform === 'win32' || process.platform === 'darwin'; function buildUpdatePatch( absoluteTargetPath: string, @@ -372,6 +374,12 @@ describe('memory commands', () => { ].join('\n'); } + function swapAsciiPathCase(filePath: string): string { + return filePath.replace(/[a-z]/gi, (char) => + char === char.toLowerCase() ? char.toUpperCase() : char.toLowerCase(), + ); + } + beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'memory-patch-test-')); // Canonicalize so test-side paths match production's @@ -466,6 +474,49 @@ describe('memory commands', () => { expect(result.message).toMatch(/outside the private memory root/i); }); + it('rejects private patches that target in-root non-memory documents', async () => { + const patchDir = path.join(memoryTempDir, '.inbox', 'private'); + await fs.mkdir(patchDir, { recursive: true }); + + const rejectedTargets = [ + ['state.patch', path.join(memoryTempDir, '.extraction-state.json')], + ['lock.patch', path.join(memoryTempDir, '.extraction.lock')], + [ + 'inbox.patch', + path.join(memoryTempDir, '.inbox', 'private', 'review.md'), + ], + [ + 'skills.patch', + path.join(memoryTempDir, 'skills', 'generated', 'SKILL.md'), + ], + ['text.patch', path.join(memoryTempDir, 'notes.txt')], + ['nested.patch', path.join(memoryTempDir, 'nested', 'topic.md')], + ] as const; + + for (const [fileName, targetPath] of rejectedTargets) { + await fs.writeFile( + path.join(patchDir, fileName), + buildCreationPatch(targetPath, 'rejected\n'), + ); + } + + const patches = await listInboxMemoryPatches(patchConfig); + expect(patches).toHaveLength(0); + + for (const [fileName, targetPath] of rejectedTargets) { + const result = await applyInboxMemoryPatch( + patchConfig, + 'private', + fileName, + ); + expect(result.success).toBe(false); + expect(result.message).toMatch( + /outside the private memory root or target allowlist/i, + ); + await expect(fs.access(targetPath)).rejects.toThrow(); + } + }); + it('omits global patches with disallowed targets from the listing', async () => { // Same defense for the global tier: only ~/.gemini/GEMINI.md is allowed. // memory.md (legacy lowercase), sibling .md files, and settings.json all @@ -490,6 +541,13 @@ describe('memory commands', () => { path.join(patchDir, 'settings.patch'), buildCreationPatch(path.join(globalMemoryDir, 'settings.json'), '{}\n'), ); + await fs.writeFile( + path.join(patchDir, 'nested.patch'), + buildCreationPatch( + path.join(globalMemoryDir, 'GEMINI.md', 'nested.md'), + 'rejected\n', + ), + ); const patches = await listInboxMemoryPatches(patchConfig); expect(patches).toHaveLength(0); @@ -519,6 +577,39 @@ describe('memory commands', () => { ).rejects.toThrow(); }); + it.runIf(isCaseInsensitivePathPlatform)( + 'accepts private memory patch targets with different path casing', + async () => { + const target = path.join(memoryTempDir, 'MEMORY.md'); + await fs.writeFile(target, '- old\n'); + + const patchDir = path.join(memoryTempDir, '.inbox', 'private'); + await fs.mkdir(patchDir, { recursive: true }); + await fs.writeFile( + path.join(patchDir, 'MEMORY.patch'), + buildUpdatePatch( + swapAsciiPathCase(target), + '- old\n', + '- accepted\n', + ), + ); + + const patches = await listInboxMemoryPatches(patchConfig); + expect(patches).toHaveLength(1); + + const result = await applyInboxMemoryPatch( + patchConfig, + 'private', + 'MEMORY.patch', + ); + + expect(result.success).toBe(true); + await expect(fs.readFile(target, 'utf-8')).resolves.toBe( + '- accepted\n', + ); + }, + ); + it('applies a private creation patch with a paired MEMORY.md pointer', async () => { // The auto-memory contract: creating a sibling .md file requires a // hunk that adds a pointer to MEMORY.md (so the sibling becomes @@ -713,6 +804,39 @@ describe('memory commands', () => { ).rejects.toThrow(); }); + it.runIf(isCaseInsensitivePathPlatform)( + 'accepts global memory patch targets with different path casing', + async () => { + const target = path.join(globalMemoryDir, 'GEMINI.md'); + await fs.writeFile(target, '- prefer X\n'); + + const patchDir = path.join(memoryTempDir, '.inbox', 'global'); + await fs.mkdir(patchDir, { recursive: true }); + await fs.writeFile( + path.join(patchDir, 'GEMINI.patch'), + buildUpdatePatch( + swapAsciiPathCase(target), + '- prefer X\n', + '- prefer Y\n', + ), + ); + + const patches = await listInboxMemoryPatches(patchConfig); + expect(patches).toHaveLength(1); + + const result = await applyInboxMemoryPatch( + patchConfig, + 'global', + 'GEMINI.patch', + ); + + expect(result.success).toBe(true); + await expect(fs.readFile(target, 'utf-8')).resolves.toBe( + '- prefer Y\n', + ); + }, + ); + it('dismisses a single memory patch from the inbox (legacy single-file mode)', async () => { const patchDir = path.join(memoryTempDir, '.inbox', 'global'); await fs.mkdir(patchDir, { recursive: true }); @@ -871,10 +995,20 @@ describe('memory commands', () => { ), ); + // Child paths under the single allowed file path are not allowed either. + await fs.writeFile( + path.join(patchDir, 'nested.patch'), + buildCreationPatch( + path.join(globalMemoryDir, 'GEMINI.md', 'nested.md'), + 'Should be rejected.\n', + ), + ); + for (const fileName of [ 'wrong-name.patch', 'sibling.patch', 'settings.patch', + 'nested.patch', ]) { const result = await applyInboxMemoryPatch( patchConfig, @@ -891,6 +1025,9 @@ describe('memory commands', () => { fs.access(path.join(globalMemoryDir, orphan)), ).rejects.toThrow(); } + await expect( + fs.access(path.join(globalMemoryDir, 'GEMINI.md', 'nested.md')), + ).rejects.toThrow(); }); it('rejects invalid memory patch paths', async () => { diff --git a/packages/core/src/commands/memory.ts b/packages/core/src/commands/memory.ts index 53f9564871..a3e331d634 100644 --- a/packages/core/src/commands/memory.ts +++ b/packages/core/src/commands/memory.ts @@ -13,7 +13,11 @@ import type { Config } from '../config/config.js'; import { Storage } from '../config/storage.js'; import { flattenMemory } from '../config/memory.js'; import { loadSkillFromFile, loadSkillsFromDir } from '../skills/skillLoader.js'; -import { getGlobalMemoryFilePath } from '../tools/memoryTool.js'; +import { + getGlobalMemoryFilePath, + PROJECT_MEMORY_INDEX_FILENAME, +} from '../tools/memoryTool.js'; +import { isSubpath } from '../utils/paths.js'; import { type AppliedSkillPatchTarget, applyParsedPatchesWithAllowedRoots, @@ -424,11 +428,7 @@ function getMemoryPatchRoot( } function isSubpathOrSame(childPath: string, parentPath: string): boolean { - const relativePath = path.relative(parentPath, childPath); - return ( - relativePath === '' || - (!relativePath.startsWith('..') && !path.isAbsolute(relativePath)) - ); + return isSubpath(parentPath, childPath); } function normalizeInboxMemoryPatchPath( @@ -455,11 +455,11 @@ function normalizeInboxMemoryPatchPath( } /** - * Returns the directory roots (or single-file allowlists) that a memory patch - * of the given kind is allowed to modify. Memory patch headers must reference - * paths inside / equal to one of these entries after canonical resolution. + * Returns coarse directory roots (or single-file roots) used for canonical + * containment checks before the kind-specific target validator runs. * - * - `private` allows any markdown file inside the project memory directory. + * - `private` is rooted at the project memory directory, then narrowed to + * direct memory markdown documents by `isAllowedPrivateMemoryDocumentPath`. * - `global` is intentionally a single-file allowlist: the only writeable * global file is the personal `~/.gemini/GEMINI.md`. Other files under * `~/.gemini/` (settings, credentials, oauth, keybindings, etc.) are off-limits. @@ -478,6 +478,178 @@ export function getAllowedMemoryPatchRoots( } } +interface MemoryPatchTargetValidationContext { + kind: InboxMemoryPatchKind; + allowedRoots: string[]; + privateMemoryDirs: string[]; + globalMemoryFiles: string[]; +} + +function hasMarkdownExtension(fileName: string): boolean { + return fileName.toLowerCase().endsWith('.md'); +} + +function isAllowedPrivateMemoryFileName(fileName: string): boolean { + if (fileName === PROJECT_MEMORY_INDEX_FILENAME) { + return true; + } + return !fileName.startsWith('.') && hasMarkdownExtension(fileName); +} + +function uniqueResolvedPaths(paths: readonly string[]): string[] { + return Array.from(new Set(paths.map((filePath) => path.resolve(filePath)))); +} + +function isSamePath(leftPath: string, rightPath: string): boolean { + return isSubpath(leftPath, rightPath) && isSubpath(rightPath, leftPath); +} + +function includesSamePath( + paths: readonly string[], + targetPath: string, +): boolean { + return paths.some((candidate) => isSamePath(candidate, targetPath)); +} + +function isAllowedPrivateMemoryDocumentPath( + targetPath: string, + memoryDirs: readonly string[], +): boolean { + const resolvedTargetPath = path.resolve(targetPath); + const targetDir = path.dirname(resolvedTargetPath); + if (!includesSamePath(memoryDirs, targetDir)) { + return false; + } + return isAllowedPrivateMemoryFileName(path.basename(resolvedTargetPath)); +} + +function isAllowedGlobalMemoryDocumentPath( + targetPath: string, + globalMemoryFiles: readonly string[], +): boolean { + const resolvedTargetPath = path.resolve(targetPath); + return includesSamePath(globalMemoryFiles, resolvedTargetPath); +} + +async function getMemoryPatchTargetValidationContext( + config: Config, + kind: InboxMemoryPatchKind, +): Promise { + const allowedRoots = await canonicalizeAllowedPatchRoots( + getAllowedMemoryPatchRoots(config, kind), + ); + + if (kind === 'global') { + const rawGlobalMemoryFile = path.resolve(getGlobalMemoryFilePath()); + const canonicalGlobalMemoryFiles = await canonicalizeAllowedPatchRoots([ + rawGlobalMemoryFile, + ]); + return { + kind, + allowedRoots, + privateMemoryDirs: [], + globalMemoryFiles: uniqueResolvedPaths([ + rawGlobalMemoryFile, + ...canonicalGlobalMemoryFiles, + ]), + }; + } + + const rawPrivateMemoryDir = path.resolve( + config.storage.getProjectMemoryTempDir(), + ); + const canonicalPrivateMemoryDirs = await canonicalizeAllowedPatchRoots([ + rawPrivateMemoryDir, + ]); + const privateMemoryDirs = uniqueResolvedPaths([ + rawPrivateMemoryDir, + ...canonicalPrivateMemoryDirs, + ]); + + return { kind, allowedRoots, privateMemoryDirs, globalMemoryFiles: [] }; +} + +function isResolvedMemoryPatchTargetAllowed( + resolvedTargetPath: string, + context: MemoryPatchTargetValidationContext, +): boolean { + if (context.kind === 'global') { + return isAllowedGlobalMemoryDocumentPath( + resolvedTargetPath, + context.globalMemoryFiles, + ); + } + if (context.kind === 'private') { + return isAllowedPrivateMemoryDocumentPath( + resolvedTargetPath, + context.privateMemoryDirs, + ); + } + return true; +} + +async function resolveMemoryPatchTargetWithinAllowedSet( + targetPath: string, + context: MemoryPatchTargetValidationContext, +): Promise { + const resolvedTargetPath = await resolveTargetWithinAllowedRoots( + targetPath, + context.allowedRoots, + ); + if (!resolvedTargetPath) { + return undefined; + } + if ( + context.kind === 'private' && + (!isAllowedPrivateMemoryDocumentPath( + targetPath, + context.privateMemoryDirs, + ) || + !isAllowedPrivateMemoryDocumentPath( + resolvedTargetPath, + context.privateMemoryDirs, + )) + ) { + return undefined; + } + if ( + context.kind === 'global' && + (!isAllowedGlobalMemoryDocumentPath( + targetPath, + context.globalMemoryFiles, + ) || + !isAllowedGlobalMemoryDocumentPath( + resolvedTargetPath, + context.globalMemoryFiles, + )) + ) { + return undefined; + } + return resolvedTargetPath; +} + +async function findDisallowedMemoryPatchTarget( + parsedPatches: Diff.StructuredPatch[], + context: MemoryPatchTargetValidationContext, +): Promise { + const validated = validateParsedSkillPatchHeaders(parsedPatches); + if (!validated.success) { + return undefined; + } + + for (const header of validated.patches) { + if ( + !(await resolveMemoryPatchTargetWithinAllowedSet( + header.targetPath, + context, + )) + ) { + return header.targetPath; + } + } + return undefined; +} + async function getFileMtimeIso(filePath: string): Promise { try { const stats = await fs.stat(filePath); @@ -585,8 +757,8 @@ async function listInboxPatchFiles( /** * Returns only the inbox patch files that pass the same validation as the - * inbox listing (parseable, has hunks, valid headers, targets in the - * kind's allowed root). Used by aggregate apply so the user only ever sees + * inbox listing (parseable, has hunks, valid headers, targets in the kind's + * allowed target set). Used by aggregate apply so the user only ever sees * results for patches the inbox actually surfaced. */ async function listValidInboxPatchFiles( @@ -598,8 +770,9 @@ async function listValidInboxPatchFiles( return []; } - const allowedRoots = await canonicalizeAllowedPatchRoots( - getAllowedMemoryPatchRoots(config, kind), + const validationContext = await getMemoryPatchTargetValidationContext( + config, + kind, ); const valid: string[] = []; @@ -629,9 +802,9 @@ async function listValidInboxPatchFiles( const targetsAllAllowed = await Promise.all( validated.patches.map( async (header) => - (await resolveTargetWithinAllowedRoots( + (await resolveMemoryPatchTargetWithinAllowedSet( header.targetPath, - allowedRoots, + validationContext, )) !== undefined, ), ); @@ -648,8 +821,8 @@ async function listValidInboxPatchFiles( * Scans `/.inbox/{private,global}/` and returns ONE consolidated * inbox entry per kind. Each entry aggregates all hunks from every valid * underlying `.patch` file. Patches that fail validation (unparseable, no - * hunks, target outside allowed root) are silently skipped so they don't - * pollute the inbox UI. + * hunks, target outside the allowed target set) are silently skipped so they + * don't pollute the inbox UI. */ export async function listInboxMemoryPatches( config: Config, @@ -658,8 +831,9 @@ export async function listInboxMemoryPatches( const aggregated: InboxMemoryPatch[] = []; for (const kind of kinds) { - const allowedRoots = await canonicalizeAllowedPatchRoots( - getAllowedMemoryPatchRoots(config, kind), + const validationContext = await getMemoryPatchTargetValidationContext( + config, + kind, ); const patchFiles = await listInboxPatchFiles(config, kind); @@ -691,13 +865,13 @@ export async function listInboxMemoryPatches( } // Skip the entire source file if ANY of its targets escapes the kind's - // allowed root. + // allowed target set. const targetsAllAllowed = await Promise.all( validated.patches.map( async (header) => - (await resolveTargetWithinAllowedRoots( + (await resolveMemoryPatchTargetWithinAllowedSet( header.targetPath, - allowedRoots, + validationContext, )) !== undefined, ), ); @@ -1015,12 +1189,31 @@ async function applyMemoryPatchFile( }; } - const allowedRoots = await canonicalizeAllowedPatchRoots( - getAllowedMemoryPatchRoots(config, kind), + const validationContext = await getMemoryPatchTargetValidationContext( + config, + kind, ); + const disallowedTargetPath = await findDisallowedMemoryPatchTarget( + parsed, + validationContext, + ); + if (disallowedTargetPath) { + return { + success: false, + message: `Memory patch "${displayName}" targets a file outside the ${kind} memory root or target allowlist: ${disallowedTargetPath}`, + }; + } + const applied = await applyParsedPatchesWithAllowedRoots( parsed, - allowedRoots, + validationContext.allowedRoots, + { + isResolvedTargetAllowed: (resolvedTargetPath) => + isResolvedMemoryPatchTargetAllowed( + resolvedTargetPath, + validationContext, + ), + }, ); if (!applied.success) { switch (applied.reason) { @@ -1037,7 +1230,7 @@ async function applyMemoryPatchFile( case 'outsideAllowedRoots': return { success: false, - message: `Memory patch "${displayName}" targets a file outside the ${kind} memory root: ${applied.targetPath}`, + message: `Memory patch "${displayName}" targets a file outside the ${kind} memory root or target allowlist: ${applied.targetPath}`, }; case 'newFileAlreadyExists': return { diff --git a/packages/core/src/services/memoryPatchUtils.ts b/packages/core/src/services/memoryPatchUtils.ts index 66cb0c6092..ef6984cd31 100644 --- a/packages/core/src/services/memoryPatchUtils.ts +++ b/packages/core/src/services/memoryPatchUtils.ts @@ -252,15 +252,24 @@ export async function applyParsedSkillPatches( return applyParsedPatchesWithAllowedRoots(parsedPatches, allowedRoots); } +export interface ApplyParsedPatchesWithAllowedRootsOptions { + /** + * Optional fine-grained allowlist for callers whose allowed root is broader + * than their actual target surface. Receives the canonical target path after + * root containment has already passed. + */ + isResolvedTargetAllowed?: (resolvedTargetPath: string) => boolean; +} + /** * Applies parsed unified diff patches against any caller-supplied set of * allowed root directories. This is the kind-agnostic core used by both the * skill patch flow and the memory patch flow. * * The patch headers must reference absolute paths inside one of the allowed - * roots (after canonical resolution). Update patches must reference an - * existing target; creation patches (`/dev/null` source) must reference a path - * that does not yet exist. + * roots (after canonical resolution) and pass any caller-supplied fine-grained + * target predicate. Update patches must reference an existing target; creation + * patches (`/dev/null` source) must reference a path that does not yet exist. * * Returns the per-target before/after content so callers can stage commits * and roll back on failure. @@ -268,6 +277,7 @@ export async function applyParsedSkillPatches( export async function applyParsedPatchesWithAllowedRoots( parsedPatches: StructuredPatch[], allowedRoots: string[], + options: ApplyParsedPatchesWithAllowedRootsOptions = {}, ): Promise { const results = new Map(); const patchedContentByTarget = new Map(); @@ -285,7 +295,11 @@ export async function applyParsedPatchesWithAllowedRoots( targetPath, allowedRoots, ); - if (!resolvedTargetPath) { + if ( + !resolvedTargetPath || + (options.isResolvedTargetAllowed && + !options.isResolvedTargetAllowed(resolvedTargetPath)) + ) { return { success: false, reason: 'outsideAllowedRoots',