From 2ec3bdbc27e1962835c05599d9df5d4b99de3bfd Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Thu, 27 Aug 2026 21:22:25 +0000 Subject: [PATCH 1/4] refactor(coderd/oauth2provider): run both callback preconditions at construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The redirect URI a response goes to had to pass two checks: an exact match against the app's registration, and a scheme check. The match ran inside extractAuthorizeParams, the scheme check separately in each handler afterwards, so the ordering that keeps an unchecked scheme out of a Location header was a convention two call sites had to keep rather than a property of the value. newAuthorizeResponse runs both, and is the only way to get a callback. The scheme is checked on the registered URL rather than on the match's result, because the parser returns the client's URI when the match fails and a 500 there would blame the app for a request it did not make. state moves onto the response, so no call site can build a §4.1.2.1 error the client cannot correlate by passing "". extractAuthorizeParams now returns one authorizeFailure instead of a slice and an error, giving both handlers an explicit three-way decision. Precedence change: an app whose registered callback has a rejected scheme now answers 500 even when the request also has parser errors, where that combination previously answered 400. --- coderd/oauth2provider/authorize.go | 214 +++++++++++------- coderd/oauth2provider/tokens_internal_test.go | 21 +- 2 files changed, 138 insertions(+), 97 deletions(-) diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 3945fbf88e7f1..136d4c2228c78 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -170,30 +170,47 @@ func consentScopes(granted string) (names []string, unrestricted bool) { type authorizeParams struct { clientID string - callback validatedCallbackURL + response authorizeResponse redirectURIProvided bool responseType codersdk.OAuth2ProviderResponseType scope []string - state string resource string // RFC 8707 resource indicator codeChallenge string // PKCE code challenge codeChallengeMethod string // PKCE challenge method } -func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizeParams, []codersdk.ValidationError, error) { +// authorizeFailure is a request that did not parse. Which answer it gets is a +// property of the failure rather than of the order the handler's checks happen +// to run in. +type authorizeFailure struct { + // validationErrors is every field the parser rejected, reported together. + validationErrors []codersdk.ValidationError + // message joins them for the response body. + message string + // corruptCallback is set when the app's registered callback is unusable. + // That is bad server state rather than a client mistake, so it answers 500 + // and stops the request before any parameter is read. + corruptCallback error +} + +func extractAuthorizeParams(r *http.Request, registered *url.URL) (authorizeParams, *authorizeFailure) { p := httpapi.NewQueryParamParser() vals := r.URL.Query() // response_type and client_id are always required. p.RequiredNotEmpty("response_type", "client_id") + response, err := newAuthorizeResponse(p, vals, registered) + if err != nil { + return authorizeParams{}, &authorizeFailure{corruptCallback: err} + } + params := authorizeParams{ clientID: p.String(vals, "", "client_id"), - callback: validatedCallbackURL{url: p.RedirectURL(vals, callbackURL, "redirect_uri")}, + response: response, redirectURIProvided: vals.Get("redirect_uri") != "", responseType: httpapi.ParseCustom(p, vals, "", "response_type", httpapi.ParseEnum[codersdk.OAuth2ProviderResponseType]), scope: strings.Fields(strings.TrimSpace(p.String(vals, "", "scope"))), - state: p.String(vals, "", "state"), resource: p.String(vals, "", "resource"), codeChallenge: p.String(vals, "", "code_challenge"), codeChallengeMethod: p.String(vals, "", "code_challenge_method"), @@ -227,28 +244,60 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar p.ErrorExcessParams(vals) if len(p.Errors) > 0 { - // Create a readable error message with validation details - var errorDetails []string - for _, err := range p.Errors { - errorDetails = append(errorDetails, err.Error()) + details := make([]string, len(p.Errors)) + for i, err := range p.Errors { + details[i] = err.Error() + } + return authorizeParams{}, &authorizeFailure{ + validationErrors: p.Errors, + message: "Invalid query params: " + strings.Join(details, ", "), } - errorMsg := "Invalid query params: " + strings.Join(errorDetails, ", ") - return authorizeParams{}, p.Errors, xerrors.Errorf(errorMsg) } - return params, nil, nil + return params, nil } -// validatedCallbackURL is a redirect URI extractAuthorizeParams has exact-matched -// against the app's registered callback. Requiring one keeps the error redirects -// below from becoming open redirects. The unexported field is a guard, not a -// proof: no other package can fabricate one; this package still can. -type validatedCallbackURL struct { - url *url.URL +// authorizeResponse names where this request's response goes and what it carries +// back. Building one runs both preconditions a Location header needs, so a +// response holding a callback is what licenses a redirect. The unexported fields +// are a guard, not a proof: no other package can fabricate one; this package +// still can. +type authorizeResponse struct { + // callback is nil when the request named a redirect URI this server will not + // send anything to. RFC 6749 §4.1.2.1 keeps that answer on this server. + callback *url.URL + state string +} + +// newAuthorizeResponse checks the app's registered callback, exact-matches any +// redirect_uri the client sent against it, and reads the state to echo back. +// +// The scheme is checked on the registered URL rather than on the match's result, +// because p.RedirectURL returns the client's URI when the match fails, and +// answering 500 for a scheme the client chose would blame the app for a request +// it did not make. It is checked before the match so no parse outcome can reach +// a Location header through a scheme nothing verified. +// +// A returned error means the registration itself is unusable, which is server +// state. A mismatch is the client's mistake and joins the other parameter +// failures in p.Errors. +func newAuthorizeResponse(p *httpapi.QueryParamParser, vals url.Values, registered *url.URL) (authorizeResponse, error) { + if err := codersdk.ValidateRedirectURIScheme(registered); err != nil { + return authorizeResponse{}, err + } + + before := len(p.Errors) + callback := p.RedirectURL(vals, registered, "redirect_uri") + response := authorizeResponse{state: p.String(vals, "", "state")} + if len(p.Errors) == before { + response.callback = callback + } + return response, nil } -// String returns the registered callback, without the query a response adds. -func (c validatedCallbackURL) String() string { - return c.url.String() +// String returns the callback, without the query a response adds. Valid only on +// a response that holds one. +func (a authorizeResponse) String() string { + return a.callback.String() } // reservedResponseParams are the response parameters RFC 6749 §4.1.2.1 and @@ -263,15 +312,15 @@ var reservedResponseParams = []string{"code", "error", "error_description", "sta // The registered query is kept (§3.1.2) except for the reserved parameters: a // registered error= would otherwise ride out on a success response, where a // client reading error first discards a valid code. -func (c validatedCallbackURL) withQuery(state string, set func(url.Values)) *url.URL { - destination := *c.url +func (a authorizeResponse) withQuery(set func(url.Values)) *url.URL { + destination := *a.callback query := destination.Query() for _, param := range reservedResponseParams { query.Del(param) } set(query) - if state != "" { - query.Set("state", state) + if a.state != "" { + query.Set("state", a.state) } destination.RawQuery = query.Encode() return &destination @@ -300,31 +349,28 @@ func sanitizeErrorDescription(description string) string { } // errorURL returns the callback carrying an RFC 6749 §4.1.2.1 error. -func (c validatedCallbackURL) errorURL(state string, code codersdk.OAuth2ErrorCode, description string) *url.URL { - return c.withQuery(state, func(query url.Values) { +func (a authorizeResponse) errorURL(code codersdk.OAuth2ErrorCode, description string) *url.URL { + return a.withQuery(func(query url.Values) { query.Set("error", string(code)) query.Set("error_description", sanitizeErrorDescription(description)) }) } // codeURL returns the callback carrying the authorization code. -func (c validatedCallbackURL) codeURL(state, code string) *url.URL { - return c.withQuery(state, func(query url.Values) { +func (a authorizeResponse) codeURL(code string) *url.URL { + return a.withQuery(func(query url.Values) { query.Set("code", code) }) } // redirectAuthorizeError reports an authorization error through the client's own // callback, as RFC 6749 §4.1.2.1 requires once the client is known. Holding a -// validatedCallbackURL is what licenses the redirect. extractAuthorizeParams -// failures answer on this server instead, even when the callback was already -// trustworthy: the parser reports one verdict for every field at once, so the -// caller cannot tell which field failed. +// response with a callback is what licenses the redirect. // // Logged because the failure leaves in a Location header, which loggermw does // not record, making it indistinguishable from a successful 302. Info, not // Warn: these are client errors. -func redirectAuthorizeError(rw http.ResponseWriter, r *http.Request, logger slog.Logger, callback validatedCallbackURL, state string, code codersdk.OAuth2ErrorCode, description string) { +func redirectAuthorizeError(rw http.ResponseWriter, r *http.Request, logger slog.Logger, response authorizeResponse, code codersdk.OAuth2ErrorCode, description string) { app := httpmw.OAuth2ProviderApp(r) logger.Info(r.Context(), "oauth2 authorization rejected", slog.F("app_id", app.ID.String()), @@ -333,7 +379,7 @@ func redirectAuthorizeError(rw http.ResponseWriter, r *http.Request, logger slog // 302 rather than 307, matching the success redirect below: some external // OAuth2 apps and browsers do not handle 307. - http.Redirect(rw, r, callback.errorURL(state, code, description).String(), http.StatusFound) + http.Redirect(rw, r, response.errorURL(code, description).String(), http.StatusFound) } // logCorruptCallback reports a registered callback URL this server should never @@ -371,10 +417,30 @@ func ShowAuthorizePage(accessURL *url.URL, logger slog.Logger) http.HandlerFunc return } - params, validationErrs, err := extractAuthorizeParams(r, callbackURL) - if err != nil { - errStr := make([]string, len(validationErrs)) - for i, err := range validationErrs { + params, failure := extractAuthorizeParams(r, callbackURL) + if failure != nil { + // 500, not 400: registration rejects these schemes, so a stored one + // is bad server state and takes precedence over anything the client + // got wrong in the same request. + if failure.corruptCallback != nil { + logCorruptCallback(r.Context(), logger, app, failure.corruptCallback) + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ + Status: http.StatusInternalServerError, + HideStatus: false, + Title: "Invalid Callback URL", + Description: "The application's registered callback URL has an invalid scheme.", + Actions: []site.Action{ + { + URL: accessURL.String(), + Text: "Back to site", + }, + }, + }) + return + } + + errStr := make([]string, len(failure.validationErrors)) + for i, err := range failure.validationErrors { errStr[i] = err.Detail } site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ @@ -393,27 +459,6 @@ func ShowAuthorizePage(accessURL *url.URL, logger slog.Logger) http.HandlerFunc return } - // Checked right after the exact match against the registered callback, - // because every redirect below writes this URL into a Location header, - // and the consent page into the cancel link's href. 500, not 400: - // registration rejects these schemes, so a stored one is bad server state. - if err := codersdk.ValidateRedirectURIScheme(params.callback.url); err != nil { - logCorruptCallback(r.Context(), logger, app, err) - site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ - Status: http.StatusInternalServerError, - HideStatus: false, - Title: "Invalid Callback URL", - Description: "The application's registered callback URL has an invalid scheme.", - Actions: []site.Action{ - { - URL: accessURL.String(), - Text: "Back to site", - }, - }, - }) - return - } - // OAuth 2.1 removes the implicit grant, and §4.1.2.1 delivers // unsupported_response_type through the client's own callback. // @@ -421,7 +466,7 @@ func ShowAuthorizePage(accessURL *url.URL, logger slog.Logger) http.HandlerFunc // code alone in response_types_supported, so a client asking for token // is misconfigured rather than mid-implicit-flow. if params.responseType != codersdk.OAuth2ProviderResponseTypeCode { - redirectAuthorizeError(rw, r, logger, params.callback, params.state, + redirectAuthorizeError(rw, r, logger, params.response, codersdk.OAuth2ErrorCodeUnsupportedResponseType, "Only response_type=code is supported") return @@ -431,7 +476,7 @@ func ShowAuthorizePage(accessURL *url.URL, logger slog.Logger) http.HandlerFunc // negotiated below: the page must not render for a request POST will // refuse. Only POST defaults an omitted method, since only POST stores it. if err := codersdk.ValidatePKCECodeChallengeMethod(params.codeChallengeMethod); err != nil { - redirectAuthorizeError(rw, r, logger, params.callback, params.state, + redirectAuthorizeError(rw, r, logger, params.response, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error()) return } @@ -441,14 +486,14 @@ func ShowAuthorizePage(accessURL *url.URL, logger slog.Logger) http.HandlerFunc // clicks Allow. The result also decides what the page states. grantedScope, err := negotiateScope(r.Context(), logger, app, params.scope) if err != nil { - redirectAuthorizeError(rw, r, logger, params.callback, params.state, + redirectAuthorizeError(rw, r, logger, params.response, codersdk.OAuth2ErrorCodeInvalidScope, err.Error()) return } // Declining is an authorization failure like any other, so the cancel // link is the same §4.1.2.1 error URL the redirects above build. - cancel := params.callback.errorURL(params.state, + cancel := params.response.errorURL( codersdk.OAuth2ErrorCodeAccessDenied, "The resource owner or authorization server denied the request") @@ -456,8 +501,8 @@ func ShowAuthorizePage(accessURL *url.URL, logger slog.Logger) http.HandlerFunc site.RenderOAuthAllowPage(rw, r, site.RenderOAuthAllowData{ AppIcon: app.Icon, AppName: app.Name, - // #nosec G203 -- The scheme is validated by - // codersdk.ValidateRedirectURIScheme after extractAuthorizeParams. + // #nosec G203 -- newAuthorizeResponse checked the scheme before this + // URL could exist. CancelURI: htmltemplate.URL(cancel.String()), DashboardURL: accessURL.String(), CSRFToken: nosurf.Token(r), @@ -483,25 +528,24 @@ func ProcessAuthorize(db database.Store, logger slog.Logger) http.HandlerFunc { return } - params, _, err := extractAuthorizeParams(r, callbackURL) - if err != nil { - httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error()) - return - } - - // As on the GET side: every redirect below writes this URL into a - // Location header. - if err := codersdk.ValidateRedirectURIScheme(params.callback.url); err != nil { - logCorruptCallback(ctx, logger, app, err) - httpapi.WriteOAuth2Error(ctx, rw, http.StatusInternalServerError, - codersdk.OAuth2ErrorCodeServerError, - "The application's registered callback URL has an invalid scheme") + params, failure := extractAuthorizeParams(r, callbackURL) + if failure != nil { + // As on the GET side: a rejected registered scheme is server state + // and outranks the client's own mistakes. + if failure.corruptCallback != nil { + logCorruptCallback(ctx, logger, app, failure.corruptCallback) + httpapi.WriteOAuth2Error(ctx, rw, http.StatusInternalServerError, + codersdk.OAuth2ErrorCodeServerError, + "The application's registered callback URL has an invalid scheme") + return + } + httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, failure.message) return } // As on the GET side: OAuth 2.1 removes the implicit grant. if params.responseType != codersdk.OAuth2ProviderResponseTypeCode { - redirectAuthorizeError(rw, r, logger, params.callback, params.state, + redirectAuthorizeError(rw, r, logger, params.response, codersdk.OAuth2ErrorCodeUnsupportedResponseType, "Only response_type=code is supported") return @@ -513,14 +557,14 @@ func ProcessAuthorize(db database.Store, logger slog.Logger) http.HandlerFunc { params.codeChallengeMethod = string(codersdk.OAuth2PKCECodeChallengeMethodS256) } if err := codersdk.ValidatePKCECodeChallengeMethod(params.codeChallengeMethod); err != nil { - redirectAuthorizeError(rw, r, logger, params.callback, params.state, + redirectAuthorizeError(rw, r, logger, params.response, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error()) return } grantedScope, err := negotiateScope(ctx, logger, app, params.scope) if err != nil { - redirectAuthorizeError(rw, r, logger, params.callback, params.state, + redirectAuthorizeError(rw, r, logger, params.response, codersdk.OAuth2ErrorCodeInvalidScope, err.Error()) return } @@ -559,8 +603,8 @@ func ProcessAuthorize(db database.Store, logger slog.Logger) http.HandlerFunc { ResourceUri: sql.NullString{String: params.resource, Valid: params.resource != ""}, CodeChallenge: sql.NullString{String: params.codeChallenge, Valid: params.codeChallenge != ""}, CodeChallengeMethod: sql.NullString{String: params.codeChallengeMethod, Valid: params.codeChallengeMethod != ""}, - StateHash: hashOAuth2State(params.state), - RedirectUri: sql.NullString{String: params.callback.String(), Valid: params.redirectURIProvided}, + StateHash: hashOAuth2State(params.response.state), + RedirectUri: sql.NullString{String: params.response.String(), Valid: params.redirectURIProvided}, // The negotiated scope, not the requested one. The exchange // copies it onto the token row but not yet onto the API key it // mints, so this records what was agreed, not what is enforced. @@ -579,7 +623,7 @@ func ProcessAuthorize(db database.Store, logger slog.Logger) http.HandlerFunc { // (ThomasK33): Use a 302 redirect as some (external) OAuth 2 apps and browsers // do not work with the 307. - http.Redirect(rw, r, params.callback.codeURL(params.state, code.Formatted).String(), http.StatusFound) + http.Redirect(rw, r, params.response.codeURL(code.Formatted).String(), http.StatusFound) } } diff --git a/coderd/oauth2provider/tokens_internal_test.go b/coderd/oauth2provider/tokens_internal_test.go index fc2148353cf1e..c1979ad0a1403 100644 --- a/coderd/oauth2provider/tokens_internal_test.go +++ b/coderd/oauth2provider/tokens_internal_test.go @@ -343,10 +343,9 @@ func TestExtractAuthorizeParams_Scopes(t *testing.T) { } // Extract authorize params - params, validationErrs, err := extractAuthorizeParams(req, callbackURL) + params, failure := extractAuthorizeParams(req, callbackURL) - require.NoError(t, err) - require.Empty(t, validationErrs) + require.Nil(t, failure) require.Equal(t, tc.expectedScopes, params.scope) }) } @@ -407,14 +406,13 @@ func TestExtractAuthorizeParams_CodeChallengeFormat(t *testing.T) { URL: reqURL, } - _, validationErrs, err := extractAuthorizeParams(req, callbackURL) + _, failure := extractAuthorizeParams(req, callbackURL) if tc.expectValid { - require.NoError(t, err) - require.Empty(t, validationErrs) + require.Nil(t, failure) } else { - require.Error(t, err) - require.Len(t, validationErrs, 1) - require.Equal(t, "code_challenge", validationErrs[0].Field) + require.NotNil(t, failure) + require.Len(t, failure.validationErrors, 1) + require.Equal(t, "code_challenge", failure.validationErrors[0].Field) } }) } @@ -442,9 +440,8 @@ func TestExtractAuthorizeParams_TokenResponseTypeDoesNotRequirePKCE(t *testing.T URL: reqURL, } - params, validationErrs, err := extractAuthorizeParams(req, callbackURL) - require.NoError(t, err) - require.Empty(t, validationErrs) + params, failure := extractAuthorizeParams(req, callbackURL) + require.Nil(t, failure) require.Equal(t, codersdk.OAuth2ProviderResponseTypeToken, params.responseType) } From 90be3d1c0c2709667cd41e443fce1862c0234054 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Thu, 27 Aug 2026 21:29:37 +0000 Subject: [PATCH 2/4] fix(coderd/oauth2provider): deliver the rest of the invalid_request class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extractAuthorizeParams discarded a callback it had already exact-matched and returned nothing on any parser error, so a malformed code_challenge, a malformed resource, an unparseable response_type, or an excess parameter stopped on Coder. GET rendered "Invalid Query Parameters", POST wrote a JSON 400, and neither carried state. The app never learned its request failed and waited on an authorization that would never arrive. RFC 6749 §4.1.2.1 delivers these to the client's own callback. Only two failures stay here: one naming the redirect URI, where the response has no destination because the URI never matched, and one naming the client identifier, where the registration the callback was matched against may not belong to whoever is asking. The redirect is safe in the same way #28450's is: the destination has passed the exact match and the scheme check, both now run at construction. Descriptions quote client input and go through the existing sanitizing chokepoint. Tests cover both verbs with redirect_uri omitted and sent explicitly, and pin the combination of a rejected registered scheme with a parser error at 500. --- coderd/oauth2provider/authorize.go | 47 +++++++- .../oauth2provider/authorize_internal_test.go | 110 ++++++++++++++++++ coderd/oauth2provider/authorize_test.go | 104 ++++++++++++++++- coderd/oauth2provider/nostore_test.go | 10 +- .../oauth2providertest/helpers.go | 19 ++- .../oauth2providertest/oauth2_test.go | 5 +- 6 files changed, 281 insertions(+), 14 deletions(-) diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 136d4c2228c78..f5b6ea9f59c5f 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -187,6 +187,10 @@ type authorizeFailure struct { validationErrors []codersdk.ValidationError // message joins them for the response body. message string + // redirect is where RFC 6749 §4.1.2.1 puts the answer. Its zero value means + // the answer stays on this server, because the failure names the redirect + // URI or the client identifier. + redirect authorizeResponse // corruptCallback is set when the app's registered callback is unusable. // That is bad server state rather than a client mistake, so it answers 500 // and stops the request before any parameter is read. @@ -248,14 +252,33 @@ func extractAuthorizeParams(r *http.Request, registered *url.URL) (authorizePara for i, err := range p.Errors { details[i] = err.Error() } - return authorizeParams{}, &authorizeFailure{ + failure := &authorizeFailure{ validationErrors: p.Errors, message: "Invalid query params: " + strings.Join(details, ", "), } + if !blamesClient(p.Errors) { + failure.redirect = response + } + return authorizeParams{}, failure } return params, nil } +// blamesClient reports whether these errors name the client identifier, the one +// RFC 6749 §4.1.2.1 carve-out a response can still satisfy: a wrong client_id +// means the registration the callback was matched against may not belong to +// whoever is asking. The other carve-out, a redirect URI at fault, needs no test +// here because the response it produced has nowhere to send. +// +// Unreachable through the query parameter, since httpmw resolves the app before +// either handler runs, but reachable through the §2.3.1 Basic credential that +// may stand in for it. +func blamesClient(errs []codersdk.ValidationError) bool { + return slices.ContainsFunc(errs, func(e codersdk.ValidationError) bool { + return e.Field == "client_id" + }) +} + // authorizeResponse names where this request's response goes and what it carries // back. Building one runs both preconditions a Location header needs, so a // response holding a callback is what licenses a redirect. The unexported fields @@ -294,6 +317,11 @@ func newAuthorizeResponse(p *httpapi.QueryParamParser, vals url.Values, register return response, nil } +// canRedirect reports whether this response has somewhere to be sent. +func (a authorizeResponse) canRedirect() bool { + return a.callback != nil +} + // String returns the callback, without the query a response adds. Valid only on // a response that holds one. func (a authorizeResponse) String() string { @@ -439,6 +467,17 @@ func ShowAuthorizePage(accessURL *url.URL, logger slog.Logger) http.HandlerFunc return } + // §4.1.2.1: once the callback has been matched against the app's + // registration, a parameter failure is a response to the client + // rather than a page for the user. Without it the app never learns + // its request failed and waits on an authorization that will not + // arrive. + if failure.redirect.canRedirect() { + redirectAuthorizeError(rw, r, logger, failure.redirect, + codersdk.OAuth2ErrorCodeInvalidRequest, failure.message) + return + } + errStr := make([]string, len(failure.validationErrors)) for i, err := range failure.validationErrors { errStr[i] = err.Detail @@ -539,6 +578,12 @@ func ProcessAuthorize(db database.Store, logger slog.Logger) http.HandlerFunc { "The application's registered callback URL has an invalid scheme") return } + // As on the GET side: §4.1.2.1 delivers this to the client. + if failure.redirect.canRedirect() { + redirectAuthorizeError(rw, r, logger, failure.redirect, + codersdk.OAuth2ErrorCodeInvalidRequest, failure.message) + return + } httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, failure.message) return } diff --git a/coderd/oauth2provider/authorize_internal_test.go b/coderd/oauth2provider/authorize_internal_test.go index e89776923d45e..8619d39f07931 100644 --- a/coderd/oauth2provider/authorize_internal_test.go +++ b/coderd/oauth2provider/authorize_internal_test.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "database/sql" "encoding/hex" + "net/url" "strings" "testing" @@ -13,7 +14,9 @@ import ( "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/codersdk" ) func TestNegotiateScope(t *testing.T) { @@ -413,3 +416,110 @@ func TestConsentScopes(t *testing.T) { }) } } + +// TestNewAuthorizeResponse covers the two preconditions the constructor exists +// to run together, and which of them is the server's fault. +func TestNewAuthorizeResponse(t *testing.T) { + t.Parallel() + + const registered = "https://app.example.com/callback" + + newParser := func() (*httpapi.QueryParamParser, *url.URL) { + t.Helper() + callback, err := url.Parse(registered) + require.NoError(t, err) + return httpapi.NewQueryParamParser(), callback + } + + t.Run("MatchingRedirectURI", func(t *testing.T) { + t.Parallel() + + p, callback := newParser() + response, err := newAuthorizeResponse(p, url.Values{ + "redirect_uri": {registered}, + "state": {"abc123"}, + }, callback) + + require.NoError(t, err) + require.Empty(t, p.Errors) + require.True(t, response.canRedirect()) + require.Equal(t, registered, response.String()) + require.Equal(t, "abc123", response.state) + }) + + t.Run("OmittedRedirectURIDefaultsToRegistered", func(t *testing.T) { + t.Parallel() + + p, callback := newParser() + response, err := newAuthorizeResponse(p, url.Values{}, callback) + + require.NoError(t, err) + require.Empty(t, p.Errors) + require.True(t, response.canRedirect()) + require.Equal(t, registered, response.String()) + }) + + t.Run("MismatchedRedirectURIHasNoDestination", func(t *testing.T) { + t.Parallel() + + p, callback := newParser() + response, err := newAuthorizeResponse(p, url.Values{ + "redirect_uri": {"https://elsewhere.example/cb"}, + }, callback) + + // The client's mistake, so it joins the parser's other errors rather + // than becoming a server fault. + require.NoError(t, err) + require.Len(t, p.Errors, 1) + require.Equal(t, "redirect_uri", p.Errors[0].Field) + require.False(t, response.canRedirect(), + "a URI that failed the match must not become a destination") + }) + + t.Run("DangerousClientSchemeIsNotTheServersFault", func(t *testing.T) { + t.Parallel() + + p, callback := newParser() + response, err := newAuthorizeResponse(p, url.Values{ + "redirect_uri": {"javascript:alert(1)"}, + }, callback) + + require.NoError(t, err, "the app registered a usable callback; the client did not send one") + require.NotEmpty(t, p.Errors) + require.False(t, response.canRedirect()) + }) + + t.Run("DangerousRegisteredSchemeIsServerState", func(t *testing.T) { + t.Parallel() + + callback, parseErr := url.Parse("javascript:alert(1)") + require.NoError(t, parseErr) + + p := httpapi.NewQueryParamParser() + response, err := newAuthorizeResponse(p, url.Values{}, callback) + + require.Error(t, err) + require.Empty(t, p.Errors, "the registration is rejected before any parameter is read") + require.False(t, response.canRedirect()) + }) +} + +// TestAuthorizeResponseZeroValue pins the zero value as inert, since it is what +// a failure with no deliverable destination carries. +func TestAuthorizeResponseZeroValue(t *testing.T) { + t.Parallel() + + require.False(t, authorizeResponse{}.canRedirect()) + require.False(t, (&authorizeFailure{}).redirect.canRedirect()) +} + +func TestBlamesClient(t *testing.T) { + t.Parallel() + + require.False(t, blamesClient(nil)) + require.False(t, blamesClient([]codersdk.ValidationError{{Field: "code_challenge"}})) + require.True(t, blamesClient([]codersdk.ValidationError{ + {Field: "code_challenge"}, + {Field: "client_id"}, + })) +} diff --git a/coderd/oauth2provider/authorize_test.go b/coderd/oauth2provider/authorize_test.go index d97b6f32ab96d..c33e21caf6c68 100644 --- a/coderd/oauth2provider/authorize_test.go +++ b/coderd/oauth2provider/authorize_test.go @@ -409,6 +409,36 @@ func TestOAuth2AuthorizeScopeNegotiation(t *testing.T) { "POST: the failure must name the scheme, not just the error class") }) + // The trap the constructor exists to close: a request that both fails the + // parser and belongs to an app whose registered scheme is rejected. Parser + // failures now redirect, so a scheme checked after parsing would be checked + // too late. + t.Run("DangerousCallbackSchemeOutranksParseFailure", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{ + Name: testutil.GetRandomName(t), + CallbackURL: "javascript:alert(1)", + Scope: sql.NullString{String: scopeInCatalog, Valid: true}, + }) + + for _, method := range []string{http.MethodGet, http.MethodPost} { + query := authorizeQuery(t, app.ID.String(), scopeInCatalog) + query.Set("code_challenge", "tooshort") + + resp := sendAuthorizeRequest(ctx, t, client, method, query) + defer resp.Body.Close() + + require.Equal(t, http.StatusInternalServerError, resp.StatusCode, + "%s: the unusable registration outranks the client's own mistake", method) + require.Empty(t, resp.Header.Get("Location"), + "%s: a dangerous scheme must never reach a Location header", method) + require.NotContains(t, readBody(t, resp), "javascript:", + "%s: the scheme must not reach the response body either", method) + } + }) + // A registered callback may carry its own state=, and the cancel link, the // success redirect, and the error redirect write onto it separately. t.Run("CallbackQueryParamsReplacedNotAppended", func(t *testing.T) { @@ -579,7 +609,72 @@ func TestOAuth2AuthorizeErrorsReachTheClient(t *testing.T) { } }) - t.Run("UnparseableResponseTypeNotRedirected", func(t *testing.T) { + // The parser reports every field at once, so these all arrive as + // invalid_request with the offending fields named in the description. + // Explicit as well as omitted redirect_uri, since the two take different + // paths through the parser. + t.Run("RejectedParametersRedirected", func(t *testing.T) { + t.Parallel() + + app := seedAppInCatalog(t) + for _, tc := range []struct { + name string + mutate func(url.Values) + description string + }{ + { + name: "UnparseableResponseType", + mutate: func(q url.Values) { q.Set("response_type", "not_a_response_type") }, + description: "response_type", + }, + { + name: "MalformedCodeChallenge", + mutate: func(q url.Values) { q.Set("code_challenge", "tooshort") }, + description: "43 to 128 characters", + }, + { + name: "MalformedResource", + mutate: func(q url.Values) { q.Set("resource", "not-an-absolute-uri") }, + description: "absolute URI", + }, + { + // The name is the client's to choose, so it reaches + // error_description through %q and has to survive sanitizing. + name: "ExcessParameter", + mutate: func(q url.Values) { q.Set(`we"ird`, "1") }, + description: "not a valid query param", + }, + } { + for _, method := range []string{http.MethodGet, http.MethodPost} { + for _, redirectURI := range []string{"", appCallbackURL} { + name := tc.name + "/" + method + if redirectURI != "" { + name += "ExplicitRedirectURI" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + query := authorizeQuery(t, app.ID.String(), scopeInCatalog) + tc.mutate(query) + if redirectURI != "" { + query.Set("redirect_uri", redirectURI) + } + + resp := sendAuthorizeRequest(ctx, t, client, method, query) + defer resp.Body.Close() + + requireAuthorizeErrorRedirect(t, resp, + codersdk.OAuth2ErrorCodeInvalidRequest, tc.description) + }) + } + } + } + }) + + // A redirect URI the parser could not use is the §4.1.2.1 carve-out: there + // is no callback worth trusting, so the answer stays on this server. + t.Run("UnparseableRedirectURINotRedirected", func(t *testing.T) { t.Parallel() app := seedAppInCatalog(t) @@ -589,15 +684,14 @@ func TestOAuth2AuthorizeErrorsReachTheClient(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) query := authorizeQuery(t, app.ID.String(), scopeInCatalog) - query.Set("response_type", "not_a_response_type") + query.Set("redirect_uri", "://not-a-url") resp := sendAuthorizeRequest(ctx, t, client, method, query) defer resp.Body.Close() - require.Equal(t, http.StatusBadRequest, resp.StatusCode, - "extractAuthorizeParams failures answer on Coder whether or not the callback was trustworthy, and this request omits redirect_uri, so it was") + require.Equal(t, http.StatusBadRequest, resp.StatusCode) require.Empty(t, resp.Header.Get("Location"), - "nothing may be redirected from inside extractAuthorizeParams") + "a redirect URI that did not parse must not become a destination") }) } }) diff --git a/coderd/oauth2provider/nostore_test.go b/coderd/oauth2provider/nostore_test.go index 5c8f2c09dcf62..ecd1b38ee8c1a 100644 --- a/coderd/oauth2provider/nostore_test.go +++ b/coderd/oauth2provider/nostore_test.go @@ -120,9 +120,13 @@ func TestOAuth2NoStoreHeaders(t *testing.T) { app, _ := oauth2providertest.CreateTestOAuth2App(t, client) _, challenge := oauth2providertest.GeneratePKCE(t) - // A response_type that does not parse renders a static error page - // rather than going through httpapi. - uri := strings.Replace(authorizeURL(baseURL, app.ID.String(), challenge), "response_type=code", "response_type=not_a_response_type", 1) + // A redirect_uri that does not match the registration is the one + // parameter failure RFC 6749 §4.1.2.1 keeps on this server, so it is + // what still renders a static error page rather than going through + // httpapi. + uri := strings.Replace(authorizeURL(baseURL, app.ID.String(), challenge), + url.QueryEscape(oauth2providertest.TestRedirectURI), + url.QueryEscape("http://localhost:9876/not-the-registered-callback"), 1) resp := doRequest(ctx, t, http.MethodGet, uri, nil, sessionToken(client)) defer resp.Body.Close() require.Equal(t, http.StatusBadRequest, resp.StatusCode) diff --git a/coderd/oauth2provider/oauth2providertest/helpers.go b/coderd/oauth2provider/oauth2providertest/helpers.go index 8102c91dea5ee..147378a902dee 100644 --- a/coderd/oauth2provider/oauth2providertest/helpers.go +++ b/coderd/oauth2provider/oauth2providertest/helpers.go @@ -374,12 +374,25 @@ func CleanupOAuth2App(t *testing.T, client *codersdk.Client, appID uuid.UUID) { } } -// AuthorizeOAuth2AppExpectingError performs the OAuth2 authorization flow expecting an error -func AuthorizeOAuth2AppExpectingError(t *testing.T, client *codersdk.Client, baseURL string, params AuthorizeParams, expectedStatusCode int) { +// AuthorizeOAuth2AppExpectingError performs the OAuth2 authorization flow +// expecting a rejection, which RFC 6749 §4.1.2.1 delivers to the redirect URI +// the app registered rather than as a status code on this server. +func AuthorizeOAuth2AppExpectingError(t *testing.T, client *codersdk.Client, baseURL string, params AuthorizeParams, expectedError codersdk.OAuth2ErrorCode) { t.Helper() resp := doAuthorizeRequest(t, client, baseURL, params) defer resp.Body.Close() - require.Equal(t, expectedStatusCode, resp.StatusCode, "unexpected status code") + require.Equal(t, http.StatusFound, resp.StatusCode, "unexpected status code") + + location, err := url.Parse(resp.Header.Get("Location")) + require.NoError(t, err, "failed to parse redirect location") + require.Equal(t, params.RedirectURI, location.Scheme+"://"+location.Host+location.Path, + "the error must go to the registered redirect URI") + + query := location.Query() + require.Equal(t, string(expectedError), query.Get("error")) + require.Equal(t, params.State, query.Get("state"), + "the client cannot correlate the failure with its request without its state") + require.Empty(t, query.Get("code"), "a rejected request must not issue a code") } diff --git a/coderd/oauth2provider/oauth2providertest/oauth2_test.go b/coderd/oauth2provider/oauth2providertest/oauth2_test.go index 9e91aa11b114f..4a040bce6c947 100644 --- a/coderd/oauth2provider/oauth2providertest/oauth2_test.go +++ b/coderd/oauth2provider/oauth2providertest/oauth2_test.go @@ -13,6 +13,7 @@ import ( "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" ) @@ -291,7 +292,7 @@ func TestOAuth2WithoutPKCEIsRejected(t *testing.T) { } oauth2providertest.AuthorizeOAuth2AppExpectingError( - t, client, client.URL.String(), authParams, http.StatusBadRequest, + t, client, client.URL.String(), authParams, codersdk.OAuth2ErrorCodeInvalidRequest, ) } @@ -324,7 +325,7 @@ func TestOAuth2MalformedCodeChallengeIsRejected(t *testing.T) { } oauth2providertest.AuthorizeOAuth2AppExpectingError( - t, client, client.URL.String(), authParams, http.StatusBadRequest, + t, client, client.URL.String(), authParams, codersdk.OAuth2ErrorCodeInvalidRequest, ) } From ffcaa33dc289eb359f6a43cbe07eac85921d0ddd Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Thu, 27 Aug 2026 21:31:24 +0000 Subject: [PATCH 3/4] docs(docs/admin/integrations): document the redirected parameter errors The two entries #28450 added cover one redirected code each. Parameter validation failures are the larger class and now arrive the same way, so integrators need to know which ones reach their callback and which two stay on Coder. --- docs/admin/integrations/oauth2-provider.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/admin/integrations/oauth2-provider.md b/docs/admin/integrations/oauth2-provider.md index 962a6b0b50418..bfc1bd71bd6bc 100644 --- a/docs/admin/integrations/oauth2-provider.md +++ b/docs/admin/integrations/oauth2-provider.md @@ -396,6 +396,23 @@ Omitting the parameter is allowed and means `S256`. An unsupported method redirects to your registered callback with `error=invalid_request`, an `error_description` that names the method, and the `state` you sent. This holds for both `GET /oauth2/authorize` and `POST /oauth2/authorize`. +### "invalid_request" for a rejected parameter + +Coder validates every authorization parameter before issuing a code, and reports all the failing fields together in one `error_description`. +Common causes are a `code_challenge` outside the 43 to 128 character unreserved set, a `resource` that is not an absolute URI without a fragment, a `response_type` value Coder cannot parse, and a query parameter the endpoint does not accept. + +The rejection redirects to your registered callback with `error=invalid_request`, an `error_description` naming the fields, and the `state` you sent. +This holds for both `GET /oauth2/authorize` and `POST /oauth2/authorize`. + +Two failures stay on Coder rather than reaching your callback, because in both cases the callback is not yet trustworthy: + +- A `redirect_uri` that does not parse, or that does not exactly match the one registered for the application. + Redirecting to it would defeat the check that just rejected it, so Coder answers 400 (see ["Invalid redirect_uri"](#invalid-redirect_uri)). +- A missing or unparsable `client_id`, which leaves Coder unable to tell whose registration the callback was matched against. + +Earlier releases answered on Coder for all of these: `GET` rendered an "Invalid Query Parameters" page and `POST` returned a 400 with a JSON body. +An integration that watched for either now has to read the error from its own callback. + ### "PKCE verification failed" Verify that the `code_verifier` used in the token request matches the one used to generate the `code_challenge`. From a68a32d134809c1067c0d3eaa8ef31068a028a57 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Thu, 27 Aug 2026 21:51:26 +0000 Subject: [PATCH 4/4] docs: stop advertising response_type=token in the API reference CRF-15 asked for this alongside the 302 responses #28450 documented, and it was the one part left undone. Both authorize endpoints reject token, but the reference listed it as an accepted value, so a client reading the table sends a request the endpoint refuses. Enums on the typed parameter appends to the list swaggo derives from the type rather than replacing it, yielding code, token, code. Declaring the parameter as a string is what narrows it. The generated JSON already rendered it as a string with an inline enum, so only the enum array changes. codersdk.OAuth2ProviderResponseType keeps both constants: it also feeds ResponseTypesSupported, which already advertises code alone. --- coderd/apidoc/docs.go | 6 ++---- coderd/apidoc/swagger.json | 4 ++-- coderd/oauth2.go | 4 ++-- docs/reference/api/enterprise.md | 12 ++++++------ 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index fe4280a86cc56..3b3ddba47ed38 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -14920,8 +14920,7 @@ const docTemplate = `{ }, { "enum": [ - "code", - "token" + "code" ], "type": "string", "description": "Response type", @@ -14979,8 +14978,7 @@ const docTemplate = `{ }, { "enum": [ - "code", - "token" + "code" ], "type": "string", "description": "Response type", diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index a4d1e398462f5..0ef08a85da5ef 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -13257,7 +13257,7 @@ "required": true }, { - "enum": ["code", "token"], + "enum": ["code"], "type": "string", "description": "Response type", "name": "response_type", @@ -13311,7 +13311,7 @@ "required": true }, { - "enum": ["code", "token"], + "enum": ["code"], "type": "string", "description": "Response type", "name": "response_type", diff --git a/coderd/oauth2.go b/coderd/oauth2.go index 51c6c2253ac0a..d11d40e4055f7 100644 --- a/coderd/oauth2.go +++ b/coderd/oauth2.go @@ -118,7 +118,7 @@ func (api *API) deleteOAuth2ProviderAppSecret() http.HandlerFunc { // @Tags Enterprise // @Param client_id query string true "Client ID" // @Param state query string true "A random unguessable string" -// @Param response_type query codersdk.OAuth2ProviderResponseType true "Response type" +// @Param response_type query string true "Response type" Enums(code) // @Param redirect_uri query string false "Redirect here after authorization" // @Param scope query string false "Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist" // @Success 200 "Returns HTML authorization page" @@ -134,7 +134,7 @@ func (api *API) getOAuth2ProviderAppAuthorize() http.HandlerFunc { // @Tags Enterprise // @Param client_id query string true "Client ID" // @Param state query string true "A random unguessable string" -// @Param response_type query codersdk.OAuth2ProviderResponseType true "Response type" +// @Param response_type query string true "Response type" Enums(code) // @Param redirect_uri query string false "Redirect here after authorization" // @Param scope query string false "Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist" // @Success 302 "Redirects to the app's registered callback carrying either an authorization code or an OAuth2 error (RFC 6749 4.1.2.1)" diff --git a/docs/reference/api/enterprise.md b/docs/reference/api/enterprise.md index 2a83c1423c907..763afa35d63b6 100644 --- a/docs/reference/api/enterprise.md +++ b/docs/reference/api/enterprise.md @@ -4887,9 +4887,9 @@ curl -X GET http://coder-server:8080/oauth2/authorize?client_id=string&state=str #### Enumerated Values -| Parameter | Value(s) | -|-----------------|-----------------| -| `response_type` | `code`, `token` | +| Parameter | Value(s) | +|-----------------|----------| +| `response_type` | `code` | ### Responses @@ -4924,9 +4924,9 @@ curl -X POST http://coder-server:8080/oauth2/authorize?client_id=string&state=st #### Enumerated Values -| Parameter | Value(s) | -|-----------------|-----------------| -| `response_type` | `code`, `token` | +| Parameter | Value(s) | +|-----------------|----------| +| `response_type` | `code` | ### Responses