feat: Make 429 passthrough instead return 401 by sboily · Pull Request #1232 · ory/oathkeeper

sboily · 2025-06-28T05:12:16Z

Fix: Properly propagate 429 Too Many Requests responses instead of converting to 401

This pull request fixes an issue where Oathkeeper was incorrectly converting 429 (Too Many Requests) responses from upstream services into 401 (Unauthorized) errors. This behavior is problematic for API consumers as it conflates rate limiting with authentication failures, leading to incorrect client behavior.

The Problem

When an upstream service returns a 429 status code (rate limit exceeded), Oathkeeper was treating it as an authentication failure and returning 401. This causes several issues:

Incorrect client behavior: Clients receive 401 and attempt to re-authenticate instead of backing off
Loss of rate limit information: Important headers like Retry-After and rate limit details are lost
Poor developer experience: Developers see authentication errors when the real issue is rate limiting
Breaks HTTP semantics: 401 and 429 have completely different meanings in the HTTP specification

Changes Made

Modified error handling in errors_hydrator.go: Updated the DefaultHydrator to properly recognize and propagate 429 status codes from upstream responses instead of converting them to 401.
Preserved upstream response details: The fix ensures that:
- The original 429 status code is maintained
- Rate limiting headers (e.g., Retry-After, X-RateLimit-*) are preserved
- The detailed error response body from the upstream service is passed through to the client

Impact

This change improves API usability by:

Allowing clients to properly distinguish between authentication failures (401) and rate limiting (429)
Enabling correct retry behavior with exponential backoff for rate limits
Preserving detailed rate limit information for better debugging
Following HTTP standards and REST API best practices

Example

Before this fix:

{
  "error": {
    "code": 401,
    "status": "Unauthorized",
    "message": "Access credentials are invalid"
  }
}

After this fix:

{
  "error": {
    "code": 429,
    "status": "Too Many Requests",
    "reason": "{\"detail\":{\"error\":\"rate_limit_exceeded\",\"message\":\"API rate limit exceeded. You have made 71 requests in the current minute, which exceeds your limit of 70 requests per minute.\",\"limit_type\":\"minute\",\"current_usage\":71,\"limit\":70,\"retry_after_seconds\":5,\"reset_time\":1751085840,\"documentation\":\"https://docs.angany.ai/api/rate-limiting\"}}",
    "message": "Too many requests"
  }
}

No breaking changes are introduced by this patch.

Related issue(s)

This fixes a previously unknown bug where 429 responses were being incorrectly converted to 401 errors. The bug can be reproduced by:

Setting up Oathkeeper as a reverse proxy to an upstream service that implements rate limiting
Having the upstream service return a 429 status code when rate limits are exceeded
Observing that Oathkeeper returns a 401 (Unauthorized) error instead of propagating the 429

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change introduces a new feature.
I am following the contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security vulnerability.
I have added tests that prove my fix is effective or that my feature works.
I have added or changed the documentation.

Further Comments

This fix addresses a significant usability issue for developers building applications that consume APIs through Oathkeeper. Converting rate limit errors to authentication errors violates the principle of least surprise and makes it impossible for clients to implement proper retry logic.

The fix has been tested in a dev environment with high-throughput rate limiting scenarios (100+ requests/second) and correctly propagates 429 responses without any performance impact.

This change aligns Oathkeeper with HTTP standards (RFC 6585) and common API gateway practices where rate limiting and authentication are treated as distinct concerns with different error codes.

This PR is intent to have discussion about this issue, let me know if it incorrect to address it like this. There is also an issue filled here #1167.

CLAassistant · 2025-06-28T05:12:23Z

All committers have signed the CLA.

finsterwalder · 2025-09-30T17:44:15Z

@wewelll after you approved the changes, what is the next step to move this forward?
I would really like to see this fixed in Oathkeeper.

sboily · 2025-10-01T22:18:31Z

@finsterwalder i think i need to check the new tests required because they failed. But it's not clear to me if maintainers are interested about this PR.

finsterwalder · 2025-10-02T07:27:17Z

@sboily Thank you so much for you continuous effort to work on this!
hackerman clearly stated, that he thinks your approach is correct, so I think it's likely that it will be merged, once the checks are green.
Unfortunately I can't help, since I have no experience with Go. 😬

aeneasr

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

aeneasr · 2025-10-15T20:03:41Z

pipeline/authn/authenticator_cookie_session.go

 		return body, nil
+	} else if res.StatusCode == http.StatusTooManyRequests {
+		// Handle rate limiting - pass through the response
+		body, _ := io.ReadAll(res.Body)


I wonder if it would be possible to pass through these headers and the body? Also what about the other handlers with upstream code?

Good point! I checked the codebase and found other places that make upstream calls and would need similar 429 handling. As i said before, i was concentrated on my use case. What is your preference here, do i need to update the PR to support other places? I need to check better but seems i need to update 2 places for authenticators and 3 for authorizers.

About headers/body i can propose a modification if you want but we'll get probably a larger PR. So we have 2 options i think for this PR. 1. merge it as this to start a 429 support, where i understand from your comment it's probably not enough to have a complete full support or 2. maybe having a better specification about what we want to support here and how. I think you the best person to make this choice :).

For the other places, i did a new commit, let me know if it what you have in your mind for the other handlers.

aeneasr · 2025-10-15T20:03:57Z

pipeline/authn/authenticator_cookie_session.go

+		body, _ := io.ReadAll(res.Body)
+		err := helper.ErrTooManyRequests
+		if len(body) > 0 {
+			err = err.WithReasonf("%s", string(body))


The body may be some HTML, do we really want it in the error as JSON? It will look super strange

Hello @aeneasr,

I agree, embedding the raw body isn't ideal, i was just focus on my use case. What's your preference here:

Detect content-type and handle appropriately (parse JSON responses, skip HTML, include plain text)
or

Remove body passthrough entirely and use a simple hardcoded message

Let me know what you think would work best!

i think we should remove this body inclusion and move the x- headers to the response body

sboily · 2025-10-18T18:41:21Z

@sboily Thank you so much for you continuous effort to work on this! hackerman clearly stated, that he thinks your approach is correct, so I think it's likely that it will be merged, once the checks are green. Unfortunately I can't help, since I have no experience with Go. 😬

CI is green but i dont change my code 😅, i have just sync main.

sboily · 2026-02-13T14:47:13Z

Hello @aeneasr, any news for this PR, what can i do to help to merge it? Thank you :)

aeneasr

sorry for the long wait times, i'll now try to help get this over the line

aeneasr · 2026-02-26T09:22:17Z

pipeline/authn/authenticator_cookie_session.go

+			err = err.WithReasonf("%s", string(body))
+		}
+		// Pass through important rate limit headers
+		for _, header := range []string{"Retry-After", "X-RateLimit-Limit", "X-RateLimit-Remaining", "X-RateLimit-Reset", "X-RateLimit-Type"} {


I think oathkeeper should return these as header

aeneasr · 2026-02-26T09:22:41Z

pipeline/authn/authenticator_cookie_session.go

+		body, _ := io.ReadAll(res.Body)
+		err := helper.ErrTooManyRequests
+		if len(body) > 0 {
+			err = err.WithReasonf("%s", string(body))


i think we should remove this body inclusion and move the x- headers to the response body

aeneasr · 2026-03-02T12:13:13Z

During review, I found some shared mem access problems that would lead to side effects and race conditions in this pr. it will take longer to fix this up.

sboily requested review from a team and aeneasr as code owners June 28, 2025 05:12

wewelll approved these changes Aug 18, 2025

View reviewed changes

aeneasr reviewed Oct 15, 2025

View reviewed changes

wewelll mentioned this pull request Oct 16, 2025

feat: add configurable session cache to cookie_session authenticator #1244

Open

7 tasks

sboily force-pushed the make_429_transparent branch from 67f65e6 to 2f47f2a Compare November 3, 2025 22:21

sboily force-pushed the make_429_transparent branch from 2f47f2a to 6b4b1b2 Compare November 13, 2025 04:18

sboily force-pushed the make_429_transparent branch from 6b4b1b2 to 66ca2e0 Compare February 3, 2026 02:58

sboily added 2 commits February 13, 2026 09:46

feat: Make 429 passthrough instead return 401

87c3d2a

feat: add 429 handling to remaining upstream handlers

b44fb61

sboily force-pushed the make_429_transparent branch from 66ca2e0 to b44fb61 Compare February 13, 2026 14:46

aeneasr added the contribution label Feb 26, 2026

aeneasr reviewed Feb 26, 2026

View reviewed changes

github-actions bot removed the contribution label Feb 27, 2026

aeneasr closed this Mar 3, 2026

Conversation

sboily commented Jun 28, 2025

Fix: Properly propagate 429 Too Many Requests responses instead of converting to 401

The Problem

Changes Made

Impact

Example

Related issue(s)

Checklist

Further Comments

Uh oh!

CLAassistant commented Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

finsterwalder commented Sep 30, 2025

Uh oh!

sboily commented Oct 1, 2025

Uh oh!

finsterwalder commented Oct 2, 2025

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboily commented Oct 18, 2025

Uh oh!

sboily commented Feb 13, 2026

Uh oh!

aeneasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeneasr commented Mar 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Jun 28, 2025 •

edited

Loading