fix(fal): bound music/video generation response reads#96886
Merged
Conversation
Contributor
|
Codex review: needs maintainer review before merge. Reviewed June 26, 2026, 8:37 PM ET / 00:37 UTC. Summary PR surface: Source +8, Tests -8. Total 0 across 4 files. Reproducibility: yes. Current main and v2026.6.10 still use Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused hardening once maintainers accept the 16 MiB successful-response metadata cap, with a credentialed fal smoke only if they need extra vendor compatibility confidence. Do we have a high-confidence way to reproduce the issue? Yes. Current main and v2026.6.10 still use Is this the best way to solve the issue? Yes. Reusing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 65fec9d787e3. Label changesLabel justifications:
Evidence reviewedPR surface: Source +8, Tests -8. Total 0 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
This was referenced Jun 26, 2026
The image, music, and video generation providers read successful fal API JSON bodies via an unbounded await response.json() after the HTTP status check. A hostile or buggy fal endpoint can stream an arbitrarily large success body with no Content-Length, forcing the runtime to buffer the entire payload in memory before parsing and risking OOM/hang. Route all three success-path reads through the shared readProviderJsonResponse helper, which reads the body through the same 16 MiB bounded reader used for binary responses and cancels the upstream stream on overflow. The video path preserves its existing FAL_VIDEO_MALFORMED_RESPONSE mapping for malformed JSON while letting the overflow error propagate. Tests updated to stream real Response bodies.
Alix-007
changed the title
This was referenced Jun 30, 2026
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
2 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 Problem This Solves
Resolves a problem where the fal image, music, and video generation providers read a successful fal API JSON response through an unbounded
await response.json()after the HTTP status check. A hostile, compromised, or buggy fal endpoint can stream an arbitrarily large success body with noContent-Length, forcing the runtime to buffer the entire payload in memory before parsing. This risks out-of-memory crashes or hangs on the media-generation path — affecting any agent or workflow that generates images, music, or video through the fal provider.This is the success path: the existing
assertOkOrThrowHttpError/ bounded error-body reader only guards non-2xx responses, so the oversized-body risk is on 2xx replies that reachresponse.json().Why This Change Was Made
All three success-path reads are routed through the shared
readProviderJsonResponsehelper (already used by other media providers, e.g. byteplus). It reads the body through the same 16 MiB bounded reader used for binary responses and cancels the upstream stream on overflow, then parses JSON. No new abstraction is introduced — this reuses existing plugin-SDK infrastructure.extensions/fal/image-generation-provider.ts(success read at the generation call)extensions/fal/music-generation-provider.ts(success read afterpostJsonRequest)extensions/fal/video-generation-provider.ts(fetchFalJson, the shared submit/status/result read)Boundary preserved: the video path keeps its existing
FAL_VIDEO_MALFORMED_RESPONSEmapping for malformed JSON while letting the new overflow error propagate unchanged, so existing behavior for malformed payloads is unaffected.User Impact
No change to normal behavior: well-formed fal responses parse exactly as before. When a fal endpoint returns an oversized success body, the provider now fails fast with a clear bounded error and tears down the connection instead of buffering unbounded memory. Operators running fal-backed media generation are protected from an OOM/hang failure mode.
Evidence
Behavior addressed: success-path untrusted JSON body is now bounded to 16 MiB; overflow throws and the TCP stream is cancelled.
Real environment tested: a real
node:httploopback server streams a 20 MiB JSON success body with noContent-Length; the real exported provider functions (generateImage,generateMusic,generateVideo) are driven against it over real TCP. Only the credential/config resolution and the fetch-guard transport are stubbed to point at loopback — the bounded read under test (readProviderJsonResponse) runs for real.Exact steps: drive each provider against the oversize server; assert it throws
... JSON response exceeds 16777216 bytes, that fewer than the full 20 MiB hit the wire (bytesSent < 20971520), and that the socket is torn down. Plus a negative control (the pre-fixresponse.json()buffers the whole body) and a happy-path control (small JSON still parses).Observed result (real terminal output):
The three numbers above show only ~17–19 MiB reached the wire before the bounded reader cancelled the stream, versus the full 20 MiB the unbounded path buffered in the negative control.
Existing suite: the fal provider tests pass after updating the test doubles to stream real
Responsebodies (the old doubles only implemented.json()):Typecheck:
tsgoover the extensions + test projects reports 0 errors.What was not tested: no live call against the real fal API (requires credentials); the proof exercises the bounded read over a real TCP loopback socket instead.
Label: security
AI-assisted.
Post-rebase scope update:
mainhas since landed the fal image response bound, so the image hunk has been dropped from this branch. This PR is now narrowed to the fal music and video success-path reads only —music-generation-provider.tsandvideo-generation-provider.ts(viafetchFalJson), both routed through the sharedreadProviderJsonResponse(16 MiB cap), with the video path preserving its existingFAL_VIDEO_MALFORMED_RESPONSEmapping. Theimage-generation-provider.tschange and the[image] …line in the Evidence block above no longer correspond to this PR's diff and can be disregarded.