Skip to content

dev → main#84

Merged
node9ai merged 23 commits intomainfrom
dev
Apr 13, 2026
Merged

dev → main#84
node9ai merged 23 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 17 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>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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>
- Add optional description field to SmartRule interface
- Pass ruleDescription through policy → orchestrator → check.ts
- Show description in /dev/tty review/block card for human-readable context
- Add descriptions to all DEFAULT_CONFIG built-in rules and ADVISORY_SMART_RULES
@github-actions
Copy link
Copy Markdown
Contributor Author

🔍 node9 Code Review

Review

The AI diff is empty — no changes were applied. The "fix" here was apparently just resolving the test infrastructure issue (the unsafe characters in the test command), not modifying any source code.

Regarding the original diff under review:

The changes look correct:

  • ruleDescription is propagated cleanly through the pipeline: SmartRule.descriptionevaluatePolicy return value → AuthResult → CLI output. No logic errors in the flow.
  • The spread conditional pattern ...(x && { key: x }) is used consistently and correctly — falsy values (empty string, undefined) are safely excluded.
  • The CLI output ordering (description before "Triggered by") is reasonable and non-breaking.
  • No security concerns — this is purely additive metadata display, not used in any auth decision path.

No issues found. The original diff is a straightforward, low-risk additive change passing through a new optional field.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

  • MEDIUM src/cli/commands/check.ts:registerCheckCommand — The ruleDescription field originates from policy/config files (matchedRule.description) and is rendered directly to the terminal via writeTty(chalk.white(...)) without any sanitization. If the policy configuration file can be influenced by an attacker (e.g., a malicious dependency supplies its own policy, or a shared config file is writable), the description string could contain terminal escape sequences that manipulate the terminal display — for example, overwriting previous output lines, injecting fake status messages, or exploiting terminal emulator vulnerabilities (e.g., title-setting sequences, OSC sequences). The same risk applies to recoveryCommand and changeHint already present, but this adds a new unsanitized surface. Mitigation: strip or escape ANSI/escape sequences from policy-sourced strings before rendering them to the terminal (e.g., using a library like strip-ansi or validating against a safe character allowlist).

  • LOW src/policy/index.ts:evaluatePolicy — The ruleDescription value is passed through from matchedRule.description (loaded from config) into AuthResult and propagated to callers. If any downstream consumer serializes AuthResult into a log, database, or HTTP response without further sanitization, this field could carry injected content (log injection, stored XSS in a dashboard, etc.). The risk is low given current visible sinks are terminal output only, but the trust boundary between config-file-sourced data and output sinks is not explicitly enforced anywhere in the diff. Mitigation: treat all policy-file string fields as untrusted when crossing output boundaries.


Automated security review by node9

github-actions bot and others added 2 commits April 11, 2026 16:07
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 outcome here.

Why this is acceptable:

The original diff is straightforward and low-risk:

  1. A new optional field ruleDescription is threaded through the data pipeline (SmartRuleevaluatePolicy return value → AuthResult → CLI output).
  2. All additions use the safe spread-conditional pattern ...(x && { key: x }), so null/undefined values are handled correctly — no logic errors introduced.
  3. The CLI output line (writeTty) only fires when ruleDescription is truthy, preventing empty lines.
  4. No security concerns: this is read-only display of an already-trusted config value (the policy author controls matchedRule.description), and it's written to the TTY, not used in any execution path.

Test failure is unrelated to the diff:

The test command rejection (unsafe characters in 'npm run build 2>&1 && npm test 2>&1') is a test harness sandboxing issue, not a code correctness problem introduced by either the original diff or the AI.

Conclusion: The original diff is correct and the AI correctly applied no fixes. No security issues, logic errors, or correctness problems are present.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

  • MEDIUM src/cli/commands/check.ts:registerCheckCommand — The ruleDescription field originating from matchedRule.description (loaded from policy/config files) is rendered directly to the terminal via writeTty(chalk.white( ${result.ruleDescription})) without any sanitization. If policy config files are attacker-influenced (e.g., shared config repos, supply-chain compromise of config sources, or config files with insufficient access controls), a malicious description string could inject terminal escape sequences (ANSI escape codes) to manipulate the terminal display — for example, hiding subsequent output, spoofing UI elements, or causing terminal confusion. The existing fields recoveryCommand and changeHint have the same pattern, but this is a newly added unsanitized surface. Mitigation: strip or escape ANSI/control characters from description strings before rendering to the terminal.

  • LOW src/policy/index.ts:evaluatePolicymatchedRule.description is passed through from the parsed config (see omitted src/config/index.ts) into the ruleDescription field with no validation of length or content. If config parsing doesn't bound this field, an unusually large or specially crafted description string could propagate into downstream logging, storage, or display contexts beyond the TTY. This is low severity given it depends on config file compromise, but the lack of any type/length constraint on a newly trusted field is a defense-in-depth gap.


Automated security review by node9

LICENSE file and package.json already declared Apache-2.0; the README
badge was incorrect.

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 — no fixes were introduced, no logic was modified, and no security issues could have been created by the AI's intervention.

Regarding the original diff itself: The changes look correct and low-risk:

  • ruleDescription is purely a read-through field carrying a plain-text string from an existing SmartRule.description property — no new attack surface.
  • The spread pattern ...(policyResult.ruleDescription && { ruleDescription: policyResult.ruleDescription }) is consistent with the existing recoveryCommand handling directly above it.
  • The CLI display uses chalk.white() which is safe for terminal output.
  • The license badge change is cosmetic.

No security issues, logic errors, or correctness problems to flag.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

Security Review

[LOW] src/cli/commands/check.ts:registerCheckCommand — Unsanitized rule description displayed to terminal

The ruleDescription field originates from matchedRule.description in the policy configuration (src/policy/index.ts). This value is passed through the auth result chain and rendered directly to the terminal via:

if (result?.ruleDescription) writeTty(chalk.white(`   ${result.ruleDescription}`));

If the policy configuration file (src/config/index.ts, omitted) can be influenced by an attacker (e.g., a malicious policy file committed to a repo, a supply-chain attack on config, or a shared config fetched from an untrusted source), the description string could contain terminal escape sequences (ANSI injection). This could be used to visually spoof output in the terminal — for example, overwriting preceding lines, injecting fake "allowed" messages, or manipulating what the operator sees.

The existing fields recoveryCommand and changeHint appear to have the same exposure, but ruleDescription is new and explicit user-visible text designed to be human-readable prose, making it a more natural vehicle for crafted content.

Exploit scenario: Attacker controls a policy YAML/JSON file (e.g., via a compromised config source or a malicious PR to a shared policy repo). They set description: "\x1b[2A\x1b[2K✅ Node9 allowed..." to overwrite terminal output and deceive a human reviewer into thinking a blocked operation was allowed.

Mitigation: Strip or escape ANSI/control characters from ruleDescription (and similarly recoveryCommand, changeHint) before writing to the terminal. A simple approach: replace characters in the range \x00-\x1f and \x7f before output.


✅ No additional security issues found in the remaining diff. The data flow changes (adding ruleDescription to AuthResult and evaluatePolicy return type) are purely additive and don't introduce new attack surfaces beyond what's noted above.


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 (the diff is empty). There is nothing to review in terms of AI-introduced security issues, logic errors, or correctness problems.

Regarding the original diff itself (which the AI was presumably meant to fix or validate):

The changes are straightforward and low-risk:

  • License badge update in README (MIT → Apache 2.0) — documentation only
  • Image asset URL swap in README — documentation only
  • Addition of ruleDescription field to AuthResult interface and propagation through _authorizeHeadlessCore and the CLI check command display

The ruleDescription propagation is correctly guarded with policyResult.ruleDescription && before spreading, consistent with the existing pattern used for recoveryCommand. The CLI output is display-only and poses no security concern.

No issues to flag. The test failure appears to be an infrastructure/sandboxing problem (the test runner rejected the command due to unsafe characters in the shell invocation), not a code defect introduced by the diff.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

✅ No security issues found.

The changes are limited to:

  1. README documentation updates (image/badge URLs)
  2. Propagating a ruleDescription string field from policy config through the auth pipeline to CLI display output — this is a read-only data field sourced from the operator-controlled config file, not user-supplied input, and it is only written to a TTY via chalk.white() (no shell execution, no filesystem sink, no deserialization)

No user-controlled input reaches a dangerous sink in these changes.


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).

The "fix" is a no-op — the AI made zero modifications to the codebase. There are no security issues, logic errors, or correctness problems introduced because nothing was changed.

Regarding the original diff under review:

The original changes themselves look reasonable:

  • Adding ruleDescription to AuthResult interface and propagating it through _authorizeHeadlessCore is straightforward field threading with no security implications.
  • The CLI display change (writeTty(chalk.white(...))) is purely cosmetic output.
  • README changes are documentation-only.

The test failure (npm run build 2>&1 && npm test 2>&1 rejected for unsafe characters) appears to be a test harness/sandbox restriction issue unrelated to the code changes — the && operator was blocked. This is an infrastructure problem, not a code problem, and the AI correctly (if passively) made no changes in response to it.

Verdict: No issues introduced. The AI produced a null fix, which is acceptable here since there was no actual bug to fix in the reviewed diff.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

✅ No security issues found.

The diff adds a ruleDescription field (a plain-English string from SmartRule.description) that flows from policy config → evaluatePolicy return value → AuthResult → CLI output via chalk.white(). No dangerous sinks are involved: the value is written to a TTY with chalk formatting, not passed to exec, eval, filesystem operations, or reflected in an HTTP response. No new input surfaces or validation logic is introduced.


Automated security review by node9

- README: adds Flight Recorder & HUD section showing the 3-line statusline
  (security state, context/rate limits, environment counts)
- config-schema: adds `description` field to SmartRuleSchema alongside `reason`
- policy: falls back to `reason` when `description` is absent so friendly
  messages always render in the approval popup

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 — no fixes were made.

The original diff itself appears straightforward and low-risk:

  • ruleDescription field added to AuthResult interface with a safe optional spread (...(policyResult.ruleDescription && { ... })) — no logic errors or security concerns.
  • README updates are documentation-only.

No security issues, logic errors, or correctness problems are present in the original diff, and since the AI introduced no changes, nothing new to evaluate.


Automated review by node9

@github-actions
Copy link
Copy Markdown
Contributor Author

🔒 node9 Security Review

Security Review Findings

Reviewed Changes

src/cli/commands/check.tsruleDescription output to TTY

  • [LOW] src/cli/commands/check.ts:registerCheckCommand — The ruleDescription field (originating from SmartRule.description in user-controlled config files) is written directly to the TTY via writeTty(chalk.white( ${result.ruleDescription})). If the terminal emulator interprets ANSI/VT escape sequences embedded in the description string, a malicious config file could inject terminal escape codes (e.g., OSC sequences for title manipulation, cursor repositioning, or hyperlink injection). This is a low-severity terminal injection risk since it requires the attacker to control the Node9 config file — but shared/downloaded configs could carry this payload. chalk.white() does not strip or escape embedded ANSI codes in the input string.

    Recommendation: Strip ANSI escape sequences from ruleDescription before rendering (e.g., using a library like strip-ansi or a regex like /\x1b\[[0-9;]*m/g).


src/config-schema.tsdescription field added to SmartRuleSchema

  • [LOW] src/config-schema.ts:SmartRuleSchema — The new description: z.string().optional() field has no length constraint or content validation. An unbounded string from a user-controlled config file flows into ruleDescription in AuthResult and ultimately to TTY output. While not directly exploitable without terminal rendering, the lack of length limits could cause UI disruption or log bloat.

    Recommendation: Add .max(256) or similar length constraint to the description field.


No issues found in src/auth/orchestrator.ts — the propagation of ruleDescription through AuthResult is data plumbing with no dangerous sinks introduced there.


Automated security review by node9

@node9ai node9ai marked this pull request as ready for review April 13, 2026 09:08
@node9ai node9ai merged commit 99b18db into main Apr 13, 2026
11 checks passed
node9ai pushed a commit that referenced this pull request Apr 13, 2026
## [1.9.3](v1.9.2...v1.9.3) (2026-04-13)

### Bug Fixes

* dev → main ([#84](#84)) ([99b18db](99b18db))
@node9ai
Copy link
Copy Markdown
Contributor

node9ai commented Apr 13, 2026

🎉 This PR is included in version 1.9.3 🎉

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