diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index fe4280a86cc..3b3ddba47ed 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 a4d1e398462..0ef08a85da5 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 51c6c2253ac..d11d40e4055 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/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 3945fbf88e7..f5b6ea9f59c 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -170,30 +170,51 @@ 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 + // 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. + 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 +248,84 @@ 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() + } + failure := &authorizeFailure{ + validationErrors: p.Errors, + message: "Invalid query params: " + strings.Join(details, ", "), + } + if !blamesClient(p.Errors) { + failure.redirect = response } - errorMsg := "Invalid query params: " + strings.Join(errorDetails, ", ") - return authorizeParams{}, p.Errors, xerrors.Errorf(errorMsg) + return authorizeParams{}, failure } - 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 +// 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" + }) } -// String returns the registered callback, without the query a response adds. -func (c validatedCallbackURL) String() string { - return c.url.String() +// 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 +} + +// 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 { + return a.callback.String() } // reservedResponseParams are the response parameters RFC 6749 §4.1.2.1 and @@ -263,15 +340,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 +377,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 +407,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 +445,41 @@ 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 + } + + // §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 } site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ @@ -393,27 +498,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 +505,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 +515,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 +525,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 +540,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 +567,30 @@ 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 + } + // 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 } // 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 +602,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 +648,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 +668,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/authorize_internal_test.go b/coderd/oauth2provider/authorize_internal_test.go index e89776923d4..8619d39f079 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 d97b6f32ab9..c33e21caf6c 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 5c8f2c09dcf..ecd1b38ee8c 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 8102c91dea5..147378a902d 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 9e91aa11b11..4a040bce6c9 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, ) } diff --git a/coderd/oauth2provider/tokens_internal_test.go b/coderd/oauth2provider/tokens_internal_test.go index fc2148353cf..c1979ad0a14 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) } diff --git a/docs/admin/integrations/oauth2-provider.md b/docs/admin/integrations/oauth2-provider.md index 962a6b0b504..bfc1bd71bd6 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`. diff --git a/docs/reference/api/enterprise.md b/docs/reference/api/enterprise.md index 2a83c1423c9..763afa35d63 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