fix(faucet): close per-recipient cooldown TOCTOU bypass via Entry guard#760
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR fixes a race condition in the faucet's per-recipient rate limiting. Previously, two concurrent requests for the same recipient could both observe no prior entry in the cooldown map and both pass the check. The fix uses DashMap's Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
check_rate_limitsin `bin/sentrix-faucet/src/main.rs` had a TOCTOU race on the per-recipient cooldown check. The old code:```rust
if let Some(last) = state.addr_last_drip.get(recipient)
&& now.duration_since(*last) < state.addr_cooldown
{
return Err(...);
}
state.addr_last_drip.insert(recipient.to_string(), now);
```
`DashMap::get` returns a `Ref` that releases the lock as soon as it drops. Between the read+check and the subsequent `insert`, another concurrent request for the same recipient address can interleave its own `get` and see the same "no prior drip" or "prior drip stale enough" outcome — both requests pass the check, both insert `now`, and both proceed to drip. Net effect: one address can receive multiple drips inside what was supposed to be a single cooldown window. The per-IP rate limit (above) already uses `entry()` which holds the per-key write lock for the whole closure, so it's atomic; the per-recipient block was the only async bypass.
Fix
Replace the get + insert pair with a `match entry()` block:
```rust
match state.addr_last_drip.entry(recipient.to_string()) {
Entry::Occupied(mut o) => {
let last = *o.get();
if now.duration_since(last) < state.addr_cooldown { return Err(...); }
o.insert(now);
}
Entry::Vacant(v) => { v.insert(now); }
}
```
`Entry` holds the per-key write lock for the entire check + update, so concurrent callers serialize cleanly. Same pattern as the per-IP block.
Test plan
A focused integration test that spawns N parallel drips against a mock RPC would be valuable but is out of scope for the minimal fix.
How I found it
Audit pass against all workspace crates one-by-one (operator request). `sentrix-faucet` was the 5th crate reviewed; this was the first real bug — earlier crates (`sentrix-proto`, `sentrix-precompiles`, `sentrix-rpc-types`, `sentrix-codec`) had only minor style notes.
Summary by CodeRabbit
Bug Fixes