Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughShield is rebuilt around profile-based rules, local GCRA shaping, optional asynchronous Redis reconciliation, JWT-aware keying and limit resolution, and separate pre-auth/post-auth middleware phases. Authentication now forwards validated claims through request extensions, with updated configuration, documentation, tests, and CI Redis provisioning. ChangesShield rate limiting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant pre_auth_middleware
participant Auth
participant post_auth_middleware
participant GcraStore
Client->>pre_auth_middleware: Send request
pre_auth_middleware->>GcraStore: Check pre-auth rule
pre_auth_middleware->>Auth: Forward admitted request
Auth->>post_auth_middleware: Provide ValidatedClaims
post_auth_middleware->>GcraStore: Check post-auth rule
post_auth_middleware->>Client: Return response or 429 headers
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
| Filename | Overview |
|---|---|
| src/shield/global.rs | Adds atomic Redis reconciliation, claimed deltas, epoch carryover, stale-estimate fallback, and guards for window changes. |
| src/shield/mod.rs | Adds two-phase enforcement, pluggable keys, profile resolution, and rate-limit response headers. |
| src/shield/store.rs | Adds the per-key local GCRA state store and stale-entry eviction. |
| src/shield/resolve.rs | Adds JWT and external-service limit resolution with background cache refresh. |
| src/lib.rs | Reorders middleware around authentication and applies CORS outside enforcement layers. |
| src/config.rs | Replaces the Shield schema with profiles, keyed rules, dynamic limit sources, and synchronization settings. |
Reviews (9): Last reviewed commit: "fix(shield): enforce limit-service cache..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 858992b114
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
527-569: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep Shield responses inside the CORS layer.
Both Shield layers now sit outside the existing CORS layer, so a short-circuited 429 lacks CORS headers. Successful responses also do not expose
RateLimit-*orRetry-Afterto browser JavaScript.Move CORS outside the enforcement layers and add these headers to
expose_headers.🤖 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/lib.rs` around lines 527 - 569, The router construction around post_auth_middleware and pre_auth_middleware currently places Shield responses outside CORS. Reorder the layers so CORS wraps both Shield enforcement phases, and update the existing CORS configuration to expose the RateLimit-* and Retry-After response headers to browser clients.
🤖 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 `@README.md`:
- Around line 181-225: Update the Rate limiting documentation to describe the
response-header contract: document the emitted rate-limit headers and
Retry-After on 429 responses, including their meanings and how clients should
use them for backoff. Place this near the existing rate-limiting behavior before
the Configuration reference.
- Around line 216-219: Update the “Sizing the overshoot” explanation so the
formula converts interval_ms from milliseconds into the same time unit used by
rate before multiplication, preserving the existing (N - 1) overshoot
relationship and default-interval guidance.
- Around line 189-195: Clarify the “Every key falls back” statement in the
“Keying and phases” documentation to state that fallback to client IP applies
only when the key’s enforcement phase runs. Specifically qualify that jwt_claim
fallback does not protect requests rejected before post-auth middleware, and
mention configuring a separate pre-auth IP/header rule for anonymous flood
protection.
In `@src/shield/gcra.rs`:
- Around line 87-96: Cap the computed admissible count in the GCRA admission
logic to the configured burst capacity before assigning or reporting remaining
requests, so an idle key cannot exceed its full bucket. Update the related
remaining calculation around the new admission path, and add a regression test
that exhausts a burst, advances past full refill, then verifies the next
admission reports remaining equal to burst minus one.
In `@src/shield/global.rs`:
- Around line 189-200: Make the delta publication flow in push_deltas and the
surrounding Phase 2 logic idempotent: ensure a successful Redis delta push is
recorded or committed independently before read_epochs can fail, so retries do
not repeat the same INCRBY contribution. Update the related handling in the
indicated aggregation paths while preserving epoch reads and existing failure
logging.
- Around line 103-129: Update the state handling in both gate and record to
detect when the resolved window_secs differs from the existing KeyState window.
Reset or namespace that entry before calling roll_to or applying counts,
ensuring reconciliation uses the current window while preserving existing
accounting for unchanged windows.
- Around line 164-175: Update the plan construction in the state iteration to
use the epoch recorded by each KeyState as the PushPlan epoch for its delta,
rather than unconditionally using the current epoch from window::epoch. Track
the source epoch separately from the epoch used when reading current windows,
preserving the existing delta calculation and ensuring previous-epoch usage is
pushed to its original Redis key.
In `@src/shield/matcher.rs`:
- Around line 92-123: Update compile_rules and the enforcement flow around
Shield::resolve_limit so IP- and header-keyed rules that configure jwt_limits
are also evaluated post-auth with claims available. Preserve any required
pre-auth flood-limit check, while ensuring the configured JWT tier or numeric
limits are reachable for every rule that defines them and retain existing
pre-auth behavior for rules without JWT limits.
- Around line 67-74: Validate the parsed rate and resolved burst in the
profile-building flow before constructing Gcra, rejecting zero values rather
than allowing Gcra::from_profile to normalize them to one. Preserve the
configured positive rate and burst unchanged when creating Profile, and return
an appropriate validation error for invalid zero values.
In `@src/shield/mod.rs`:
- Around line 218-235: Replace the separate global.gate and global.record calls
in the request flow with the fleet’s atomic reservation API, reserving the key
before shield.store.check. If the local GCRA verdict rejects the request, invoke
the reservation rollback for that key; otherwise retain the reservation as the
admission record, preserving the existing rejection responses and Redis feature
gating.
- Around line 262-273: Update rule_key to namespace generated keys with a stable
rule identifier or canonical hash derived from the rule, replacing the idx-based
prefix. Ensure all key-source branches use this stable namespace consistently so
configuration reordering and rolling deployments preserve the same budget while
distinct rules remain isolated.
- Around line 273-282: Update the accounting-key construction around the
KeySource::Header and KeySource::JwtClaim branches so raw header and claim
values are never embedded in Redis keys or limit-service queries. Replace each
value with an opaque keyed-hash representation using the established protected
hashing mechanism, while preserving the existing by_ip fallback when the value
is missing; use a separate protected lookup contract only if downstream identity
data is required.
- Around line 405-407: Update secs_ceil to round directly from the Duration’s
nanosecond representation rather than truncated milliseconds, so every nonzero
sub-second duration produces at least one second while preserving u64::MAX
saturation for conversion overflow.
In `@src/shield/resolve.rs`:
- Around line 133-135: Bound client-keyed state in both locations: in
src/shield/resolve.rs lines 133-135, configure the cached-resolution map with a
finite capacity and idle expiration; in src/shield/store.rs lines 78-84, add a
hard capacity and overflow policy alongside the existing lazy drained-key
sweeps. Use the existing cache/store APIs and preserve normal lookup behavior
while preventing unbounded growth.
In `@src/shield/store.rs`:
- Around line 54-66: Retain nanosecond precision in the GCRA accounting flow
around the store entry handling in src/shield/store.rs:54-66 by storing and
restoring TAT values without millisecond truncation. In the response timing
conversion in src/shield/mod.rs:405-407, derive whole-second headers directly
from nanoseconds and round upward so nonzero waits are not reported as zero.
---
Outside diff comments:
In `@src/lib.rs`:
- Around line 527-569: The router construction around post_auth_middleware and
pre_auth_middleware currently places Shield responses outside CORS. Reorder the
layers so CORS wraps both Shield enforcement phases, and update the existing
CORS configuration to expose the RateLimit-* and Retry-After response headers to
browser clients.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ace2bb7c-fcd2-4afa-8767-e668bf441bf6
📒 Files selected for processing (15)
.github/workflows/ci.ymlREADME.mdsrc/auth/forward.rssrc/auth/jwks.rssrc/auth/mod.rssrc/config.rssrc/lib.rssrc/shield/gcra.rssrc/shield/global.rssrc/shield/matcher.rssrc/shield/mod.rssrc/shield/resolve.rssrc/shield/store.rssrc/shield/tests.rssrc/shield/window.rs
Member
Author
|
@coderabbitai Re: CORS outside the Shield layers (src/lib.rs) — fixed. CORS is now applied as the outermost layer so it wraps both Shield enforcement phases and the auth layer: short-circuited 401/429/503 responses carry CORS headers, and preflight OPTIONS is answered before auth. RateLimit-Limit / -Remaining / -Reset and Retry-After were added to expose_headers. |
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@README.md`:
- Around line 190-207: Update the README guidance around phase selection and
limit-source resolution to state that enabling JWT-based limit resolution moves
any rule, including IP- or header-keyed rules, to the post-auth phase. Separate
key selection from JWT-derived rate resolution, and remove the claim that JWT
resolution applies only to jwt_claim rules while preserving the documented
fallback and per-phase behavior.
In `@src/shield/global.rs`:
- Around line 105-115: Replace the separate GlobalState::gate and
GlobalState::record flow with an atomic reservation operation that checks and
increments the per-key local reservation under the same synchronization. Have
the caller release that reservation whenever the local GCRA check rejects, while
preserving the existing budget and window semantics so concurrent requests
cannot admit more than the fleet budget.
In `@src/shield/resolve.rs`:
- Around line 190-192: Update resolve() to record each key’s most recent access
independently of Cached.at, including accesses served from stale cached entries.
Change the eviction logic around self.cache.retain to compare against this
access timestamp so actively used stale keys are retained while genuinely idle
keys are evicted; keep Cached.at for refresh-age semantics.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e29c21bd-08c3-40d6-9d7b-63cfa1be5a4a
📒 Files selected for processing (9)
README.mdsrc/lib.rssrc/shield/gcra.rssrc/shield/global.rssrc/shield/matcher.rssrc/shield/mod.rssrc/shield/resolve.rssrc/shield/store.rssrc/shield/tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07b7b134b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace the fixed-window limiter with an embedded, config-driven GCRA shaper that adds no blocking latency to the request path. - Local GCRA shaper (one timestamp per key): bursts up to `burst`, then throttles to `rate`, with no fixed-window boundary burst. - Keys: client IP, a header value, or a validated JWT claim, each with an IP fallback. Two-phase: IP/header rules run before auth (shed floods pre-verification), claim-keyed rules run after auth using the verified, un-forgeable principal. Phase is derived from the key. - Limit resolution chain: JWT claim (tier name to a profile, or explicit numbers), external service (cached and refreshed off the request path), rule profile, then default. Tiers are named config data. - Optional async cross-instance reconciliation over a shared store (feature `redis`): delta push / aggregate pull on an interval with a sliding-window counter, converging on an approximate fleet-wide limit. A store outage degrades to per-instance limiting, never failing requests. - Emit draft-ietf RateLimit-Limit / -Remaining / -Reset headers, plus Retry-After on 429. Auth now attaches the verified claims as a typed request extension for the post-auth phase. CI gains a Redis service for the reconciliation test. BREAKING CHANGE: the `shield` config schema is replaced. `endpoint_classes`, `identifier_endpoints`, and `window_secs` are removed in favour of `profiles`, `rules` (pattern + key + profile), `default_profile`, `jwt_limits`, `limit_service`, and `sync`. Closes #68
An idle key (TAT far in the past) reports a slack-derived remaining far above the bucket capacity. The test exercises drain-then-admit and expects remaining == burst - 1; it fails on current code.
An idle key whose TAT sits far in the past produced a slack-derived admissible count well above burst, so RateLimit-Remaining over-reported the available budget (admission itself was already correct). Cap the count at burst so remaining never exceeds a full bucket.
Three reconciliation bugs in the shared-store sync: - Push each pending delta with the epoch it was accumulated in, not the current epoch. Crossing a window boundary between a record and the tick previously misattributed the delta to the new epoch counter. - Commit the pushed amount immediately after a successful INCRBY, before the aggregate read. A read failure after a successful push previously left the delta uncommitted, so the next tick pushed it again (double count). - Reset a key state when its resolved window changes, since a different window is a different accounting unit. The shared counter is always updated with an atomic INCRBY of the delta (never a SET of a computed absolute), so concurrent instances' increments accumulate instead of clobbering. The read-failure-after-push path is guarded structurally (commit precedes read); simulating a mid-reconcile store failure needs fault injection the harness lacks, so it is covered by inspection plus an idempotency test asserting repeated passes do not re-push.
Store and shared-counter keys previously embedded the raw client value (API key, principal, IP), so secrets could land in the shared store's keyspace and its logs. Hash the value into the key instead; the hash is stable across instances so reconciliation keys still agree. The limit service still receives the raw identity, since it resolves a tier per real principal. Also key by a stable per-rule fingerprint (hash of pattern + key + profile) rather than the rule's list index, so reordering rules or a rolling deploy no longer resets budgets.
secs_ceil computed from truncated milliseconds reports 0 for a sub-millisecond wait; the test expects >= 1s and fails on current code.
Computing whole seconds from truncated milliseconds reported 0 for a sub-millisecond wait. Round up from the nanosecond value so a nonzero wait is never surfaced as 0.
A rate of 0 or an explicit burst of 0 was silently clamped to 1, hiding a misconfigured tier. Reject them at build time instead.
CORS was the innermost layer, so a request short-circuited by auth (401) or Shield (429) never passed back through it and returned without CORS headers, and preflight OPTIONS was subject to auth. Move CORS to the outermost layer and expose the RateLimit-* and Retry-After headers so browser clients can read the budget and back off.
The resolution cache grew with distinct client keys and never shrank. Sweep entries not refreshed within a few refresh cycles (at least 5 minutes), at most once per minute, so client-controlled key cardinality cannot grow it without bound.
Truncating the stored TAT to whole milliseconds lost precision for sub-millisecond emission intervals (very high per-second rates), so a fresh key could re-admit slightly early. Store nanoseconds instead; u64 nanoseconds span far beyond any process uptime. The periodic sweep still throttles on milliseconds.
…shoot - Document that a key's IP fallback is phase-local: an anonymous request under a jwt_claim rule is keyed by IP post-auth but not shed pre-auth, so anonymous flood protection needs a separate pre-auth IP/header rule. Note a path may match one rule per phase (defense in depth) and JWT-based limit resolution applies only to jwt_claim rules. - Document the RateLimit-* / Retry-After response contract and how clients back off, including CORS exposure. - Correct the overshoot formula to normalise the sync interval against the window so the units match, with a worked example.
The store key omits the tier's numbers on purpose: a tier change reuses the existing absolute-time TAT, causing only a brief self-correcting transient, whereas keying by the numbers would let a client reset its budget by flipping tiers.
…entry During a limit-service outage a key's cache entry stops refreshing, so its fetch timestamp goes stale even while the key is actively used. The test inserts a stale entry, accesses it, sweeps, and expects it to survive; it fails on current code, which evicts by fetch age rather than access age. Extracts the sweep body so the test can drive it deterministically.
The cache evicted entries by fetch age, so during a limit-service outage an actively-used key (whose fetch timestamp cannot advance) was dropped after evict_after, silently falling back to the static profile. Track a separate last-access timestamp, bumped on every resolve including stale hits, and evict on that instead.
- README: note that jwt_limits has no effect on an IP/header rule (those run pre-auth without claims); use a jwt_claim key to let the token's tier drive the limit. - global: document that the gate/record split is intentionally not an atomic reservation. The per-instance GCRA store is the hard local cap; the fleet gate is an approximate cross-instance cap whose gate/record race is bounded by the local burst plus the documented one-interval overshoot, kept lock-free on the hot path on purpose.
Two reconciliation races surfaced by fault injection: - The delta push pipeline was non-transactional, so a mid-pipeline failure could apply some INCRBYs while the caller skipped commit_pushed and re-pushed the same deltas next tick (inflating fleet usage into premature 429s). Wrap the push in MULTI/EXEC so it applies all-or-nothing. - A request could reset a key to a new window (or roll it to a newer epoch) while read_epochs awaited the store; applying the old plan's estimate then clobbered the fresh state. Skip the estimate write when the state's window or epoch has moved past the plan.
With defense-in-depth (a pre-auth IP/header rule and a post-auth principal rule on the same path), a 429 from the inner principal limiter propagated out through the outer pre-auth phase, which overwrote RateLimit-Limit/Remaining/Reset with its own verdict. The client then saw the wrong rule's remaining capacity. The outer phase now leaves rate-limit headers intact when an inner limiter already set them.
When a key accumulated admits late in a window and was seen again just after the boundary before the reconciler ran, roll_to zeroed the counts and the previous epoch's unpushed admits were dropped, so other replicas missed that window's traffic in the sliding-window prev count. Stash the unpushed remainder as a carryover owed to the epoch being left; the reconciler publishes it to that epoch's counter and clears it. Assumes the reconcile interval stays much shorter than the window (default 500ms vs seconds), so at most one unpushed epoch is outstanding between reconciles.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ddb41311c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reconciliation is now fire-and-forget: once a pass claims a key's admits (zeroing them), a push that fails or whose EXEC ack is lost simply drops them. Restoring risked republishing a committed-but-unacked batch on the next tick, inflating the fleet count into a false 429; dropping instead under-counts this instance's last interval, which is exactly the documented "store unreachable → degrade to per-instance limiting" behaviour and never rejects valid traffic. It also stops an unpushable delta from accumulating across window boundaries into a collapsed carryover. A window change still resets the entry (a different window is a different accounting unit), a bounded under-count noted at the reset.
Match the rule phase on &key so the KeySource is unambiguously not moved before the fingerprint and CompiledRule reuse it. No behaviour change.
…umbers - Parse limit_service.endpoint at build time and fail startup on a malformed URL, rather than silently disabling dynamic limits when the first background fetch fails to parse it. - When the service returns an unknown tier alongside explicit numbers (e.g. a mid rollout tier the proxy doesn't know yet), fall through to those numbers instead of treating the response as no limit, matching JWT resolution.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b0280e997
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When two window boundaries are crossed before a reconcile claims the carryover (reconcile interval misconfigured longer than the window), roll_to adds the new epoch's count onto the previous carryover under one epoch label. The test expects the latest window to be kept, not the two collapsed; it fails on current code.
…ollapse Three fleet-gate correctness fixes: - Expire the remote estimate: once the store has been unreachable for several intervals the cached estimate is ignored and the gate falls back to per-instance counts, instead of subtracting a frozen estimate forever and rejecting a key indefinitely during an outage. - Count carryover in fleet_remaining: unpushed previous-window admits still contribute to the sliding window, so the gate must subtract them too, not just remote_estimate + local_count. - roll_to keeps only the most recent window when a second boundary is crossed before a reconcile claims the carryover (interval misconfigured longer than the window), rather than collapsing two epochs' counts under one label.
A header-keyed rule whose configured name isn't a valid HTTP header (e.g. it contains whitespace) would never match, silently downgrading a per-API-key limit to per-IP. Parse the name as a HeaderName during compile and fail startup on an invalid value.
…outage A hot key with a stale cached limit re-triggered a fetch on every request while the service was down, since only brand-new keys were negative-cached. Reset the existing entry's fetch timestamp on failure so it is served for another TTL before the next retry, capping outbound calls at one per key per TTL.
…-After - On an allowed response where both phases matched, report the smaller remaining across phases (overwrite the inner limiter's headers only when this phase is tighter), so a client near the tighter budget backs off in time. Inner rejections (remaining 0) are still preserved. - A fleet-gate 429 now advertises a modest poll interval (a tenth of the window, min 1s) instead of the epoch boundary: the sliding-window estimate decays continuously and does not free capacity at the boundary, so a client waiting exactly to the boundary could be rejected again.
There was a problem hiding this comment.
💡 Codex Review
structured-proxy/src/shield/mod.rs
Lines 396 to 397 in 02aad01
Do not trust XFF when peer information is absent
When the router is embedded or served without ConnectInfo<SocketAddr>, peer is None and this branch accepts client-controlled X-Forwarded-For/X-Real-IP even when trusted_proxies is empty. That contradicts the Shield config contract that forwarding headers are honored only from trusted proxies, and lets clients rotate those headers to evade IP-keyed limits in that embedding context.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t client-driven - Use a self-reconnecting ConnectionManager for the shared store instead of a cached MultiplexedConnection. A Redis restart or dropped TCP connection is now re-established internally, so reconciled mode recovers on its own rather than staying degraded until the proxy restarts. - Clarify that a key's window only changes when its resolved tier does (from the signed JWT, the limit service, or config, never client input), so the one-window under-count on reset is a rare, bounded, non-client-triggerable event, not an abuse vector.
URL syntax validation alone let schemes like redis:// or file:// pass startup even though the background GET can never use them, silently disabling dynamic limits. Reject any non-http(s) scheme at build time.
A header-keyed rule configured as X-API-Key on one instance and x-api-key on another selects the same header but produced different rule fingerprints, splitting the counter namespace so one API key could consume multiple budgets. Store the parsed HeaderName's normalized (lowercased) form so the fingerprint is casing-independent.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@README.md`:
- Around line 233-238: Update the “Sizing the overshoot” reconciled-mode
documentation to include each peer’s configured burst capacity in the worst-case
fleet overshoot bound, rather than documenting only the rate-based lag term.
Ensure the formula and numeric example reflect the burst contribution, or
explicitly state a separately verified global cap if that is the intended
behavior.
In `@src/config.rs`:
- Around line 430-434: Correct the phase-derivation documentation above the
rate-limit rule type so only rules using a jwt_claim key are described as
running after authentication. Remove the claim that JWT-based limit resolution
changes the phase, and state that IP/header rules remain before authentication.
- Around line 378-380: Update ShieldConfig and every nested Shield configuration
struct to reject unknown deserialization fields using deny_unknown_fields or the
project’s equivalent validation, ensuring misspelled keys cannot silently
produce incomplete rules. Add a regression test covering an unknown field such
as profil and verify deserialization fails.
In `@src/shield/global.rs`:
- Around line 333-339: Update the estimate assignment in the sliding-window
gating path to use ceil() instead of round() before clamping and converting to
u64, ensuring fractional usage is rounded upward before admission decisions. Add
or update boundary coverage for estimates just below and above integer
thresholds, including exact integer values, to verify requests cannot exceed the
sliding budget.
In `@src/shield/mod.rs`:
- Around line 104-113: Update the global counter initialization around
GlobalCounters::build and spawn so global is set to Some only when
reconciliation starts successfully. If spawn cannot start because no Tokio
runtime is available, leave global as None so request gating falls back to local
GCRA enforcement; alternatively move startup into an async lifecycle hook.
- Around line 207-211: Update the peer/client identity flow around the
ConnectInfo extraction and client_ip call so missing ConnectInfo cannot cause
forwarding headers to be trusted; return a stable fail-closed identity such as
"unknown" unless a trusted client-IP source is available. Preserve normal
client_ip processing when ConnectInfo exists, and adjust affected tests to
inject ConnectInfo.
In `@src/shield/resolve.rs`:
- Around line 216-222: Bound all client-keyed state with finite capacities and
explicit overflow handling: update src/shield/resolve.rs lines 216-222 around
sweep to cap the cache, src/shield/store.rs lines 78-84 to cap live TAT entries
in addition to draining them, and src/shield/global.rs lines 393-405 to cap
fleet reconciliation states alongside idle eviction; preserve existing eviction
behavior while ensuring rapid key rotation cannot grow any collection without
limit.
- Around line 249-296: Update trigger_refresh to enforce a global concurrency
limit in addition to per-key inflight deduplication, using an existing or newly
initialized semaphore around the fetch task. When the limit cannot be acquired,
remove the key from inflight and serve the existing cached stale/static result
without starting an outbound request; preserve the current success, failure,
cache, and inflight cleanup behavior for permitted refreshes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f8cfa84-db4d-469c-802e-e2c76c9323f6
📒 Files selected for processing (16)
.github/workflows/ci.ymlCargo.tomlREADME.mdsrc/auth/forward.rssrc/auth/jwks.rssrc/auth/mod.rssrc/config.rssrc/lib.rssrc/shield/gcra.rssrc/shield/global.rssrc/shield/matcher.rssrc/shield/mod.rssrc/shield/resolve.rssrc/shield/store.rssrc/shield/tests.rssrc/shield/window.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9afc81674
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Idle sweeps bound how long entries live, not peak cardinality. Add hard caps so client-controlled key churn (IP / principal / API key) cannot grow memory without bound: - limit-service cache: cap at 100k entries, evicting least-recently-accessed on overflow, plus a semaphore capping concurrent background fetches at 32 so a miss storm cannot fan out unbounded outbound calls or tasks - GCRA store: size-triggered drained-key eviction between timer sweeps; only drained (past-TAT) keys are dropped, never active ones (dropping a limited key would reset its budget and hand a flooder a fresh burst) - fleet counters: cap tracked keys at 500k; a new key past the cap degrades to per-instance limiting (still locally capped) rather than being tracked, matching the existing best-effort, never-double fleet contract
- client_ip: when no connection peer is available, bucket as "unknown" instead of trusting client-supplied X-Forwarded-For / X-Real-IP. Without a trusted peer those headers are attacker-rotatable, so honouring them let a single client dodge a per-IP limit by varying the header. - fleet gate: keep it only when reconciliation actually started; otherwise drop to per-instance limiting rather than gating on an estimate that would never be refreshed. - document that the fleet budget is the sustained rate and per-instance burst is not pre-reserved fleet-wide, and fold burst into the README overshoot formula: (N-1) * (burst + rate * (interval/window)).
…aders - config: regression test that a typo in a shield-config field is a hard deserialization error (deny_unknown_fields), so a misspelled security key cannot silently leave a limit unapplied. - two-phase: assert an allowed response advertises the tighter pre-auth remaining rather than the roomier post-auth one, so the client backs off against the binding limit.
Make explicit that a profile with burst greater than the sustained limit (e.g. rate 1/min, burst 5) is capped at the sustained limit fleet-wide, so the extra burst headroom a local-only deployment allows is not granted under reconciliation. This is the intended conservative choice: gating on burst would let the fleet sustain burst-per-window, loosening the sustained cap.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c16ca65ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When two defense-in-depth phases both leave the same remaining, the response must advertise the longer reset (the budget that binds longest) so a paced client doesn't retry early into a still-blocked outer limit. This test drives a pre-auth 1/hour IP rule and a post-auth 1/min principal rule; it fails on current code (the inner 60s reset wins) and passes once the tie-break lands.
On a remaining tie between two defense-in-depth phases the header guard kept the inner phase's reset unconditionally, so a client pacing off an allowed response could see a short per-minute reset while an hourly IP cap still blocked it, retry early, and hit 429. The guard now keeps the inner headers on a tie only when their reset is at least as long as this phase's; otherwise the longer-binding budget's headers win. Covered by the test in c12f872.
The high-water bypass ran a full DashMap retain whenever the map exceeded the mark, but active TATs cannot be reclaimed, so an all-active key flood kept the map over the mark and triggered an O(n) scan on every request (a CPU DoS). High-water sweeps are now floored to one per second, bounding the scan cost while still reclaiming drained keys sooner than the normal cadence.
Capacity was only enforced from the request path before a fetch and in the 60s sweep, but completed fetches insert from a background task. A healthy service plus clients rotating distinct identities could therefore push the cache far past MAX_CACHE_ENTRIES between sweeps, and a burst that stops leaves no later request to trim it. Inserts now refuse to create a new entry once at capacity (existing keys still refresh), so peak size is bounded at write time; the sweep trim stays as a backstop for the rare concurrent-insert overshoot.
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)
README.md (1)
28-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument both prerequisites for Redis reconciliation.
This bullet says fleet reconciliation is available when
redisis enabled, but the detailed deployment section requires both theredisfeature and a configuredsyncblock. Clarify the summary to avoid users enabling the feature without configuring reconciliation.🤖 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 `@README.md` at line 28, Update the Rate limiting (Shield) summary bullet to state that fleet-wide Redis reconciliation requires both the redis feature and a configured sync block, matching the detailed deployment prerequisites.
🤖 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 `@README.md`:
- Line 28: Update the Rate limiting (Shield) summary bullet to state that
fleet-wide Redis reconciliation requires both the redis feature and a configured
sync block, matching the detailed deployment prerequisites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 067ede6c-554f-4844-972e-4f5f882e4f31
📒 Files selected for processing (7)
README.mdsrc/config.rssrc/shield/global.rssrc/shield/mod.rssrc/shield/resolve.rssrc/shield/store.rssrc/shield/tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5bef30306
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // set headers on the way out; overwrite them only when this (outer) phase's | ||
| // remaining is smaller, so the client always sees the budget that will bite | ||
| // first. An inner rejection carries remaining 0, so it is never overwritten. | ||
| maybe_tighten_rate_headers(response.headers_mut(), profile.limit, reported, &verdict); |
There was a problem hiding this comment.
When the redis feature is enabled and fleet_remaining is the tighter budget, reported is reduced to the fleet value, but the headers are still generated from the local GCRA verdict. A fresh local key with only one fleet slot left can therefore return RateLimit-Remaining: 0 with a short local RateLimit-Reset, even though the Redis sliding-window estimate may stay saturated much longer; clients pacing from that allowed response will retry too early and immediately hit the fleet gate. Use a fleet-derived reset, or avoid advertising a precise reset, when the reported remaining count came from the fleet budget.
Useful? React with 👍 / 👎.
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.
Summary
Replaces the fixed-window rate limiter with an embedded, config-driven GCRA
shaper designed for a data plane: every decision is made in-process, so the
request path never blocks on a store.
burst, then throttles torate, with no fixed-window boundary burst.ratelimit_rpm/ratelimit_burst) → external service (cached, refreshed off the request path) → rule profile → default. Tiers are named config data, so numbers retune without re-issuing tokens.sync+redisfeature): instances push deltas / pull the aggregate on an interval using a sliding-window counter, converging on an approximate fleet-wide limit. A store outage degrades to per-instance limiting rather than failing requests.RateLimit-Limit/-Remaining/-Reset, plusRetry-Afteron429.Auth attaches the verified claims as a typed request extension (never set from client input) for the post-auth phase.
Testing
--features redis, and--no-default-features --features aws_lc_rs,redis.reconciles_across_instances) drives two instances against a live store and asserts they enforce one combined budget. CI gains a Redis service andSHIELD_REDIS_TEST_URLso it runs there.cargo fmt --check,clippy -D warnings(both feature sets), andcargo doc -D warningsare clean.Docs
Issue #68 rewritten to the revised design; README gains a Rate limiting section (keying/phases, limit sources, deployment modes, overshoot sizing) and an updated config example.
BREAKING CHANGE: the
shieldconfig schema is replaced.endpoint_classes,identifier_endpoints, andwindow_secsare removed in favour ofprofiles,rules(pattern + key + profile),default_profile,jwt_limits,limit_service, andsync.Closes #68