Repository navigation
fix(studio-server): keep the desktop app's hand-off and chat history out of the file routes - #5113
Conversation
…out of the file routes The file and preview routes reach any path inside the project. The desktop app keeps two files there that hold the person's agent session and their chat: .hyperframes/agent-handoff.json and .hyperframes/app-history.jsonl. No route now reads, writes, renames onto, duplicates or uploads over them, by any spelling or through a link. The rest of .hyperframes/ (Studio's motion file, prepared GIF assets) is served as before.
…utes too The app records every chat run, the person's words included, in .hyperframes/agent-runs.jsonl. Only the app's main process reads it.
Edit accuracy: accurate 2055 (base branch 2055), smooth 1625 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
The direct-read, preview and symlink guards work, and their tests fail without them. But the claim that routes "never read or write the hand-off and chat records" doesn't hold yet.
Blocking
- A record is missing from the list.
safePath.ts:34-38leaves out.hyperframes/agent-handoff-read.json, the read cursor #2928 writes. The file routes can still read and overwrite it. #5114's.hyperframes/app-history-seen.jsonisn't listed either. - Renaming the folder escapes the guard.
isPrivateProjectFilematches exact file paths only. The rename route (files.ts:3403-3425) accepts.hyperframesitself. AfterPATCH .../files/.hyperframes {newPath: "x"},x/app-history.jsonlreads through the file routes. Matching.hyperframes/as a prefix for the private names (or denying a rename of.hyperframesor any folder above the records) would close it. - The project listing still shows the records.
GET /projects/:id(projects.ts:49-55) returnswalkDirunfiltered, andwalkDironly ignores.hyperframes/backup, so the record paths are still listed. Filter them with the same predicate.
Non-blocking
- The upload test sends a
.jsonlpayload that media validation may reject on its own, so it may pass even without the guard. Use a file type uploads accept, so the private-path guard is the only reason for the 403.
— Jerrai
jrusso1020
left a comment
There was a problem hiding this comment.
Request changes at 23b76be7. Wrapping the shared resolveWithinProject is the right place: every route that resolves a request path now passes through one check, and the tests cover GET, PUT, DELETE, PATCH-rename, duplicate, upload and both preview routes. This review adds to Jerrai's (missing files, renaming the folder, the listing).
Blocker: make this an allow-list for .hyperframes/, not a list of three names
- Probe on this head through
registerFileRoutes:GET .hyperframes/agent-handoff-read.jsonreturns 200, and so do.hyperframes/app-history-seen.jsonand.hyperframes/share.json. All three were written by the desktop app or CLI. The first holds the samesessionIdthis PR hides inagent-handoff.json. - Every new record file will need another entry, and the sibling changes already add two. The holistic shape is the reverse: Studio's own entries under
.hyperframes/are a small, known set, and every other path under.hyperframes/is refused. That set isstudio-motion.json,studio-manual-edits.json,prepared-assets/, and whichever ofbackup/,preview/,history-idandhf-ids-stamped.jsonthe routes actually need. It also closes the folder-rename gap Jerrai found, as long as the check runs on the real path too, asisPrivateProjectFilealready does.
Should-fix
- Studio's own copy of the folder name and file list should come from one exported constant, not literals. Then the desktop app and the CLI can import the same set, and a new record file can't be added on one side only.
The earlier concern that the file API served a private project file with a token in it is gone: nothing in this design carries a token. What's left is the session id and the person's chat, which is what this PR covers.
Checks at this head:
safePath,files.pathSafetyandpreviewtests: 169/169.- CI is green.
— Rames
…s own files Review (Jerrai, Rames): a list of three names missed the read marks the desktop app and the CLI also keep there, and renaming the folder reached them under a new name. The check is now an allow-list: no route reaches .hyperframes/ or anything in it, by any spelling or through a link, except Studio's studio-motion.json, studio-manual-edits.json and prepared-assets/. Studio's own internal files (backups, preview documents, history id) are read by path inside the server, not through a request path. The project's file tree leaves out the same paths. The upload test now sends a file uploads accept elsewhere, so the folder is the only reason it is refused.
|
Thanks, both. At
studio-server suite: 1160 passed. |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Changes requested at 51550e42. The allow-list itself is right. The direct file routes, the folder rename and the project listing are all closed. Two paths still reach private records without going through it.
Should-fix
- The render-file route checks against the renders folder, not the project.
routes/render.ts:271resolves the filename againstrendersDir(<project>/renders,cli/src/server/studioServer.ts:553). The private check then sees a path relative to that folder. In a project that shipsrenders -> .hyperframes,GET /projects/:id/renders/file/app-history.jsonlresolves toapp-history.jsonlunder the base,isPrivateProjectPathsays no, and the route serves it. A project's own preview script can make that request. Fix: also refuse whenisPrivateProjectFile(project.dir, fp)is true. - A rename still reads and can rewrite private JSON.
updateReferences(routes/files.ts:789-815) walks the whole project withwalkFiles(:654-670). That walk doesn't skip.hyperframes, and the rewrite checks onlyisSafePath. Renaming a referenced file rewrites any matching path inside.hyperframes/*.jsonrecords (for exampleagent-handoff.jsonandapp-history-seen.json). Today's records hold no project paths, so the practical impact is small, but it breaks the "nothing under.hyperframes/is read or written" guarantee. Fix: skip private paths in the walk.
Confirmed fixed
- The allow-list exposes only
studio-motion.json,studio-manual-edits.jsonandprepared-assets/, and the folder itself is denied (helpers/safePath.ts:33-55). - PATCH checks both the source and the destination. The same resolver covers read, write, delete, duplicate and upload.
- Case variants,
./.., decoded segments and links into private records are checked against both the typed and the real path (safePath.ts:18-20,42-67). - The project listing filters private paths (
routes/projects.ts:49-55), and the upload test now has a real success control. - The allow-list excludes the files #5114 writes.
Not run locally (no installed dependencies in my checkout).
— Jerrai
jrusso1020
left a comment
There was a problem hiding this comment.
Request changes at 51550e42. The allow-list is the right shape, and it closes the leak on the file, preview, upload, rename and listing routes. One thing it now blocks is a path Studio's own preview route has to serve. One write still reaches the private records.
Fixed
- Allow-list:
safePath.tsrefuses.hyperframesand everything under it exceptstudio-motion.json,studio-manual-edits.jsonandprepared-assets/. It checks the lowercased path both as asked and after resolving links. - Reads, via
createStudioApi(base → head):files/returns 403 foragent-handoff-read.json,app-history-seen.json,share.json,app-history.jsonlandagent-handoff.json.preview/returns 404 for the same files.- Other spellings, all 403:
.HyperFrames/,./,%2ehyperframes,%2EHYPERFRAMES%2F,//,/./,assets/../,prepared-assets/../share.jsonand..%2f. - A linked folder or a linked file is refused.
- The three allowed entries still return 200.
- Writes: PUT, create, delete, rename in or out, and upload into
.hyperframesor a link to it all return 403. A rename of the folder itself returns 403, and the listing shows only the three entries.
Blocker: request pictures under .hyperframes/requests/ stop loading.
- HyperFrames Desktop saves each chat request's picture at
.hyperframes/requests/<n>/before.png, and its request cards load it through Studio's/api/projects/:id/preview/<path>route. - Probe:
GET preview/.hyperframes/requests/1/before.pngis 200 at the base and 404 at this head. When Desktop picks up this release, those cards show a broken picture. - These are crops of the person's own video, not records, so
requests/belongs on the allow-list, with a test. - This also means "nothing else under
.hyperframes/is requested" isn't quite true. It's worth a grep of what else Desktop's renderer loads throughpreview/before the list is final.
Should-fix (same as Jerrai's second point): a rename still rewrites private records.
updateReferences(files.ts) walks every.json/.mdin the project,.hyperframes/included.- Probe: renaming
assets/a.png→assets/b.pngrewrote therefin.hyperframes/share.jsonand returnedupdatedReferences: 4. That count also tells the caller which private records mention a path. - Skipping
isPrivateProjectFile(projectDir, file)in that walk closes it.
Jerrai's other point, the render-file route checking against renders/ instead of the project, is also a real bypass of the same class. Putting the check in the one place every route resolves a path would cover both.
Non-blocking
- Reuse:
STUDIO_FILESretypes two paths that already exist as exported constants,STUDIO_MOTION_PATH(studioMotionRenderScript.ts) andSTUDIO_MANUAL_EDITS_PATH(manualEditsRenderScript.ts). Importing them keeps the list and the writers from drifting apart. - The allowed folder itself (
.hyperframes/prepared-assets, no trailing slash) is refused, so an upload into it returns 403. I found no caller that does this today. - Product note: skill output under
.hyperframes/disappears from the file tree. That includesexpanded-prompt.md, which the prompt-expansion skill tells the person to review, plusanim-map/,contrast/andframe-packets/. Probably fine, but it's a visible change.
Checks at 51550e42:
- The four changed test files pass 176 of 176.
- The studio-server suite passes 1157, with 2 skipped.
- CI: the required checks that finished are green, and the edit-accuracy shards were still pending.
- Not tested: Windows path forms and macOS case-insensitivity. The lowercasing plus
realpathSync.nativeshould cover them, but I only reasoned that through.
— Rames
…rs and rename paths Review (Rames, Jerrai): - Desktop loads each request's picture from .hyperframes/requests/ through the preview route, so that folder joins the allow-list. An allowed folder's own path is allowed too. - The render-file route resolves against renders/, so a renders/ linked into .hyperframes/ reached the records. It now also checks the file against the project. - A rename's reference update walked every JSON/MD file, records included, and could rewrite them. It now skips private paths. - The allow-list reuses STUDIO_MOTION_PATH and STUDIO_MANUAL_EDITS_PATH.
|
Thanks, both. At
studio-server suite: 1163 passed. |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Changes requested at 58a7a86e. Both fixes from my last review hold. One more render path still serves private files.
Should-fix: renders listed from disk are served without the private check. GET /projects/:id/renders (routes/render.ts:298-346) lists every .mp4/.webm/.mov in renders/. It statSyncs each one, following links, and registers each in renderJobs with outputPath: join(rendersDir, filename). GET /render/:jobId/view and /download (:214-245) then readFileSync(job.outputPath) with no project or real-path check. So a project that ships renders/leak.mp4 -> ../.hyperframes/app-history.jsonl gets its chat history served from /render/leak/view once the renders list loads. Suggested fix: skip any entry where isPrivateProjectFile(project.dir, fp) is true (or any entry whose real path leaves renders/) before registering it, and add a test like the renders -> .hyperframes one.
Confirmed fixed
/renders/file/*checks the resolved file againstproject.dir, on both the typed and the real path (render.ts:281-285,safePath.ts:50-54). The new test coversrenders -> .hyperframes.- The rename reference update skips private files before reading or rewriting them (
files.ts:799-812). - The other path-helper callers pass the project dir: file mutations, media, thumbnails, previews and freeze frames. Static preview assets also apply
isPrivateProjectFile(preview.ts:615-629). .hyperframes/requests/is allowed as a whole subtree. A link inside it to../app-history.jsonlresolves to a private real path and is refused.
— Jerrai
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes at 58a7a86e. Everything from the last round is fixed. One route in render.ts still serves private records through a link. Jerrai's review raised the same issue, and I confirmed it with a separate test.
Blocking: the render list and the view/download routes follow links
GET /projects/:id/renders(render.ts:297-352) finds every.mp4/.webm/.movinrenders/withjoinandstatSync, which follow links. It saves that path as the job'soutputPath.GET /render/:jobId/viewand/download(render.ts:214-250) then callreadFileSync(job.outputPath)with no check.- Test at this head, in a project that ships
renders/leak.mp4 -> ../.hyperframes/app-history.jsonl:- The list returns 200 with
leak. render/leak/viewandrender/leak/downloadboth return 200 with the chat history.- A link to a file outside the project is served the same way.
renders/file/leak.mp4correctly returns 403.
- The list returns 200 with
- The same code is on main, so this predates the PR. But it is the shipped-link case the new
renders/test covers, one route over in the same file. - Suggested fix: build the list with the check
renders/file/*already uses. Keep an entry only whenresolveWithinProject(rendersDir, f)returns a path andisPrivateProjectFile(project.dir, path)is false, and save that resolved path. Checking again in view/download would also cover a link swapped after the list was built. - I looked at every file read in
routes/. These two routes are the only ones that serve a path that never goes through the sharedresolveWithinProjectcheck, so this closes the gap everywhere.
Fixed since 51550e42
- Request pictures:
preview/.hyperframes/requests/3/before.pngnow returns 200 (it was 404).after.png,edit-1/was-0.jpgandrect.jsonreturn 200 on bothpreview/andfiles/.- The match is the folder name plus
/, so.hyperframes/requestsX/ais still refused. - Desktop writes only picture crops and a crop-box
rect.jsonunderrequests/<n>/. No prompt or chat text is stored there, so the allow-list stays narrow.
- Escapes from
requests/are refused:requests/../app-history.jsonl,requests/3/..%2f..%2fapp-history.jsonlandrequests%2f..%2fshare.jsonreturn 403 onfiles/and 404 onpreview/.- A link inside
requests/to a record file or to the records folder is refused, because the real path is checked too. - A PUT through a folder link returns 403.
- Renaming
requestsinto.hyperframes/returns 403.
- Rename: renaming
assets/a.pngrewrote onlyindex.html(updatedReferences: 1).share.json,agent-handoff.json,comments.jsonlandreferences/moments/m.jsonwere left as they were. - Render-file route: it now also checks
isPrivateProjectFile(project.dir, fp). - Reuse:
STUDIO_FILESnow importsSTUDIO_MOTION_PATHandSTUDIO_MANUAL_EDITS_PATH, and.hyperframes/prepared-assetsitself is allowed. - The three new tests fail against the
51550e42source and pass here.
Non-blocking
- The allow-list grants write as well as read, so files under
requests/can now be overwritten, deleted and renamed through the file routes. That is probably fine for the person's own crops, but it is a choice worth knowing about. - Request pictures now show in the Studio file tree.
Checks at 58a7a86e
- The four changed test files pass 213 of 213.
- CI had no failures when I looked. "Tests on windows-latest" passes. The edit-accuracy shards were still running.
— Rames
…ile route would serve Review (Jerrai, Rames): the render list found files in renders/ with join and statSync, which follow links, and registered them for /render/:id/view and /download, which read them with no check. A project shipping renders/leak.mp4 -> ../.hyperframes/app-history.jsonl had its chat served. The list now keeps an entry only when resolveWithinProject accepts it and it isn't private, and registers the resolved path. View and download serve only a plain file, so a link swapped in after the list is refused. This predates the PR; it's the same shipped-link case.
|
Thanks, both. At
studio-server suite: 1164 passed. |
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Approving at dc304cd9.
The bypass from my last review is closed, in two layers:
GET /projects/:id/rendersnow builds each entry's path withresolveWithinProjectand drops anythingisPrivateProjectFileflags. That predicate also compares real paths, so arenders/leak.mp4link into.hyperframes/is never listed and never registered inrenderJobs./render/:jobId/viewand/downloadnowlstatthe output path and serve only a plain file.
The new test proves the list never registers the link, and that real still views and downloads.
Non-blocking: the test can't tell the two layers apart. leak is never registered, so its 404 holds even without isPlainFile. A case that seeds renderJobs with a symlinked outputPath directly would pin the second layer. Not needed for this PR.
— Jerrai
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at dc304cd9. The blocker from my last review is fixed.
What I checked
dc304cd9 is the only commit since 58a7a86e, and it touches only render.ts and its test. I wrote a scratch test against createStudioApi and checked each case on GET /projects/:id/renders, /render/:id/view and /render/:id/download:
renders/leak.mp4linking to.hyperframes/app-history.jsonl, by absolute and by relative path: left out of the list, and view/download return 404.renders/out.mp4linking to a file outside the project: left out, 404.renders/x.mp4linking to the.hyperframesfolder: left out, 404. Arenders/subfolder link isn't listed, andrenders/file/sub/app-history.jsonlreturns 403.- A link swapped in after the list was built (list
a.mp4andb.mp4as plain files, then replace them with links to the history and to an outside file): view/download return 404 for both. - A plain render in
renders/still lists, views, downloads and deletes. - A job started by
POST /projects/:id/renderstill views and downloads. I checked three setups: renders inside the project, renders outside it (the../renderssetup invite.adapter.ts), and a project opened through a symlinked path.
The list now uses the same check as renders/file/* (resolveWithinProject plus isPrivateProjectFile), so the two routes agree on what's in scope. The full studio-server suite passes (1161 tests), along with tsc, oxlint and oxfmt. In CI, every check passes, including "Tests on windows-latest", the edit-accuracy gate and CodeQL.
Non-blocking
- A hard link in
renders/to a private file is still listed and served. A hard link can't be told apart from a real file, so this is a known limit, not something to fix here. - View and download run
lstatonly on the last part of the path. If the wholerenders/folder is swapped for a link after the list, a file with the same name in the new target gets served. That needs a.mp4/.webm/.movnamed like a listed render, and the app's own records don't use those names, so it's fine as is. - A link inside
renders/to another render (alias.mp4 -> real.mp4) is listed, but view and download return 404 for it, whilerenders/file/alias.mp4serves it. Studio plays renders throughrenders/file/*, so users won't see this. The list and view/download just don't quite agree. - Still open from last time: the
.hyperframes/requests/allow-list also allows writes, and request pictures show in the file tree.
— Rames
The desktop app keeps three files in each project that hold the person's agent session and their chat:
.hyperframes/agent-handoff.json, whichhyperframes openand Studio's Edit with Framey write..hyperframes/agent-runs.jsonl, the app's own log of every chat run..hyperframes/app-history.jsonl, the record of chat turns that the terminal agent catches up on (hyperframes catch-up, coming next).Studio's file and preview routes reach any path inside the project, so anything that reached the preview port could read those files, or plant a fake hand-off or history.
What changed
isPrivateProjectFile(safePath.ts): names those three files, in any case and through a link (lexical and real path).resolveWithinProject: studio-server's copy now wraps core's and refuses them, so every route that resolves a request path does too: read, write, delete, rename, duplicate.preview/compreturn 404 for them..hyperframes/: served as before, including Studio's motion file and prepared GIF assets.Split out of the closed #5073. Unlike that version, this blocks only these three files, not all of
.hyperframes/, so prepared GIF assets keep loading.What I measured
safePath.test.ts,files.pathSafety.test.tsandpreview.test.tscover every route by plain path, by another case, and through a link, plus the files that stay reachable.tsc,oxlint, the comment ratchet, and the pre-commit hooks (fallow, typecheck): all pass.What I did NOT exercise