feat(producer): fail typed when a media source is a text document, not media - #3037
Open
vanceingalls wants to merge 3 commits into
Open
feat(producer): fail typed when a media source is a text document, not media#3037vanceingalls wants to merge 3 commits into
vanceingalls wants to merge 3 commits into
Conversation
…tion STUDIO-5433 defense: when the downloaded media file begins with <!DOCTYPE, <html, or <?xml, throw a typed HtmlNotVideoError naming the offending src instead of letting ffprobe emit an inscrutable moov-atom-not-found on a plain HTML page. Complements #3033 diagnosability layer. Root-cause EF fix ships separately. Signed-off-by: Via <vance@heygen.com>
Fallow audit reportFound 1 finding. Details
Generated by fallow. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * read from. Exported for direct unit-test coverage. | ||
| */ | ||
| export async function assertNotHtmlPayload(filePath: string, src: string): Promise<void> { | ||
| const fh = await openFile(filePath, "r"); |
Review follow-up on the STUDIO-5433 defense.
Correctness
- The sniff ran above the documented video/audio failure split, so an
<audio> src that resolved to an HTML payload aborted the whole render
instead of degrading to duration 0. It now runs inside the same try, so
video surfaces the typed error while audio still drops out, with a
warning naming the element.
- Raw fs errors (EISDIR on a directory src, EACCES, the existsSync->open
ENOENT race, EMFILE) escaped and failed the compile with an unclassified
error carrying an unredacted temp path. The sniff is now a classifier that
never throws: an unreadable file reports "not markup" and the real probe
produces the real error.
- Elements whose duration the compiler never resolves (a data-end video, a
looping audio) skipped the sniff entirely, so the original ffprobe error
still escaped, and looping audio was reported as owner "system" after
every frame had been captured. Video is now caught in the asset preflight,
which sees every local src regardless of authored timing; audio is
classified per-element in audioMixer as source/invalid_media/owner "user",
keeping audio failures non-fatal as they already were.
- Detection is a byte-level check for a leading "<" (BOM-, whitespace- and
NUL-tolerant, looped read) instead of a <!doctype|<html|<?xml string
prefix, which missed a NUL-prefixed payload, >256B of leading whitespace,
UTF-16-encoded HTML, and a prolog-less <svg. No supported container starts
with "<", so the allowlist no longer grows per payload shape.
- finally { await fh.close() } could replace the in-flight typed error with
the close error.
Routing and privacy
- MARKUP_NOT_MEDIA is now in SAFE_RENDER_ERROR_CODES, the Lambda terminal
name map, the CDK and SAM non-retryable plan lists, and the Cloud Run
non-retryable set, and the class carries owner/retryable. Previously the
API emitted errorCode: undefined and a deterministic authoring bug burned
the full distributed retry budget.
- The message no longer carries 32 raw payload bytes or the src.
redactTelemetryString preserves host and path for HTTP srcs, so
per-tenant CDN paths reached a message the server forwards to clients.
Correlation is a sha256 element fingerprint, matching
AssetMediaTypeMismatchError.
- The message names both causes (unresolved nested-composition URL, or an
HTML/XML error page served as 200) rather than misdiagnosing an S3 403
body as an authoring bug.
Tests
- Byte-level detection is unit-tested in engine: markup shapes, BOMs,
UTF-16, nine container signatures, unreadable inputs.
- Replaced the tautological assertions. The old checks for "html" in and
"moov" absent from a fixed message template could not fail for any input.
- New coverage for audio degradation, the audioMixer classification, the
preflight video/image/audio split, and the API error metadata.
- The sibling htmlCompiler.mediaType failure was a vitest-under-bun runner
mismatch, not a missing ffmpeg binary. It passes, including the 4-wide
probe-semaphore invariant the sniff now runs inside.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vanceingalls
changed the title
A source that answers with a JSON error body still reached ffprobe and
produced `moov atom not found`. Replicate returns
`{"detail": "requested file not found"}` for a dead asset, and a gateway
in front of it can relay that body with a success status.
The sniff now treats `<`, `{`, or `[` as the opening byte of a text
document. No supported container starts with any of them, so this is the
same trade as before: three bytes instead of an allowlist that grows one
entry per payload shape observed in production.
Renamed accordingly, since the class now covers JSON as well as markup:
MARKUP_NOT_MEDIA -> NOT_MEDIA_PAYLOAD, MarkupNotMediaError ->
NotMediaPayloadError, markupPayload.ts -> notMediaPayload.ts. Registry
entries in the Lambda name map, the CDK and SAM plan lists, the Cloud Run
set, and SAFE_RENDER_ERROR_CODES move with it.
Also documents the reachability boundary on the error class: only a 2xx
response gets here. `downloadToTemp` rejects 404/410 as `http_not_found`
before writing a byte, and every ffprobe input is local because
videoFrameExtractor downloads http srcs first. So the shapes this
classifies are soft-404 and interstitial HTML, S3/CloudFront error
documents, and JSON API error bodies -- each served with a success
status. A genuine 404 surfaces as a download failure, not as this error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vanceingalls
changed the title
jerrai-bot-heygen
left a comment
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Reviewed. Solid, well-tested fix for a real production incident (STUDIO-5433).
Verified independently (not just the PR body):
- Hand-traced
isNotMediaPayload's byte-sniff logic (notMediaPayload.ts) against every documented container signature (mp4/mov box-size prefix, Matroska EBML, Ogg, RIFF, MPEG-TS, FLAC, MP3/ID3, ADTS, MPEG-PS) — none share a first-meaningful-byte with</{/[, so no false positives. The BOM-then-skip-then-break loop correctly handles interleaved-NUL UTF-16 (both endiannesses) and the 512-byte sniff-window-overrun case (padding > window → correctly "unknown", not a misread). - Traced the
resolveMediaDurationintegration inhtmlCompiler.ts: a video-tagged error propagates (hard fail before ffprobe), an audio-tagged error degrades to duration 0 + warns — matches the documented pre-existing audio/video split, both directions covered inhtmlCompiler.test.ts. - Traced
preflightCompositionAssetMediaTypes'ssniffableReferencesvideo-only filter — an audio-only reference at the same path correctly skips the new check and falls through to the existing (unchanged) probe-failure handling; confirmed against theassetMediaType.test.tscases including the SVG-image exemption. - The
github-advanced-security/fallowunused-class-memberfinding onHtmlNotVideoError.codeis from a stale commit (8a11e977…) that predates the current head (349c066a…) — that class doesn't exist at head anymore (confirmed viasearch_code+ a direct file read), so the finding is stale and doesn't apply to what's actually being merged.
Two things worth flagging, not blockers on the logic itself:
mergeable_state: dirty— will need a rebase against currentmainbefore merge; base is pinned ata99caad…and main has moved since (a couple of release/version-bump commits), most likely just a routine collision rather than a real conflict with this PR's own changes, but worth confirming at rebase time.- No CI test-run checks are visible on this PR from where I sit (only
WIPand a skippedMintlify Deployment) — the PR body's claimed pass counts (producer 38 files, engine 1406 tests, aws-lambda 140, gcp-cloud-run 101) are self-reported; I couldn't independently confirm they ran green via GitHub checks.
— Review by Jerrai
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
3 participants
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.
What
Refuses to probe a media source that is actually a text document (HTML, XML, SVG, JSON), failing with a typed
NotMediaPayloadError(code: NOT_MEDIA_PAYLOAD,owner: "user",retryable: false) instead of letting ffprobe emit an inscrutable[mov,mp4,m4a,3gp,3g2,mj2 @ ...] moov atom not found.Why
STUDIO-5433 — enterprise Maximus customer (
erictburkey@maximus.com, space5457a018a47644fc8b57f9832f20cf15) was blocked with "moov atom not found" on videoMTS - Cancel Work Order Training(57a488a2585f4c1fbed86d4671cbb4a4). Root cause (via CloudTrail + Athena on the actual failing S3 object): the a-roll element withtype: "hyperframes"hadcontent.srcpointing at astreamed-preview.htmlURL — a legit 6.5 KB<!DOCTYPE html>page, NOT an MP4. ffprobe's[mov,mp4,m4a,...]prefix in the error was just its default probe order, not the file's actual format. The same signature also fired on a second unrelated user Aug 4 17:40:35Z via/v2/distributed/plan-s3— cross-user pattern, not a one-off.Reachability: only a 2xx response gets here
Worth stating precisely, because it bounds what this can and cannot classify.
downloadToTemprejects a 404/410 asUrlDownloadError("http_not_found")before writing a byte (urlDownloader.tsclassifyHttpFailure), and every ffprobe input is local —videoFrameExtractor.ts:1420-1427downloads an http src first. So a genuine 404 surfaces as a download failure or a silent duration-0 drop, never asmoov atom not found.The payloads that do reach ffprobe are served with a success status: a soft-404 or interstitial HTML body, an S3/CloudFront error document, or a JSON API error body relayed with a 200. Those are what this classifies.
Relationship to the EF fix
experiment-framework#44667 (merged) mirrors non-trusted external media URLs into S3 at compose time and fails fast on an unfetchable URL. It removes the authoring source of dead external URLs for new Zephyr compositions. This PR still covers what it leaves:
c6f32d10…. Renders of those still hit the class.<sha>.binunder a trusted host — permanently. That is the shape of the original incident, and after the merge this PR is what catches it at render..amazonaws.com,.heygen.ai,.cloudfront.netare never mirrored.compose_addressable_artifact; CLI projects, Studio, and hand-authored compositions don't pass through it.Also complements #3033, which adds the src URL to all ffprobe errors; this PR fails before ffprobe for the non-media-payload subset.
How
Detection lives in
packages/engine/src/utils/notMediaPayload.tsso all three call sites share one classifier.Detection —
isNotMediaPayload(filePath)reads up to 512 bytes (looped, so a short read on NFS/FUSE can't truncate the window), skips a UTF-8/UTF-16 BOM plus leading whitespace and NUL bytes, and reports whether the first meaningful byte is<,{, or[.Three bytes instead of a shape allowlist:
<!doctype,<html,<?xml, a prolog-less<svg, an S3<Error>body, and Replicate's{"detail": "requested file not found"}all start with one of them, and no supported container does — mp4/mov open with a box size, Matroska/WebM1A 45 DF A3, OggOggS, RIFFRIFF, MPEG-TS0x47, FLACfLaC, ADTSFF Fx, MP3ID3. An allowlist needs a new entry per payload shape observed in production.It never throws. An unreadable file — EISDIR on a directory src, EACCES, a temp file evicted between
existsSyncand the read, EMFILE — reports "not a document" so the real probe still surfaces the real error. This is a classifier, not a gate.Call sites
htmlCompiler.resolveMediaDurationtry, so<video>surfaces the typed error and<audio>still degrades to duration 0 (with a warning naming the element) — the documented audio/video split is preservedpreflightCompositionAssetMediaTypesdata-endvideo the compiler never resolvesaudioMixerpreparesource/invalid_media/owner: "user"failure, replacing aprepare/ffmpeg_failed/owner: "system"that fired after every frame had been capturedImage sources are exempt: an
<img>may legitimately be an SVG, which ffprobe reads through itssvg_pipedemuxer.Message discipline mirrors the sibling
AssetMediaTypeMismatchError: bounded text, a sha256 element fingerprint for correlation, and never the authored src or the payload's own bytes — producer forwardserror.messageto API clients, andredactTelemetryStringpreserves host and path for HTTP srcs. It names both causes (an unresolved nested-composition URL, or an error page / API error body carrying a success status) rather than pointing on-call at an authoring bug when an expired signed URL is the actual cause.Routing —
NOT_MEDIA_PAYLOADis registered inSAFE_RENDER_ERROR_CODES, the Lambda terminal-name map, the CDK and SAM non-retryable plan lists, and the Cloud Run non-retryable set, so a deterministic authoring bug does not burn the distributed retry budget.Overhead: one 512-byte read per probed source. No new dependencies.
Not covered
A remote (
https:) video src carrying an authoreddata-end: the preflight skips remote sources and the compiler never resolves its duration, so nothing downloads it before extraction. Closing that gap means moving the download earlier and is left out deliberately.Test plan
packages/engine/src/utils/notMediaPayload.test.ts— 30 cases: document shapes (doctype, bare<html>, XML prolog, prolog-less<svg, S3<Error>, leading comment, JSON object, JSON array), UTF-8 and UTF-16 LE/BE BOMs, a leading NUL run, whitespace overrunning the sniff window, nine real container signatures, a container containing a document byte deeper in, an empty file, a directory, and a missing path. Plus the error's routing metadata, fingerprint correlation, and that neither the src nor a token in the payload reaches the message.htmlCompiler.test.ts—<video>aborts with the typed error before ffprobe;<audio>drops to duration 0 with a warning and the render continues.assetMediaType.test.ts— preflight rejects a document under video, leaves audio to the mixer, leaves an SVG image alone, and reports the document verdict ahead of the type mismatch the same file also produces.audioMixer.test.ts— a document audio source classified assource/invalid_media/owner: "user"without reaching ffmpeg.server.notMediaErrorCode.test.ts— the API emitserrorCode/errorOwner/retryablerather thanundefined.Green: producer unit lane 38 files, engine 1406 tests, aws-lambda 140, gcp-cloud-run 101. Producer integration matches baseline —
crossWorkerIdempotencyfails 2 tests identically on the parent commit (localcaptureModeexpectsbeginframe), verified by re-running against the stashed tree.Correcting the earlier test plan on this PR: the
htmlCompiler.mediaType.test.tsfailure was not a missing ffmpeg binary. Both binaries are present; that file imports fromvitestand fails underbun testwithvi.waitFor is not a function. It is the only suite asserting the 4-widesharedMediaProbeSemaphoreinvariant acrosscompileForRender— it passes under vitest, including with the sniff now running inside a probe slot.Signed-off-by: Via vance@heygen.com