Return 429 from the OAuth token endpoint when rate-limited#3236
Return 429 from the OAuth token endpoint when rate-limited#3236
Conversation
The /oauth/token endpoint's two rate-limit branches (no-IP fail-closed and the >=20/min cap) returned 400 with the OAuth error_code "invalid_request" because the token_error() helper was hard-wired to HTTP 400. RFC 6749 §5.2 specifies 400 for OAuth-defined error codes, but rate limiting isn't an OAuth-defined error — RFC 6585 defines 429 "Too Many Requests" for that case, and every other OAuth controller in this plugin (authorize, register, revoke) already returns 429. - Add an optional $status parameter to token_error() defaulting to 400 so existing callers are unchanged. - Use 429 with a "rate_limited" error code on both rate-limit branches. - Add a controller test asserting 429 + the new error code.
There was a problem hiding this comment.
Pull request overview
Adjusts the OAuth token endpoint’s rate-limit responses to use the correct HTTP status code (429) and a distinct error code, aligning /oauth/token with the plugin’s other OAuth endpoints and improving client UX/protocol signaling.
Changes:
- Update
/oauth/tokenrate-limit branches to return429witherror=rate_limitedinstead of400 invalid_request. - Extend
token_error()with an optional$statusparameter (defaulting to 400) to support non-400 errors. - Add a PHPUnit controller test for the new 429 rate-limit response and a changelog entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
includes/rest/oauth/class-token-controller.php |
Returns 429 for rate-limited token requests and adds configurable status support to token_error(). |
tests/phpunit/tests/includes/rest/oauth/class-test-token-controller.php |
Adds a test asserting the token endpoint returns 429 + rate_limited when throttled. |
.github/changelog/update-oauth-token-rate-limit-status |
Documents the behavior change in the changelog system. |
RFC 6749 §5.1 requires the same Cache-Control: no-store and Pragma: no-cache headers on token-endpoint error responses as on success responses. token_response() set them; token_error() did not. Add them. Pin REMOTE_ADDR in the rate-limit test so it exercises the >=20 cap branch (the path the test name claims) and not the fail-closed empty-IP branch — both happen to produce the same response shape today, but a future regression in either path should be caught distinctly. Also assert the new no-cache headers in the response.
There was a problem hiding this comment.
Pull request overview
This PR makes the OAuth /oauth/token endpoint return an HTTP 429 Too Many Requests response when rate limiting triggers, aligning it with other OAuth endpoints in the plugin and allowing clients to distinguish throttling from malformed requests.
Changes:
- Update
/oauth/tokenrate-limit responses to return429witherror=rate_limitedinstead of400 invalid_request. - Extend
token_error()to accept an optional HTTP status (defaulting to 400) and include required no-cache headers on error responses. - Add a PHPUnit controller test asserting the new 429 response shape.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
includes/rest/oauth/class-token-controller.php |
Changes rate-limit branches to return 429 and enhances token_error() to support non-400 statuses + no-cache headers. |
tests/phpunit/tests/includes/rest/oauth/class-test-token-controller.php |
Adds coverage for the per-IP cap branch returning 429 with rate_limited and asserts no-cache headers. |
.github/changelog/update-oauth-token-rate-limit-status |
Adds changelog entry documenting the behavioral change. |
The previous test pinned only $_SERVER['REMOTE_ADDR'], but get_client_ip() prefers the proxy headers first (CF-Connecting-IP, X-Forwarded-For, etc.). A stray header set by the runner or another test could flip which branch fires. - Snapshot every $_SERVER key get_client_ip() walks before the test, clear them all, then set REMOTE_ADDR (or leave unset for the new empty-IP test). Restore in a finally block. - Add test_token_rate_limited_returns_429_without_client_ip() to exercise the empty-IP fail-closed branch the previous PR introduced — it asserts 429 + the rate_limited error code and verifies no shared empty-IP transient is written.
There was a problem hiding this comment.
Pull request overview
Aligns the OAuth /oauth/token endpoint’s rate-limiting behavior with standard HTTP semantics by returning 429 Too Many Requests instead of overloading OAuth’s 400 invalid_request, matching the plugin’s other OAuth endpoints and improving client UX.
Changes:
- Update token endpoint rate-limit responses (both “no client IP” fail-closed and
>= 20/mincap) to return HTTP 429 witherror=rate_limited. - Extend
token_error()to accept an optional HTTP status (defaulting to 400) and ensure token error responses include RFC 6749 no-cache headers. - Add PHPUnit coverage for the 429 rate-limit behavior and headers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
includes/rest/oauth/class-token-controller.php |
Returns 429 + rate_limited on rate-limit branches; adds $status to token_error() and ensures no-cache headers on token errors. |
tests/phpunit/tests/includes/rest/oauth/class-test-token-controller.php |
Adds tests for both rate-limit branches and asserts 429 status, rate_limited error, and required cache headers. |
.github/changelog/update-oauth-token-rate-limit-status |
Documents the behavioral change in the changelog. |
Proposed changes:
/oauth/tokenendpoint's two rate-limit branches (no-IP fail-closed and the>=20/mincap) returned HTTP 400 with the OAutherror_code: invalid_requestbecause thetoken_error()helper was hard-wired to status 400. That confused two unrelated cases — "your request was malformed" and "you're being throttled" — into the same response shape./oauth/authorize,/oauth/clients,/oauth/revoke) already returns 429. The token endpoint was the odd one out.$statusparameter totoken_error()defaulting to 400 (existing callers unchanged) and use 429 with arate_limitederror code on both rate-limit branches. Adds a controller test asserting the new shape.This is a UX/protocol-correctness improvement — clients can distinguish "I have a coding error" from "I am being throttled" and cooperative tooling that watches for 429 will now see the rate-limit signal.
Other information:
Testing instructions:
/oauth/token21 times in a minute from the same IP. The 21st call should return429 {"error":"rate_limited", …}instead of400 {"error":"invalid_request", …}.429 rate_limited(was400 invalid_request).invalid_client,invalid_grant,unsupported_grant_type,invalid_requestfor missing code/refresh_token) continue to return 400 with their existing OAuth error codes.Changelog entry
Changelog Entry Details
Significance
Type
Message
Returned the standard rate-limit response from the OAuth token endpoint when too many requests are sent.