fix(lsp): re-check physical containment after server startup, right before didOpen (#1268)

docs/plans/2026-09-27-lsp-open-containment.md

@@ -0,0 +1,34 @@
+# LSP open: re-check physical containment at the moment of use (#1268)
+
+**Gap.** `authorizeContext` (`src/main/ipc/lsp.ts`) validates the physical target, then `LspManager.openDocumentNow` awaits server startup, which can take seconds on a cold spawn. Only after that does it build a lexical `file://` URI and send `didOpen`. If a directory on the path is swapped for a symlink to an outside directory during startup, the server is handed a URI that resolves outside the root.
+
+**Fix.**
+- **Manager:** `OpenDocumentParams.assertPhysicalTarget`, a callback from the authorizing caller. The manager awaits it inside the per-document queue, after server startup and immediately before a NEW server document's `didOpen`. A refusal fails open (returns false: no LSP for this document) and names nothing to the server.
+- **IPC:** both open paths (`lsp:open-document`, `lsp:reopen-document`) pass `lspPhysicalTargetAssertion(context)`. It re-runs the same rule: `resolveInsideRoot` + `validateExistingTarget` (no symlink, canonical inside the root) + regular file + an unchanged relative location.
+- **Why a callback and not a filesystem check in the manager:** the IPC layer owns authorization for both editor roots and AI Workspace entries, and the manager's unit tests run on fake roots.
+
+**Tests.**
+- **Real filesystem:** the reviewer's probe. Authorize `src/a.ts`, then swap `src` for a symlink to an outside directory: refused. A leaf that became a symlink: refused. An untouched file: passes. A virtual document: nothing to check.
+- **Manager:**
+  - the re-check runs after `initialized` and before `didOpen`;
+  - a refusal returns false with no notification;
+  - a pass opens normally.
+- **Mutations killed:** no re-check in the manager; no physical validation in the assertion.
+
+## After review a of #1412
+- **Shared joins:** the re-check runs at the top of the queued step for EVERY open, including one that joins an existing shared document (another alias of the same file). Before, only a new document's `didOpen` was guarded, so a join after a swap sent `didChange` for the escaped URI.
+- **Virtual documents:** they are named under `root/.agent-code-lsp`, so that directory must not be a symlink out of the root. They now get a re-check too.
+- **No exact relative-path comparison:** a case-only rename on a case-insensitive filesystem still resolves inside the root, and refusing it only lost LSP. Containment plus a regular file is the property.
+
+**Residuals.**
+- **One await:** the window between the re-check and the notification is one await, inherent to any path-based open.
+- **A swap after a document is already open** (a later `didChange`, or a document request, for a URI the server already holds) is not guarded. The server already has that URI, and no per-change path check stops it from reading the path later. This issue is the authorization-to-use window of an OPEN.
+- **The IPC wiring** of the callback is not covered by a test, because the handlers need Electron.
+
+## After review b (1cb8b3cf)
+Every assertion first checks that the canonical root still resolves to itself (`assertRootUnchanged`). A pathless document's check used to return early when the virtual directory was missing, without checking the root. Review b's other findings are the path-based LSP limit; B6 (owner proxy) accepted this PR as narrowing the window, `Refs #1268`.
+
+## After review c
+- The virtual branch now checks the LEAF `didOpen` names (`virtual-<hash>.<ext>`, from `lspVirtualDocumentName`, shared with the manager). It must be absent, or a regular file inside the root. A leaf symlink created in advance escaped with no timing window. A pathless open without its leaf name is refused.
+- The regular-file re-check is pinned: an authorized file that became a directory is refused.
+- The IPC wiring of the callback still has no committed test. A wiring test is feasible with the existing Electron mock; it is left out under the freeze and stated as a residual.

src/main/ipc/lsp.ts

@@ -1,17 +1,86 @@
 import { ipcMain, type WebContents } from 'electron'
-import { lstat } from 'fs/promises'
-import { relative } from 'path'
+import { lstat, realpath } from 'fs/promises'
+import { join, relative } from 'path'
 
 import type { AiWorkspaceRegistry } from '@main/aiWorkspace/AiWorkspaceRegistry.js'
 import { resolveInsideRoot, validateExistingTarget } from '@main/ipc/editorFs.js'
 import type { EditorFsRootRegistry } from '@main/ipc/editorFsRootRegistry.js'
+import { LSP_VIRTUAL_DIR, lspVirtualDocumentName } from '@main/lspManager.js'
 import type { LspManager } from '@main/lspManager.js'
 import type {
   LspCompletionContext,
   LspDocumentAuthorization,
   LspPosition,
 } from '@shared/types/lsp.js'
 
+/**
+ * The manager re-runs the physical check that authorizeContext ran, at the
+ * moment of use, after server startup (#1268): the path must still resolve,
+ * without symlinks, to a regular file inside the root.
+ *
+ * WHY no "same relative path" check (review a of #1412): on a case-
+ * insensitive filesystem a case-only rename still resolves, and realpath
+ * returns the new spelling, so an exact comparison refused a legitimate open.
+ * Containment is the property that matters.
+ *
+ * A pathless (virtual) document still names a file to the server:
+ * `root/.agent-code-lsp/virtual-<hash>.<ext>` (makeVirtualServerUri). That
+ * directory must not be a symlink out of the root.
+ */
+/**
+ * The authorized root must still BE the root (#1412 review b): renamed away and
+ * replaced by a symlink out, every lexical path under it names outside
+ * content. `workspaceRoot` is the canonical path `roots.authorize` returned,
+ * so it must still resolve to exactly itself.
+ */
+async function assertRootUnchanged(workspaceRoot: string): Promise<void> {
+  if ((await realpath(workspaceRoot)) !== workspaceRoot) throw new Error('LSP workspace root no longer resolves to the authorized root')
+}
+
+export function lspPhysicalTargetAssertion(context: { workspaceRoot: string; filePath: string | null; virtualName?: string }): () => Promise<void> {
+  const { workspaceRoot, filePath, virtualName } = context
+  if (filePath === null) {
+    return async () => {
+      // The leaf name is required (#1412 review c): without it the check
+      // cannot see what didOpen will name, so the open is refused.
+      if (!virtualName) throw new Error('LSP virtual document has no leaf name to check')
+      // Checked FIRST: a missing virtual directory below proves nothing about
+      // where the root now points (review b, finding 2).
+      await assertRootUnchanged(workspaceRoot)
+      const directory = join(workspaceRoot, LSP_VIRTUAL_DIR)
+      let entry
+      try {
+        entry = await lstat(directory)
+      } catch (error) {
+        if ((error as NodeJS.ErrnoException).code === 'ENOENT') return
+        throw error
+      }
+      if (entry.isSymbolicLink()) throw new Error('LSP virtual document directory is a symbolic link')
+      await validateExistingTarget(workspaceRoot, directory)
+      // The LEAF didOpen names (#1412 review c): a symlink created there in
+      // advance (its hash is steerable through the renderer's clientUri)
+      // resolved outside the root with no timing window at all. It must be
+      // absent, or a regular file inside the root.
+      const leaf = join(directory, virtualName)
+      let leafEntry
+      try {
+        leafEntry = await lstat(leaf)
+      } catch (error) {
+        if ((error as NodeJS.ErrnoException).code === 'ENOENT') return
+        throw error
+      }
+      if (leafEntry.isSymbolicLink() || !leafEntry.isFile()) throw new Error('LSP virtual document path is not a regular file')
+      await validateExistingTarget(workspaceRoot, leaf)
+    }
+  }
+  return async () => {
+    await assertRootUnchanged(workspaceRoot)
+    const requested = resolveInsideRoot(workspaceRoot, filePath)
+    const physical = await validateExistingTarget(workspaceRoot, requested)
+    if (!(await lstat(physical)).isFile()) throw new Error('LSP document is not a file')
+  }
+}
+
 // LSP-backed code intelligence for Monaco surfaces.
 //
 // The renderer's CodeBlock component opens a document per visible
@@ -287,6 +356,7 @@ export function registerLspIpc(
               language: params.language,
               workspaceRoot: context.workspaceRoot,
               filePath: context.filePath,
+              assertPhysicalTarget: lspPhysicalTargetAssertion({ ...context, virtualName: lspVirtualDocumentName(params.clientUri, params.language) }),
             })
             // A page that left meanwhile does not get the marker back: clear()
             // dropped it, and the close clear() queued behind this entry will
@@ -402,6 +472,7 @@ export function registerLspIpc(
               language: params.language,
               workspaceRoot: context.workspaceRoot,
               filePath: context.filePath,
+              assertPhysicalTarget: lspPhysicalTargetAssertion({ ...context, virtualName: lspVirtualDocumentName(params.clientUri, params.language) }),
             })
             if (!ok) break
           }

src/main/ipc/lspPhysicalTarget.test.ts

@@ -0,0 +1,117 @@
+import { mkdir, mkdtemp, realpath, rename, rm, symlink, writeFile } from 'node:fs/promises'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { afterEach, expect, it, vi } from 'vitest'
+
+vi.mock('electron', () => ({ ipcMain: { handle: vi.fn(), on: vi.fn() } }))
+
+const { lspPhysicalTargetAssertion } = await import('./lsp.js')
+const VIRTUAL = 'virtual-1a2b3c.ts'
+
+// #1268 review A's real-filesystem probe: authorize `src/a.ts` inside the root,
+// then swap `src` for a symlink to an outside directory holding its own
+// `a.ts`. The lexical path is unchanged, but it now names an escaped file.
+let base = ''
+afterEach(async () => { if (base) await rm(base, { recursive: true, force: true }) })
+
+async function layout() {
+  base = await realpath(await mkdtemp(join(tmpdir(), 'lsp-toctou-')))
+  const root = join(base, 'root')
+  const outside = join(base, 'outside')
+  await mkdir(join(root, 'src'), { recursive: true })
+  await mkdir(outside, { recursive: true })
+  await writeFile(join(root, 'src', 'a.ts'), 'export const inside = 1\n')
+  await writeFile(join(outside, 'a.ts'), 'export const secret = 1\n')
+  return { root, outside }
+}
+
+it('passes while the authorized file is still inside the root', async () => {
+  const { root } = await layout()
+  await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })()).resolves.toBeUndefined()
+})
+
+it('refuses once a directory on the path was swapped for a symlink outside the root', async () => {
+  const { root, outside } = await layout()
+  const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })
+  await rename(join(root, 'src'), join(root, 'src-moved'))
+  await symlink(outside, join(root, 'src'))
+  await expect(assertion()).rejects.toThrow(/escapes project root/)
+})
+
+it('refuses a leaf that became a symlink, even to a file inside the root', async () => {
+  const { root } = await layout()
+  const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })
+  await writeFile(join(root, 'src', 'b.ts'), 'export const other = 1\n')
+  await rm(join(root, 'src', 'a.ts'))
+  await symlink(join(root, 'src', 'b.ts'), join(root, 'src', 'a.ts'))
+  await expect(assertion()).rejects.toThrow(/symbolic links/)
+})
+
+// Review a of #1412: a virtual document is named under root/.agent-code-lsp,
+// so that directory must not be a symlink out of the root.
+it('refuses a virtual document whose directory is a symlink out of the root', async () => {
+  const { root, outside } = await layout()
+  await symlink(outside, join(root, '.agent-code-lsp'))
+  await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null, virtualName: VIRTUAL })()).rejects.toThrow(/symbolic link/)
+})
+
+it('passes a virtual document with no directory yet, or a real one', async () => {
+  const { root } = await layout()
+  await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null, virtualName: VIRTUAL })()).resolves.toBeUndefined()
+  await mkdir(join(root, '.agent-code-lsp'))
+  await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null, virtualName: VIRTUAL })()).resolves.toBeUndefined()
+})
+
+// Review a: a case-only rename on a case-insensitive filesystem still resolves
+// to the same file inside the root, and must not be refused.
+it('accepts a case-only rename of the authorized file', async () => {
+  const { root } = await layout()
+  const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })
+  await rename(join(root, 'src', 'a.ts'), join(root, 'src', 'A.ts'))
+  const caseInsensitive = await realpath(join(root, 'src', 'a.ts')).then(() => true, () => false)
+  if (caseInsensitive) await expect(assertion()).resolves.toBeUndefined()
+  else await expect(assertion()).rejects.toThrow()
+})
+
+// #1412 review b (finding 2): the ROOT itself swapped for a symlink out, with
+// no virtual-document directory at the target. The virtual branch returned
+// early on ENOENT without checking the root, and didOpen then named
+// `root/.agent-code-lsp/…`, which resolves outside. Every assertion now first
+// checks that the root still resolves to itself.
+it('refuses a pathless document when the root was replaced by a symlink out', async () => {
+  const { root, outside } = await layout()
+  const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null, virtualName: VIRTUAL })
+  await rename(root, join(base, 'root-moved'))
+  await symlink(outside, root)
+  await expect(assertion()).rejects.toThrow(/root/)
+})
+
+// #1412 review c: the virtual branch checked `.agent-code-lsp/` but not the
+// `virtual-<hash>.<ext>` leaf didOpen NAMES. A leaf symlink created in advance
+// (the hash of a renderer-chosen clientUri is steerable) resolved outside with
+// no timing window at all. The leaf must be absent or a regular, contained
+// file, and a pathless open without its leaf name is refused (fail closed).
+it('refuses a virtual document whose named leaf is a symlink out, or not a file', async () => {
+  const { root, outside } = await layout()
+  await mkdir(join(root, '.agent-code-lsp'))
+  await symlink(join(outside, 'a.ts'), join(root, '.agent-code-lsp', VIRTUAL))
+  await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null, virtualName: VIRTUAL })()).rejects.toThrow()
+  await rm(join(root, '.agent-code-lsp', VIRTUAL))
+  await mkdir(join(root, '.agent-code-lsp', VIRTUAL))
+  await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null, virtualName: VIRTUAL })()).rejects.toThrow()
+  await rm(join(root, '.agent-code-lsp', VIRTUAL), { recursive: true })
+  await writeFile(join(root, '.agent-code-lsp', VIRTUAL), '')
+  await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null, virtualName: VIRTUAL })()).resolves.toBeUndefined()
+  await expect(lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: null })()).rejects.toThrow()
+})
+
+// #1412 review c (minor): the regular-file re-check had no test. An authorized
+// file replaced by a directory before didOpen is refused.
+it('refuses an authorized file that became a directory', async () => {
+  const { root } = await layout()
+  const assertion = lspPhysicalTargetAssertion({ workspaceRoot: root, filePath: join('src', 'a.ts') })
+  await rm(join(root, 'src', 'a.ts'))
+  await mkdir(join(root, 'src', 'a.ts'))
+  await expect(assertion()).rejects.toThrow(/not a file/)
+})
+