diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 55177421b..701cc3cc9 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -1,6 +1,7 @@ import { existsSync, readFileSync, writeFileSync, statSync, realpathSync } from 'fs'; import { execFileSync } from 'child_process'; import { join, relative } from 'path'; +import { isPathContained, isResolvedContained } from '../core/path-confine.ts'; import type { BrainEngine } from '../core/engine.ts'; import { DELETE_BATCH_SIZE } from '../core/engine-constants.ts'; import { importFile } from '../core/import-file.ts'; @@ -1112,15 +1113,14 @@ function createSyncBaselineCommit(repoPath: string): void { * #774 NAV-1 TOCTOU: true only if filePath realpath-resolves inside gitRoot. * Guards symlink escape at the per-file level (a committed symlink whose * target lives outside the repo), not just at scope entry. + * + * #3057: delegates to the shared separator-agnostic helper — the previous + * inline `startsWith(rootReal + '/')` was always false on Windows + * (realpathSync returns backslashes), recording every in-repo file as + * SYMLINK_NOT_ALLOWED and freezing the sync bookmark. */ function isPathSafe(filePath: string, gitRoot: string): boolean { - try { - const real = realpathSync(filePath); - const rootReal = realpathSync(gitRoot); - return real === rootReal || real.startsWith(rootReal + '/'); - } catch { - return false; - } + return isPathContained(filePath, gitRoot); } function hasOriginRemote(repoPath: string): boolean { @@ -1887,7 +1887,8 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise = nodePath, +): boolean { + const rel = pathMod.relative(parent, child); + // '' = same path; '..' or '../…' = escapes upward; absolute = different + // root entirely (e.g. another drive on Windows, where relative() returns + // the child verbatim). + return rel === '' || (rel !== '..' && !rel.startsWith('..' + pathMod.sep) && !pathMod.isAbsolute(rel)); +} + /** * Symlink-safe path confinement: realpath BOTH sides, then a separator-aware - * prefix check. A plain `startsWith()` on un-resolved paths would let a + * containment check. A plain `startsWith()` on un-resolved paths would let a * `parent/skills` symlink → `/etc` (or `$GBRAIN_HOME/clones/` → `/etc`) * bypass the boundary; resolving first defeats that. * @@ -41,9 +64,7 @@ export function isPathContained(child: string, parent: string): boolean { } catch { return false; // missing / unresolvable path → not contained } - // Append a separator so /foo doesn't match /foobar. - const parentWithSep = resolvedParent.endsWith('/') ? resolvedParent : resolvedParent + '/'; - return resolvedChild === resolvedParent || resolvedChild.startsWith(parentWithSep); + return isResolvedContained(resolvedChild, resolvedParent); } /** diff --git a/test/path-confine.test.ts b/test/path-confine.test.ts index 960d2f861..a04f70a65 100644 --- a/test/path-confine.test.ts +++ b/test/path-confine.test.ts @@ -15,10 +15,10 @@ import { mkdtempSync, mkdirSync, writeFileSync, rmSync, symlinkSync, chmodSync, lstatSync, type Stats, } from 'fs'; -import { join } from 'path'; +import { join, win32, posix } from 'path'; import { tmpdir } from 'os'; import { - isTrustedDotfile, isPathContained, realpathOrResolve, isWriteTargetContained, + isTrustedDotfile, isPathContained, isResolvedContained, realpathOrResolve, isWriteTargetContained, } from '../src/core/path-confine.ts'; import { validateSlug } from '../src/core/utils.ts'; import { resolveSourceId } from '../src/core/source-resolver.ts'; @@ -149,6 +149,37 @@ describe('isPathContained', () => { }); }); +// #3057: the containment predicate must be separator-agnostic. On Windows, +// realpathSync returns backslash separators, and the old +// `startsWith(parent + '/')` form was false for EVERY in-repo file — sync's +// isPathSafe recorded every file SYMLINK_NOT_ALLOWED and froze the bookmark. +// win32 semantics are pinned here via the injectable path module so this +// regression is caught on POSIX CI. +describe('isResolvedContained — win32 separators (#3057)', () => { + test('in-repo file with backslash separators IS contained', () => { + expect(isResolvedContained('C:\\repo\\notes\\a.md', 'C:\\repo', win32)).toBe(true); + }); + test('root itself is contained', () => { + expect(isResolvedContained('C:\\repo', 'C:\\repo', win32)).toBe(true); + }); + test('upward escape is NOT contained', () => { + expect(isResolvedContained('C:\\other\\x.md', 'C:\\repo', win32)).toBe(false); + expect(isResolvedContained('C:\\', 'C:\\repo', win32)).toBe(false); + }); + test('sibling prefix does not match (C:\\repo vs C:\\repofoo)', () => { + expect(isResolvedContained('C:\\repofoo\\x.md', 'C:\\repo', win32)).toBe(false); + }); + test('different drive is NOT contained', () => { + expect(isResolvedContained('D:\\repo\\x.md', 'C:\\repo', win32)).toBe(false); + }); + test('posix semantics unchanged', () => { + expect(isResolvedContained('/repo/notes/a.md', '/repo', posix)).toBe(true); + expect(isResolvedContained('/repofoo/x.md', '/repo', posix)).toBe(false); + expect(isResolvedContained('/repo', '/repo', posix)).toBe(true); + expect(isResolvedContained('/', '/repo', posix)).toBe(false); + }); +}); + describe('realpathOrResolve', () => { test('resolves a symlink to its real target', () => { const dir = scratch();