feat: harden the ClawBox MCP (auth, structured errors, health, bash safety, durable webapps) - #178
Merged
Merged
Conversation
The CLI called /setup-api/* with no Authorization header, so once setup completes middleware.ts 307'd it to /login and JSON.parse choked on the login HTML ("Failed to parse JSON"). Read CLAWBOX_MCP_TOKEN from the env, falling back to the data/.mcp-token file the gateway pre-start script writes (the CLI is launched separately from the MCP server and may not inherit its env), and inject it as a Bearer header — mirroring clawbox-mcp.ts. Loaded lazily so token-free commands like `app list` still work.
… durable webapps
- Structured errors: wrap every tool handler (tool()) so failures return a
parseable { error, code, message, details } envelope; api() throws a typed
ApiError classified by status (AUTH_FAILED on 401/403, ENDPOINT_DOWN on 5xx, …).
- clawbox_health tool: verifies the MCP bearer + /setup-api/* reachability so a
broken token is diagnosable up front instead of via a cryptic tool failure.
- bash safety: hard-block destructive commands (rm -rf /, dd, mkfs, fork bombs,
…) unless allowDangerous:true; enforced at the spawnBackground chokepoint so
the agent tool can't bypass it. Git-safety patterns still only warn.
- Durable webapps: the webapps POST route and buildProject now register the app
in desktop preferences server-side (registerWebappInPreferences), so a webapp
created while the desktop is closed still appears on its next load instead of
relying on the lossy ui:pending-action handoff.
- Docs: mcp/README.md (auth flow, tool catalog, testing, failure modes).
|
Warning Review limit reached
More reviews will be available in 8 minutes and 24 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR hardens the ClawBox MCP server by adding bearer-token authentication, implementing structured error handling across all tools, enforcing bash command safety with override capability, and refactoring webapp deployment through a durable preference-registration mechanism. ChangesMCP Server Hardening, Tool Error Handling, & Webapp Deployment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Extract deployWebapp(appId, html, {name, color, icon}) — the one place that
writes data/webapps/<id>/{index.html,meta.json} and registers the app on the
desktop. The webapps POST route and buildProject both call it, so their
on-disk layout / meta.json shape / desktop registration can't drift (or be
half-applied by one caller forgetting a step).
- webapps POST: deploy+register only on create (name present); an update now
just rewrites index.html instead of clobbering meta.json's saved name.
- Fix code-projects.test.ts: stub @/lib/webapp-registry so buildProject's new
registration call doesn't hit real config IO — this was failing #178's `test`
job (EACCES mkdir /home/clawbox).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcp/clawbox-mcp.ts (1)
139-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
AUTH_FAILEDclassification is bypassed when auth redirects are auto-followed.For
/setup-api/*, redirect-follow can turn missing/invalid bearer into a 200 login HTML response; thenres.json()throws and gets classified asINVALID_RESPONSEinstead ofAUTH_FAILED.Suggested fix
async function api(path: string, options?: RequestInit) { const headers = new Headers(options?.headers); + if (!headers.has("accept")) headers.set("accept", "application/json"); if (API_TOKEN && !headers.has("authorization")) { headers.set("authorization", `Bearer ${API_TOKEN}`); } - const res = await fetch(`${API_BASE}${path}`, { ...options, headers }); + const res = await fetch(`${API_BASE}${path}`, { ...options, headers, redirect: "manual" }); if (!res.ok) { const body = await res.text().catch(() => ""); throw new ApiError(res.status, body); } return res.json(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/clawbox-mcp.ts` around lines 139 - 152, The api function can accidentally follow redirects and return the login HTML as a 200, causing res.json() to throw and be misclassified; update api (the async function api(path: string, options?: RequestInit)) to detect auth redirects for /setup-api/* by either forcing fetch to not auto-follow redirects (set options.redirect = 'manual') and treating any 3xx/redirect or redirected login URL as an auth failure, or by checking the response content-type (e.g., text/html) and the path prefix when res.ok is true and throwing an ApiError with an AUTH_FAILED classification (using API_TOKEN and API_BASE context) instead of calling res.json(); ensure the header injection logic (authorization via API_TOKEN) remains unchanged.mcp/clawbox-cli.ts (1)
55-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent auth redirects from being misreported as JSON parse failures.
fetchcurrently follows/setup-api/*auth redirects to/login, which can turn auth failures into HTML parse errors. Force JSON semantics for API calls to preserve actionable HTTP auth failures.Suggested fix
async function api(path: string, options?: RequestInit) { const headers = new Headers(options?.headers); + if (!headers.has("accept")) headers.set("accept", "application/json"); if (!headers.has("authorization")) { headers.set("authorization", `Bearer ${getApiToken()}`); } - const res = await fetch(`${API_BASE}${path}`, { ...options, headers }); + const res = await fetch(`${API_BASE}${path}`, { ...options, headers, redirect: "manual" }); if (!res.ok) { const body = await res.text().catch(() => ""); console.error(`Error ${res.status}: ${body}`); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcp/clawbox-cli.ts` around lines 55 - 63, The code is misreporting auth redirects as HTML/JSON parse failures because fetch follows redirects to /login; to fix, ensure API calls include explicit JSON semantics and avoid auto-following redirects: add headers.set("accept", "application/json") when constructing Headers (alongside the existing authorization header) and pass a redirect: "manual" into the fetch options unless options.redirect is already set, so the fetch call (using API_BASE, path, options, headers) will surface 3xx auth responses instead of returning HTML from /login.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcp/clawbox-cli.ts`:
- Around line 40-43: The CLI's root resolution (variable root) skips the
development-mode process.cwd() behavior used in src/lib/mcp-token.ts; update the
root resolution to match that logic by either importing and using the same
resolver from src/lib/mcp-token.ts or changing the assignment to prefer
process.env.CLAWBOX_ROOT, then process.cwd() in dev/local runs, and finally the
hardcoded "/home/clawbox/clawbox" fallback so the readFileSync(join(root,
"data", ".mcp-token")) logic will find the same token file as the server.
In `@mcp/clawbox-mcp.ts`:
- Around line 628-636: The fetch in checkApiEndpoint (and the health check
logic) currently calls await res.json() unconditionally so 401/403 or HTML
redirect pages can surface as JSON parse errors; change the logic to first
inspect res.status and immediately return explicit auth failures for 401/403
(e.g., detail `HTTP ${res.status} (token rejected)`), and only attempt
res.json() when res.ok and the Content-Type header indicates application/json —
otherwise return a non-JSON detail like `HTTP ${res.status} (non-JSON response)`
so auth/connectivity errors aren’t misdiagnosed as parse failures.
In `@mcp/README.md`:
- Around line 9-12: The fenced code block in the README lacks a language
identifier which triggers markdownlint MD040; update the block that contains the
diagram referencing clawbox-mcp.ts and clawbox-cli.ts to include a language tag
(e.g., use ```text) immediately after the opening backticks so the block reads
as a labeled text/code fence.
In `@src/app/setup-api/webapps/route.ts`:
- Around line 76-89: Change the truthy check for "name" into explicit payload
validation: treat a request as a create only when the body has a "name" property
that is a non-empty string (e.g. typeof name === "string" && name.trim().length
> 0) and call deployWebapp(appId, html, { name, color, icon }); if "name" is
present but empty return a 400 error; treat requests that do not include the
"name" property as updates but first verify the app exists under WEBAPPS_DIR
(check fs.stat or fs.access for path.join(WEBAPPS_DIR, appId)) and return 404 if
missing, otherwise continue with the existing mkdir/writeFile update flow;
ensure error responses have appropriate HTTP status codes instead of silently
falling into the wrong branch.
In `@src/lib/code-projects.ts`:
- Around line 474-477: The PR currently routes rebuilds through
deployWebapp(projectId, html, { name, color }), but deployWebapp performs
create-time persistence (rewrites meta.json, sets icon: "" when no icon passed
and re-registers the app in prefs), which wipes existing icon and clears
pref:hidden_installed on rebuilds; modify the flow so rebuilds only write
deployed files and do not perform initial registration: split deployWebapp into
two responsibilities (e.g., writeDeployedFiles or deployFiles and registerWebapp
or keep an explicit updateMode flag on deployWebapp) and when called from the
rebuild path use the "update" mode or deployFiles helper that preserves existing
meta.json fields (icon) and does not touch preferences (pref:hidden_installed)
or re-register the app, ensuring only create-time registration path calls the
registration logic.
In `@src/tests/unit/code-projects.test.ts`:
- Around line 41-45: Add an assertion to the unit test that the
durable-registration side effect is invoked: after calling buildProject(...) in
the test, assert that the mocked registerWebappInPreferences function was called
(e.g., vi.fn().toHaveBeenCalled() / toHaveBeenCalledWith(...)) with the expected
registration payload; locate the mock defined for registerWebappInPreferences
and the call site where buildProject is exercised and add a single test
expectation referencing registerWebappInPreferences and buildProject to ensure
desktop registration occurs.
---
Outside diff comments:
In `@mcp/clawbox-cli.ts`:
- Around line 55-63: The code is misreporting auth redirects as HTML/JSON parse
failures because fetch follows redirects to /login; to fix, ensure API calls
include explicit JSON semantics and avoid auto-following redirects: add
headers.set("accept", "application/json") when constructing Headers (alongside
the existing authorization header) and pass a redirect: "manual" into the fetch
options unless options.redirect is already set, so the fetch call (using
API_BASE, path, options, headers) will surface 3xx auth responses instead of
returning HTML from /login.
In `@mcp/clawbox-mcp.ts`:
- Around line 139-152: The api function can accidentally follow redirects and
return the login HTML as a 200, causing res.json() to throw and be
misclassified; update api (the async function api(path: string, options?:
RequestInit)) to detect auth redirects for /setup-api/* by either forcing fetch
to not auto-follow redirects (set options.redirect = 'manual') and treating any
3xx/redirect or redirected login URL as an auth failure, or by checking the
response content-type (e.g., text/html) and the path prefix when res.ok is true
and throwing an ApiError with an AUTH_FAILED classification (using API_TOKEN and
API_BASE context) instead of calling res.json(); ensure the header injection
logic (authorization via API_TOKEN) remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bbfcfaa-f0e8-4308-8bd2-f135644a3119
📒 Files selected for processing (8)
mcp/README.mdmcp/clawbox-cli.tsmcp/clawbox-mcp.tssrc/app/setup-api/webapps/route.tssrc/lib/code-projects.tssrc/lib/webapp-registry.tssrc/tests/unit/code-projects.test.tssrc/tests/unit/webapp-registry.test.ts
The deployWebapp() extraction made the create POST call it, but webapps.test.ts only mocked WEBAPPS_DIR + APP_ID_RE — so deployWebapp was undefined and the create test 500'd. Stub it (its desktop registration is covered by code-projects/webapp-registry tests).
The structured-error wrapper renamed server.tool( -> tool(, so the source-string assertion for the browser_open registration needs the same.
- cli: resolve token-file root via dev-mode cwd like mcp-token.ts - health: don't follow 302->/login (manual redirect + accept json) so auth failures aren't misreported as JSON parse errors - readme: tag the fenced diagram block as text (markdownlint MD040) - webapps route: treat missing 'name' as update (404 if app absent) and reject empty-name creates, instead of a truthiness branch - code-projects: split writeWebappIndex from deployWebapp so rebuilds refresh index.html only and don't clobber icon / re-surface hidden apps - tests: assert buildProject registers on the desktop; cover the route's empty-name (400), update (200) and missing-app (404) branches
Merged
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardens the ClawBox MCP server + CLI across six areas. Each was verified on a Jetson device (MCP server loads + functional JSON-RPC tool calls; unit tests).
Fixes
CLI auth (the
Failed to parse JSONbug).clawbox-cli.tscalled/setup-api/*with noAuthorizationheader, so once setup completesmiddleware.ts307'd it to/loginandJSON.parsechoked on the login HTML. It now readsCLAWBOX_MCP_TOKEN(falling back to thedata/.mcp-tokenfile the CLI may not inherit via env) and injects the Bearer, mirroringclawbox-mcp.ts. Verified:clawbox-cli system inforeturns JSON.Durable webapps.
webapp_create/code_project_buildonly emitted aui:pending-action— dropped silently if the desktop wasn't open, so the app's HTML saved but it never reached the grid. The webapps POST route +buildProjectnow also register the app in desktop preferences server-side (registerWebappInPreferences), mirroring the liveregister_webapphandler, so the desktop picks it up from/setup-api/preferenceson next load. (Implemented server-side rather than writing prefs from the MCP — the durability backstop belongs at the deploy chokepoint, and uninstall stays sticky with no resurrection.)Structured errors. A
tool()wrapper around all 44 handlers catches throws and returns a parseable{ error, code, message, details }envelope;api()throws a typedApiErrorclassified by status (AUTH_FAILED401/403,NOT_FOUND,ENDPOINT_DOWN5xx,TIMEOUT,INVALID_RESPONSE,DANGEROUS_COMMAND,INTERNAL). Agents branch oncodeinstead of scraping English.clawbox_healthtool. Verifies the MCP bearer +/setup-api/*reachability, returning{ healthy, checks }— so a broken token is diagnosable up front. Verified: healthy with a token, and correctly reportsmcp_token: not set/Failed to parse JSONwithout one.Docs.
mcp/README.md— auth flow, full tool catalog, test commands, failure-mode table.bash safety. Destructive commands (
rm -rf /,dd,mkfs, fork bombs, …) are now hard-blocked unlessallowDangerous: true, enforced at the sharedspawnBackgroundchokepoint so theagenttool can't bypass it (it previously could). Git-safety patterns still only warn. Verified:bashandagentboth blockrm -rf; override + git-safety warn paths work.Notes
mcp/is excluded from the Nexttsconfigand runs under bun (types stripped), so the bar is runtime — hence the on-device smoke tests.webapp-registry.test.ts(3 cases)./simplifyapplied (closed the agent bypass, parallelized the health checks, collapsedregisterWebappInPreferencesto a single config read).Summary by CodeRabbit
New Features
allowDangerousoverrideDocumentation
Refactor