Skip to content

dev → main#82

Merged
node9ai merged 15 commits intomainfrom
dev
Apr 11, 2026
Merged

dev → main#82
node9ai merged 15 commits intomainfrom
dev

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

Changes on dev

Tests: ✗ failing


PR opened automatically by node9 CI agent

github-actions bot and others added 8 commits April 9, 2026 18:38
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Automatically hashes MCP server tool definitions (name, description,
inputSchema) on first connection and blocks if they change on subsequent
connections. This defends against "rug pull" attacks where a trusted MCP
server silently modifies tool descriptions to inject malicious instructions.

- Replace child.stdout.pipe() with readline interceptor in MCP gateway
  to inspect tools/list responses before forwarding to the agent
- SHA-256 hash of canonicalized tool definitions, sorted by name
- Pin storage at ~/.node9/mcp-pins.json (atomic writes, mode 0o600)
- On mismatch: return JSON-RPC -32000 error with clear remediation steps
- CLI: node9 mcp pin list/update/reset for pin management
- 20 unit tests (hashing, storage, pin lifecycle)
- 5 integration tests (first pin, match, rug pull block, re-pin, transparency)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… review-and-approve update

Addresses adversarial review findings:

1. Pin file reads fail closed: corrupt/unreadable pin files now throw
   instead of silently returning empty (which re-trusted the upstream).
   Only ENOENT is treated as "no pin exists."

2. Session quarantine: tools/call is blocked until a tools/list pin check
   passes. Mismatch or corrupt pin state permanently quarantines the
   session — no tool calls forwarded until the operator resolves it.

3. Pin update is now a review flow: `mcp pin update` spawns the upstream,
   fetches current tools, diffs old vs new definitions, and requires
   explicit operator confirmation before re-pinning.

4. README updated with MCP tool pinning section explaining the rug pull
   defense and CLI commands.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Revert mcp pin update to simple delete-and-repin. The review-and-approve
flow (upstream fetch, diff display, confirmation prompt) adds ~170 lines
and is a UX enhancement — not a security fix. Moving to a follow-up PR
to keep this one focused on the two security hardening changes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- pin list: uses readMcpPinsSafe() to show friendly error on corrupt file
- pin update: catches corrupt file with recovery instructions
- pin reset: works on corrupt files (clears without reading first)
- README: fix stale comment about pin update reviewing diffs

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
node9_rule_add only accepts block/review verdicts — allow is explicitly
rejected at both schema and runtime levels to prevent AI from bypassing
node9 security policies.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 node9 Code Review

Review

The AI applied no changes (empty diff).

The original diff contains two modifications:

  1. A license badge change from Apache-2.0 to MIT
  2. An extra blank line added to the README

These are documentation-only changes with no security, logic, or correctness implications. There is nothing to fix here, and the AI correctly made no changes.

Note: The test failure (npm run build 2>&1 && npm test 2>&1 rejected for unsafe characters) is unrelated to this diff — it's a test harness configuration issue with shell metacharacters, not a problem introduced by these changes.

Everything looks fine.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

✅ No security issues found.

The diff only contains README.md changes (license badge update, whitespace addition) and notes that src/mcp-server/index.ts was omitted due to size. No code changes are visible to review.


Automated security review by node9

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 node9 Code Review

Review

The AI diff is empty — no actual code changes were applied by the AI fix. The commit shown is the original diff itself (the feat(init) commit), not an AI-generated fix on top of it.

Reviewing the original changes for issues:

README.md — License badge change (Apache-2.0 → MIT):
This is a potential correctness/legal issue. If the actual license file or package.json still declares Apache-2.0, this badge change creates a mismatch and could be misleading to users. This should be verified against the actual LICENSE file and package.json#license field. Not a security issue, but a factual correctness concern worth flagging.

src/cli/commands/init.ts — Summary output change:

  • found.join(', ')found must be a non-empty array here for the message to make sense. If found is empty, the output becomes "Node9 is protecting !" which is misleading. Whether this is reachable depends on logic earlier in the function (not shown), but it's a minor logic gap.
  • No security issues introduced.
  • The added hardcoded https://node9.ai/signup URL is fine — it's display-only output with no dynamic input involved.

Overall: No security issues. The license badge change is the only real correctness concern worth verifying. The AI applied no changes, so nothing was introduced or broken by the fix itself.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

✅ No security issues found.

The diff contains only cosmetic/documentation changes (README badge update, console.log output formatting in the init command). The agentList variable is constructed from found.join(', ') where found appears to be an internal array of detected agents — it is only passed to chalk.green.bold() for terminal display, not to any dangerous sink (no shell execution, no file writes, no network calls, no deserialization). No user-controlled input reaches any exploitable sink in this diff.


Automated security review by node9

Chained commands like `git add . && git commit && git push` were
bypassing git push/destructive/force-push rules because ^ only matched
when git was at the start of the command. Replaced with \b word boundary.

Same fix applied to review-sudo.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 node9 Code Review

Review

The AI applied no changes (empty diff), which is the correct response here.

Assessment: Correct behavior.

The original diff contains only:

  1. A license badge change in README.md (Apache-2.0 → MIT) — a documentation/metadata change
  2. A UI string improvement in src/cli/commands/init.ts — cosmetic output formatting

Neither change introduces security issues, logic errors, or correctness problems that require fixing. The init.ts change is straightforward string formatting with no control flow modifications. The agentList variable is only used for display output, not in any security-sensitive context.

The test failure ("unsafe characters" in test command) is unrelated to the code changes in the diff — it's a test runner configuration issue with the && operator, not something the AI should patch in source code.

No issues to flag.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

Security Findings

  • [MEDIUM] src/config/index.ts:DEFAULT_CONFIG — Removal of ^\\s* anchors from all regex patterns used for security policy matching creates bypass opportunities. Previously, patterns like ^\\s*sudo\\s would only match sudo at the start of a command. The new \\bsudo\\s pattern can match sudo anywhere in a string, which sounds stricter but introduces a different bypass: an attacker could construct a command where sudo appears as an argument to another program (e.g., echo sudo rm -rf /) and potentially trigger unintended review/block verdicts, or conversely, craft inputs that exploit inconsistencies in how the engine evaluates partial matches. More critically, the git push --force block regex no longer anchors to command start, meaning a command like some-wrapper git push --force would now match — previously it would not. If the security model relies on these patterns to block dangerous commands, the lack of anchoring could cause false negatives or false positives depending on how the matching engine works with the full command string. Without seeing src/mcp-server/index.ts (omitted as too large), it is unclear whether the field value passed to matches is the raw shell command string or a pre-parsed argv[0], making this a medium-severity concern.

  • [LOW] src/cli/commands/init.ts:registerInitCommandfound.join(', ') inserts agent names directly into a console.log chalk-formatted string. The found array is populated from filesystem detection (detected agent names). If agent names are sourced from user-controlled filenames or config values, terminal escape sequence injection is theoretically possible. However, since chalk strips or encodes control characters in most versions, exploitation is limited. Low severity, defense-in-depth concern.


Automated security review by node9

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 node9 Code Review

Review

The AI applied no changes (empty diff). There is nothing to review.

The original diff itself contains a few noteworthy items worth flagging for the broader review:

Regex anchor removal (src/config/index.ts): The change from ^\s*git\b to \bgit\b removes the start-of-string anchor. This is a security regression — it allows matching git push --force embedded anywhere in a longer command string (e.g., echo "not git push --force" && malicious-command). A command like legitimate-looking-prefix; git push --force would now match, but more critically, an attacker could craft input where the git command appears mid-string in unexpected contexts. The original anchor ensured the command started with git (modulo whitespace). Similarly, ^\s*sudo\s\bsudo\s means sudo embedded in a string like export PSEUDO=1 would not match (word boundary handles that fine), but the loss of positional anchoring is still a weakening of the intent.

License change (README.md): Apache-2.0 → MIT is a material legal change that should be intentional and coordinated with the actual LICENSE file — if the file itself wasn't updated, this creates an inconsistency.

The AI fix is empty — it neither addressed these issues nor introduced new ones. No correctness or security problems were introduced by the AI's (non-)changes.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

Security Findings

  • MEDIUM src/config/index.ts:DEFAULT_CONFIG — The regex patterns for security policy rules were changed from anchored (^\s*) to unanchored (\b). This weakens bypass resistance for the review-sudo rule specifically. An attacker-controlled command like echo "sudo rm -rf /" or cat file_with_sudo_in_it | bash could now match the \bsudo\s pattern even when sudo is not the operative command being run. The original ^\s*sudo\s ensured sudo appeared at the start of the command string. With \bsudo\s, a benign-looking command containing the word sudo anywhere triggers a review, but more critically, a malicious command could be crafted to avoid matching by embedding the dangerous operation before the word "sudo" appears (e.g., malicious_command; sudo echo). The intent is reversed — the \b-anchored version can match in contexts the author didn't intend, while simultaneously being bypassable via prefix injection (alias sudo=./malicious_bin; sudo echo hi).

  • MEDIUM src/config/index.ts:DEFAULT_CONFIG — The git push --force detection regex was changed from ^\s*git\b to \bgit\b. This means a command like echo git push --force or cat script | grep git push --force would now match the rule and trigger review incorrectly, potentially causing alert fatigue. More critically for the allow rule (the git push without --force), the unanchored pattern could match commands where git appears in a non-operative position (e.g., a path /home/git/push), causing legitimate non-git commands to be subject to git-push policy logic. The security control is now bypassable: true; git push --force — if the engine evaluates the full shell string, the leading true; previously broke the ^\s*git anchor, but now the \bgit will still match and enforce policy. This is actually more restrictive in that case, not less — but the inverse bypass env git=x git push --force style injections in the non-anchored context depend heavily on how the matching engine consumes the command field.

  • LOW src/cli/commands/init.ts:registerInitCommandagentList is constructed from found.join(', ') and interpolated directly into a console.log chalk template string. If found contains values derived from filenames, config files, or other external input (not visible in this diff), and if chalk's template processing ever evaluates embedded expressions, this could be a low-severity log injection. In current chalk versions this is not exploitable for code execution, but terminal escape sequences in agent names could manipulate terminal output (terminal injection).


Automated security review by node9

node9ai and others added 2 commits April 11, 2026 17:43
Two bugs fixed:

1. mcp-pin.unit.test.ts Windows home dir mock:
   Set USERPROFILE alongside HOME — os.homedir() on Windows reads
   USERPROFILE, not HOME, so the temp dir mock was ignored and all
   pin file operations read from the real home directory.
   Skip 0o600 permission test on Windows (Unix file modes not supported).

2. mcp-gateway/index.ts ERR_USE_AFTER_CLOSE crash:
   When drainPendingToolCalls() replays queued tool calls after stdin
   has already closed, agentIn is in a closed state. Calling pause()
   on a closed readline interface throws ERR_USE_AFTER_CLOSE.
   Guard with !deferredStdinEnd — if stdin already closed, the stream
   is done and there is nothing to pause.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: andreykh89 <andreykh89@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 node9 Code Review

Review

The AI applied no changes (empty diff).

There is nothing to review — the AI produced an empty fix against the original diff. No code was modified, added, or removed.

What this means practically:

  • The test command was rejected before running due to unsafe characters in the command string (&& chaining flagged by the sandbox), so there's no test result to evaluate.
  • The original diff itself introduces a license badge change (Apache-2.0 → MIT) and MCP tool pinning functionality, but none of that was touched by the AI fix.
  • 8 files containing the core implementation (src/mcp-pin.ts, src/mcp-gateway/index.ts, tests, etc.) were omitted from review due to diff size limits, meaning the security-critical implementation details of the pinning mechanism cannot be assessed here.

Concerns with the original diff that remain unaddressed:

  • The license change (Apache-2.0 → MIT) is a significant legal change that should be intentional and verified — not a candidate for an AI fix, but worth flagging.
  • The pinning security model described in the README cannot be verified against the actual implementation given the omitted files.

Verdict: No AI changes to evaluate. The empty fix is a no-op.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

Security Findings

  • [HIGH] src/config/index.ts — The regex patterns for detecting dangerous commands (git push --force, sudo, git reset --hard, etc.) have been weakened by removing the ^\\s* start anchors. The new patterns use \\b word boundaries instead, which means they can be matched anywhere in a string. An attacker-controlled command could bypass the policy by prefixing the dangerous command: e.g., echo 'safe' && sudo rm -rf / — the \\bsudo\\s pattern still matches here, but more critically, a string like nosudo bash could previously have been caught differently. More dangerously, the \\bgit\\b change means a path like /tmp/notgit/git push --force could trigger the allowlist rule for "safe git push" while the force-push block rule also matches — the interaction between the two rules now depends on ordering rather than the anchor guaranteeing the command starts with git. The real bypass risk: echo "git push --force" | bash — the \\bgit\\b pattern matches the echo argument, potentially triggering the wrong policy branch. The anchor ^\\s* ensured the dangerous command was actually being executed, not merely mentioned in arguments or heredocs. Removing it allows commands that reference these strings without executing them to match, and inversely allows injected prefixes to confuse rule matching.

  • [MEDIUM] src/config/index.ts — The review-sudo rule change from ^\\s*sudo\\s to \\bsudo\\s means that a command like cat file_with_sudo_in_name won't match (word boundary protects that), but a command like VAR=sudo bash -c 'sudo rm ...' — the sudo in the value assignment now matches \\bsudo\\s, potentially triggering review on a false positive, OR an attacker could craft a command where the real sudo invocation is in a subshell/eval that the regex sees as a secondary match. The main concern is that the policy enforcement relied on the command starting with sudo to identify privilege escalation; \\bsudo\\s matches sudo appearing anywhere, including in variable assignments, comments, or arguments passed to other commands, creating both false positives (unnecessary reviews) and potential logic confusion in multi-rule evaluation.

  • [LOW] src/cli/commands/init.tsfound.join(', ') where found is an array of agent/path names is interpolated directly into a chalk template string for console output. If agent names contain ANSI escape sequences or terminal control characters (sourced from filesystem scanning of agent configs), this could cause terminal injection. Low severity as it's local CLI output, but worth sanitizing if found entries come from untrusted file/directory names.

  • [LOW] src/cli/commands/mcp-pin.ts (omitted file) — The mcp pin update <serverKey> command accepts a serverKey argument from CLI. If this key is used as a filesystem path component or dictionary key to access pin state without validation, it could allow path traversal or state corruption. Cannot fully assess without the diff, but the pattern warrants scrutiny.


Automated security review by node9

@node9ai node9ai marked this pull request as ready for review April 11, 2026 15:15
@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 node9 Code Review

Review

The AI applied no changes (the diff is empty).

The original diff shows meaningful functionality being added (MCP tool pinning for rug-pull defense, a new CLI command, license badge update), but the AI produced zero fixes. This could mean:

  1. The AI determined no fixes were needed — but with 8 files omitted from review (including src/mcp-pin.ts, src/mcp-gateway/index.ts, tests, and config), there's no way to validate the correctness of the core pinning logic, SHA-256 hashing implementation, quarantine behavior, or pin storage security.

  2. Critical unreviewed areas given the omitted files:

    • Whether the SHA-256 pinning in src/mcp-pin.ts is correctly implemented (hash collisions, encoding issues)
    • Whether "fails closed" behavior in quarantine is actually enforced or just documented
    • Whether pin state storage has any TOCTOU race conditions
    • Whether mcp pin reset requires any authentication/confirmation guard itself (clearing all pins is a high-impact operation)

On the visible changes: The README and CLI registration changes look structurally correct. The license badge change (Apache-2.0 → MIT) is a substantive claim that should be verified against the actual LICENSE file and package.json — this is a legal concern, not just cosmetic.

Verdict: The AI produced no actionable output. The review is incomplete due to omitted files containing all security-relevant implementation details. Cannot approve.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

Security Findings

  • [HIGH] src/config/index.ts — The regex patterns for detecting sudo, git push --force, and destructive git commands have been changed from anchored (^\s*) to unanchored (\b) patterns. While the intent is broader matching, the sudo rule change is the most dangerous: the old pattern ^\s*sudo\s matched commands starting with sudo (actual privilege escalation), while \bsudo\s can now match sudo appearing anywhere in a string — including in filenames, comments, or arguments. More critically, an attacker can exploit the inverse: a command like echo 'not sudo here' && sudo rm -rf / would now match via \bsudo\s, but so would a benign string like cat /path/to/pseudo script. The deeper security concern is the reverse — commands designed to evade these rules. For git push, removing the ^\s* anchor means a command like python exploit.py; git push --force origin main now matches, but the rule was previously bypassable by prepending whitespace anyway. More critically, the removal of anchors on the allow/review rules means injection via subshells or command chaining may produce false matches triggering incorrect verdicts.

  • [HIGH] src/config/index.ts:DEFAULT_CONFIG — The review-sudo rule change from ^\s*sudo\s to \bsudo\s is a bypass regression: a command like VAR=sudo bash or env sudo=1 evil_binary would now potentially trigger the review gate on benign commands, but more critically, bash -c 'sudo rm -rf /' where sudo is not the first token was previously undetected — so this "fix" partially addresses evasion but the absence of the omitted mcp-gateway/index.ts and mcp-pin.ts files means the core enforcement logic (pin state storage, hash comparison, quarantine bypass) cannot be audited. Pin state stored on disk could be subject to race conditions or TOCTOU attacks if not implemented with atomic writes.

  • [MEDIUM] src/cli/commands/init.ts:registerInitCommandfound.join(', ') interpolated directly into a chalk template string for terminal output. If found array entries contain terminal escape sequences or ANSI codes sourced from agent/config names on disk, this could enable terminal injection. Severity is medium since this is CLI output, not a web context, but crafted config filenames could manipulate terminal display or trigger escape-sequence-based attacks on some terminals.

  • [MEDIUM] Omitted files (src/mcp-pin.ts, src/mcp-gateway/index.ts) — The MCP tool pinning feature stores SHA-256 hashes of tool definitions. Without reviewing the implementation, critical questions cannot be answered: (1) Is the pin state file written atomically? (2) Is the serverKey parameter in node9 mcp pin update <serverKey> sanitized before use as a file path or object key? (3) Is the hash comparison constant-time? The CLI surface mcp pin update <serverKey> accepts user-controlled input that likely reaches filesystem operations — a path traversal via serverKey (e.g., ../../etc/passwd) is plausible given the pattern.

  • [LOW] src/config/index.ts — Regex patterns with flags: 'i' and complex lookaheads (e.g., (?!.*(-f\b|--force|--force-with-lease))) applied to attacker-controlled command strings may be vulnerable to ReDoS if the regex engine backtracks on pathologically crafted inputs, though the specific patterns here are not obviously catastrophic.


Automated security review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 node9 Code Review

Review

The AI applied no changes (empty diff).

There is nothing to review. The AI produced an empty fix, meaning it made no modifications to the codebase.

Regarding the original diff under review:

The original diff itself is reasonable at the surface level (license badge update, README documentation for MCP tool pinning, CLI command registration), but 8 critical files were omitted due to size — including the actual implementation (src/mcp-pin.ts, src/mcp-gateway/index.ts, src/mcp-pin.unit.test.ts, etc.). The security-sensitive logic (SHA-256 pinning, quarantine behavior, "fails closed" guarantee) cannot be verified without those files.

The test rejection (unsafe characters in npm run build 2>&1 && npm test 2>&1) indicates the test harness blocked execution entirely, so there is no test result to evaluate.

Bottom line: The AI did nothing. No fix was applied, no logic was changed, and no security issues were introduced — but equally, nothing was corrected. If there was an actual problem to fix, it was left unaddressed.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

Security Findings

  • [HIGH] src/config/index.ts (regex patterns) — Removing ^\s* anchors from security policy regexes allows bypass via prefix injection. The sudo pattern changed from ^\s*sudo\s to \bsudo\b — an attacker-controlled command like echo "not sudo" && sudo rm -rf / would now match, but more critically, commands designed to evade detection could previously be caught by the start-anchor. More dangerously, the git push --force block pattern changed from ^\s*git\b to \bgit\b, meaning a command like echo git push --force or # git push --force\n real_malicious_command containing the word git anywhere could trigger the wrong policy branch. An AI agent constructing a shell command with embedded substrings matching these patterns could cause false-positive blocks or, inversely, a malicious payload that contains git as a substring could hit the wrong rule. The unanchored \bsudo\s pattern is the most critical: a string like describessudo something won't match due to \b, but alias x=sudo; x bypasses entirely — the real issue is that removing the start anchor means the pattern now matches sudo anywhere in a multi-command string, which could cause policy misclassification when commands are chained (e.g., safe-cmd && sudo dangerous — the policy now matches but previously wouldn't if safe-cmd appeared first without whitespace). This weakens the security posture of the rule engine.

  • [MEDIUM] src/config/index.ts (review-sudo condition) — The sudo detection regex changed from ^\s*sudo\s (anchored to start of command) to \bsudo\s (unanchored). This means a command like cat /etc/sudoers or grep sudo /etc/passwd now contains sudo as a word boundary match but won't trigger (correct), however the removal of the start anchor means that policy enforcement now depends entirely on word-boundary semantics rather than command position. A crafted command legitimate_tool --flag "inject sudo rm -rf /" where sudo appears in an argument string would now match the review policy — causing false positives that could be used to DoS the agent workflow, or conversely an attacker could study which pattern branch is taken to craft evasions.

  • [LOW] src/cli/commands/init.ts (registerInitCommand) — found.join(', ') is interpolated into a chalk-formatted console string. If agent names sourced from filesystem scanning (directory names, config file keys) contain terminal escape sequences, this could result in terminal escape injection. Severity is low as impact is limited to the local terminal of the user running node9 init, but filenames with ANSI codes could corrupt terminal state.


Automated security review by node9

@node9ai node9ai merged commit 8f55caf into main Apr 11, 2026
11 checks passed
node9ai pushed a commit that referenced this pull request Apr 11, 2026
## [1.9.1](v1.9.0...v1.9.1) (2026-04-11)

### Bug Fixes

* dev → main ([#82](#82)) ([8f55caf](8f55caf))
@node9ai
Copy link
Copy Markdown
Contributor

node9ai commented Apr 11, 2026

🎉 This PR is included in version 1.9.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants