(sessions): trust a transcript cwd only if it encodes back to its folder (#385) - #419
Conversation
|
Reviewing |
|
Adversarial review at |
A sandboxed session can forge a transcript in its own project folder whose cwd points below the project. The cwd fed the sidebar project path, the resume and fork spawn directory, the sandbox project folder and the schedule registration, so a schedule planted there could run outside the sandbox. verifiedTranscriptCwd accepts a cwd only when it encodes back to the folder holding the transcript. deriveProjectPath, the cold-start scan worker, resolveSessionRealCwd and the reuse of a stored cache_meta project path go through it; remote hosts keep their recorded cwd. The schedule seed uses the same function. Closes #385
9220cbe to
422deea
Compare
|
Reviewing |
devsuitup
left a comment
There was a problem hiding this comment.
Adversarial review at 422deea: changes requested. (B1, reproduced) remap-project rewrites cwds to newPath inside folder enc(oldPath), so nothing verifies and the remapped project vanishes or stays missing. The fix is a folder-to-path record written only by main. (M1) buildProjectsFromCache trusts a stored cache_meta.projectPath unverified, so a value forged before the upgrade survives until the folder changes. Minor: no warning when a folder has cwds but none verifies, so a CLI naming change would hide every project silently; long paths with mixed separators. Checked: no worktree regression, since resume only reads the first cwd, as on main. Chains A and B are closed, and no other raw-cwd consumer was found.
The remap dialog rewrites every transcript cwd of enc(oldPath) to the new path while the folder keeps its name, so the verified-cwd rule rejected every line and the project vanished. Main now records folder -> newPath in the projectRemaps setting before rewriting, and a cwd equal to that record verifies. Stored state from before the upgrade is verified too: cache_meta in the sidebar build and in reconcile, and cached rows whose projectPath differs from the folder's are rewritten. A folder with no verifiable transcript loses its cached rows and logs one warning naming the folder and the first rejected cwd. Closes #385
|
Reviewing |
devsuitup
left a comment
There was a problem hiding this comment.
Re-review at ccc03d6 (delta from 422deea): 0 blocking. Remap works again. Only main writes projectRemaps, and the sandbox has no bind for the settings DB. The newPath comes from the native picker. A cwd is accepted only if it equals the record exactly. The new row deletion is cache-only and local-only. A file with no cwd yet does not trigger it, since one verified transcript keeps the folder. Reconcile converges after one refresh (no re-index every launch), and the warning fires once per folder per process. Each of 5 mutations turns exactly its target test red. Being fixed: forged rows in the cold or interrupted-scan path are not purged by writeScannedFolder(null), and the logged cwd is not escaped.
A cold or interrupted scan wrote nothing for a local folder with no verified project path, so a cached row from before the upgrade could still be served and opened by session restore. The scan now deletes that folder's cached rows and search entries, as refreshFolder does. The rejected cwd in the warning is logged through JSON.stringify so a newline in it cannot forge a log line. Closes #385
devsuitup
left a comment
There was a problem hiding this comment.
Re-review at 52486c9 (delta from ccc03d6): 0 blocking. The cold or interrupted scan now purges the cached rows and search entries of a local folder whose result is null. Remote mirrors are excluded (FOLDER_PREFIX). The test drives the real worker and goes red without the delete. The rejected cwd is logged JSON-quoted, and a test checks that a newline inside it no longer forges a line.
Closes #385
Threat
A sandboxed session in project P can write its own transcript folder
~/.claude/projects/<enc(P)>/and P's subtree, and no other encoded folder. It forges a JSONL inenc(P)withcwd: P/eviland plantsP/evil/.claude/commands/schedule-x.md.deriveProjectPathreturned the first JSONLcwdwithout checking it, soP/evilbecame the sidebar project path. From there it became the resume/fork spawn directory (and the sandbox'sSWITCHBOARD_SANDBOX_PROJECT_FOLDER, whichmkdir -psenc(P/evil)), the target of the schedule creator'smkdir, and a schedule-registry entry via the plain launch registration inopen-terminal. With the sandbox chosen per launch and P itself unsandboxed, the planted schedule then ran outside the sandbox.Rule
A transcript
cwdis trusted for a filesystem decision only if it encodes back to the name of the folder holding the transcript. One helper,verifiedTranscriptCwd(cwd, folderName)inencode-project-path.js: absolute cwd,path.resolve,encodeProjectPath(resolved) === folderName, elsenull. The schedule seed now calls it too (same comparison as before; a relative recorded path is now refused instead of resolved against the process cwd).Consumers of a transcript cwd
deriveProjectPath(sidebar,session.projectPath,cache_meta,getKnownProjectPaths)nullworkers/scan-projects.js(cold-start scan, writescache_meta)refreshFolderreuse of a storedcache_meta.projectPathresolveSessionRealCwd: resume/fork spawn cwd, Changes panel, panel terminal, terminal path links, subagent worktree discovery, sandbox bind folder (follows the spawn cwd)create-schedule-sessionmkdir enc(projectPath);open-terminalregistrationprojectPathcomes from the sidebar; the registration is unchanged from main (still plain, no folder precondition)remap-project(main.js, nowproject-remap.js)folder -> newPathin theprojectRemapssetting before rewriting, and the rule also accepts a cwd equal to that record. No existence checkcache_meta,session_cacherows)refreshFolder,buildProjectsFromCacheandreconcileCacheFromFilesystemverify a stored path and re-derive; a row with a differentprojectPathis rewritten even if its file is unchanged; a folder with nothing verifiable loses its rowsremote-index.js, remote folders)deriveProjectPath(..., { remote: true })projectPathstrings, a session-restore state saved before the upgradeResidual (not fixed)
encodeProjectPathtruncates at 200 characters and appends a 32-bit hash, so a path of 200+ characters can collide (enc(P/long) === enc(P)). The seed has the same property. A transcript the CLI wrote whose cwd does not encode to its folder (a symlinked cwd named by its real path, say) no longer yields a project path or a resume directory; not observed, not measured.Tests
New
test/transcript-cwd-trust.test.js(15 tests), all red before the change (10 of 13 at first run; the helper andstoredProjectPathMatchesFoldertests were added with the code and checked by mutation): helper accept/refuse/../relative, forged+genuine and forged-onlyderiveProjectPath, forged subagent transcript, worktree collapse kept, remote kept,resolveSessionRealCwdforged/worktree/shadowed id, chain A (schedule creation from a forged folder: project isnull, notP/evil), chain B (resume of a forged session does not spawn inP/evil, the sidebar project isP), stored-meta replacement inrefreshFolder, first launch of a normal project (source check that the registration line inopen-terminalis unconditional).test/scan-projects-worker.test.jsgains a forged-folder case.Existing fixtures that named a folder arbitrarily were changed to
encodeProjectPath(cwd): derive-project-path, session-cache-refresh/bridge-dedup/cold-start-progress, build-projects-cold-scan-fallback, scan-projects-worker, remote-scan-file-granularity. They exercised the old unverified behaviour.Mutations (each makes the new tests red, then restored)
deriveProjectPathtrusts any cwd: 5 red (chains A and B among them)deriveProjectPathdrops the remote bypass: 1 red (remote kept)resolveSessionRealCwdreturns the raw cwd: 3 red; returns null on first hit instead of continuing: 1 redrefreshFolder: 1 red;storedProjectPathMatchesFolderalways true: 2 redisAbsolute: 1 red (needed an added relative-cwd case, since the first version survived); dropspath.resolve: 1 redremote: truealways: red; worker without the remote option: remote-indexing-e2e redReview round 2
remapProjectTranscriptsinproject-remap.jsrecords the new path, then rewrites;verifiedTranscriptCwdaccepts a cwd equal to the recorded value for that folder. Test drives the real rewrite,deriveProjectPath,refreshFolderand a coldbuildProjectsFromCache; a second test refuses a cwd that differs from the recorded one.buildProjectsFromCacheandreconcileCacheFromFilesystem; rows fixed through the dirty rule inrefreshFolder. Tested.[session-cache]warning per folder with the folder name and the first rejected cwd, inrefreshFolder,buildProjectsFromCacheand the cold scan. Tested for once-per-folder.buildProjectsFromCache; they are fixed by the refresh above, so untilreconcileCacheFromFilesystemhas run a stale forged row can still be listed.Review round 3
writeScannedFolderdeletes that folder's cached rows and search entries, asrefreshFolderdoes, so a forged pre-upgrade row is not served before the scan ends. Test runs the real worker; mutation (drop the delete) is red.JSON.stringify, so a newline cannot forge a log line. Test uses a cwd with a newline; mutation (raw cwd) is red.Not verified
main.jsis exercised throughresolveSessionRealCwdonly.task check: lint 0 errors; the test run shows one failure,viewer-file-watch(known Windows libuv assertion). A first run also failed twogit-changes-runner-real-gittests, which pass alone (31/31) and on the primary checkout, and passed on the second run.Changelog: none (
no-changeloglabel). In normal use nothing a user sees changes; the only observable effect is on a transcript whose cwd does not match its folder.