🛡️ Sentinel: [HIGH] Prevent Web UI execution of sensitive CLI commands#99
🛡️ Sentinel: [HIGH] Prevent Web UI execution of sensitive CLI commands#99SlasshyOverhere wants to merge 1 commit into
Conversation
* Added interactive and destructive CLI commands (`vault2fa`, `totp`, `sync`, `duress`, `autolock`, `settings`, `theme`, `update`, `desktop`) to `BLOCKED_WEB_CLI_COMMANDS` in `server.ts`. * Prevents bypass of intended CLI-only safety mechanisms via the Web UI's `/api/cli/run` endpoint.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the Web UI’s /api/cli/run endpoint by expanding the server-side blocklist of CLI commands that must not be runnable from the Web UI context, reducing the risk of destructive or sensitive operations being triggered non-interactively.
Changes:
- Expanded
BLOCKED_WEB_CLI_COMMANDSto include additional sensitive commands (e.g.,update,desktop, settings-related commands). - Reformatted the blocklist into a multi-line
Setinitializer for readability.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const CLI_RUN_MAX_BUFFER_BYTES = 10 * 1024 * 1024; | ||
| const BLOCKED_WEB_CLI_COMMANDS = new Set(['web', 'ui', 'destruct', 'init', 'auth', 'delete']); | ||
| const BLOCKED_WEB_CLI_COMMANDS = new Set([ | ||
| 'web', 'ui', 'destruct', 'init', 'auth', 'delete', |
There was a problem hiding this comment.
delete has an alias rm in the CLI (see src/index.ts), but rm is not included in BLOCKED_WEB_CLI_COMMANDS. As a result, /api/cli/run can still perform deletions by running rm .... Add rm to the blocked set (and generally ensure aliases for blocked commands are also blocked).
| 'web', 'ui', 'destruct', 'init', 'auth', 'delete', | |
| 'web', 'ui', 'destruct', 'init', 'auth', 'delete', 'rm', |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/webui/server.ts`:
- Around line 50-54: The blocklist check currently only tests canonical IDs in
BLOCKED_WEB_CLI_COMMANDS but runCliCommand matches the first token and CLI
definitions expose aliases (e.g., totp|otp, duress|panic, delete|rm,
2fa-setup|vault-2fa|2fa), so update runCliCommand to resolve the invoked command
token to its canonical command before checking the blocklist (or alternatively
check the token against both canonical and alias names). Locate the CLI
registry/definitions in src/cli/shell.ts (where aliases are declared) and
implement a lookup function (e.g., resolveCliCommandAlias(token) used by
runCliCommand) that maps aliases to the canonical ID, then enforce blocking by
testing the resolved canonical ID against BLOCKED_WEB_CLI_COMMANDS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const BLOCKED_WEB_CLI_COMMANDS = new Set([ | ||
| 'web', 'ui', 'destruct', 'init', 'auth', 'delete', | ||
| 'vault2fa', 'totp', 'sync', 'duress', 'autolock', | ||
| 'settings', 'theme', 'update', 'desktop' | ||
| ]); |
There was a problem hiding this comment.
Alias-based command bypass remains possible in Web UI blocklist.
Line 50–54 blocks canonical command IDs, but runCliCommand does exact first-token matching, and the CLI definitions expose aliases (e.g., totp|otp, duress|panic, delete|rm, 2fa-setup|vault-2fa|2fa in src/cli/shell.ts Lines 179–212). This means sensitive commands can still be invoked via aliases through /api/cli/run.
🔒 Proposed hardening patch
const BLOCKED_WEB_CLI_COMMANDS = new Set([
- 'web', 'ui', 'destruct', 'init', 'auth', 'delete',
- 'vault2fa', 'totp', 'sync', 'duress', 'autolock',
+ 'web', 'ui', 'destruct', 'init', 'auth', 'delete', 'rm',
+ 'vault2fa', '2fa-setup', 'vault-2fa', '2fa',
+ 'totp', 'otp', 'sync', 'duress', 'panic', 'autolock',
'settings', 'theme', 'update', 'desktop'
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const BLOCKED_WEB_CLI_COMMANDS = new Set([ | |
| 'web', 'ui', 'destruct', 'init', 'auth', 'delete', | |
| 'vault2fa', 'totp', 'sync', 'duress', 'autolock', | |
| 'settings', 'theme', 'update', 'desktop' | |
| ]); | |
| const BLOCKED_WEB_CLI_COMMANDS = new Set([ | |
| 'web', 'ui', 'destruct', 'init', 'auth', 'delete', 'rm', | |
| 'vault2fa', '2fa-setup', 'vault-2fa', '2fa', | |
| 'totp', 'otp', 'sync', 'duress', 'panic', 'autolock', | |
| 'settings', 'theme', 'update', 'desktop' | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/webui/server.ts` around lines 50 - 54, The blocklist check currently only
tests canonical IDs in BLOCKED_WEB_CLI_COMMANDS but runCliCommand matches the
first token and CLI definitions expose aliases (e.g., totp|otp, duress|panic,
delete|rm, 2fa-setup|vault-2fa|2fa), so update runCliCommand to resolve the
invoked command token to its canonical command before checking the blocklist (or
alternatively check the token against both canonical and alias names). Locate
the CLI registry/definitions in src/cli/shell.ts (where aliases are declared)
and implement a lookup function (e.g., resolveCliCommandAlias(token) used by
runCliCommand) that maps aliases to the canonical ID, then enforce blocking by
testing the resolved canonical ID against BLOCKED_WEB_CLI_COMMANDS.
🚨 Severity: HIGH
💡 Vulnerability: Interactive and destructive CLI commands could be executed non-interactively via the Web UI's
/api/cli/runendpoint, bypassing intended CLI-only safety mechanisms and user prompts.🎯 Impact: Attackers or malicious scripts with access to the local Web UI could potentially trigger destructive actions, modify authentication settings, or initiate synchronization without proper confirmation or interactive context.
🔧 Fix: Added
vault2fa,totp,sync,duress,autolock,settings,theme,update, anddesktopto theBLOCKED_WEB_CLI_COMMANDSSet insrc/webui/server.ts, effectively preventing their execution via the Web UI.✅ Verification: Ensure the listed commands are blocked by
BLOCKED_WEB_CLI_COMMANDSand cannot be invoked from the/api/cli/runendpoint.PR created automatically by Jules for task 5957204070675577767 started by @SlasshyOverhere
Summary by CodeRabbit