Skip to content

fix: cap HTTP PDF response bodies - #20

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/http-pdf-body-cap
Open

fix: cap HTTP PDF response bodies#20
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/http-pdf-body-cap

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

HTTP(S) PDF inputs previously buffered an unbounded response, so a fast oversized download could exhaust memory within the existing 30-second deadline. This adds a 100 MB default body budget, rejects oversized declared lengths (including values beyond safe-integer range), counts streamed bytes, and preserves PdfBudgetError with limit=fetchMaxBytes.

Library callers can set fetchMaxBytes; CLI extraction and rendering accept --fetch-max-bytes <n>. Both support 0 to disable the cap, preserving an explicit path for large remote PDFs. Rejected HTTP status responses now abort their unread bodies and release connections. File, blob, data URL, and stdin bodies are not capped.

The original implementation and commit are by @SebTardif (Sebastien Tardif). Maintainer follow-up adds CLI compatibility, connection cleanup, documentation/changelog, and installed-package coverage.

Validation: pnpm build, pnpm typecheck, pnpm test (18 tests), pnpm test:package, pnpm docs:site, and pnpm pack --dry-run pass on Node 24. The package test installs the tarball and uses a real local HTTP server to exercise implicit/explicit extraction and rendering, numeric/disabled budgets, default rejection, invalid-argument exit codes, library text extraction, PNG bytes, and rejected-response connection closure. Its HTTP 503 cleanup check failed before the cancellation fix and passes afterward. Existing tests cover stalled headers/bodies and caller cancellation.

A separate built-CLI proof used a valid 100,000,611-byte PDF: extraction and rendering rejected it by default, then produced valid text/PNG with an exact-size budget and with 0. Browser execution and multi-gigabyte downloads were not exercised. CI validates the supported Node 22/24/26 matrix. No release or deployment is included.

@clawsweeper

clawsweeper Bot commented Sep 2, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 2, 2026
@clawsweeper

clawsweeper Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 4, 2026, 12:41 PM ET / 16:41 UTC.

ClawSweeper review

What this changes

Adds a 100 MB HTTP(S) PDF response limit, configurable library and CLI overrides, typed budget errors, connection cleanup, documentation, and package-level coverage.

Merge readiness

Blocked before merge - 3 items remain

Keep open for maintainer review: the patch correctly adds a bounded remote-PDF read path with documented escape hatches, but the new 100 MB default intentionally changes upgrade behavior for existing large remote inputs.

Priority: P2
Reviewed head: d5ca3a79d344879a192b747538acd8dea3150c30
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The implementation and focused coverage are strong, with the remaining blocker being compatibility-policy approval rather than a correctness defect.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The changed production owner is the remote URL reader; the supplied package harness runs the installed library and CLI through a real local HTTP server and the PR body records an after-fix 100,000,611-byte built-CLI run showing default rejection and successful exact/disabled overrides.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production owner is the remote URL reader; the supplied package harness runs the installed library and CLI through a real local HTTP server and the PR body records an after-fix 100,000,611-byte built-CLI run showing default rejection and successful exact/disabled overrides.
Evidence reviewed 6 items Current main remains unbounded: The current main parent reads a successful remote response with response.arrayBuffer() and has no response-size option, so this PR still addresses a live gap rather than duplicating main.
Bounded implementation: The introduced code resolves a 100 MB default, rejects oversized declared lengths, counts streamed body bytes, cancels rejected reads, and preserves a typed PdfBudgetError.
Library and CLI escape hatches: Extraction forwards the new option to document opening, while the CLI validates and forwards --fetch-max-bytes for both extraction and rendering.
Findings None None.
Security None None.

How this fits together

ClawPDF normalizes local and remote PDF inputs before passing bytes to its PDFium engine for text extraction or rendering. This change bounds HTTP(S) input buffering while preserving local, blob, data-URL, and stdin behavior.

flowchart LR
  A[Remote PDF URL] --> B[Input normalizer]
  C[Library or CLI byte limit] --> B
  B --> D[Header size check]
  D --> E[Streaming body counter]
  E --> F[PDFium engine]
  F --> G[Text or PNG output]
Loading

Decision needed

Question Recommendation
Should ClawPDF make the 100 MB remote-response cap the default for existing callers, with fetchMaxBytes and --fetch-max-bytes 0 as the compatibility escape hatches? Approve the capped default: Ship the 100 MB default with the documented per-call and CLI overrides, and disclose the upgrade behavior in release notes.

Why: The implementation is coherent, but choosing a new default that rejects previously successful large remote inputs is an upgrade-policy decision rather than a mechanical defect.

Before merge

  • Resolve merge risk (P1) - Existing scripts or CLI workflows that successfully load remote PDFs above 100 MB will fail after upgrade unless they set a larger limit or explicitly use 0; this is documented and covered, but needs maintainer acceptance as the default compatibility contract.
  • Complete next step (P2) - Confirm the intended 100 MB default and documented upgrade escape hatch before merge.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Implementation and coverage production +105/-6, tests +152/-2, docs +19/-8 The change adds a new remote-input default while pairing it with substantial installed-package and unit coverage.

Merge-risk options

Maintainer options:

  1. Approve the documented default cap (recommended)
    Accept the intentional 100 MB upgrade cutoff and publish the library and CLI override path with the next release.
  2. Preserve legacy default behavior
    Revise the patch so response limits are opt-in if maintainers do not want existing large remote inputs to fail on upgrade.
  3. Pause pending policy ownership
    Leave the PR unmerged until an owner explicitly chooses the compatibility contract.

Technical review

Best possible solution:

Accept the 100 MB default only with explicit ownership of the upgrade contract, retain the documented library and CLI escape hatches, and call out the new default in the next release notes.

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

Yes—current main visibly buffers successful remote responses without a size limit, and the introduced tests and package harness cover declared and streamed oversize responses.

Is this the best way to solve the issue?

Unclear—the streamed cap and escape hatches are a narrow implementation, but maintainers must first approve the 100 MB default as an intentional upgrade behavior.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: This is a bounded reliability and resource-safety improvement with an intentional, recoverable compatibility impact.
  • merge-risk: 🚨 compatibility: The new default rejects previously successful remote PDFs above 100 MB unless callers set an explicit override.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is the remote URL reader; the supplied package harness runs the installed library and CLI through a real local HTTP server and the PR body records an after-fix 100,000,611-byte built-CLI run showing default rejection and successful exact/disabled overrides.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the remote URL reader; the supplied package harness runs the installed library and CLI through a real local HTTP server and the PR body records an after-fix 100,000,611-byte built-CLI run showing default rejection and successful exact/disabled overrides.

Evidence

What I checked:

  • Current main remains unbounded: The current main parent reads a successful remote response with response.arrayBuffer() and has no response-size option, so this PR still addresses a live gap rather than duplicating main. (src/input.ts:79, f356c4565305)
  • Bounded implementation: The introduced code resolves a 100 MB default, rejects oversized declared lengths, counts streamed body bytes, cancels rejected reads, and preserves a typed PdfBudgetError. (src/input.ts:117, d5ca3a79d344)
  • Library and CLI escape hatches: Extraction forwards the new option to document opening, while the CLI validates and forwards --fetch-max-bytes for both extraction and rendering. (src/engine.ts:98, d5ca3a79d344)
  • Coverage and after-fix proof: The package E2E path exercises installed-package library and CLI reads against a real local HTTP server, including size limits, disabled/exact overrides, CLI output, invalid values, streamed bodies, and cleanup. The captured PR body also reports a successful built-CLI run against a valid 100,000,611-byte PDF. (scripts/test-package-e2e.mjs:193, d5ca3a79d344)
  • Related feature history: The existing 30-second remote-read deadline was introduced by the prior merged remote-input change, providing relevant ownership context for the new size boundary. (src/input.ts:44, 1dbe5249df49)
  • Release status: The branch head is newer than v0.3.1 and is not contained by any local release tag, so this behavior has not shipped. (CHANGELOG.md:9, d5ca3a79d344)

Likely related people:

  • SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Record maintainer approval of the 100 MB default and its documented upgrade escape hatch.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-09-02T21:50:10.606Z sha 7dbb0ba :: blocked before merge. :: [P1] Expose a CLI override for the fetch limit
  • reviewed 2026-09-03T01:16:40.622Z sha 7dbb0ba :: blocked before merge. :: [P1] Expose the fetch-size override through the CLI
  • reviewed 2026-09-03T07:51:40.675Z sha 7dbb0ba :: blocked before merge. :: [P1] Expose the fetch-size override through the CLI
  • reviewed 2026-09-03T11:57:55.767Z sha 7dbb0ba :: blocked before merge. :: [P1] Expose the fetch-size override through the CLI
  • reviewed 2026-09-03T15:03:43.032Z sha 7dbb0ba :: blocked before merge. :: [P1] Expose the fetch-size override through the CLI
  • reviewed 2026-09-03T21:52:13.633Z sha 7dbb0ba :: blocked before merge. :: [P1] Expose the fetch-size override through the CLI
  • reviewed 2026-09-04T01:57:13.651Z sha 7dbb0ba :: blocked before merge. :: [P1] Expose the fetch-size override through the CLI
  • reviewed 2026-09-04T08:01:03.827Z sha ebbca58 :: blocked before merge. :: none

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. P2 Normal priority bug or improvement with limited blast radius. 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 P2 Normal priority bug or improvement with limited blast radius. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 2, 2026
Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@SebTardif
SebTardif force-pushed the fix/http-pdf-body-cap branch from ebbca58 to d5ca3a7 Compare September 4, 2026 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant