Repository: supabase/gotrue
Pull request #789 (2022-11-02 09:30:41)

fix: use clear hCaptcha error messages
Previously the code was quite messy and was not easy to determine why hCaptcha protection was failing. This PR attempts to simplify the logic and provide clear error messages.


Patch:
diff --git a/api/helpers.go b/api/helpers.go
index 30ce9aa5..c0283eb9 100644
--- a/api/helpers.go
+++ b/api/helpers.go
@@ -1,11 +1,9 @@
 package api
 
 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
-	"io"
 	"net"
 	"net/http"
 	"net/http/httptrace"
@@ -14,6 +12,7 @@ import (
 	"github.com/gofrs/uuid"
 	"github.com/netlify/gotrue/conf"
 	"github.com/netlify/gotrue/models"
+	"github.com/netlify/gotrue/utilities"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -237,19 +236,5 @@ func isStringInSlice(checkValue string, list []string) bool {
 
 // getBodyBytes returns a byte array of the request's Body.
 func getBodyBytes(req *http.Request) ([]byte, error) {
-	if req.Body == nil || req.Body == http.NoBody {
-		return nil, nil
-	}
-
-	originalBody := req.Body
-	defer originalBody.Close()
-
-	buf, err := io.ReadAll(originalBody)
-	if err != nil {
-		return nil, err
-	}
-
-	req.Body = io.NopCloser(bytes.NewReader(buf))
-
-	return buf, nil
+	return utilities.GetBodyBytes(req)
 }
diff --git a/api/middleware.go b/api/middleware.go
index 7f1048d5..51b46218 100644
--- a/api/middleware.go
+++ b/api/middleware.go
@@ -9,7 +9,6 @@ import (
 
 	"github.com/netlify/gotrue/observability"
 	"github.com/netlify/gotrue/security"
-	"github.com/sirupsen/logrus"
 
 	"github.com/didip/tollbooth/v5"
 	"github.com/didip/tollbooth/v5/limiter"
@@ -139,6 +138,7 @@ func (a *API) requireEmailProvider(w http.ResponseWriter, req *http.Request) (co
 func (a *API) verifyCaptcha(w http.ResponseWriter, req *http.Request) (context.Context, error) {
 	ctx := req.Context()
 	config := a.config
+
 	if !config.Security.Captcha.Enabled {
 		return ctx, nil
 	}
@@ -149,28 +149,17 @@ func (a *API) verifyCaptcha(w http.ResponseWriter, req *http.Request) (context.C
 	if shouldIgnore := isIgnoreCaptchaRoute(req); shouldIgnore {
 		return ctx, nil
 	}
-	if config.Security.Captcha.Provider != "hcaptcha" {
-		logrus.WithField("provider", config.Security.Captcha.Provider).Warn("Unsupported captcha provider")
-		return nil, internalServerError("server misconfigured")
-	}
-	secret := strings.TrimSpace(config.Security.Captcha.Secret)
-	if secret == "" {
-		return nil, internalServerError("server misconfigured")
-	}
 
-	verificationResult, err := security.VerifyRequest(req, secret)
+	verificationResult, err := security.VerifyRequest(req, strings.TrimSpace(config.Security.Captcha.Secret))
 	if err != nil {
-		logrus.WithField("err", err).Infof("failed to validate result")
-		return nil, internalServerError("request validation failure")
-	}
-	if verificationResult == security.VerificationProcessFailure {
-		return nil, internalServerError("request validation failure")
-	} else if verificationResult == security.UserRequestFailed {
-		return nil, badRequestError("request disallowed")
+		return nil, internalServerError("hCaptcha verification process failed").WithInternalError(err)
 	}
-	if verificationResult != security.SuccessfullyVerified {
-		return nil, internalServerError("")
+
+	if !verificationResult.Success {
+		return nil, badRequestError("hCaptcha protection: request disallowed (%s)", strings.Join(verificationResult.ErrorCodes, ", "))
+
 	}
+
 	return ctx, nil
 }
 
diff --git a/conf/configuration.go b/conf/configuration.go
index 7937bc39..f84efd9c 100644
--- a/conf/configuration.go
+++ b/conf/configuration.go
@@ -2,8 +2,10 @@ package conf
 
 import (
 	"errors"
+	"fmt"
 	"net/url"
 	"os"
+	"strings"
 	"time"
 
 	"github.com/gobwas/glob"
@@ -219,6 +221,24 @@ type CaptchaConfiguration struct {
 	Secret   string `json:"provider_secret"`
 }
 
+func (c *CaptchaConfiguration) Validate() error {
+	if !c.Enabled {
+		return nil
+	}
+
+	if c.Provider != "hcaptcha" {
+		return fmt.Errorf("unsupported captcha provider: %s", c.Provider)
+	}
+
+	c.Secret = strings.TrimSpace(c.Secret)
+
+	if c.Secret == "" {
+		return errors.New("hcaptcha provider secret is empty")
+	}
+
+	return nil
+}
+
 type SecurityConfiguration struct {
 	Captcha                               CaptchaConfiguration `json:"captcha"`
 	RefreshTokenRotationEnabled           bool                 `json:"refresh_token_rotation_enabled" split_words:"true" default:"true"`
@@ -226,6 +246,10 @@ type SecurityConfiguration struct {
 	UpdatePasswordRequireReauthentication bool                 `json:"update_password_require_reauthentication" split_words:"true"`
 }
 
+func (c *SecurityConfiguration) Validate() error {
+	return c.Captcha.Validate()
+}
+
 func loadEnvironment(filename string) error {
 	var err error
 	if filename != "" {
@@ -393,6 +417,7 @@ func (c *GlobalConfiguration) Validate() error {
 		&c.Metrics,
 		&c.SMTP,
 		&c.SAML,
+		&c.Security,
 	}
 
 	for _, validatable := range validatables {
diff --git a/security/hcaptcha.go b/security/hcaptcha.go
index ba8e6da1..427e762f 100644
--- a/security/hcaptcha.go
+++ b/security/hcaptcha.go
@@ -1,10 +1,7 @@
 package security
 
 import (
-	"bytes"
 	"encoding/json"
-	"fmt"
-	"io"
 	"log"
 	"net/http"
 	"net/url"
@@ -15,7 +12,6 @@ import (
 
 	"github.com/netlify/gotrue/utilities"
 	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 )
 
 type GotrueRequest struct {
@@ -32,14 +28,6 @@ type VerificationResponse struct {
 	Hostname   string   `json:"hostname"`
 }
 
-type VerificationResult int
-
-const (
-	UserRequestFailed VerificationResult = iota
-	VerificationProcessFailure
-	SuccessfullyVerified
-)
-
 var Client *http.Client
 
 func init() {
@@ -56,29 +44,30 @@ func init() {
 	Client = &http.Client{Timeout: defaultTimeout}
 }
 
-func VerifyRequest(r *http.Request, secretKey string) (VerificationResult, error) {
-	res := GotrueRequest{}
-	bodyBytes, err := io.ReadAll(r.Body)
+func VerifyRequest(r *http.Request, secretKey string) (VerificationResponse, error) {
+	bodyBytes, err := utilities.GetBodyBytes(r)
 	if err != nil {
-		return UserRequestFailed, err
+		return VerificationResponse{}, err
 	}
-	if err := r.Body.Close(); err != nil {
-		return UserRequestFailed, err
+
+	var requestBody GotrueRequest
+
+	if err := json.Unmarshal(bodyBytes, &requestBody); err != nil {
+		return VerificationResponse{}, errors.Wrap(err, "request body was not JSON")
 	}
 
-	// re-init body so downstream route handlers don't get borked
-	r.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
+	captchaResponse := strings.TrimSpace(requestBody.Security.Token)
 
-	jsonDecoder := json.NewDecoder(bytes.NewBuffer(bodyBytes))
-	err = jsonDecoder.Decode(&res)
-	if err != nil || strings.TrimSpace(res.Security.Token) == "" {
-		return UserRequestFailed, errors.Wrap(err, "couldn't decode captcha info")
+	if captchaResponse == "" {
+		return VerificationResponse{}, errors.New("no hCaptcha response (captcha_token) found in request")
 	}
+
 	clientIP := utilities.GetIPAddress(r)
-	return verifyCaptchaCode(res.Security.Token, secretKey, clientIP)
+
+	return verifyCaptchaCode(captchaResponse, secretKey, clientIP)
 }
 
-func verifyCaptchaCode(token string, secretKey string, clientIP string) (VerificationResult, error) {
+func verifyCaptchaCode(token string, secretKey string, clientIP string) (VerificationResponse, error) {
 	data := url.Values{}
 	data.Set("secret", secretKey)
 	data.Set("response", token)
@@ -87,24 +76,21 @@ func verifyCaptchaCode(token string, secretKey string, clientIP string) (Verific
 
 	r, err := http.NewRequest("POST", "https://hcaptcha.com/siteverify", strings.NewReader(data.Encode()))
 	if err != nil {
-		return VerificationProcessFailure, errors.Wrap(err, "couldn't initialize request object for hcaptcha check")
+		return VerificationResponse{}, errors.Wrap(err, "couldn't initialize request object for hcaptcha check")
 	}
 	r.Header.Add("Content-Type", "application/x-www-form-urlencoded")
 	r.Header.Add("Content-Length", strconv.Itoa(len(data.Encode())))
 	res, err := Client.Do(r)
 	if err != nil {
-		return VerificationProcessFailure, errors.Wrap(err, "failed to verify hcaptcha token")
+		return VerificationResponse{}, errors.Wrap(err, "failed to verify hcaptcha response")
 	}
-	verResult := VerificationResponse{}
 	defer res.Body.Close()
-	decoder := json.NewDecoder(res.Body)
-	err = decoder.Decode(&verResult)
-	if err != nil {
-		return VerificationProcessFailure, errors.Wrap(err, "failed to decode hcaptcha response")
-	}
-	logrus.WithField("result", verResult).Info("obtained hcaptcha verification result")
-	if !verResult.Success {
-		return UserRequestFailed, fmt.Errorf("user request suppressed by hcaptcha")
+
+	var verificationResponse VerificationResponse
+
+	if err := json.NewDecoder(res.Body).Decode(&verificationResponse); err != nil {
+		return VerificationResponse{}, errors.Wrap(err, "failed to decode hcaptcha response: not JSON")
 	}
-	return SuccessfullyVerified, nil
+
+	return verificationResponse, nil
 }
diff --git a/utilities/request.go b/utilities/request.go
index 3e244b7b..cf61ba6e 100644
--- a/utilities/request.go
+++ b/utilities/request.go
@@ -1,6 +1,8 @@
 package utilities
 
 import (
+	"bytes"
+	"io"
 	"net"
 	"net/http"
 	"strings"
@@ -33,3 +35,22 @@ func GetIPAddress(r *http.Request) string {
 
 	return ip
 }
+
+// GetBodyBytes reads the whole request body properly into a byte array.
+func GetBodyBytes(req *http.Request) ([]byte, error) {
+	if req.Body == nil || req.Body == http.NoBody {
+		return nil, nil
+	}
+
+	originalBody := req.Body
+	defer originalBody.Close()
+
+	buf, err := io.ReadAll(originalBody)
+	if err != nil {
+		return nil, err
+	}
+
+	req.Body = io.NopCloser(bytes.NewReader(buf))
+
+	return buf, nil
+}


Test patch:
diff --git a/api/middleware_test.go b/api/middleware_test.go
index d5cdf3f6..fa95e1a5 100644
--- a/api/middleware_test.go
+++ b/api/middleware_test.go
@@ -114,25 +114,6 @@ func (ts *MiddlewareTestSuite) TestVerifyCaptchaInvalid() {
 		expectedCode int
 		expectedMsg  string
 	}{
-		{
-			"Unsupported provider",
-			&conf.CaptchaConfiguration{
-				Enabled:  true,
-				Provider: "test",
-			},
-			http.StatusInternalServerError,
-			"server misconfigured",
-		},
-		{
-			"Missing secret",
-			&conf.CaptchaConfiguration{
-				Enabled:  true,
-				Provider: "hcaptcha",
-				Secret:   "",
-			},
-			http.StatusInternalServerError,
-			"server misconfigured",
-		},
 		{
 			"Captcha validation failed",
 			&conf.CaptchaConfiguration{
@@ -140,8 +121,8 @@ func (ts *MiddlewareTestSuite) TestVerifyCaptchaInvalid() {
 				Provider: "hcaptcha",
 				Secret:   "test",
 			},
-			http.StatusInternalServerError,
-			"request validation failure",
+			http.StatusBadRequest,
+			"hCaptcha protection: request disallowed (not-using-dummy-secret)",
 		},
 	}
 	for _, c := range cases {