Skip to content

fix(signal): bound GitHub release info JSON response with readProviderJsonResponse#97536

Merged
vincentkoc merged 1 commit into
openclaw:mainfrom
hugenshen:fix/bound-signal-cli-release-json-response
Jun 28, 2026
Merged

fix(signal): bound GitHub release info JSON response with readProviderJsonResponse#97536
vincentkoc merged 1 commit into
openclaw:mainfrom
hugenshen:fix/bound-signal-cli-release-json-response

Conversation

@hugenshen

Copy link
Copy Markdown
Contributor

What Problem This Solves

installSignalCliFromRelease fetches
https://api.github.com/repos/AsamK/signal-cli/releases/latest and reads the
response with an unbounded await response.json() call. GitHub release objects
include a body field containing the full changelog text plus an assets[]
array; a misbehaving or slow endpoint can return an arbitrarily large payload
that is buffered entirely into memory.

The error path already guarded against a bad HTTP status (!response.ok), but
the success path had no size limit — a textbook asymmetric-bounding gap.

Why This Change Was Made

Replace the bare response.json() with readProviderJsonResponse, which
enforces a 16 MiB cap, cancels the underlying stream on overflow, and surfaces a
consistent labelled error (signal.release-info: …). The existing inner
try/catch continues to convert any parse or overflow error into the friendly
{ ok: false, error: "Failed to parse signal-cli release info." } result,
preserving the caller's graceful-degradation path.

User Impact

An operator cannot cause the Signal plugin setup to OOM the process by returning
an oversized release-info JSON body (whether from a misconfigured proxy or a
slow/malformed GitHub response).

Evidence

Proof script (scripts/proof-signal-release-info-bound.mjs) — 4 assertions:

[case 1] signal.release-info oversized, ReadableStream, cap=1 MiB
  ok: rejected on oversized release-info body — true
  ok: bounded error message present (got: Content too large: 2097152 bytes (limit: 1048576 bytes)) — true
  ok: stream cancelled before all 20 chunks (readCount=2) — true
  ok: cancel() was called — true

[case 2] negative control — raw response.json() does NOT cancel
  ok: unbounded .json() drained all 20 chunks (readCount=20) — true
  ok: stream NOT cancelled — drained to EOF — true

[case 3] signal.release-info oversized, node:http, bytes-on-wire
  ok: rejected on oversized release-info body (node:http) — true
  ok: bounded error message present (got: Content too large: 1113792 bytes (limit: 1048576 bytes)) — true

[case 4] small GitHub release JSON parses cleanly
  ok: small release JSON parsed into result — true
  ok: tag_name intact in parsed result — true

ALL PROOF ASSERTIONS PASSED

Unit tests (extensions/signal/src/install-signal-cli.test.ts) — 29/29
pass, including new regression:

✓ installSignalCliFromRelease > bounds oversized GitHub release metadata and cancels the stream
✓ installSignalCliFromRelease > returns an installer error when GitHub release metadata is malformed JSON
✓ installSignalCliFromRelease > bounds the release metadata request with an explicit timeout

…rJsonResponse

Replace bare `await response.json()` in `installSignalCliFromRelease` with
`readProviderJsonResponse` (16 MiB cap, stream cancel on overflow). The
external GitHub Releases endpoint can include a large `body` changelog field;
the error path was already guarded but the success path was unbounded.
The existing inner catch continues to convert overflow errors into the
graceful `{ ok: false, error: "Failed to parse signal-cli release info." }` path.

Adds a regression test verifying the stream is cancelled before all chunks are
read on an oversized 20 MiB streaming response.

Co-authored-by: Cursor <cursoragent@cursor.com>
@openclaw-barnacle openclaw-barnacle Bot added channel: signal Channel integration: signal size: XS labels Jun 28, 2026
@clawsweeper

clawsweeper Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 28, 2026, 1:27 PM ET / 17:27 UTC.

Summary
The PR replaces the Signal installer's GitHub release metadata response.json() call with readProviderJsonResponse and adds an oversized streaming-response cancellation regression test.

PR surface: Source +1, Tests +32. Total +33 across 2 files.

Reproducibility: yes. Current main has a source-level reproduction path because a successful Signal release metadata response is parsed with unbounded response.json(), and the PR proof/test model oversized streamed responses that exercise the gap.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: vector/embedding metadata: extensions/signal/src/install-signal-cli.test.ts. Confirm migration or upgrade compatibility proof before merge.

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.

Next step before merge

  • [P2] No repair job is needed because the PR already contains the focused fix and has no actionable review findings.

Security
Cleared: The diff reduces a memory-exhaustion exposure and does not add dependencies, workflows, lockfiles, secret handling, or new code-execution paths.

Review details

Best possible solution:

Merge this narrow Signal installer hardening after normal maintainer approval, keeping the shared provider HTTP bounded JSON reader as the canonical implementation.

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

Yes. Current main has a source-level reproduction path because a successful Signal release metadata response is parsed with unbounded response.json(), and the PR proof/test model oversized streamed responses that exercise the gap.

Is this the best way to solve the issue?

Yes. Using the existing Plugin SDK bounded JSON reader at the exact success-path parser is the narrow owner-boundary fix; a Signal-local limiter would duplicate the shared response-limit contract.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P2: The PR fixes a concrete Signal setup hardening bug with limited blast radius and targeted regression coverage.
  • 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 (live_output): The PR body includes after-fix proof-script output for oversized stream cancellation, node:http bytes-on-wire overflow, a raw response.json() negative control, and small JSON success, with the GitHub Real behavior proof check passing.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix proof-script output for oversized stream cancellation, node:http bytes-on-wire overflow, a raw response.json() negative control, and small JSON success, with the GitHub Real behavior proof check passing.
Evidence reviewed

PR surface:

Source +1, Tests +32. Total +33 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 2 1 +1
Tests 1 32 0 +32
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 34 1 +33

What I checked:

Likely related people:

  • steipete: Authored prior Signal installer hardening and multiple nearby installer/test maintenance commits, including timeout, size, and cleanup coverage for the same GitHub Releases flow. (role: adjacent hardening author; confidence: high; commits: 44ad65f02bbd, 2afff85ca43c, 68cec724b1af; files: extensions/signal/src/install-signal-cli.ts, extensions/signal/src/install-signal-cli.test.ts)
  • vincentkoc: Authored the existing malformed Signal release metadata handling and originally added the provider JSON helper exported through the SDK. (role: metadata error-path contributor; confidence: high; commits: 77e5a492f275, d16f79f49d24; files: extensions/signal/src/install-signal-cli.ts, extensions/signal/src/install-signal-cli.test.ts, src/agents/provider-http-errors.ts)
  • Alix-007: Authored the commit that changed readProviderJsonResponse from a wrapper around response.json() into a bounded readResponseWithLimit parser with oversized-stream tests. (role: shared bounded-reader contributor; confidence: high; commits: 2592f8a51a4e; files: src/agents/provider-http-errors.ts, src/agents/provider-http-errors.test.ts)
  • RomneyDa: Recently touched the same Signal installer and tests for macOS/Homebrew installation behavior. (role: recent Signal installer contributor; confidence: medium; commits: 3b292ba9d4bc; files: extensions/signal/src/install-signal-cli.ts, extensions/signal/src/install-signal-cli.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 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 28, 2026
@vincentkoc vincentkoc merged commit 51064bd into openclaw:main Jun 28, 2026
141 of 153 checks passed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 29, 2026
…rJsonResponse (openclaw#97536)

Replace bare `await response.json()` in `installSignalCliFromRelease` with
`readProviderJsonResponse` (16 MiB cap, stream cancel on overflow). The
external GitHub Releases endpoint can include a large `body` changelog field;
the error path was already guarded but the success path was unbounded.
The existing inner catch continues to convert overflow errors into the
graceful `{ ok: false, error: "Failed to parse signal-cli release info." }` path.

Adds a regression test verifying the stream is cancelled before all chunks are
read on an oversized 20 MiB streaming response.

Co-authored-by: NIO <nocodet@mail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
…rJsonResponse (openclaw#97536)

Replace bare `await response.json()` in `installSignalCliFromRelease` with
`readProviderJsonResponse` (16 MiB cap, stream cancel on overflow). The
external GitHub Releases endpoint can include a large `body` changelog field;
the error path was already guarded but the success path was unbounded.
The existing inner catch continues to convert overflow errors into the
graceful `{ ok: false, error: "Failed to parse signal-cli release info." }` path.

Adds a regression test verifying the stream is cancelled before all chunks are
read on an oversized 20 MiB streaming response.

Co-authored-by: NIO <nocodet@mail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 1, 2026
…rJsonResponse (openclaw#97536)

Replace bare `await response.json()` in `installSignalCliFromRelease` with
`readProviderJsonResponse` (16 MiB cap, stream cancel on overflow). The
external GitHub Releases endpoint can include a large `body` changelog field;
the error path was already guarded but the success path was unbounded.
The existing inner catch continues to convert overflow errors into the
graceful `{ ok: false, error: "Failed to parse signal-cli release info." }` path.

Adds a regression test verifying the stream is cancelled before all chunks are
read on an oversized 20 MiB streaming response.

Co-authored-by: NIO <nocodet@mail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Rorqualx pushed a commit to Rorqualx/cortex that referenced this pull request Jul 2, 2026
…rJsonResponse (openclaw#97536)

Replace bare `await response.json()` in `installSignalCliFromRelease` with
`readProviderJsonResponse` (16 MiB cap, stream cancel on overflow). The
external GitHub Releases endpoint can include a large `body` changelog field;
the error path was already guarded but the success path was unbounded.
The existing inner catch continues to convert overflow errors into the
graceful `{ ok: false, error: "Failed to parse signal-cli release info." }` path.

Adds a regression test verifying the stream is cancelled before all chunks are
read on an oversized 20 MiB streaming response.

Co-authored-by: NIO <nocodet@mail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
(cherry picked from commit 51064bd)
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 3, 2026
…rJsonResponse (openclaw#97536)

Replace bare `await response.json()` in `installSignalCliFromRelease` with
`readProviderJsonResponse` (16 MiB cap, stream cancel on overflow). The
external GitHub Releases endpoint can include a large `body` changelog field;
the error path was already guarded but the success path was unbounded.
The existing inner catch continues to convert overflow errors into the
graceful `{ ok: false, error: "Failed to parse signal-cli release info." }` path.

Adds a regression test verifying the stream is cancelled before all chunks are
read on an oversized 20 MiB streaming response.

Co-authored-by: NIO <nocodet@mail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
ianalloway pushed a commit to ianalloway/openclaw that referenced this pull request Jul 11, 2026
…rJsonResponse (openclaw#97536)

Replace bare `await response.json()` in `installSignalCliFromRelease` with
`readProviderJsonResponse` (16 MiB cap, stream cancel on overflow). The
external GitHub Releases endpoint can include a large `body` changelog field;
the error path was already guarded but the success path was unbounded.
The existing inner catch continues to convert overflow errors into the
graceful `{ ok: false, error: "Failed to parse signal-cli release info." }` path.

Adds a regression test verifying the stream is cancelled before all chunks are
read on an oversized 20 MiB streaming response.

Co-authored-by: NIO <nocodet@mail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: signal Channel integration: signal 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: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants