fix: serve projects and assets whose names contain @ % # & or ? - #4415
Conversation
Five routes cut the decoded project id out of Hono's c.req.path, which leaves %40 %25 %23 %26 %3F encoded, so the cut missed and files, preview assets, sub-compositions, thumbnails and waveforms 404ed or 403ed for those names. One helper now takes the sub-path by segment from the raw URL.
…for any name The thumbnail route built its preview URL from the raw project id and path, so # ? and % broke it, and neither route checked that the decoded path stays inside the project. Thumbnails now use the composition guard and an encoded URL; waveforms use the read-only asset guard.
Both /composition/* routes cut a prefix out of Hono's c.req.path with no decode. They now share studio-server's requestSubPath (renamed from projectSubPath and generalised to any route) with the five studio-server routes, so one helper owns how a request path becomes a file path.
…s 404 An empty thumbnail path resolved to the project directory and crashed reading it (500).
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approve at 076ef518. I read the helper, all seven call sites and both new test files, and ran them locally.
Encoding is applied once. requestSubPath splits new URL(url).pathname into raw segments and runs decodeURIComponent once on everything after the route. The old c.req.path + decodeURIComponent pair is gone from every site, so nothing decodes twice. I checked this directly: %252e%252e%252Fsecret.txt comes back as the literal %2e%2e%2Fsecret.txt, and the files route serves the file with that literal name.
No new traversal. Decoding can now turn %2F into a separator, so containment has to happen after the decode. Every consumer does it that way:
resolveProjectPath→resolveWithinProjectpreview/comp→resolveWithinProjectpreview→isWithinProjectRootthumbnail→ newresolveWithinProjectwaveform→ newisWithinProjectRoot- CLI
play/present→isSafePath
I ran a throwaway probe: 75 cases, covering the five routes × ..%2F, %2e%2e%2F, %2E%2E%2F, sub%2F..%2F..%2F and a nested proj%2F.. escape, × .txt/.html/.mp3, all aimed at a sibling of the project dir. None returned 200 or leaked the file. In-project controls on files, preview and preview/comp returned 200, so the probe wasn't passing because everything 404s. I also re-ran the PR's own mutations: disabling the waveform guard or the thumbnail guard each fails exactly one traversal test in projectIdEncoding.test.ts.
Tests at head: studio-server projectIdEncoding, files, files.pathSafety, preview and thumbnail: 224 passed. CLI compositionRoute.encoding and play: 30 passed.
I checked the PR body's claim of seven sites against the source. The only remaining c.req.path users are render.ts renders/file/ (generated job ids, as the body says) and the CLI's static SPA handler. Neither takes a user-named path.
Nit, not blocking: a malformed escape (/files/50%zz) makes decodeURIComponent throw, and the route answers 500. Studio-server already behaved this way on main. For play / present it is new, since the old line never decoded and returned 404. Catching the URIError in the helper and treating it as not-found would make every site answer 404.
— Rames
Checking the path with stat and then reading it let the file change in between (CodeQL js/file-system-race). One open now serves the type check, the mtime and the read; a missing file still thumbnails from the preview as before, anything that is not a file answers 404.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approve at dd4cec13. The delta since my approval 5304588945 at 076ef518 is one commit, dd4cec13b, touching thumbnail.ts only.
readCompositionSource opens the file once, then does fstatSync and readFileSync(fd) on that same descriptor. That closes the stat-then-read window CodeQL flagged. Behaviour is otherwise the same as the head I approved:
- ENOENT returns
"missing", and the thumbnail is still generated from the preview. On the old head theexistsSyncguard did the same. - A directory, or anything that isn't a regular file, returns
"not-a-file"and answers 404. The emptythumbnail/case is still covered. resolveWithinProjectstill runs before the open, so the containment I probed last round is unchanged.- The
fdis closed infinallyon every return.
Tests at head: projectIdEncoding and thumbnail passed 67 (47 + 20).
Nit, not blocking: on Windows, openSync on a directory throws (EISDIR/EPERM) instead of returning a descriptor. It isn't ENOENT, so it is rethrown and the empty-path request answers 500 there rather than 404. Mapping EISDIR to "not-a-file" would make it platform-independent.
— Rames
2 participants
What changes for a user
A project whose name contains
@,%,#,&or?now loads its files, preview assets, sub-compositions, thumbnails and waveforms when the client percent-encodes the name in the URL. Before, those requests answered 404 (403 for the files route), so a project named from a sentence with an@mention showed no preview or timeline pictures.hyperframes playandhyperframes presenthad the same problem with asset and scene files named with those characters.Why
Seven routes turned a request path into a file path by cutting a prefix out of Hono's
c.req.path. Hono decodes that path withdecodeURI, which leaves%40 %25 %23 %26 %3Fencoded. So the cut missed (studio-server, where the prefix holds the decoded project id), or the file name kept its escapes (CLI, which never decoded).One helper,
requestSubPath(url, route)in@hyperframes/studio-server, now takes the part after the route by segment from the raw URL and decodes it once. The seven sites:routes/files.tsresolveProjectPath(six callers: files, file-mutations, gsap-animations, gsap-mutations, gsap-mutations-batch, gsap-mutation-rollback)projects/:id/<route>routes/preview.tsprojects/:id/preview/comproutes/preview.tsprojects/:id/previewroutes/thumbnail.tsprojects/:id/thumbnailroutes/waveform.tsprojects/:id/waveformcommands/play.tsregisterCompositionRoutecompositioncommands/present.ts(route extracted intoregisterPresentCompositionRoute, same behaviour)compositionTwo more defects in the same routes, fixed here:
thumbnail/*built its internal preview URL from the raw id and path, so#turned the rest into a fragment,?into a query string, and a lone%was malformed. It now encodes both.thumbnail/*andwaveform/*joined the decoded path with no containment check, so an encoded../reached outside the project. That was the same on main. They now use the guards their siblings already use:resolveWithinProjectlikepreview/comp/*for compositions,isWithinProjectRootlikepreview/*for read-only assets.thumbnail/path resolved to the project folder and crashed reading it (500). It now answers 404.Name matrix
studio-server:
src/routes/projectIdEncoding.test.tsruns every name as a project id against all five routes, plus two traversal cases. cli:src/commands/compositionRoute.encoding.test.tsruns the same names as asset file names againstplayandpresent(?is skipped on Windows, where file names cannot hold it).A @HyperFrames launch50% off#2 takeTom & Jerrywhy?playandpresent: with the old line, both fail the same five reserved names (10 of 16); on this branch, 16 passed.What I measured
#,%and?. Skipping one segment too few in the helper turns the matrix red.compositionRoute.encoding.test.ts16 passed andplay.test.ts14 passed, each exit 0. Putting onlypresentback on the old line fails onlypresent's five reserved names.files.test.ts80 passed,files.pathSafety.test.ts25 passed,preview.test.ts52 passed,thumbnail.test.ts20 passed, each exit 0.tsc --noEmitexit 0 in studio-server and cli.oxfmt --checkexit 0 on every changed file.A @HyperFrames launch, requested asA%20%40HyperFrames%20launch(first commit):files/index.htmlpreview/clip.mp4preview/index.htmlpreview/comp/index.htmlNo visual change: server routing only.
What I did NOT exercise
app.requestin tests. The helper finds the firstprojectssegment, so a host that mounts the API under a prefix still works unless the prefix itself contains aprojectssegment.render.tsrenders/file/and the engine's file server take a path the same way, but the names there are generated (job ids), so this bug does not reach a user. Not changed.