Return err on invalid params

That way the caller can just check err instead of the length of errors.
coder · code-asher · Feb 20, 2024 · Jan 20, 2024 · Jan 20, 2024 · Jan 20, 2024
commit 035092a31934fe7b86d29d84efb8a7dd0e27d2ec
diff --git a/enterprise/coderd/identityprovider/authorize.go b/enterprise/coderd/identityprovider/authorize.go
@@ -26,19 +26,15 @@ type authorizeParams struct {
 	state        string
 }
 
-func extractAuthorizeParams(r *http.Request, callbackURL string) (authorizeParams, []codersdk.ValidationError, error) {
+func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizeParams, []codersdk.ValidationError, error) {
 	p := httpapi.NewQueryParamParser()
 	vals := r.URL.Query()
 
 	p.RequiredNotEmpty("state", "response_type", "client_id")
 
-	cb, err := url.Parse(callbackURL)
-	if err != nil {
-		return authorizeParams{}, nil, err
-	}
 	params := authorizeParams{
 		clientID:     p.String(vals, "", "client_id"),
-		redirectURL:  p.RedirectURL(vals, cb, "redirect_uri"),
+		redirectURL:  p.RedirectURL(vals, callbackURL, "redirect_uri"),
 		responseType: httpapi.ParseCustom(p, vals, "", "response_type", httpapi.ParseEnum[codersdk.OAuth2ProviderResponseType]),
 		scope:        p.Strings(vals, []string{}, "scope"),
 		state:        p.String(vals, "", "state"),
@@ -48,7 +44,10 @@ func extractAuthorizeParams(r *http.Request, callbackURL string) (authorizeParam
 	_ = p.String(vals, "", "redirected")
 
 	p.ErrorExcessParams(vals)
-	return params, p.Errors, nil
+	if len(p.Errors) > 0 {
+		return authorizeParams{}, p.Errors, xerrors.Errorf("invalid query params: %w", p.Errors)
+	}
+	return params, nil, nil
 }
 
 /**
@@ -63,17 +62,20 @@ func Authorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
 		apiKey := httpmw.APIKey(r)
 		app := httpmw.OAuth2ProviderApp(r)
 
-		params, validationErrs, err := extractAuthorizeParams(r, app.CallbackURL)
+		callbackURL, err := url.Parse(app.CallbackURL)
 		if err != nil {
 			httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{
 				Message: "Failed to validate query parameters.",
 				Detail:  err.Error(),
 			})
 			return
 		}
-		if len(validationErrs) > 0 {
+
+		params, validationErrs, err := extractAuthorizeParams(r, callbackURL)
+		if err != nil {
 			httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 				Message:     "Invalid query params.",
+				Detail:      err.Error(),
 				Validations: validationErrs,
 			})
 			return

diff --git a/enterprise/coderd/identityprovider/middleware.go b/enterprise/coderd/identityprovider/middleware.go
@@ -57,12 +57,7 @@ func authorizeMW(accessURL *url.URL) func(next http.Handler) http.Handler {
 					return
 				}
 
-				// Extract the form parameters for two reasons:
-				// 1. We need the redirect URI to build the cancel URI.
-				// 2. Since validation will run once the user clicks "allow", it is
-				//    better to validate now to avoid wasting the user's time clicking a
-				//    button that will just error anyway.
-				params, errs, err := extractAuthorizeParams(r, app.CallbackURL)
+				callbackURL, err := url.Parse(app.CallbackURL)
 				if err != nil {
 					site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
 						Status:       http.StatusInternalServerError,
@@ -75,9 +70,16 @@ func authorizeMW(accessURL *url.URL) func(next http.Handler) http.Handler {
 					})
 					return
 				}
-				if len(errs) > 0 {
-					errStr := make([]string, len(errs))
-					for i, err := range errs {
+
+				// Extract the form parameters for two reasons:
+				// 1. We need the redirect URI to build the cancel URI.
+				// 2. Since validation will run once the user clicks "allow", it is
+				//    better to validate now to avoid wasting the user's time clicking a
+				//    button that will just error anyway.
+				params, validationErrs, err := extractAuthorizeParams(r, callbackURL)
+				if err != nil {
+					errStr := make([]string, len(validationErrs))
+					for i, err := range validationErrs {
 						errStr[i] = err.Detail
 					}
 					site.RenderStaticErrorPage(rw, r, site.ErrorPageData{

diff --git a/enterprise/coderd/identityprovider/tokens.go b/enterprise/coderd/identityprovider/tokens.go
@@ -33,49 +33,49 @@ type tokenParams struct {
 	redirectURL  *url.URL
 }
 
-func extractTokenParams(r *http.Request, callbackURL string) (tokenParams, []codersdk.ValidationError, error) {
+func extractTokenParams(r *http.Request, callbackURL *url.URL) (tokenParams, []codersdk.ValidationError, error) {
 	p := httpapi.NewQueryParamParser()
 	err := r.ParseForm()
 	if err != nil {
 		return tokenParams{}, nil, xerrors.Errorf("parse form: %w", err)
 	}
-
-	cb, err := url.Parse(callbackURL)
-	if err != nil {
-		return tokenParams{}, nil, err
-	}
-
 	p.RequiredNotEmpty("grant_type", "client_secret", "client_id", "code")
 
 	vals := r.Form
 	params := tokenParams{
 		clientID:     p.String(vals, "", "client_id"),
 		clientSecret: p.String(vals, "", "client_secret"),
 		code:         p.String(vals, "", "code"),
-		redirectURL:  p.RedirectURL(vals, cb, "redirect_uri"),
+		redirectURL:  p.RedirectURL(vals, callbackURL, "redirect_uri"),
 		grantType:    httpapi.ParseCustom(p, vals, "", "grant_type", httpapi.ParseEnum[codersdk.OAuth2ProviderGrantType]),
 	}
 
 	p.ErrorExcessParams(vals)
-	return params, p.Errors, nil
+	if len(p.Errors) > 0 {
+		return tokenParams{}, p.Errors, xerrors.Errorf("invalid query params: %w", p.Errors)
+	}
+	return params, nil, nil
 }
 
 func Tokens(db database.Store, defaultLifetime time.Duration) http.HandlerFunc {
 	return func(rw http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()
 		app := httpmw.OAuth2ProviderApp(r)
 
-		params, validationErrs, err := extractTokenParams(r, app.CallbackURL)
+		callbackURL, err := url.Parse(app.CallbackURL)
 		if err != nil {
-			httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+			httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 				Message: "Failed to validate form values.",
 				Detail:  err.Error(),
 			})
 			return
 		}
-		if len(validationErrs) > 0 {
+
+		params, validationErrs, err := extractTokenParams(r, callbackURL)
+		if err != nil {
 			httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 				Message:     "Invalid query params.",
+				Detail:      err.Error(),
 				Validations: validationErrs,
 			})
 			return