Repository navigation
test(limiter): pin rate-limiter behavior — limits, 429, exclusions, identities, documented fail-open - #449
test(limiter): pin rate-limiter behavior — limits, 429, exclusions, identities, documented fail-open#449Alajmah wants to merge 1 commit into
Conversation
…dentities, documented fail-open Unit C of the #425 dismissal-gate sequence (test-only; NO limiter changes). Seven regressions pin the existing semantics that the 13 API-alert false-positive dismissals rest on: - applicable requests pass and carry X-RateLimit headers - the 121st request in the window → 429 + Retry-After 60 - distinct Bearer identities get independent windows - requests without Bearer credentials fall back to IP identity (shared per-IP bucket) — correcting the matrix's identity statement - /health excluded entirely (no Redis interaction, unlimited passage) - /webhooks* excluded - DOCUMENTED FAIL-OPEN: a Redis failure currently allows the request through (logged). Pinned deliberately — changing to fail-closed is an availability trade-off reserved to a separate decision, and this test must change WITH that decision, never silently. Signed-off-by: Alajmah <55954113+Alajmah@users.noreply.github.com>
|
/gitwire run review |
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 0f3c412d3d
ℹ️ 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".
| it("requests WITHOUT Bearer credentials fall back to IP identity", async () => { | ||
| const a = await runMiddleware({ ip: "10.0.0.1" }); | ||
| const b = await runMiddleware({ ip: "10.0.0.2" }); | ||
| const a2 = await runMiddleware({ ip: "10.0.0.1" }); |
There was a problem hiding this comment.
This exercises only a missing Authorization header, so it passes even though rateLimiter currently applies slice(7) to every authorization scheme. Requests such as Authorization: Basic attacker-1 therefore use the sliced header as their identity instead of the IP, and a client can vary that value to evade the shared IP bucket while this purported regression remains green. Include a non-Bearer or malformed header in this case so the claimed fallback is actually verified.
AGENTS.md reference: AGENTS.md:L111-L114
Useful? React with 👍 / 👎.
| redisState.counts.set(key, next); | ||
| return next; | ||
| }), | ||
| pexpire: jest.fn(async () => 1), |
There was a problem hiding this comment.
The Redis mock stores counts indefinitely and its pexpire implementation has no effect, so the suite would still pass if the production middleware stopped setting a TTL or used the wrong duration. In that regression, an identity that reaches 120 requests would remain blocked permanently rather than only for the documented minute. Model time/expiry or at least assert the pexpire call and duration, then verify admission after expiration.
Useful? React with 👍 / 👎.
| expect(passed).toBe(true); | ||
| expect(res.statusCode).toBe(200); |
There was a problem hiding this comment.
This case claims that a Redis failure is logged, but it checks only passage and status, so removing the logger.error call would leave the test green and silently violate the repository's error-logging requirement. Retain a reference to the mocked logger and assert the expected log invocation for ECONNREFUSED.
AGENTS.md reference: AGENTS.md:L103-L105
Useful? React with 👍 / 👎.
![gitwire-hq[bot]](https://avatars.githubusercontent.com/in/3727207?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Review — ✅ Approved
Confidence: high · Findings: 2
AI judgment: APPROVE · Evidence: COMPLETE · Authority: advisory
Evidence complete · 1/1 changed files accounted for
Other findings (2)
- [medium] Fail-open test never asserts the logging behavior its title and the PR description pin
- [low] Unused makeApp helper is dead code whose mount order would produce vacuous tests if ever used
🔮 Devil's Advocate (2 turns): 2 upheld
GitWire AI Review Gate (bundle-driven v2) · Structured schema · Scope-validated · Devil's Advocate · AI recommendation — maintainers and repository policy retain authority
| expect(redisState.counts.size).toBe(0); | ||
| }); | ||
|
|
||
| it("DOCUMENTED FAIL-OPEN: a Redis failure currently allows the request through and logs", async () => { |
There was a problem hiding this comment.
[MEDIUM] Fail-open test never asserts the logging behavior its title and the PR description pin
The test is titled 'DOCUMENTED FAIL-OPEN: a Redis failure currently allows the request through and logs', and the PR description states the fail-open is '(logged)'. However, the test never imports the mocked logger module and never asserts that logger.warn/error was called, so the 'logged' half of the pinned contract is unverified. A regression that removes the warning log (fail-open goes silent, losing all observability of limiter bypass) would keep this suite green. Additionally, expect(res.statusCode).toBe(200) is tautological: the mock res initializes statusCode to 200 and passed=true already proves next() was invoked. Suggested fix: after the middleware import, add const { logger } = await import("../../src/lib/logger.js"); and in this test assert e.g. expect(logger.warn).toHaveBeenCalled() (or whichever level the middleware uses), keeping the log assertion tied to the same 'must change with the fail-closed decision' comment.
|
|
||
| const express = (await import("express")).default; | ||
|
|
||
| function makeApp() { |
There was a problem hiding this comment.
[LOW] Unused makeApp helper is dead code whose mount order would produce vacuous tests if ever used
makeApp() builds an Express app but mounts app.use(rateLimiter) AFTER the route handlers. Since the routes terminate the response, the limiter would never execute for any request through that app, so any future test written against makeApp would pass vacuously while asserting nothing about the limiter. The author clearly knows this (the NOTE comment and void makeApp;), but keeping ~10 lines of known-broken dead code in a regression-pinning suite is a trap for later maintainers. Either delete makeApp entirely, or fix it to mount the limiter before the routes (and use supertest) so the helper is actually valid. The direct-middleware runMiddleware approach is the right one; this leftover only adds confusion.
Adjudication — GitWire AI Review @ 0f3c412 (APPROVED, high confidence, 2 findings)Disposition: none blocking; both accepted as recorded test-quality notes.
Gate state: all checks green, |
|
Superseded by the v2 PR: logging assertion, TTL pin, mount-order fix, and the end-to-end non-Bearer identity-bypass reproducer. |
Unit C of the authorized dismissal-gate sequence — test-only, no limiter changes. Seven regressions pin the exact semantics the 13 API-alert false-positive dismissals rest on:
X-RateLimit-*headers.Retry-After: 60./healthexcluded entirely — no Redis interaction across 130 requests./webhooks*excluded.Holding at the merge gate — exact head
0f3c412d3df2417a46e5fd5b2e279250cf319126.