From 49bc7dc5fee66f4a0474314d10ca78cc34cfc734 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Fri, 14 Aug 2026 17:27:50 +0000 Subject: [PATCH 1/3] fix: bound OAuth2 form bodies and report 413 per RFC 6749 POST /oauth2/tokens and POST /oauth2/revoke authenticate the client from the request body, so they carry no API key middleware and r.ParseForm ran before any authorization decision. net/http caps an unwrapped urlencoded body at its own 10 MiB maxFormSize, 2.5x the ceiling every other endpoint carries, and /oauth2 is mounted outside apiRateLimiter. The middleware now installs a MaxBytesReader at DefaultMaxRequestBodyBytes ahead of every reader, which net/http defers to when it finds one. tokens.go and revoke.go translate the same error, since they perform the first read when client_id arrived in the query string and the middleware had no reason to parse. These endpoints answer under RFC 6749 rather than through httpapi.Read, so the rejection is written as an invalid_request. RFC 6749 defines no error code for a transport rejection, so that is the closest compliant framing. The middleware also discarded its ParseForm error, so an oversized body reported the client_id it may well have carried as missing. That is fixed here: a size failure is now reported as one, and any other parse failure still falls through, since the client_id may arrive through HTTP Basic. --- coderd/apidoc/docs.go | 57 ++++++++++++++++ coderd/apidoc/swagger.json | 57 ++++++++++++++++ coderd/httpapi/httpapi.go | 14 ++++ coderd/httpmw/oauth2.go | 38 ++++++++++- coderd/httpmw/oauth2_test.go | 49 ++++++++++++++ coderd/oauth2.go | 2 + coderd/oauth2provider/formlimit_test.go | 88 +++++++++++++++++++++++++ coderd/oauth2provider/revoke.go | 8 +++ coderd/oauth2provider/tokens.go | 8 +++ docs/reference/api/enterprise.md | 20 ++++-- docs/reference/api/schemas.md | 32 +++++++++ 11 files changed, 365 insertions(+), 8 deletions(-) create mode 100644 coderd/oauth2provider/formlimit_test.go diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index f64d82782672c..306507ee2070f 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -14952,6 +14952,12 @@ const docTemplate = `{ "responses": { "200": { "description": "Token successfully revoked" + }, + "413": { + "description": "Request Entity Too Large", + "schema": { + "$ref": "#/definitions/codersdk.OAuth2Error" + } } } } @@ -15012,6 +15018,12 @@ const docTemplate = `{ "schema": { "$ref": "#/definitions/oauth2.Token" } + }, + "413": { + "description": "Request Entity Too Large", + "schema": { + "$ref": "#/definitions/codersdk.OAuth2Error" + } } } }, @@ -22137,6 +22149,51 @@ const docTemplate = `{ } } }, + "codersdk.OAuth2Error": { + "type": "object", + "properties": { + "error": { + "$ref": "#/definitions/codersdk.OAuth2ErrorCode" + }, + "error_description": { + "type": "string" + }, + "error_uri": { + "type": "string" + } + } + }, + "codersdk.OAuth2ErrorCode": { + "type": "string", + "enum": [ + "invalid_request", + "invalid_client", + "invalid_grant", + "unauthorized_client", + "unsupported_grant_type", + "invalid_scope", + "access_denied", + "unsupported_response_type", + "server_error", + "temporarily_unavailable", + "unsupported_token_type", + "invalid_target" + ], + "x-enum-varnames": [ + "OAuth2ErrorCodeInvalidRequest", + "OAuth2ErrorCodeInvalidClient", + "OAuth2ErrorCodeInvalidGrant", + "OAuth2ErrorCodeUnauthorizedClient", + "OAuth2ErrorCodeUnsupportedGrantType", + "OAuth2ErrorCodeInvalidScope", + "OAuth2ErrorCodeAccessDenied", + "OAuth2ErrorCodeUnsupportedResponseType", + "OAuth2ErrorCodeServerError", + "OAuth2ErrorCodeTemporarilyUnavailable", + "OAuth2ErrorCodeUnsupportedTokenType", + "OAuth2ErrorCodeInvalidTarget" + ] + }, "codersdk.OAuth2GithubConfig": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index b98b8857a5ea1..b1845c24fc467 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -13276,6 +13276,12 @@ "responses": { "200": { "description": "Token successfully revoked" + }, + "413": { + "description": "Request Entity Too Large", + "schema": { + "$ref": "#/definitions/codersdk.OAuth2Error" + } } } } @@ -13332,6 +13338,12 @@ "schema": { "$ref": "#/definitions/oauth2.Token" } + }, + "413": { + "description": "Request Entity Too Large", + "schema": { + "$ref": "#/definitions/codersdk.OAuth2Error" + } } } }, @@ -20201,6 +20213,51 @@ } } }, + "codersdk.OAuth2Error": { + "type": "object", + "properties": { + "error": { + "$ref": "#/definitions/codersdk.OAuth2ErrorCode" + }, + "error_description": { + "type": "string" + }, + "error_uri": { + "type": "string" + } + } + }, + "codersdk.OAuth2ErrorCode": { + "type": "string", + "enum": [ + "invalid_request", + "invalid_client", + "invalid_grant", + "unauthorized_client", + "unsupported_grant_type", + "invalid_scope", + "access_denied", + "unsupported_response_type", + "server_error", + "temporarily_unavailable", + "unsupported_token_type", + "invalid_target" + ], + "x-enum-varnames": [ + "OAuth2ErrorCodeInvalidRequest", + "OAuth2ErrorCodeInvalidClient", + "OAuth2ErrorCodeInvalidGrant", + "OAuth2ErrorCodeUnauthorizedClient", + "OAuth2ErrorCodeUnsupportedGrantType", + "OAuth2ErrorCodeInvalidScope", + "OAuth2ErrorCodeAccessDenied", + "OAuth2ErrorCodeUnsupportedResponseType", + "OAuth2ErrorCodeServerError", + "OAuth2ErrorCodeTemporarilyUnavailable", + "OAuth2ErrorCodeUnsupportedTokenType", + "OAuth2ErrorCodeInvalidTarget" + ] + }, "codersdk.OAuth2GithubConfig": { "type": "object", "properties": { diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index d071ca8931989..1da178a6674f7 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -552,3 +552,17 @@ func WriteOAuth2Error(ctx context.Context, rw http.ResponseWriter, status int, e ErrorDescription: description, }) } + +// WriteOAuth2RequestTooLarge reports a request body over limit as an RFC 6749 +// error, for the OAuth2 endpoints that read a form body rather than decoding +// through Read. RFC 6749 defines no error code for a transport rejection, so +// invalid_request is the closest compliant framing. +// +// The limit is the one carried by the *http.MaxBytesError that tripped, so a +// caller of this function reports the bound that actually applied rather than +// the one it assumes applied. +func WriteOAuth2RequestTooLarge(ctx context.Context, rw http.ResponseWriter, limit int64) { + RecordRequestBodyLimit(ctx, limit) + WriteOAuth2Error(ctx, rw, http.StatusRequestEntityTooLarge, codersdk.OAuth2ErrorCodeInvalidRequest, + fmt.Sprintf("Maximum request body size is %d bytes.", limit)) +} diff --git a/coderd/httpmw/oauth2.go b/coderd/httpmw/oauth2.go index fccc89642c000..1031ba6255571 100644 --- a/coderd/httpmw/oauth2.go +++ b/coderd/httpmw/oauth2.go @@ -2,6 +2,7 @@ package httpmw import ( "context" + "errors" "fmt" "net" "net/http" @@ -374,6 +375,7 @@ type errorWriter interface { writeMissingClientID(ctx context.Context, rw http.ResponseWriter) writeInvalidClientID(ctx context.Context, rw http.ResponseWriter, err error) writeClientNotFound(ctx context.Context, rw http.ResponseWriter) + writeRequestTooLarge(ctx context.Context, rw http.ResponseWriter, limit int64) } // codersdkErrorWriter writes standard codersdk errors for general API endpoints @@ -402,6 +404,14 @@ func (*codersdkErrorWriter) writeClientNotFound(ctx context.Context, rw http.Res }) } +func (*codersdkErrorWriter) writeRequestTooLarge(ctx context.Context, rw http.ResponseWriter, limit int64) { + httpapi.RecordRequestBodyLimit(ctx, limit) + httpapi.Write(ctx, rw, http.StatusRequestEntityTooLarge, codersdk.Response{ + Message: "Request body too large.", + Detail: fmt.Sprintf("Maximum request body size is %d bytes.", limit), + }) +} + // oauth2ErrorWriter writes OAuth2-compliant errors for OAuth2 endpoints type oauth2ErrorWriter struct{} @@ -417,6 +427,10 @@ func (*oauth2ErrorWriter) writeClientNotFound(ctx context.Context, rw http.Respo httpapi.WriteOAuth2Error(ctx, rw, http.StatusUnauthorized, codersdk.OAuth2ErrorCodeInvalidClient, "The client credentials are invalid") } +func (*oauth2ErrorWriter) writeRequestTooLarge(ctx context.Context, rw http.ResponseWriter, limit int64) { + httpapi.WriteOAuth2RequestTooLarge(ctx, rw, limit) +} + // extractOAuth2ProviderAppBase is the internal implementation that uses the strategy pattern // instead of a control flag to handle different error formats. func extractOAuth2ProviderAppBase(db database.Store, errWriter errorWriter) func(http.Handler) http.Handler { @@ -424,6 +438,18 @@ func extractOAuth2ProviderAppBase(db database.Store, errWriter errorWriter) func return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() + // POST /oauth2/tokens and POST /oauth2/revoke authenticate the + // client from the request body, so they cannot require an API key + // and the body below is read before any authorization decision. + // Bound it here, ahead of every reader: r.ParseForm below, and the + // handlers downstream, all read through this wrap. + // + // net/http caps an unwrapped urlencoded body at its own 10 MiB + // maxFormSize, which is 2.5x the ceiling every other endpoint + // carries. It defers to a *http.MaxBytesReader when it finds one, + // so installing one replaces that cap with this one. + r.Body = http.MaxBytesReader(rw, r.Body, httpapi.DefaultMaxRequestBodyBytes) + // App can come from a URL param, query param, or form value. paramID := "app" var appID uuid.UUID @@ -441,9 +467,19 @@ func extractOAuth2ProviderAppBase(db database.Store, errWriter errorWriter) func paramAppID := r.URL.Query().Get("client_id") if paramAppID == "" { // Check the form params! - if r.ParseForm() == nil { + err := r.ParseForm() + if maxBytesErr, ok := errors.AsType[*http.MaxBytesError](err); ok { + // An oversized body is a size failure and is reported as + // one. Falling through would report the client_id this + // body may well have carried as missing. + errWriter.writeRequestTooLarge(ctx, rw, maxBytesErr.Limit) + return + } + if err == nil { paramAppID = r.Form.Get("client_id") } + // Any other parse failure is not fatal here: the client_id + // may still arrive through HTTP Basic below. } if paramAppID == "" { // RFC 6749 §2.3.1: confidential clients may authenticate via diff --git a/coderd/httpmw/oauth2_test.go b/coderd/httpmw/oauth2_test.go index 6638194ab3acc..31653a558ff5d 100644 --- a/coderd/httpmw/oauth2_test.go +++ b/coderd/httpmw/oauth2_test.go @@ -2,15 +2,21 @@ package httpmw_test import ( "context" + "encoding/json" "net/http" "net/http/httptest" "net/url" + "strconv" + "strings" "testing" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" @@ -369,3 +375,46 @@ func (p *exchangeAssertingProvider) Exchange(_ context.Context, _ string, opts . func (*exchangeAssertingProvider) TokenSource(_ context.Context, _ *oauth2.Token) oauth2.TokenSource { return nil } + +// TestExtractOAuth2ProviderAppBodyTooLarge covers the pre-authentication body +// read on POST /oauth2/tokens and POST /oauth2/revoke. Those routes authenticate +// the client from the request body, so they carry no API key middleware and the +// form parse below runs before any authorization decision. +// +// net/http would otherwise cap the body at its own 10 MiB maxFormSize, which is +// larger than the ceiling every other endpoint carries. The rejection is +// asserted in the RFC 6749 error shape because a client that parses that shape +// cannot read a codersdk.Response, so the status alone is not sufficient. +func TestExtractOAuth2ProviderAppBodyTooLarge(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + + // Padding a valid form past the limit leaves size as the only reason to + // reject the request. client_id is present in the body, so a fallthrough to + // the missing-client_id path would be observable as a 400. + oversized := "client_id=" + uuid.NewString() + "&pad=" + + strings.Repeat("a", httpapi.DefaultMaxRequestBodyBytes) + require.Greater(t, len(oversized), httpapi.DefaultMaxRequestBodyBytes) + + var nextCalled bool + handler := httpmw.ExtractOAuth2ProviderAppWithOAuth2Errors(db)( + http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + nextCalled = true + }), + ) + + r := httptest.NewRequest(http.MethodPost, "/oauth2/tokens", strings.NewReader(oversized)) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rw := httptest.NewRecorder() + + handler.ServeHTTP(rw, r) + + require.Equal(t, http.StatusRequestEntityTooLarge, rw.Code) + require.False(t, nextCalled, "handler ran on an oversized body") + + var errResp codersdk.OAuth2Error + require.NoError(t, json.Unmarshal(rw.Body.Bytes(), &errResp)) + require.Equal(t, codersdk.OAuth2ErrorCodeInvalidRequest, errResp.Error) + require.Contains(t, errResp.ErrorDescription, strconv.Itoa(httpapi.DefaultMaxRequestBodyBytes)) +} diff --git a/coderd/oauth2.go b/coderd/oauth2.go index 2e083eeca63f3..73595f19a2606 100644 --- a/coderd/oauth2.go +++ b/coderd/oauth2.go @@ -152,6 +152,7 @@ func (api *API) postOAuth2ProviderAppAuthorize() http.HandlerFunc { // @Param refresh_token formData string false "Refresh token, required if grant_type=refresh_token" // @Param grant_type formData codersdk.OAuth2ProviderGrantType true "Grant type" // @Success 200 {object} oauth2.Token +// @Failure 413 {object} codersdk.OAuth2Error // @Router /oauth2/tokens [post] func (api *API) postOAuth2ProviderAppToken() http.HandlerFunc { return oauth2provider.Tokens(api.Database, api.DeploymentValues.Sessions) @@ -176,6 +177,7 @@ func (api *API) deleteOAuth2ProviderAppTokens() http.HandlerFunc { // @Param token formData string true "The token to revoke" // @Param token_type_hint formData string false "Hint about token type (access_token or refresh_token)" // @Success 200 "Token successfully revoked" +// @Failure 413 {object} codersdk.OAuth2Error // @Router /oauth2/revoke [post] func (api *API) revokeOAuth2Token() http.HandlerFunc { return oauth2provider.RevokeToken(api.Database, api.Logger) diff --git a/coderd/oauth2provider/formlimit_test.go b/coderd/oauth2provider/formlimit_test.go new file mode 100644 index 0000000000000..9095454fe8ee8 --- /dev/null +++ b/coderd/oauth2provider/formlimit_test.go @@ -0,0 +1,88 @@ +package oauth2provider_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "cdr.dev/slog/v3/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/oauth2provider" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" +) + +// TestFormEndpointsBodyTooLarge covers the form-parsing OAuth2 endpoints when +// client_id arrives in the query string. +// +// ExtractOAuth2ProviderAppWithOAuth2Errors installs the body bound, but it +// parses the form only when client_id is absent from the query. When client_id +// is present the middleware resolves the app without touching the body, so the +// handler's own r.ParseForm is the first read and the bound trips there. These +// endpoints authenticate the client from the body and therefore carry no API key +// middleware, so this read is still pre-authentication. +func TestFormEndpointsBodyTooLarge(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + path string + handler func(database.Store) http.Handler + }{ + { + name: "Tokens", + path: "/oauth2/tokens", + handler: func(db database.Store) http.Handler { + return oauth2provider.Tokens(db, codersdk.SessionLifetime{}) + }, + }, + { + name: "Revoke", + path: "/oauth2/revoke", + handler: func(db database.Store) http.Handler { + return oauth2provider.RevokeToken(db, slogtest.Make(t, nil)) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + db, _ := dbtestutil.NewDB(t) + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{}) + + handler := httpmw.ExtractOAuth2ProviderAppWithOAuth2Errors(db)(tc.handler(db)) + + // Padded past the limit so size is the only reason to reject. The + // form itself is well formed, so a rejection for any other reason + // would carry a different error code. + body := "grant_type=authorization_code&token=abc&pad=" + + strings.Repeat("a", httpapi.DefaultMaxRequestBodyBytes) + require.Greater(t, len(body), httpapi.DefaultMaxRequestBodyBytes) + + r := httptest.NewRequest(http.MethodPost, tc.path+"?client_id="+app.ID.String(), + strings.NewReader(body)).WithContext(ctx) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rw := httptest.NewRecorder() + + handler.ServeHTTP(rw, r) + + require.Equal(t, http.StatusRequestEntityTooLarge, rw.Code, + "response body: %s", rw.Body.String()) + + var errResp codersdk.OAuth2Error + require.NoError(t, json.Unmarshal(rw.Body.Bytes(), &errResp)) + require.Equal(t, codersdk.OAuth2ErrorCodeInvalidRequest, errResp.Error) + require.Contains(t, errResp.ErrorDescription, strconv.Itoa(httpapi.DefaultMaxRequestBodyBytes)) + }) + } +} diff --git a/coderd/oauth2provider/revoke.go b/coderd/oauth2provider/revoke.go index bba2757775d56..844152e36f1d4 100644 --- a/coderd/oauth2provider/revoke.go +++ b/coderd/oauth2provider/revoke.go @@ -68,6 +68,14 @@ func RevokeToken(db database.Store, logger slog.Logger) http.HandlerFunc { req, err := extractRevocationRequest(r) if err != nil { + // ExtractOAuth2ProviderAppWithOAuth2Errors bounds the body, but it + // parses the form only when client_id is absent from the query + // string. When it is present, extractRevocationRequest performs the + // first read and the bound trips here rather than in the middleware. + if maxBytesErr, ok := errors.AsType[*http.MaxBytesError](err); ok { + httpapi.WriteOAuth2RequestTooLarge(ctx, rw, maxBytesErr.Limit) + return + } httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error()) return } diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index 1beedf8cc3535..b53cc7d973194 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -149,6 +149,14 @@ func Tokens(db database.Store, lifetimes codersdk.SessionLifetime) http.HandlerF req, validationErrs, err := extractTokenRequest(r, callbackURL) if err != nil { + // ExtractOAuth2ProviderAppWithOAuth2Errors bounds the body, but it + // parses the form only when client_id is absent from the query + // string. When it is present, extractTokenRequest performs the first + // read and the bound trips here rather than in the middleware. + if maxBytesErr, ok := errors.AsType[*http.MaxBytesError](err); ok { + httpapi.WriteOAuth2RequestTooLarge(ctx, rw, maxBytesErr.Limit) + return + } if errors.Is(err, errConflictingClientAuth) { httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, "Conflicting client credentials between Authorization header and request body") return diff --git a/docs/reference/api/enterprise.md b/docs/reference/api/enterprise.md index a6c7114b16252..6340120bbd7c4 100644 --- a/docs/reference/api/enterprise.md +++ b/docs/reference/api/enterprise.md @@ -5118,7 +5118,7 @@ curl -X POST http://coder-server:8080/oauth2/register \ ```sh # Example request using curl curl -X POST http://coder-server:8080/oauth2/revoke \ - + -H 'Accept: */*' ``` `POST /oauth2/revoke` @@ -5141,11 +5141,16 @@ token_type_hint: string | `» token` | body | string | true | The token to revoke | | `» token_type_hint` | body | string | false | Hint about token type (access_token or refresh_token) | +### Example responses + +> 413 Response + ### Responses -| Status | Meaning | Description | Schema | -|--------|---------------------------------------------------------|----------------------------|--------| -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | Token successfully revoked | | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|----------------------------|--------------------------------------------------------| +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | Token successfully revoked | | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request Entity Too Large | [codersdk.OAuth2Error](schemas.md#codersdkoauth2error) | ## OAuth2 token exchange @@ -5203,9 +5208,10 @@ grant_type: authorization_code ### Responses -| Status | Meaning | Description | Schema | -|--------|---------------------------------------------------------|-------------|----------------------------------------| -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [oauth2.Token](schemas.md#oauth2token) | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|--------------------------|--------------------------------------------------------| +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [oauth2.Token](schemas.md#oauth2token) | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request Entity Too Large | [codersdk.OAuth2Error](schemas.md#codersdkoauth2error) | ## Delete OAuth2 application tokens diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 527d79f185ebc..761c6a42653d8 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -9269,6 +9269,38 @@ Only certain features set these fields: - FeatureManagedAgentLimit - FeatureAgen |----------|------------------------------------------------------------|----------|--------------|-------------| | `github` | [codersdk.OAuth2GithubConfig](#codersdkoauth2githubconfig) | false | | | +## codersdk.OAuth2Error + +```json +{ + "error": "invalid_request", + "error_description": "string", + "error_uri": "string" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +|---------------------|------------------------------------------------------|----------|--------------|-------------| +| `error` | [codersdk.OAuth2ErrorCode](#codersdkoauth2errorcode) | false | | | +| `error_description` | string | false | | | +| `error_uri` | string | false | | | + +## codersdk.OAuth2ErrorCode + +```json +"invalid_request" +``` + +### Properties + +#### Enumerated Values + +| Value(s) | +|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `access_denied`, `invalid_client`, `invalid_grant`, `invalid_request`, `invalid_scope`, `invalid_target`, `server_error`, `temporarily_unavailable`, `unauthorized_client`, `unsupported_grant_type`, `unsupported_response_type`, `unsupported_token_type` | + ## codersdk.OAuth2GithubConfig ```json From da6db4482312b349ad6dd81b0d8220dfbff1f022 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Fri, 14 Aug 2026 17:37:00 +0000 Subject: [PATCH 2/3] fix(coderd/oauth2provider): bound the registration body per RFC 7591 POST /oauth2/register is reachable pre-authentication and decoded through httpapi.Read, which reports a codersdk.Response. That was the one protocol-inconsistent response in a handler that is otherwise RFC 7591 compliant, and it becomes visible once httpapi.Read starts answering 413. readOAuth2ClientRegistrationRequest bounds and decodes locally so the rejection is an RFC 7591 error. http.MaxBytesReader surfaces the limit through the decoder's error, so the error shape belongs to whoever decodes; that is why the decode moves into the handler rather than passing a limit to httpapi.ReadLimit. RFC 7591 section 3.2.2 defines invalid_client_metadata and friends for semantic validation rather than transport rejection, so invalid_request is the closest compliant framing. The limit is DefaultMaxRequestBodyBytes, unchanged from what httpapi.Read would have applied. Only the error shape and the recorded limit differ. --- coderd/oauth2provider/registration.go | 38 ++++++++++++++++-- coderd/oauth2provider/registration_test.go | 46 ++++++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/coderd/oauth2provider/registration.go b/coderd/oauth2provider/registration.go index 2261a5c5b51a2..efec94fe2d70d 100644 --- a/coderd/oauth2provider/registration.go +++ b/coderd/oauth2provider/registration.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "encoding/json" + "errors" "fmt" "net/http" "net/url" @@ -57,8 +58,8 @@ func CreateDynamicClientRegistration(db database.Store, accessURL *url.URL, audi defer commitAudit() // Parse request - var req codersdk.OAuth2ClientRegistrationRequest - if !httpapi.Read(ctx, rw, r, &req) { + req, ok := readOAuth2ClientRegistrationRequest(ctx, rw, r) + if !ok { return } @@ -272,8 +273,8 @@ func UpdateClientConfiguration(db database.Store, auditor *audit.Auditor, logger } // Parse request - var req codersdk.OAuth2ClientRegistrationRequest - if !httpapi.Read(ctx, rw, r, &req) { + req, ok := readOAuth2ClientRegistrationRequest(ctx, rw, r) + if !ok { return } @@ -532,6 +533,35 @@ func generateRegistrationAccessToken() (plaintext string, hashed []byte, err err } // writeOAuth2RegistrationError writes RFC 7591 compliant error responses +// readOAuth2ClientRegistrationRequest decodes a client registration request, +// bounded by httpapi.DefaultMaxRequestBodyBytes, and reports failures as RFC +// 7591 errors. +// +// It exists instead of httpapi.Read because these handlers route every other +// error through writeOAuth2RegistrationError, and httpapi.Read reports a +// codersdk.Response. Since http.MaxBytesReader surfaces the limit through the +// decoder's error, the error shape belongs to whoever decodes, so the decode +// happens here. RFC 7591 section 3.2.2 defines invalid_client_metadata and +// friends for semantic validation rather than transport rejection, so +// invalid_request is the closest compliant framing for both cases below. +func readOAuth2ClientRegistrationRequest(ctx context.Context, rw http.ResponseWriter, r *http.Request) (codersdk.OAuth2ClientRegistrationRequest, bool) { + var req codersdk.OAuth2ClientRegistrationRequest + + r.Body = http.MaxBytesReader(rw, r.Body, httpapi.DefaultMaxRequestBodyBytes) + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + if _, ok := errors.AsType[*http.MaxBytesError](err); ok { + httpapi.RecordRequestBodyLimit(ctx, httpapi.DefaultMaxRequestBodyBytes) + writeOAuth2RegistrationError(ctx, rw, http.StatusRequestEntityTooLarge, "invalid_request", + fmt.Sprintf("Maximum request body size is %d bytes.", httpapi.DefaultMaxRequestBodyBytes)) + return req, false + } + writeOAuth2RegistrationError(ctx, rw, http.StatusBadRequest, "invalid_request", + "Request body must be valid JSON") + return req, false + } + return req, true +} + func writeOAuth2RegistrationError(_ context.Context, rw http.ResponseWriter, status int, errorCode, description string) { // RFC 7591 error response format errorResponse := map[string]string{ diff --git a/coderd/oauth2provider/registration_test.go b/coderd/oauth2provider/registration_test.go index f23e82dbf76f9..c6284075e813e 100644 --- a/coderd/oauth2provider/registration_test.go +++ b/coderd/oauth2provider/registration_test.go @@ -6,6 +6,8 @@ import ( "net/http" "net/http/httptest" "net/url" + "strconv" + "strings" "testing" "github.com/stretchr/testify/require" @@ -13,6 +15,7 @@ import ( "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/oauth2provider" "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/coderd/util/ptr" @@ -97,3 +100,46 @@ func TestCreateDynamicClientRegistration_DCREnabled(t *testing.T) { }) } } + +// TestCreateDynamicClientRegistration_BodyTooLarge asserts that an oversized +// body is rejected with an RFC 7591 error body rather than a codersdk.Response. +// Every other error in this handler is protocol-shaped, and a client that parses +// the OAuth2 error shape would fail to read a codersdk.Response, so the status +// alone is not sufficient. +func TestCreateDynamicClientRegistration_BodyTooLarge(t *testing.T) { + t.Parallel() + + accessURL, err := url.Parse("https://oauth2-registration-too-large-test.example.com") + require.NoError(t, err) + + ctx := testutil.Context(t, testutil.WaitLong) + + db, _ := dbtestutil.NewDB(t) + require.NoError(t, db.UpsertOAuth2DCREnabled(ctx, true)) + + logger := slogtest.Make(t, nil) + auditor := audit.NewNop() + handler := tracing.StatusWriterMiddleware(oauth2provider.CreateDynamicClientRegistration(db, accessURL, &auditor, logger)) + + // One valid redirect URI padded past the limit, so size is the only reason + // to reject the request. + req := codersdk.OAuth2ClientRegistrationRequest{ + RedirectURIs: []string{"https://example.com/callback"}, + ClientName: strings.Repeat("a", httpapi.DefaultMaxRequestBodyBytes), + } + body, err := json.Marshal(req) + require.NoError(t, err) + require.Greater(t, len(body), httpapi.DefaultMaxRequestBodyBytes) + + r := httptest.NewRequest(http.MethodPost, "/oauth2/register", bytes.NewReader(body)).WithContext(ctx) + r.Header.Set("Content-Type", "application/json") + rw := httptest.NewRecorder() + + handler.ServeHTTP(rw, r) + require.Equal(t, http.StatusRequestEntityTooLarge, rw.Code) + + var errResp map[string]string + require.NoError(t, json.Unmarshal(rw.Body.Bytes(), &errResp)) + require.Equal(t, "invalid_request", errResp["error"]) + require.Contains(t, errResp["error_description"], strconv.Itoa(httpapi.DefaultMaxRequestBodyBytes)) +} From ae6d23c827239a17461031d603b70feb3fb2d333 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Fri, 14 Aug 2026 17:39:41 +0000 Subject: [PATCH 3/3] fix(coderd): expose the decoder text on the OAuth2 registration 400 readOAuth2ClientRegistrationRequest reported every malformed body as a bare "Request body must be valid JSON", so a client integrator saw one message whether a proxy had returned HTML or redirect_uris carried a string where an array belongs. The decoder's own text names the offending field and the type it expected, it describes the caller's own bytes so it discloses nothing, and httpapi.Read has exposed it on every other endpoint for as long as it has existed. This handler routes every other error through err.Error() too. That 400 also had no test. It is a shape the previous commit introduced on purpose: a malformed body used to produce a codersdk.Response and now produces an RFC 7591 error, which is the whole reason the decode is local to the handler rather than httpapi.Read. A future refactor routing it back through httpapi.Read would have regressed the protocol shape silently. TestOAuth2SpecificErrorScenarios/InvalidJSONStructure was an empty subtest claiming coverage happened implicitly through typed request structs, which by construction cannot produce a decode failure. It now posts raw bodies, an unterminated object and a mistyped field, and asserts the status, the invalid_request code, and that the decoder's text survives into error_description. The doc comment for writeOAuth2RegistrationError had been left above readOAuth2ClientRegistrationRequest when that function was inserted, so godoc attributed it to the wrong function and writeOAuth2RegistrationError had none of its own. --- coderd/oauth2_error_compliance_test.go | 45 ++++++++++++++++++++++++-- coderd/oauth2provider/registration.go | 8 +++-- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/coderd/oauth2_error_compliance_test.go b/coderd/oauth2_error_compliance_test.go index 5900e9ee1e80d..6189cdf507b9e 100644 --- a/coderd/oauth2_error_compliance_test.go +++ b/coderd/oauth2_error_compliance_test.go @@ -1,8 +1,10 @@ package coderd_test import ( + "encoding/json" "fmt" "net/http" + "strings" "testing" "time" @@ -384,12 +386,49 @@ func TestOAuth2SpecificErrorScenarios(t *testing.T) { // Error properly returned with bad request status }) + // A malformed body is rejected before validation, so it is the one path on + // this handler that does not go through req.Validate. It still owes the + // caller an RFC 7591 error rather than a codersdk.Response, which is the + // reason the decode is local to the handler rather than httpapi.Read. The + // body is sent raw because a typed request cannot express invalid JSON. t.Run("InvalidJSONStructure", func(t *testing.T) { t.Parallel() - // For invalid JSON structure, we'd need to make raw HTTP requests - // This is tested implicitly through the other tests since we're using - // typed requests that ensure proper JSON structure + for _, tc := range []struct { + name string + body string + // The decoder's own text, which the response must carry so a client + // integrator can tell an unterminated body from a mistyped field. + detail string + }{ + { + name: "UnterminatedObject", + body: `{"client_name": "test"`, + detail: "unexpected EOF", + }, + { + name: "WrongFieldType", + body: `{"redirect_uris": "https://example.com/callback"}`, + detail: "redirect_uris", + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + res, err := client.Request(ctx, http.MethodPost, "/oauth2/register", + strings.NewReader(tc.body)) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, http.StatusBadRequest, res.StatusCode) + + var errResp OAuth2ErrorResponse + require.NoError(t, json.NewDecoder(res.Body).Decode(&errResp)) + require.Equal(t, "invalid_request", errResp.Error) + require.Contains(t, errResp.ErrorDescription, tc.detail) + }) + } }) t.Run("UnsupportedFields", func(t *testing.T) { diff --git a/coderd/oauth2provider/registration.go b/coderd/oauth2provider/registration.go index efec94fe2d70d..9dd874054ef68 100644 --- a/coderd/oauth2provider/registration.go +++ b/coderd/oauth2provider/registration.go @@ -532,7 +532,6 @@ func generateRegistrationAccessToken() (plaintext string, hashed []byte, err err return apikey.GenerateSecret(secretLength) } -// writeOAuth2RegistrationError writes RFC 7591 compliant error responses // readOAuth2ClientRegistrationRequest decodes a client registration request, // bounded by httpapi.DefaultMaxRequestBodyBytes, and reports failures as RFC // 7591 errors. @@ -555,13 +554,18 @@ func readOAuth2ClientRegistrationRequest(ctx context.Context, rw http.ResponseWr fmt.Sprintf("Maximum request body size is %d bytes.", httpapi.DefaultMaxRequestBodyBytes)) return req, false } + // The decoder's own text is the diagnosis a client integrator needs: it + // names the offending field and the type it expected. It describes the + // caller's own bytes, so it discloses nothing, and httpapi.Read has + // exposed it on every other endpoint for as long as it has existed. writeOAuth2RegistrationError(ctx, rw, http.StatusBadRequest, "invalid_request", - "Request body must be valid JSON") + fmt.Sprintf("Request body must be valid JSON: %s", err)) return req, false } return req, true } +// writeOAuth2RegistrationError writes RFC 7591 compliant error responses func writeOAuth2RegistrationError(_ context.Context, rw http.ResponseWriter, status int, errorCode, description string) { // RFC 7591 error response format errorResponse := map[string]string{