Conversation
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughShield 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 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 asynchronous Redis reconciliation with epoch carryover, stale-estimate handling, and claim-based delta publication. |
| src/shield/mod.rs | Adds two-phase enforcement, dynamic profile resolution, request key derivation, and response headers. |
| src/shield/gcra.rs | Implements local GCRA calculations for burst limits, refill, retry timing, and remaining capacity. |
| src/shield/store.rs | Adds atomic per-key storage for local GCRA timestamps. |
| src/shield/resolve.rs | Adds JWT and cached external-service limit resolution. |
| src/config.rs | Replaces the Shield schema with profiles, rules, key sources, limit services, and synchronization settings. |
| src/lib.rs | Places Shield around authentication in two phases and moves CORS outside enforcement middleware. |
| Cargo.toml | Enables Redis connection-manager support for the reconciliation task. |
| .github/workflows/ci.yml | Adds Redis-backed integration testing and updates the cargo-deny action. |
Reviews (12): Last reviewed commit: "fix(shield): skip idle zero-delta keys i..." | 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.
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".
Pull the local-vs-fleet remaining reconciliation out of enforce() into a pure helper returning the reported remaining plus the verdict that drives the response headers. Behaviour is unchanged; this creates a unit-testable seam for the header values.
When the fleet budget binds an allowed response, the reset must reflect the fleet's sliding-window decay, not the short local GCRA reset, or a paced client retries early into the fleet gate. This threads the window into reconciled_headers (still unused) and adds a test that fails on current code (reset stays 100ms) and a companion that pins the local-binds case. The fix lands next.
An allowed response whose remaining was reduced to the fleet value still carried the local GCRA reset, which for a high-rate key can be a fraction of a second even while the shared sliding-window estimate stays saturated for much longer. A client pacing off that reset retried early and immediately hit the fleet gate. reconciled_headers now widens the reset (and retry-after) to the fleet backoff whenever the fleet budget binds, never shrinking a longer local reset. The backoff is factored into fleet_backoff, shared with global_reject. Covered by the test in 7aefc04.
…block The feature summary implied enabling the `redis` feature alone turns on fleet reconciliation, but the deployment section requires a configured `sync` block too. Clarify the bullet so users don't build with the feature and expect fleet-wide limiting without configuring it.
Update the SHA-pinned advisory-scan action to v2.1.1.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70d067fac7
ℹ️ 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 the outer phase's longer reset wins a RateLimit-Remaining tie on an error response, Retry-After must be widened to match, or a client retries after the shorter overwritten wait and immediately hits the binding outer gate. This test drives maybe_tighten_rate_headers with an inner 429's short headers and a longer outer verdict; it fails on current code (Retry-After stays 60) until the fix.
maybe_tighten_rate_headers overwrote RateLimit-* with the longer-binding outer budget on a remaining tie but left the inner layer's Retry-After untouched, so a 429 could advertise the outer's hour-long reset yet tell the client to retry after the inner's minute. It now widens Retry-After to the winning reset on an error response (never shrinking, and never adding one to a 200). Covered by the test in 13acd1a.
There was a problem hiding this comment.
💡 Codex Review
structured-proxy/src/shield/mod.rs
Lines 494 to 498 in 068517b
Ignore malformed forwarded hops
When a trusted proxy passes an X-Forwarded-For chain whose rightmost untrusted hop is malformed, such as junk-<nonce>, 10.0.0.5 with 10.0.0.5 in trusted_proxies, the failed IP parse makes trusted_hop false and returns the attacker-controlled string as the client identity. Because that value feeds the IP rate-limit key, a client that can influence the forwarded header can rotate invalid hop strings to get fresh buckets; skip malformed hops or fall back to the peer/unknown bucket instead.
ℹ️ 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".
A rule pattern without a leading slash (e.g. `api/**`) can never match the request path, which always starts with `/`, so the route runs unmetered while Shield is enabled. This test expects compile_rules to reject such a pattern; it fails on current code until the validation lands.
compile_rules now fails a rule whose pattern does not start with '/'. Patterns are matched against the request path, which always has a leading slash, so a relative pattern (a missing-slash typo) compiled a matcher that never fired and left the route unmetered. Covered by the test in 5c48ddd.
The internally-tagged KeySourceConfig enum did not reject unknown fields, so a stray `name` on an `ip` key was silently ignored, keeping a rule IP-keyed instead of the intended per-header limit. This test expects such a config to fail deserialization; it fails on current code until the fix lands.
serde does not honour deny_unknown_fields on internally-tagged enums, so a stray field in a rule key (e.g. `name` on an `ip` key) was silently ignored, leaving the rule IP-keyed instead of the intended per-header limit. KeySourceConfig now deserializes through a flat deny_unknown_fields form plus per-variant validation that rejects a field belonging to a different key type and a genuinely unknown field alike. The wire schema is unchanged. Covered by the test in 50b23ab.
Move the phase-1 per-key claim-and-plan loop into a sync helper returning the push plans. Behaviour is unchanged (a plan per tracked key); this creates a seam to test which keys a reconcile tick refreshes.
A reconcile tick planned a store read for every tracked key, so a one-shot burst of distinct keys kept issuing MGETs for idle principals until KEY_TTL expiry, turning a brief cardinality spike into sustained load. This test expects claim_plans to skip an idle zero-delta key; it fails on current code until the activity gate lands.
A reconcile tick planned a store read for every tracked key, so a one-shot burst of distinct keys kept issuing per-key MGETs for idle principals until KEY_TTL (up to 10 min at the default interval), turning a brief cardinality spike into sustained CPU/store load. KeyState now carries a per-tick activity flag set when the key is recorded or gated and cleared each pass; claim_plans skips a key that is idle and has no pending delta/carryover. A later request re-marks it and the next tick refreshes its estimate. Covered by the test in f248982.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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