Skip to content

fix(security): address CodeRabbit critical findings#31

Merged
kienbui1995 merged 1 commit intomainfrom
fix/coderabbit-critical
Apr 11, 2026
Merged

fix(security): address CodeRabbit critical findings#31
kienbui1995 merged 1 commit intomainfrom
fix/coderabbit-critical

Conversation

@kienbui1995
Copy link
Copy Markdown
Owner

@kienbui1995 kienbui1995 commented Apr 11, 2026

Fixes 4 issues found by CodeRabbit:

🔴 Plugin path traversal/plugin remove ../../ could delete arbitrary dirs. Added sanitize_plugin_name() rejecting .., /, \.

🔴 --yes override — config tool_permissions was applied AFTER --yes, overriding it. Now config applies first, --yes takes precedence.

🟠 Empty model override — `Some("")" in subagent now falls back to default model.

🟡 Redundant format! — removed unnecessary format!("{system_prompt}").

180 tests pass.

Summary by CodeRabbit

  • Bug Fixes
    • Plugin names are now validated to reject invalid names, path traversal characters, and empty values with appropriate error messaging.
    • Permission policy now correctly applies configuration-defined overrides before evaluating the --yes flag.
    • Model override validation improved to handle empty or whitespace-only values.

- Plugin path traversal: sanitize_plugin_name() rejects .., /, \
- --yes override order: config applied first, --yes takes precedence
- Empty model override: filter empty strings in subagent
- Redundant format! macro removed
@kienbui1995 kienbui1995 merged commit 9331572 into main Apr 11, 2026
4 of 5 checks passed
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 11, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1e583ee1-a45d-4255-974d-f9d708166b29

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf6ae1 and da60c20.

📒 Files selected for processing (2)
  • mc/crates/mc-cli/src/main.rs
  • mc/crates/mc-core/src/subagent.rs

📝 Walkthrough

Walkthrough

Permission policy construction now applies config-defined per-tool overrides before evaluating the --yes flag, with --yes rebuilding an all-allow policy applied last as override (excluding bash unless --dangerously-allow-bash is also set). Plugin management introduces sanitize_plugin_name function to reject empty names, relative path references, separators, and null bytes. Subagent model override validation requires non-empty trimmed strings.

Changes

Cohort / File(s) Summary
Permission Policy & Plugin Management
mc/crates/mc-cli/src/main.rs
Adds sanitize_plugin_name() function to validate plugin names for path traversal and unsafe characters. Reorders permission policy construction: applies config overrides first, then conditionally applies --yes all-allow policy (preserving bash in Prompt mode unless --dangerously-allow-bash is set) as final override. Applies sanitization check to /plugin install, /plugin remove, and /plugin update commands before path operations.
Model Override Validation
mc/crates/mc-core/src/subagent.rs
Changes model_override validation in SubagentSpawner::run_task to treat it as valid only if non-empty after trimming; uses system_prompt.to_string() instead of format!() macro when building enriched prompt with empty shared context.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐰 A bunny hops through paths so clean,
No traversals in plugin names seen!
Permission rules now stack just right,
While --yes shines bright as night,
Validation trimmed, the code takes flight! 🌙

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/coderabbit-critical

Comment @coderabbitai help to get the list of available commands and usage tips.

@kienbui1995 kienbui1995 deleted the fix/coderabbit-critical branch April 11, 2026 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant