ci(rs): add cargo-deny and resolve outstanding advisories#1486
Conversation
Adds cargo-deny to the Nix dev shell and wires `cargo deny check` into `just rs check` (and thus `just rs ci`) so license, advisory, source, and ban policy is enforced on every PR. The new deny-reminder recipe prints active advisory ignores with their inline rationale after each run so they don't get forgotten; cargo-deny's native ignored-advisory notes only surface at info log level. Initial run surfaced 9 findings; this commit resolves 5 of them: - Bump web-transport-iroh 0.4 -> 0.5 to pull hickory 0.26.1, fixing RUSTSEC-2026-0119 (hickory-proto CPU exhaustion) and RUSTSEC-2026-0120 (hickory-net DNSSEC infinite loop). Drops atomic-polyfill (RUSTSEC-2023-0089) transitively. - Migrate moq-native off the unmaintained rustls-pemfile crate (RUSTSEC-2025-0134) to the rustls-pki-types `PemObject` trait, the upstream-recommended replacement. - Bump mp4-atom 0.10 -> 0.11 so moq-mux pulls pastey (maintained) rather than paste via mp4-atom. paste is still pulled by gstreamer 0.23 (whose 0.25 release requires Rust 1.92, beyond our 1.85 MSRV); RUSTSEC-2024-0436 stays in the ignore list. 0.11 adds a Moov.ainf field; default to None. The four remaining ignores (rsa 0.9 Marvin Attack, yaml-rust via foundations/tokio-quiche, paste via gstreamer, bincode 1.x via http-cache) each have an inline rationale in deny.toml naming the upstream blocker. rsa is unreachable for us: moq-token uses it only for keygen and JWK component serialization, while signing runs through jsonwebtoken backed by aws-lc-rs (constant time).
|
Warning Review limit reached
Your plan currently allows 4 reviews/hour. Refill in 2 minutes and 53 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces three independent bodies of work: (1) cargo-deny security audit infrastructure, including configuration, documentation, and integration into the development and verification workflows; (2) dependency updates to mp4-atom (0.10.0 → 0.11) and web-transport-iroh (0.4 → 0.5), plus a field initialization change in FMP4 single-moov segment construction; and (3) a systematic migration of TLS/PEM parsing across four modules (client, quiche, server, and general) from rustls_pemfile to rustls pki_types APIs, removing the rustls_pemfile dependency entirely. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`cargo deny check --show-stats` already prints the notes count; the extra reminder block was more noise than signal.
When CodeRabbit's hourly quota or org credits are exhausted, it posts a "review limit reached" comment instead of an actual review. Document running `/review` locally as the fallback so PRs don't sit unreviewed waiting for the quota to refill.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-native/src/client.rs (1)
135-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad every certificate from each root PEM file.
Line 135 now takes only the first certificate from each
--tls-rootfile. That silently drops the rest of a CA bundle and can break custom trust stores. It also differs fromrs/moq-native/src/server.rsLine 82 andrs/moq-native/src/tls.rsLine 112, which both collect every PEM certificate.Suggested fix
- let cert = CertificateDer::pem_reader_iter(&mut reader) - .next() - .context("no roots found")? - .context("failed to read root cert")?; - roots.add(cert).context("failed to add root cert")?; + let certs: Vec<CertificateDer<'static>> = CertificateDer::pem_reader_iter(&mut reader) + .collect::<Result<_, _>>() + .context("failed to read root cert")?; + anyhow::ensure!(!certs.is_empty(), "no roots found"); + for cert in certs { + roots.add(cert).context("failed to add root cert")?; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-native/src/client.rs` around lines 135 - 139, The code only reads the first certificate from CertificateDer::pem_reader_iter(&mut reader) and drops the rest of the PEM file; change it to iterate over all certificates returned by CertificateDer::pem_reader_iter, adding each parsed cert to roots (using roots.add(cert) for every item) and propagate/context errors per-certificate (e.g., "no roots found" only if iterator yields nothing, and "failed to read root cert"/"failed to add root cert" for individual certs) so the entire CA bundle is loaded rather than only the first cert.
🧹 Nitpick comments (1)
deny.toml (1)
55-55: ⚡ Quick winConsider stricter enforcement for duplicate versions.
multiple-versions = "warn"permits duplicate dependency versions without blocking the build. Setting it to"deny"would enforce a cleaner dependency tree and catch version conflicts earlier.📦 Stricter duplicate-version enforcement
-multiple-versions = "warn" +multiple-versions = "deny"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deny.toml` at line 55, Update the deny configuration to reject duplicate dependency versions by changing the "multiple-versions" setting from "warn" to "deny" in the deny.toml; locate the "multiple-versions" key and set its value to "deny" so the build fails on duplicate versions instead of only warning, ensuring stricter enforcement of dependency version conflicts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rs/moq-native/src/client.rs`:
- Around line 135-139: The code only reads the first certificate from
CertificateDer::pem_reader_iter(&mut reader) and drops the rest of the PEM file;
change it to iterate over all certificates returned by
CertificateDer::pem_reader_iter, adding each parsed cert to roots (using
roots.add(cert) for every item) and propagate/context errors per-certificate
(e.g., "no roots found" only if iterator yields nothing, and "failed to read
root cert"/"failed to add root cert" for individual certs) so the entire CA
bundle is loaded rather than only the first cert.
---
Nitpick comments:
In `@deny.toml`:
- Line 55: Update the deny configuration to reject duplicate dependency versions
by changing the "multiple-versions" setting from "warn" to "deny" in the
deny.toml; locate the "multiple-versions" key and set its value to "deny" so the
build fails on duplicate versions instead of only warning, ensuring stricter
enforcement of dependency version conflicts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35441a19-0104-471e-b327-0edacc9450d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
CLAUDE.mdCargo.tomldeny.tomlflake.nixrs/justfilers/moq-mux/Cargo.tomlrs/moq-mux/src/container/fmp4/import.rsrs/moq-native/Cargo.tomlrs/moq-native/src/client.rsrs/moq-native/src/quiche.rsrs/moq-native/src/server.rsrs/moq-native/src/tls.rs
💤 Files with no reviewable changes (1)
- rs/moq-native/Cargo.toml
The client root cert loader was discarding every certificate after the first one in each PEM file, so a multi-cert CA bundle would silently lose its other entries. Bug predates this branch but the PemObject migration left it untouched; aligning now with the collect-all pattern already used in server.rs and tls.rs.
`cargo deny` (added in #1486) flags audiopus_sys as unmaintained: last commit 5 years ago, maintainer unresponsive to PRs fixing the exact CMake 4 issue we already work around with CMAKE_POLICY_VERSION_MINIMUM=3.5. There's no safe upgrade and the candidate replacements (`magnum-opus`/`opusic-sys`) all wrap the same vendored libopus, so swapping doesn't fix the underlying problem. Add the advisory to deny.toml's ignore list with rationale. Drop when libopus's CMakeLists gets updated upstream or a maintained opus binding emerges. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
cargo-denyto the Nix dev shell and wirescargo deny checkintojust rs check(andjust rs ci) for license / advisory / source / ban policy enforcement on every PR.just rs deny-reminderrecipe that prints active advisory ignores with their inline rationale after each run, so deferred items don't get forgotten. cargo-deny's native ignored-advisory notes only surface at--log-level=info, which dumps full dep trees and is too noisy for CI.Findings resolved
web-transport-iroh = "0.5", pulls hickory 0.26.1moq-nativetorustls::pki_types::pem::PemObject(the upstream-recommended replacement); 5 call sites acrossclient.rs,server.rs,tls.rs,quiche.rsmp4-atom = "0.11"(usespastey); small API fix for the newMoov.ainffieldFindings ignored (with rationale)
Each has an inline comment in
deny.tomlnaming the upstream blocker so it's clear when the ignore can be removed:rsacrate is unreachable for us:moq-tokenuses it only for keygen + JWK component serialization, while signing runs throughjsonwebtokenbacked byaws-lc-rs(constant time). Vulnerable code path is not exercised.Test plan
cargo deny checkreportsadvisories ok, bans ok, licenses ok, sources okcargo check --workspace --all-features(sansmoq-gst, which needs GStreamer system deps) builds cleanlycargo test -p moq-native --all-features— 92 tests passcargo test -p moq-mux -p moq-token— 240 tests passcargo fmt --all --check,cargo shear,cargo sort --workspace --checknix develop --command just rs ciin CI(Written by Claude)