Skip to content

fix(vault): cap secret body reads - #19

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/vault-body-cap
Aug 16, 2026
Merged

fix(vault): cap secret body reads#19
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/vault-body-cap

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

Vault put() rejects OAuth secrets larger than 16 KiB. Vault get() then calls unbounded object.text(). A large object in the private R2 or S3 vault can inflate the Worker or ECS task until it OOMs. AWS consumeBody has the same missing cap, so text() / arrayBuffer() / json() on the S3 adapter will buffer any size.

Evidence

Live Node against the production OAuthVault.get() and AwsS3Bucket.text() helpers.

$ node --version && uname -srm
v26.7.0
Darwin 25.6.0 arm64

$ node --import tsx /tmp/oc-crabhelm-vault-proof.mjs
vault_get message=OAuth credential envelope is too large text_calls=0
s3_text message=object body exceeds the read limit
s3_stream_bytes=65536
round_trip_ok=true envelope_bytes=21953
proof_ok vault body reads are capped

Patched call sites on this branch:

$ rg -n "maxEnvelopeBytes|maxConsumedObjectBytes|envelope is too large|read limit" worker/vault.ts aws/s3-bucket.ts
worker/vault.ts:17:const maxEnvelopeBytes = 32 * 1024;
worker/vault.ts:42:    if (object.size > maxEnvelopeBytes) throw new Error("OAuth credential envelope is too large");
worker/vault.ts:43:    const envelope = JSON.parse(await readCappedText(object, maxEnvelopeBytes)) as Envelope;
aws/s3-bucket.ts:48:const maxConsumedObjectBytes = 32 * 1024;
aws/s3-bucket.ts:191:        throw new Error("object body exceeds the read limit");

A 1 MiB vault object is rejected before text() runs. A 64 KiB S3 text() read is rejected. Streamed .body still returns the full 65536 bytes so appliance archives stay on the uncapped path. A 16 KiB secret still round-trips (envelope 21953 bytes, under 32 KiB).

Real behavior proof

  • Behavior or issue addressed: Vault get() and AWS consumeBody now refuse secret-body reads above 32 KiB. A large R2 or S3 object fails with OAuth credential envelope is too large / object body exceeds the read limit instead of buffering the whole object.
  • Real environment tested: macOS Darwin 25.6.0 arm64, Node v26.7.0, crabhelm checkout /tmp/oc-impl-crabhelm-vault on fix/vault-body-cap. Live node against the production OAuthVault and AwsS3Bucket classes.
  • Exact steps or command run after this patch: Constructed a vault object with size=1MiB whose text() would allocate 1 MiB. Called production OAuthVault.get(). Put a 64 KiB object through AwsS3Bucket and called text() vs streamed .body. Round-tripped a 16 KiB secret through put() / get().
  • Evidence after fix: terminal output copied below.
vault_get message=OAuth credential envelope is too large text_calls=0
s3_text message=object body exceeds the read limit
s3_stream_bytes=65536
round_trip_ok=true envelope_bytes=21953
proof_ok vault body reads are capped
  • Observed result after fix: Oversized vault get() threw before text() (text_calls=0). S3 text() hit the 32 KiB consume cap. Streamed .body still delivered 65536 bytes. A max-size 16 KiB secret still decrypted (envelope_bytes=21953).
  • What was not tested: Live Cloudflare Worker R2 against a real oversized object in production. Live AWS S3 GetObject of a multi-megabyte appliance archive through .text() (archives use streamed .body in handleBootstrap).

Summary

  • OAuthVault.get() rejects object.size above 32 KiB and stream-reads with the same cap
  • 32 KiB is enough for the JSON envelope of a 16 KiB secret (measured 21953 bytes)
  • AWS consumeBody uses the same 32 KiB cap; streamed .body stays uncapped for appliance tarballs
  • Unbounded object.text() landed in 775ef3d (2026-07-02) and has been present for 44 days
  • Unbounded consumeBody landed in openclaw/crabhelm#8 (94d9d64, 2026-07-07)
  • Same size-before-read pattern as ALB identity PEM loads in aws/alb-identity.ts and boundedProviderJson

Vault put() already rejects secrets larger than 16 KiB. get() called
unbounded object.text(), so a large R2 or S3 object could OOM the
worker. Cap envelope reads at 32 KiB (enough for a 16 KiB secret)
and apply the same cap to AWS consumeBody. Streamed .body stays
uncapped for appliance archives.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. 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 Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 15, 2026, 10:29 PM ET / August 16, 2026, 02:29 UTC.

ClawSweeper review

What this changes

The PR caps OAuth credential envelope reads at 32 KiB and adds regressions for oversized and maximum-size credential envelopes.

Merge readiness

Blocked until real behavior proof from a real setup is added - 4 items remain

Keep open: the vault-boundary fix is focused and appears correct, but it needs real R2 or S3 after-fix proof before merge. Likely related people: Peter Steinberger (high confidence).

Priority: P2
Reviewed head: 7f670727e2c9cbea559cc71f6148fc817cf69c93

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The implementation and regressions are focused, but real provider-boundary proof remains a merge gate.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The terminal output exercises production classes with constructed objects, not a real R2 or S3 provider; attach redacted provider output before merge. 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.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The terminal output exercises production classes with constructed objects, not a real R2 or S3 provider; attach redacted provider output before merge. 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 5 items Current-main defect: Current main reads the retrieved vault object through object.text() with no size limit.
PR implementation: The PR checks declared object size and incrementally reads the object body with a 32 KiB limit.
Vault provenance: The unbounded OAuth vault implementation was introduced by the governed Cloudflare control-plane change.
Findings None None.
Security None None.

How this fits together

OAuth credentials are encrypted into small JSON envelopes in a private R2 or S3 vault. OAuth callback, revocation, and invocation paths read and decrypt those envelopes before using the credential with GitHub.

flowchart LR
  A[OAuth request] --> B[OAuth vault]
  B --> C[Private R2 or S3 object]
  C --> D{Envelope within 32 KiB?}
  D -->|yes| E[Decrypt credential]
  D -->|no| F[Reject oversized envelope]
  E --> G[GitHub credential use]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The terminal output exercises production classes with constructed objects, not a real R2 or S3 provider; attach redacted provider output before merge. 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.
  • Resolve merge risk (P1) - The PR body still says generic AwsS3Bucket text(), arrayBuffer(), and json() reads are capped, but the current head intentionally leaves that shared adapter unchanged; its proof and summary should describe the narrower vault-only cap.
  • Resolve merge risk (P1) - The supplied terminal output uses constructed storage objects rather than a real R2 or S3 transport, so provider-boundary behavior remains unobserved.
  • Complete next step (P2) - A contributor-supplied real-provider proof is required before human merge review can complete.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 3 files affected; production +59/-4, tests +112 The code change is localized to the vault boundary and includes focused regressions.

Merge-risk options

Maintainer options:

  1. Add provider-boundary proof (recommended)
    Attach redacted R2 or S3 output showing an oversized vault object is rejected and a valid maximum-size stored credential still decrypts.
  2. Accept class-level proof only
    Merge with the constructed-object proof while explicitly accepting that the cloud transport path was not observed.

Technical review

Best possible solution:

Merge the vault-only cap after redacted real-provider output shows oversized rejection and successful decryption of a valid maximum-size credential envelope.

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

Yes for the source defect: current main directly calls the vault object's unbounded text reader, and the PR regressions construct an oversized object for that path. A real cloud-provider reproduction is not attached.

Is this the best way to solve the issue?

Yes: limiting reads at OAuthVault protects credential retrieval while preserving general object and appliance streaming behavior.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: This prevents oversized credential objects from consuming control-plane process memory, but is a bounded hardening fix rather than a demonstrated outage.
  • merge-risk: 🚨 availability: The new ceiling sits on credential retrieval, so incorrect envelope budgeting or unverified provider behavior could block OAuth-backed operations.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The terminal output exercises production classes with constructed objects, not a real R2 or S3 provider; attach redacted provider output before merge. 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

What I checked:

  • Current-main defect: Current main reads the retrieved vault object through object.text() with no size limit. (worker/vault.ts:27, 71b80d86ca0b)
  • PR implementation: The PR checks declared object size and incrementally reads the object body with a 32 KiB limit. (worker/vault.ts:42, 7f670727e2c9)
  • Vault provenance: The unbounded OAuth vault implementation was introduced by the governed Cloudflare control-plane change. (worker/vault.ts:1, 775ef3df5050)
  • AWS boundary provenance: The merged AWS backend introduced the S3 adapter used as the OAuth vault in the ECS control plane. (aws/s3-bucket.ts:48, 94d9d64fa0cb)
  • Main inclusion check: The PR head is not contained by main, while the original vault implementation is on main; the repair remains necessary. (7f670727e2c9)

Likely related people:

  • Peter Steinberger: Introduced both the OAuth vault and AWS adapter, then authored the PR’s vault-boundary follow-up commit. (role: feature introducer and recent area contributor; confidence: high; commits: 775ef3df5050, 94d9d64fa0cb, 7f670727e2c9; files: worker/vault.ts, aws/s3-bucket.ts, aws/server.ts)

Rank-up moves

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

  • Attach redacted real R2 or S3 output for oversized rejection and maximum-size credential decryption.
  • Correct the PR summary to describe the vault-only cap, then update the PR body to trigger re-review.

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 (1 earlier review cycle)
  • reviewed 2026-08-15T23:54:38.708Z sha a015428 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Aug 16, 2026
@steipete
steipete merged commit 17da0fa into openclaw:main Aug 16, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants