[smarcet]

ref: https://app.clickup.com/t/86bchtzb4

Summary

Promo code GET /api/v1/summits/{id}/promo-codes/{code}/apply was returning {"message":"server error"} (HTTP 429) to every user behind an IP that had previously used another rate.limit route. During 2026 OCP Global (summit 73) the venue IP stayed blocked on checkout for about an hour after a show-admin PUT .../submission-period/reopen/notify.

Root cause

• RateLimitMiddleware keyed its counter on sha1(domain|ip) only (request()->user() is always null for OAuth2 bearer requests). Every rate.limit route, and the whole public_api group, shared one counter and one window per IP. The first writer of a key fixes its TTL, so notify's 30-hit / 3600 s window became the budget of apply (25,1). Verified in prod Redis: the key held 47 hits with the TTL created at the exact second of a notify call.
• Handler::render turned every HttpException into {"message":"server error"} and dropped the exception headers, so a 429 looked like a server fault and carried no Retry-After.

Changes

• RateLimitMiddleware: override resolveRequestSignature so the key includes the HTTP method and the matched route template (not the concrete URL). Each rate limited route now has its own counter and window per IP.
• Handler: TooManyRequestsHttpException (which ThrottleRequestsException extends) is rendered as {"message":"Too Many Requests"} with status 429 and the exception headers (Retry-After, X-RateLimit-*). Every other exception keeps the current behaviour.

Deliberately not changed: units and numeric limits. The second rate.limit argument is seconds by convention in this codebase (see the note on the notify route and #590), so 25,1 and 10000,1 are per second today. Enforcing the per-minute intent during a live event could create new 429s for shared venue IPs, so the unit/limit audit goes in a separate ticket.

Deployment notes

• Limiter keys change shape: counters start from zero at deploy and the old keys expire on their own.
• The notify limit (30 per hour) is now actually enforced per IP; before it was effectively shared with every other rate limited route.

Tests

• New tests/Unit/RateLimitMiddlewareTest.php:
  • exhausting notify from one IP does not block apply from the same IP (failed before the fix with ThrottleRequestsException);
  • the same route still enforces its own limit;
  • a 429 renders Too Many Requests with Retry-After (failed before the fix with server error).
• Run: docker exec summit-api bash -lc "cd /var/www && vendor/bin/phpunit tests/Unit/RateLimitMiddlewareTest.php"
• Also green (run on their own): OAuth2SummitPromoCodesApiTest (34 tests, 3 skipped), OAuth2PromoCodesApiTest (40), PresentationReopenApiTest (33), PresentationReopenAuthzTest (14).
• tests/Unit ran in full: 410 tests, 3 not passing, none in files this PR touches: CacheMiddlewareTest::test_lock_timeout_exception_handled_gracefully (mock cannot configure lock) and two in SponsorUserPermissionTrackingTest (testRemoveSponsorUserFromGroupStillCleansUpWhenSponsorWasDeleted, testRemoveSponsorUserFromGroupRemovesGlobalGroupWhenLastSponsor). They do not reference the rate limiter or the handler, but I did not re-run them on main to confirm they fail there too.
• Not run to completion: the full suite and tests/oauth2 as a whole (stopped after more than an hour). CI will cover those.

Summary by CodeRabbit

• Bug Fixes
  • Rate limits are now tracked separately by route and HTTP method, so reaching the limit on one route does not prevent the same client from accessing other routes.
  • Requests that exceed a rate limit receive a clear 429 Too Many Requests response instead of being reported as a server error. JSON responses include retry timing information.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2a994115-89bc-4114-9196-7d33e558b796

📥 Commits

Reviewing files that changed from the base of the PR and between ed5ae2a and a058c67.

📒 Files selected for processing: 3

• app/Exceptions/Handler.php
• app/Http/Middleware/RateLimitMiddleware.php
• tests/Unit/RateLimitMiddlewareTest.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Rate-limit signatures now include the HTTP method and route. For JSON and API requests, throttling exceptions now produce a 429 JSON response with the exception’s headers.

Changes: Rate-limit handling

Layer / File(s)	Summary
Route-scoped rate-limit counters app/Http/Middleware/RateLimitMiddleware.php, tests/Unit/RateLimitMiddlewareTest.php	The rate-limit signature includes the HTTP method, route URI or fallback request path, and parent request signature. Tests check that separate routes have separate counters and that the limit still applies on the same route.
Throttling exception response app/Exceptions/Handler.php, tests/Unit/RateLimitMiddlewareTest.php	JSON and API requests with a throttling exception receive status 429 and a JSON message. The response includes the exception’s headers. A test checks the message and Retry-After value.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to a058c

The change gives each route its own rate-limit counter and returns a proper 429 JSON response. No actionable merge-blocking risk remains, though the notify limit is still keyed per authenticated user rather than per IP, as it was before.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes both primary changes: scoping rate-limit keys per route and returning a proper 429 response body.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-639/

This page is automatically updated on each push to this PR.

[Copilot]

🟢 Approval recommended

The focused implementation addresses both root causes and includes appropriate regression coverage.

0 open findings

What changed in this PR

Scopes rate-limit counters per HTTP method and route while preserving actionable 429 responses.

Changes:

• Adds route-specific limiter signatures.
• Preserves 429 messages and retry headers.
• Adds regression tests for limiter isolation and responses.

File	Description
app/​Http/​Middleware/​RateLimitMiddleware.php	Scopes limiter keys by route and method.
app/​Exceptions/​Handler.php	Returns proper JSON 429 responses and headers.
tests/​Unit/​RateLimitMiddlewareTest.php	Tests isolation, enforcement, and rendering.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.