fix: harden hook telemetry handling for secrets and unsafe inputs#351
fix: harden hook telemetry handling for secrets and unsafe inputs#351tsubasakong wants to merge 1 commit intoaffaan-m:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request implements security hardening across multiple scripts: sanitizes session IDs in file paths, adds command prefix allowlist validation, scrubs sensitive data (API keys, tokens, passwords) from observation logs with retention purging, and strips embedded credentials from Git remote URLs. Includes test coverage for each security improvement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
1 issue found across 6 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/lib/utils.js">
<violation number="1" location="scripts/lib/utils.js:342">
P1: The prefix-only allowlist doesn't prevent shell injection *after* the allowed command name. Because `execSync` runs through a shell, metacharacters like `;`, `&&`, `|`, or `$()` after `git`/`node`/etc. can execute arbitrary commands. Consider also rejecting shell metacharacters in the command string, e.g.:
```js
if (/[;|&`$(){}]/.test(command)) {
return { success: false, output: 'Command contains disallowed characters' };
}
Alternatively, switch to spawnSync with a parsed argument array to avoid shell interpretation entirely.
</details>
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review.</sub>
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
| */ | ||
| function runCommand(cmd, options = {}) { | ||
| const command = typeof cmd === 'string' ? cmd.trim() : ''; | ||
| const allowlistRegex = /^(git|node|npx|which|where)(\s|$)/; |
There was a problem hiding this comment.
P1: The prefix-only allowlist doesn't prevent shell injection after the allowed command name. Because execSync runs through a shell, metacharacters like ;, &&, |, or $() after git/node/etc. can execute arbitrary commands. Consider also rejecting shell metacharacters in the command string, e.g.:
if (/[;|&`$(){}]/.test(command)) {
return { success: false, output: 'Command contains disallowed characters' };
}Alternatively, switch to spawnSync with a parsed argument array to avoid shell interpretation entirely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/utils.js, line 342:
<comment>The prefix-only allowlist doesn't prevent shell injection *after* the allowed command name. Because `execSync` runs through a shell, metacharacters like `;`, `&&`, `|`, or `$()` after `git`/`node`/etc. can execute arbitrary commands. Consider also rejecting shell metacharacters in the command string, e.g.:
```js
if (/[;|&`$(){}]/.test(command)) {
return { success: false, output: 'Command contains disallowed characters' };
}
Alternatively, switch to spawnSync with a parsed argument array to avoid shell interpretation entirely.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/lib/utils.js (1)
341-353:⚠️ Potential issue | 🟠 MajorThe allowlist is bypassable via shell chaining and error messages leak sensitive data.
The regex checks only the prefix;
execSync(command)interprets shell metacharacters by default, sogit status && <anything>orgit status; <anything>still executes the trailing payload. The error response also echoes the full blocked command, potentially exposing embedded credentials or tokens in logs.Use
execFileSync()orspawnSync()withshell: falseto avoid shell interpretation, or strictly reject shell metacharacters and return a generic error message without echoing the 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 341 - 353, The current check uses allowlistRegex on the raw command and then calls execSync(command), which allows shell metacharacters to execute chained payloads and also returns error messages that echo the full command; update the logic around the command variable and allowlistRegex so you either (a) reject any shell metacharacters (e.g., characters like &, ;, |, $, `, >, <) before executing, or preferably (b) switch to using execFileSync or spawnSync with shell: false and pass the executable and args separately (split by whitespace after validating the executable against allowlistRegex) to prevent shell interpretation; also change the error return to a generic message that does not include the full command (e.g., "Command not allowed" or "Execution failed") to avoid leaking sensitive data.
🧹 Nitpick comments (2)
tests/hooks/suggest-compact.test.js (1)
53-59: Avoid reimplementing the sanitizer inside the test.Because
getCounterFilePath()now uses the samesanitizeSessionId()logic as production, this test will still pass if both sides regress the same way. Prefer asserting an explicit expected basename for the unsafe sample (for example,claude-tool-count-badsession) so the test fails if the sanitizer becomes too permissive or strips the wrong characters.Also applies to: 373-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/suggest-compact.test.js` around lines 53 - 59, The test currently duplicates the production sanitizer by calling sanitizeSessionId/getCounterFilePath, which masks regressions; change the assertions to check for an explicit expected basename for the unsafe sample (e.g., assert that getCounterFilePath('bad/session?') endsWith '/claude-tool-count-badsession' or similar) rather than relying on the sanitizer's output, and update the other occurrences noted (lines around the second instance) to assert explicit basenames for their unsafe inputs so the test will fail if the sanitizer becomes too permissive or strips the wrong characters.skills/continuous-learning-v2/hooks/observe.sh (1)
152-156: Don't make retention and archiving failures invisible.Both the purge and the archive move are fully silent right now. If either starts failing, old observation files will accumulate or active logs won't rotate, and there is no signal anywhere that the retention policy stopped working. A lightweight warning to the homunculus log would keep the hook non-blocking while making the feature diagnosable.
Also applies to: 162-162
🤖 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 152 - 156, The retention purge and archive move are currently silent; add lightweight non-blocking warnings when operations fail by checking exit codes and logging a message including the relevant variables. Specifically, after mkdir -p "$OBSERVATIONS_ARCHIVE_DIR" verify its success and log a warning if it failed; after the find ... -delete command check its exit status and emit a warning that includes "$OBSERVATIONS_ARCHIVE_DIR" and "$OBSERVATION_RETENTION_DAYS" if non-zero; and apply the same pattern to the archive mv/rename operation referenced around the other block (check mv/rename exit code and log a warning mentioning the source, destination and "$OBSERVATIONS_ARCHIVE_DIR"). Use the existing homunculus logging mechanism (or echo to stderr / logger) so the hook remains non-blocking but failures are visible for diagnosis.
🤖 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 138-145: The scrubber currently only applies regexes to unquoted
text (patterns list applied in the for loop to raw) so JSON-serialized
tool_input/tool_output like "Authorization": "Bearer ..." or "api_key": "..."
remains unredacted; update the hook to either (1) detect when
tool_input/tool_output are dict/objects and redact known secret keys (e.g.,
"authorization", "api_key", "token", "password", "secret", "client_secret",
"ghp_", "sk-") on the parsed object before calling json.dumps, or (2) extend the
existing regex patterns to also match quoted JSON keys and quoted values (e.g.,
include patterns that match "\"authorization\"\s*:\s*\"[^\"]+\"" and similar)
and apply those augmented patterns to raw; apply the same change to the other
scrubber block referenced around lines 177-189 so all serialized JSON
observations are redacted consistently.
In `@skills/continuous-learning-v2/scripts/detect-project.sh`:
- Around line 79-83: The change makes detect-project.sh compute hash_input from
sanitized_remote_url (via _clv2_sanitize_remote_url) but instinct-cli.py still
hashes the raw remote_url and writes registry entries under that raw-derived
project_id, causing split IDs; fix by making both sides consistent: either
revert detect-project.sh to use the raw remote_url for hash_input, or
(preferable) update instinct-cli.py hashing to sanitize the remote_url with the
same logic used in _clv2_sanitize_remote_url before computing project_id and
when writing registry entries (see the project_id/hash routine in
instinct-cli.py around the project hashing and registry write code), and add a
one-time migration that maps existing raw-hash registry entries to the new
sanitized-hash IDs to avoid stranded state.
---
Outside diff comments:
In `@scripts/lib/utils.js`:
- Around line 341-353: The current check uses allowlistRegex on the raw command
and then calls execSync(command), which allows shell metacharacters to execute
chained payloads and also returns error messages that echo the full command;
update the logic around the command variable and allowlistRegex so you either
(a) reject any shell metacharacters (e.g., characters like &, ;, |, $, `, >, <)
before executing, or preferably (b) switch to using execFileSync or spawnSync
with shell: false and pass the executable and args separately (split by
whitespace after validating the executable against allowlistRegex) to prevent
shell interpretation; also change the error return to a generic message that
does not include the full command (e.g., "Command not allowed" or "Execution
failed") to avoid leaking sensitive data.
---
Nitpick comments:
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 152-156: The retention purge and archive move are currently
silent; add lightweight non-blocking warnings when operations fail by checking
exit codes and logging a message including the relevant variables. Specifically,
after mkdir -p "$OBSERVATIONS_ARCHIVE_DIR" verify its success and log a warning
if it failed; after the find ... -delete command check its exit status and emit
a warning that includes "$OBSERVATIONS_ARCHIVE_DIR" and
"$OBSERVATION_RETENTION_DAYS" if non-zero; and apply the same pattern to the
archive mv/rename operation referenced around the other block (check mv/rename
exit code and log a warning mentioning the source, destination and
"$OBSERVATIONS_ARCHIVE_DIR"). Use the existing homunculus logging mechanism (or
echo to stderr / logger) so the hook remains non-blocking but failures are
visible for diagnosis.
In `@tests/hooks/suggest-compact.test.js`:
- Around line 53-59: The test currently duplicates the production sanitizer by
calling sanitizeSessionId/getCounterFilePath, which masks regressions; change
the assertions to check for an explicit expected basename for the unsafe sample
(e.g., assert that getCounterFilePath('bad/session?') endsWith
'/claude-tool-count-badsession' or similar) rather than relying on the
sanitizer's output, and update the other occurrences noted (lines around the
second instance) to assert explicit basenames for their unsafe inputs so the
test will fail if the sanitizer becomes too permissive or strips the wrong
characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6078bd4-2d44-4d11-b00c-1788468fd2af
📒 Files selected for processing (6)
scripts/hooks/suggest-compact.jsscripts/lib/utils.jsskills/continuous-learning-v2/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.shtests/hooks/suggest-compact.test.jstests/lib/utils.test.js
| patterns = [ | ||
| (re.compile(r'(?i)(authorization\s*[:=]\s*(?:bearer\s+)?)[^\s,}]+'), r'\1[REDACTED]'), | ||
| (re.compile(r'(?i)((?:api[_-]?key|token|password|secret|passwd|client[_-]?secret)\s*[:=]\s*)[^\s,}]+'), r'\1[REDACTED]'), | ||
| (re.compile(r'\bgh[pousr]_[A-Za-z0-9]{20,}\b'), '[REDACTED_GITHUB_TOKEN]'), | ||
| (re.compile(r'\bsk-[A-Za-z0-9]{20,}\b'), '[REDACTED_API_KEY]') | ||
| ] | ||
| for regex, replacement in patterns: | ||
| raw = regex.sub(replacement, raw) |
There was a problem hiding this comment.
The scrubber misses the JSON form this hook actually writes.
When tool_input or tool_output is an object, this script serializes it with json.dumps(...) first, so the common shape here is "Authorization": "Bearer ...", "api_key": "...", etc. These regexes only match unquoted authorization: / api_key= forms, so structured tool payloads will still be persisted to observations.jsonl unredacted. Redact known secret keys on the parsed object before serializing, or extend the patterns to handle quoted JSON keys and quoted values.
Also applies to: 177-189
🤖 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 138 - 145, The
scrubber currently only applies regexes to unquoted text (patterns list applied
in the for loop to raw) so JSON-serialized tool_input/tool_output like
"Authorization": "Bearer ..." or "api_key": "..." remains unredacted; update the
hook to either (1) detect when tool_input/tool_output are dict/objects and
redact known secret keys (e.g., "authorization", "api_key", "token", "password",
"secret", "client_secret", "ghp_", "sk-") on the parsed object before calling
json.dumps, or (2) extend the existing regex patterns to also match quoted JSON
keys and quoted values (e.g., include patterns that match
"\"authorization\"\s*:\s*\"[^\"]+\"" and similar) and apply those augmented
patterns to raw; apply the same change to the other scrubber block referenced
around lines 177-189 so all serialized JSON observations are redacted
consistently.
| local sanitized_remote_url="" | ||
| sanitized_remote_url=$(_clv2_sanitize_remote_url "$remote_url") | ||
|
|
||
| local hash_input="${sanitized_remote_url:-$project_root}" | ||
| # Use SHA256 via python3 (portable across macOS/Linux, no shasum/sha256sum divergence) |
There was a problem hiding this comment.
Keep project_id hashing consistent with the Python path.
These lines now hash the sanitized remote URL, but skills/continuous-learning-v2/scripts/instinct-cli.py:138-160 still hashes the raw remote_url and skills/continuous-learning-v2/scripts/instinct-cli.py:170-190 writes registry entries under that raw-hash-derived ID. That splits one repo into two project IDs depending on whether the Bash hook or the Python CLI touched it, and it also strands existing state created from the old raw hash. Update both paths together or add a migration before changing the hash source here.
Also applies to: 107-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/continuous-learning-v2/scripts/detect-project.sh` around lines 79 -
83, The change makes detect-project.sh compute hash_input from
sanitized_remote_url (via _clv2_sanitize_remote_url) but instinct-cli.py still
hashes the raw remote_url and writes registry entries under that raw-derived
project_id, causing split IDs; fix by making both sides consistent: either
revert detect-project.sh to use the raw remote_url for hash_input, or
(preferable) update instinct-cli.py hashing to sanitize the remote_url with the
same logic used in _clv2_sanitize_remote_url before computing project_id and
when writing registry entries (see the project_id/hash routine in
instinct-cli.py around the project hashing and registry write code), and add a
one-time migration that maps existing raw-hash registry entries to the new
sanitized-hash IDs to avoid stranded state.
|
Hey @tsubasakong — heads up that PR #348 was merged earlier today and addresses the same issue (#347) with the same set of fixes (secret scrubbing in This PR will likely have significant merge conflicts with the code that's now in main. You may want to close this one to avoid duplicate work. Thanks for jumping on it though! |
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
Description
skills/continuous-learning-v2/hooks/observe.shskills/continuous-learning-v2/scripts/detect-project.shrunCommandinscripts/lib/utils.jsto a safe command-prefix allowlist (git,node,npx,which,where)CLAUDE_SESSION_IDbefore using it inscripts/hooks/suggest-compact.jstemp counter filenamesValidation:
bash -n skills/continuous-learning-v2/hooks/observe.shbash -n skills/continuous-learning-v2/scripts/detect-project.shnode tests/hooks/suggest-compact.test.jsnode tests/lib/utils.test.jsCloses #347
Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesChecklist
node tests/run-all.js)