Skip to content

fix(google): bound JSON response reads to prevent OOM#96324

Closed
lsr911 wants to merge 3 commits into
openclaw:mainfrom
lsr911:fix/google-bounded-json-response
Closed

fix(google): bound JSON response reads to prevent OOM#96324
lsr911 wants to merge 3 commits into
openclaw:mainfrom
lsr911:fix/google-bounded-json-response

Conversation

@lsr911

@lsr911 lsr911 commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Replace unbounded await response.json() with readResponseWithLimit (1 MiB cap)
across Google embedding-batch, image-generation, media-understanding, oauth.token,
oauth.project, and speech providers (11 call sites in 6 files) — the symmetric
Google-endpoint counterpart to bound-stream work already merged for Google prompt
cache (#95417) and other providers (#95420, #95218).

A malicious or faulty Google API endpoint returning an oversized JSON response body
would previously buffer the entire body in memory before parsing. The Google
video-generation module already uses readResponseWithLimit for downloads (#95417
handles the prompt-cache path), but these 6 modules were still unbounded.

Evidence

Summary

Bound 11 unbounded JSON response reads in Google providers with 1 MiB cap.

Changes

  • extensions/google/embedding-batch.ts — bound file upload, batch status, and
    result reads (3 call sites)
  • extensions/google/image-generation-provider.ts — bound image generation
    response read
  • extensions/google/media-understanding-provider.ts — bound media description
    response read
  • extensions/google/oauth.token.ts — bound token exchange JSON response read
  • extensions/google/oauth.project.ts — bound 5 JSON response reads (userinfo,
    project list, error payload, token refresh, onboarding LRO)
  • extensions/google/speech-provider.ts — bound speech synthesis response read

@clawsweeper

clawsweeper Bot commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 25, 2026, 5:03 AM ET / 09:03 UTC.

Summary
The PR replaces Google plugin response.json() success reads with readResponseWithLimit plus 1 MiB caps across embedding batch, image generation, media understanding, OAuth token/project discovery, and speech paths.

PR surface: Source +72. Total +72 across 6 files.

Reproducibility: yes. source inspection is enough to reproduce the review-relevant behavior: current main has the unbounded response.json() calls, and the PR head has a compile-breaking duplicate import. I did not run an oversized-response harness in this read-only review.

Review metrics: 1 noteworthy metric.

  • Google success JSON caps: 11 changed to 1 MiB. The cap count matters because the diff changes provider and auth runtime behavior, including media-bearing responses where the payload itself can be large.

Stored data model
Persistent data-model change detected: serialized state: extensions/google/embedding-batch.ts, serialized state: extensions/google/image-generation-provider.ts, serialized state: extensions/google/media-understanding-provider.ts, serialized state: extensions/google/oauth.project.ts, serialized state: extensions/google/oauth.token.ts, serialized state: extensions/google/speech-provider.ts, and 2 more. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • Remove the duplicate OAuth import and unused cap constant so type and lint checks can pass.
  • Reuse readProviderJsonResponse or an equivalent shared helper with explicit caps sized to each Google response surface.
  • [P1] Add redacted after-fix proof for oversized cancellation plus normal OAuth, metadata, image, and speech responses.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides static checks and prior-art rationale only; it needs redacted terminal output, logs, or an artifact showing oversized Google JSON cancellation and normal responses before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The current head fails prod type and lint checks because oauth.token.ts declares readResponseWithLimit twice and leaves an unused cap constant.
  • [P1] A fixed 1 MiB ceiling can reject valid Google image and TTS responses that include base64 media in the JSON body, changing behavior current users would get today.
  • [P1] The contributor has not supplied after-fix real behavior proof showing both oversized cancellation and normal small/media responses.
  • [P1] The PR reimplements bounded JSON parsing at each call site instead of using readProviderJsonResponse, creating a second provider JSON error/overflow path that can drift from the SDK contract.

Maintainer options:

  1. Repair helper usage, caps, and proof (recommended)
    Use the shared bounded JSON helper, remove the OAuth compile errors, choose payload-appropriate caps for media-bearing responses, and require redacted overflow plus happy-path proof before merge.
  2. Accept a hard cap knowingly
    Maintainers could intentionally accept stricter Google response ceilings, but only after the compile errors are fixed and the cap choice is documented as an upgrade-sensitive compatibility decision.
  3. Pause for a narrower replacement
    If proof or cap sizing remains unresolved, pause this branch and preserve the useful hardening in a narrower replacement PR.

Next step before merge

  • [P1] Human handling is needed because the external PR lacks contributor-supplied real behavior proof and requires maintainer cap-policy acceptance; ClawSweeper should not auto-repair until the proof gate is satisfied.

Security
Cleared: No new dependency, workflow, permission, secret-handling, package-resolution, or supply-chain surface was introduced; the blockers are functional correctness, compatibility, and proof.

Review findings

  • [P1] Remove the duplicate OAuth bounded-reader import — extensions/google/oauth.token.ts:7-12
  • [P1] Use media-sized caps for base64 Google responses — extensions/google/image-generation-provider.ts:47
  • [P1] Route provider JSON reads through the shared helper — extensions/google/embedding-batch.ts:132-137
Review details

Best possible solution:

Keep the Google bounded-read hardening, but route provider success JSON through the shared helper with explicit per-surface caps, remove the OAuth compile errors, size media caps to valid base64 payloads, and require redacted real behavior proof before merge.

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

Yes, source inspection is enough to reproduce the review-relevant behavior: current main has the unbounded response.json() calls, and the PR head has a compile-breaking duplicate import. I did not run an oversized-response harness in this read-only review.

Is this the best way to solve the issue?

No. Bounded reads are the right direction, but this PR is not the best mergeable fix until it uses the shared helper, repairs the OAuth token compile error, sizes media caps correctly, and adds real behavior proof.

Full review comments:

  • [P1] Remove the duplicate OAuth bounded-reader import — extensions/google/oauth.token.ts:7-12
    oauth.token.ts imports readResponseWithLimit twice and leaves GOOGLE_OAUTH_JSON_RESPONSE_MAX_BYTES unused, which CI reports as TS2300/TS6133. Remove the duplicate import and unused constant so the Google OAuth token module compiles.
    Confidence: 0.99
  • [P1] Use media-sized caps for base64 Google responses — extensions/google/image-generation-provider.ts:47
    This caps the entire Google image JSON response at 1 MiB before the parser reaches the inline base64 image data; the speech path has the same base64-audio shape. Use per-surface limits that preserve normal image and TTS payloads while still bounding hostile responses.
    Confidence: 0.92
  • [P1] Route provider JSON reads through the shared helper — extensions/google/embedding-batch.ts:132-137
    openclaw/plugin-sdk/provider-http already exports readProviderJsonResponse<T>(response, label, { maxBytes }), which wraps readResponseWithLimit and standardizes malformed JSON errors. Reimplementing that pattern at every call site creates a competing provider JSON path that can drift from the SDK contract.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 2b8c089b7699.

Label changes

Label justifications:

  • P2: This is normal-priority Google plugin OOM hardening with limited blast radius, but the current PR still has merge blockers.
  • merge-risk: 🚨 compatibility: The new 1 MiB success-body caps can reject Google API responses that current main would parse.
  • merge-risk: 🚨 auth-provider: The diff touches Google OAuth token and project discovery code, and the current token module does not compile.
  • merge-risk: 🚨 availability: The cap can cause normal Google image or speech generation flows to fail when base64 media payloads exceed 1 MiB.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides static checks and prior-art rationale only; it needs redacted terminal output, logs, or an artifact showing oversized Google JSON cancellation and normal responses before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +72. Total +72 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 6 84 12 +72
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 84 12 +72

What I checked:

  • Root policy read: Root AGENTS.md was read fully; its deep PR review, plugin-boundary, compatibility, proof, and security-review guidance applies to this provider-runtime PR. (AGENTS.md:1, 2b8c089b7699)
  • Scoped extension policy read: The scoped extensions policy was read; the PR stays within the plugin SDK boundary but must still use established shared SDK helpers when available. (extensions/AGENTS.md:1, 2b8c089b7699)
  • Current main still has the bug class: Current main still reads Google OAuth, media, image, speech, and embedding success JSON bodies through unbounded response.json() calls, so the hardening work remains meaningful. (extensions/google/oauth.token.ts:34, 2b8c089b7699)
  • PR head compile blocker: The latest PR head imports readResponseWithLimit twice in oauth.token.ts and leaves GOOGLE_OAUTH_JSON_RESPONSE_MAX_BYTES unused. (extensions/google/oauth.token.ts:7, 821de7f6841a)
  • CI corroborates the compile blocker: Live CI reports TS2300 duplicate identifier errors at oauth.token.ts lines 7 and 8 and TS6133 for the unused constant at line 12. (extensions/google/oauth.token.ts:7, 821de7f6841a)
  • Media-bearing JSON cap risk: The PR caps the entire Google image response at 1 MiB before parsing, while the same response parser expects inline base64 image data from inline.data. (extensions/google/image-generation-provider.ts:47, 821de7f6841a)

Likely related people:

  • wanglu241: Blame and log history show commit 2fe50f6 introduced the current Google plugin files and carried the unbounded success JSON reads this PR targets. (role: recent current-main area contributor; confidence: high; commits: 2fe50f69db7c; files: extensions/google/oauth.token.ts, extensions/google/oauth.project.ts, extensions/google/image-generation-provider.ts)
  • joshavant: Commit 0a14444 added readProviderJsonResponse and its plugin SDK export, the existing helper directly relevant to this PR's duplicated parsing pattern. (role: shared helper contributor; confidence: high; commits: 0a14444924e3; files: src/agents/provider-http-errors.ts, src/plugin-sdk/provider-http.ts)
  • Alix-007: Merged related response-limit PRs established nearby bounded JSON and Google prompt-cache patterns cited by this PR. (role: adjacent bounded-response hardening contributor; confidence: high; commits: 2592f8a51a4e, 91b0567e8902, 3da4280caf4c; files: src/agents/provider-http-errors.ts, src/agents/embedded-agent-runner/google-prompt-cache.ts, src/agents/embedded-agent-runner/openrouter-model-capabilities.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 rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 24, 2026
@lsr911 lsr911 mentioned this pull request Jun 24, 2026
Closed
@Alix-007 Alix-007 mentioned this pull request Jun 25, 2026
Merged
This was referenced Jun 25, 2026
Closed
Closed
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 25, 2026
@lsr911
lsr911 force-pushed the fix/google-bounded-json-response branch from 01e7df5 to 507f5ab Compare June 25, 2026 08:11
lsr911 and others added 2 commits June 25, 2026 16:25
@lsr911 @claude
fix(google): bound JSON response reads to prevent OOM
3068f70 
Replace unbounded await response.json() with readResponseWithLimit
(1 MiB cap) across embedding-batch, image-generation,
media-understanding, oauth.token, oauth.project, and speech providers.

A malicious or faulty Google API endpoint returning an oversized JSON
response body would previously buffer the entire body in memory before
parsing.  readResponseWithLimit cancels the stream once the cap is
reached, preventing unbounded buffering.

- embedding-batch: bound file upload, batch status, and result reads
  (3 call sites, plus error-path text() via existing pattern)
- image-generation-provider: bound image generation response read
- media-understanding-provider: bound media description response read
- oauth.token: bound token exchange JSON response read
- oauth.project: bound 5 JSON response reads (userinfo, project list,
  error payload, token refresh, onboarding LRO)
- speech-provider: bound speech synthesis response read

Co-Authored-By: Claude <[email protected]>
@lsr911 @claude
fix(google): add missing JSON response limit constants
fbe8946 
speech-provider, media-understanding-provider, and oauth.project were
missing their JSON_RESPONSE_MAX_BYTES constant declarations after the
batch script added readResponseWithLimit usage but skipped the constant.

Co-Authored-By: Claude <[email protected]>
@lsr911
lsr911 force-pushed the fix/google-bounded-json-response branch from 507f5ab to fbe8946 Compare June 25, 2026 08:26
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. label Jun 25, 2026
@lsr911 @claude
fix(google): restore missing variable assignments in oauth.project.ts
821de7f 
The batch script replaced await response.json() but dropped the
const declarations, causing 'data' and 'errorPayload' to be undefined
at runtime.  Also removed the duplicate readResponseWithLimit import.

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

lsr911 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing: source branch has been deleted, CI can no longer recover.

@lsr911 lsr911 closed this Jun 25, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 26, 2026
Merged
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: google merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lsr911