From 1c82e3e1d4351fa5bb4ca4ef53f5577a725fd625 Mon Sep 17 00:00:00 2001 From: Anso Date: Fri, 26 Jun 2026 09:23:50 -0400 Subject: [PATCH] fix: contain file-explorer writes and browse reachable out-of-base binds (#1465) The file-explorer editor save resolved a dangling symlink leaf to the link path and wrote through it with a plain writeFile, which followed the link and created a file outside the bind/stack root. Reject a resolved leaf that is itself a symlink (mirroring the managed-stack guard) and promote the save through the atomic stage-and-rename helper, so the editor save matches its documented atomicity and can never leave a partial file or land outside the root. Bind-root discovery reported every source outside the compose base as unreachable without probing it, so a config directory mounted into both the app and the Sencho container was wrongly non-browsable. Probe the declared source as Sencho actually sees it; dangerous host roots, docker-socket mounts, and managed-area overlaps stay blocked, and the dangerous classification also reads the literal declared source so it holds across platforms. --- .../filesystem-symlink-escape.test.ts | 58 +++++++++++++++++ .../stack-file-roots-service.test.ts | 43 ++++++++++++ backend/src/services/FileSystemService.ts | 22 ++++++- backend/src/services/StackFileRootsService.ts | 65 ++++++++++--------- 4 files changed, 156 insertions(+), 32 deletions(-) diff --git a/backend/src/__tests__/filesystem-symlink-escape.test.ts b/backend/src/__tests__/filesystem-symlink-escape.test.ts index abf1606d..1ac0761f 100644 --- a/backend/src/__tests__/filesystem-symlink-escape.test.ts +++ b/backend/src/__tests__/filesystem-symlink-escape.test.ts @@ -218,6 +218,64 @@ describe.skipIf(isWindows)('FileSystemService symlink-escape: dangling (broken) }); }); +// ── Scoped file-explorer writes (bind roots) must reject a dangling symlink ── +// leaf the same way the legacy managed-stack path does: the editor save sink +// (writeStackFileIfUnchanged) resolves through resolveSafePathWithin, which +// must not hand back a link path a follow-on write would traverse out of the +// root. The escape only triggers on first save (no expected mtime). +describe.skipIf(isWindows)('FileSystemService scoped-root: dangling symlink leaf write', () => { + let tmpBase: string; + let composeDir: string; + let rootDir: string; + let externalTarget: string; + + beforeEach(async () => { + tmpBase = await fs.mkdtemp(path.join(os.tmpdir(), 'sencho-scopedangle-')); + composeDir = path.join(tmpBase, 'compose'); + rootDir = path.join(tmpBase, 'bindroot'); + externalTarget = path.join(tmpBase, 'outside'); + await fs.mkdir(composeDir, { recursive: true }); + await fs.mkdir(rootDir, { recursive: true }); + // The escape target dir exists; only the leaf file is missing, so the leaf + // symlink is dangling (realpath ENOENT) but a writeFile would create it. + await fs.mkdir(externalTarget, { recursive: true }); + mockState.composeDir = composeDir; + }); + + afterEach(async () => { + await fs.rm(tmpBase, { recursive: true, force: true }); + }); + + it('rejects a first-save through a dangling symlink leaf and writes nothing outside the root', async () => { + await fs.symlink(path.join(externalTarget, 'pwned.conf'), path.join(rootDir, 'leak.conf')); + const svc = FileSystemService.getInstance(); + await expect( + svc.writeStackFileIfUnchanged(STACK, 'leak.conf', 'PWNED=1\n', null, { rootAbsDir: rootDir }), + ).rejects.toMatchObject({ code: 'SYMLINK_ESCAPE' }); + await expect(fs.access(path.join(externalTarget, 'pwned.conf'))).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('rejects a scoped read through a dangling symlink leaf (shared resolver guard)', async () => { + await fs.symlink(path.join(externalTarget, 'secret.conf'), path.join(rootDir, 'peek.conf')); + const svc = FileSystemService.getInstance(); + await expect( + svc.readStackFile(STACK, 'peek.conf', undefined, { scope: { rootAbsDir: rootDir } }), + ).rejects.toMatchObject({ code: 'SYMLINK_ESCAPE' }); + }); + + it('writes a normal scoped file atomically (no leftover staging file)', async () => { + const svc = FileSystemService.getInstance(); + const res = await svc.writeStackFileIfUnchanged(STACK, 'app.conf', 'key=value\n', null, { rootAbsDir: rootDir }); + expect(res.ok).toBe(true); + expect(await fs.readFile(path.join(rootDir, 'app.conf'), 'utf-8')).toBe('key=value\n'); + // The atomic helper stages into a sibling .sencho-tmp-* file and renames it + // away; nothing transient should remain in the root after a clean write. + const entries = await fs.readdir(rootDir); + expect(entries.filter((e) => e.includes('sencho-tmp'))).toHaveLength(0); + expect(entries).toEqual(['app.conf']); + }); +}); + // ── The compose root itself is a symlink: must NOT be a false positive ─────── describe.skipIf(isWindows)('FileSystemService symlink-escape: symlinked compose root is allowed', () => { let tmpBase: string; diff --git a/backend/src/__tests__/stack-file-roots-service.test.ts b/backend/src/__tests__/stack-file-roots-service.test.ts index 05fe4dc0..76197f42 100644 --- a/backend/src/__tests__/stack-file-roots-service.test.ts +++ b/backend/src/__tests__/stack-file-roots-service.test.ts @@ -117,6 +117,35 @@ describe('StackFileRootsService.listRoots', () => { expect(bind?.browsable).toBe(false); }); + it('classifies a reachable absolute bind outside the compose base as browsable and writable', async () => { + // A config directory mounted into both the app and the Sencho container can + // legitimately live outside the compose base. When Sencho can stat it, the + // root must be fully browsable/editable, not silently dropped as unreachable. + const outside = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), 'sfr-ext-'))); + try { + stub({ rendered: renderModel({ web: [{ type: 'bind', source: outside, target: '/config', read_only: false }] }) }); + const roots = await StackFileRootsService.getInstance(1).listRoots(STACK, { fresh: true }); + const bind = roots.find((r) => r.kind === 'bind'); + expect(bind?.accessible).toBe(true); + expect(bind?.browsable).toBe(true); + expect(bind?.writable).toBe(true); + expect(bind?.managedSourceOverlap).toBe(false); + expect(bind?.hostPathOrName).toBe(outside); + } finally { + await fs.rm(outside, { recursive: true, force: true }).catch(() => {}); + } + }); + + it('marks an unreachable absolute bind outside the compose base as non-browsable', async () => { + const outsideMissing = path.join(os.tmpdir(), 'sfr-ext-absent-xyz-12345'); + stub({ rendered: renderModel({ web: [{ type: 'bind', source: outsideMissing, target: '/config' }] }) }); + const roots = await StackFileRootsService.getInstance(1).listRoots(STACK, { fresh: true }); + const bind = roots.find((r) => r.kind === 'bind'); + expect(bind?.accessible).toBe(false); + expect(bind?.browsable).toBe(false); + expect(bind?.warning).toBeTruthy(); + }); + it('blocks a dangerous host bind (/etc) and never exposes it as browsable', async () => { stub({ rendered: renderModel({ web: [{ type: 'bind', source: '/etc', target: '/host-etc' }] }) }); const roots = await StackFileRootsService.getInstance(1).listRoots(STACK, { fresh: true }); @@ -125,6 +154,20 @@ describe('StackFileRootsService.listRoots', () => { expect(bind?.browsable).toBe(false); }); + it('blocks a dangerous declared source even when realpath rewrites it to a benign canonical', async () => { + // Guards the dangerousSource term: realpath can rewrite a dangerous POSIX + // source to a benign-looking canonical (a non-existent POSIX path resolves + // drive-prefixed on a non-Linux host), so isDangerousHostPath(canonical) + // alone would miss it. The classification must also read the literal source. + vi.spyOn(fs, 'realpath').mockResolvedValue('/srv/benign-canonical' as never); + vi.spyOn(fs, 'stat').mockResolvedValue({ isDirectory: () => true } as never); + stub({ rendered: renderModel({ web: [{ type: 'bind', source: '/etc', target: '/host-etc' }] }) }); + const roots = await StackFileRootsService.getInstance(1).listRoots(STACK, { fresh: true }); + const bind = roots.find((r) => r.kind === 'bind'); + expect(bind?.dangerous).toBe(true); + expect(bind?.browsable).toBe(false); + }); + it('folds a bind equal to the stack dir into stack-source (no second editable root)', async () => { stub({ rendered: renderModel({ web: [{ type: 'bind', source: stackDir, target: '/app' }] }) }); const roots = await StackFileRootsService.getInstance(1).listRoots(STACK, { fresh: true }); diff --git a/backend/src/services/FileSystemService.ts b/backend/src/services/FileSystemService.ts index 640039eb..a09f8e28 100644 --- a/backend/src/services/FileSystemService.ts +++ b/backend/src/services/FileSystemService.ts @@ -1376,6 +1376,22 @@ export class FileSystemService { throw Object.assign(new Error('Symlink escapes root directory'), { code: 'SYMLINK_ESCAPE' }); } + // A dangling symlink leaf realpath()s to ENOENT (its target is missing), so + // the reattach branch above rebuilds the path with the link name intact and + // it passes the lexical containment check. lstat does not follow the final + // component, so it tells a not-yet-created file (ENOENT, allowed) apart from + // a dangling link (a symlink, rejected) that a follow-on writeFile/readFile + // would traverse out of the root. Mirrors assertRealWithinBase's guard. + let leafIsSymlink = false; + try { + leafIsSymlink = (await fsPromises.lstat(realTarget)).isSymbolicLink(); + } catch (err: unknown) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + } + if (leafIsSymlink) { + throw Object.assign(new Error('Symlink escapes root directory'), { code: 'SYMLINK_ESCAPE' }); + } + return realTarget; } @@ -1696,7 +1712,11 @@ export class FileSystemService { } } - await fsPromises.writeFile(safePath, content, 'utf-8'); + // Promote through the atomic stage-and-rename helper so a crash or write + // failure leaves the previous target intact rather than a truncated file. + // rename also replaces a (rejected upstream) symlink leaf in place instead + // of following it, so the write can never land outside the root. + await this.writeStackFileAtomic(safePath, content); const newStat = await fsPromises.stat(safePath); return { ok: true, mtimeMs: newStat.mtimeMs }; } diff --git a/backend/src/services/StackFileRootsService.ts b/backend/src/services/StackFileRootsService.ts index 0b08b617..48f5838e 100644 --- a/backend/src/services/StackFileRootsService.ts +++ b/backend/src/services/StackFileRootsService.ts @@ -34,6 +34,24 @@ export interface RootMount { readOnly: boolean; } +/** All declarations resolving to one canonical bind source, with its probe result. */ +interface BindGroup { + canonical: string; + accessible: boolean; + isDir: boolean; + dockerSock: boolean; + /** + * Set when any raw declared source mapping to this canonical is a dangerous + * host root. Tracked from the declared source (a POSIX container path that + * stays literal across platforms), not the canonical, so that buildBindRoot's + * combined dangerous check (canonical OR this) still blocks a bind like /etc + * even where realpath rewrites the source (a non-existent POSIX path resolves + * drive-prefixed on a Windows dev box). + */ + dangerousSource: boolean; + mounts: RootMount[]; +} + export interface StackFileRoot { /** Opaque, server-minted id. The resolved path/name lives in metadata, never in the id. */ id: string; @@ -117,27 +135,22 @@ export function isDangerousHostPath(p: string): boolean { } /** - * Browse-accessibility probe for a bind root, scoped to the compose base dir. - * The realpath/stat run ONLY for a source that lexically resolves inside - * `baseDir`: in the containerized deployment that is the only host area the - * Sencho process can reach, so a source outside it is unreachable anyway and is - * reported non-accessible without touching the filesystem. The containment is - * an INLINE path.resolve + startsWith guard at each filesystem sink (the - * realpath of a within-base symlink can still escape, so the resolved canonical - * is re-checked before stat). Returns the canonical realpath so the route can - * pass it to FileSystemService as the containment root. + * Browse-accessibility probe for a declared bind source. realpath + stat the + * source as Sencho actually sees it: a path Sencho cannot reach (not mounted + * into the container, or absent on the host) surfaces ENOENT and degrades to + * non-accessible. Bind sources are declared by the stack's own compose file and + * may legitimately live outside the compose base (a config directory mounted + * into both the app and the Sencho container), so accessibility is decided by + * whether the resolved path is stat-able, not by where it sits. Safety against + * privileged or managed paths is the caller's job: buildBindRoot still blocks + * dangerous host roots, docker-socket mounts, and overlaps with Sencho's managed + * areas. Returns the canonical realpath the route uses as the containment root + * for file operations on this bind. */ export async function probeBindRootAccess( absPath: string, - baseDir: string, ): Promise<{ canonical: string; accessible: boolean; isDir: boolean }> { - const base = path.resolve(baseDir); const resolved = path.resolve(absPath); - // Out-of-base sources keep their original path for the dangerous/overlap - // classification the caller does, but are never statted. - if (resolved !== base && !resolved.startsWith(base + path.sep)) { - return { canonical: absPath, accessible: false, isDir: false }; - } // A missing path (ENOENT) is the common, expected "not reachable" outcome and // is left silent; any other code (e.g. EACCES on a path that exists but Sencho // cannot read) is logged so an operator chasing "why can't I browse this bind" @@ -155,11 +168,6 @@ export async function probeBindRootAccess( logNonEnoent('realpath', err); return { canonical: resolved, accessible: false, isDir: false }; } - // A within-base source can be a symlink whose target escapes the base; re-check - // the resolved canonical inline before the stat sink. - if (canonical !== base && !canonical.startsWith(base + path.sep)) { - return { canonical, accessible: false, isDir: false }; - } try { const st = await fsPromises.stat(canonical); return { canonical, accessible: true, isDir: st.isDirectory() }; @@ -299,13 +307,6 @@ export class StackFileRootsService { baseDir: string, stackDir: string, ): Promise { - interface BindGroup { - canonical: string; - accessible: boolean; - isDir: boolean; - dockerSock: boolean; - mounts: RootMount[]; - } const probeByRaw = new Map(); const bindByCanonical = new Map(); const volByName = new Map(); @@ -317,7 +318,7 @@ export class StackFileRootsService { const rawAbs = path.isAbsolute(m.source) ? m.source : path.resolve(stackDir, m.source); let probe = probeByRaw.get(rawAbs); if (!probe) { - probe = await probeBindRootAccess(rawAbs, baseDir); + probe = await probeBindRootAccess(rawAbs); probeByRaw.set(rawAbs, probe); } let group = bindByCanonical.get(probe.canonical); @@ -327,11 +328,13 @@ export class StackFileRootsService { accessible: probe.accessible, isDir: probe.isDir, dockerSock: false, + dangerousSource: false, mounts: [], }; bindByCanonical.set(probe.canonical, group); } group.mounts.push(mount); + if (isDangerousHostPath(rawAbs)) group.dangerousSource = true; if (isDockerSocketMount({ source: m.source, target: m.target })) group.dockerSock = true; } else if (m.type === 'named' && m.source) { const resolvedName = model.volumes[m.source]?.name ?? m.source; @@ -358,7 +361,7 @@ export class StackFileRootsService { } private buildBindRoot( - group: { canonical: string; accessible: boolean; isDir: boolean; dockerSock: boolean; mounts: RootMount[] }, + group: BindGroup, baseDir: string, stackDir: string, ): StackFileRoot | null { @@ -377,7 +380,7 @@ export class StackFileRootsService { const dataDir = resolveDataDir(); const overlapsManaged = (dir: string): boolean => isPathWithinBase(canonical, dir) || isPathWithinBase(dir, canonical); const overlap = !inStack && (overlapsManaged(baseDir) || overlapsManaged(dataDir)); - const dangerous = isDangerousHostPath(canonical) || group.dockerSock; + const dangerous = isDangerousHostPath(canonical) || group.dangerousSource || group.dockerSock; const readonly = group.mounts.every((m) => m.readOnly); const isFile = group.accessible && !group.isDir;