feat: integrate reliakit (Secret + PositiveInt) in RPC auth, rate-limit, exporter#772
Conversation
|
Warning Review limit reached
More reviews will be available in 43 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces input validation and secret safety improvements across the sentrix codebase. Two new dependencies—reliakit-primitives (for PositiveInt validation) and reliakit-secret (for SecretString wrapping)—are added to enable type-safe configuration parsing. The SENTRIX_PROBE_INTERVAL_SEC environment variable is validated to reject zero and invalid values, falling back to 15 seconds. The API key configuration is wrapped in SecretString to prevent accidental logging or display. Rate limit configurations (global and write) are validated using a shared helper that rejects zero and unparseable inputs, with comprehensive unit tests ensuring correctness across edge cases. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/sentrix-prom-exporter/src/main.rs (1)
345-385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize the env-mutating test (or make parsing pure)
parse_targets()readsSENTRIX_TARGETSviastd::env::var, andparse_targets_all_scenariosmutates/removesSENTRIX_TARGETSinside anunsafeblock. Even though no other tests incrates/sentrix-prom-exporter/src/main.rstouchSENTRIX_TARGETS, theunsafecontract forstd::env::set_var/remove_varstill requires that no other thread may access the process environment concurrently—current “single-test scope” doesn’t enforce that under parallel test execution. Extract a pureparse_targets_from_str(&str)and test that, or guard env mutation with a global mutex / run that test serially.🤖 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 `@crates/sentrix-prom-exporter/src/main.rs` around lines 345 - 385, The test parse_targets_all_scenarios mutates the process environment unsafely by calling std::env::set_var/remove_var around parse_targets(), so fix by making parsing pure: extract a new function parse_targets_from_str(&str) that contains the parsing logic currently in parse_targets(), have parse_targets() call std::env::var and forward to parse_targets_from_str, and update parse_targets_all_scenarios to call parse_targets_from_str with literal strings for each scenario (or alternatively, if you must keep env mutation, serialize the test by acquiring a global Mutex/lock (e.g., a once_cell::sync::Lazy<Mutex<()>>) in the test before set_var/remove_var to prevent concurrent env access); update references to parse_targets() in tests to use parse_targets_from_str or wrap env changes with the mutex accordingly.
🤖 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.
Inline comments:
In `@crates/sentrix-rpc/src/routes/ratelimit.rs`:
- Around line 208-244: The tests mutate process-wide env unsafely; instead
extract the pure parsing/validation into a new function (e.g.,
rate_limit_from_str) which accepts an Option<&str> or &str and returns the
numeric rate or default so you can unit-test parsing logic without touching
std::env, then update rate_limit_from_env to call this helper; if you must keep
env-based tests, wrap them with a test-level serialization mechanism (a global
Mutex or the serial_test crate) so tests that call rate_limit_from_env do not
run in parallel and remove all unsafe std::env::set_var/remove_var calls inside
tests.
---
Outside diff comments:
In `@crates/sentrix-prom-exporter/src/main.rs`:
- Around line 345-385: The test parse_targets_all_scenarios mutates the process
environment unsafely by calling std::env::set_var/remove_var around
parse_targets(), so fix by making parsing pure: extract a new function
parse_targets_from_str(&str) that contains the parsing logic currently in
parse_targets(), have parse_targets() call std::env::var and forward to
parse_targets_from_str, and update parse_targets_all_scenarios to call
parse_targets_from_str with literal strings for each scenario (or alternatively,
if you must keep env mutation, serialize the test by acquiring a global
Mutex/lock (e.g., a once_cell::sync::Lazy<Mutex<()>>) in the test before
set_var/remove_var to prevent concurrent env access); update references to
parse_targets() in tests to use parse_targets_from_str or wrap env changes with
the mutex accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e2a9307-c081-4998-9a8f-c04818b9bedc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (5)
crates/sentrix-prom-exporter/Cargo.tomlcrates/sentrix-prom-exporter/src/main.rscrates/sentrix-rpc/Cargo.tomlcrates/sentrix-rpc/src/routes/auth.rscrates/sentrix-rpc/src/routes/ratelimit.rs
Three genuine type-safety wins from the reliakit crates (crates.io): - auth.rs: SENTRIX_API_KEY now wrapped in reliakit_secret::SecretString. Accidental Debug/Display of the key prints [REDACTED]; .expose_secret() is the only read path, so every access is auditable. Closes the accidental-key-leak-in-logs class. - ratelimit.rs: rate-limit env overrides parsed through PositiveInt. SENTRIX_GLOBAL_RATE_LIMIT=0 / SENTRIX_WRITE_RATE_LIMIT=0 previously capped the limiter at zero, silently locking every client out of the endpoint (self-DoS). 0/unparseable now falls back to the default. 4 unit tests cover the guard. - prom-exporter: SENTRIX_PROBE_INTERVAL_SEC parsed through PositiveInt. A 0 interval turned poll_loop into a busy-spin (from_secs(0) = no sleep) pinning a core and hammering every target. 0 now falls back to 15s. Deliberately NOT applied to consensus paths: BoundedStr uses chars().count() while the token-name/symbol validation in block_executor uses byte length (name.len()) plus a different empty-check — swapping it would change Pass-1 block-validity rules and fork the chain. Left untouched.
satyakwok
force-pushed
the
feat/reliakit-deep-integration
branch
from
6b9406f to
ec3b96a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CodeRabbit flagged the unit tests using unsafe std::env::set_var/remove_var. The finding is valid: under edition 2024 these are unsafe because the race is on the process-wide environment table, not key names — unique var names don't make parallel tests sound (another thread reading any env var, or stdlib doing a DNS lookup, can race the write → UB). Fix: extract pure rate_limit_from_str(Option<&str>, default) -> u32 and test that directly. rate_limit_from_env now just reads the env and delegates. Removes all unsafe from the tests; adds a clamp-not-truncate test for the u64 → u32 path.
1 participant
Deeper integration of the reliakit crates (crates.io) into non-consensus boundaries, after verifying each candidate against real code rather than the audit summary.
Shipped
reliakit-secret::SecretString— API key (routes/auth.rs)SENTRIX_API_KEYis now held inSecretString. Debug/Display prints[REDACTED];.expose_secret()is the only read path (used once, at the constant-time compare). Closes the accidental-key-in-logs class.reliakit-primitives::PositiveInt— rate-limit env (routes/ratelimit.rs)SENTRIX_GLOBAL_RATE_LIMIT=0/SENTRIX_WRITE_RATE_LIMIT=0previously capped the limiter at zero requests, silently locking every client out (self-DoS).0/unparseable now falls back to the default. 4 unit tests cover the guard.reliakit-primitives::PositiveInt— probe interval (prom-exporter)SENTRIX_PROBE_INTERVAL_SEC=0turnedpoll_loopinto a busy-spin (from_secs(0)= no sleep), pinning a core and hammering every target.0now falls back to 15s.Deliberately rejected (verified against real code)
BoundedStrfor token name/symbol (block_executor.rs):BoundedStrmeasureschars().count(); the consensus rule uses byte length (name.len()) plus a different empty-check. Swapping it changes Pass-1 block-validity → chain fork. Consensus rule changes need a fork gate; left untouched.BoundedVecfor RPC batch (jsonrpc/mod.rs): the current code checksarr.len()on the cheapValue::Arraybefore the typed deserialize. Wrapping inBoundedVec<JsonRpcRequest>would force the expensive deserialize first — a regression. Current code is already optimal.Test plan
cargo check --workspace -D warningscleancargo clippy -p sentrix-rpc -p sentrix-prom-exporter --all-targets -D warningscleanSummary by CodeRabbit
Bug Fixes
Security