Apply user-based API rate limits - #1640
Conversation
Greptile SummaryThe PR replaces primary IP-based throttling on authenticated API families with shared controller-level limits keyed by users or Admin API keys, retains an API-wide IP ceiling, and documents the resulting response contract.
Confidence Score: 1/5The PR should not merge until failed authentication requests remain protected by a primary throttle, Admin OAuth shares the intended user bucket, and the generated API contract is updated. Path-only throttle exemptions permit large bursts of rejected authentication traffic, the Admin OAuth prefix doubles the documented per-user allowance across API families, and the committed Swagger contract will remain stale. Files Needing Attention: config/initializers/rack_attack.rb, app/controllers/api/admin/application_controller.rb, spec/requests/api/hackatime/v1/compatibility_spec.rb
|
| Filename | Overview |
|---|---|
| app/controllers/concerns/authenticated_api_rate_limiting.rb | Introduces the shared fixed-window controller limiter and compatible 429 response headers; its discriminator makes identity-prefix consistency important. |
| config/initializers/rack_attack.rb | Enables Rack::Attack and removes authenticated path families from primary IP throttles, inadvertently weakening protection for failed authentication requests. |
| app/controllers/api/admin/application_controller.rb | Adds per-key and OAuth identities, but the Admin OAuth prefix splits the same user's allowance from regular authenticated APIs. |
| app/controllers/api/hackatime/v1/hackatime_controller.rb | Applies the user limiter after credential resolution; callback ordering prevents the provisional nil-user failure. |
| test/controllers/concerns/authenticated_api_rate_limiting_test.rb | Covers the 300-request boundary, reset response, and safelist bypass but not cross-controller identity sharing or failed authentication. |
| spec/requests/api/hackatime/v1/compatibility_spec.rb | Updates the documented 429 contract without regenerating the repository's committed Swagger output. |
| docs/developers/rate-limits.md | Documents shared user and per-key limits, retry headers, and client guidance; its shared-user promise conflicts with the Admin OAuth discriminator. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
R[API request] --> P{Authenticated API path?}
P -- No --> IP[Primary IP throttles]
P -- Yes --> A[Controller authentication]
A -- Invalid --> U[401 response]
A -- Valid --> I{Identity type}
I -- User or regular OAuth --> B[Shared user bucket]
I -- Admin OAuth --> O[Admin OAuth user bucket]
I -- Admin API key --> K[Per-key bucket]
R --> C[10,000/hour API-wide IP ceiling]
Prompt To Fix All With AI
### Issue 1
config/initializers/rack_attack.rb:45-51
**Rejected requests bypass throttles**
When a client repeatedly sends missing or invalid credentials to these API families, the path-based exclusions skip both primary IP throttles and authentication halts before the controller limiter runs, allowing a burst of up to 10,000 failed authentication requests instead of 300 per minute. **How this was verified:** The excluded paths retain only the 10,000-per-hour API throttle, while each controller registers authentication before the new rate-limit callback.
### Issue 2
app/controllers/api/admin/application_controller.rb:16-20
**Admin OAuth splits allowance**
When an administrative user calls both regular and Admin APIs through OAuth, `oauth_user:<id>` and `user:<id>` create independent counters in the shared scope, allowing 600 requests per minute instead of the documented shared 300-request user allowance.
```suggestion
def authenticated_api_rate_limit_identity
return "admin_api_key:#{current_admin_api_key.id}" if current_admin_api_key
"user:#{current_user.id}"
end
```
### Issue 3
spec/requests/api/hackatime/v1/compatibility_spec.rb:136-143
**API contract remains stale**
The Rswag response schema and rate-limit header examples changed without regenerating `swagger/v1/swagger.yaml`, so the published OpenAPI contract continues to expose the previous heartbeat rate-limit response.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Re-enable Rack Attack" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Rack::Attack.throttle("general", limit: 300, period: 1.minute) do |req| | ||
| unless req.path.start_with?("/assets") | ||
| oauth_user_id(req) || req.ip | ||
| end | ||
| req.ip unless req.path.start_with?("/assets") || authenticated_api_request?(req) | ||
| end | ||
|
|
||
| Rack::Attack.throttle("posts by ip", limit: 60, period: 5.minutes) do |req| | ||
| req.ip if req.post? && !heartbeat_request?(req) | ||
| req.ip if req.post? && !authenticated_api_request?(req) | ||
| end |
There was a problem hiding this comment.
When a client repeatedly sends missing or invalid credentials to these API families, the path-based exclusions skip both primary IP throttles and authentication halts before the controller limiter runs, allowing a burst of up to 10,000 failed authentication requests instead of 300 per minute. How this was verified: The excluded paths retain only the 10,000-per-hour API throttle, while each controller registers authentication before the new rate-limit callback.
Prompt To Fix With AI
This is a comment left during a code review.
Path: config/initializers/rack_attack.rb
Line: 45-51
Comment:
**Rejected requests bypass throttles**
When a client repeatedly sends missing or invalid credentials to these API families, the path-based exclusions skip both primary IP throttles and authentication halts before the controller limiter runs, allowing a burst of up to 10,000 failed authentication requests instead of 300 per minute. **How this was verified:** The excluded paths retain only the 10,000-per-hour API throttle, while each controller registers authentication before the new rate-limit callback.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| def authenticated_api_rate_limit_identity | ||
| return "admin_api_key:#{current_admin_api_key.id}" if current_admin_api_key | ||
|
|
||
| "oauth_user:#{current_user.id}" | ||
| end |
There was a problem hiding this comment.
When an administrative user calls both regular and Admin APIs through OAuth, oauth_user:<id> and user:<id> create independent counters in the shared scope, allowing 600 requests per minute instead of the documented shared 300-request user allowance.
| def authenticated_api_rate_limit_identity | |
| return "admin_api_key:#{current_admin_api_key.id}" if current_admin_api_key | |
| "oauth_user:#{current_user.id}" | |
| end | |
| def authenticated_api_rate_limit_identity | |
| return "admin_api_key:#{current_admin_api_key.id}" if current_admin_api_key | |
| "user:#{current_user.id}" | |
| end |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/api/admin/application_controller.rb
Line: 16-20
Comment:
**Admin OAuth splits allowance**
When an administrative user calls both regular and Admin APIs through OAuth, `oauth_user:<id>` and `user:<id>` create independent counters in the shared scope, allowing 600 requests per minute instead of the documented shared 300-request user allowance.
```suggestion
def authenticated_api_rate_limit_identity
return "admin_api_key:#{current_admin_api_key.id}" if current_admin_api_key
"user:#{current_user.id}"
end
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| message: { type: :string, example: 'Woah there, way too fast, take a chill pill speedy gonzales!' }, | ||
| retry_after: { type: :integer, example: 30 }, | ||
| reset_at: { type: :string, format: :date_time, example: '2024-03-20T15:30:30Z' } | ||
| } | ||
| header 'Retry-After', schema: { type: :string, example: '30' }, description: 'Seconds until the rate limit resets' | ||
| header 'X-RateLimit-Limit', schema: { type: :string, example: '360' } | ||
| header 'X-RateLimit-Limit', schema: { type: :string, example: '300' } | ||
| header 'X-RateLimit-Remaining', schema: { type: :string, example: '0' } | ||
| header 'X-RateLimit-Reset', schema: { type: :string, example: '30' } | ||
| header 'X-RateLimit-Reset', schema: { type: :string, example: '1710948630' } |
There was a problem hiding this comment.
The Rswag response schema and rate-limit header examples changed without regenerating swagger/v1/swagger.yaml, so the published OpenAPI contract continues to expose the previous heartbeat rate-limit response.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: spec/requests/api/hackatime/v1/compatibility_spec.rb
Line: 136-143
Comment:
**API contract remains stale**
The Rswag response schema and rate-limit header examples changed without regenerating `swagger/v1/swagger.yaml`, so the published OpenAPI contract continues to expose the previous heartbeat rate-limit response.
**Context Used:** AGENTS.md ([source](https://github.com/hackclub/hackatime/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary of the problem
API clients running behind shared network addresses can consume the same primary rate limit, causing unrelated users and integrations to receive 429 responses.
Describe your changes
Apply a shared 300 requests per minute allowance per authenticated user across OAuth, API key and Waka-compatible APIs, with separate allowances for Admin API keys. Keep an API-wide IP ceiling as secondary abuse protection while preserving the existing 429 response contract and bypass behaviour.
Add developer documentation covering API limits, retry headers and OAuth usage.
Screenshots / Media
Not applicable.