fix(daemon): reject '.', '..' and empty session names so session artifacts stay under sessions/ - #1815
Merged
Merged
Conversation
…onDir safeSessionName only rewrites characters outside [a-zA-Z0-9._-], so the names '.' and '..' survive unchanged and path.join resolves them to the sessions dir itself or its parent, the daemon state dir. A remote caller's --session .. would then land app.log / runner.log / requests/*.ndjson outside the sessions tree. SessionStore.resolveSessionDir is the one place a session name becomes a directory (AGENTS.md: session artifact paths come from session-store), so it now refuses such a name with INVALID_ARGS. Every request goes through it first thing in createRequestExecutionScope, before any artifact path is used, so this is also the admission-time rejection; every other caller passes an already admitted name. isSafeSessionSegment mirrors the predicate PR #1814 adds for its request-diagnostics route; whichever lands second takes the trivial merge. Regression tests were proven red against the pre-fix code: resolveSessionDir returned the sessions dir / state dir for '.', '..', '' and the request scope resolved runnerLogPath to <stateDir>/runner.log.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
added
the
ready-for-human
Member
Author
|
Clean review at |
|
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
`safeSessionName` only rewrites characters outside `[a-zA-Z0-9._-]`, so the names `.` and `..` survive unchanged. `SessionStore.resolveSessionDir` then does `path.join(sessionsDir, '..')` → the daemon state dir, and `app.log` / `runner.log` / `requests/*.ndjson` are written there. For a remote daemon this lets a caller steer daemon-side artifact writes outside the sessions tree with `--session ..`.
Fix — at the owning interface
`SessionStore.resolveSessionDir` is the one place a session name becomes a directory (AGENTS.md: session artifact paths come from `session-store.ts`), so the invariant "every session dir lies beneath `sessionsDir`" is enforced there: an unsafe name (`.`, `..`, empty after `safeSessionName`) throws `INVALID_ARGS`.
Why not (only) at `scopeRequestSession`: `createRequestExecutionScope` calls `resolveSessionDir` first thing after resolving the effective name — before any artifact path is used — so the store-level check is the admission-time rejection, and the request finalizes as an `INVALID_ARGS` response like the existing tenant-id rejection does. Every other caller (`ensureSessionDir`, app-log/event-log/record paths, inventory, doctor, teardown) receives an already admitted name, so nothing else changes. Session state is in-memory only, so no on-disk record can carry an unsafe name across the upgrade.
Checked for existing behavior depending on relative names: none — no code, test, or docs use `--session .` / `..`; `defaultTracePath`, the `.ad` writer, metro-session hints and claim/recovery all build file names under a fixed dir (a `..-.trace.log` file is harmless).
Overlap with #1814
`isSafeSessionSegment` is the same predicate #1814 adds for its request-diagnostics route (which deliberately left `resolveSessionDir` alone). Whichever lands second takes a one-hunk merge in `session-paths.ts`.
Tests (proven red first)
`pnpm check:affected --run` green (336 files / 2834 tests, format, layering, fallow, build).