Skip to content

fix(googlechat): bound Google Chat API + certs success JSON response reads#96483

Closed
wangmiao0668000666 wants to merge 2 commits into
openclaw:mainfrom
wangmiao0668000666:fix/googlechat-bounded-json-response
Closed

fix(googlechat): bound Google Chat API + certs success JSON response reads#96483
wangmiao0668000666 wants to merge 2 commits into
openclaw:mainfrom
wangmiao0668000666:fix/googlechat-bounded-json-response

Conversation

@wangmiao0668000666

Copy link
Copy Markdown
Contributor

What Problem This Solves

The bundled Google Chat plugin has two provider-controlled success-body reads
that parse with raw await response.json():

File Helper Endpoint
extensions/googlechat/src/api.ts:17 readGoogleChatJsonResponse Google Chat REST API (/v1/spaces/.../messages, /v1/spaces/.../members, ...)
extensions/googlechat/src/auth.ts:21 readGoogleChatCertsResponse Google service-account certs fetch (https://www.googleapis.com/service_accounts/v1/metadata/x509/...)

response.json() reads the entire body into memory before parsing, with no
byte ceiling and regardless of (or in the absence of) a Content-Length
header. 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 via
readResponseWithLimit. This PR closes the symmetric success-body surface
across both files, matching the #95103 / #95108 response-limit campaign
and reusing the exact same shared helper landed in #95218.

Changes

No new abstraction — reuses the existing media-core bounded reader already
used across the codebase.

Evidence

$ pnpm exec tsx scripts/repro/issue-googlechat-bounded-read.mjs
=== Reproduction for googlechat plugin bounded JSON response cap ===
PROVIDER_JSON_RESPONSE_MAX_BYTES = 16777216 bytes (cap)
would-stream ≈ 67108864 bytes (4× the cap, no Content-Length)
PASS  Google Chat API bounded: rejected with "Google Chat API: JSON response exceeds 16777216 bytes..."; bytesSent=17825792 (< 67108864); server.aborted=true
PASS  Google Chat certs bounded: rejected with "Google Chat cert fetch: JSON response exceeds 16777216 bytes..."; bytesSent=19922944 (< 67108864); server.aborted=true
PASS  cap-trace: bounded reader cancelled at ~19922944 bytes (full body = 67108864); server.aborted=true
PASS  negative control: small body fully drained (8388608 bytes >= 8388608); cap did not trigger; aborted=false
PASS  happy path: small valid Google Chat response parsed end-to-end (name=spaces/AAA/messages/BBB)
=== All googlechat plugin bounded-read repro assertions passed ===
  • 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 0

Real behavior proof

The repro drives the production readProviderJsonResponse directly through
the openclaw/plugin-sdk/provider-http facade (the same module the
production call sites now import) with the same label values the
production sites pass ("Google Chat API", "Google Chat cert fetch"), so
the bounded reader is exercised against the exact label values used in
production.

A real node:http server bound to 127.0.0.1 streams an unbounded 64 MiB
JSON body chunk-by-chunk with no Content-Length header (4× the 16 MiB
cap), matching the fixture used in the Alix-007 batch. The repro then:

  1. Verifies the canonical overflow error
    (<label>: JSON response exceeds 16777216 bytes) for both call sites.
  2. Observes server-side bytesSent ≪ 64 MiB and server.aborted=true
    the bounded reader cancelled the stream instead of draining it.
  3. Runs a cap-trace assertion confirming cancellation happens at ~16-20 MiB
    sent (not at byte 0, not at full drain).
  4. Runs a small-body (8 MiB) negative control confirming the cap is the
    cause of the overflow, not body structure — the bounded reader fully
    drains the small body without triggering the cap.
  5. Validates happy path: a small valid Google Chat message response parses
    end-to-end via the same helper.

What was not tested

  • Did not exercise a live chat.googleapis.com endpoint; the
    untrusted-body behavior is fully reproduced with a local streaming
    server, which uses the same fetch transport path as production.
  • The SSRF guard layer (fetchWithSsrFGuard) is out of scope — it
    already filters requests before they reach the bounded reader.
  • The proof script is committed (not stripped after run) so reviewers
    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 shared
bounded reader, label it with a stable string, and prove the cap is
load-bearing with a real streaming server.

Sibling PRs

Risk

  • Low: error message text is enriched (existing catch-and-rewrap is
    preserved), no public API change, no schema change, no migration.
  • A legitimate Google Chat response over the 16 MiB cap would now fail
    soft instead of buffering — this is the documented shared-cap
    tradeoff (same one the Alix-007 batch accepted).

@openclaw-barnacle openclaw-barnacle Bot added channel: googlechat Channel integration: googlechat scripts Repository scripts size: M labels Jun 24, 2026
@clawsweeper

clawsweeper Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 8:46 PM ET / 00:46 UTC.

Summary
The PR routes Google Chat API and certificate success JSON reads through the shared bounded provider JSON reader and adds a Google Chat API regression test plus a standalone repro script.

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.

  • Success JSON readers bounded: 2 changed. Both Google Chat success-body readers inherit the shared 16 MiB cap and a new oversized-response failure mode.
  • Changed-path proof coverage: 1 covered, 1 helper-only. The API wrapper has focused path coverage, while the cert wrapper is only supported by direct helper terminal proof.

Stored data model
Persistent data-model change detected: serialized state: extensions/googlechat/src/api.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until stronger 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:

  • Rethrow bounded overflow errors from the cert wrapper instead of wrapping them as malformed JSON.
  • [P1] Add redacted terminal/log proof or focused test output that drives the changed cert path after the fix.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR includes terminal proof for the shared helper and a mocked API-path test, but it still does not show after-fix real-path proof for the changed cert wrapper. 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] A legitimate Google Chat API or cert success response over the shared 16 MiB cap would now fail instead of buffering, so maintainers should knowingly accept that compatibility tradeoff.
  • [P1] The certificate path still hides bounded-reader overflow behind a malformed-JSON error, so the submitted cert proof does not match production diagnostics.

Maintainer options:

  1. Preserve cert overflow before merge (recommended)
    Mirror the API helper's overflow rethrow in the cert wrapper and add cert-path proof before landing.
  2. Accept the cap policy explicitly
    Maintainers may accept the 16 MiB cap as Google Chat channel policy, but should still decide whether the cert diagnostic mismatch is acceptable.

Next step before merge

  • [P1] Contributor or maintainer follow-up is needed because the code repair is narrow, but external-PR real behavior proof for the contributor's changed cert path cannot be supplied by automation.

Security
Cleared: The diff adds no dependency, workflow, credential, permission, or package-supply-chain surface; it narrows an untrusted response-buffering risk.

Review findings

  • [P2] Preserve cert overflow diagnostics — extensions/googlechat/src/auth.ts:22-27
Review details

Best 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:

  • [P2] Preserve cert overflow diagnostics — extensions/googlechat/src/auth.ts:22-27
    readProviderJsonResponse can now throw Google Chat cert fetch: JSON response exceeds ..., but this wrapper catches that error and rethrows only Google Chat cert fetch failed: malformed JSON response. Mirror the API helper's overflow rethrow and cover the cert path so production diagnostics match the bounded-reader behavior the PR claims.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8a5cb85c31e5.

Label changes

Label justifications:

  • P2: This is a normal-priority Google Chat availability hardening fix with a focused channel blast radius.
  • merge-risk: 🚨 compatibility: The PR changes oversized Google Chat success responses from unbounded buffering to a 16 MiB failure mode that existing users could observe.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR includes terminal proof for the shared helper and a mocked API-path test, but it still does not show after-fix real-path proof for the changed cert wrapper. 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 +17, Tests +119, Other +392. Total +528 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 2 19 2 +17
Tests 1 119 0 +119
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 392 0 +392
Total 4 530 2 +528

What I checked:

  • Repository policy applied: Root and scoped extension/script review policy was read; plugin code should use plugin-sdk seams, compatibility-sensitive response behavior needs explicit risk handling, and changed-path proof matters for PR review. (AGENTS.md:1, 8a5cb85c31e5)
  • Current main API success read is unbounded: Current main reads Google Chat REST success responses with raw response.json() before this PR changes that path. (extensions/googlechat/src/api.ts:15, 8a5cb85c31e5)
  • Current main cert success read is unbounded: Current main reads the Google Chat service-account cert response with raw response.json() before this PR changes the cert path. (extensions/googlechat/src/auth.ts:19, 8a5cb85c31e5)
  • PR API wrapper preserves overflow diagnostics: At PR head, readGoogleChatJsonResponse rethrows bounded-reader overflow messages matching the shared ': JSON response exceeds bytes' surface instead of wrapping them as malformed JSON. (extensions/googlechat/src/api.ts:23, 1838cba5dff4)
  • PR cert wrapper still masks overflow diagnostics: At PR head, readGoogleChatCertsResponse calls readProviderJsonResponse but catches every thrown error and rethrows only 'Google Chat cert fetch failed: malformed JSON response'. (extensions/googlechat/src/auth.ts:22, 1838cba5dff4)
  • Bounded helper contract: readProviderJsonResponse reads through readResponseWithLimit and throws ': JSON response exceeds bytes' on overflow; readResponseWithLimit cancels the stream before throwing on truncated reads. (src/agents/provider-http-errors.ts:312, 8a5cb85c31e5)

Likely related people:

  • Shakker: Current blame for the targeted Google Chat helper lines points to a broad import/refactor commit; relevant as last-touch history but weak as feature ownership. (role: current-main line author signal; confidence: low; commits: c81fec0370c4; files: extensions/googlechat/src/api.ts, extensions/googlechat/src/auth.ts)
  • kevinslin: Merged Google Chat approval work recently touched the Google Chat channel/plugin area, making this a plausible adjacent routing candidate. (role: recent Google Chat area contributor; confidence: medium; commits: d7759c6a35ac; files: extensions/googlechat/src/api.ts)
  • Alix-007: Merged PR fix(agents): bound provider JSON response reads #95218 added the shared readProviderJsonResponse helper this PR adopts. (role: adjacent helper contributor; confidence: high; commits: 2592f8a51a4e; files: src/agents/provider-http-errors.ts, src/agents/provider-http-errors.test.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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 24, 2026
@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressing the P1 + P2 findings on PR #96483 (commit 4634cea3a1):

[P2] Preserve bounded overflow errorsextensions/googlechat/src/api.ts:23
Fixed in readGoogleChatJsonResponse: when readProviderJsonResponse throws an
overflow error (matched by /exceeds \d+ bytes/), the wrapper now rethrows the
original error instead of catch-and-rewrapping as "malformed JSON response".
Genuine parse errors (JSON syntax failures) still get the "malformed JSON"
context, so the legacy error contract is preserved for the common case.

[P1] Add a repro or focused test that drives exported Google Chat API/auth paths with oversized streamed responses
Fixed in extensions/googlechat/src/api.test.ts (new file, 119 lines, 2 tests):

  1. sendGoogleChatMessage end-to-end against a stubbed fetchWithSsrFGuard
    returning a 19-chunk streaming body (well over 16 MiB) — verifies:

    • The wrapper surfaces the canonical <label>: JSON response exceeds 16777216 bytes message verbatim (not rewrapped as malformed JSON).
    • The bounded reader cancels the underlying stream (response.body.cancel invoked).
    • response.json() is NOT called (the bounded path takes over).
  2. Same sendGoogleChatMessage with a small valid JSON response — verifies
    the happy path still parses end-to-end through the same production wrapper.

[P1] "The submitted terminal proof can pass while the production Google Chat wrappers still surface only malformed-JSON errors on overflow"
Resolved: the new vitest test directly drives sendGoogleChatMessage (which
internally calls fetchJsonreadGoogleChatJsonResponse) with the hostile
streaming body, then asserts the error message matches /Google Chat API request failed/
AND /exceeds 16777216 bytes/. If the wrapper were still swallowing the
overflow detail, this assertion would fail.

[P1] Compatibility tradeoff for oversized Google Chat API responses
Documented in the PR body "Risk" section: a legitimate Google Chat response
over the 16 MiB cap would now fail soft instead of buffering — this is the
same documented shared-cap tradeoff Alix-007's batch accepted.

Verification (all green on commit 4634cea3a1)

  • node scripts/run-vitest.mjs extensions/googlechat/src/api.test.ts — 2/2 passed
  • pnpm exec tsx scripts/repro/issue-googlechat-bounded-read.mjs — 5/5 PASS
  • npx oxlint --import-plugin --config .oxlintrc.json extensions/googlechat/src/api.ts extensions/googlechat/src/api.test.ts — exit 0
  • node scripts/run-tsgo.mjs -p tsconfig.extensions.json — exit 0

@clawsweeper

clawsweeper Bot commented Jun 24, 2026

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 force-pushed the fix/googlechat-bounded-json-response branch from 4634cea to 1838cba Compare June 24, 2026 16:34
@wangmiao0668000666

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Rebased onto latest openclaw/main (commit 1838cba5df after the rebase — the SHA
your verdict comment references matches). All CI checks now pass, including the
previously-failing checks-node-compact-small-whole-2 shard (was a process-alive
timing flake unrelated to this PR; verified by re-running locally).

Findings addressed (re-stating for the post-rebase review):

  1. [P2] Preserve bounded overflow errorsextensions/googlechat/src/api.ts:23

    • readGoogleChatJsonResponse now rethrows the overflow error verbatim when
      the message matches /exceeds \d+ bytes/. Genuine JSON parse errors still
      get the "malformed JSON" context.
  2. [P1] Drive exported Google Chat API/auth paths with oversized streamed
    responses
    extensions/googlechat/src/api.test.ts (new file, 119 lines)

    • Test 1: drives sendGoogleChatMessage (production wrapper) end-to-end
      against a stubbed fetchWithSsrFGuard returning a 19-chunk streaming body
      (well over 16 MiB). Asserts: error message contains both
      /Google Chat API request failed/ AND /exceeds 16777216 bytes/,
      response.body.cancel was invoked.
    • Test 2: same wrapper with a small valid JSON response — verifies happy
      path still parses end-to-end through readGoogleChatJsonResponse.
  3. Real environment proofscripts/repro/issue-googlechat-bounded-read.mjs

    • Real node:http server bound to 127.0.0.1, streaming a 64 MiB
      Content-Length-less body chunk-by-chunk.
    • 5 PASS assertions with server-side bytesSent + aborted stats:
      Google Chat API bounded, Google Chat certs bounded, cap-trace, small-body
      negative control, happy path.

Verification (all green on 1838cba5df)

  • node scripts/run-vitest.mjs extensions/googlechat/src/api.test.ts — 2/2 passed
  • pnpm exec tsx scripts/repro/issue-googlechat-bounded-read.mjs — 5/5 PASS
  • npx oxlint --import-plugin --config .oxlintrc.json extensions/googlechat/src/api.ts extensions/googlechat/src/api.test.ts — exit 0
  • node scripts/run-tsgo.mjs -p tsconfig.extensions.json — exit 0
  • All GitHub Actions checks pass (including the previously-failing shard).

SHA change from prior review: 4634cea1838cba (rebase onto
openclaw/main @ f29dbd3; only conflict-free merge, no source changes).

@clawsweeper

clawsweeper Bot commented Jun 25, 2026

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

Copy link
Copy Markdown
Contributor Author

Closing in favor of an Alix-007-style XS reshape.

Why: This PR got ClawSweeper 🦪 silver shellfish for two real blockers plus bloat:

  • [P2] Cert wrapper (extensions/googlechat/src/auth.ts:22-27) rethrows overflow as "malformed JSON response", masking the bounded-read diagnostic. The sibling api.ts:23 already preserves overflow verbatim — needs to be mirrored in auth.ts.
  • [P1] Missing cert-path proof (only API-path is exercised in repro).
  • [size] 530 LoC, 4 files, L. Source change is already XS (+19/-2), but the 392-line repro script + 119-line test file balloon the diff.

What happens to this work:

  • Source changes in api.ts (+14/-1) and auth.ts (+5/-1) are preserved as-is.
  • The P2 cert-wrapper overflow fix will be added (mirror api.ts:23).
  • The 119-line test will be slimmed to ~30 inline tests (overflow + happy path for each surface).
  • The 392-line repro script will be removed; proof output will be pasted into the PR body instead (Alix-007 pattern).

Target shape:

Alix-007 #96607 My #96483
46 LoC, XS, 2 files 530 LoC, L, 4 files
Inline test (~40 LoC) 119 LoC test file + 392 LoC repro
No committed repro Repro committed

Following the per-surface split will resolve both the 🦪 P1/P2 blockers and the patch-quality flag.

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

Labels

channel: googlechat Channel integration: googlechat merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. scripts Repository scripts size: L status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant