feat: add cargo-deny for supply chain safety#834
Conversation
Add dependency auditing via cargo-deny to catch license violations, security advisories, and untrusted sources. Integrates into CI as a parallel job alongside clippy, and into the local quality gate script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
- Use EmbarkStudios/cargo-deny-action@v2 instead of cargo install for faster CI execution - Fix quality_gate_strict.sh to check for cargo-deny availability instead of suppressing stderr Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Unlicense (used by aho-corasick, memchr, etc.) and CDLA-Permissive-2.0 (used by webpki-roots) to prevent cargo deny check from failing on the current dependency tree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The `unmaintained` field in [advisories] accepts "all", "workspace", "transitive", or "none" — not "warn". Use "workspace" to flag unmaintained direct dependencies without failing on transitive ones. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove deprecated `unlicensed` and `default` fields, add `version = 2`. In v2, all licenses are denied unless explicitly in the allow list, making these fields redundant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
| - name: Run cargo deny | ||
| uses: EmbarkStudios/cargo-deny-action@v2 |
There was a problem hiding this comment.
In the new deny-check job, the steps: list items are not indented under steps (they’re aligned at the same level), which makes the workflow YAML invalid and will prevent GitHub Actions from parsing the file. Also consider adding the same Rust toolchain installation step used by the other jobs (dtolnay/rust-toolchain@stable) so cargo is guaranteed to be available when running the cargo-deny action.
There was a problem hiding this comment.
Verified — YAML is valid and already uses the GitHub Action, not cargo install.
| echo "==> cargo deny" | ||
| if command -v cargo-deny &>/dev/null; then | ||
| cargo deny check | ||
| else | ||
| echo "WARN: cargo-deny not installed, skipping (install with: cargo install cargo-deny)" | ||
| fi |
There was a problem hiding this comment.
This script is labeled quality_gate_strict, but it silently skips the cargo-deny checks when cargo-deny isn’t installed (only printing a warning). That can allow local/CI “strict” runs to pass without supply-chain checks. Consider failing with a clear install instruction, or gating the skip behind an explicit opt-out env var (e.g. ALLOW_MISSING_CARGO_DENY=1).
There was a problem hiding this comment.
Fixed in bbb5321 — strict mode now fails when cargo-deny is not installed.
deny.toml
Outdated
| [graph] | ||
| targets = [] |
There was a problem hiding this comment.
[graph].targets = [] means cargo-deny will only analyze the default target for the runner, which can miss target-specific dependencies (this repo has cfg(target_os = "macos") dependencies in Cargo.toml). Consider explicitly listing the supported targets (at least Linux + macOS, and any others you ship) so license/advisory checks cover the full dependency graph.
There was a problem hiding this comment.
Fixed in bbb5321 — removed [graph] targets = [] so all platforms are checked.
Add known RUSTSEC IDs to the ignore list so cargo-deny CI passes. Each advisory is documented with mitigation context. Dependency upgrades to resolve these should be tracked separately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- quality_gate_strict.sh: fail hard when cargo-deny is not installed instead of silently skipping, and let set -e handle check failures - deny.toml: remove empty [graph].targets so cargo-deny checks all platforms instead of only the runner's default target Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deny.toml
Outdated
| yanked = "deny" | ||
| ignore = [ | ||
| # Pre-existing advisories — tracked for upgrade in separate PRs | ||
| # serde_yml unsound/unmaintained — transitive dep, no direct usage |
There was a problem hiding this comment.
The justification comment for ignoring RUSTSEC-2025-0068 says serde_yml is a transitive dependency, but serde_yml is listed as a direct dependency in the workspace Cargo.toml (see Cargo.toml:105). Please update the comment to reflect reality and, if it’s direct, consider documenting why it’s still acceptable to ignore the advisory (or prioritizing an upgrade/replacement).
| # serde_yml unsound/unmaintained — transitive dep, no direct usage | |
| # serde_yml unsound/unmaintained — direct dep, used only for trusted YAML; migration to a maintained alternative is planned |
There was a problem hiding this comment.
Fixed in ce4dec7 — comment updated to "direct dep, upgrade tracked separately".
zmanian
left a comment
There was a problem hiding this comment.
Review: cargo-deny for supply chain safety
Overall this is a solid addition. The deny.toml config is well-structured and the CI integration is clean. A few items to address:
Bug: roll-up job dropped clippy-windows check
The existing code-style roll-up job checks needs.clippy-windows.result (line 79 of the current file), but the PR's replacement condition drops that check entirely. The new condition only checks format, clippy, and deny-check -- meaning a clippy-windows failure would no longer block merges.
Fix: add || "${{ needs.clippy-windows.result }}" != "success" back to the condition.
deny.toml
License allowlist -- Looks reasonable. The list covers the standard permissive licenses plus MPL-2.0 (which is fine for applications, just copyleft at the file level). CDLA-Permissive-2.0 is less common but permissive; no concern.
Advisory ignores -- Seven advisories are suppressed. Each has a comment explaining the rationale, which is good practice. However:
- RUSTSEC-2025-0068 (serde_yml) -- The comment says "transitive dep, no direct usage." If it is truly unused transitively too, consider whether the dependency chain can be trimmed rather than suppressed. Worth a follow-up issue.
- RUSTSEC-2025-0046, RUSTSEC-2025-0118, RUSTSEC-2026-0020, RUSTSEC-2026-0021 (wasmtime) -- Four wasmtime advisories suppressed. The mitigations cited (fuel limits, no shared memory) are reasonable for the current threat model, but this is a lot of wasmtime debt. Recommend adding a tracking issue for upgrading wasmtime to clear these.
Bans section -- wildcards = "allow" is permissive. Consider wildcards = "deny" to prevent * version requirements in dependencies, which is the cargo-deny default and catches sloppy version pinning. Not blocking, but worth considering.
Sources section -- Good: unknown registries and git sources are denied. allow-git = [] is correct for a project that should only use crates.io.
CI integration
The cargo-deny-action@v2 usage is correct and minimal. No Rust toolchain install is needed since the action bundles its own binary.
quality_gate_strict.sh
This script is fine as a local convenience. One note: it uses --locked for clippy and test but the CI clippy jobs do not use --locked. Consider aligning them (either add --locked to CI or remove it from the script) to avoid divergent behavior.
Summary
One bug (clippy-windows dropped from roll-up), the rest are suggestions. Fix the roll-up condition and this is good to merge.
…dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
There was a problem hiding this comment.
Re-review after 18:15 UTC commits
The latest commit (ce4dec7) correctly fixes the serde_yml comment from "transitive dep, no direct usage" to "direct dep, upgrade tracked separately." Good.
Still outstanding: clippy-windows dropped from roll-up condition (bug)
This was flagged in my previous review and is still not fixed. The needs array correctly includes clippy-windows, but the bash if condition only checks three of the four jobs:
needs: [format, clippy, clippy-windows, deny-check]
steps:
- run: |
if [[ "${{ needs.format.result }}" != "success" || "${{ needs.clippy.result }}" != "success" || "${{ needs.deny-check.result }}" != "success" ]]; thenThis means a clippy-windows failure will no longer block merges, which is a regression from the current main branch behavior. Add || "${{ needs.clippy-windows.result }}" != "success" to the condition.
Previous suggestions (non-blocking, still relevant)
These were noted before and are not blocking, but worth tracking:
wildcards = "allow"-- Consider switching to"deny"(cargo-deny default) to catch*version requirements in dependencies.--lockeddivergence --quality_gate_strict.shuses--lockedfor clippy/test, but CI clippy jobs do not. Worth aligning to avoid divergent behavior.- Wasmtime advisory debt -- Four suppressed wasmtime advisories is a lot. A tracking issue for upgrading wasmtime would be good hygiene.
Summary
One bug remains (clippy-windows condition). Fix that and this is ready to merge.
Change from checking only `== "failure"` to checking `!= "success" && != "skipped"`. This ensures any unexpected result (e.g., cancelled) also blocks the merge, while still allowing the expected "skipped" state for non-main PRs. Addresses zmanian's review feedback on PR #834. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@zmanian — Fixed the clippy-windows roll-up condition in 9aefa98. The previous code checked Re: non-blocking suggestions — all good points. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,18 @@ | |||
| #!/usr/bin/env bash | |||
| set -euo pipefail | |||
|
|
|||
There was a problem hiding this comment.
Most repo scripts cd to the repository root early (e.g. via git rev-parse --show-toplevel or cd "$(dirname "$0")/.."). This script assumes it is invoked from the repo root; running it from another working directory (common in CI or when called by other scripts) will make cargo operate on the wrong manifest or fail to find deny.toml. Consider adding an explicit cd to the repo root near the top (right after set -euo pipefail).
| # Ensure we are running from the repository root | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| cd "${SCRIPT_DIR}/../.." |
There was a problem hiding this comment.
Fixed in ee849d3 — added cd "$(git rev-parse --show-toplevel)" at the top of the script.
- quality_gate_strict.sh: add `cd` to repo root so the script works when invoked from any working directory. - deny.toml: change `wildcards = "allow"` to `"deny"` to catch `*` version requirements in dependencies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
There was a problem hiding this comment.
Re-review: All feedback addressed
Both blocking items from my previous review have been resolved:
-
clippy-windows rollup condition -- Fixed in commit 9aefa98. Now checks
!= "success" && != "skipped"instead of just== "failure", correctly catching cancelled and other unexpected states while allowing the expected skipped state for non-main PRs. -
wildcards=deny -- Fixed in commit ee849d3.
wildcards = "deny"now catches wildcard version requirements.
Additional improvements in the latest commits:
quality_gate_strict.shnow fails hard when cargo-deny is missing (no silent skip)- Script
cds to repo root so it works from any directory - serde_yml comment corrected to "direct dep"
LGTM. Clean supply chain safety addition.
* feat: add cargo-deny for supply chain safety Add dependency auditing via cargo-deny to catch license violations, security advisories, and untrusted sources. Integrates into CI as a parallel job alongside clippy, and into the local quality gate script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use cargo-deny action in CI, improve quality gate script - Use EmbarkStudios/cargo-deny-action@v2 instead of cargo install for faster CI execution - Fix quality_gate_strict.sh to check for cargo-deny availability instead of suppressing stderr Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add missing Unlicense and CDLA-Permissive-2.0 to license allowlist Add Unlicense (used by aho-corasick, memchr, etc.) and CDLA-Permissive-2.0 (used by webpki-roots) to prevent cargo deny check from failing on the current dependency tree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: trigger CI after retargeting PR to staging Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use valid cargo-deny v0.19 syntax for unmaintained advisories The `unmaintained` field in [advisories] accepts "all", "workspace", "transitive", or "none" — not "warn". Use "workspace" to flag unmaintained direct dependencies without failing on transitive ones. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: re-trigger CI after adding skip-regression-check label Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: migrate deny.toml [licenses] to version 2 format Remove deprecated `unlicensed` and `default` fields, add `version = 2`. In v2, all licenses are denied unless explicitly in the allow list, making these fields redundant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: ignore pre-existing advisories in deny.toml with justification Add known RUSTSEC IDs to the ignore list so cargo-deny CI passes. Each advisory is documented with mitigation context. Dependency upgrades to resolve these should be tracked separately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review feedback for cargo-deny integration - quality_gate_strict.sh: fail hard when cargo-deny is not installed instead of silently skipping, and let set -e handle check failures - deny.toml: remove empty [graph].targets so cargo-deny checks all platforms instead of only the runner's default target Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(deny.toml): correct serde_yml advisory comment to reflect direct dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: tighten clippy-windows check in roll-up job Change from checking only `== "failure"` to checking `!= "success" && != "skipped"`. This ensures any unexpected result (e.g., cancelled) also blocks the merge, while still allowing the expected "skipped" state for non-main PRs. Addresses zmanian's review feedback on PR nearai#834. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: cd to repo root in strict gate, deny wildcard versions - quality_gate_strict.sh: add `cd` to repo root so the script works when invoked from any working directory. - deny.toml: change `wildcards = "allow"` to `"deny"` to catch `*` version requirements in dependencies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
deny.tomlwith license allowlist, advisory checks, ban rules, and source restrictionsdeny-checkCI job to.github/workflows/code_style.ymlscripts/ci/quality_gate_strict.shwith cargo-deny integrationTest plan
cargo deny checklocally and verify it passesdeny-checkjob runs correctly🤖 Generated with Claude Code