fix(hooks): scrub secrets and harden hook security#348
Merged
affaan-m merged 5 commits intoaffaan-m:mainfrom Mar 7, 2026
Merged
fix(hooks): scrub secrets and harden hook security#348affaan-m merged 5 commits intoaffaan-m:mainfrom
affaan-m merged 5 commits intoaffaan-m:mainfrom
Conversation
- Scrub common secret patterns (api_key, token, password, etc.) from observation logs before persisting to JSONL (observe.sh) - Auto-purge observation files older than 30 days (observe.sh) - Strip embedded credentials from git remote URLs before saving to projects.json (detect-project.sh) - Add command prefix allowlist to runCommand — only git, node, npx, which, where are permitted (utils.js) - Sanitize CLAUDE_SESSION_ID in temp file paths to prevent path traversal (suggest-compact.js) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSanitizes session IDs, restricts runCommand to an allowlist and block unquoted shell metacharacters, adds secret-scrubbing and a 30‑day auto‑purge for observations, and strips credentials from git remote URLs with migration of legacy project hash directories. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/suggest-compact.js">
<violation number="1" location="scripts/hooks/suggest-compact.js:28">
P2: Sanitization can produce an empty `sessionId`, losing session isolation. If `CLAUDE_SESSION_ID` contains only non-alphanumeric characters (e.g. `"//../.."` → `""`), the `|| 'default'` fallback has already been bypassed. Add a second fallback after `.replace()` to guarantee a non-empty value.</violation>
</file>
<file name="scripts/lib/utils.js">
<violation number="1" location="scripts/lib/utils.js:343">
P1: The prefix allowlist is trivially bypassable because the command is still passed to `execSync`, which invokes a shell. Any input like `'git log; evil_command'` passes the `'git '` prefix check yet executes arbitrary shell commands. Consider either validating that the command contains no shell metacharacters, or splitting the command and using `spawnSync` (no shell) instead of `execSync`.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/lib/utils.js (1)
342-356:⚠️ Potential issue | 🟠 Major
execSyncwith prefix-only validation allows shell metacharacter injection.The
startsWith()allowlist check on line 344 does not prevent shell metacharacter injection. A command likenode -e "..." ; printf "malicious"passes the allowlist but the shell still executes the chained suffix. Additionally, line 345 echoes the full blocked command in the error message, leaking any secrets in the arguments.Replace this with
spawnSync(command, args, { shell: false })to avoid shell interpretation, or at minimum reject shell metacharacters (; | & < > $ ( ) { }etc.) and return a generic error message instead of the user-supplied command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/utils.js` around lines 342 - 356, The allowlist using allowedPrefixes plus execSync allows shell metacharacter injection and leaks secrets by returning the full cmd; update the implementation that currently uses execSync and allowedPrefixes to instead parse the input into a command and args and call spawnSync(command, args, { shell: false, encoding: 'utf8', stdio: ['pipe','pipe','pipe'], ...options }) (or otherwise validate and split safely), or at minimum reject any shell metacharacters (e.g. ; | & < > $ ( ) { } `) before executing; also stop returning the raw cmd in error responses—return a generic error message and include only non-sensitive error details (err.message) to avoid leaking secrets (referencing allowedPrefixes, execSync, spawnSync, and the error return that echoes cmd).skills/continuous-learning-v2/hooks/observe.sh (1)
165-183:⚠️ Potential issue | 🔴 CriticalThe embedded Python scrubber is syntactically invalid.
This
python3 -cpayload is wrapped in a double-quoted shell string, so the'\''fragments on lines 180–181 are passed straight through to Python instead of acting as shell escapes. Python then seesr'([\"'\''\\s:=]+)'and raisesSyntaxError: unexpected character after line continuation character, which aborts the hook before any observation is written.Fix
- r'([\"'\''\\s:=]+)' - r'[\"'\''\\s]*' + r"([\"'\s:=]+)" + r"[\"'\s]*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 165 - 183, The embedded Python scrubber in the hook is failing due to incorrect quoting in the regex literal _SECRET_RE (the r'([\"'\''\\s:=]+)' fragment) being broken by the surrounding double-quoted shell string; update the hook so the Python payload is not corrupted by shell escapes (for example, switch the inline Python to a here-doc or use a triple-quoted Python string literal for the payload) and fix the regex literal in _SECRET_RE to valid Python quoting (e.g., use a single Python string that contains escaped single-quote and backslash sequences or a triple-quoted raw string) so the pattern for ([\"'\s:=]+) compiles correctly; ensure the change is made where _SECRET_RE is defined in the embedded Python and that the rest of the scrubber code (the observation construction) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 75-80: The purge currently searches
"${_CLV2_PROJECTS_DIR:-$CONFIG_DIR/projects}" for files named
"observations*.jsonl", which can match the live
${PROJECT_DIR}/observations.jsonl and cross-delete other projects' live queues;
update the find call to target only archived files in the current project by
changing the search root to "${PROJECT_DIR}" and the name filter to
"observations-*.jsonl" (so it won't match the live "observations.jsonl"); keep
the existing PURGE_MARKER logic and error redirection intact.
- Around line 177-189: The _SECRET_RE regex used by scrub fails to redact
Authorization headers with schemes like "Bearer" because the final character
class ([A-Za-z0-9_\-/.+=]{8,}) doesn't allow a scheme and space before the
token; update _SECRET_RE so it optionally accepts an auth scheme (e.g., allow an
optional word and whitespace like (?:[A-Za-z]+[ \t]+)? or accept any
non-newline, non-quote characters between the separator and the token) before
matching the token, then keep scrub unchanged to substitute the captured secret
with [REDACTED]; make sure to still require a minimum token length (e.g., {8,})
and preserve the captured groups \1 and \2 in the replacement.
In `@skills/continuous-learning-v2/scripts/detect-project.sh`:
- Around line 67-74: The script now computes project_id from the sanitized
remote (hash_input) which breaks continuity with the legacy hash and can orphan
existing data; update detect-project.sh to detect an existing project dir under
the legacy hash and either reuse it or migrate its contents into the new
${_CLV2_PROJECTS_DIR}/${project_id}. Specifically, after computing the new
project_id, compute the legacy hash (the previous behavior using the original
remote/project_root input) and check both ${_CLV2_PROJECTS_DIR}/${legacy_hash}
and ${_CLV2_PROJECTS_DIR}/${project_id}; if the legacy directory exists and the
new one does not, set project_id to the legacy hash or move the legacy directory
into the new project_id location (preserving files and permissions), otherwise
leave project_id as computed. Ensure this logic references the variables
hash_input, project_id, and the legacy hash variable you introduce so future
changes are obvious.
---
Outside diff comments:
In `@scripts/lib/utils.js`:
- Around line 342-356: The allowlist using allowedPrefixes plus execSync allows
shell metacharacter injection and leaks secrets by returning the full cmd;
update the implementation that currently uses execSync and allowedPrefixes to
instead parse the input into a command and args and call spawnSync(command,
args, { shell: false, encoding: 'utf8', stdio: ['pipe','pipe','pipe'],
...options }) (or otherwise validate and split safely), or at minimum reject any
shell metacharacters (e.g. ; | & < > $ ( ) { } `) before executing; also stop
returning the raw cmd in error responses—return a generic error message and
include only non-sensitive error details (err.message) to avoid leaking secrets
(referencing allowedPrefixes, execSync, spawnSync, and the error return that
echoes cmd).
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 165-183: The embedded Python scrubber in the hook is failing due
to incorrect quoting in the regex literal _SECRET_RE (the r'([\"'\''\\s:=]+)'
fragment) being broken by the surrounding double-quoted shell string; update the
hook so the Python payload is not corrupted by shell escapes (for example,
switch the inline Python to a here-doc or use a triple-quoted Python string
literal for the payload) and fix the regex literal in _SECRET_RE to valid Python
quoting (e.g., use a single Python string that contains escaped single-quote and
backslash sequences or a triple-quoted raw string) so the pattern for
([\"'\s:=]+) compiles correctly; ensure the change is made where _SECRET_RE is
defined in the embedded Python and that the rest of the scrubber code (the
observation construction) remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e19644e-aaa5-4264-ba8f-f0e5cff74c85
📒 Files selected for processing (4)
scripts/hooks/suggest-compact.jsscripts/lib/utils.jsskills/continuous-learning-v2/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.sh
- Reject shell command-chaining operators (;|&`) in runCommand, strip quoted sections before checking to avoid false positives (utils.js) - Remove command string from blocked error message to avoid leaking secrets (utils.js) - Fix Python regex quoting: switch outer shell string from double to single quotes so regex compiles correctly (observe.sh) - Add optional auth scheme match (Bearer, Basic) to secret scrubber regex (observe.sh) - Scope auto-purge to current project dir and match only archived files (observations-*.jsonl), not live queue (observe.sh) - Add second fallback after session ID sanitization to prevent empty string (suggest-compact.js) - Preserve backward compatibility when credential stripping changes project hash — detect and migrate legacy directories (detect-project.sh) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Thanks for the thorough review! All 6 findings were valid — addressed in 953babd:
|
Contributor
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/continuous-learning-v2/hooks/observe.sh">
<violation number="1" location="skills/continuous-learning-v2/hooks/observe.sh:182">
P2: The non-capturing auth-scheme group `(?:[A-Za-z]+\s+)?` is consumed by the match but not included in the replacement `\1\2[REDACTED]`. This silently drops the scheme word from the output (e.g. `Authorization: Bearer token` → `Authorization: [REDACTED]` instead of `Authorization: Bearer [REDACTED]`). Make the group capturing and include it in the replacement, or move it inside the secret-value capture so the redaction doesn't eat non-secret text.</violation>
</file>
<file name="scripts/lib/utils.js">
<violation number="1" location="scripts/lib/utils.js:350">
P1: The metacharacter regex misses `$(...)` command substitution and newline-based command separation. A payload like `git log $(malicious)` passes both the prefix allowlist and this check, yet `execSync` will still execute the substitution via the shell. Consider adding `$` and `\n` to the blocked characters.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/lib/utils.js (1)
348-352: Quote-stripping has edge cases with escaped quotes, but acceptable for current scope.The regex
/"[^"]*"/gdoesn't handle escaped quotes (e.g.,"foo\"bar"), which could leave partial strings. However, given:
- Commands come from trusted internal code
- The allowlist limits the attack surface
- This is defense-in-depth, not the primary security boundary
This is acceptable. If stricter parsing is ever needed, consider a state-machine tokenizer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/utils.js` around lines 348 - 352, The quote-stripping regexes (/"[^"]*"/g and /'[^']*'/g) used when building the unquoted string in runCommand can miss escaped quotes (e.g., "foo\"bar"), so add a short inline comment above the unquoted = cmd.replace(...) line noting this limitation and that it’s an accepted trade-off for now (trusted internal input + allowlist), and include a TODO suggesting a state-machine/tokenizer if stricter parsing is required in the future; keep the current logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/lib/utils.js`:
- Around line 342-346: Add unit tests for runCommand to verify the allowlist and
shell metacharacter protections: create tests that call runCommand with
disallowed prefixes like "rm ", "curl ", "bash " and assert the return equals {
success: false, output: 'runCommand blocked: unrecognized command prefix' }, and
create tests that call runCommand with commands containing metacharacters such
as "git status; echo pwned" and 'node -e "$(whoami)"' and assert the return
equals { success: false, output: 'runCommand blocked: shell metacharacters not
allowed' }; reference the allowedPrefixes constant and the runCommand function
in your test descriptions so failures point back to the allowlist/shell
metacharacter logic.
---
Nitpick comments:
In `@scripts/lib/utils.js`:
- Around line 348-352: The quote-stripping regexes (/"[^"]*"/g and /'[^']*'/g)
used when building the unquoted string in runCommand can miss escaped quotes
(e.g., "foo\"bar"), so add a short inline comment above the unquoted =
cmd.replace(...) line noting this limitation and that it’s an accepted trade-off
for now (trusted internal input + allowlist), and include a TODO suggesting a
state-machine/tokenizer if stricter parsing is required in the future; keep the
current logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f656702-8642-4300-b760-d6d6c1160240
📒 Files selected for processing (4)
scripts/hooks/suggest-compact.jsscripts/lib/utils.jsskills/continuous-learning-v2/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/hooks/suggest-compact.js
…y tests - Add $ and \n to blocked shell metacharacters in runCommand to prevent command substitution via $(cmd) and newline injection (utils.js) - Make auth scheme group capturing so Bearer/Basic is preserved in redacted output instead of being silently dropped (observe.sh) - Add 10 unit tests covering runCommand allowlist blocking (rm, curl, bash prefixes) and metacharacter rejection (;|&`$ chaining), plus error message leak prevention (utils.test.js) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Round 2 feedback addressed in a187e72:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 157-198: The parse-error branch is writing the raw hook body
unsanitized; run the same Python scrubber used for parsed payloads on the raw
payload before appending to OBSERVATIONS_FILE (or stop storing the raw body).
Specifically, ensure the branch that currently writes raw stdin uses the same
scrub(val) function and _SECRET_RE regex from the python block (the code that
builds observation with "timestamp", "event", "tool", "session",
"project_id"/"project_name" and scrubs "input"/"output") to sanitize the raw
payload (e.g., apply scrub(raw_payload) and store it under "input" or "raw" in
the observation) before printing/json-dumping to OBSERVATIONS_FILE so no
tokens/passwords are persisted unredacted.
In `@tests/lib/utils.test.js`:
- Around line 1007-1035: Add a regression test to tests/lib/utils.test.js that
verifies utils.runCommand also blocks newline injection (\n); specifically,
mirror the existing tests that call utils.runCommand and assert result.success
is false and result.output includes 'metacharacters not allowed', but use an
input string containing a newline (e.g., "git status\necho pwned") to ensure
scripts/lib/utils.js runCommand rejects multiline commands as well.
- Around line 1037-1040: Update the "runCommand allows metacharacters inside
double quotes" test so it actually exercises quote-stripping by including shell
metacharacters inside the double quotes passed to utils.runCommand (instead of
just 'node -e "process.exit(0)"'). For example, call utils.runCommand with a
string containing a metacharacter like ';' inside the quoted segment (e.g.,
'node -e "console.log(1);process.exit(0)"' or use 'sh -c "echo a; echo b"') and
assert result.success is true; change the test block that currently calls
utils.runCommand('node -e "process.exit(0)"') to one of these variants to ensure
the quote-handling branch in utils.runCommand is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17eb2702-cd7e-47e6-8dc0-75083edf8590
📒 Files selected for processing (3)
scripts/lib/utils.jsskills/continuous-learning-v2/hooks/observe.shtests/lib/utils.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/lib/utils.js
Address remaining reviewer feedback from CodeRabbit and Cubic: - Scrub secrets in observe.sh parse-error fallback path (was writing raw unsanitized input to observations file) - Remove redundant re.IGNORECASE flag ((?i) inline flag already set) - Add inline comment documenting quote-stripping limitation trade-off - Fix misleading test name for error-output test - Add 5 new security tests: single-quote passthrough, mixed quoted+unquoted metacharacters, prefix boundary (no trailing space), npx acceptance, and newline injection - Improve existing quoted-metacharacter test to actually exercise quote-stripping logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Round 3 feedback addressed in 597d431:
All 155 tests pass (1 pre-existing Windows symlink EPERM, unrelated). |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/lib/utils.js (1)
348-357:⚠️ Potential issue | 🔴 CriticalQuoted command substitution is still a shell-injection bypass.
Line 351 removes quoted substrings before scanning, but Line 357 still passes the original string to
execSync().node -e "$(whoami)"orgit show "`cat secret`"will pass this check and still execute through the shell. At minimum, reject`and$anywhere incmd; the safer fix is to stop shelling out here and usespawnSync/execFileSyncwith argv.🔒 Minimal hardening patch
- const unquoted = cmd.replace(/"[^"]*"/g, '').replace(/'[^']*'/g, ''); - if (/[;|&`$\n]/.test(unquoted)) { + const unquoted = cmd.replace(/"[^"]*"/g, '').replace(/'[^']*'/g, ''); + if (/[;|&\n]/.test(unquoted) || /[`$]/.test(cmd)) { return { success: false, output: 'runCommand blocked: shell metacharacters not allowed' }; }In Node.js `child_process.execSync`, are shell command substitutions like `$()` and backticks still evaluated when they appear inside double-quoted arguments passed as part of the command string?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/utils.js` around lines 348 - 357, The current check strips quoted substrings into unquoted and then still passes the original cmd to execSync, allowing command substitution via `$()` or backticks; update the runCommand logic to stop using execSync with a shell string and instead invoke child_process.spawnSync or execFileSync with an explicit argv array (pass the binary as the command and arguments separately) so no shell interpolation occurs; if you must keep a quick mitigation, expand the rejection check on cmd/unquoted to include backtick (`) and dollar-sign-substitution ($) (i.e., reject if /[`$]/ appears) and ensure execSync is not called with shell: true — prefer spawnSync/execFileSync with args to fully eliminate shell evaluation of cmd.
🧹 Nitpick comments (1)
skills/continuous-learning-v2/hooks/observe.sh (1)
141-146: Consider extracting the duplicate regex definition.
_SECRET_REis defined identically in both Python blocks (lines 141-146 and 188-193). While acceptable given the separatepython3 -cinvocations, extracting to a shared Python module would reduce maintenance risk if the pattern needs updating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 141 - 146, The same regex _SECRET_RE is duplicated across two separate python3 -c invocations; extract it into a shared Python module (e.g., a new small helper file) and import or source that module in both places so both invocations reference the single authoritative _SECRET_RE definition; update the two call sites that currently define _SECRET_RE inline to instead import/use the shared symbol (keeping the exact pattern and name _SECRET_RE) to avoid future divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/lib/utils.js`:
- Around line 342-345: The current allowlist hardcodes 'npx ' and blocks other
package managers; update the logic that builds allowedPrefixes (used where
allowedPrefixes and cmd.startsWith(...) are checked) to include the repo's
configured package manager by reading CLAUDE_PACKAGE_MANAGER (or project config)
and adding its prefix (e.g., "npm ", "pnpm ", "yarn ", or "bun ") to the allowed
set, or alternatively expand the static list to include all supported managers;
ensure the resulting allowedPrefixes array includes the chosen manager before
the existing startsWith check so scripts/**/*.js hooks can run with the selected
package manager.
---
Duplicate comments:
In `@scripts/lib/utils.js`:
- Around line 348-357: The current check strips quoted substrings into unquoted
and then still passes the original cmd to execSync, allowing command
substitution via `$()` or backticks; update the runCommand logic to stop using
execSync with a shell string and instead invoke child_process.spawnSync or
execFileSync with an explicit argv array (pass the binary as the command and
arguments separately) so no shell interpolation occurs; if you must keep a quick
mitigation, expand the rejection check on cmd/unquoted to include backtick (`)
and dollar-sign-substitution ($) (i.e., reject if /[`$]/ appears) and ensure
execSync is not called with shell: true — prefer spawnSync/execFileSync with
args to fully eliminate shell evaluation of cmd.
---
Nitpick comments:
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 141-146: The same regex _SECRET_RE is duplicated across two
separate python3 -c invocations; extract it into a shared Python module (e.g., a
new small helper file) and import or source that module in both places so both
invocations reference the single authoritative _SECRET_RE definition; update the
two call sites that currently define _SECRET_RE inline to instead import/use the
shared symbol (keeping the exact pattern and name _SECRET_RE) to avoid future
divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 629b889a-96ab-4cc4-8180-e406a27085a8
📒 Files selected for processing (3)
scripts/lib/utils.jsskills/continuous-learning-v2/hooks/observe.shtests/lib/utils.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/lib/utils.test.js
Comment on lines
+342
to
+345
| // Allowlist: only permit known-safe command prefixes | ||
| const allowedPrefixes = ['git ', 'node ', 'npx ', 'which ', 'where ']; | ||
| if (!allowedPrefixes.some(prefix => cmd.startsWith(prefix))) { | ||
| return { success: false, output: 'runCommand blocked: unrecognized command prefix' }; |
Contributor
There was a problem hiding this comment.
Allow the configured package manager instead of hardcoding only npx.
This guard now rejects npm, pnpm, yarn, and bun, so hooks that follow the repo’s package-manager selection will fail even though scripts/**/*.js is supposed to support them. Please derive the allowed package-manager prefix from CLAUDE_PACKAGE_MANAGER / project config, or include the supported set here.
As per coding guidelines, "scripts/**/*.js: Support multiple package managers: npm, pnpm, yarn, bun, with configurable selection via CLAUDE_PACKAGE_MANAGER env var or project config."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/utils.js` around lines 342 - 345, The current allowlist hardcodes
'npx ' and blocks other package managers; update the logic that builds
allowedPrefixes (used where allowedPrefixes and cmd.startsWith(...) are checked)
to include the repo's configured package manager by reading
CLAUDE_PACKAGE_MANAGER (or project config) and adding its prefix (e.g., "npm ",
"pnpm ", "yarn ", or "bun ") to the allowed set, or alternatively expand the
static list to include all supported managers; ensure the resulting
allowedPrefixes array includes the chosen manager before the existing startsWith
check so scripts/**/*.js hooks can run with the selected package manager.
Shell evaluates $() and backticks inside double quotes, so checking only the unquoted portion was insufficient. Now $ and ` are rejected anywhere in the command string, while ; | & remain quote-aware. Addresses CodeRabbit and Cubic review feedback on PR affaan-m#348. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Round 5 — Quoted command substitution bypassAddressed: CodeRabbit (critical, duplicate) + Cubic (P1) — Root causeOur previous fix (commit Fix (commit
|
This was referenced Mar 7, 2026
Closed
nyashkn
added a commit
to nyashkn/everything-claude-code
that referenced
this pull request
Mar 8, 2026
Notable upstream changes: - feat: v1.8.0 harness reliability and parity updates - feat: project-scoped instinct isolation - feat: automatic project type/framework detection (affaan-m#293) - feat: chief-of-staff communication triage agent (affaan-m#280) - feat: Cursor, Codex, OpenCode, Antigravity IDE harnesses - feat: auto-start dev servers in tmux (hooks refactor to run-with-flags) - feat: new skills — autonomous-loops, plankton-code-quality, liquid-glass-design, foundation-models-on-device, swift-concurrency-6-2, skill-stocktake, search-first, visa-doc-translate, swiftui-patterns, cpp-coding-standards, regex-vs-llm - fix: hook security hardening — secret scrubbing (affaan-m#348) - fix: UTF-8 for instinct CLI file I/O - fix: .prettierrc added for consistent formatting Conflict resolutions: - .github/workflows/{release,reusable-release}.yml: kept deleted (private fork) - agents/chief-of-staff.md: took upstream (gogcli → gog rename) - hooks/hooks.json: took upstream (run-with-flags.js refactor) - tests/integration/hooks.test.js: took upstream (updated to match new hooks)
This was referenced Mar 10, 2026
|
Really solid defense-in-depth work here — the runCommand allowlist and the quote-stripping metacharacter check are exactly the kind of hardening that should be standard in any skill that executes shell commands. The secret scrubbing in observation logs is particularly important. Snyk/Max Planck's SKILL-INJECT research found that 36% of skills in the wild contain prompt injection payloads, and a big part of the attack surface is skills that silently exfiltrate data through observation/logging channels. Having the scrubber run on the fallback path too (round 3 fix) closes what would've been a real bypass. One thing this PR highlights: even well-intentioned skills need this level of scrutiny. The broader ecosystem doesn't have a great answer for "how do I know the skill I'm installing has done this work." SkillSafe has been tackling the distribution side of this — namespace verification, scan-before-share, and dual-side verification so both the publisher and consumer can independently confirm integrity. But the in-skill hardening you're doing here is the other half of the equation that no registry can replace. Nice work on the test coverage too — 155 passing with the metacharacter edge cases is reassuring. |
jacky99714
added a commit
to jacky99714/everything-claude-code
that referenced
this pull request
Mar 20, 2026
Merge upstream/main which includes: - fix: observer memory explosion with throttling and tail sampling (affaan-m#536) - fix: observer hooks hardening, secret scrubbing (affaan-m#348) - fix: lazy-start observer logic (affaan-m#508) - fix: read tool_response field in observe.sh (affaan-m#377) - feat: add C++, Java, Rust, PyTorch language support - feat: SQLite state store, skill evolution, session adapters - feat: orchestration harness and selective install - feat: DevFleet multi-agent orchestration skill Conflict resolution: - observe.sh: kept our hook_event_name + tool_response fixes, merged upstream's $PYTHON_CMD, secret scrubbing, lazy-start, throttled signaling, and session guards - package.json: accepted upstream's new scripts and dependencies
floatingman
pushed a commit
to floatingman/everything-claude-code
that referenced
this pull request
Mar 22, 2026
* fix(hooks): scrub secrets and harden hook security - Scrub common secret patterns (api_key, token, password, etc.) from observation logs before persisting to JSONL (observe.sh) - Auto-purge observation files older than 30 days (observe.sh) - Strip embedded credentials from git remote URLs before saving to projects.json (detect-project.sh) - Add command prefix allowlist to runCommand — only git, node, npx, which, where are permitted (utils.js) - Sanitize CLAUDE_SESSION_ID in temp file paths to prevent path traversal (suggest-compact.js) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(hooks): address review feedback from CodeRabbit and Cubic - Reject shell command-chaining operators (;|&`) in runCommand, strip quoted sections before checking to avoid false positives (utils.js) - Remove command string from blocked error message to avoid leaking secrets (utils.js) - Fix Python regex quoting: switch outer shell string from double to single quotes so regex compiles correctly (observe.sh) - Add optional auth scheme match (Bearer, Basic) to secret scrubber regex (observe.sh) - Scope auto-purge to current project dir and match only archived files (observations-*.jsonl), not live queue (observe.sh) - Add second fallback after session ID sanitization to prevent empty string (suggest-compact.js) - Preserve backward compatibility when credential stripping changes project hash — detect and migrate legacy directories (detect-project.sh) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(hooks): block $() substitution, fix Bearer redaction, add security tests - Add $ and \n to blocked shell metacharacters in runCommand to prevent command substitution via $(cmd) and newline injection (utils.js) - Make auth scheme group capturing so Bearer/Basic is preserved in redacted output instead of being silently dropped (observe.sh) - Add 10 unit tests covering runCommand allowlist blocking (rm, curl, bash prefixes) and metacharacter rejection (;|&`$ chaining), plus error message leak prevention (utils.test.js) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(hooks): scrub parse-error fallback, strengthen security tests Address remaining reviewer feedback from CodeRabbit and Cubic: - Scrub secrets in observe.sh parse-error fallback path (was writing raw unsanitized input to observations file) - Remove redundant re.IGNORECASE flag ((?i) inline flag already set) - Add inline comment documenting quote-stripping limitation trade-off - Fix misleading test name for error-output test - Add 5 new security tests: single-quote passthrough, mixed quoted+unquoted metacharacters, prefix boundary (no trailing space), npx acceptance, and newline injection - Improve existing quoted-metacharacter test to actually exercise quote-stripping logic Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): block $() and backtick inside quotes in runCommand Shell evaluates $() and backticks inside double quotes, so checking only the unquoted portion was insufficient. Now $ and ` are rejected anywhere in the command string, while ; | & remain quote-aware. Addresses CodeRabbit and Cubic review feedback on PR affaan-m#348. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
4 tasks
This was referenced Apr 14, 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
3 participants
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.
Summary
Addresses #347 — defense-in-depth security hardening across 4 hook/library files.
[REDACTED]before writing to JSONL, plus auto-purge observation files older than 30 days (observe.sh)projects.json(detect-project.sh)runCommand— onlygit,node,npx,which,whereare permitted (utils.js)CLAUDE_SESSION_IDto prevent path traversal (suggest-compact.js)All patches are conservative — they constrain without changing existing functionality.
Test plan
node tests/lib/utils.test.js— 140/141 pass (1 pre-existing Windows symlink failure, unrelated)node tests/hooks/suggest-compact.test.js— 20/20 passprojects.json🤖 Generated with Claude Code
Summary by cubic
Hardened hook security and improved secret scrubbing to prevent credential leaks; now blocks $() and backticks even inside quotes, and prevents newline injection. Addresses Linear #347 with defense-in-depth updates across hooks and utils.
Written for commit 1cab6a7. Summary will update on new commits.
Summary by CodeRabbit