Skip to content

fix(openai-video-gen): bound video submit response JSON read at 16 MiB#96786

Closed
wangmiao0668000666 wants to merge 2 commits into
openclaw:mainfrom
wangmiao0668000666:fix/openai-video-gen-bounded-read
Closed

fix(openai-video-gen): bound video submit response JSON read at 16 MiB#96786
wangmiao0668000666 wants to merge 2 commits into
openclaw:mainfrom
wangmiao0668000666:fix/openai-video-gen-bounded-read

Conversation

@wangmiao0668000666

@wangmiao0668000666 wangmiao0668000666 commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

extensions/openai/video-generation-provider.ts:427 parses the Sora submit response with a raw await response.json() — no byte cap. A malicious or runaway Sora submit endpoint can return an oversized JSON body that exhausts process memory. The download path in the same file already caps at readResponseWithLimit — this closes the asymmetry.

Changes

  • extensions/openai/video-generation-provider.ts:427 — replace (await response.json()) as OpenAIVideoResponse with readProviderJsonResponse<OpenAIVideoResponse>(response, "OpenAI video generation failed").

Real behavior proof

  • Behavior addressed: unbounded response.json() on OpenAI video submit response; after the fix reads are capped at 16 MiB.

  • Real environment tested: real node:http server on 127.0.0.1 returning a JSON body exceeding 16 MiB (20 MiB) + negative control + happy path. Node v22.22.0.

  • Exact steps or command run after this patch:

    node --import tsx _proof_openai_video_gen_bounded_read.mts

  • Evidence after fix: ```
    === OpenAI video-generation JSON bounded read (cap=16777216) ===

    PASS hostile body: overflow; bytesSent=20971758; aborted=true
    PASS negative control: small body parsed (id=video_abc123)
    PASS happy path: valid JSON parsed (status=completed)

    === All OpenAI video-generation bounded-read assertions passed ===

  • Observed result after fix: 3/3 assertions pass. Hostile 20 MiB body triggers overflow at 16 MiB cap (bytesSent=20971758, aborted=true). Negative control: small body parsed (id=video_abc123). Happy path: valid JSON parsed (status=completed).

  • What was not tested: live Sora API call; cross-platform Node differences.

Out of scope

  • Other response.json() sites — covered by separate Alix-007-style per-surface PRs.

Risk

  • Very low: swap to readProviderJsonResponse — same error shape, 16 MiB default cap. Label "OpenAI video generation failed" matches the existing assertOkOrThrowHttpError label. The download path in the same file already uses readResponseWithLimit (same family).

Diff stats

1 file changed, 5 insertions(+), 1 deletion(-)

@wangmiao0668000666
fix(openai-video-gen): bound video submit response JSON read at 16 MiB
a758cdc 
Replace the unbounded (await response.json()) call in
extensions/openai/video-generation-provider.ts with
readProviderJsonResponse (16 MiB cap). The download path in the same
file already uses readResponseWithLimit — this closes the asymmetry.

Reuses the existing SDK helper already used by 15+ other providers.
@clawsweeper

clawsweeper Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: the helper swap is valid, but #96905 now covers the same OpenAI video create-submit read with stronger provider-path proof, a clean merge state, and Response-backed test-helper changes, so keeping both branches open adds duplicate maintainer choice rather than unique value.

Root-cause cluster
Relationship: superseded
Canonical: #96905
Summary: This PR and #96905 target the same OpenAI video create-submit unbounded JSON read; the later PR is open, clean, proof-sufficient, and has stronger test/proof coverage, so it is the canonical landing candidate.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Use #96905 as the canonical OpenAI video submit bounded-read fix, close this duplicate branch, and land exactly one maintainer-accepted 16 MiB cap decision.

So I’m closing this here and keeping the remaining discussion on #96905.

Review details

Best possible solution:

Use #96905 as the canonical OpenAI video submit bounded-read fix, close this duplicate branch, and land exactly one maintainer-accepted 16 MiB cap decision.

Do we have a high-confidence way to reproduce the issue?

Yes at source/proof level: current main reaches raw response.json() on the successful OpenAI video submit response, and the PR body includes terminal output from a real node:http server showing after-fix overflow rejection plus small-response controls. I did not rerun the proof because this review is read-only.

Is this the best way to solve the issue?

No, not as the best current landing path: the helper swap is correct, but #96905 is the stronger same-surface fix because it keeps the provider JSON reader real in tests and has proof through the exported OpenAI provider path.

Security review:

Security review cleared: Cleared: the diff narrows an unbounded provider response memory exposure and adds no dependency, workflow, credential, package, or supply-chain surface.

AGENTS.md: found and applied where relevant.

What I checked:

Likely related people:

  • steipete: git log -S shows Peter Steinberger introduced the OpenAI video provider path in feat(video): add provider support and discord fallback, including the submit response flow now being bounded. (role: feature-history contributor; confidence: medium; commits: 932194b7d58c; files: extensions/openai/video-generation-provider.ts)
  • ly-wang19: Current shallow-checkout blame for the OpenAI provider, provider HTTP helper, byte-limit reader, and shared provider HTTP mock points to the recent plugin classification refactor that carried these files into current main. (role: recent area contributor; confidence: medium; commits: c1336b6b412b; files: extensions/openai/video-generation-provider.ts, src/agents/provider-http-errors.ts, packages/media-core/src/read-response-with-limit.ts)
  • Alix-007: Live GitHub metadata shows Alix-007 has merged adjacent bounded success-response work in the same provider-hardening family and authored the open canonical replacement candidate for this exact OpenAI video submit path. (role: adjacent bounded-response contributor; confidence: medium; commits: cc124d2921b8; files: extensions/qwen/media-understanding-provider.ts, extensions/qwen/media-understanding-provider.test.ts, extensions/openai/video-generation-provider.ts)

Codex review notes: model internal, reasoning high; reviewed against 1bccd2930437.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 25, 2026
@wangmiao0668000666 @claude
fix(provider-http-test-mocks): add readProviderJsonResponse mock export
21ef5b9 
ClawSweeper found that the shared provider-http test mock does not export
readProviderJsonResponse, causing test failures for any PR importing it.

Add the mock to vi.hoisted(), the mock interface, vi.mock() exports, and
installProviderHttpMockCleanup(). Default implementation delegates to
response.json() to preserve existing test behavior.

Co-Authored-By: Claude <[email protected]>
@wangmiao0668000666

wangmiao0668000666 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Added readProviderJsonResponse to the shared provider-http-mocks.ts mock:

  1. Added readProviderJsonResponseMock to vi.hoisted() — delegates to response.json() (preserves existing test behavior)
  2. Added type declaration to ProviderHttpMocks interface
  3. Added export to vi.mock("openclaw/plugin-sdk/provider-http")
  4. Added cleanup in installProviderHttpMockCleanup()

Pre-flight: oxlint ✅, tsgo ✅, vitest 14/14 ✅

@clawsweeper

clawsweeper Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jun 25, 2026
Merged
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 25, 2026
@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jun 25, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jun 26, 2026
Merged
@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jun 26, 2026
Merged
@clawsweeper clawsweeper Bot closed this Jun 27, 2026
@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper autoclose is complete.

Reason: structured ClawSweeper close marker: close-required (sha=21ef5b9dd68c833210c9d2e2d37c5fe6bf1c8f0c)

Closed:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: openai merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@wangmiao0668000666