Repository navigation
test(security): fix the subtitle force_style regression test (unblocks main CI) - #867
Merged
Merged
Conversation
…oice The subtitle force_style regression test added with the GHSA-cg89-3297-mwhg fix compared a frame rendered with the attack payload against a frame rendered with `FontName=Arial`, requiring them to be byte-identical. Those two frames differ for a benign reason: `_build_subtitle_style` strips `' \ : , ; [ ] =` from the value, so the payload collapses into a single inert font family (`Arialdrawtextfontsize30...`). fontconfig resolves "Arial" to an installed metric-compatible face and falls back to its default family for the sanitised payload, so the comparison measured typeface selection rather than filtergraph injection -- and failed on ubuntu-latest, which ships those aliases. Replace it with two assertions that test the property the advisory is about: 1. Structural: the generated force_style string gains no extra style entries from the payload, and every value is free of the characters that could close the quoted literal or open a new filter/option. 2. Behavioural: render the payload frame with the named canary file present, remove the file, and render again. Injection of a `drawtext=textfile=` filter would change the frame or make FFmpeg fail; identical frames prove the output cannot depend on local file contents. A payload naming a never-existent file is checked the same way, and a lit-pixel assertion keeps this from passing on two blank frames. The production fix is unchanged; only the test was measuring the wrong thing.
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.
Why
Main's CI has been red since the GHSA-cg89-3297-mwhg merge. One test out of ~2,230 fails:
test_subtitle_style_payload_does_not_read_local_file.The production fix is sound — the injection it closes is genuinely closed. The test is measuring the wrong thing.
What was wrong
The test rendered two frames and required them byte-identical:
FontName=ArialArial',drawtext=...textfile=canary.txt,drawtext=...text='_build_subtitle_stylestrips' \ : , ; [ ] =from every value, so the payload collapses into one inert font family —Arialdrawtextfontsize30fontcolorwhitetextfilecanary.txtdrawtextfontsize10text. There is no=,,or quote left for libass or the filtergraph parser to act on, so no second filter and no file read is possible.But the two frames still differ: fontconfig resolves
Arialto an installed metric-compatible face (Liberation Sans onubuntu-latest) and falls back to its default family for the unknown sanitised name. Different typeface, different glyph outlines, frames don't match. The assertion was comparing font resolution, not injection — which is why it passes on a host without those aliases and fails on the runner.What this changes
Only the test. Two assertions, both about the property the advisory is about:
force_stylestring gains no extra style entries from the payload, and every value is free of the characters that could close the quoted literal or open a new filter / option / style entry.drawtext=textfile=injection would change the frame or make FFmpeg fail outright; identical frames prove the output cannot depend on local file contents. A payload naming a never-existent file is checked the same way, and a lit-pixel assertion keeps the comparison from passing on two blank frames.Both are independent of which typeface the host happens to resolve.
Verification
pytest tests/tools/test_showcase_card_security.py— 24 passed.🤖 Generated with Claude Code