fix(googlechat): bound Google Chat API + certs success JSON response reads#96483
fix(googlechat): bound Google Chat API + certs success JSON response reads#96483wangmiao0668000666 wants to merge 2 commits into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 8:46 PM ET / 00:46 UTC. Summary PR surface: Source +17, Tests +119, Other +392. Total +528 across 4 files. Reproducibility: yes. from source. Current main uses raw response.json() in both targeted success-body readers, and the shared helper contract plus PR proof show the bounded cap shape, though I did not execute a live harness in this read-only review. Review metrics: 2 noteworthy metrics.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the shared bounded-reader approach, preserve overflow diagnostics in both Google Chat wrappers, and add cert-path proof before merge. Do we have a high-confidence way to reproduce the issue? Yes from source. Current main uses raw response.json() in both targeted success-body readers, and the shared helper contract plus PR proof show the bounded cap shape, though I did not execute a live harness in this read-only review. Is this the best way to solve the issue? No, not yet. The shared plugin-sdk helper is the right owner-boundary fix, but the cert wrapper still masks overflow errors and needs changed-path proof. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8a5cb85c31e5. Label changesLabel justifications:
Evidence reviewedPR surface: Source +17, Tests +119, Other +392. Total +528 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 re-review Addressing the P1 + P2 findings on PR #96483 (commit [P2] Preserve bounded overflow errors — [P1] Add a repro or focused test that drives exported Google Chat API/auth paths with oversized streamed responses
[P1] "The submitted terminal proof can pass while the production Google Chat wrappers still surface only malformed-JSON errors on overflow" [P1] Compatibility tradeoff for oversized Google Chat API responses Verification (all green on commit
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
4634cea to
1838cba
Compare
|
@clawsweeper re-review Rebased onto latest openclaw/main (commit Findings addressed (re-stating for the post-rebase review):
Verification (all green on
SHA change from prior review: 4634cea → 1838cba (rebase onto |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Closing in favor of an Alix-007-style XS reshape. Why: This PR got ClawSweeper 🦪 silver shellfish for two real blockers plus bloat:
What happens to this work:
Target shape:
Following the per-surface split will resolve both the 🦪 P1/P2 blockers and the patch-quality flag. |
What Problem This Solves
The bundled Google Chat plugin has two provider-controlled success-body reads
that parse with raw
await response.json():extensions/googlechat/src/api.ts:17readGoogleChatJsonResponse/v1/spaces/.../messages,/v1/spaces/.../members, ...)extensions/googlechat/src/auth.ts:21readGoogleChatCertsResponsehttps://www.googleapis.com/service_accounts/v1/metadata/x509/...)response.json()reads the entire body into memory before parsing, with nobyte ceiling and regardless of (or in the absence of) a
Content-Lengthheader. Google Chat is an external, untrusted source: a misbehaving,
compromised, or hostile proxy (or a path where the SSRF guard was bypassed in
the future) can stream an arbitrarily large or never-ending JSON body and
force the plugin to buffer it all — driving the certs/OAuth or message-send
path into memory pressure or an indefinite hang.
The error-body path in
api.ts(line 129) is already bounded viareadResponseWithLimit. This PR closes the symmetric success-body surfaceacross both files, matching the
#95103/#95108response-limit campaignand reusing the exact same shared helper landed in
#95218.Changes
extensions/googlechat/src/api.ts—+7/-1— importreadProviderJsonResponsefromopenclaw/plugin-sdk/provider-http;replace the unbounded
(await response.json()) as TinreadGoogleChatJsonResponsewith the bounded reader (label:"Google Chat API").extensions/googlechat/src/auth.ts—+5/-1— same swap on the certshelper (label:
"Google Chat cert fetch").scripts/repro/issue-googlechat-bounded-read.mjs— new — standalonerepro that drives the production helper against a real
node:httpstreaming server with 5 PASS assertions, matching the Alix-007 🦞-grade
proof pattern from fix(ollama): bound model-discovery JSON response reads #96027 / fix(parallel): bound successful web-search JSON response reads #96035 / fix(exa): bound untrusted search JSON response reads #96038 / fix(lmstudio): bound model load success response body to prevent OOM #96042.
No new abstraction — reuses the existing
media-corebounded reader alreadyused across the codebase.
Evidence
npx oxlint --import-plugin --config .oxlintrc.json extensions/googlechat/src/api.ts extensions/googlechat/src/auth.ts scripts/repro/issue-googlechat-bounded-read.mjs— exit 0
node scripts/run-tsgo.mjs -p tsconfig.extensions.json— exit 0Real behavior proof
The repro drives the production
readProviderJsonResponsedirectly throughthe
openclaw/plugin-sdk/provider-httpfacade (the same module theproduction call sites now import) with the same label values the
production sites pass (
"Google Chat API","Google Chat cert fetch"), sothe bounded reader is exercised against the exact label values used in
production.
A real
node:httpserver bound to127.0.0.1streams an unbounded 64 MiBJSON body chunk-by-chunk with no
Content-Lengthheader (4× the 16 MiBcap), matching the fixture used in the Alix-007 batch. The repro then:
(
<label>: JSON response exceeds 16777216 bytes) for both call sites.bytesSent≪ 64 MiB andserver.aborted=true—the bounded reader cancelled the stream instead of draining it.
sent (not at byte 0, not at full drain).
cause of the overflow, not body structure — the bounded reader fully
drains the small body without triggering the cap.
end-to-end via the same helper.
What was not tested
chat.googleapis.comendpoint; theuntrusted-body behavior is fully reproduced with a local streaming
server, which uses the same
fetchtransport path as production.fetchWithSsrFGuard) is out of scope — italready filters requests before they reach the bounded reader.
can re-execute it directly.
Sibling coverage
This is the googlechat half of the unbounded-response.json() family that
Alix-007's bound-stream batch (#96027 / #96035 / #96038 / #96042) covered
for ollama / parallel / exa / lmstudio. It mirrors the symmetric shape of
the upstream merge pattern: swap the bare
response.json()for the sharedbounded reader, label it with a stable string, and prove the cap is
load-bearing with a real streaming server.
Sibling PRs
(openai video, dashscope, chutes oauth), each with sibling coverage and
Alix-007-grade repro proof.
Risk
preserved), no public API change, no schema change, no migration.
soft instead of buffering — this is the documented shared-cap
tradeoff (same one the Alix-007 batch accepted).