Skip to content

feat: Make 429 passthrough instead return 401 #1232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sboily wants to merge 2 commits into from
Closed

feat: Make 429 passthrough instead return 401 #1232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87c3d2a
feat: Make 429 passthrough instead return 401
sboily Jun 28, 2025
b44fb61
feat: add 429 handling to remaining upstream handlers
sboily Oct 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions helper/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ var (
CodeField: http.StatusBadRequest,
StatusField: http.StatusText(http.StatusBadRequest),
}
ErrTooManyRequests = &herodot.DefaultError{
ErrorField: "Too many requests",
CodeField: http.StatusTooManyRequests,
StatusField: http.StatusText(http.StatusTooManyRequests),
}
ErrUpstreamServiceNotAvailable = &herodot.DefaultError{
ErrorField: "The upstream service is not available",
CodeField: http.StatusServiceUnavailable,
Expand Down
14 changes: 14 additions & 0 deletions pipeline/authn/authenticator_cookie_session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,20 @@ func forwardRequestToSessionStore(client *http.Client, r *http.Request, cf Authe
return json.RawMessage{}, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to fetch cookie session context from remote: %+v", err))
}
return body, nil
} else if res.StatusCode == http.StatusTooManyRequests {
// Handle rate limiting - pass through the response
body, _ := io.ReadAll(res.Body)
Copy link
Copy Markdown
Member

@aeneasr aeneasr Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@sboily sboily Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :).

Copy link
Copy Markdown
Author

@sboily sboily Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

err := helper.ErrTooManyRequests
if len(body) > 0 {
err = err.WithReasonf("%s", string(body))
Copy link
Copy Markdown
Member

@aeneasr aeneasr Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@sboily sboily Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Member

@aeneasr aeneasr Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should remove this body inclusion and move the x- headers to the response 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"} {
Copy link
Copy Markdown
Member

@aeneasr aeneasr Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think oathkeeper should return these as header

if value := res.Header.Get(header); value != "" {
err = err.WithDetail(header, value)
}
}
return json.RawMessage{}, errors.WithStack(err)
} else {
return json.RawMessage{}, errors.WithStack(helper.ErrUnauthorized)
}
Expand Down
2 changes: 2 additions & 0 deletions pipeline/authn/authenticator_oauth2_client_credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,8 @@ func (a *AuthenticatorOAuth2ClientCredentials) Authenticate(r *http.Request, ses
if err != nil {
if rErr, ok := err.(*oauth2.RetrieveError); ok {
switch httpStatusCode := rErr.Response.StatusCode; httpStatusCode {
case http.StatusTooManyRequests:
return errors.Wrap(helper.ErrTooManyRequests, err.Error())
case http.StatusServiceUnavailable:
return errors.Wrap(helper.ErrUpstreamServiceNotAvailable, err.Error())
case http.StatusInternalServerError:
Expand Down
4 changes: 3 additions & 1 deletion pipeline/authn/authenticator_oauth2_introspection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,9 @@ func (a *AuthenticatorOAuth2Introspection) Authenticate(r *http.Request, session
}
defer resp.Body.Close() //nolint:errcheck

if resp.StatusCode != http.StatusOK {
if resp.StatusCode == http.StatusTooManyRequests {
return errors.WithStack(helper.ErrTooManyRequests)
} else if resp.StatusCode != http.StatusOK {
return errors.Errorf("Introspection returned status code %d but expected %d", resp.StatusCode, http.StatusOK)
}

Expand Down
4 changes: 3 additions & 1 deletion pipeline/authz/keto_engine_acp_ory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,9 @@ func (a *AuthorizerKetoEngineACPORY) Authorize(r *http.Request, session *authn.A
}
defer res.Body.Close() //nolint:errcheck // response body close failure not actionable

if res.StatusCode == http.StatusForbidden {
if res.StatusCode == http.StatusTooManyRequests {
return errors.WithStack(helper.ErrTooManyRequests)
} else if res.StatusCode == http.StatusForbidden {
return errors.WithStack(helper.ErrForbidden)
} else if res.StatusCode != http.StatusOK {
return errors.Errorf("expected status code %d but got %d", http.StatusOK, res.StatusCode)
Expand Down
4 changes: 3 additions & 1 deletion pipeline/authz/remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@ func (a *AuthorizerRemote) Authorize(r *http.Request, session *authn.Authenticat
}
defer res.Body.Close() //nolint:errcheck // body close errors ignored in tests and handlers

if res.StatusCode == http.StatusForbidden {
if res.StatusCode == http.StatusTooManyRequests {
return errors.WithStack(helper.ErrTooManyRequests)
} else if res.StatusCode == http.StatusForbidden {
return errors.WithStack(helper.ErrForbidden)
} else if res.StatusCode != http.StatusOK {
return errors.Errorf("expected status code %d but got %d", http.StatusOK, res.StatusCode)
Expand Down
4 changes: 3 additions & 1 deletion pipeline/authz/remote_json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,9 @@ func (a *AuthorizerRemoteJSON) Authorize(r *http.Request, session *authn.Authent
}
defer func() { _ = res.Body.Close() }()

if res.StatusCode == http.StatusForbidden {
if res.StatusCode == http.StatusTooManyRequests {
return errors.WithStack(helper.ErrTooManyRequests)
} else if res.StatusCode == http.StatusForbidden {
return errors.WithStack(helper.ErrForbidden)
} else if res.StatusCode != http.StatusOK {
return errors.Errorf("expected status code %d but got %d", http.StatusOK, res.StatusCode)
Expand Down
3 changes: 3 additions & 0 deletions pipeline/errors/error_json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/ory/x/httpx"

"github.com/ory/oathkeeper/driver/configuration"
"github.com/ory/oathkeeper/helper"
"github.com/ory/oathkeeper/pipeline"
)

Expand Down Expand Up @@ -56,6 +57,8 @@ func (a *ErrorJSON) Handle(w http.ResponseWriter, r *http.Request, config json.R
handleError = herodot.ErrUnauthorized.WithTrace(handleError)
case http.StatusBadRequest:
handleError = herodot.ErrBadRequest.WithTrace(handleError)
case http.StatusTooManyRequests:
handleError = helper.ErrTooManyRequests.WithTrace(handleError)
case http.StatusUnsupportedMediaType:
handleError = herodot.ErrUnsupportedMediaType.WithTrace(handleError)
case http.StatusConflict:
Expand Down
3 changes: 3 additions & 0 deletions proxy/request_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,9 @@ func (d *requestHandler) HandleRequest(r *http.Request, rl *rule.Rule) (session
case helper.ErrUnauthorized.ErrorField:
d.r.Logger().Info(err)
return nil, err
case helper.ErrTooManyRequests.ErrorField:
d.r.Logger().Info(err)
return nil, err
default:
d.r.Logger().WithError(err).
WithFields(fields).
Expand Down
Loading