Repository navigation
fix(studio): stop file names with %, apostrophes or $ from breaking Studio - #5156
Conversation
A browser sends src="100%.png" with the % unescaped, and decoding the whole path threw, so Studio answered 500. Only well-formed escapes are decoded now.
…pshot server One shared decode rule for request paths; the image thumbnail route's malformed-escape 400 could no longer fire, so a name that is not a file is a 404.
Edit accuracy: accurate 2059 (base branch 2059), smooth 1638 of thoseThe gate passes. Quarantined, measured but not gated (0) |
…ting attributes An attribute value is matched by its own quote kind, so src="it's.png" is replaced instead of duplicated, and every insert uses a function replacer, so $1 or $& in a file name is written as is.
…ted media sources WIP: not yet tested; see the resume note.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
There was a problem hiding this comment.
Review at 2b2c4f177f58233796fbf06a022a83a26e4c1469 — requesting changes for a CSS asset regression.
packages/core/src/compiler/htmlBundler.ts:150-153: rebaseCssUrls now feeds the source CSS token directly to encodeUrlPath, which treats a CSS escape as a literal filesystem backslash. For a linked css/theme.css containing url(icon\ 1.png) and a physical css/icon 1.png, the effective base emits url(css/icon\ 1.png) (the backslash escapes the space in CSS); this head emits url(css/icon%5C%201.png). The latter requests a backslash-containing filename rather than css/icon 1.png, so the image disappears from the bundled composition. An exact-head bundleToSingleHtml(project, { inlineAssets: false }) run produced url(css/icon%5C%201.png); the actual static project server returned 404 for that bundled URL and 200 for /css/icon%201.png. I did not run a full browser render. Please decode CSS escapes in the CSS URL grammar before resolving/encoding the physical filename, and add a linked-stylesheet bundling regression for an escaped-space filename.
The required Linux Test and Windows test lanes also fail at packages/cli/src/server/studioServer.previewAssets.test.ts:70: the new URL rebase correctly emits assets/pic%402x.png, but the unchanged assertion expects the raw spelling assets/pic@2x.png. The preview asset route decodes %40 to @; this looks like a stale spelling assertion, not a second asset-serving defect. Please update the assertion while retaining the following fetchAsset(nested) byte check, then rerun the required checks. The capture check used an older PR body without ## Before/## After; those sections are now present, so that check also needs a fresh run. I did not run the full local suite.
— tai
…encoded reference renames
…es, %, # or ? load
… reads a src as a URL
…s files named with %, # or ?
|
Thanks. Both points are fixed at dfb749a:
This head also folds in related spelling fixes: a dropped timeline asset writes its |
jrusso1020
left a comment
There was a problem hiding this comment.
Approve. Both changes the earlier review asked for are in, and I found no blocking defects at dfb749a3.
What I checked
- CSS escapes:
rebaseCssUrlsnow decodes CSS escapes before resolving, then re-encodes the rebased path.- I ran the real
bundleToSingleHtml(project, { inlineAssets: false })on a project whosecss/theme.cssusesurl(icon\ 1.png)next to a realcss/icon 1.png. It now producesurl(css/icon%201.png). - These all rebase to the correct file: hex escapes (
\31,\000020),\27, a line continuation,%,$, non-ASCII, an already percent-encoded name, and?/#suffixes. data:andhttps:URLs are left alone.- With
inlineAssets: true, each one inlines the right bytes.
- I ran the real
- Preview assets:
studioServer.previewAssets.test.tsexpectsassets/pic%402x.png, and it still fetches that URL and checks the bytes. - Every path that reads or writes a file reference: timeline drop, the authored-src reader, thumbnails, peaks, the media and loudness routes, preview serving, sub-composition rewriting, producer media resolution, lint and rename.
- Each path decodes once.
- Encoded spellings that Studio now writes still resolve in frame extraction, audio mixing and duration probing, because those try decoded variants.
- Decoded request paths are still checked against the project root.
- Callers: callers of the removed exports are gone or updated.
Verification
- Tests:
- core compiler: 528/528
- parsers: 1245 passed
- cli server/static/validate: 257/257
- studio-server: 1211 passed
- studio: 7739 passed. The only failures are two browser tests that need Chrome, and this PR doesn't touch them.
- producer natural-duration: 7/7
- Six mutants, all caught:
- dropping CSS escape decoding
- dropping the rebased-path encoding
- dropping suffix escaping
- reverting the shared decoder to a single
decodeURIComponent - suffix-matching attribute names in the source patcher
- writing the dropped
srcraw
- CI: required checks are green. The timeline viewport gate is not required, and it fails with the same budget miss on the last four commits on main.
Non-blocking
files.tsrename and JSON's escaped slash: the guard that skips a match inside a longer existing path no longer understands\/in a non-script HTML attribute.- Example: with
logo.pngandassets/my logo.pngin the project, renaminglogo.pngtobrand.pngrewritesdata-props='{"img":"assets\/my logo.png"}'toassets\/my brand.png. Main leaves it alone. - The old "compares escaped spellings by their text" test was deleted, not adapted.
- This is rare, since
JSON.stringifydoes not write\/.
- Example: with
- Two CSS cases that already fail on main:
- In a linked stylesheet,
@import "my\ file.css"is still not decoded for CSS escapes, so it stays un-inlined and resolves against the document root. url("it\'s.png")andurl("p\(1\).png")are still not matched by the CSS url pattern, so they are not rebased. The known-limits note mentions only).
- In a linked stylesheet,
— Rames
Requested on an older commit; later commits answer it (see the thread). Current head is dfb749a.
…es-with-a-bare-percent # Conflicts: # packages/studio/src/utils/timelineAssetDrop.ts
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approving at a3afb8e8. Since my approval at dfb749a3, four commits:
8b0259a3deletes the one-line design note underdocs/contracts/. No code.a8ea52f0, a main merge: the remerge-diff is empty, so it resolved cleanly.f6833e9f, a main merge: the one conflict is the import block ofpackages/studio/src/utils/timelineAssetDrop.ts. Both lines are kept, and both are live.encodeUrlPathis still used to write the dropped clip'ssrc(line 109).buildTimelineAssetIdis main's re-export from@hyperframes/core/timeline-asset-id(#5169). No other conflicts.a3afb8e8:matchesGeneratedIdandreplacementTimelineAssetIdnow derive ids fromdecodedUrlPath(src). Because this PR writes dropped clips with an encodedsrc(my%20clip.mp4), the generated-id check comparedmy_clipagainstmy_20clip. A replaced clip was therefore never recognised as auto-named and kept its old id.decodedUrlPathdecodes only well-formed escapes and strips any query or fragment, so a bare%can't throw. Core already depends on@hyperframes/parsers, so the import adds no new package edge. The new test covers both call sites: dropping either decode fails it (new_20take, or no rename).
CI is green at this head.
— Rames
What
Special characters in file names keep working when Studio serves, edits or renames a file.
100%.pngare served by Studio and the CLI's static project server without a decoding exception.src="assets/it's.mp4"replaces the existing attribute.srcnever matches insidedata-src,x-src,xml:srcorsrcset; replacement values containing$remain literal.data-composition-srcpaths. HTML URL fields, includingsrcsetand SVGxlink:href, and CSS URLs receive escaping appropriate to their consumers, including newly introduced quotes, URL punctuation and parentheses. HTML URLs also escape scheme delimiters, backslashes and characters that browsers strip.sale 50% off #1?.mp4onto the timeline writes itssrcas a URL, probes its real length, and its thumbnail, waveform, peaks, inspector actions (Remove background, the audio check, color grading) load. Studio reads an authoredsrcas a URL in one place, so each reader decodes it once. Renaming a file also renames references already spelled percent-encoded, such asassets/My%20clip.mp4.%,#or?gets its metadata, background removal and loudness normalize, and a?v=query on an existing name is still ignored.id, never by an earlier element whosedata-idmatches.%20stays distinct from a name containing a space. References inside longer existing file names remain untouched. Numeric and named HTML entities reach the native decoder before candidate matching, and a second rename preserves literal backslashes in physical names.How
One shared asset-path decoder handles well-formed escape runs and leaves malformed or bare percent text literal. Studio-server keeps its existing decoder export for compatibility.
One shared source scanner reads actual attributes by their own quote delimiters and exact names, skipping comments and raw text bodies. Native HTML entity decoding runs before URL decoding. The source patcher, timing compiler, composition collector and codec scan use that boundary. Function replacements preserve literal dollar signs, and linked audio reads use the same attribute reader.
The rename rewriter checks identity in the reference's actual grammar before replacing it. Core's URL consumers split query/fragment suffixes, decode once, then check containment and resolve the physical file. Linked CSS URLs decode CSS escapes before URL decoding and physical path resolution, then normalize raw URL separators before percent decoding and encode the rebased path for URL grammar. Decoded whitespace remains part of the file name, and query/fragment suffixes are escaped when written back into CSS. Composition paths and the shared asset-existence callback remain physical paths. Clip ids derived from a media
src(the rename when a clip's media is replaced) decode thesrcfirst, so a droppedmy clip.mp4, now writtenmy%20clip.mp4, still renames to its new file's name.Validation
Focused tests run serially on Linux with at most two workers. The rename suite passes three consecutive runs. Shared sub-composition paths, core bundling, media duration, source editing and serving routes have focused regressions. The linked-stylesheet escaped-space regression checks both the rebased URL and the inlined asset contents. Separate cases preserve escaped leading-space file identities, keep CSS line continuations from becoming filename characters, and serialize decoded suffixes safely. A linked CSS sibling-file witness distinguishes raw URL separators from percent-encoded physical backslashes. The CLI preview test fetches the rebased URL and checks the exact asset bytes, including percent-encoded
@names. Regressions invoke the real rename route and bundler, resolve raw JSON paths against real files, rename quoted references twice, and assert inlined asset contents from directories containing literal percent and URL punctuation. The source patcher and linked-audio suites cover exact attribute names, both quote kinds and dollar signs, an apostrophe insrc,data-srcbeforesrc, and an earlierdata-idequal to the target id. The timeline drop test fails with thesrcwritten raw, and the encoded-rename test fails on main's rename route (the reference keeps its old name). The authored-src reader, both dropped-asset probes, the media-probe source, the media and loudness routes and a nested-composition snapshot link each have a test that fails without the change. The whole Studio suite (7751) and Studio server suite (1211) pass.Old implementations are restored temporarily to prove the regressions fail at their intended assertions. Typechecks, lint, formatting and comment checks run against the final revision.
Before
The Studio “Remove background” action with “Keep sound” saves duplicate
srcattributes for an apostrophe-named video; reloading shows the original red video. The removal service and job progress use a deterministic fixture. Studio's source save and reload are real.A file named
sale 50% off #1?.mp4(a 4 s test pattern) dropped onto the timeline of a fixture project built for this capture: the clip lands with the default 5 s length and an empty strip, because its URL is cut at#.After
The same action replaces the source once and preserves the linked audio's original file path. Reloading shows the blue cutout fixture.
The same drop: the clip shows its frames and its real 4 s length.
Known limits
A clip dropped before this change keeps a raw
src. If that name contains#,?or a literal%XX(a#b.mp4,50%20off.mp4), replacing its media keeps the old clip id instead of renaming it. Nothing is lost; asrccannot say whether it was written raw or encoded.Two neighbouring gaps exist on main today and get their own follow-up: choosing an imported font whose file name has a lone
%fails to save, and the image field in the property panel writes a picked file'ssrcand backgroundurl()without encoding.A media or loudness request path that names no existing file is now read as a URL and decoded once, so a requested background-removal
outputPathofassets/x%20y.webmwritesassets/x y.webm. Studio itself never sends an output path.JSX/TSX references keep their existing raw string handling; JSX attribute grammar is outside this change.
Quoted URLs containing
)in linked CSS still fall outside the existing rebasing grammar. Broader CSS parsing remains a follow-up.The producer's existing file server keeps its per-segment fallback for malformed URL escapes. Its handling of a segment mixing a bare percent and an escape is unchanged.