Skip to content

fix(api): admit every stored configuration and escaped workspace file - #2177

Merged
freeqaz merged 2 commits into
mainfrom
fix-1118
Oct 11, 2026
Merged

freeqaz merged 2 commits into
mainfrom
fix-1118

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Request-body limits disagreed with the code that consumes the bodies (finding 1118):

  • createConfiguration and updateConfiguration used the general 64 KiB limit, while provisionAgent accepts 448 KiB and the Kubernetes Configuration Driver stores up to 1 MiB of values. A dedicated Agent provisioned with a ~100 KB Configuration could never save it again (413). That includes the console's Slack channel editor, which PATCHes the whole Configuration, and an embedded Agent launched from a Preset with such values.
  • putAgentWorkspaceFile caps content at 16 KiB of UTF-8 but the body at 48 KiB (3x). JSON can need six bytes per content byte, so 16 KiB of U+0001 (a 98 KB body as JSON.stringify writes it) or 8192 two-byte letters sent as ASCII-only JSON (49,166 bytes) got 413. The Agent create route already reserves 6x.

Change

  • Configuration POST/PATCH: bodyLimit 6 x (1 MiB + 32 KiB), about 6.2 MiB: the Driver's 1 MiB of values plus the envelope (kind and up to 64 Secret bindings, under 32 KiB), all written in six-byte JSON escapes. An explicit maxBodyBytes still wins, as on the other large routes.
  • To keep that from becoming a memory lever for callers who cannot write Configurations, these two routes now work like Preset writes: onRequest resolves the caller and runs the new OpenClawController.authorizeConfigurationWrite (configuration:create on the Namespace, or configuration:update on the Configuration) before the body is read. Callers without the grant get the same 403/404 and audit row as before, and the controller never buffers their body. The transaction repeats the check, followed by the Secret binding checks that need the body.
  • Why this option rather than the 448 KiB provisioning limit: 448 KiB still refuses Configurations the Driver stores (448 KiB to 1 MiB, for example from a 1 MiB Preset template launched as an embedded Agent). With the before-body check, the memory per request is the same budget Preset writes already have, available only to principals allowed to write Configurations.
  • Workspace file PUT: 6 x 16 KiB + 1 KiB (97 KiB).
  • Reference docs (configuration, agents, development settings) and the configuration-driver flow describe the new limits. No other behaviour changes; provisioning keeps 448 KiB.

Verification

  • New tests/integration/configuration-body-limits.test.mjs (real HTTP server):
    • a ~100 KB Configuration is created and PATCHed back; a just-under-1 MiB Configuration the Kubernetes Driver accepts is created and updated with every character sent as a six-byte escape (~6 MiB body); one byte over the route limit gets 413 on POST and PATCH;
    • a caller without the grant who declares 6 MiB and sends only 64 KiB gets the 403 and its audit row without the controller waiting for the body (POST and PATCH);
    • the transaction re-checks the grant after the before-body check.
  • tests/conformance/workspace-files.test.mjs: a PUT of 16 KiB of U+0001, of ASCII-escaped two-byte letters, and of fully escaped ASCII succeeds; the existing over-limit case now sends one byte over the new limit and still gets 413.
  • All four new tests fail on main.
  • tests/conformance/occ-api-security.test.mjs: an explicit smaller maxBodyBytes still bounds both Configuration routes (as it does Preset routes).
  • Mutation check: 11/11 mutants killed (Configuration limit back to default, set to 448 KiB, or +1; maxBodyBytes ignored on the Configuration routes; create or update dropped from the before-body set; update checks the create grant; before-body check a no-op; workspace limit without envelope, +1, or back to 48 KiB).
  • Regression run: configuration-controller, presets-controller, occ-api, console-api, http-errors, configuration-occ, occ-api-security, secret-occ, split-layout-tenants, controller-lifecycle, secret-api, runtime-credentials-api, repository-credentials-admission, dependency-unavailable-log: 415/415 pass. tsc --build, eslint and prettier on touched files clean.
  • Touched test files 20/20 at 2 CPUs. New test file mapped in checks-baseline-1; run-tests.mjs audit clean.
  • Open PRs touching the shared test files and suite manifest (feat(openshell): support x-api-key model credentials #2070, feat(openshell): integrate OAuth refresh and upstream credential retrieval #1785, feat(dev): expose first-agent Control UI with OpenShell #1750, Enable dedicated OpenClaw from Console with pinned native runtime #1685, Enable Codex OAuth with OpenShell-owned credentials #1559, Support OpenAI-compatible Codex model endpoints #913): git merge-tree shows the same conflicted files against this branch as against main, so no new conflicts.

Risks

  • For Configuration writes, an unauthorized caller who also sends an invalid body now gets 403 instead of 400, the same ordering Preset writes have had since the before-body check landed.
  • An authorized writer can make the controller buffer about 6.2 MiB per Configuration write, about the same budget as a Preset write. Deeply nested JSON in such a body already answers 500 and costs heap during validation (Preset writes too); that is tracked separately.

Configuration POST/PATCH used the general 64 KiB body limit, while
provisioning accepts 448 KiB and the Kubernetes Configuration Driver
stores up to 1 MiB of values. A dedicated Agent provisioned with a
~100 KB Configuration could never save it again, including from the
console's Slack editor, which PATCHes the whole Configuration; an
embedded Agent launched from such a Preset failed the same way.

Configuration writes now allow 6 MiB + 64 KiB: the Driver's 1 MiB of
values written with six-byte JSON escapes, plus `kind` and up to 64
Secret bindings. As with Preset writes, onRequest runs the new
OpenClawController.authorizeConfigurationWrite (configuration:create on
the Namespace or configuration:update on the Configuration) before the
body is read, so only callers with the grant can make the controller
buffer a large body; the transaction repeats the check with the Secret
binding checks.

The workspace file PUT allowed 48 KiB (3x the 16 KiB content cap), so
16 KiB of U+0001 (98 KB as JSON) or ASCII-escaped two-byte letters got
413. It now allows 6 x 16 KiB + 1 KiB.

Finding 1118.
@freeqaz
freeqaz requested a review from a team as a code owner October 11, 2026 13:14
@clawsweeper

clawsweeper Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
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: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Oct 11, 2026
@clawsweeper

clawsweeper Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR raises Configuration and workspace-file request limits and authorizes Configuration writes before reading their bodies.

Example: An authorized user saves a Configuration containing 102,400 bytes of plugin notes.

  • Before: Configuration POST and PATCH return HTTP 413 under the general 64 KiB limit.
  • After: The operation-specific limit admits creation and subsequent replacement.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Useful, focused repair with both previous defects resolved; execution evidence remains limited to reported integration and conformance results.
Proof confidence 🦐 gold shrimp (3/6) This maintainer-authored PR is outside the ordinary contributor-proof gate; supplied verification exercises Fastify admission, Native IAM denial, and revoked-grant rechecks using fixture storage rather than a deployed stack.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Users cannot save otherwise valid large Configurations or maximally sized escaped workspace files.
Reason: The change aligns transport admission with existing supported content limits, and its maintainer-authored direction already supplies the owner decision.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it repairs current request-admission bugs, and both prior findings are resolved in source; no new actionable defect was found.

Priority: P2
Reviewed head: d2e349b607aa88bb77582968f2594efcd82a7104

Before merge

None.

Findings

None.

Provenance

  • Configuration use of the general transport default: original intent not found (unknown: The default is carried at the checkout history boundary; scoped history searches did not establish a Configuration-specific sizing rationale.)

Tests

  • Missing end-to-end proof: Separate shipped-composition acceptance is not supplied; the reported base failures and head successes cover the HTTP fixture path.
Agent review details

How this fits together

The Fastify API authenticates requests and applies transport limits before OCC authorizes exact resources, validates content, and invokes Configuration or workspace storage owners.

flowchart TB
  Client[Authenticated client] --> API[Fastify admission]
  API --> IAM[Exact resource IAM]
  IAM --> Body[Bounded JSON body]
  Body --> OCC[OCC transaction and authorization recheck]
  OCC --> Storage[Configuration or workspace owner]
  OCC --> Audit[Attributable audit]
Loading

Technical review

Best possible solution:

Retain the focused admission repair within the existing API, OCC authorization, and Driver ownership boundaries.

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

Source establishes the mismatch: 102,400-byte Configuration values exceed main's 64 KiB route budget, and 16,384 U+0001 characters require about 98 KiB of JSON against a 48 KiB workspace budget; supplied regression results were not independently executed.

Is this the best way to solve the issue?

The repair fits the existing owners, preserves explicit Configuration body-limit overrides and decoded validation, and repeats exact-resource authorization inside the transaction.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 220cc9572400.

Provenance checked

  • Workspace request-body budget changes intended behavior with a stated reason (71aa1b6: The workspace feature bounded four supported files and retained exact-Agent authorization and audit.)
  • Authorization before large request bodies keeps the original intent (3ccb7b1: Refuse principals without the write grant before buffering large bodies while retaining transaction authorization.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Current-main admission mismatch: Inspected main retains a 64 KiB general limit and 48 KiB workspace-body limit, while Kubernetes Configuration validation accepts serialized values below 1 MiB. (apps/controller/src/index.ts:219, 220cc9572400)
  • Both prior defects repaired: The follow-up increases the budget to 6 × (1 MiB + 32 KiB), covering escaped binding names and references, and registers the integration file in both CI manifest inventories; the previous automation risk no longer applies. (apps/controller/src/index.ts:228, d2e349b607aa)
  • Authorization remains enforced before storage: The new pre-body method uses the same exact create/update targets as the existing transaction checks; Secret-binding authorization, ownership checks, and Driver validation still precede Driver mutations. (packages/occ/src/index.ts:3887, d2e349b607aa)
  • Established admission pattern and accepted tradeoff: fix(api): authorize preset writes before reading the body #1976 established authorization before large Preset bodies with transaction rechecks; fix(api): accept contract-sized Preset writes #1968 established escape-aware transport sizing. This maintainer-authored PR explicitly accepts the authorized-writer memory budget and earlier denial ordering. (apps/controller/src/index.ts:1875, 3ccb7b1df4db)
  • Verification boundary: The supplied tests exercise a real Fastify listener and Native IAM with in-memory platform state and passive Configuration storage; Kubernetes size validation is called separately. Full body and discussion inspection found test reports, without a separate deployed acceptance artifact. No target code or tests were executed during this review. (tests/integration/configuration-body-limits.test.mjs:92, d2e349b607aa)
  • Policy and history boundaries: Read the complete root AGENTS.md and architecture implementation status; no nested AGENTS.md or maintainer-notes directory was present. Feature history identifies workspace, transport-limit, and pre-body authorization contributors. Older OCC content searches encountered unavailable promisor blobs, so earlier introduction remains unclaimed; the checkout stayed clean. (AGENTS.md:1, d2e349b607aa)

Likely related people:

  • Eric Cai: Raw commit ab094dc adds apps/controller/src/index.ts:223 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: ab094dc676cf; files: apps/controller/src/index.ts)

Labels

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove merge-risk: 🚨 automation: Current PR review selected no merge-risk labels.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: Transport limits reject valid Configuration and workspace writes; the repair is focused and has limited blast radius.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rank-up moves

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

  • Extend the near-limit HTTP case with 64 valid distinct 253-character Secret-binding names, escaping their characters alongside the values, to protect the repaired envelope allowance.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only; repair and merge need explicit maintainer commands such as @clawsweeper autofix or @clawsweeper automerge.

History

Review history (1 earlier review cycle)
  • reviewed 2026-10-11T13:17:26.907Z sha 6a6f02b :: needs changes before merge. :: [P2] Include escaped Secret-binding names in the envelope budget | [P2] Register the new integration file in a CI suite

Reviewed October 11, 2026, 9:28 AM ET / 13:28 UTC (Revision 2).

…uite

Review follow-ups for finding 1118: the Configuration body limit is now
6 x (1 MiB + 32 KiB), so the envelope (kind and up to 64 Secret
bindings) is also covered in six-byte escapes. Map
configuration-body-limits.test.mjs in checks-baseline-1 (Suite Audit
refused the unmapped file), check that an explicit maxBodyBytes still
bounds the Configuration routes, and reflow the flow doc paragraph.
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 11, 2026 13:24 — with GitHub Actions Inactive
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 11, 2026 13:24 — with GitHub Actions Inactive
@clawsweeper clawsweeper Bot added 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. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Oct 11, 2026
@freeqaz
freeqaz merged commit 09766ec into main Oct 11, 2026
41 of 44 checks passed
@freeqaz
freeqaz deleted the fix-1118 branch October 11, 2026 13:29

This branch was previously deployed

1 inactive deployment
integration-qa-pr — d2e349b6 Deployed Oct 11, 2026 by freeqaz via QA advisory (kubernetes) #337
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant