fix: rate limit the OAuth2 endpoints (#29510)

Apply the login rate limit to OAuth2 authorization, token, revocation, registration, client configuration, and discovery endpoints, closing the brute-force protection gap (D6-01) and bounding database work.

- Limit requests per IP without a Coder session and per user otherwise.
- Use independent, fixed-key endpoint buckets to prevent path-based bypasses; unmatched routes consume no budget.
- Return HTTP 429 with `temporarily_unavailable` and a `message` matching `error_description` for dashboard/SDK compatibility.
- Preserve `X-Coder-Bypass-Ratelimit` and `--dangerous-disable-rate-limits`.
- Add documentation and tests for route coverage, variable paths, and endpoint/user isolation.

No migrations or new configuration.

Refs PLAT-624
This commit is contained in:
Bobby Ho
2026-09-22 21:02:42 +00:00
committed by GitHub
parent 83bef8a12d
commit fc707a15d1
4 changed files with 401 additions and 52 deletions
+47 -46
View File
@@ -1240,12 +1240,20 @@ func New(options *Options) *API {
// OAuth2 metadata endpoint for RFC 8414 discovery
r.Route("/.well-known/oauth-authorization-server", func(r chi.Router) {
r.Use(httpmw.RequireOAuth2Provider(oauth2ProviderEnabled))
r.Use(
httpmw.RequireOAuth2Provider(oauth2ProviderEnabled),
// Discovery carries no credential, so the limiter keys on the
// address rather than a user.
httpmw.RateLimitOAuth2(options.LoginRateLimit, time.Minute),
)
r.Get("/*", api.oauth2AuthorizationServerMetadata())
})
// OAuth2 protected resource metadata endpoint for RFC 9728 discovery
r.Route("/.well-known/oauth-protected-resource", func(r chi.Router) {
r.Use(httpmw.RequireOAuth2Provider(oauth2ProviderEnabled))
r.Use(
httpmw.RequireOAuth2Provider(oauth2ProviderEnabled),
httpmw.RateLimitOAuth2(options.LoginRateLimit, time.Minute),
)
r.Get("/*", api.oauth2ProtectedResourceMetadata())
})
@@ -1261,56 +1269,49 @@ func New(options *Options) *API {
// rejection carries no credential, so it needs none.
httpmw.NoStore,
)
r.Route("/authorize", func(r chi.Router) {
r.Use(
// Fetch the app as system for the authorize endpoint
httpmw.AsAuthzSystem(httpmw.ExtractOAuth2ProviderAppWithOAuth2Errors(options.Database)),
apiKeyMiddlewareRedirect,
)
// GET shows the consent page, POST processes the consent
r.Get("/", api.getOAuth2ProviderAppAuthorize())
r.Post("/", api.postOAuth2ProviderAppAuthorize())
})
r.Route("/tokens", func(r chi.Router) {
r.Use(
// Use OAuth2-compliant error responses for the tokens endpoint
httpmw.AsAuthzSystem(httpmw.ExtractOAuth2ProviderAppWithOAuth2Errors(options.Database)),
)
r.Group(func(r chi.Router) {
r.Use(apiKeyMiddleware)
// DELETE on /tokens is not part of the OAuth2 spec. It is our own
// route used to revoke permissions from an application. It is here for
// parity with POST on /tokens.
r.Delete("/", api.deleteOAuth2ProviderAppTokens())
})
// The POST /tokens endpoint will be called from an unauthorized client so
// we cannot require an API key.
r.Post("/", api.postOAuth2ProviderAppToken())
})
authorize := r.With(
httpmw.RateLimitOAuth2(options.LoginRateLimit, time.Minute),
// Fetch the app as system for the authorize endpoint
httpmw.AsAuthzSystem(httpmw.ExtractOAuth2ProviderAppWithOAuth2Errors(options.Database)),
apiKeyMiddlewareRedirect,
)
// GET shows the consent page, POST processes the consent
authorize.Get("/authorize", api.getOAuth2ProviderAppAuthorize())
authorize.Post("/authorize", api.postOAuth2ProviderAppAuthorize())
tokens := r.With(
httpmw.RateLimitOAuth2(options.LoginRateLimit, time.Minute),
// Use OAuth2-compliant error responses for the tokens endpoint
httpmw.AsAuthzSystem(httpmw.ExtractOAuth2ProviderAppWithOAuth2Errors(options.Database)),
)
// DELETE on /tokens is not part of the OAuth2 spec. It is our own
// route used to revoke permissions from an application. It is here for
// parity with POST on /tokens.
tokens.With(apiKeyMiddleware).Delete("/tokens", api.deleteOAuth2ProviderAppTokens())
// The POST /tokens endpoint will be called from an unauthorized client so
// we cannot require an API key.
tokens.Post("/tokens", api.postOAuth2ProviderAppToken())
// RFC 7009 Token Revocation Endpoint
r.Route("/revoke", func(r chi.Router) {
r.Use(
// RFC 7009 endpoint uses OAuth2 client authentication, not API key
httpmw.AsAuthzSystem(httpmw.ExtractOAuth2ProviderAppWithOAuth2Errors(options.Database)),
)
// POST /revoke is the standard OAuth2 token revocation endpoint per RFC 7009
r.Post("/", api.revokeOAuth2Token())
})
r.With(
httpmw.RateLimitOAuth2(options.LoginRateLimit, time.Minute),
// RFC 7009 endpoint uses OAuth2 client authentication, not API key
httpmw.AsAuthzSystem(httpmw.ExtractOAuth2ProviderAppWithOAuth2Errors(options.Database)),
).Post("/revoke", api.revokeOAuth2Token())
// RFC 7591 Dynamic Client Registration - Public endpoint
r.Post("/register", api.postOAuth2ClientRegistration())
r.With(httpmw.RateLimitOAuth2(options.LoginRateLimit, time.Minute)).
Post("/register", api.postOAuth2ClientRegistration())
// RFC 7592 Client Configuration Management - Protected by registration access token
r.Route("/clients/{client_id}", func(r chi.Router) {
r.Use(
// Middleware to validate registration access token
oauth2provider.RequireRegistrationAccessToken(api.Database),
)
r.Get("/", api.oauth2ClientConfiguration()) // Read client configuration
r.Put("/", api.putOAuth2ClientConfiguration()) // Update client configuration
r.Delete("/", api.deleteOAuth2ClientConfiguration()) // Delete client
})
clients := r.With(
httpmw.RateLimitOAuth2(options.LoginRateLimit, time.Minute),
// Middleware to validate registration access token
oauth2provider.RequireRegistrationAccessToken(api.Database),
)
clients.Get("/clients/{client_id}", api.oauth2ClientConfiguration()) // Read client configuration
clients.Put("/clients/{client_id}", api.putOAuth2ClientConfiguration()) // Update client configuration
clients.Delete("/clients/{client_id}", api.deleteOAuth2ClientConfiguration()) // Delete client
})
// Experimental routes are not guaranteed to be stable and may change at any time.
+55 -6
View File
@@ -22,6 +22,51 @@ import (
// RateLimit returns a handler that limits requests per-minute based
// on IP, endpoint, and user ID (if available).
func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler {
return rateLimitWithEndpointKey(count, window, keyByNormalizedEndpoint, nil)
}
// RateLimitOAuth2 returns a handler that limits requests per-minute based on
// the caller. Mount it on each route, since every call is a separate bucket.
//
// The key ignores the request path, which some of these routes vary, so a
// caller cannot respell a path to get a fresh bucket.
//
// A refusal reports temporarily_unavailable, the closest RFC 6749 code for
// throttling, so a client can parse it like the other OAuth2 errors.
func RateLimitOAuth2(count int, window time.Duration) func(http.Handler) http.Handler {
return rateLimitWithEndpointKey(count, window,
func(*http.Request) (string, error) {
return "oauth2", nil
},
func(rw http.ResponseWriter, r *http.Request) {
message := rateLimitMessage(count, window)
httpapi.Write(r.Context(), rw, http.StatusTooManyRequests, oauth2RateLimitError{
OAuth2Error: codersdk.OAuth2Error{
Error: codersdk.OAuth2ErrorCodeTemporarilyUnavailable,
ErrorDescription: message,
},
Message: message,
})
},
)
}
// oauth2RateLimitError is the body of a refused OAuth2 request. DELETE
// /oauth2/tokens shares the limiter with the RFC 6749 routes, and the
// dashboard and codersdk read message rather than error_description, so
// the body carries both. RFC 6749 allows extra members in an error response.
type oauth2RateLimitError struct {
codersdk.OAuth2Error
Message string `json:"message"`
}
// rateLimitMessage is the message a refused request reports, shared so the
// limiters built here say the same thing whatever the body shape.
func rateLimitMessage(count int, window time.Duration) string {
return fmt.Sprintf("You've been rate limited for sending more than %v requests in %v.", count, window)
}
func rateLimitWithEndpointKey(count int, window time.Duration, endpointKey func(*http.Request) (string, error), onLimited http.HandlerFunc) func(http.Handler) http.Handler {
// -1 is no rate limit
if count <= 0 {
return func(handler http.Handler) http.Handler {
@@ -29,6 +74,14 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler
}
}
if onLimited == nil {
onLimited = func(rw http.ResponseWriter, r *http.Request) {
httpapi.Write(r.Context(), rw, http.StatusTooManyRequests, codersdk.Response{
Message: rateLimitMessage(count, window),
})
}
}
return httprate.Limit(
count,
window,
@@ -86,12 +139,8 @@ func RateLimit(count int, window time.Duration) func(http.Handler) http.Handler
"%q provided but user is not %v",
codersdk.BypassRatelimitHeader, rbac.RoleOwner(),
)
}, keyByNormalizedEndpoint),
httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) {
httpapi.Write(r.Context(), w, http.StatusTooManyRequests, codersdk.Response{
Message: fmt.Sprintf("You've been rate limited for sending more than %v requests in %v.", count, window),
})
}),
}, endpointKey),
httprate.WithLimitHandler(onLimited),
)
}
+290
View File
@@ -0,0 +1,290 @@
package oauth2provider_test
import (
"context"
"encoding/json"
"io"
"net/http"
"net/url"
"strings"
"testing"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/oauth2provider/oauth2providertest"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/testutil"
)
func TestOAuth2RateLimit(t *testing.T) {
t.Parallel()
const rateLimit = 3
newServer := func(t *testing.T) (*codersdk.Client, codersdk.CreateFirstUserResponse, string) {
client := coderdtest.New(t, &coderdtest.Options{
LoginRateLimit: rateLimit,
})
owner := coderdtest.CreateFirstUser(t, client)
return client, owner, client.URL.String()
}
requireLimited := func(t *testing.T, send func() *http.Response, wantStatus int) {
t.Helper()
for i := range rateLimit {
resp := send()
_ = resp.Body.Close()
require.Equal(t, wantStatus, resp.StatusCode, "request %d should be inside the limit", i+1)
}
resp := send()
defer resp.Body.Close()
require.Equal(t, http.StatusTooManyRequests, resp.StatusCode, "request %d should be rate limited", rateLimit+1)
body, err := io.ReadAll(resp.Body)
require.NoError(t, err)
var oauthErr codersdk.OAuth2Error
require.NoError(t, json.Unmarshal(body, &oauthErr), "a refusal should carry an RFC 6749 body")
require.Equal(t, codersdk.OAuth2ErrorCodeTemporarilyUnavailable, oauthErr.Error)
// The dashboard and codersdk read message, not error_description.
var apiErr codersdk.Response
require.NoError(t, json.Unmarshal(body, &apiErr))
require.Equal(t, oauthErr.ErrorDescription, apiErr.Message)
}
t.Run("Authorize", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
client, _, baseURL := newServer(t)
app, _ := oauth2providertest.CreateTestOAuth2App(t, client)
_, challenge := oauth2providertest.GeneratePKCE(t)
uri := authorizeURL(baseURL, app.ID.String(), challenge)
requireLimited(t, func() *http.Response {
return doRequest(ctx, t, http.MethodGet, uri, nil)
}, http.StatusSeeOther)
resp := doRequest(ctx, t, http.MethodGet, authorizeURL(baseURL, uuid.NewString(), challenge), nil)
_ = resp.Body.Close()
require.Equal(t, http.StatusTooManyRequests, resp.StatusCode, "an unknown client should be refused before the app lookup")
})
t.Run("Tokens", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
client, _, baseURL := newServer(t)
app, _ := oauth2providertest.CreateTestOAuth2App(t, client)
form := url.Values{}
form.Set("grant_type", "refresh_token")
form.Set("refresh_token", "coder_wrongprefix_wrongsecret")
form.Set("client_id", app.ID.String())
form.Set("client_secret", "coder_wrongprefix_wrongsecret")
requireLimited(t, func() *http.Response {
return doRequest(ctx, t, http.MethodPost, baseURL+"/oauth2/tokens", strings.NewReader(form.Encode()), formContentType)
}, http.StatusUnauthorized)
form.Set("client_id", uuid.NewString())
resp := doRequest(ctx, t, http.MethodPost, baseURL+"/oauth2/tokens", strings.NewReader(form.Encode()), formContentType)
_ = resp.Body.Close()
require.Equal(t, http.StatusTooManyRequests, resp.StatusCode, "an unknown client should be refused before the app lookup")
})
t.Run("Revoke", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
client, _, baseURL := newServer(t)
app, _ := oauth2providertest.CreateTestOAuth2App(t, client)
form := url.Values{}
form.Set("token", "coder_wrongprefix_wrongsecret")
form.Set("client_id", app.ID.String())
form.Set("client_secret", "coder_wrongprefix_wrongsecret")
requireLimited(t, func() *http.Response {
return doRequest(ctx, t, http.MethodPost, baseURL+"/oauth2/revoke", strings.NewReader(form.Encode()), formContentType)
}, http.StatusUnauthorized)
form.Set("client_id", uuid.NewString())
resp := doRequest(ctx, t, http.MethodPost, baseURL+"/oauth2/revoke", strings.NewReader(form.Encode()), formContentType)
_ = resp.Body.Close()
require.Equal(t, http.StatusTooManyRequests, resp.StatusCode, "an unknown client should be refused before the app lookup")
})
t.Run("Register", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
client, _, baseURL := newServer(t)
oauth2providertest.EnableDCR(t, client)
body, err := json.Marshal(codersdk.OAuth2ClientRegistrationRequest{
RedirectURIs: []string{"https://example.com/callback"},
})
require.NoError(t, err)
requireLimited(t, func() *http.Response {
return doRequest(ctx, t, http.MethodPost, baseURL+"/oauth2/register", strings.NewReader(string(body)), jsonContentType)
}, http.StatusCreated)
})
t.Run("ClientConfigurationVaryingClientID", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
_, _, baseURL := newServer(t)
requireLimited(t, func() *http.Response {
uri := baseURL + "/oauth2/clients/" + uuid.NewString()
return doRequest(ctx, t, http.MethodGet, uri, nil, bearer("wrongtoken"))
}, http.StatusUnauthorized)
})
t.Run("AuthorizationServerMetadata", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
_, _, baseURL := newServer(t)
requireLimited(t, func() *http.Response {
return doRequest(ctx, t, http.MethodGet, baseURL+"/.well-known/oauth-authorization-server", nil)
}, http.StatusOK)
})
t.Run("ProtectedResourceMetadata", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
_, _, baseURL := newServer(t)
requireLimited(t, func() *http.Response {
return doRequest(ctx, t, http.MethodGet, baseURL+"/.well-known/oauth-protected-resource", nil)
}, http.StatusOK)
})
t.Run("ProtectedResourceMetadataVaryingSuffix", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
_, _, baseURL := newServer(t)
requireLimited(t, func() *http.Response {
uri := baseURL + "/.well-known/oauth-protected-resource/" + uuid.NewString()
return doRequest(ctx, t, http.MethodGet, uri, nil)
}, http.StatusOK)
})
t.Run("SeparateBucketPerEndpoint", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
client, _, baseURL := newServer(t)
app, _ := oauth2providertest.CreateTestOAuth2App(t, client)
_, challenge := oauth2providertest.GeneratePKCE(t)
authorize := authorizeURL(baseURL, app.ID.String(), challenge)
for i := range rateLimit + 1 {
resp := doRequest(ctx, t, http.MethodGet, authorize, nil)
_ = resp.Body.Close()
if i == rateLimit {
require.Equal(t, http.StatusTooManyRequests, resp.StatusCode, "authorize should run out of budget")
}
}
form := url.Values{}
form.Set("token", "coder_wrongprefix_wrongsecret")
form.Set("client_id", app.ID.String())
form.Set("client_secret", "coder_wrongprefix_wrongsecret")
resp := doRequest(ctx, t, http.MethodPost, baseURL+"/oauth2/revoke", strings.NewReader(form.Encode()), formContentType)
_ = resp.Body.Close()
require.Equal(t, http.StatusUnauthorized, resp.StatusCode, "revoke should have its own budget")
})
t.Run("UnmatchedPathKeepsBudget", func(t *testing.T) {
t.Parallel()
type endpoint struct {
send func(ctx context.Context, t *testing.T, baseURL, clientID string) *http.Response
wantStatus int
}
authorize := endpoint{
send: func(ctx context.Context, t *testing.T, baseURL, clientID string) *http.Response {
_, challenge := oauth2providertest.GeneratePKCE(t)
return doRequest(ctx, t, http.MethodGet, authorizeURL(baseURL, clientID, challenge), nil)
},
wantStatus: http.StatusSeeOther,
}
tokens := endpoint{
send: func(ctx context.Context, t *testing.T, baseURL, clientID string) *http.Response {
form := url.Values{}
form.Set("grant_type", "refresh_token")
form.Set("refresh_token", "coder_wrongprefix_wrongsecret")
form.Set("client_id", clientID)
form.Set("client_secret", "coder_wrongprefix_wrongsecret")
return doRequest(ctx, t, http.MethodPost, baseURL+"/oauth2/tokens", strings.NewReader(form.Encode()), formContentType)
},
wantStatus: http.StatusUnauthorized,
}
revoke := endpoint{
send: func(ctx context.Context, t *testing.T, baseURL, clientID string) *http.Response {
form := url.Values{}
form.Set("token", "coder_wrongprefix_wrongsecret")
form.Set("client_id", clientID)
form.Set("client_secret", "coder_wrongprefix_wrongsecret")
return doRequest(ctx, t, http.MethodPost, baseURL+"/oauth2/revoke", strings.NewReader(form.Encode()), formContentType)
},
wantStatus: http.StatusUnauthorized,
}
clients := endpoint{
send: func(ctx context.Context, t *testing.T, baseURL, _ string) *http.Response {
return doRequest(ctx, t, http.MethodGet, baseURL+"/oauth2/clients/"+uuid.NewString(), nil, bearer("wrongtoken"))
},
wantStatus: http.StatusUnauthorized,
}
for _, tc := range []struct {
name string
method string
junkPath string
endpoint endpoint
}{
{name: "OutsideAnyEndpoint", method: http.MethodGet, junkPath: "/oauth2/does-not-exist", endpoint: revoke},
{name: "AuthorizeSuffix", method: http.MethodGet, junkPath: "/oauth2/authorize/does-not-exist", endpoint: authorize},
{name: "TokensSuffix", method: http.MethodPost, junkPath: "/oauth2/tokens/does-not-exist", endpoint: tokens},
{name: "RevokeSuffix", method: http.MethodPost, junkPath: "/oauth2/revoke/does-not-exist", endpoint: revoke},
{name: "RevokeWrongMethod", method: http.MethodGet, junkPath: "/oauth2/revoke", endpoint: revoke},
{name: "ClientsSuffix", method: http.MethodGet, junkPath: "/oauth2/clients/" + uuid.NewString() + "/does-not-exist", endpoint: clients},
} {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
client, _, baseURL := newServer(t)
app, _ := oauth2providertest.CreateTestOAuth2App(t, client)
for i := range rateLimit + 1 {
resp := doRequest(ctx, t, tc.method, baseURL+tc.junkPath, nil)
_ = resp.Body.Close()
require.NotEqual(t, http.StatusTooManyRequests, resp.StatusCode, "junk request %d should not be counted", i+1)
}
requireLimited(t, func() *http.Response {
return tc.endpoint.send(ctx, t, baseURL, app.ID.String())
}, tc.endpoint.wantStatus)
})
}
})
t.Run("DeleteTokensLimitedPerUser", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
client, owner, baseURL := newServer(t)
other, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
app, _ := oauth2providertest.CreateTestOAuth2App(t, client)
uri := baseURL + "/oauth2/tokens?client_id=" + app.ID.String()
requireLimited(t, func() *http.Response {
return doRequest(ctx, t, http.MethodDelete, uri, nil, sessionToken(client))
}, http.StatusNoContent)
resp := doRequest(ctx, t, http.MethodDelete, uri, nil, sessionToken(other))
_ = resp.Body.Close()
require.Equal(t, http.StatusNoContent, resp.StatusCode, "a second user should have its own bucket")
})
}
@@ -721,6 +721,15 @@ These rules apply to every entry in `redirect_uris`, not only the first one.
plane, custom URI schemes for native apps (`myapp://`) are permitted, and
public clients additionally cannot use `mailto:`, `tel:`, or `sms:`
- **Rotate secrets**: Periodically rotate client secrets using the management API
- **Rate limits**: every `/oauth2` endpoint and both `/.well-known` discovery
endpoints draw on the login rate limit of 60 requests per minute. Each
endpoint counts on its own, so a caller that exhausts one can still reach the
others. Requests with no Coder session are counted per IP address, and the
rest are counted per user. A caller over the limit receives HTTP 429 with a
`temporarily_unavailable` error body. The limit is fixed. Running the
deployment with `--dangerous-disable-rate-limits` turns it off, and a user
with the Owner role can bypass it on a single request with the
`X-Coder-Bypass-Ratelimit` header
- **Refresh tokens are not self-sufficient**: a confidential client must present
its `client_secret` to refresh or revoke, so a leaked token alone cannot mint
new access tokens or end another client's session