Просмотр исходного кода

fix(resolution): keep the existence probe inside the project root (#1631)

Fixes #1631.

Rebased contributor PR #1632 onto main (post-#1749). Lexical containment for `fileExists` filesystem fallback via `lexicalPathWithinRoot`; #935 in-root symlink behaviour preserved.
Max Hsu 7 часов назад
Родитель
Сommit
a6f52d737a
4 измененных файлов с 100 добавлено и 6 удалено
  1. 2 0
      CHANGELOG.md
  2. 64 0
      __tests__/resolution-fileexists-containment.test.ts
  3. 13 2
      src/resolution/index.ts
  4. 21 4
      src/utils.ts

+ 2 - 0
CHANGELOG.md

@@ -139,6 +139,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 
 - **A second `codegraph serve --mcp` on the same project no longer silently kills auto-sync (#1740).** Direct mode (`CODEGRAPH_NO_DAEMON=1` or proxy→in-process fallback) now takes an exclusive `.codegraph/writer.pid` lock; a second writer exits immediately with guidance to stop the other server or unset `CODEGRAPH_NO_DAEMON` so clients share the daemon. The shared daemon already multiplexes N clients onto one watcher — this closes the same-OS dual-direct gap the docs warned about for Windows/WSL but did not guard.
 - **A second `codegraph serve --mcp` on the same project no longer silently kills auto-sync (#1740).** Direct mode (`CODEGRAPH_NO_DAEMON=1` or proxy→in-process fallback) now takes an exclusive `.codegraph/writer.pid` lock; a second writer exits immediately with guidance to stop the other server or unset `CODEGRAPH_NO_DAEMON` so clients share the daemon. The shared daemon already multiplexes N clients onto one watcher — this closes the same-OS dual-direct gap the docs warned about for Windows/WSL but did not guard.
 
 
+- Indexing no longer checks whether files outside your project exist. A relative import that points above the project directory (`../../something`) made CodeGraph probe that location on disk while resolving it. Nothing outside the project was ever read, and no such file was ever added to the index or linked to, but the check itself should not have happened — such an import now simply resolves to nothing. Symlinks inside your project that point at code kept elsewhere are unaffected and still index as before. Thanks @ErQrYfkrju. (#1631)
+
 #### Screens, links and navigation
 #### Screens, links and navigation
 
 
 - **Where the app goes after login is a fork, not two always-es.** A navigation whose destination comes back from a helper — `router.replace(await resolvePostLoginRoute())` over `return (await hasSeenWelcome(…)) ? '/home/' : '/welcome/'` — drew both screens with no condition, reading as if the welcome screen always shows. The two arms share a line, and only a column can tell them apart; each synthesized edge now carries its literal's own position, so the guard reader says which arm it is: `WHEN await hasSeenWelcome(…)` → home, and its negation → welcome. And the scan starts at the helper's body, so a literal-union return type — `Promise<'/welcome/' | '/home/'>`, whose routes are string literals too, written first — no longer stands in for the navigation itself. Re-index after upgrading to pick the positions up.
 - **Where the app goes after login is a fork, not two always-es.** A navigation whose destination comes back from a helper — `router.replace(await resolvePostLoginRoute())` over `return (await hasSeenWelcome(…)) ? '/home/' : '/welcome/'` — drew both screens with no condition, reading as if the welcome screen always shows. The two arms share a line, and only a column can tell them apart; each synthesized edge now carries its literal's own position, so the guard reader says which arm it is: `WHEN await hasSeenWelcome(…)` → home, and its negation → welcome. And the scan starts at the helper's body, so a literal-union return type — `Promise<'/welcome/' | '/home/'>`, whose routes are string literals too, written first — no longer stands in for the navigation itself. Re-index after upgrading to pick the positions up.

+ 64 - 0
__tests__/resolution-fileexists-containment.test.ts

@@ -0,0 +1,64 @@
+/**
+ * `fileExists` must not probe outside the project root (#1631).
+ *
+ * `resolveRelativeImport` hands this callback paths built with
+ * `path.relative(projectRoot, basePath)`, which can carry `../` segments, and
+ * `path.join` does not clamp — so a crafted relative import in an indexed file
+ * made the resolver stat arbitrary absolute paths. Nothing outside is read (the
+ * content sinks are guarded separately, #527) and no edge is produced, but the
+ * probe itself is an existence oracle driven by repository content.
+ */
+
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import * as fs from 'fs';
+import * as os from 'os';
+import * as path from 'path';
+import { ReferenceResolver } from '../src/resolution';
+import type { QueryBuilder } from '../src/db/queries';
+
+describe('fileExists containment (#1631)', () => {
+  let sandbox: string;
+  let projectRoot: string;
+
+  /** The resolver only needs a project root here — `fileExists` never queries. */
+  const contextFor = (root: string) =>
+    new ReferenceResolver(root, {} as unknown as QueryBuilder).getResolutionContext();
+
+  beforeEach(() => {
+    sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-test-'));
+    projectRoot = path.join(sandbox, 'proj');
+    fs.mkdirSync(path.join(projectRoot, 'src'), { recursive: true });
+    fs.writeFileSync(path.join(projectRoot, 'src', 'a.js'), 'export const a = 1;');
+    // A real file two levels above the root, as the reproduction in #1631 has.
+    fs.mkdirSync(path.join(sandbox, 'outside'), { recursive: true });
+    fs.writeFileSync(path.join(sandbox, 'outside', 'secret.js'), 'export const secret = 42;');
+  });
+
+  afterEach(() => {
+    fs.rmSync(sandbox, { recursive: true, force: true });
+  });
+
+  it('still reports files inside the root', () => {
+    expect(contextFor(projectRoot).fileExists('src/a.js')).toBe(true);
+    expect(contextFor(projectRoot).fileExists('src/missing.js')).toBe(false);
+  });
+
+  it('refuses to probe a path that escapes the root, even though it exists', () => {
+    const escaping = path.join('..', 'outside', 'secret.js');
+    // Baseline: the target really is there — so `false` can only come from the guard.
+    expect(fs.existsSync(path.join(projectRoot, escaping))).toBe(true);
+
+    expect(contextFor(projectRoot).fileExists(escaping)).toBe(false);
+  });
+
+  it('keeps following an in-root symlink whose target is outside the root (#935)', () => {
+    const link = path.join(projectRoot, 'vendor');
+    try {
+      fs.symlinkSync(path.join(sandbox, 'outside'), link, 'dir');
+    } catch {
+      return; // symlink creation not permitted (e.g. Windows without privilege)
+    }
+    // Lexically inside the root, physically outside — the indexing tier allows this.
+    expect(contextFor(projectRoot).fileExists(path.join('vendor', 'secret.js'))).toBe(true);
+  });
+});

+ 13 - 2
src/resolution/index.ts

@@ -26,6 +26,7 @@ import { loadProjectAliases, type AliasMap } from './path-aliases';
 import { loadGoModule, type GoModule } from './go-module';
 import { loadGoModule, type GoModule } from './go-module';
 import { loadWorkspacePackages, type WorkspacePackages } from './workspace-packages';
 import { loadWorkspacePackages, type WorkspacePackages } from './workspace-packages';
 import { logDebug } from '../errors';
 import { logDebug } from '../errors';
+import { lexicalPathWithinRoot } from '../utils';
 import type { ReExport } from './types';
 import type { ReExport } from './types';
 import { LRUCache } from './lru-cache';
 import { LRUCache } from './lru-cache';
 
 
@@ -538,8 +539,18 @@ export class ReferenceResolver {
             return true;
             return true;
           }
           }
         }
         }
-        // Fall back to filesystem for files not yet indexed
-        const fullPath = path.join(this.projectRoot, filePath);
+        // Fall back to filesystem for files not yet indexed. `path.join` does
+        // not clamp, and relative-import resolution hands us paths carrying
+        // `../` segments, so the probe has to be contained (#1631): a path
+        // outside the root can never be an indexed project file, and the
+        // `knownFiles` check above already answered for everything that is.
+        // Lexical containment only: this is a per-candidate hot path, and the
+        // symlink half of `validatePathWithinRoot` costs two `realpathSync`
+        // calls per probe (~70x slower here). It would also be wrong to apply
+        // — indexing deliberately follows in-root symlinks whose targets live
+        // outside the root (#935), so only the `../` escape is refused.
+        const fullPath = lexicalPathWithinRoot(this.projectRoot, filePath);
+        if (fullPath === null) return false;
         try {
         try {
           return fs.existsSync(fullPath);
           return fs.existsSync(fullPath);
         } catch (error) {
         } catch (error) {

+ 21 - 4
src/utils.ts

@@ -80,6 +80,24 @@ function isWithinDir(child: string, parent: string): boolean {
   return c === p || c.startsWith(p + path.sep);
   return c === p || c.startsWith(p + path.sep);
 }
 }
 
 
+/**
+ * The lexical half of {@link validatePathWithinRoot}, on its own.
+ *
+ * Returns the resolved absolute path when `filePath` stays inside
+ * `projectRoot` after `../` segments are applied, or null when it escapes.
+ * No filesystem access — for callers on a hot path that only need to refuse a
+ * lexical escape, and for which the realpath half would be both unnecessary
+ * and far too expensive (the existence probe in resolution's `fileExists`,
+ * #1631: two `realpathSync` calls per probe made it ~70x slower).
+ *
+ * This is NOT a substitute for `validatePathWithinRoot` on any path whose
+ * contents get served — those must keep the symlink-aware check (#527).
+ */
+export function lexicalPathWithinRoot(projectRoot: string, filePath: string): string | null {
+  const resolved = path.resolve(projectRoot, filePath);
+  return isWithinDir(resolved, path.resolve(projectRoot)) ? resolved : null;
+}
+
 /**
 /**
  * Validate that a file path stays within the project root, resolving symlinks.
  * Validate that a file path stays within the project root, resolving symlinks.
  *
  *
@@ -112,14 +130,13 @@ export function validatePathWithinRoot(
   filePath: string,
   filePath: string,
   options?: { allowSymlinkEscape?: boolean }
   options?: { allowSymlinkEscape?: boolean }
 ): string | null {
 ): string | null {
-  const resolved = path.resolve(projectRoot, filePath);
-  const normalizedRoot = path.resolve(projectRoot);
-
   // 1. Lexical containment — cheap, catches `../` traversal. Applies even on
   // 1. Lexical containment — cheap, catches `../` traversal. Applies even on
   //    the indexing read path: a crafted `../` escape is still rejected.
   //    the indexing read path: a crafted `../` escape is still rejected.
-  if (!isWithinDir(resolved, normalizedRoot)) {
+  const resolved = lexicalPathWithinRoot(projectRoot, filePath);
+  if (resolved === null) {
     return null;
     return null;
   }
   }
+  const normalizedRoot = path.resolve(projectRoot);
 
 
   // 2. Symlink-aware containment — resolve symlinks on both sides and re-check,
   // 2. Symlink-aware containment — resolve symlinks on both sides and re-check,
   //    so an in-repo symlink whose real target escapes the root is rejected.
   //    so an in-repo symlink whose real target escapes the root is rejected.