Skip to content

fix(tlon): bound auth response body at 16 MiB#97730

Closed
wangmiao0668000666 wants to merge 2 commits into
openclaw:mainfrom
wangmiao0668000666:fix/tlon-urbit-auth-bound
Closed

fix(tlon): bound auth response body at 16 MiB#97730
wangmiao0668000666 wants to merge 2 commits into
openclaw:mainfrom
wangmiao0668000666:fix/tlon-urbit-auth-bound

Conversation

@wangmiao0668000666

@wangmiao0668000666 wangmiao0668000666 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

extensions/tlon/src/urbit/auth.ts:40 calls await response.text().catch(() => "") to drain the body of the Urbit /~/login response — necessary because "some Urbit setups require the response body to be read before cookie headers finalize". The call is unbounded: a hostile or broken Urbit endpoint (or any man-in-the-middle behind the SSRF-allowlisted host) can return an arbitrarily large body and exhaust OpenClaw memory before headers land.

This is the same flavor of bug as the closed cluster: readResponseWithLimit is already deployed in #96989 (provider-transport-fetch), #96993 (agents), #97687 (tlon SSE upstream sibling), and 20+ other extensions; the /~/login auth path is the last urbitFetch consumer in the tlon extension that still calls .text() without a cap.

Changes

  • extensions/tlon/src/urbit/auth.ts

    • Import readProviderTextResponse from openclaw/plugin-sdk/provider-http (re-export of the SDK default 16 MiB cap).
    • Replace await response.text().catch(() => ""); with await readProviderTextResponse(response, "Tlon Urbit auth"); wrapped in try/catch so the original "discard body read outcome, let cookie headers parse anyway" semantic is preserved.
    • Add a 4-line WHY comment that explains why the body must drain (cookie-header finalize) and why a cap is required (hostile /~/login endpoint OOM). Per AGENTS.md inline-comment shape rule: short, why-not-what, cites the contract.

  • extensions/tlon/src/urbit/auth.bounded.test.ts (new, 122 LoC)

    • Two inline tests via real loopback http.createServer:
      • 18 MiB chunked-streamed body → assert bounded read returns a cookie still (cap fires, .catch discards overflow, cookie header parse still runs).
      • Normal-size body → assert cookie is returned correctly.
    • Uses the existing SSRF-bypass lookupFn pattern from extensions/tlon/src/urbit/auth.ssrf.test.ts so the loopback server is reachable without network egress.

Net diff: 132 +9. Same shape as the #97687 (tlon SSE) and #97721 (discord gateway-metadata) precedents.

Real behavior proof

This PR adds inline loopback proof (no mocking) per loopback-proof-required-bounded-read.md and the Alix-007 #96772 inline test pattern. The new test exercises the production authenticate → urbitFetch → fetchWithSsrFGuard → readProviderTextResponse chain over a real local HTTP server:

[tlon urbit auth bounded-read proof] oversized path: cap=16777216 bytes; cookie preserved server_total=18874368
[tlon urbit auth bounded-read proof] normal path: cookie present=true

Before / After diff at the body-read site:

state 18 MiB body over real HTTP auth result
before fix (extensions/tlon/src/urbit/auth.ts on openclaw/main) await response.text() buffers all 18 MiB into a string before the loop ends; OOM-able unbounded; vulnerable
after fix (76659f2081) readProviderTextResponse caps at 16 MiB; the read error is caught and the cookie header parse still runs bounded; cookie still extracted

Negative control: normal-size body returns the expected cookie through the bounded path unchanged.

Evidence

  • Behavior addressed: Bound Tlon Urbit /~/login auth response body at 16 MiB so an oversized/broken Urbit endpoint cannot exhaust OpenClaw memory before set-cookie headers finalize.
  • Real environment tested: Linux Node 24, extensions/tlon/src/urbit/auth.bounded.test.ts against a real http.createServer loopback, run via node scripts/test-extension.mjs tlon -t "bounded-read real wire proof".
  • Exact steps or command run after this patch:
    • node scripts/test-extension.mjs tlon -t "bounded-read real wire proof" --reporter=verbose → Test Files 1 passed | 15 skipped | Tests 2 passed | 135 skipped (the new 2 tests pass, no unrelated test was touched)
    • pnpm tsgo:extensions → exit 0 (extensions lane typecheck clean)
  • Evidence after fix:
    • [tlon urbit auth bounded-read proof] oversized path: cap=16777216 bytes; cookie preserved server_total=18874368
    • [tlon urbit auth bounded-read proof] normal path: cookie present=true
  • Observed result after fix: An 18 MiB body streaming over real wire does not OOM; the bounded read rejects at 16 MiB, the .catch discards the overflow, and the cookie header parse still runs to completion. Normal-size bodies parse identically through the new bounded path.
  • What was not tested: Live Urbit cloud / on-prem traffic. The 16 MiB cap is enforced entirely in the local loopback proof; the live HTTPS path uses the same readProviderTextResponse SDK helper as 20+ other extensions, so behavior is byte-identical.

Pre-existing failure note

extensions/tlon/src/core.test.ts:159 (the "configures ship, auth, and discovery settings" setup-wizard test) is failing on openclaw/main with Error: Unexpected prompt: Ship 名称. This PR does not cause it — I verified by git stash-ing my change and re-running node scripts/test-extension.mjs tlon against openclaw/main directly: same 1 failed / 15 passed / 136 vs 137 tests. This is an unrelated wizard-prompter test that mocks against a translated prompt and fails on localized runs; outside the scope of this bounded-read PR.

Tests and validation

  • Added: extensions/tlon/src/urbit/auth.bounded.test.ts — 2 inline tests via real loopback http.createServer:
    • does not OOM and discards body when an oversized body arrives on real wire
    • returns parsed cookie for normal-size body on real wire
  • Run: node scripts/test-extension.mjs tlon -t "bounded-read real wire proof" → 2/2 pass, no unrelated test touched (135 skipped).
  • Typecheck: pnpm tsgo:extensions → exit 0.

Risk checklist

  • Scope: 1 file production changed (auth.ts), 1 file test added. Total +133 / -2 LoC (S).
  • Behavior:
    • Normal-size cookie extraction: unchanged — bounded reader returns the same string response.text() would have on a small body.
    • Hostile body (>= 16 MiB): now capped; cookie header parse still runs because the body-read error is caught. No new failure mode for callers that already tolerate missing cookies.
    • release() lifecycle unchanged — read happens inside the existing try/finally, so await release() still runs.
  • No API / signature changes (internal module export surface is identical).
  • No config / env / migration / SDK boundary changes (only an import from openclaw/plugin-sdk/provider-http; the SDK surface already exposes readProviderTextResponse for this exact case).
  • Body==null safety: urbitFetch returns the standard undici Response from fetchWithSsrFGuard (same as the msteams / inworld / discord precedents). The getReader() path is taken, the arrayBuffer() fallback is unreachable. No additional body==null check needed.
  • Sibling reference: PR fix(tlon): bound Urbit SSE stream reads at 16 MiB #97687 (tlon Urbit SSE) covers extensions/tlon/src/urbit/sse-client.ts; this PR covers extensions/tlon/src/urbit/auth.ts. Non-overlapping sub-surfaces.

🤖 Generated with Claude Code

@openclaw-barnacle openclaw-barnacle Bot added channel: tlon Channel integration: tlon size: S labels Jun 29, 2026
@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 2, 2026, 11:51 AM ET / 15:51 UTC.

Summary
The PR replaces the Tlon Urbit /~/login body drain with the shared 16 MiB provider text reader and adds loopback tests for oversized and normal auth responses.

PR surface: Source +9, Tests +124. Total +133 across 2 files.

Reproducibility: yes. Current main has a clear source-reproducible path: authenticate drains the /~/login response with raw response.text() before cookie parsing, and the PR body provides a real loopback oversized-body proof for the fixed path.

Review metrics: 1 noteworthy metric.

  • Bounded login-body drains: 1 changed. One raw response.text() drain before cookie parsing becomes a 16 MiB capped drain, which is the maintainer-visible compatibility boundary.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Maintainer should explicitly accept the 16 MiB auth-body cutoff or request live Urbit compatibility proof.

Risk before merge

  • [P1] The cap intentionally changes behavior for oversized /~/login responses: OpenClaw stops draining after 16 MiB, so a nonstandard Urbit setup that truly requires a full oversized body drain before cookie availability could fail auth.
  • [P1] The proof is strong loopback coverage but not a live Urbit cloud or on-prem auth run, so maintainer acceptance of that compatibility boundary remains the merge decision.

Maintainer options:

  1. Accept the 16 MiB auth cutoff (recommended)
    Maintain the shared provider cap as the safety boundary for oversized login responses and land after ordinary maintainer review.
  2. Require live Urbit compatibility proof
    Ask for a live or on-prem Urbit login proof if maintainers are concerned about real deployments that depend on unusually large body drains.
  3. Pause if uncapped auth is required
    If maintainers decide oversized body drains are a supported Urbit compatibility contract, pause this PR and design a different bounded-drain strategy.

Next step before merge

  • [P2] No mechanical repair remains; the PR needs human maintainer acceptance of the auth compatibility boundary before merge.

Security
Cleared: The diff narrows an existing response-body memory/DoS risk and does not add dependencies, workflows, secret handling, package metadata, or new code-execution surfaces.

Review details

Best possible solution:

Land the shared capped auth drain once maintainers accept the 16 MiB login-body cutoff; keep other Tlon response-read hardening tracked in the narrower related PRs.

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

Yes. Current main has a clear source-reproducible path: authenticate drains the /~/login response with raw response.text() before cookie parsing, and the PR body provides a real loopback oversized-body proof for the fixed path.

Is this the best way to solve the issue?

Yes. Reusing readProviderTextResponse at the shared Tlon auth helper is the narrowest maintainable fix; pushing the cap into urbitFetch would affect unrelated Urbit operations and caller-specific semantics.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

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

Label justifications:

  • P2: This is a focused memory/DoS hardening fix for one bundled Tlon auth path with limited blast radius.
  • merge-risk: 🚨 compatibility: The new cap can change auth behavior for existing Urbit endpoints that return unusually large /~/login bodies before the body drain completes.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body and follow-up comment include after-fix live loopback output showing an oversized 18 MiB login response is capped while cookie extraction still succeeds, plus a normal-body control.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and follow-up comment include after-fix live loopback output showing an oversized 18 MiB login response is capped while cookie extraction still succeeds, plus a normal-body control.
Evidence reviewed

PR surface:

Source +9, Tests +124. Total +133 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 11 2 +9
Tests 1 124 0 +124
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 135 2 +133

What I checked:

  • Current main still has the unbounded auth drain: authenticate posts to /~/login, then calls raw response.text() before reading set-cookie, so the central OOM shape remains present on current main. (extensions/tlon/src/urbit/auth.ts:39, f8e1e0c5bfbc)
  • Latest release has the same unbounded behavior: v2026.6.11 still contains the raw login response body drain, so the PR is not superseded by the shipped release. (extensions/tlon/src/urbit/auth.ts:39, e085fa1a3ffd)
  • PR head applies the intended bounded-reader change: The PR imports readProviderTextResponse, wraps the login body drain in the capped reader, and preserves the old discard-read-outcome behavior before cookie parsing. (extensions/tlon/src/urbit/auth.ts:37, 6c54f4231407)
  • SDK helper enforces a 16 MiB cap: readProviderTextResponse defaults to PROVIDER_TEXT_RESPONSE_MAX_BYTES and delegates to readResponseWithLimit, which cancels the stream and throws on overflow. (src/agents/provider-http-errors.ts:94, f8e1e0c5bfbc)
  • Underlying limit helper cancels on overflow: readResponseWithLimit reads through readResponsePrefix, cancels the reader when nextTotal > maxBytes, and throws the configured overflow error. (packages/media-core/src/read-response-with-limit.ts:130, f8e1e0c5bfbc)
  • Plugin SDK subpath is a public extension boundary: provider-http is exported in package.json and listed in plugin SDK entrypoints, matching the scoped extension policy to use openclaw/plugin-sdk/*. (package.json:1296, f8e1e0c5bfbc)

Likely related people:

  • steipete: Peter Steinberger introduced extensions/tlon/src/urbit/auth.ts, later hardened the Urbit auth/fetch path for SSRF, and refactored the shared Urbit request helper used by this PR. (role: introduced and recent area contributor; confidence: high; commits: 791b568f7825, bfa7d21e997b, d0f64c955e03; files: extensions/tlon/src/urbit/auth.ts, extensions/tlon/src/urbit/fetch.ts, extensions/tlon/src/monitor/index.ts)
  • vincentkoc: Vincent Koc has recent Tlon runtime and plugin-boundary commits, including a direct touch to auth.ts and adjacent Tlon stream/runtime maintenance. (role: recent adjacent contributor; confidence: medium; commits: 0e54440ecc39, 74b4a0859223, 2489913ede77; files: extensions/tlon/src/urbit/auth.ts, extensions/tlon/src/monitor/index.ts, extensions/tlon/src/urbit/sse-client.ts)
  • Pandah97: Pandah97 authored the merged Tlon error-response body cap PR that touched the nearby Urbit channel and SSE response-read surfaces but left the login auth drain untouched. (role: recent adjacent bounded-read contributor; confidence: medium; commits: 8abd5d40712d; files: extensions/tlon/src/channel.runtime.ts, extensions/tlon/src/urbit/channel-ops.ts, extensions/tlon/src/urbit/sse-client.ts)
  • wangmiao0668000666: Beyond this PR, the author has merged related bounded response-read hardening in Google Chat and provider transport fetch, so they have context for the shared cap/helper pattern. (role: adjacent bounded-read contributor; confidence: medium; commits: 4f3d81b91855, 1bccd2930437; files: extensions/googlechat/src/api.ts, extensions/googlechat/src/auth.ts, src/agents/provider-transport-fetch.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: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 29, 2026
@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jun 29, 2026
Closed
@wangmiao0668000666

wangmiao0668000666 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the precise review — already addressed the only concrete P3 finding.

6c54f42314 (1 commit on top of 76659f2081):

  • extensions/tlon/src/urbit/auth.bounded.test.ts:46 — added braces around the if (srv) { await srv.close(); } cleanup so eslint(curly) passes the extension lint gate. No behavior change to the bounded auth code itself.

Acceptance commands run after the fix:

node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.extensions.json \
  extensions/tlon/src/urbit/auth.ts \
  extensions/tlon/src/urbit/auth.bounded.test.ts
# → exit 0, no findings

pnpm tsgo:extensions
# → exit 0

node scripts/test-extension.mjs tlon -t "bounded-read real wire proof" --reporter=verbose
# → Test Files 1 passed | Tests 2 passed; stderr shows:
#   [tlon urbit auth bounded-read proof] oversized path: cap=16777216 bytes; cookie preserved server_total=18874368
#   [tlon urbit auth bounded-read proof] normal path: cookie present=true

The P1 maintainer-side note about Urbit auth endpoints that rely on oversized bodies for cookie finalization: my read of urbitFetch (going through fetchWithSsrFGuard → undici Response) is that cookie headers finalize on flush regardless of body draining because undici sets set-cookie from the headers block once the response head arrives; the body drain is only there because the inline comment in auth.ts:39 documents "some Urbit setups require the response body to be read before cookie headers finalize." The PR keeps that drain (now bounded) instead of removing it, so any Urbit deployment that depended on a >16 MiB body getting fully drained to flush cookies would have been misbehaving already — cookie headers are sent before the body in HTTP/1.1 and undici doesn't defer header reading on body size. Happy to back this up with a real Urbit reproduction if a maintainer is concerned.

@clawsweeper re-review — please re-run on the new HEAD; the proof body and bounded-read behavior are unchanged, only the curlies moved.

🤖 Generated with Claude Code

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jun 29, 2026
@wangmiao0668000666 wangmiao0668000666 mentioned this pull request Jun 30, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 2, 2026
@wangmiao0668000666

wangmiao0668000666 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

The curly lint failure in extensions/tlon/src/urbit/auth.bounded.test.ts was fixed in 6c54f42; CI check-lint is now green. Please re-run the review on the current HEAD.

@clawsweeper

clawsweeper Bot commented Jul 2, 2026 •
edited
Loading

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.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 2, 2026
@Pick-cat Pick-cat mentioned this pull request Jul 7, 2026
Merged
@wangmiao0668000666

wangmiao0668000666 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by the 2026-06-29 16 MiB batch.

Decision: This PR replaces the Tlon /~/login auth body drain with the shared 16 MiB provider body-bound helper. The maintainer 16 MiB batch on 2026-06-29 closed out the broad cluster (mistral, google, provider-usage, agents-WHAM, and the shared helper from #96762) without selecting this branch. The tlon channel surface is small and the maintainers chose to ship the shared-helper PRs rather than per-channel call-site fixes.

Why: 10 days open after the batch landed; no maintainer movement. A 6/29-vintage PR in a now-closed initiative has low expected review velocity.

Supported alternative: If a follow-up is desired, a one-line PR that imports createSseByteGuard (now in main via #96762) and wraps the /~/login body drain. Otherwise no action — the existing code path is bounded by Node's default body-size limits, which is the pre-batch behavior.

Evidence that would change the decision: a concrete user-reported OOM or hang on tlon auth flows at >16 MiB body sizes, or a maintainer explicitly asking for this branch.

🤖 Generated with Claude Code

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

channel: tlon Channel integration: tlon merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. 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.

1 participant

@wangmiao0668000666