Repository navigation
fix(lint): check passes valid sub-compositions and scene-relative asset paths - #5337
Conversation
7fcca79 to
69303c1
Compare
Edit accuracy: accurate 2059 (base branch 2059), smooth 1593 of thoseThe gate passes. Quarantined, measured but not gated (0) |
…ng backslash stays absolute
somanshreddy
left a comment
There was a problem hiding this comment.
Review at ea2a8b40: the code is clean, no findings that block. I'm leaving a comment rather than approving only because a required check is red at this head: Tests on windows-latest, shard engine-cli. It looks unrelated:
- The failure is
packageHls.test.ts:420("writes a master and no audio playlist when there is no audio"), a real-ffmpeg test that hit the 5 s timeout. ItsafterEachthen failed withENOTEMPTYon the temp dir. - This PR touches no file under
packages/engine.
A rerun of that job should clear it. I'll approve on a re-ping once it's green.
Checked:
- Size rule:
root_missing_dimensionsnow applies only when!options.isSubComposition, the same flagroot_dimensions_mismatchalready uses.lintProjectpassescompSrcPath, the entry asrootFileand each scene's own path; the Studio route and the producer pass the file they lint. - Root lookup:
findRootTagnow skips an<html>/<head>with no composition markers, using the sharedhasCompositionMarker. An<html>that carries the markers is still eligible as the root. - Traversal rule: it's now a segment walk from the file's folder depth, after normalising
\and%2e, decoding entities, and dropping?/#. Scheme, absolute and#values are skipped. A linked stylesheet'surl()s resolve from the stylesheet's ownrootRelativePath. - Backslash paths:
rewriteAssetPathreads\as/before decoding and leaves a path starting with/or\unchanged. Resolution still goes throughresolve("/", join(compDir, …)), so a..\..\..\xpath is clamped to the project root like the/form. It can't reach outside the project.
Tests:
- lint: 902 passed. 3
slideshow.test.tsfailures (malformed hotspot) also fail on main on this box, so they aren't from this PR. - parsers
rewriteSubCompPaths: 15/15. studio-serverroutes/lint: 3/3. - Mutants: 5 of 6 caught:
- sub-composition exemption dropped;
- file depth ignored;
- stylesheet depth taken from the HTML file;
<html>/<head>skip dropped;- backslash swap dropped in
rewriteAssetPath.
Low:
..-clamp infolderDepthuntested. Removing theMath.max(0, …)passes every test. It only matters for an unnormalisedcompSrcPathsuch as../x/scene.html, which no caller passes today.- Encoded
%2fisn't treated as a climb (from reading the code; not run). The rule normalises%2ebut not%2f, whilerewriteAssetPathdecodes every well-formed escape (decodeWellFormedEscapes). So..%2f..%2fx.pngfrom a scene isn't flagged, yet render decodes it to../../x.pngand clamps it tox.png, while the browser preview requests a literal file name. That's niche, but it's the "loads a different file than written" case the new message describes. Normalising%2flike%2ewould close it.
— Somu
…limb out of the project
|
Thanks. Both lows are in at
The red Windows |
somanshreddy
left a comment
There was a problem hiding this comment.
Approving at aef230d7. My comment at ea2a8b40 (5473957847) found nothing blocking, and the only commit since then takes both of my lows:
%2f: the traversal rule now reads an encoded%2fas a separator (.replace(/\\|%2f/gi, "/")), matching how render decodes it...%2f..%2fx.pngfrom a scene is now flagged.folderDepthclamp: now pinned by a test (../x/scene.htmlwith../y.pngpasses).
composition.test.ts passes 201/201. Reverting either change fails its new test.
Merges cleanly with main. Tests on windows-latest is green at this head; the packageHls timeout I flagged didn't recur, and #5368 fixes it separately. Every other required check is green; Render on windows-latest was still running when I approved.
jrusso1020
left a comment
There was a problem hiding this comment.
Approve at aef230d7. This is in addition to Somu's approval. Both of Somu's lows are fixed, each new test fails without its fix, and lint now agrees with render on every traversal shape I tried.
Somu's lows
folderDepthclamp (composition.ts:140): the test atcomposition.test.ts:1411fails ifMath.max(0, …)is removed.%2fas a separator (composition.ts:671): the test at:1405fails if%2fis dropped from the regex.- There's no false positive. Turning
%2finto/only adds segments, soassets/a%2fb.pngfrom a scene still passes, while..%2F..%2Fx.pngis flagged. - It matches render. From
compositions/,rewriteAssetPathturns..%2f..%2fx.pngintox.pngclamped to the project root, so it loads a different file than the one written.
- There's no false positive. Turning
Traversal: lint vs rewriteAssetPath
- They agree on mixed
%2E%2eand.%2e/, on%252e(decoded once, so a literal name), on....//, fullwidth dots and%5c(not climbs), and on//host,file:anddata:(skipped). - Quoted or space-padded
url(), an inlinestylewith", and<link href>are all flagged.
Entry exemption (core.ts:506): the entry can't pick up the sub-composition exemption. Only lintProject's compositions/ loop and Studio's lint route pass isSubComposition. A nested entryFile gets no exemption, and its depth comes from its own folder. Studio's route takes compSrcPath from its own folder walk, not from the request, and only counts depth with it.
Nits
- The producer's new
compSrcPath(producer/src/services/hyperframeLint.ts:157-160) has no test. No producer test callsrunHyperframeLint, so deleting the line leaves everything green. - Gaps that predate this PR, where lint says OK but the path escapes; render's clamp is the real guard in each case:
srcsetandposter(neither lint nor render handles them,parsers/src/assetPaths.ts:20)- CSS hex escapes such as
url(\2e\2e/\2e\2e/x.png), whichhtmlBundler.ts:152decodes for linked stylesheets - a tab or newline inside the URL, which the browser strips
- Studio still labels any uncovered
.html(for example an alternate rootmain.html) as a sub-composition. That's older than this PR, and the body says so.
Tests:
- lint 907/907, parsers
rewriteSubCompPaths15/15, studio-serverroutes/lint3/3. - 9 of 10 mutants are caught: the
%2fnormalisation, the clamp, checking the climb only at the end, stylesheet depth taken from the HTML file, the raw vs entity-decoded attribute, the exemption, the<html>/<head>skip, the backslash order, and Studio'scompSrcPath. The survivor is the producer line above.
— Rames
What changes for a user
hyperframes checkstops failing valid sub-compositions in two ways that sent authors in circles:A scene without its own size. A sub-composition whose root has no
data-width/data-heightgotroot_missing_dimensions(an error), and the error stoppedcheckbefore its browser audits. A mounted sub-composition without a size fills its host at 100% (compositionLoadersizes the flattened root to100%when it declares none), and the core skill tells sub-composition roots to use 100%. The rule now applies to the entry composition only, as its own fix hint already says. Sub-compositions are found the same wayroot_dimensions_mismatchalready exempts them (isSubComposition).A scene whose
<html>carries its variables. With<html data-composition-variables=…>around a<template>, the linter took<html>itself as the composition root and reportedroot_missing_composition_idandroot_missing_dimensionsagainst it. A<html>or<head>with no composition markers is now skipped when looking for the root, the same exemption a marker-less leading<svg>already gets, so the template's real root is found.../assets/…in a scene. The bundled guides write sub-composition asset paths relative to the scene file (../assets/logo.pngfromcompositions/scene.html), andinvalid_parent_traversal_in_asset_pathrejected every../path as an error. The bundler, the producer and the preview runtime all resolve such a path from the scene's own folder (rewriteSubCompPaths), and project lint's missing-asset check already does too. Studio preview on the current release loads../assets/a.pngfrom a scene (checked: the image is rewritten toassets/a.pngand decodes, no failed request). The rule now flags only a../path that still leaves the project once resolved from the file's folder, e.g.../../x.pngfromcompositions/scene.html, or any../from the rootindex.html...\assets\x.pngin a scene. The bundler and Studio preview rewrite a scene's../paths when inlining it into the root document, but only recognised/as a separator. A path written with backslashes was left as written, so preview resolved it against its own URL and 404'd, while render clamped it and loaded the file.rewriteAssetPathnow reads a written\as/before decoding, so both load it. As in a browser, an encoded%5Cstays part of the file name and a path starting with\stays absolute.How
HyperframeLinterOptionsgainscompSrcPath, the file's project-relative path, the same name project lint already uses for it.lintProjectpasses it for the entry and everycompositions/file; Studio's lint route passes the file it lints.../path's segments from that folder and flags it only if it climbs above the project root. It is a plain segment walk, notnode:path, because the rule also ships in@hyperframes/lint/browser. WithoutcompSrcPathit treats the file as the project root, which keeps the old answer for callers that lint a single loose file.url()s resolve from the stylesheet's own folder, as the browser and the bundler resolve them.lintProjectnow hands each linked stylesheet's project-relative path to the rule. Before,url('../fonts/B.woff2')instyles/main.csslinked fromindex.htmlwas a false error, and a real escape in a stylesheet linked from a nested scene could pass.../. Backslashes,%2fand%2eare normalised and entities decoded first (render decodes%2finto a separator), and the query and hash are dropped, so%2e%2e/x.png,..\x.png,assets/../../x.pngand../x.pngall count as climbing out. URLs with a scheme, absolute paths and fragments are skipped...-aware walk; an absolute or unnormalised path (sub/../index.html) counts as the project root, as before this change.compSrcPath, so a nested entry gets the same answer there as inhyperframes lint.browser.test.tsnow walks the browser entry's imports, including@hyperframes/parserssubpaths, and fails on any Node built-in. Theplatform: "browser"build cannot catch those, because the parsers package stays external.findRootTagshares onehasCompositionMarkercheck for<body>,<svg>and now<html>/<head>.Tests
rules/core.test.ts: a template sub-composition without a size reports noroot_missing_dimensions;<html>with variables around a<template>finds the real root.rules/composition.test.ts: the traversal tests now state the contract:../from the root file errors;../assets/fromcompositions/scene.htmlpasses and../../assets/errors; fromcompositions/frames/scene.html,../../assets/passes and../../../errors. A path that dips out midway (../assets/../../x.png) and a./-prefixed file path also error.browser.test.ts: the browser entry reaches no Node built-in, including through dynamicimport()and bare names likepath. Re-adding anode:pathhelper to the rule fails it.rules/composition.test.tsalso covers climbs written encoded, backslashed, mid-path or entity-encoded (from the root file and from a scene), absolute and..-containing file paths (a leading..never counts below the root), an encoded%2fclimb, skipped URLs, a query or hash that would climb if read as a path, and a file linted withoutcompSrcPath(treated as the project root).studio-serverroutes/lint.test.ts: the lint route passes the file it lints ascompSrcPath.project.test.ts: a stylesheet linked fromindex.htmlwithurl('../fonts/…')passes; a stylesheet linked from a scene withurl('../../outside.png')is reported.project.test.ts: throughlintProject, a scene's../assets/logo.pngpasses while../../outside.pngis reported, and a scene without a size does not getroot_missing_dimensions.tsc --noEmitclean for lint, cli, studio-server and producer; comment ratchet clean.<html>as a root candidate fails the<html>-variables test; ignoring the file's folder fails two traversal tests and one project test; resolving stylesheets from the HTML file's folder, skipping the%2e/backslash normalisation, reading the query as path, dropping the normalisation, the entity decoding, the absolute-path rule, the..-aware depth, or the skip for URLs each fail at least one test; droppingcompSrcPathfrom project lint fails the project test.parsersrewriteSubCompPaths.test.ts:..\assets\x.pngfromcompositions/scene.htmlrewrites toassets/x.png(fails without the change);a%5Cb.pngkeeps its encoded backslash and\x.pngis left alone (each fails if the swap moves after decoding or the leading-slash skip goes). 15 passed, three runs; coreinlineSubCompositionsandhtmlBundlertests 202 passed.Before
hyperframes linton 0.8.143, three small projects (a scene without a size, a scene with variables on<html>around its<template>, a scene using../assets/a.png):After
The same projects on this branch: none of these findings. A path that really leaves the project (
../../outside.pngfromcompositions/) is still an error.Not changed here
Studio's lint route treats every HTML file outside the project graph (such as
scenes/intro.html) as a sub-composition, so such a file no longer getsroot_missing_dimensionsthere. That labelling predates this change and its test pins it, so it stays.Two older copies of the sub-composition path rewrite still read only
/: the runtime loader's fallback for scenes loaded at runtime (core/src/runtime/compositionLoader.ts) and Studio's copy-to-clipboard (studio/src/utils/authoredSource.ts). Neither is the path preview or render takes for bundled scenes; they are left for a follow-up.