fix(core): treat unparsable timing attributes as missing instead of NaN#1206
Open
calcarazgre646 wants to merge 2 commits into
Open
fix(core): treat unparsable timing attributes as missing instead of NaN#1206calcarazgre646 wants to merge 2 commits into
calcarazgre646 wants to merge 2 commits into
Conversation
A typo like data-start="abc" flowed through parseFloat unguarded in both the timing compiler and the HTML parser. The compiler serialized it into the compiled output as data-end="NaN"; the parser turned a garbage data-end into a NaN duration via Math.max(0, NaN), which is NaN. Neither lint nor compile surfaced anything to the user. compileTag now normalizes an unparsable data-start to 0 in the output, strips unparsable data-end/data-duration so they take the existing missing-attribute paths (recompute or resolver), and the parser falls back to its own defaults. Same policy extractResolvedMedia already applied to data-duration, and the same validation the composition-level duration already gets in extractCompositionMetadata.
CodeQL flags the \s* prefix as polynomial backtracking on attacker-controlled input (js/polynomial-redos). A single \s is enough: the attribute always has whitespace before it inside a tag.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Element-level timing attributes go through
parseFloatunguarded in the timing compiler and the HTML parser, so a typo likedata-start="abc"propagates as NaN with no signal to the user:compileTag(timingCompiler.ts) computesdata-endasstart + parseFloat(durationStr)and serializes the result into the compiled HTML: the output literally containsdata-end="NaN". The same unguarded reads exist ininjectDurations,extractResolvedMedia(start/mediaStart; duration was already guarded),clampDurations, and the unresolved-div scan.parseHtml(htmlParser.ts) computesduration = Math.max(0, parseFloat(dataEnd) - start)— andMath.max(0, NaN)is NaN, so a garbagedata-endlands in the timeline model as a NaN duration.updateElementInHtmlwritesdata-end="NaN"back into the document whendata-startis garbage.Neither lint nor compile reports anything: the linter has no numeric check for element timing attributes, while composition-level duration is already validated (
validateCompositionHtmlerrors on non-finitedata-composition-duration, andextractCompositionMetadataguards withisFinite). Element timing attrs missed the same policy.Change
Unparsable is treated as missing, which all of these paths already know how to handle:
parseTimingAttr(value, fallback)helper intimingCompiler.ts(the same policyextractResolvedMediaalready applies todata-duration), used at every timing-attribute read in both files.compileTagadditionally normalizes the compiled output: an unparsabledata-startis rewritten to0, and unparsabledata-end/data-durationare stripped so the element takes the existing recompute-or-resolver path. Downstream consumers (parser, runtime) never see the garbage values.parseHtmlfalls back to its existing defaults (start 0, the 5s default duration).No behavior change for valid inputs.
Tests
data-startnormalized to 0 withdata-endcomputed from it; unparsabledata-durationdropped and the element marked unresolved; unparsabledata-endstripped and recomputed; unparsabledata-media-startresolved to 0; directparseTimingAttrcases (including"Infinity"). All assert the compiled output contains noNaN.data-startparses as 0; unparsabledata-endfalls back to the 5s default instead of NaN;updateElementInHtmlrecomputesdata-endfrom 0 instead of writing NaN.bun run buildgreen.