Skip to content

fix(vydra): bound control response reads#96875

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
Alix-007:fix/bound-vydra
Jun 29, 2026
Merged

fix(vydra): bound control response reads#96875
vincentkoc merged 1 commit into
openclaw:mainfrom
Alix-007:fix/bound-vydra

Conversation

@Alix-007

@Alix-007 Alix-007 commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Vydra success-path untrusted body handling read submit and job-poll control JSON with unbounded await response.json(), so a hostile, misconfigured, or SSRF-reachable endpoint could stream a no-Content-Length body indefinitely and force OpenClaw to buffer it before parsing, causing OOM pressure or a hang on image, video, and speech generation paths.

The affected reads were:

  • extensions/vydra/image-generation-provider.ts image submit JSON
  • extensions/vydra/speech-provider.ts speech submit JSON
  • extensions/vydra/video-generation-provider.ts video submit JSON
  • extensions/vydra/shared.ts shared job poll JSON used by image/video async jobs

Changes

  • Replace those four success-path response.json() calls with the shared readProviderJsonResponse helper from openclaw/plugin-sdk/provider-http.
  • Keep the fix plugin-scoped and reuse the existing 16 MiB provider JSON cap; no new abstraction.
  • Preserve the existing assertOkOrThrowHttpError behavior, job id/status extraction, asset download path, and release() cleanup.
  • Add focused Vydra regression tests for oversized image submit, speech submit, video submit, and shared poll JSON responses.

Real behavior proof

  • Behavior addressed: Vydra submit and job-poll control JSON from an untrusted external endpoint must not be buffered whole on the success path. Oversized no-Content-Length bodies must stop at the 16 MiB cap, cancel the stream, and throw a bounded ...exceeds 16777216 bytes error. Small valid JSON must continue to parse normally.
  • Real environment tested: A real node:http server (createServer) bound to 127.0.0.1, streaming JSON in 64 KiB chunks with no Content-Length header and a would-stream size of about 64 MiB. The script drove the real exported buildVydraImageGenerationProvider().generateImage() over a real loopback socket through the real postJsonRequest, guarded fetch path, and readProviderJsonResponse. The shared poll helper was exercised by returning a queued submit payload and then streaming the oversized poll response from /jobs/<id>. Node v22.22.0.
  • Exact steps: node --import tsx scratchpad/vydra-bound-proof.mts — start the streaming server → call real Vydra image submit and assert bounded throw + server-side socket abort near the cap → call real Vydra image job poll path and assert bounded throw + socket abort near the cap → run a negative control using old unbounded fetch().arrayBuffer() against the same streaming body → call the real provider against a small valid JSON response and asset response.
  • Observed result: submit JSON threw Vydra image generation: JSON response exceeds 16777216 bytes and aborted after 20,119,564 bytes; shared poll JSON threw Vydra job status: JSON response exceeds 16777216 bytes and aborted after 18,022,412 bytes. The negative control buffered the full 67,108,866 bytes, proving the cap is load-bearing. The small happy path returned the generated image payload and metadata.
  • What was not tested: I did not hit the live Vydra service; live endpoints would not reproduce a hostile oversized body. The real socket proof covers the transport and cancellation behavior. The TCP proof uses the image provider to exercise submit and shared poll because Vydra image requests expose the request-level SSRF policy needed for local loopback proof; speech/video submit bounding is covered by the focused in-repo Vitest regressions using the same shared reader and error text. The proof script is not committed.

Evidence

Real node:http terminal proof (real loopback socket, real exported Vydra provider, real bounded reader):

[proof] real node:http server on http://127.0.0.1:39391/api/v1, cap=16777216 bytes, would-stream≈67108864 bytes per overflow route

PASS  image submit JSON: overflow throws bounded error :: threw=true msg="Vydra image generation: JSON response exceeds 16777216 bytes"
PASS  image submit JSON: stream cancelled near 16MiB :: aborted=true bytesSent=20119564 (<33554432)
PASS  shared poll JSON: overflow throws bounded error :: threw=true msg="Vydra job status: JSON response exceeds 16777216 bytes"
PASS  shared poll JSON: stream cancelled near 16MiB :: aborted=true bytesSent=18022412 (<33554432)
PASS  negative control: OLD unbounded read buffers PAST the 16MiB cap :: buffered=67108866 bytes (> 16777216)
PASS  negative control consumed far more than bounded reads :: unbounded buffered=67108866 vs image submit sent≈20119564
PASS  happy path: small submit JSON still parses and asset download still works :: jobId=job-happy bytes=8 status=completed

[proof] 7 PASS / 0 FAIL
[proof] ALL PASS

Focused Vitest after rebase:

node scripts/run-vitest.mjs extensions/vydra/image-generation-provider.test.ts extensions/vydra/video-generation-provider.test.ts extensions/vydra/speech-provider.test.ts

Test Files  3 passed (3)
Tests  16 passed (16)
[proof] focused Vydra Vitest exit=0

Type and format checks after rebase:

pnpm tsgo:extensions
[proof] post-rebase tsgo:extensions exit=0

pnpm tsgo:test:extensions
[proof] post-rebase tsgo:test:extensions exit=0

pnpm format:check -- extensions/vydra/image-generation-provider.ts extensions/vydra/image-generation-provider.test.ts extensions/vydra/shared.ts extensions/vydra/speech-provider.ts extensions/vydra/speech-provider.test.ts extensions/vydra/video-generation-provider.ts extensions/vydra/video-generation-provider.test.ts
All matched files use the correct format.

git diff --check
# clean

Autoreview after rebase:

.agents/skills/autoreview/scripts/autoreview --mode branch --base upstream/main
autoreview clean: no accepted/actionable findings reported

Label: security

AI-assisted.

Post-rebase scope update: main has since landed the Vydra image submit bound, so this PR no longer changes image-generation-provider.ts (only a focused image regression test remains). This PR now bounds the remaining success-path reads: shared.ts (shared job-poll JSON used by image/video async jobs), speech-provider.ts (speech submit), and video-generation-provider.ts (video submit) — all routed through the shared readProviderJsonResponse (16 MiB cap). The image-generation-provider.ts source line in the bullet list above can be disregarded.

@clawsweeper

clawsweeper Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 26, 2026, 8:38 PM ET / 00:38 UTC.

Summary
The PR routes Vydra shared job-poll, speech-submit, and video-submit success JSON parsing through readProviderJsonResponse and adds oversized-response regression coverage, while only adding image-submit test coverage for the already-bounded source path.

PR surface: Source +6, Tests +79. Total +85 across 6 files.

Reproducibility: yes. at source/proof level. Current main and v2026.6.10 still show raw Vydra response.json() reads on the remaining poll, speech, and video paths, and the PR body includes loopback HTTP proof for oversized no-Content-Length bodies after the patch.

Review metrics: 1 noteworthy metric.

  • Vydra control JSON coverage: 3 prod parse sites changed, 1 prod parse site already bounded on main, 4 oversized regressions added. This distinguishes the PR's remaining unique runtime value from the Vydra image-submit path already fixed by merged image-provider hardening.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] Legitimate Vydra success control JSON bodies above the shared 16 MiB cap would now fail early instead of buffering, so maintainers need to accept that compatibility boundary before merge.
  • [P1] The contributor proof uses loopback HTTP rather than a hosted Vydra call; it is strong for hostile oversized bodies and a small local happy path, but it does not prove current hosted Vydra service semantics.

Maintainer options:

  1. Accept The Shared Cap (recommended)
    Land after required checks if maintainers accept that Vydra success control JSON above 16 MiB now fails early instead of buffering.
  2. Ask For Hosted Happy-Path Proof
    Request a live Vydra happy-path smoke only if maintainers want hosted-service confirmation beyond the loopback overflow proof.
  3. Pause The Hardening
    Hold or close the PR if maintainers do not want the shared provider JSON cap applied to Vydra control responses yet.

Next step before merge

  • Maintainer review is the remaining action because the patch is mechanically correct but intentionally changes oversized Vydra success control JSON behavior.

Security
Cleared: The diff adds no dependency, workflow, secret, package, or supply-chain surface and replaces unbounded external provider JSON parsing with the existing bounded helper.

Review details

Best possible solution:

Land the plugin-scoped shared-reader change after required checks if maintainers accept the 16 MiB control-JSON cap for Vydra.

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

Yes at source/proof level. Current main and v2026.6.10 still show raw Vydra response.json() reads on the remaining poll, speech, and video paths, and the PR body includes loopback HTTP proof for oversized no-Content-Length bodies after the patch.

Is this the best way to solve the issue?

Yes. Reusing readProviderJsonResponse inside the Vydra plugin is the narrowest maintainable fix because it preserves the plugin owner boundary, uses the existing shared provider JSON cap, and avoids new config or duplicate local readers.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 65fec9d787e3.

Label changes

Label justifications:

  • P2: The PR fixes Vydra plugin-level provider response hardening with limited blast radius and no evidence of an active widespread outage.
  • merge-risk: 🚨 compatibility: The patch changes existing oversized Vydra success control JSON behavior from unbounded buffering to fail-closed at the shared 16 MiB provider JSON cap.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes terminal proof from a real loopback HTTP server exercising Vydra oversized-response cancellation, a negative unbounded-read control, a small happy path, and focused validation output.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal proof from a real loopback HTTP server exercising Vydra oversized-response cancellation, a negative unbounded-read control, a small happy path, and focused validation output.
Evidence reviewed

PR surface:

Source +6, Tests +79. Total +85 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 3 9 3 +6
Tests 3 79 0 +79
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 88 3 +85

What I checked:

Likely related people:

  • steipete: GitHub commit history shows this account introduced the Vydra media provider and carried later Vydra request-helper and live-coverage work in the same files. (role: feature introducer and recent area contributor; confidence: high; commits: 9b2b22f35010, 8fdaa5da4936, b1c95a82a009; files: extensions/vydra/shared.ts, extensions/vydra/image-generation-provider.ts, extensions/vydra/speech-provider.ts)
  • vincentkoc: Recent provider hardening bounded Vydra and Comfy media downloads in the same Vydra shared/provider files. (role: recent adjacent contributor; confidence: medium; commits: 0902ee723b76; files: extensions/vydra/shared.ts, extensions/vydra/image-generation-provider.ts, extensions/vydra/speech-provider.ts)
  • hugenshen: The merged image-generation JSON response hardening that already bounded the Vydra image-submit path on current main was authored by this account. (role: adjacent current-main fix author; confidence: medium; commits: 527f8f0cbba1; files: extensions/vydra/image-generation-provider.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. P2 Normal backlog priority with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 26, 2026
This was referenced Jun 26, 2026
Merged
Merged
@mushuiyu886 mushuiyu886 mentioned this pull request Jun 26, 2026
Merged
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Jun 27, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 27, 2026
Closed
@vincentkoc vincentkoc self-assigned this Jun 29, 2026
@vincentkoc
vincentkoc merged commit 74a9beb into openclaw:main Jun 29, 2026
103 checks passed
@github-actions github-actions Bot mentioned this pull request Jun 29, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Closed
@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jul 9, 2026
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

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: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: S 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.

2 participants

@Alix-007 @vincentkoc