feat(upload): reject mp4 uploads missing the moov atom - #1804
Open
giladresisi wants to merge 2 commits into
Open
feat(upload): reject mp4 uploads missing the moov atom#1804giladresisi wants to merge 2 commits into
giladresisi wants to merge 2 commits into
Conversation
What it validates: a top-level moov box exists in the uploaded mp4. ISO/IEC 14496-12 (the ISO base media file format underlying mp4, publicly available at https://standards.iso.org/ittf/PubliclyAvailableStandards/ - see the Movie Box definition, section 8.2.1: 'exactly one MovieBox shall be present') makes moov mandatory: it is the index holding every track's codec config and sample tables, and no player or platform can decode a single frame without it. Encoders write moov last (or rewrite the file with moov first for faststart), so a file grabbed before the encoder finished has only ftyp + free + mdat-with-size-0 and is permanently unplayable. That is exactly the observed customer issue this addresses: an API automation uploaded renders before the generator finished, Postiz stored them (black tiles in the media gallery, since the <video> preview cannot decode them either), and the posts failed days later at publish time with Pinterest's 'The file is corrupted and cannot be uploaded'. Two such production files showed the identical unfinalized signature; a successfully published one had a proper moov. The check walks top-level boxes only (a handful of size-field hops, no byte scanning, handles 64-bit largesize), so cost is negligible. What it does NOT validate: that the file is decodable. A structurally complete mp4 with a corrupt or empty video track still passes; catching that class needs ffprobe-level demuxing. It also does not cover the hosted web-UI upload path, which goes browser -> R2 via Uppy S3 multipart, so the server never sees the bytes; covered here are the paths automation uses (public API /upload and /upload-from-url, the agent upload tool, and the XHR/self-hosted routes through the validation pipe). That asymmetry matches the failure mode: a human uploads a file that finished rendering and immediately sees a black preview if not, while automation races the encoder and uploads mid-encode - which is why this only ever surfaced via API/MCP. Verified e2e on a local run: public API /upload returns 201 for a valid mp4 and 400 for a moov-less one; the MCP uploadFromUrlTool uploads a known-good production file and returns the graceful error for the customer's actual corrupt file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
| size = Number(largeSize); | ||
| } | ||
| if (size < 8) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Author
There was a problem hiding this comment.
Confirmed and fixed in 22c51d3. Note the fix keeps the generic minimum at 8 for ordinary 32-bit boxes (8 is legal there) and enforces the 16-byte minimum only inside the largesize branch. Verified with a crafted file (largesize=9 with a misaligned moov planted after it) which is now rejected, while the real-world valid/corrupt fixtures behave as before.
A size==1 box carries its real size in a 64-bit largesize field, so its header is 16 bytes and any largesize under 16 is malformed; the generic size<8 guard let 8-15 through, misaligning the box walk on crafted input. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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 kind of change does this PR introduce?
Feature (upload validation)
Why was this change needed?
A customer's API automation uploads video renders before the generator finishes writing them. Postiz accepted the files, showed them as black tiles in the media gallery, and the posts failed days later at publish time with Pinterest's "The file is corrupted and cannot be uploaded" (investigated alongside #1802/#1803). Two failed posts' production files showed the identical unfinalized signature —
ftyp+free+mdatwith size 0 and nomoovbox — while a successfully published one had a propermoov.What this validates: that a top-level
moovbox exists in an uploaded mp4. Per ISO/IEC 14496-12 (the ISO base media file format underlying mp4; publicly available at https://standards.iso.org/ittf/PubliclyAvailableStandards/ — Movie Box, §8.2.1: "exactly one MovieBox shall be present"),moovis mandatory: it is the file's index (codec config + sample tables), and nothing can decode a single frame without it. Encoders write it last (or rewrite with it first for faststart), so its absence is the definitive signature of a file grabbed mid-encode. The check hops top-level box headers only (no byte scanning, handles 64-bitlargesize), so cost is a handful of reads regardless of file size.What it does not validate: decodability — a structurally complete mp4 with a corrupt/empty track still passes (that would need ffprobe-level demuxing). It also does not cover the hosted web-UI path, which uploads browser → R2 via Uppy S3 multipart, so the server never sees the bytes. Covered are the paths automation uses: public API
/uploadand/upload-from-url, the agentuploadFromUrlTool, and the XHR/self-hosted routes throughCustomFileValidationPipe. That asymmetry matches the failure mode: a human uploads a finished render (and would instantly see a black preview otherwise), while automation races the encoder — which is why this only ever surfaced via API/MCP.Verified e2e on a local run: public API
/upload→ 201 for a valid mp4, 400 with the new message for a moov-less one; MCPuploadFromUrlTool→ uploads a known-good production file, returns the graceful{error}for the customer's actual corrupt file.Other information:
Rejected uploads get an actionable message telling the user to wait for the render to complete and re-upload. Existing corrupted files already in storage are unaffected (they keep failing at publish time as today).
Checklist:
🤖 Generated with Claude Code