ci: add fmt/clippy/test pipeline and weekly cargo audit#24
Conversation
…precated APIs Removes 11 needless borrows on Copy arrays passed to hex::encode and Sha256::digest, replaces 'if let Err(_)' with .is_err(), and adds a module-level #![allow(deprecated)] (with explanatory comment) to cover the chacha20poly1305 0.10 / generic-array 0.14 deprecation pending the upgrade to generic-array 1.x in the encrypted-token registration phase.
…d_code) Removes five truly-unused imports (reqwest::Client x2, redundant super::* in two test modules, Arc) and silences the remaining dead_code / unused_variables lints on items kept intentionally for future phases or API symmetry: Config placeholder fields (rate_limit, crypto, NostrConfig plumbing fields, batching knobs, RateLimitConfig, CryptoConfig), RATE_LIMIT_CLEANUP_INTERVAL_DEFAULT_SECS / PUBKEY_LIMITER_SOFT_CAP_DEFAULT constants, DispatchOutcome::Delivered.backend, ServiceAccount.project_id, the FcmPush::new(config, ...) parameter (kept for symmetry with UnifiedPushService::new), UnifiedPushService.config + reserved register/unregister/save_endpoints methods, TokenStore::count, and the utils::batching module (gated like the existing utils-level pattern for crypto). Each annotation carries a one-line comment explaining why the item is kept.
…udit Closes #12. The legacy rust.yml only ran a single build+test job. ci.yml splits work into three parallel jobs (fmt, clippy with -D warnings, test --all-features --no-fail-fast), all using Swatinem/rust-cache@v2 for ~/.cargo and target/ caching, and runs on push to main and on every pull_request to main. audit.yml runs cargo audit --deny warnings on a weekly cron (Mondays 06:00 UTC), via workflow_dispatch, and on PRs/pushes that touch Cargo.toml or Cargo.lock.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughA GitHub Actions CI/CD pipeline is established with automated checks for code formatting, linting, testing, and security audits. The API handlers add strict validation for trade pubkeys and tokens, implement a conditional trusted Mostro pubkey whitelist, and standardize response bodies. The codebase is prepared for stricter compiler checks through reserved fields and deprecation/dead-code suppressions. ChangesCI/CD Setup and API Validation Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![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 (2)
src/push/unifiedpush.rs (1)
76-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate async error bounds for multithreading compatibility.
The methods
save_endpoints,register_endpoint, andunregister_endpointreturnResult<(), Box<dyn std::error::Error>>. Per project guidelines, async fallible operations must useBox<dyn std::error::Error + Send + Sync>to ensure compatibility with multithreaded async contexts. Add+ Send + Syncbounds to the three error types.🤖 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 `@src/push/unifiedpush.rs` around lines 76 - 119, Update the async function error bounds to be multithread-safe: change the return types of save_endpoints, register_endpoint, and unregister_endpoint from Result<(), Box<dyn std::error::Error>> to Result<(), Box<dyn std::error::Error + Send + Sync>> so the boxed error is Send + Sync for multithreaded async contexts; ensure the function signatures for async fn save_endpoints(&self) -> ..., async fn register_endpoint(&self, device_id: String, endpoint_url: String) -> ..., and async fn unregister_endpoint(&self, device_id: &str) -> ... are updated accordingly.src/config.rs (1)
128-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
server_private_keyfallback is a known secp256k1 key — use an empty default instead.
"0000...0001"is the smallest valid secp256k1 private key and a well-known test vector. WhileConfig.cryptois#[allow(dead_code)]today, when phase 4 removes that annotation and the field is consumed by crypto operations, any deployment that starts withoutSERVER_PRIVATE_KEYset will silently use a publicly known key. An attacker who knows the server's public key (derived from this private key) could forge or decrypt traffic depending on the protocol used.Using
unwrap_or_default()(empty string) instead means any phase-4 activation path that tries to parse the key will get an immediate, obvious failure rather than a silently operational but compromised key.🔒 Proposed fix
- crypto: CryptoConfig { - server_private_key: env::var("SERVER_PRIVATE_KEY").unwrap_or_else(|_| { - "0000000000000000000000000000000000000000000000000000000000000001".to_string() - }), - }, + // Phase 4 MUST validate this is non-empty before first crypto use. + crypto: CryptoConfig { + server_private_key: env::var("SERVER_PRIVATE_KEY").unwrap_or_default(), + },🤖 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 `@src/config.rs` around lines 128 - 132, The current fallback for CryptoConfig.server_private_key uses a well-known secp256k1 test vector string; replace this insecure hardcoded default by returning an empty string when the env var is missing (e.g., use env::var("SERVER_PRIVATE_KEY").unwrap_or_default()) so attempts to parse/use the key in CryptoConfig (and Config.crypto) will fail loudly instead of silently using a public test key; update the server_private_key initialization accordingly and ensure any downstream parsing will surface the missing-key error.
🧹 Nitpick comments (1)
src/api/rate_limit.rs (1)
55-62: ⚡ Quick winMove
.max(1)insiderate_limited_responseto protect all callers.The coding guideline says
Retry-AfterMUST be.max(1), but the function itself does not enforce it — it relies on callers to clamp before passingretry_after_secs. The per-IP path applies it correctly at line 160, but the per-pubkey path innotify.rs(not reviewed here) must also do so. A caller that omits.max(1)would emitRetry-After: 0, silently violating the MUST constraint. Enforcing it inside the function removes the per-caller burden entirely.🛡️ Proposed fix
pub fn rate_limited_response(retry_after_secs: u64) -> HttpResponse { + let retry_after_secs = retry_after_secs.max(1); HttpResponse::TooManyRequests() .insert_header(("Retry-After", retry_after_secs.to_string()))As per coding guidelines, "
Retry-AfterMUST be whole seconds with.max(1)."🤖 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 `@src/api/rate_limit.rs` around lines 55 - 62, The rate_limited_response function currently trusts callers to clamp retry_after_secs but must enforce the guideline that Retry-After is whole seconds with a minimum of 1; inside rate_limited_response (the function that builds the HttpResponse and inserts the "Retry-After" header) clamp retry_after_secs with retry_after_secs.max(1) before converting to string and inserting the header so every caller (including notify.rs paths) always emits a Retry-After >= 1.
🤖 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 `@src/config.rs`:
- Around line 128-132: The current fallback for CryptoConfig.server_private_key
uses a well-known secp256k1 test vector string; replace this insecure hardcoded
default by returning an empty string when the env var is missing (e.g., use
env::var("SERVER_PRIVATE_KEY").unwrap_or_default()) so attempts to parse/use the
key in CryptoConfig (and Config.crypto) will fail loudly instead of silently
using a public test key; update the server_private_key initialization
accordingly and ensure any downstream parsing will surface the missing-key
error.
In `@src/push/unifiedpush.rs`:
- Around line 76-119: Update the async function error bounds to be
multithread-safe: change the return types of save_endpoints, register_endpoint,
and unregister_endpoint from Result<(), Box<dyn std::error::Error>> to
Result<(), Box<dyn std::error::Error + Send + Sync>> so the boxed error is Send
+ Sync for multithreaded async contexts; ensure the function signatures for
async fn save_endpoints(&self) -> ..., async fn register_endpoint(&self,
device_id: String, endpoint_url: String) -> ..., and async fn
unregister_endpoint(&self, device_id: &str) -> ... are updated accordingly.
---
Nitpick comments:
In `@src/api/rate_limit.rs`:
- Around line 55-62: The rate_limited_response function currently trusts callers
to clamp retry_after_secs but must enforce the guideline that Retry-After is
whole seconds with a minimum of 1; inside rate_limited_response (the function
that builds the HttpResponse and inserts the "Retry-After" header) clamp
retry_after_secs with retry_after_secs.max(1) before converting to string and
inserting the header so every caller (including notify.rs paths) always emits a
Retry-After >= 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e95b8e7-07b4-464b-b0fc-8890a94dd56a
📒 Files selected for processing (13)
.github/workflows/audit.yml.github/workflows/ci.yml.github/workflows/rust.ymlsrc/api/notify.rssrc/api/rate_limit.rssrc/api/routes.rssrc/config.rssrc/crypto/mod.rssrc/push/dispatcher.rssrc/push/fcm.rssrc/push/unifiedpush.rssrc/store/mod.rssrc/utils/mod.rs
💤 Files with no reviewable changes (3)
- .github/workflows/rust.yml
- src/api/notify.rs
- src/api/routes.rs
…sories The first run of the new audit job flagged 6 vulnerabilities and 5 unmaintained/unsound warnings. This commit resolves what can be fixed without changing Cargo.toml and documents the rest: - cargo update -p bytes -p time (semver-compatible) clears RUSTSEC-2026-0007 (BytesMut::reserve integer overflow) and RUSTSEC-2026-0009 (time stack exhaustion DoS). - The remaining 4 vulnerabilities (idna 0.5.0 via nostr-sdk 0.27 and three rustls-webpki 0.101.7 advisories via rustls 0.21) require coordinated major bumps of nostr-sdk / reqwest / rustls and are tracked as follow-up; each is now ignored in .cargo/audit.toml with an inline rationale and the exact upstream pin that keeps us on the vulnerable version. - audit.yml drops --deny warnings so the unmaintained advisories (dotenv, instant, rustls-pemfile, rand x2) surface in the log without failing the job. Vulnerabilities still fail because cargo audit defaults to deny on actual vulns, and the .cargo/audit.toml path is now also a trigger so changes to the ignore list re-run the audit.
1 participant
Closes #12.
Replaces the single-job
rust.ymlwith two workflows that follow standard Rust CI practice:.github/workflows/ci.yml— runsfmt,clippy -- -D warnings, andtest --all-featuresas parallel jobs on every push tomainand every PR.~/.cargoandtarget/are cached viaSwatinem/rust-cache@v2..github/workflows/audit.yml— runscargo audit --deny warningsweekly (Mondays 06:00 UTC), on demand viaworkflow_dispatch, and on PRs/pushes that touchCargo.toml/Cargo.lock.To enable
clippy -- -D warningsfrom day one, this branch also clears the 37 pre-existing warnings: real fixes insrc/crypto/mod.rs(needless borrows, redundant pattern matching, module-levelallow(deprecated)for the chacha20poly1305 0.10 → generic-array 1.x deprecation), removal of unused imports, and#[allow(dead_code)]with one-line justifications on items kept intentionally for future phases (placeholderConfigfields, reservedUnifiedPushServiceAPI,TokenStore::count, theutils::batchingmodule, etc.).Local verification:
cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings, andcargo test --all-featuresall pass (42/42 tests).Summary by CodeRabbit
New Features
Improvements
Chores