fix(session): bound reload file access

[benvinegar]

Summary

• Bound daemon-driven session reloads to the filesystem roots established by the initial Hunk session.
• Reject reload --source, file operands, patch files, and agent-context sidecars that escape those roots, including symlink escapes.
• Add unit, AppHost, and session CLI integration coverage for allowed and rejected reload paths.

Testing

• bun run lint
• bun run typecheck
• bun run format:check
• bun test

This PR description was generated by Pi using GPT-5

[greptile-apps]

Greptile Summary

This PR introduces filesystem-boundary enforcement for daemon-driven session reloads. A new SessionReloadBounds object is computed once at session startup from the initial AppBootstrap, and every subsequent reload request is validated against those roots before any file is read from disk.

• sessionFileBounds.ts: establishes roots from the initial VCS repo root (sourceLabel) or from the containing directories of the initially compared files, deduplicates/normalises them with symlink resolution, then validates every reload path — --source, left/right operands, patch files, and --agent-context sidecars — against those roots. Stdin-backed inputs and symlink escapes are explicitly rejected.
• AppHost.tsx: wires the bounds into the reloadSession callback, calling validateSessionReloadWithinBounds before resolveConfiguredCliInput or loadAppBootstrap, so a rejected reload never touches the filesystem.
• Tests: unit tests in sessionFileBounds.test.ts, an AppHost interaction test, and a CLI integration test all cover both allowed and rejected reload paths, including symlink escapes for existing files.

Confidence Score: 4/5

The core bounds logic is sound and the integration is clean; the main concern is a narrow intermediate-symlink gap when the reload target file does not exist at check time.

The assertReloadPathWithinBounds fallback to a lexical path when realpathSync.native throws ENOENT does not resolve intermediate symlinks in the path, so a crafted reload pointing to a non-existent file via an in-repo symlink passes the check while the actual OS read resolves the symlink to a location outside the allowed roots. The attack requires write access to both the repo and the outside location plus precise timing, which limits practical exposure, but it is a real gap in the symlink protection the PR was specifically designed to provide. Everything else — the normalizeRoots deduplication, VCS-root selection via sourceLabel, stdin blocking, and the agentContext validation — looks correct and well-tested.

src/hunk-session/sessionFileBounds.ts — specifically the resolveMaybeRealPath fallback path inside assertReloadPathWithinBounds.

Security Review

• Intermediate-symlink TOCTOU (src/hunk-session/sessionFileBounds.ts, assertReloadPathWithinBounds): when the final path component of a reload input does not exist on disk, realpathSync.native throws and the fallback returns the lexical (un-resolved) path. This lexical path passes isWithinRoot even though it contains an intermediate symlink pointing outside the session root. If the file is created at the outside location between the bounds check and the loadAppBootstrap read, content outside the root is served. The window is narrow and requires write access to both the repo and the outside location, but it represents a gap in the symlink-escape protection that the PR explicitly introduces.
• No secrets exposure, injection, or auth/authz issues were identified in the remaining changes.

Important Files Changed

Filename	Overview
src/hunk-session/sessionFileBounds.ts	New module establishing filesystem bounds for daemon-driven session reloads; correct core logic but the realpathSync fallback for non-existent files through intermediate symlinks leaves a narrow TOCTOU bypass.
src/ui/AppHost.tsx	Correctly integrates bounds validation into the reload pipeline; bounds are captured once at mount using the registration's cwd, and validateSessionReloadWithinBounds is called before any file loading.
src/hunk-session/sessionFileBounds.test.ts	Comprehensive unit tests covering VCS, diff, patch, difftool, agent-context, stdin, and symlink escape cases; the symlink test only covers existing files, not the non-existent-file TOCTOU case.
test/session/cli.test.ts	Adds a CLI integration test for boundary rejection; moves fixtureB files into fixtureA.dir to comply with new bounds, losing CLI-level coverage for a successful multi-directory reload.
src/ui/AppHost.interactions.test.tsx	Adds an AppHost integration test confirming outside-root reloads are rejected end-to-end; mock factory parameterised with cwd/repoRoot for scenario control.
CHANGELOG.md	CHANGELOG entry added under the correct unreleased Fixed section.

Sequence Diagram

sequenceDiagram
    participant Daemon
    participant AppHost
    participant sessionFileBounds
    participant loadAppBootstrap

    Note over AppHost: Mount: createSessionReloadBounds(bootstrap, cwd)<br/>roots = [repoRoot | parentDirs of initial files]

    Daemon->>AppHost: reload_session(nextInput, sourcePath)
    AppHost->>sessionFileBounds: validateSessionReloadWithinBounds(bounds, nextInput, sourcePath)
    sessionFileBounds->>sessionFileBounds: assertReloadPathWithinBounds(sourcePath)
    sessionFileBounds->>sessionFileBounds: validateCommonReloadOptions (agentContext)
    alt diff / difftool / patch
        sessionFileBounds->>sessionFileBounds: assertReloadPathWithinBounds(left, right / file)
    end
    alt path outside roots
        sessionFileBounds-->>AppHost: throw outside the initial Hunk root
        AppHost-->>Daemon: error response
    else within roots
        sessionFileBounds-->>AppHost: "{ cwd }"
        AppHost->>loadAppBootstrap: "loadAppBootstrap(configuredInput, { cwd })"
        loadAppBootstrap-->>AppHost: nextBootstrap
        AppHost-->>Daemon: success response
    end

Loading

Comments Outside Diff (1)

1. test/session/cli.test.ts, line 199-218 (link)

   Multi-root successful-reload scenario no longer exercised at CLI level

   The original fixtureB was created in a separate temp directory via createFixtureFiles, so the reload tested a diff session switching to files in a different directory. Since diff bounds are directory-level, that scenario now falls outside the allowed root and the test had to be restructured. The unit tests in sessionFileBounds.test.ts do cover the case where left/right initial files come from two different directories and later reloads from either of those roots succeed, so the logic is verified. However, no CLI integration test exercises a successful reload from a directory that is in bounds but different from the primary fixture directory. Adding that coverage would close the gap between the unit and integration layers.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: test/session/cli.test.ts
   Line: 199-218
   
   Comment:
   **Multi-root successful-reload scenario no longer exercised at CLI level**
   
   The original `fixtureB` was created in a separate temp directory via `createFixtureFiles`, so the reload tested a `diff` session switching to files in a different directory. Since `diff` bounds are directory-level, that scenario now falls outside the allowed root and the test had to be restructured. The unit tests in `sessionFileBounds.test.ts` do cover the case where left/right initial files come from two different directories and later reloads from either of those roots succeed, so the logic is verified. However, no CLI integration test exercises a *successful* reload from a directory that is in bounds but different from the primary fixture directory. Adding that coverage would close the gap between the unit and integration layers.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/hunk-session/sessionFileBounds.ts:98-113
**Intermediate-symlink TOCTOU gap for non-existent files**

When the final path component doesn't exist, `realpathSync.native` throws ENOENT *after* having resolved all intermediate symlinks in the path. The catch block then falls back to the raw lexical result of `resolve(cwd, path)`, which still contains the un-expanded symlink. `isWithinRoot` sees a path that is lexically inside the repo root and passes it — but the actual OS read in `loadAppBootstrap` will follow the symlink and access the real location outside the bounds.

Concrete scenario: an agent creates `/repo/evil-link → /outside`, then requests a reload with `left: "/repo/evil-link/new-file.ts"`. If `/outside/new-file.ts` does not exist at check time, the fallback path `/repo/evil-link/new-file.ts` passes the bounds check. If the file is then created before `loadAppBootstrap` reads it, the content is served from outside the allowed root. The existing symlink test covers only the case where the target file already exists.

### Issue 2 of 3
src/hunk-session/sessionFileBounds.ts:59-96
**`diff`/`difftool` bounds are directory-wide, not file-specific**

For a `diff` or `difftool` initial session, roots are set to the *parent directories* of the two compared files. Any file anywhere in those directories is then accessible to daemon-driven reloads. If both comparison files happen to live in a broad directory (e.g., a shared `/tmp` prefix or the user's home directory), subsequent reloads can read any file in that tree. This is probably the intended trade-off to support "load next pair from the same project", but it is worth confirming, given that VCS sessions use the tighter repo-root boundary.

### Issue 3 of 3
test/session/cli.test.ts:199-218
**Multi-root successful-reload scenario no longer exercised at CLI level**

The original `fixtureB` was created in a separate temp directory via `createFixtureFiles`, so the reload tested a `diff` session switching to files in a different directory. Since `diff` bounds are directory-level, that scenario now falls outside the allowed root and the test had to be restructured. The unit tests in `sessionFileBounds.test.ts` do cover the case where left/right initial files come from two different directories and later reloads from either of those roots succeed, so the logic is verified. However, no CLI integration test exercises a *successful* reload from a directory that is in bounds but different from the primary fixture directory. Adding that coverage would close the gap between the unit and integration layers.

_{Reviews (1): Last reviewed commit: "fix(session): bound reload file access" | Re-trigger Greptile}

[greptile-apps]

Intermediate-symlink TOCTOU gap for non-existent files

When the final path component doesn't exist, realpathSync.native throws ENOENT after having resolved all intermediate symlinks in the path. The catch block then falls back to the raw lexical result of resolve(cwd, path), which still contains the un-expanded symlink. isWithinRoot sees a path that is lexically inside the repo root and passes it — but the actual OS read in loadAppBootstrap will follow the symlink and access the real location outside the bounds.

Concrete scenario: an agent creates /repo/evil-link → /outside, then requests a reload with left: "/repo/evil-link/new-file.ts". If /outside/new-file.ts does not exist at check time, the fallback path /repo/evil-link/new-file.ts passes the bounds check. If the file is then created before loadAppBootstrap reads it, the content is served from outside the allowed root. The existing symlink test covers only the case where the target file already exists.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/hunk-session/sessionFileBounds.ts
Line: 98-113

Comment:
**Intermediate-symlink TOCTOU gap for non-existent files**

When the final path component doesn't exist, `realpathSync.native` throws ENOENT *after* having resolved all intermediate symlinks in the path. The catch block then falls back to the raw lexical result of `resolve(cwd, path)`, which still contains the un-expanded symlink. `isWithinRoot` sees a path that is lexically inside the repo root and passes it — but the actual OS read in `loadAppBootstrap` will follow the symlink and access the real location outside the bounds.

Concrete scenario: an agent creates `/repo/evil-link → /outside`, then requests a reload with `left: "/repo/evil-link/new-file.ts"`. If `/outside/new-file.ts` does not exist at check time, the fallback path `/repo/evil-link/new-file.ts` passes the bounds check. If the file is then created before `loadAppBootstrap` reads it, the content is served from outside the allowed root. The existing symlink test covers only the case where the target file already exists.

How can I resolve this? If you propose a fix, please make it concise.

[benvinegar]

Fixed. The resolver now walks up to the nearest existing ancestor and realpaths that ancestor before appending missing path segments, so non-existent leaves behind an intermediate symlink are rejected too. I also added regression coverage for the missing-target symlink case.

This comment was generated by Pi using GPT-5

[greptile-apps]

diff/difftool bounds are directory-wide, not file-specific

For a diff or difftool initial session, roots are set to the parent directories of the two compared files. Any file anywhere in those directories is then accessible to daemon-driven reloads. If both comparison files happen to live in a broad directory (e.g., a shared /tmp prefix or the user's home directory), subsequent reloads can read any file in that tree. This is probably the intended trade-off to support "load next pair from the same project", but it is worth confirming, given that VCS sessions use the tighter repo-root boundary.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/hunk-session/sessionFileBounds.ts
Line: 59-96

Comment:
**`diff`/`difftool` bounds are directory-wide, not file-specific**

For a `diff` or `difftool` initial session, roots are set to the *parent directories* of the two compared files. Any file anywhere in those directories is then accessible to daemon-driven reloads. If both comparison files happen to live in a broad directory (e.g., a shared `/tmp` prefix or the user's home directory), subsequent reloads can read any file in that tree. This is probably the intended trade-off to support "load next pair from the same project", but it is worth confirming, given that VCS sessions use the tighter repo-root boundary.

How can I resolve this? If you propose a fix, please make it concise.

[benvinegar]

Good call. I tightened file-scoped diff/difftool sessions to exact initially loaded files instead of parent-directory access, and added tests that reject same-directory sibling reloads. VCS sessions still use repo-root bounds.

This comment was generated by Pi using GPT-5

[benvinegar]

Correction after maintainer clarification: same-parent reloads for non-repo file sessions are intentional, so I preserved that behavior. The latest commit uses parent-directory bounds for non-repo file pairs, and upgrades file-pair sessions launched from inside a repo to repo-root bounds so they can reload/show other repo files safely.

This comment was generated by Pi using GPT-5