feat: security & quality CI — audit, deny, dependency review#62
feat: security & quality CI — audit, deny, dependency review#62kienbui1995 merged 5 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub Actions workflows for dependency review and security (cargo-audit, cargo-deny, strict Clippy), tightens Clippy lint flags in CI, adds a Cargo Deny config for the Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant Repo as Repository (mc)
participant Audit as cargo-audit
participant Deny as cargo-deny
participant Clippy as cargo-clippy
PR->>GH: push / pull_request triggers workflows
GH->>Repo: checkout (mc workspace)
GH->>Audit: run cargo-audit (audit job)
GH->>Deny: run cargo-deny (deny job)
GH->>Clippy: run cargo-clippy (clippy-strict job)
Audit-->>GH: audit results
Deny-->>GH: deny results
Clippy-->>GH: lint pass/fail
GH-->>PR: report status (pass/fail)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several minor refactors, including case-insensitive file extension checks for Jupyter notebooks, removal of unused imports, and TUI history mapping improvements. It also adds a deny.toml configuration for dependency auditing. Feedback focuses on ensuring that the new Jupyter notebook reading logic respects pagination parameters (offset and limit) and recommends tightening security policies in deny.toml by denying yanked crates, wildcard dependencies, and unknown registries or git sources.
| ) -> Result<String, ToolError> { | ||
| // Jupyter notebook support: extract cell sources as readable text | ||
| if path.ends_with(".ipynb") { | ||
| if path.to_ascii_lowercase().ends_with(".ipynb") { |
There was a problem hiding this comment.
The newly added Jupyter notebook support returns the entire extracted content immediately, bypassing the offset and limit logic defined later in the function. This leads to inconsistent behavior where pagination or previews are not supported for .ipynb files. Consider refactoring the logic to load the content (either raw or extracted) first, and then apply the slicing/preview logic uniformly.
| [advisories] | ||
| vulnerability = "deny" | ||
| unmaintained = "warn" | ||
| yanked = "warn" |
There was a problem hiding this comment.
Using yanked crates can lead to builds that are not reproducible or contain known issues that led to the yank. It is recommended to set yanked to deny to ensure the supply chain only uses valid, active crate versions.
| yanked = "warn" | |
| yanked = "deny" |
mc/deny.toml
Outdated
|
|
||
| [bans] | ||
| multiple-versions = "warn" | ||
| wildcards = "allow" |
There was a problem hiding this comment.
Allowing wildcard dependencies (*) bypasses version pinning and can lead to unpredictable builds or supply chain vulnerabilities. Since this PR introduces security auditing, it is recommended to set this to deny to enforce explicit versioning.
| wildcards = "allow" | |
| wildcards = "deny" |
| unknown-registry = "warn" | ||
| unknown-git = "warn" |
There was a problem hiding this comment.
To strictly enforce the allowed sources and prevent the accidental inclusion of crates from untrusted registries or git repositories, it is recommended to set unknown-registry and unknown-git to deny.
| unknown-registry = "warn" | |
| unknown-git = "warn" | |
| unknown-registry = "deny" | |
| unknown-git = "deny" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
mc/crates/mc-tui/src/history.rs (1)
166-166: Consider using an atomic counter for temp file uniqueness.The test uses
std::process::id()for generating unique temp file paths, but this doesn't prevent collisions when tests run in parallel within the same process (Cargo's default). As per coding guidelines, use an atomic counter for unique temp file paths in tests to avoid parallel test collisions.♻️ Suggested pattern with atomic counter
Add a static atomic counter at module level:
#[cfg(test)] mod tests { use super::*; use std::sync::atomic::{AtomicU64, Ordering}; static TEST_COUNTER: AtomicU64 = AtomicU64::new(0);Then update the test:
fn persistence_roundtrip() { - let path = std::env::temp_dir().join(format!("mc-hist-{}", std::process::id())); + let id = TEST_COUNTER.fetch_add(1, Ordering::Relaxed); + let path = std::env::temp_dir().join(format!("mc-hist-{}", id)); {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-tui/src/history.rs` at line 166, Replace use of std::process::id() for test temp-file uniqueness with a module-level atomic counter: add a static AtomicU64 (e.g. TEST_COUNTER) inside the #[cfg(test)] mod and in the test code that builds the temp path (the expression creating path via std::env::temp_dir().join(format!("mc-hist-{}", std::process::id())) in mc-tui::history tests) use TEST_COUNTER.fetch_add(1, Ordering::SeqCst) (or Relaxed ordering if appropriate) to produce the unique suffix instead of std::process::id(), ensuring each parallel test gets a distinct temp filename..github/workflows/security.yml (1)
21-22: Pincargo-audit/cargo-denyversions for reproducible CI.Installing latest-at-runtime can introduce nondeterministic failures and unexpected policy/tool behavior drift.
📌 Suggested pattern
+env: + CARGO_AUDIT_VERSION: "x.y.z" + CARGO_DENY_VERSION: "a.b.c" ... - - name: Install cargo-audit - run: cargo install cargo-audit --locked + - name: Install cargo-audit + run: cargo install cargo-audit --locked --version $CARGO_AUDIT_VERSION ... - - name: Install cargo-deny - run: cargo install cargo-deny --locked + - name: Install cargo-deny + run: cargo install cargo-deny --locked --version $CARGO_DENY_VERSIONAlso applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/security.yml around lines 21 - 22, The workflow installs tools without pinned versions; change the steps that run "cargo install cargo-audit --locked" and the analogous "cargo-deny" install to specify exact versions (e.g., add "--version <version>" or use a pinned release tag) so CI is reproducible; update the step names "Install cargo-audit" and the "cargo-deny" install step to include the chosen version strings and ensure any corresponding cache or verification steps are updated accordingly..github/workflows/ci.yml (1)
29-29: Consider restoring warnings-as-errors in this CI Clippy job.Line 29 no longer enforces all warnings as errors in this workflow step; adding
-D warningskeeps this gate aligned with strict CI expectations.🔁 Proposed adjustment
- run: cargo clippy --workspace --all-targets -- -D clippy::correctness -D clippy::suspicious -W clippy::complexity -A clippy::too_many_lines + run: cargo clippy --workspace --all-targets -- -D warnings -D clippy::correctness -D clippy::suspicious -W clippy::complexity -A clippy::too_many_linesBased on learnings: CI enforces
RUSTFLAGS=-Dwarnings— all warnings are errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 29, The CI clippy step currently runs the command shown in the `run: cargo clippy --workspace --all-targets -- -D clippy::correctness -D clippy::suspicious -W clippy::complexity -A clippy::too_many_lines` invocation but no longer treats general warnings as errors; update that `cargo clippy` invocation to also fail on all warnings by adding `-D warnings` (or set `RUSTFLAGS=-Dwarnings` for the job) so the step enforces warnings-as-errors consistently with the rest of CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/security.yml:
- Around line 56-60: The CI clippy invocation currently lowers the severity for
specific lints by using -W for clippy::unwrap_used, clippy::expect_used, and
clippy::panic even though -D warnings is set; change those three flags from -W
to -D so they are denied (e.g., replace -W clippy::unwrap_used, -W
clippy::expect_used, -W clippy::panic with -D clippy::unwrap_used, -D
clippy::expect_used, -D clippy::panic) to ensure unwrap/expect/panic usages fail
the workflow.
- Around line 3-11: Add a top-level permissions block to the workflow (placed
after the existing on: block and before jobs:) to scope GITHUB_TOKEN to
read-only; for example add permissions: contents: read (and any other specific
read scopes you need like pull-requests: read or issues: read) so the workflow
does not inherit broad defaults.
In `@mc/crates/mc-config/src/types.rs`:
- Around line 482-487: The TOML raw-string literal r"..." usage was simplified
in one place but identical TOML snippets in other test functions still use
r#"..."#; update those remaining occurrences (the test functions that embed the
same TOML snippet containing [managed_agents] with enabled, budget_usd and
executor_max_turns) to use the simpler r"..." form for consistency with Clippy
pedantic fixes, i.e., replace the r#"..."# delimiters with r"..." for each
identical TOML literal in this file.
In `@mc/deny.toml`:
- Around line 30-33: Update the source policy to block unapproved package
sources by changing the unknown source levels: set unknown-registry = "deny" and
unknown-git = "deny" (currently "warn") in the deny.toml so cargo-deny will fail
on any unapproved registry or git dependency; ensure existing allow-registry and
allow-git entries (allow-registry and allow-git) remain unchanged unless you
intentionally want to expand approved sources.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 29: The CI clippy step currently runs the command shown in the `run:
cargo clippy --workspace --all-targets -- -D clippy::correctness -D
clippy::suspicious -W clippy::complexity -A clippy::too_many_lines` invocation
but no longer treats general warnings as errors; update that `cargo clippy`
invocation to also fail on all warnings by adding `-D warnings` (or set
`RUSTFLAGS=-Dwarnings` for the job) so the step enforces warnings-as-errors
consistently with the rest of CI.
In @.github/workflows/security.yml:
- Around line 21-22: The workflow installs tools without pinned versions; change
the steps that run "cargo install cargo-audit --locked" and the analogous
"cargo-deny" install to specify exact versions (e.g., add "--version <version>"
or use a pinned release tag) so CI is reproducible; update the step names
"Install cargo-audit" and the "cargo-deny" install step to include the chosen
version strings and ensure any corresponding cache or verification steps are
updated accordingly.
In `@mc/crates/mc-tui/src/history.rs`:
- Line 166: Replace use of std::process::id() for test temp-file uniqueness with
a module-level atomic counter: add a static AtomicU64 (e.g. TEST_COUNTER) inside
the #[cfg(test)] mod and in the test code that builds the temp path (the
expression creating path via std::env::temp_dir().join(format!("mc-hist-{}",
std::process::id())) in mc-tui::history tests) use TEST_COUNTER.fetch_add(1,
Ordering::SeqCst) (or Relaxed ordering if appropriate) to produce the unique
suffix instead of std::process::id(), ensuring each parallel test gets a
distinct temp filename.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6de70ef3-ecd5-4893-a660-57d558034b72
📒 Files selected for processing (9)
.github/workflows/ci.yml.github/workflows/dependency-review.yml.github/workflows/security.ymlmc/crates/mc-config/src/types.rsmc/crates/mc-provider/tests/http_mock.rsmc/crates/mc-tools/src/file_ops.rsmc/crates/mc-tools/src/permissions.rsmc/crates/mc-tui/src/history.rsmc/deny.toml
💤 Files with no reviewable changes (1)
- mc/crates/mc-provider/tests/http_mock.rs
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: | ||
| branches: [main] | ||
| schedule: | ||
| - cron: "0 8 * * 1" # Weekly Monday 8am | ||
|
|
||
| jobs: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/security.ymlRepository: kienbui1995/mc-code
Length of output: 2025
🌐 Web query:
GitHub Actions GITHUB_TOKEN permissions least privilege best practice
💡 Result:
The best practice for GITHUB_TOKEN permissions in GitHub Actions follows the principle of least privilege: explicitly specify only the minimum required permissions needed for each workflow or job, rather than relying on broad defaults. Key practices: 1. Set repository/organization defaults to restricted: In repository Settings > Actions > General > Workflow permissions, select "Read repository contents and packages permissions" (contents: read, packages: read). This is the default for new repositories/organizations and sufficient for basic clone/build workflows. Overrides in workflow files can add more as needed. {2,9,20} 2. Use the permissions key in workflow YAML: Define at workflow level (applies to all jobs) or job level (more granular). Unspecified permissions default to none. Examples: - Workflow level: permissions: contents: read packages: read - Job level (overrides workflow): jobs: build: permissions: contents: read release: permissions: contents: write packages: write Common minimal permissions: contents: read (for checkout), checks: write (for status), pull-requests: write (for comments), id-token: write (for OIDC). {1,3,4,14,15} 3. Determine minimal permissions: Analyze workflow steps/actions. Tools like GitHub's actions-permissions monitor API calls and recommend minimal set. Start broad, monitor failures, iteratively reduce. {5,6,10} 4. Fork/PR considerations: For pull_request from forks, permissions auto-downgraded to read-only unless "Send write tokens to workflows from pull requests" enabled (avoid for security). Use pull_request event for untrusted code. {15,20} 5. Additional security: Pin actions to SHA/commit (not tags/branches), avoid passing GITHUB_TOKEN explicitly if possible (actions access via github.token context), use OIDC for cloud auth instead of long-lived secrets. {9,12} This minimizes blast radius if workflows are compromised (e.g., via malicious action or fork PR).
Citations:
- 1: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
- 2: https://docs.github.com/en/actions/concepts/security/github_token
- 3: https://docs.github.com/actions/reference/authentication-in-a-workflow
- 4: https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs/
- 5: https://www.stepsecurity.io/blog/github-token-how-it-works-and-how-to-secure-automatic-github-action-tokens
- 6: https://docs.github.com/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions
- 7: https://github.blog/security/new-tool-to-secure-your-github-actions/
- 8: https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs
- 9: https://docs.github.com/actions/reference/workflow-syntax-for-github-actions
- 10: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
Add least-privilege permissions for GITHUB_TOKEN.
This workflow does not declare permissions, so it inherits broader defaults than needed. Set explicit read-only scope at workflow level.
🔒 Proposed fix
name: Security
on:
push:
branches: [main]
pull_request:
branches: [main]
schedule:
- cron: "0 8 * * 1" # Weekly Monday 8am
+
+permissions:
+ contents: read🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/security.yml around lines 3 - 11, Add a top-level
permissions block to the workflow (placed after the existing on: block and
before jobs:) to scope GITHUB_TOKEN to read-only; for example add permissions:
contents: read (and any other specific read scopes you need like pull-requests:
read or issues: read) so the workflow does not inherit broad defaults.
.github/workflows/security.yml
Outdated
| cargo clippy --workspace --all-targets -- \ | ||
| -D warnings \ | ||
| -W clippy::unwrap_used \ | ||
| -W clippy::expect_used \ | ||
| -W clippy::panic \ |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For rustc/clippy CLI lint flags, when both -D warningsand later-W clippy::unwrap_used are provided, which lint level takes precedence?
💡 Result:
The later flag -W clippy::unwrap_used takes precedence, setting the level of that specific Clippy lint to "warn". The earlier -D warnings sets all warn-level lints (including rustc and Clippy warn-by-default lints) to "deny", but subsequent specific flags override previous group settings for individual lints.
Citations:
- 1: https://doc.rust-lang.org/stable/rustc/lints/levels.html
- 2: https://doc.rust-lang.org/stable/rustc/command-line-arguments.html
🏁 Script executed:
cat -n .github/workflows/security.yml | sed -n '50,65p'Repository: kienbui1995/mc-code
Length of output: 575
Set unwrap/expect/panic lints to deny instead of warn.
Line 57 sets -D warnings, but lines 58–60 explicitly set those lints to -W, which overrides the deny severity back to warn. These should be -D to block merges when unwrap/expect/panic are used, consistent with the project's requirement that library code never use unwrap().
Proposed fix
cargo clippy --workspace --all-targets -- \
-D warnings \
- -W clippy::unwrap_used \
- -W clippy::expect_used \
- -W clippy::panic \
+ -D clippy::unwrap_used \
+ -D clippy::expect_used \
+ -D clippy::panic \
-A clippy::too_many_lines \
-A clippy::module_name_repetitions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/security.yml around lines 56 - 60, The CI clippy
invocation currently lowers the severity for specific lints by using -W for
clippy::unwrap_used, clippy::expect_used, and clippy::panic even though -D
warnings is set; change those three flags from -W to -D so they are denied
(e.g., replace -W clippy::unwrap_used, -W clippy::expect_used, -W clippy::panic
with -D clippy::unwrap_used, -D clippy::expect_used, -D clippy::panic) to ensure
unwrap/expect/panic usages fail the workflow.
| r" | ||
| [managed_agents] | ||
| enabled = true | ||
| budget_usd = -1.0 | ||
| executor_max_turns = 0 | ||
| "#, | ||
| ", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if clippy suggests simplifying other raw string literals in this file.
# Expected: If clippy pedantic is enabled, it may flag other instances of unnecessary hash delimiters.
cd mc/crates/mc-config
cargo clippy --all-targets -- -W clippy::pedantic 2>&1 | grep -A 2 "types.rs"Repository: kienbui1995/mc-code
Length of output: 149
🏁 Script executed:
cd mc/crates/mc-config && sed -n '475,495p' src/types.rsRepository: kienbui1995/mc-code
Length of output: 686
🏁 Script executed:
cd mc/crates/mc-config && sed -n '379,392p;401,406p;420,426p;434,448p;462,471p' src/types.rsRepository: kienbui1995/mc-code
Length of output: 1120
Simplify remaining raw string delimiters for consistency.
The change from r#"..."# to r"..." is correct since the TOML content contains no literal quote characters. However, this appears to be an incomplete clippy fix—other test functions in this file (lines 379-385, 387-392, 401-406, 420-426, 434-448, 462-471) use identical TOML strings without embedded quotes yet still retain r#"..."#.
Since the coding guidelines require following all clippy pedantic suggestions, either simplify these other instances as well for consistency, or clarify why they were excluded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-config/src/types.rs` around lines 482 - 487, The TOML raw-string
literal r"..." usage was simplified in one place but identical TOML snippets in
other test functions still use r#"..."#; update those remaining occurrences (the
test functions that embed the same TOML snippet containing [managed_agents] with
enabled, budget_usd and executor_max_turns) to use the simpler r"..." form for
consistency with Clippy pedantic fixes, i.e., replace the r#"..."# delimiters
with r"..." for each identical TOML literal in this file.
mc/deny.toml
Outdated
| unknown-registry = "warn" | ||
| unknown-git = "warn" | ||
| allow-registry = ["https://github.com/rust-lang/crates.io-index"] | ||
| allow-git = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In cargo-deny, does setting [sources].unknown-git = "warn"allowcargo deny check sources to pass instead of failing?
💡 Result:
Yes, setting [sources].unknown-git = "warn" allows cargo deny check sources to pass instead of failing. It prints a warning for each crate from an unknown git repository (not in the allow-git list) but does not fail the check.
Citations:
- 1: https://embarkstudios.github.io/cargo-deny/checks/sources/cfg.html
- 2: https://github.com/EmbarkStudios/cargo-deny/blob/main/deny.template.toml
Use "deny" instead of "warn" for unknown package sources to block unapproved registries/git dependencies.
Lines 30–31 are set to "warn", which allows cargo deny check sources to pass when an unknown registry or git source is introduced. This defeats the supply-chain control this workflow should enforce. Change both to "deny" to gate PRs on unauthorized sources.
Proposed fix
[sources]
-unknown-registry = "warn"
-unknown-git = "warn"
+unknown-registry = "deny"
+unknown-git = "deny"
allow-registry = ["https://github.com/rust-lang/crates.io-index"]
allow-git = []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unknown-registry = "warn" | |
| unknown-git = "warn" | |
| allow-registry = ["https://github.com/rust-lang/crates.io-index"] | |
| allow-git = [] | |
| unknown-registry = "deny" | |
| unknown-git = "deny" | |
| allow-registry = ["https://github.com/rust-lang/crates.io-index"] | |
| allow-git = [] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/deny.toml` around lines 30 - 33, Update the source policy to block
unapproved package sources by changing the unknown source levels: set
unknown-registry = "deny" and unknown-git = "deny" (currently "warn") in the
deny.toml so cargo-deny will fail on any unapproved registry or git dependency;
ensure existing allow-registry and allow-git entries (allow-registry and
allow-git) remain unchanged unless you intentionally want to expand approved
sources.
…clippy New workflows: 1. security.yml: - cargo-audit: scan for known vulnerabilities in deps - cargo-deny: license compliance, supply chain checks - Strict clippy: correctness + suspicious as errors 2. dependency-review.yml: - Block PRs adding deps with high-severity vulns - Deny AGPL-3.0 and GPL-3.0 licenses 3. deny.toml: license allowlist, advisory checks, source restrictions 4. CI clippy upgraded: -D clippy::correctness -D clippy::suspicious 5. Clippy fixes: unused import, redundant closure, case-sensitive extension, raw string hashes 6. Dependabot vulnerability alerts enabled 274 tests, 0 fail.
- deny.toml: fix invalid unmaintained value format - security.yml: use same clippy level as ci.yml (-D correctness -D suspicious, not -D warnings)
- Removed deprecated fields: vulnerability, notice (now always error) - unmaintained: 'warn' → 'workspace' (valid: all/workspace/transitive/none) - Removed db-path/db-urls (use defaults)
…-threshold, wildcards, allow-git)
…allowance warnings
kienbui1995
force-pushed
the
feat/security-quality-checks
branch
from
3a4f166 to
28eafd4
Compare
1 participant
Security & Quality Checks
deny.toml
274 tests, 0 fail.
Summary by CodeRabbit
New Features
Bug Fixes
Chores