Security/harden shell cicd#211
Conversation
📝 WalkthroughWalkthroughThis PR applies security hardening (shell fallback removal, unsafe command filtering, backend notification flagging), optimizes CI workflows, tightens release secret handling, and pins npm global updates to exact fetched versions instead of resolving ChangesInfrastructure, Security Hardening, and Update Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/hooks/memory-path-utils.ts`:
- Around line 8-25: The isSafe() check relying on SAFE_BUILTINS is bypassable
because it only checks the first token and allows shell wrappers/control syntax
to dispatch child commands; update the validation in isSafe() (and any related
code that iterates SAFE_BUILTINS) to perform strict token-level checks: parse
the entire command line into tokens and ensure the first token exactly matches
an allowed binary name and that no control/operator tokens (e.g., ; && || | `$(
)` backticks, $(), `exec`, `if`, `then`, `else`, `for`, `while`, `find -exec`,
`timeout`, meta-words like `env`) or redirections are present anywhere in the
token list, or alternatively remove wrapper/keyword names from SAFE_BUILTINS so
only literal safe binaries remain; in short, replace the current first-token
substring check with exact-token matching plus a reject-list of shell control
keywords to prevent wrappers from invoking child commands (update SAFE_BUILTINS
usage and the isSafe() implementation accordingly).
In `@src/hooks/pre-tool-use.ts`:
- Around line 284-286: The early return of null when shellCmd and config are
present lets dangerous real-shell commands run; instead, inside the pre-tool-use
flow (the branch after getShellCommand() returns a shellCmd and config exists)
detect memory-touching/virtual-memory paths and do not return null — either
reject the tool invocation or return the existing retry guidance response used
elsewhere; update the logic around getShellCommand(), shellCmd and config to
call the same denial/retry handler used for other memory-touching cases (reuse
the retry guidance path) so commands like sort /etc/passwd ~/.deeplake/... are
blocked rather than forwarded to the real Bash tool.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb3c19f8-f016-465b-a950-65a8f41ec869
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/ci.yaml.github/workflows/release.yamlsrc/cli/update.tssrc/hooks/memory-path-utils.tssrc/hooks/pre-tool-use.tssrc/notifications/sources/backend.tstests/claude-code/pre-tool-use-branches.test.tstests/cli/cli-update.test.ts
| export const SAFE_BUILTINS = new Set([ | ||
| "cat", "ls", "cp", "mv", "rm", "rmdir", "mkdir", "touch", "ln", "chmod", | ||
| "stat", "readlink", "du", "tree", "file", | ||
| "grep", "egrep", "fgrep", "rg", "sed", "awk", "cut", "tr", "sort", "uniq", | ||
| // sed and awk removed: sed supports `-e '1e <cmd>'` (execute shell command) | ||
| // and awk supports `system()` / `|` pipelines — both enable arbitrary code | ||
| // execution through the just-bash fallback. | ||
| "grep", "egrep", "fgrep", "rg", "cut", "tr", "sort", "uniq", | ||
| "wc", "head", "tail", "tac", "rev", "nl", "fold", "expand", "unexpand", | ||
| "paste", "join", "comm", "column", "diff", "strings", "split", | ||
| "find", "xargs", "which", | ||
| "jq", "yq", "xan", "base64", "od", | ||
| "tar", "gzip", "gunzip", "zcat", | ||
| // tar removed: --to-command=<cmd> executes an arbitrary program per entry. | ||
| // env removed: `env <cmd>` runs an arbitrary program. | ||
| "gzip", "gunzip", "zcat", | ||
| "md5sum", "sha1sum", "sha256sum", | ||
| "echo", "printf", "tee", | ||
| "pwd", "cd", "basename", "dirname", "env", "printenv", "hostname", "whoami", | ||
| "pwd", "cd", "basename", "dirname", "printenv", "hostname", "whoami", | ||
| "date", "seq", "expr", "sleep", "timeout", "time", "true", "false", "test", |
There was a problem hiding this comment.
isSafe() is still bypassable via shell wrappers/control syntax.
This still only validates the first token of each stage, but the allowlist includes tokens that can hide a second command. if true; then curl ~/.deeplake/memory/x; fi, timeout 1 curl ..., and find / -exec curl ... \; all pass this check today because the dangerous command is never the first token. Please switch this to explicit validation of the exact Bash shapes you later intercept, or remove every keyword/wrapper that can dispatch a child command.
Also applies to: 30-38
🤖 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 `@src/hooks/memory-path-utils.ts` around lines 8 - 25, The isSafe() check
relying on SAFE_BUILTINS is bypassable because it only checks the first token
and allows shell wrappers/control syntax to dispatch child commands; update the
validation in isSafe() (and any related code that iterates SAFE_BUILTINS) to
perform strict token-level checks: parse the entire command line into tokens and
ensure the first token exactly matches an allowed binary name and that no
control/operator tokens (e.g., ; && || | `$( )` backticks, $(), `exec`, `if`,
`then`, `else`, `for`, `while`, `find -exec`, `timeout`, meta-words like `env`)
or redirections are present anywhere in the token list, or alternatively remove
wrapper/keyword names from SAFE_BUILTINS so only literal safe binaries remain;
in short, replace the current first-token substring check with exact-token
matching plus a reject-list of shell control keywords to prevent wrappers from
invoking child commands (update SAFE_BUILTINS usage and the isSafe()
implementation accordingly).
| if (!shellCmd) return null; | ||
| if (!config) return buildFallbackDecision(shellCmd, shellBundle); | ||
| if (!config) return null; | ||
|
|
There was a problem hiding this comment.
Do not fall through to the real Bash tool for accepted memory commands.
Once a memory-touching Bash command has made it past getShellCommand(), returning null here hands the original command back to Claude Code's host shell. That is not harmless: sort /etc/passwd ~/.deeplake/memory/index.md > /tmp/out will still read/write real files if no virtual handler matches. These paths should deny the tool or emit the existing retry guidance instead of no-oping.
Also applies to: 512-518
🤖 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 `@src/hooks/pre-tool-use.ts` around lines 284 - 286, The early return of null
when shellCmd and config are present lets dangerous real-shell commands run;
instead, inside the pre-tool-use flow (the branch after getShellCommand()
returns a shellCmd and config exists) detect memory-touching/virtual-memory
paths and do not return null — either reject the tool invocation or return the
existing retry guidance response used elsewhere; update the logic around
getShellCommand(), shellCmd and config to call the same denial/retry handler
used for other memory-touching cases (reuse the retry guidance path) so commands
like sort /etc/passwd ~/.deeplake/... are blocked rather than forwarded to the
real Bash tool.
| expect(code).toBe(0); | ||
| expect(spawn).toHaveBeenCalledTimes(2); | ||
| expect(spawn.mock.calls[0]).toEqual(["npm", ["install", "-g", "@deeplake/hivemind@latest"]]); | ||
| expect(spawn.mock.calls[0]).toEqual(["npm", ["install", "-g", "@deeplake/hivemind@1.3.0"]]); |
There was a problem hiding this comment.
Is setting 1.3.0 correct?
2 participants
Summary
Version Bump
Test plan
npm test)package.json, or no release needed for this changeSummary by CodeRabbit
Security Updates
Improvements