fix: security hardening, error handling, and accessibility improvements - #22
Merged
Merged
Conversation
Sanitize appId inputs to prevent path traversal in install/uninstall/settings routes. Extract duplicated getSkillsDir and reloadGateway to openclaw-config. Add ARIA attributes to toggles, modals, and progress bars. Replace silent catch blocks with logged errors. Fix postbuild find option order and add SRVJS validation. Add AbortController timeout to uninstall fetch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
✅ Test Report
|
yalexx
added a commit
that referenced
this pull request
Sep 6, 2026
…740) * security: close the CodeRabbit deep-scan findings that still hold on beta The 2026-09-05 scan of main reported 23 findings; each was re-verified against beta before anything changed. Five were already fixed on beta (#1 #2 #5 #8 #18), three are the appliance's documented design (#3 #13 #15), two need a design decision rather than a patch (#12 the self-updating root steps, #16 system_power via the bearer) and are deferred with their designs written up. This closes the rest: - #21/#8: root units (clawbox-ap, ap-watchdog, the NM failover hook, first-boot VNC, recover) run the root-owned /usr/local/libexec/clawbox copies and load /etc/clawbox/network.env, never the clawbox-owned tree; clawbox-heartbeat runs as User=clawbox; a class-wide test pins the rule. - #11: the Files API refuses to rename or delete a protected container (data/, the checkout, ~/.config, the browse root) — protected_container. - #19: the MCP path guard judges the canonical path (nearest existing ancestor) as well as the typed one, and the file tools open the vetted target with O_NOFOLLOW. - #17: the webapp document carries a sandbox CSP wherever it is opened (shipped through next.config.ts, since a route header is dropped in production), and installed_* preference writes are owner-only. - #20/#22: clawkeep restore derives every destination on the box and refuses the manifest's before anything moves; link members must resolve inside the staging root; restore/unpair/snapshot/encryption/reset-state are owner-only and same-origin. - #7: CF-Connecting-IP and its siblings are stripped unless the socket peer is loopback (cloudflared's), so a LAN client cannot pick its lockout bucket. - #4: regex code search is gone (400 regex_unsupported). - #6: uploads are bounded by a free-space reserve with busboy limits and partials unlinked; the attachments route gets the same teardown deferral. - #14: the Kokoro/Whisper sockets are 0600 with SO_PEERCRED, and Kokoro's output path is confined to a .wav regular file under /tmp. - #9 (part): the MCP server scrubs CLAWBOX_MCP_TOKEN from its environment at startup; allow_dangerous is documented as a typo override, not consent. - #10: issue-triage/pr-review validate the model's JSON on both transports, derive labels from fixed tables and sanitise comment text. - #23: e2e-install writes repository secrets only off pull_request events. - #1/#5 residuals: setup/complete checks the session in-handler; the middleware matcher no longer skips /fonts/ and /images/. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SuyrrYnKgrUkBXECWqW1gb * fix: vouch for the path at the two sinks CodeQL flagged The multipart cleanup unlinked paths whose containment check governed the write inside the promise, not the catch block; and the dangling-link resolver lstat/readlink'd a name straight off the caller's path. Both now resolve and prefix-check right before the call, the shape safePath already uses (js/path-injection alerts 519-521). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SuyrrYnKgrUkBXECWqW1gb * fix: address CodeRabbit's review of the security sweep - e2e-install: the one job names its Environment by event (e2e-credentials off pull_request, an empty e2e-pull-request on one), documented for the owner; the schema strip for the SDK transport is schema-aware and covers Anthropic's whole unsupported set, and the local validator refuses any constraint it cannot check so no cap is silently unenforced. - clawkeep: a Hermes sessions asset that omits sqlite still retires the sidecars (the box's own flag wins); OPENCLAW_STATE_DIR placeholders count as unset; the no-state fallback matches both CLI message forms, with one shared recorded-CLI fixture. - install.sh: a libexec copy that did not land is never a success — collected, recorded as root_libexec, and the units that name the copies are not written over it. - root-unit tests parse User= (User=root is root) and refuse /home/clawbox anywhere in a directive value; the code search route refuses a non-string pattern; notebook_edit has its symlink regression case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SuyrrYnKgrUkBXECWqW1gb * test: give the libexec test in root-steps both ceilings It runs install_root_libexec under a real bash, and the timeout-hygiene rule (test-timeout-hygiene.test.ts) asks every spawning suite for a declared testTimeout and hookTimeout — the one CI failure on the previous commit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SuyrrYnKgrUkBXECWqW1gb --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.
Sanitize appId inputs to prevent path traversal in install/uninstall/settings routes. Extract duplicated getSkillsDir and reloadGateway to openclaw-config. Add ARIA attributes to toggles, modals, and progress bars. Replace silent catch blocks with logged errors. Fix postbuild find option order and add SRVJS validation. Add AbortController timeout to uninstall fetch.
Changes to the Terminal: copy and paste via Ctrl+Shift+C and Ctrl+V. Copy is now possible on the chat with the mascot. Fixed issues with the remote desktop not showing the browser and the toolbar. Store implementations now show the user validation for the newly installed skill and refreshed session.