feat: allow npx pipe sources and auto-merge permissions#28
Conversation
Add npx_segment_safe() to the no-chaining hook, allowing npx with safe binaries (vitest, jest, mocha, tsc, eslint, prettier, biome, oxlint) as pipe sources. Extend install.sh to merge permissions.allow entries from settings/hooks.json into ~/.claude/settings.json, shipping default permissions for git -C, npm/pnpm/npx --prefix patterns.
- Block npx --call/-c as unsafe (arbitrary code execution bypass) - Move --shell-auto-fallback to boolean flag handling - Add tests for bare npx and --call bypass - Atomic write for settings.json (tmpfile + rename) to prevent corruption - Type validation on permissions shape before merging - Capture stderr from Python merge for actionable error messages - Rename hooks_failed -> settings_failed for accurate reporting - Warn when settings/hooks.json is missing
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request introduces npx support to the TrimKit hook system and refactors the settings installer to manage permissions alongside hooks. The npx validation adds a new 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
install.sh (1)
340-342: ⚡ Quick winSpecify UTF-8 encoding explicitly for JSON file writes.
Another day, another missing encoding parameter. The universe conspires to fill my existence with trivial oversights.
Text mode in Python 3 defaults to locale encoding, which is typically UTF-8 but not guaranteed. If the locale is ASCII or Latin-1 and the JSON contains non-ASCII characters (plausible in statusMessage or command paths), you'll get encoding errors.
JSON is standardised as UTF-8. Specify it explicitly for portability.
🔧 Proposed fix
- with os.fdopen(tmp_fd, "w") as f: + with os.fdopen(tmp_fd, "w", encoding="utf-8") as f: json.dump(dst, f, indent=2) f.write("\n")🤖 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 `@install.sh` around lines 340 - 342, The JSON write uses os.fdopen(tmp_fd, "w") without specifying encoding, which can break with non-ASCII text; update the os.fdopen call used around json.dump(dst, ...) to explicitly set encoding="utf-8" (e.g., change os.fdopen(tmp_fd, "w") to os.fdopen(tmp_fd, "w", encoding="utf-8") or otherwise open the temp file with UTF-8) so json.dump and f.write produce a UTF-8 encoded file.
🤖 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 `@hooks/no-chaining.sh`:
- Around line 144-147: The --package/-p flag is currently treated like --prefix
in the case block (pattern "--prefix|--package|-p)"), which allows arbitrary npm
packages; change that so --package and -p are blocked like the existing
--call/-c handling: locate the flag-parsing case in hooks/no-chaining.sh (the
branch handling --prefix currently) and split it into two cases so that
"--prefix" still skips its value (i=$((i+2))) but "--package" and "-p" instead
trigger an immediate error/exit (print a clear error message about forbidden
--package usage and exit non-zero) to enforce defence-in-depth.
In `@hooks/no-chaining.test.sh`:
- Around line 194-259: The test suite is missing coverage for the npx
--package/-p flag; add new test cases using the existing
assert_blocked/assert_allowed helpers to exercise both blocked and allowed
behaviors—e.g. add assert_blocked "blocks: npx --package arbitrary | tail" "npx
--package malicious-pkg vitest | tail -20" and assert_blocked "blocks: npx -p
arbitrary | tail" "npx -p malicious-pkg vitest | tail -20" if --package should
be blocked, or an assert_allowed variant like assert_allowed "allows: npx
--package with safe binary | tail" "npx --package `@vitest/ui` vitest | tail -20"
if it should be allowed; place these alongside the other npx tests near the
existing assert_allowed/assert_blocked npx cases so they run in the same group.
In `@install.sh`:
- Around line 269-270: The tempfile merge_stderr created with mktemp is
allocated before the early-return that triggers when settings/hooks.json is
missing, causing a leak; move the merge_stderr="$(mktemp)" (and any use of
merge_result/merge_stderr with the python3 here-doc) so it is created only after
the existence check for settings/hooks.json, or alternatively ensure you remove
"$merge_stderr" (rm -f "$merge_stderr") immediately before any early return that
runs when settings/hooks.json is absent.
---
Nitpick comments:
In `@install.sh`:
- Around line 340-342: The JSON write uses os.fdopen(tmp_fd, "w") without
specifying encoding, which can break with non-ASCII text; update the os.fdopen
call used around json.dump(dst, ...) to explicitly set encoding="utf-8" (e.g.,
change os.fdopen(tmp_fd, "w") to os.fdopen(tmp_fd, "w", encoding="utf-8") or
otherwise open the temp file with UTF-8) so json.dump and f.write produce a
UTF-8 encoded file.
🪄 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
Run ID: c7e7deb3-2bb8-4a7a-9d0a-056592a21a12
📒 Files selected for processing (5)
hooks/no-chaining.shhooks/no-chaining.test.shinstall.shsettings/claude-md.mdsettings/hooks.json
- Block --package/-p alongside --call/-c (arbitrary package installation with trojan binary names) - Add tests for --package and -p blocking - Specify UTF-8 encoding on atomic write fd
|
All CodeRabbit comments addressed in 22f8972:
|
1 participant
Summary
npx_segment_safe()to the no-chaining hook, allowingnpxwith safe binaries (vitest,jest,mocha,tsc,eslint,prettier,biome,oxlint) as pipe sources — fixes commands likenpx --prefix /path vitest run tests/foo.test.ts 2>&1 | tail -30being blockedinstall.shto mergepermissions.allowentries fromsettings/hooks.jsoninto~/.claude/settings.json, shipping default permissions forgit -C,npm/pnpm/npx --prefixpatternsnpx --call/-cas unsafe (arbitrary code execution)Test plan
--callbypass and barenpx)Summary by CodeRabbit
New Features
npxcommands with approved binaries in pipe operations (vitest, jest, mocha, tsc, eslint, prettier, biome, oxlint).Documentation
npxintegration examples and supported command patterns.