feat(rpc): add IP whitelist to bypass rate limiting - #4847
Open
lushengguo wants to merge 4 commits into
Open
Conversation
- Add ip_whitelist config to ApiQuotaConfiguration with CLI flag --jsonrpc-ratelimit-ip-whitelist - Implement whitelist bypass check in rate limit middleware (call, notification, and batch) - Add startup log for whitelist config, debug log on bypass, warn log on rate limit rejection, debug log for HTTP metadata IP extraction - Add 3 unit tests for whitelist (call, notification, non-whitelisted)
📝 WalkthroughWalkthroughAdds an optional IP whitelist to RPC quota configuration and threads it into the JSON-RPC rate-limiting middleware; requests whose extracted metadata.user matches a whitelisted IP bypass rate checks. Logging and unit tests were updated to cover whitelist behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HttpMetadata as HttpMetadataService
participant RateLimit as JsonApiRateLimitMiddleware
participant Config as ApiQuotaConfiguration
participant Inner as InnerService
Client->>HttpMetadata: HTTP RPC Request
HttpMetadata->>HttpMetadata: extract user/IP into Metadata
HttpMetadata->>RateLimit: forward request + Metadata
RateLimit->>Config: access ip_whitelist()
alt Metadata.user in ip_whitelist
RateLimit->>RateLimit: bypass limiter (debug)
RateLimit->>Inner: forward request
Inner->>RateLimit: response
RateLimit->>Client: return response
else Metadata.user not in ip_whitelist
RateLimit->>RateLimit: run limiters.check(...) (warn on limit)
alt allowed
RateLimit->>Inner: forward request
Inner->>RateLimit: response
RateLimit->>Client: return response
else limited
RateLimit->>Client: return rate-limit error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rpc/server/src/rate_limit_middleware.rs (1)
348-401: Add whitelist coverage for the batch bypass paths.The new logic in
batch()has separate whitelist branches forBatchEntry::CallandBatchEntry::Notification, but the added tests only exercise single-call and single-notification flows. A regression in either batch whitelist path would still pass this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/server/src/rate_limit_middleware.rs` around lines 348 - 401, Add tests that exercise the batch() whitelist branches for both BatchEntry::Call and BatchEntry::Notification: create a middleware via test_middleware_with_whitelist(...) with "10.0.0.1" whitelisted, then call middleware.batch(...) with a Batch containing two BatchEntry::Call entries (each built with request_with_user("state.get","10.0.0.1")) and assert neither entry is rate-limited; likewise add a batch test using two BatchEntry::Notification entries (or mixed entries) where each Notification has Metadata.user = Some("10.0.0.1") and assert both succeed and middleware.service.notifications increments appropriately; reference the batch() method, BatchEntry::Call, BatchEntry::Notification, test_middleware_with_whitelist, request_with_user, Notification, and Metadata to locate spots to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/src/rpc_config.rs`:
- Around line 341-347: The help text for the clap option ip_whitelist (flag name
"jsonrpc-ratelimit-ip-whitelist") is misleading because
JsonApiRateLimitMiddleware checks Metadata.user and the peer IP is only
populated into metadata by the middleware in
rpc/server/src/metadata_middleware.rs when trust_forwarded_ip_headers is
enabled; update either the behavior or the docs: either modify
metadata_middleware.rs to always source the peer IP into Metadata (e.g., set
Metadata.user or a dedicated Metadata.peer_ip from the incoming connection when
available so JsonApiRateLimitMiddleware can match without requiring
trust_forwarded_ip_headers) or change the clap help string for ip_whitelist to
clearly state that IPs only bypass rate limiting when forwarded-IP extraction is
enabled (trust_forwarded_ip_headers=true) and/or when Metadata.user contains the
peer IP; reference JsonApiRateLimitMiddleware and trust_forwarded_ip_headers in
the updated help text or the metadata population change.
- Around line 395-403: In RpcConfig::merge_with_opt the current logic unions
o.ip_whitelist with self.ip_whitelist (using ip_whitelist, o.ip_whitelist),
which breaks precedence and prevents revoking inherited IPs; change the behavior
so that when o.ip_whitelist.is_some() you replace self.ip_whitelist with
o.ip_whitelist.clone() (i.e., assign the incoming Some(...) directly) instead of
extending/merging; if you need additive semantics, add an explicit additive
flag/mode to RpcConfig and only perform the union when that flag is set.
In `@rpc/server/src/metadata_middleware.rs`:
- Line 67: The new debug! invocation in metadata_middleware.rs (the debug! call
that logs user and self.trust_forwarded_ip_headers) is not formatted to
rustfmt's expected multi-line style; run rustfmt (cargo fmt) or reflow the macro
so the format string and its arguments are split across lines (put the format
string on its own line and each argument on its own indented line) to satisfy
rustfmt and CI.
In `@rpc/server/src/rate_limit_middleware.rs`:
- Around line 58-63: The current logs print raw allowlist entries and caller IPs
(e.g., the ip_whitelist from quotas.ip_whitelist() and the various
info!/warn!/debug! calls), which leaks network identifiers; replace those prints
with non-identifying summaries: log only counts (e.g., ip_whitelist.len()) and,
when an identifier is needed, log a redacted form or stable hash (e.g., SHA256
or masked IP) instead of the raw string; gate any full-list or full-IP output
behind a dedicated diagnostic flag/feature or trace-level logging so normal logs
never contain raw addresses; update the initialization log that currently prints
ip_whitelist, and all reject/warning logs that emit caller IPs (the logging
sites referenced around lines 133-146, 162-172, 185-211) to use the count or
hashed/redacted identifier and switch to a stricter log level for full details.
---
Nitpick comments:
In `@rpc/server/src/rate_limit_middleware.rs`:
- Around line 348-401: Add tests that exercise the batch() whitelist branches
for both BatchEntry::Call and BatchEntry::Notification: create a middleware via
test_middleware_with_whitelist(...) with "10.0.0.1" whitelisted, then call
middleware.batch(...) with a Batch containing two BatchEntry::Call entries (each
built with request_with_user("state.get","10.0.0.1")) and assert neither entry
is rate-limited; likewise add a batch test using two BatchEntry::Notification
entries (or mixed entries) where each Notification has Metadata.user =
Some("10.0.0.1") and assert both succeed and middleware.service.notifications
increments appropriately; reference the batch() method, BatchEntry::Call,
BatchEntry::Notification, test_middleware_with_whitelist, request_with_user,
Notification, and Metadata to locate spots to add these tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74a91d9b-8f79-488e-9524-a25cfa481425
📒 Files selected for processing (3)
config/src/rpc_config.rsrpc/server/src/metadata_middleware.rsrpc/server/src/rate_limit_middleware.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rpc/server/src/rate_limit_middleware.rs (1)
58-66:⚠️ Potential issue | 🟠 MajorAvoid logging raw whitelist entries and caller IPs.
Line 63, Line 140, Line 150, and related logs still emit full IP/user identifiers. Please log counts and/or redacted identifiers instead of raw values.
Suggested direction
- info!("RPC rate limit middleware initialized with IP whitelist: {:?}", ip_whitelist); + info!( + "RPC rate limit middleware initialized with ip_whitelist_count={}", + ip_whitelist.len() + ); - warn!("Rate limited: method={}, user={:?}, reason={}", method, user, e); + warn!( + "Rate limited: method={}, user_masked={}, reason={}", + method, + mask_user(user.as_deref()), + e + );Also applies to: 139-142, 149-152, 174-176, 184-186, 205-207, 214-216, 229-229, 236-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/server/src/rate_limit_middleware.rs` around lines 58 - 66, The logs currently emitting full IPs/identifiers must be changed to avoid sensitive data exposure: replace any logging that prints the ip_whitelist contents with a count (e.g., ip_whitelist.len()) and replace logs that print caller IPs or user IDs (search for variables like caller_ip, remote_addr, user_id, and ip_whitelist) to use a redaction helper (create a simple redact_identifier/redact_ip function that masks all but the last octet/characters) and log the redacted value instead; update every info/debug/error call in this module that currently interpolates ip_whitelist or raw caller identifiers so they emit either counts or the redacted identifier plus minimal context.
🧹 Nitpick comments (2)
rpc/server/src/rate_limit_middleware.rs (2)
137-145: Consolidate whitelist checks/logging into one helper.The whitelist predicate and bypass logging are duplicated across call/notification/batch paths, which increases drift risk.
Refactor sketch
+fn is_whitelisted(ip_whitelist: &HashSet<String>, user: &Option<String>) -> bool { + user.as_ref().is_some_and(|ip| ip_whitelist.contains(ip)) +}Then reuse this helper in all three RPC paths.
Also applies to: 171-179, 200-209, 225-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/server/src/rate_limit_middleware.rs` around lines 137 - 145, Create a small helper function (e.g., is_ip_whitelisted_and_bypass) that takes the Option<String>/user, ip_whitelist, method, and a closure to invoke the service path; the helper should check the user/IP against ip_whitelist, emit the same debug log ("IP {} is whitelisted, bypassing rate limit for method={}", ip, method) and immediately call/await the provided closure to return the response when whitelisted, otherwise fall through to normal rate-limiting; replace the duplicated blocks in the call, notification, and batch paths (the snippets using if let Some(ref ip) = user { if ip_whitelist.contains(ip) { debug!(...); return service.call(request).await; } }) with calls to this helper so all whitelist checking and logging is consolidated.
376-426: Add batch whitelist tests to match new batch bypass logic.You added whitelist behavior in batch paths (Line 197-243), but tests currently only cover call/notification and non-whitelisted call. Please add whitelisted batch call/notification coverage (and ideally mixed-entry batch behavior).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/server/src/rate_limit_middleware.rs` around lines 376 - 426, Add tests that exercise the new batch whitelist bypass: create tests named like whitelisted_ip_bypasses_batch_rate_limit and whitelisted_ip_bypasses_notification_batch_rate_limit using test_middleware_with_whitelist("state.get", vec!["10.0.0.1".to_string()]) and construct batch requests/notifications that include multiple entries (use request_with_user for per-entry requests and build Notification objects with Metadata.user set to "10.0.0.1"). Call middleware.batch_call(...) and middleware.batch_notification(...) (or the crate's batch equivalents) twice and assert that whitelisted entries are not rate limited (responses are not error -10000 and middleware.service.calls/notifications counters increase for all whitelisted entries). Also add a mixed-entry batch test where one entry is whitelisted and one is not, asserting the whitelisted entry succeeds and the non-whitelisted entry is rate limited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rpc/server/src/rate_limit_middleware.rs`:
- Around line 58-66: The logs currently emitting full IPs/identifiers must be
changed to avoid sensitive data exposure: replace any logging that prints the
ip_whitelist contents with a count (e.g., ip_whitelist.len()) and replace logs
that print caller IPs or user IDs (search for variables like caller_ip,
remote_addr, user_id, and ip_whitelist) to use a redaction helper (create a
simple redact_identifier/redact_ip function that masks all but the last
octet/characters) and log the redacted value instead; update every
info/debug/error call in this module that currently interpolates ip_whitelist or
raw caller identifiers so they emit either counts or the redacted identifier
plus minimal context.
---
Nitpick comments:
In `@rpc/server/src/rate_limit_middleware.rs`:
- Around line 137-145: Create a small helper function (e.g.,
is_ip_whitelisted_and_bypass) that takes the Option<String>/user, ip_whitelist,
method, and a closure to invoke the service path; the helper should check the
user/IP against ip_whitelist, emit the same debug log ("IP {} is whitelisted,
bypassing rate limit for method={}", ip, method) and immediately call/await the
provided closure to return the response when whitelisted, otherwise fall through
to normal rate-limiting; replace the duplicated blocks in the call,
notification, and batch paths (the snippets using if let Some(ref ip) = user {
if ip_whitelist.contains(ip) { debug!(...); return service.call(request).await;
} }) with calls to this helper so all whitelist checking and logging is
consolidated.
- Around line 376-426: Add tests that exercise the new batch whitelist bypass:
create tests named like whitelisted_ip_bypasses_batch_rate_limit and
whitelisted_ip_bypasses_notification_batch_rate_limit using
test_middleware_with_whitelist("state.get", vec!["10.0.0.1".to_string()]) and
construct batch requests/notifications that include multiple entries (use
request_with_user for per-entry requests and build Notification objects with
Metadata.user set to "10.0.0.1"). Call middleware.batch_call(...) and
middleware.batch_notification(...) (or the crate's batch equivalents) twice and
assert that whitelisted entries are not rate limited (responses are not error
-10000 and middleware.service.calls/notifications counters increase for all
whitelisted entries). Also add a mixed-entry batch test where one entry is
whitelisted and one is not, asserting the whitelisted entry succeeds and the
non-whitelisted entry is rate limited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14ccef6f-8b0f-4813-acb9-ceef0b5d2ecb
📒 Files selected for processing (2)
rpc/server/src/metadata_middleware.rsrpc/server/src/rate_limit_middleware.rs
✅ Files skipped from review due to trivial changes (1)
- rpc/server/src/metadata_middleware.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rpc/server/src/rate_limit_middleware.rs (1)
363-449: Add one mixed-caller assertion to prove bypassed requests do not consume quota.These tests only show that whitelisted traffic succeeds. They don't catch a regression where the request is still counted by
limiters.check(...)and only exempted from the error path. A stronger check is: send one whitelisted request first, then verify the first non-whitelisted request is still allowed.Example test to add
+ #[test] + fn whitelisted_call_does_not_consume_quota() { + let middleware = + test_middleware_with_whitelist("state.get", vec!["10.0.0.1".to_string()]); + + let _ = futures::executor::block_on( + middleware.call(request_with_user("state.get", "10.0.0.1")), + ); + + let rsp = futures::executor::block_on( + middleware.call(request_with_user("state.get", "192.168.1.1")), + ); + assert_ne!(rsp.as_error_code(), Some(-10000)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc/server/src/rate_limit_middleware.rs` around lines 363 - 449, The tests only assert whitelisted callers succeed but don't prove they don't consume quota; update the rate-limit test(s) using test_middleware_with_whitelist and middleware.call (and request_with_user) to add a mixed-caller check: send one request from the whitelisted IP first, then send a request from a non-whitelisted IP and assert the non-whitelisted call is still allowed (use rsp.as_error_code() != Some(-10000)); this proves whitelisted requests do not decrement the limiter so non-whitelisted callers remain unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rpc/server/src/rate_limit_middleware.rs`:
- Around line 39-40: The ip_whitelist currently holds raw Strings which compare
verbatim against Metadata.user (populated via IpAddr::to_string()), so
equivalent IPv6 forms or malformed entries miss or silently fail; when
building/storing ip_whitelist (the ip_whitelist field in your rate limit
middleware/constructor), parse each configured entry with
std::net::IpAddr::from_str (or equivalent), convert back to the canonical
IpAddr::to_string() (or better: store HashSet<IpAddr> instead of strings) and
insert that canonical form into ip_whitelist; skip and log/warn on parse errors
so malformed entries are not silently ignored; update any lookup logic that
compares to Metadata.user to use the same canonical representation (or compare
as IpAddr).
---
Nitpick comments:
In `@rpc/server/src/rate_limit_middleware.rs`:
- Around line 363-449: The tests only assert whitelisted callers succeed but
don't prove they don't consume quota; update the rate-limit test(s) using
test_middleware_with_whitelist and middleware.call (and request_with_user) to
add a mixed-caller check: send one request from the whitelisted IP first, then
send a request from a non-whitelisted IP and assert the non-whitelisted call is
still allowed (use rsp.as_error_code() != Some(-10000)); this proves whitelisted
requests do not decrement the limiter so non-whitelisted callers remain
unaffected.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 928d2434-f576-4e89-8fc8-4ac48f2e5426
📒 Files selected for processing (2)
config/src/rpc_config.rsrpc/server/src/rate_limit_middleware.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- config/src/rpc_config.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit