From c2357ec5347960a01aec0af4a95c024ad680570e Mon Sep 17 00:00:00 2001 From: Anso Date: Mon, 25 May 2026 01:30:00 -0400 Subject: [PATCH] fix(stack-files): symlink-aware delete and chmod (#1214) deleteStackPath now lstats the leaf and unlinks the link entry itself when it is a symbolic link, so the file the user clicked on in the tree is what gets removed (the linked target stays intact). chmodStackPath rejects with LINK_CHMOD_UNSUPPORTED on a symlink rather than silently mutating the target's permissions; Node's lchmod is macOS-only and following the link is the bug being fixed here. Path-component symlinks are still resolved via the existing resolveSafeStackPath, so a symlinked parent that escapes the stack dir still surfaces SYMLINK_ESCAPE before the leaf is inspected. Service-level tests cover delete on internal-target / external-target / broken / dir-target symlinks, chmod rejection on symlinks (including the broken case), and non-symlink regression checks. Route-level tests pin the 409 LINK_CHMOD_UNSUPPORTED mapping and the link-only-delete behaviour. The describe blocks are platform-gated; Windows symlink creation needs admin/developer-mode and is skipped along with the existing SYMLINK_ESCAPE test. Docs updated to describe both behaviours in plain product terms. --- .../__tests__/filesystem-stack-paths.test.ts | 113 ++++++++++++++++++ .../src/__tests__/stack-files-routes.test.ts | 51 ++++++++ backend/src/routes/stacks.ts | 6 +- backend/src/services/FileSystemService.ts | 57 ++++++++- docs/features/stack-file-explorer.mdx | 4 +- 5 files changed, 222 insertions(+), 9 deletions(-) diff --git a/backend/src/__tests__/filesystem-stack-paths.test.ts b/backend/src/__tests__/filesystem-stack-paths.test.ts index 6ab7d3d8..6cd1d73f 100644 --- a/backend/src/__tests__/filesystem-stack-paths.test.ts +++ b/backend/src/__tests__/filesystem-stack-paths.test.ts @@ -441,4 +441,117 @@ describe('FileSystemService stack methods', () => { await expect(service.readStackFile(STACK, 'escape-link')).rejects.toMatchObject({ code: 'SYMLINK_ESCAPE' }); }); }); + + // ── symlink semantics ──────────────────────────────────────────────────── + // Symlink creation requires admin/developer-mode on Windows; skip the + // whole block there to avoid spurious EPERM failures unrelated to the + // behaviour being tested. + + describe.skipIf(isWindows)('symlink semantics (Linux/macOS only)', () => { + it('delete on a symlink removes the link entry and leaves the target intact', async () => { + const targetPath = path.join(stackDir, 'target.txt'); + const linkPath = path.join(stackDir, 'link.txt'); + await fs.writeFile(targetPath, 'payload'); + await fs.symlink(targetPath, linkPath); + + const service = FileSystemService.getInstance(); + await service.deleteStackPath(STACK, 'link.txt'); + + await expect(fs.lstat(linkPath)).rejects.toMatchObject({ code: 'ENOENT' }); + const targetContent = await fs.readFile(targetPath, 'utf-8'); + expect(targetContent).toBe('payload'); + }); + + it('delete on a symlink that points outside the stack removes only the link entry', async () => { + const externalFile = path.join(tmpBase, 'outside.txt'); + await fs.writeFile(externalFile, 'external'); + const linkPath = path.join(stackDir, 'escape-link'); + await fs.symlink(externalFile, linkPath); + + const service = FileSystemService.getInstance(); + await service.deleteStackPath(STACK, 'escape-link'); + + await expect(fs.lstat(linkPath)).rejects.toMatchObject({ code: 'ENOENT' }); + const externalContent = await fs.readFile(externalFile, 'utf-8'); + expect(externalContent).toBe('external'); + }); + + it('chmod on a symlink rejects with LINK_CHMOD_UNSUPPORTED and leaves the target mode unchanged', async () => { + const targetPath = path.join(stackDir, 'target.txt'); + await fs.writeFile(targetPath, 'payload'); + await fs.chmod(targetPath, 0o644); + await fs.symlink(targetPath, path.join(stackDir, 'link.txt')); + + const service = FileSystemService.getInstance(); + await expect(service.chmodStackPath(STACK, 'link.txt', 0o600)).rejects.toMatchObject({ + code: 'LINK_CHMOD_UNSUPPORTED', + }); + + const stat = await fs.stat(targetPath); + expect(stat.mode & 0o777).toBe(0o644); + }); + + it('chmod on a regular file still succeeds (symlink branch does not regress non-symlink paths)', async () => { + const filePath = path.join(stackDir, 'plain.txt'); + await fs.writeFile(filePath, 'data'); + await fs.chmod(filePath, 0o644); + + const service = FileSystemService.getInstance(); + await service.chmodStackPath(STACK, 'plain.txt', 0o600); + + const stat = await fs.stat(filePath); + expect(stat.mode & 0o777).toBe(0o600); + }); + + it('delete on a regular file still succeeds (symlink branch does not regress non-symlink paths)', async () => { + const filePath = path.join(stackDir, 'plain.txt'); + await fs.writeFile(filePath, 'data'); + + const service = FileSystemService.getInstance(); + await service.deleteStackPath(STACK, 'plain.txt'); + + await expect(fs.access(filePath)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('delete on a broken symlink (target already removed) removes the dangling link entry', async () => { + const targetPath = path.join(stackDir, 'gone.txt'); + const linkPath = path.join(stackDir, 'broken-link.txt'); + await fs.writeFile(targetPath, ''); + await fs.symlink(targetPath, linkPath); + await fs.unlink(targetPath); + + const service = FileSystemService.getInstance(); + await service.deleteStackPath(STACK, 'broken-link.txt'); + + await expect(fs.lstat(linkPath)).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('chmod on a broken symlink rejects with LINK_CHMOD_UNSUPPORTED rather than ENOENT', async () => { + const targetPath = path.join(stackDir, 'gone-chmod.txt'); + const linkPath = path.join(stackDir, 'broken-link-chmod.txt'); + await fs.writeFile(targetPath, ''); + await fs.symlink(targetPath, linkPath); + await fs.unlink(targetPath); + + const service = FileSystemService.getInstance(); + await expect(service.chmodStackPath(STACK, 'broken-link-chmod.txt', 0o600)).rejects.toMatchObject({ + code: 'LINK_CHMOD_UNSUPPORTED', + }); + }); + + it('delete on a symlink to a directory removes only the link entry, not the target directory', async () => { + const targetDir = path.join(stackDir, 'real-dir'); + const linkPath = path.join(stackDir, 'link-to-dir'); + await fs.mkdir(targetDir); + await fs.writeFile(path.join(targetDir, 'kept.txt'), 'preserve'); + await fs.symlink(targetDir, linkPath); + + const service = FileSystemService.getInstance(); + await service.deleteStackPath(STACK, 'link-to-dir'); + + await expect(fs.lstat(linkPath)).rejects.toMatchObject({ code: 'ENOENT' }); + const kept = await fs.readFile(path.join(targetDir, 'kept.txt'), 'utf-8'); + expect(kept).toBe('preserve'); + }); + }); }); diff --git a/backend/src/__tests__/stack-files-routes.test.ts b/backend/src/__tests__/stack-files-routes.test.ts index 7a5122e0..1c81ae85 100644 --- a/backend/src/__tests__/stack-files-routes.test.ts +++ b/backend/src/__tests__/stack-files-routes.test.ts @@ -942,3 +942,54 @@ describe('protected stack files', () => { if (res.status === 409) expect(res.body.code).toBe('PROTECTED_FILE'); }); }); + +// ── symlink semantics ──────────────────────────────────────────────────────── +// Symlink creation requires admin/developer-mode on Windows; skip on that +// platform so the suite stays green where the OS denies the setup itself. + +describe.skipIf(isWindows)('symlink semantics (Linux/macOS only)', () => { + it('PUT /files/permissions returns 409 LINK_CHMOD_UNSUPPORTED on a symlink', async () => { + const targetPath = path.join(stacksDir, STACK, 'symlink-target.txt'); + const linkPath = path.join(stacksDir, STACK, 'symlink-link.txt'); + await fs.writeFile(targetPath, 'payload'); + await fs.chmod(targetPath, 0o644); + await fs.symlink(targetPath, linkPath); + + try { + const res = await request(app) + .put(`/api/stacks/${STACK}/files/permissions`) + .query({ path: 'symlink-link.txt' }) + .set('Cookie', adminCookie) + .send({ mode: 0o600 }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('LINK_CHMOD_UNSUPPORTED'); + + const stat = await fs.stat(targetPath); + expect(stat.mode & 0o777).toBe(0o644); + } finally { + await fs.unlink(linkPath).catch(() => {}); + await fs.unlink(targetPath).catch(() => {}); + } + }); + + it('DELETE /files removes a symlink and leaves the target intact', async () => { + const targetPath = path.join(stacksDir, STACK, 'sym-delete-target.txt'); + const linkPath = path.join(stacksDir, STACK, 'sym-delete-link.txt'); + await fs.writeFile(targetPath, 'survives'); + await fs.symlink(targetPath, linkPath); + + try { + const res = await request(app) + .delete(`/api/stacks/${STACK}/files`) + .query({ path: 'sym-delete-link.txt' }) + .set('Cookie', adminCookie); + expect(res.status).toBe(204); + + await expect(fs.lstat(linkPath)).rejects.toMatchObject({ code: 'ENOENT' }); + const targetContent = await fs.readFile(targetPath, 'utf-8'); + expect(targetContent).toBe('survives'); + } finally { + await fs.unlink(targetPath).catch(() => {}); + } + }); +}); diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index 6c17d3f1..4eef3bb3 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -1312,7 +1312,8 @@ type FsErrorCode = | 'ALREADY_EXISTS' | 'FILE_EXISTS' | 'DIR_EXISTS' - | 'PROTECTED_FILE'; + | 'PROTECTED_FILE' + | 'LINK_CHMOD_UNSUPPORTED'; function sendFsError( res: Response, @@ -1333,6 +1334,9 @@ function sendFsError( if (e.code === 'PROTECTED_FILE') { return res.status(409).json({ error: e.message, code: 'PROTECTED_FILE' satisfies FsErrorCode }); } + if (e.code === 'LINK_CHMOD_UNSUPPORTED') { + return res.status(409).json({ error: e.message, code: 'LINK_CHMOD_UNSUPPORTED' satisfies FsErrorCode }); + } if (e.code === 'EEXIST') { return res.status(409).json({ error: e.message, code: 'ALREADY_EXISTS' satisfies FsErrorCode }); } diff --git a/backend/src/services/FileSystemService.ts b/backend/src/services/FileSystemService.ts index b89fee32..90a82732 100644 --- a/backend/src/services/FileSystemService.ts +++ b/backend/src/services/FileSystemService.ts @@ -877,21 +877,53 @@ export class FileSystemService { return { ok: true, mtimeMs: newStat.mtimeMs }; } + /** + * Like resolveSafeStackPath but does NOT follow a symlink at the leaf. + * Path-component symlinks are still resolved and validated (so a symlinked + * parent that escapes the stack dir still throws SYMLINK_ESCAPE), but the + * final entry stays as the link path the user sees in the tree. Callers + * use this to act on the link entry itself (unlink the link, not the + * target) for operations where following would mutate a file other than + * the one the user clicked on. + */ + private async resolveSafeStackLeafPath(stackName: string, relPath: string): Promise { + if (relPath === '' || relPath === '.') { + return this.resolveSafeStackPath(stackName, ''); + } + const parentRel = path.dirname(relPath); + const baseName = path.basename(relPath); + if (!baseName || baseName === '.' || baseName === '..') { + throw Object.assign(new Error('Invalid path'), { code: 'INVALID_PATH' }); + } + const safeParent = await this.resolveSafeStackPath(stackName, parentRel === '.' ? '' : parentRel); + return path.join(safeParent, baseName); + } + async deleteStackPath(stackName: string, relPath: string, recursive: boolean = false): Promise { if (isProtectedRelPath(relPath)) throw protectedFileError(relPath); - const safePath = await this.resolveSafeStackPath(stackName, relPath); + const leafPath = await this.resolveSafeStackLeafPath(stackName, relPath); + + // Branch on whether the leaf is a symlink BEFORE following it. Deleting + // a symlink should remove the link entry the user clicked on; following + // through to the target would silently delete a file with a different + // name and leave the link entry dangling. + const leafStat = await fsPromises.lstat(leafPath); + if (leafStat.isSymbolicLink()) { + await fsPromises.unlink(leafPath); + return; + } if (recursive) { - await fsPromises.rm(safePath, { recursive: true, force: true }); + await fsPromises.rm(leafPath, { recursive: true, force: true }); return; } try { - await fsPromises.unlink(safePath); + await fsPromises.unlink(leafPath); } catch (err: unknown) { const e = err as NodeJS.ErrnoException; if (e.code === 'EISDIR') { try { - await fsPromises.rmdir(safePath); + await fsPromises.rmdir(leafPath); } catch (inner: unknown) { const ie = inner as NodeJS.ErrnoException; if (ie.code === 'ENOTEMPTY' || ie.code === 'EEXIST') { @@ -946,8 +978,21 @@ export class FileSystemService { throw Object.assign(new Error('Invalid permission bits'), { code: 'INVALID_PATH' }); } if (isProtectedRelPath(relPath)) throw protectedFileError(relPath); - const safePath = await this.resolveSafeStackPath(stackName, relPath); - await fsPromises.chmod(safePath, mode); + const leafPath = await this.resolveSafeStackLeafPath(stackName, relPath); + + // chmod on a symlink is rejected. Following the link would silently + // mutate permissions on a file with a different name than the entry the + // user clicked on. Node's fsPromises.lchmod is macOS-only, so for the + // common Linux/Windows case there is no safe in-place alternative; we + // surface a clear error so the user edits the target file directly. + const leafStat = await fsPromises.lstat(leafPath); + if (leafStat.isSymbolicLink()) { + throw Object.assign( + new Error('Cannot change permissions of a symlink. Edit the target file directly.'), + { code: 'LINK_CHMOD_UNSUPPORTED' as const }, + ); + } + await fsPromises.chmod(leafPath, mode); } async statStackEntry(stackName: string, relPath: string): Promise { diff --git a/docs/features/stack-file-explorer.mdx b/docs/features/stack-file-explorer.mdx index 8c9ddd30..0ed35993 100644 --- a/docs/features/stack-file-explorer.mdx +++ b/docs/features/stack-file-explorer.mdx @@ -35,7 +35,7 @@ The Files tab splits into two panes. The left pane holds the upload affordance, Folders sort before files, and entries within each group sort alphabetically. -Click a folder to expand or collapse it. Click a file to open it in the viewer on the right. Symlinks render with a chain icon and behave like files when clicked. +Click a folder to expand or collapse it. Click a file to open it in the viewer on the right. Symlinks render with a chain icon and behave like files when clicked. Deleting a symlink removes only the link entry; the file it points to is untouched. **Display cap.** Each directory render is capped at 500 entries. A folder with more than 500 children shows the first 500 alphabetically and appends `Showing 500 of N - refine in shell` at the bottom of the list. For directories with more entries, work from a host shell. @@ -127,7 +127,7 @@ Right-click any file and choose **Permissions** to inspect or edit its Unix mode When your account has stack edit permission, the toggles are interactive and the footer adds **Save**. For viewer accounts the dialog opens read-only: the toggles render the current state and the footer shows only **Close**. - Permissions are applied with `chmod`. Symlinks may not honour the change depending on the host kernel. + Permissions are applied with `chmod`. Permission changes on symlinks are not supported; edit the target file's permissions directly. ## Deleting