Skip to content

test(provider-usage): cover bounded shared response reads#96360

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
solodmd:fix/bound-provider-usage-response
Jun 29, 2026
Merged

test(provider-usage): cover bounded shared response reads#96360
vincentkoc merged 1 commit into
openclaw:mainfrom
solodmd:fix/bound-provider-usage-response

Conversation

@solodmd

@solodmd solodmd commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

readUsageJson in src/infra/provider-usage.fetch.shared.ts reads provider
usage API responses with an unbounded await response.json(). A
compromised or hijacked provider endpoint can stream an arbitrarily large
body and force the runtime to buffer the entire payload before parsing —
an OOM/DoS vector. This is the same vulnerability class already hardened
for OpenRouter (#95420), Google (#95417), provider HTTP errors (#95218),
Matrix (#95240), and ClawHub (#95226); the provider usage monitoring path
was still reading without a cap.

Changes

  • src/infra/provider-usage.fetch.shared.ts — import
    readResponseWithLimit from
    @openclaw/media-core/read-response-with-limit; replace
    await response.json() with readResponseWithLimit at 16 MiB cap +
    JSON.parse(new TextDecoder().decode(buffer)). Uses TextDecoder (not
    Buffer.toString) to preserve UTF-8 BOM stripping semantics that
    response.json() provides, matching the sibling bounded-read pattern in
    provider-http-errors.ts:308.

Evidence

Real behavior proof

$ node --import tsx /tmp/verify-overflow-cancel.mjs
PASS: readResponseWithLimit threw: Provider usage response too large: 16777217 bytes
PASS: pullCount = 1 (<= 2 means stream cancelled early)
PASS: stream cancelled = true

A ReadableStream enqueues a single 16777217 byte chunk (just over the 16 MiB
cap) into a Response body. readResponseWithLimit detects the overflow,
throws via onOverflow, and cancels the underlying stream — pullCount = 1
proves no further chunks were read, and cancelled = true proves the stream
was cleaned up.

Tests

  • New Vitest case rejects an oversized JSON response creates a
    ReadableStream whose first chunk exceeds the 16 MiB cap and proves the
    bounded read cancels the body (cancel called once, pull count ≤ 2)
    and returns { ok: false, snapshot: { error: "Malformed usage response" } }.
  • New Vitest case parses a normal-sized JSON response proves a valid
    response under the cap returns { ok: true, data } unchanged.
  • New Vitest case handles a JSON parse error gracefully proves
    non-JSON content still returns "Malformed usage response" via the catch
    block.
  • pnpm test src/infra/provider-usage.fetch.shared.test.ts → 16 passed (16)
  • oxlint on the changed file → exit 0, no findings

@clawsweeper

clawsweeper Bot commented Jun 24, 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.

Keep open: the runtime provider-usage bounded-read fix is already on current main, but this PR now contains focused shared-helper regression tests that are not on main and are still useful.

Root-cause cluster
Relationship: partial_overlap
Canonical: #97659
Summary: The merged replacement PR is canonical for the runtime provider-usage bounded-read fix; this PR remains a useful test-only follow-up for the shared helper.

Members:

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

Canonical path: Close this PR as superseded by #97659.

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

Review details

Best possible solution:

Close this PR as superseded by #97659.

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

Yes, but not as a current-main failure: current main already has the runtime fix. The oversized-stream behavior is source-proven through readUsageJson delegating to readProviderJsonResponse and readResponseWithLimit, and this PR adds focused regression coverage for that path.

Is this the best way to solve the issue?

Yes. After the runtime fix moved to main, focused shared-helper regression coverage is the narrowest maintainable remaining solution; reintroducing the older runtime diff would create drift.

Security review:

Security review cleared: The diff is test-only and adds no dependencies, workflows, secrets handling, package resolution, or executable supply-chain surface.

AGENTS.md: found and applied where relevant.

What I checked:

  • linked superseding PR: fix(provider-usage): bound usage JSON response reads #97659 (fix(provider-usage): bound usage JSON response reads) is merged at 2026-06-29T04:57:22Z.
  • cluster evidence: the durable review links that PR in the work cluster or recommended risk path.
  • no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

  • vincentkoc: Authored the current-main bounded provider-usage runtime fix and the current narrowed test-only head for this PR. (role: recent area contributor; confidence: high; commits: db2786bde105, afe42eea220e; files: src/infra/provider-usage.fetch.shared.ts, src/infra/provider-usage.fetch.shared.test.ts, src/infra/provider-usage.fetch.claude.ts)
  • RomneyDa: Current blame shows the shared readUsageJson wrapper was introduced in the helper extraction history that this PR now tests. (role: introduced helper; confidence: medium; commits: 39e1be080ca0; files: src/infra/provider-usage.fetch.shared.ts, src/infra/provider-usage.fetch.shared.test.ts)
  • Alix-007: History for fix(agents): bound provider JSON response reads #95218 introduced and tested the bounded provider JSON helper contract used by the current implementation. (role: adjacent hardening contributor; confidence: medium; commits: 2592f8a51a4e; files: src/agents/provider-http-errors.ts, src/agents/provider-http-errors.test.ts)

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

@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
@openclaw-barnacle openclaw-barnacle Bot added triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 24, 2026
@solodmd
solodmd force-pushed the fix/bound-provider-usage-response branch from dbc8341 to 24c8d08 Compare June 24, 2026 10:30
@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. and removed 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. labels Jun 24, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 25, 2026
Merged
@hugenshen hugenshen mentioned this pull request Jun 28, 2026
Merged
This was referenced Jun 29, 2026
Closed
Merged
@vincentkoc
vincentkoc force-pushed the fix/bound-provider-usage-response branch from 24c8d08 to afe42ee Compare June 29, 2026 05:00
@vincentkoc vincentkoc changed the title fix(infra): bound provider usage JSON response reads test(provider-usage): cover bounded shared response reads Jun 29, 2026
@vincentkoc

vincentkoc commented Jun 29, 2026

Copy link
Copy Markdown
Member

Maintainer update: rebased this PR onto current main and narrowed it to the still-useful shared readUsageJson coverage only.

The runtime implementation already landed through #97659 as db2786b, using the shared readProviderJsonResponse helper and also covering MiniMax success reads plus Claude OAuth error-body parsing. This PR now preserves the extra shared-helper tests from the original contribution without reintroducing the older duplicate implementation shape.

Validation run locally:

  • ./node_modules/.bin/oxlint --tsconfig config/tsconfig/oxlint.core.json src/infra/provider-usage.fetch.shared.test.ts src/infra/provider-usage.fetch.shared.ts
  • node scripts/test-projects-serial.mjs src/infra/provider-usage.fetch.shared.test.ts

@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Jun 29, 2026
@vincentkoc
vincentkoc merged commit 7e2f425 into openclaw:main Jun 29, 2026
99 of 102 checks passed
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

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

@solodmd @vincentkoc