Repository: supabase/gotrue
Pull request #885 (2023-01-04 21:06:51)

fix: add `email` as verification type for email OTPs
## What kind of change does this PR introduce?
* Adds a new verification type for email OTPs called `email`. Currently, it's impossible to tell whether the verification type should be set to "signup" or "magiclink" when a user enters an OTP. As such, developers have to resort to trying `magiclink` first, then catching the error and trying with `signup` next. 
* Deprecate `signup`, `magiclink` types for `POST /verify` 
* Also remove references to the v1 OTP format since it's no longer used and any existing v1 OTPs should have expired by now.

Example request:
```bash
curl -X POST 'http://localhost:9999/verify' -H 'Content-Type: application/json' -d '{"email": "foo@example.com", "token": "123456", "type": "email"}'
```


Patch:
diff --git a/api/external.go b/api/external.go
index ea75697e..db6f4429 100644
--- a/api/external.go
+++ b/api/external.go
@@ -192,7 +192,7 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
 			return internalServerError("Failed to set JWT cookie. %s", err)
 		}
 	} else {
-		rurl = a.prepErrorRedirectURL(unauthorizedError("Unverified email with %v", providerType), r, rurl)
+		rurl = a.prepErrorRedirectURL(unauthorizedError("Unverified email with %v", providerType), w, r, rurl)
 	}
 
 	http.Redirect(w, r, rurl, http.StatusFound)
diff --git a/api/verify.go b/api/verify.go
index 0d424e1d..98e19b25 100644
--- a/api/verify.go
+++ b/api/verify.go
@@ -31,6 +31,8 @@ const (
 	emailChangeVerification = "email_change"
 	smsVerification         = "sms"
 	phoneChangeVerification = "phone_change"
+	// includes signupVerification and magicLinkVerification
+	emailOTPVerification = "email"
 )
 
 const (
@@ -38,12 +40,6 @@ const (
 	singleConfirmation
 )
 
-const (
-	// v1 uses crypto.SecureToken()
-	v1OtpLength      = 22
-	sum224HashLength = 28
-)
-
 // Only applicable when SECURE_EMAIL_CHANGE_ENABLED
 const singleConfirmationAccepted = "Confirmation link accepted. Please proceed to confirm link sent to the other email"
 
@@ -89,10 +85,8 @@ func (a *API) verifyGet(w http.ResponseWriter, r *http.Request) error {
 		if params.Token == "" {
 			return badRequestError("Verify requires a token")
 		}
-		if len(params.Token) > v1OtpLength {
-			// token follows the v2 format and includes "-"
-			params.Token = strings.ReplaceAll(params.Token, "-", "")
-		}
+		params.Token = strings.ReplaceAll(params.Token, "-", "")
+
 		if params.Type == "" {
 			return badRequestError("Verify requires a verification type")
 		}
@@ -138,7 +132,7 @@ func (a *API) verifyGet(w http.ResponseWriter, r *http.Request) error {
 	if err != nil {
 		var herr *HTTPError
 		if errors.As(err, &herr) {
-			rurl := a.prepErrorRedirectURL(herr, r, params.RedirectTo)
+			rurl := a.prepErrorRedirectURL(herr, w, r, params.RedirectTo)
 			http.Redirect(w, r, rurl, http.StatusSeeOther)
 			return nil
 		}
@@ -174,10 +168,7 @@ func (a *API) verifyPost(w http.ResponseWriter, r *http.Request) error {
 	if params.Token == "" {
 		return badRequestError("Verify requires a token")
 	}
-	if len(params.Token) > v1OtpLength {
-		// token follows the v2 format and includes "-"
-		params.Token = strings.ReplaceAll(params.Token, "-", "")
-	}
+	params.Token = strings.ReplaceAll(params.Token, "-", "")
 
 	if params.Type == "" {
 		return badRequestError("Verify requires a verification type")
@@ -340,12 +331,12 @@ func (a *API) smsVerify(r *http.Request, ctx context.Context, conn *storage.Conn
 	return user, nil
 }
 
-func (a *API) prepErrorRedirectURL(err *HTTPError, r *http.Request, rurl string) string {
+func (a *API) prepErrorRedirectURL(err *HTTPError, w http.ResponseWriter, r *http.Request, rurl string) string {
 	q := url.Values{}
-
 	log := observability.GetLogEntry(r)
-	log.Error(err.Message)
-
+	errorID := getRequestID(r.Context())
+	err.ErrorID = errorID
+	log.WithError(err.Cause()).Info(err.Error())
 	if str, ok := oauthErrorMap[err.Code]; ok {
 		q.Set("error", str)
 	}
@@ -501,38 +492,27 @@ func (a *API) verifyUserAndToken(ctx context.Context, conn *storage.Connection,
 
 	var isValid bool
 	switch params.Type {
-	case signupVerification, inviteVerification:
-		// TODO(km): remove when old token format is deprecated
-		// the new token format is represented by a MD5 hash which is 32 characters (128 bits) long
-		// anything shorter than 32 characters can safely be assumed to be using the old token format
-		if len(user.ConfirmationToken) < sum224HashLength {
-			tokenHash = params.Token
+	case emailOTPVerification:
+		// if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns
+		if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) {
+			isValid = true
+			params.Type = signupVerification
+		} else if isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) {
+			isValid = true
+			params.Type = magicLinkVerification
+		} else {
+			isValid = false
 		}
+	case signupVerification, inviteVerification:
 		isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp)
 	case recoveryVerification, magicLinkVerification:
-		// TODO(km): remove when old token format is deprecated
-		if len(user.RecoveryToken) < sum224HashLength {
-			tokenHash = params.Token
-		}
 		isValid = isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp)
 	case emailChangeVerification:
-		// TODO(km): remove when old token format is deprecated
-		if len(user.EmailChangeTokenCurrent) < sum224HashLength && len(user.EmailChangeTokenNew) < sum224HashLength {
-			tokenHash = params.Token
-		}
 		isValid = isOtpValid(tokenHash, user.EmailChangeTokenCurrent, user.EmailChangeSentAt, config.Mailer.OtpExp) ||
 			isOtpValid(tokenHash, user.EmailChangeTokenNew, user.EmailChangeSentAt, config.Mailer.OtpExp)
 	case phoneChangeVerification:
-		// TODO(km): remove when old token format is deprecated
-		if len(user.PhoneChangeToken) < sum224HashLength {
-			tokenHash = params.Token
-		}
 		isValid = isOtpValid(tokenHash, user.PhoneChangeToken, user.PhoneChangeSentAt, config.Sms.OtpExp)
 	case smsVerification:
-		// TODO(km): remove when old token format is deprecated
-		if len(user.ConfirmationToken) < sum224HashLength {
-			tokenHash = params.Token
-		}
 		isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Sms.OtpExp)
 	}
 
diff --git a/hack/postgresd.sh b/hack/postgresd.sh
index ace18257..c4b6a58e 100755
--- a/hack/postgresd.sh
+++ b/hack/postgresd.sh
@@ -11,4 +11,4 @@ docker run --name gotrue_postgresql \
 	-e POSTGRES_DB=postgres \
 	--volume postgres_data:/var/lib/postgresql/data \
 	--volume "$(pwd)"/hack/init_postgres.sql:/docker-entrypoint-initdb.d/init.sql \
-	-d postgres:14
+	-d postgres:15


Test patch:
diff --git a/api/verify_test.go b/api/verify_test.go
index 3a3e77e1..ad16ed6d 100644
--- a/api/verify_test.go
+++ b/api/verify_test.go
@@ -658,6 +658,17 @@ func (ts *VerifyTestSuite) TestVerifyValidOtp() {
 			},
 			expected: expectedResponse,
 		},
+		{
+			desc:     "Valid Email OTP",
+			sentTime: time.Now(),
+			body: map[string]interface{}{
+				"type":      emailOTPVerification,
+				"tokenHash": fmt.Sprintf("%x", sha256.Sum224([]byte(u.GetEmail()+"123456"))),
+				"token":     "123456",
+				"email":     u.GetEmail(),
+			},
+			expected: expectedResponse,
+		},
 		{
 			desc:     "Valid Email Change OTP",
 			sentTime: time.Now(),