Repository: supabase/gotrue
Pull request #466 (2022-05-03 00:34:13)

fix: add reuse interval for token refresh
## What kind of change does this PR introduce?
* Addresses https://github.com/supabase/gotrue-js/pull/274 and https://github.com/supabase/gotrue-js/issues/226
* The reuse interval can be configured in the `.env` under `GOTRUE_SECURITY_REFRESH_TOKEN_REUSE_INTERVAL` 
* During the reuse interval, exchanging the same refresh token multiple times will always return the next refresh token. The refresh token must be the previous token and the requests must fall within the reuse interval. If the refresh token isn't the immediate parent of the currently valid refresh token, then the reuse detection will kick in.
* Example
```
token | revoked | parent
foo        t      null
bar        t      foo
baz        f      bar
```

Assuming the refresh token requests are being made within the reuse interval:
1. Using `bar` in the request will return `baz` since it's the previous revoked token. 
2. Using `foo` in the request will **not** return `baz` since it's the second-last previous revoked token. This will trigger the reuse detection and cause `bar` and `baz` to be revoked.
3. Using `baz` in the request will create a new refresh token. 

## Other considerations
* Reuse interval should preferably be set to a value greater than the [refresh token interval](https://github.com/supabase/gotrue-js/blob/a76829739494314b6ad99a6601b2b3d78521e7ce/src/GoTrueClient.ts#L707) in gotrue-js


kangmingtay: > So maybe the default interval should be larger than that? Or we shorten the retries to every second and make the default server interval 5s? 

Hmm i think there are 2 cases we need to consider:
1. To prevent the case where there are concurrent tabs and the JWT expires, the reuse interval has to be larger than the [expiresIn - refreshDurationBeforeExpires](https://github.com/supabase/gotrue-js/pull/278/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5R722) 
2. Offline retries. Reuse interval > Offline retry interval

I was thinking somewhere between 10-30s to also account for potential delay in network or clock skew. 

Patch:
diff --git a/README.md b/README.md
index 322b8ef5..b0342ce7 100644
--- a/README.md
+++ b/README.md
@@ -66,6 +66,15 @@ Rate limit the number of emails sent per hr on the following endpoints: `/signup
 
 Minimum password length, defaults to 6.
 
+`GOTRUE_SECURITY_REFRESH_TOKEN_ROTATION_ENABLED` - `bool`
+
+If refresh token rotation is enabled, gotrue will automatically detect malicious attempts to reuse a revoked refresh token. When a malicious attempt is detected, gotrue immediately revokes all tokens that descended from the offending token.
+
+`GOTRUE_SECURITY_REFRESH_TOKEN_REUSE_INTERVAL` - `string`
+
+This setting is only applicable if `GOTRUE_SECURITY_REFRESH_TOKEN_ROTATION_ENABLED` is enabled. The reuse interval for a refresh token allows for exchanging the refresh token multiple times during the interval to support concurrency or offline issues. During the reuse interval, gotrue will not consider using a revoked token as a malicious attempt and will simply return the child refresh token. 
+
+Only the previous revoked token can be reused. Using an old refresh token way before the current valid refresh token will trigger the reuse detection. 
 ### API
 
 ```properties
diff --git a/api/token.go b/api/token.go
index edd86b83..1ce341be 100644
--- a/api/token.go
+++ b/api/token.go
@@ -273,41 +273,50 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 		return oauthError("invalid_grant", "Invalid Refresh Token")
 	}
 
-	if !(config.External.Email.Enabled && config.External.Phone.Enabled) {
-		providers, err := models.FindProvidersByUser(a.db, user)
-		if err != nil {
-			return internalServerError(err.Error())
-		}
-		for _, provider := range providers {
-			if provider == "email" && !config.External.Email.Enabled {
-				return badRequestError("Email logins are disabled")
+	var newToken *models.RefreshToken
+	if token.Revoked {
+		a.clearCookieTokens(config, w)
+		err = a.db.Transaction(func(tx *storage.Connection) error {
+			validToken, terr := models.GetValidChildToken(tx, token)
+			if terr != nil {
+				if errors.Is(terr, models.RefreshTokenNotFoundError{}) {
+					// revoked token has no descendants
+					return nil
+				}
+				return terr
 			}
-			if provider == "phone" && !config.External.Phone.Enabled {
-				return badRequestError("Phone logins are disabled")
+			// check if token is the last previous revoked token
+			if validToken.Parent == storage.NullString(token.Token) {
+				refreshTokenReuseWindow := token.UpdatedAt.Add(time.Second * time.Duration(config.Security.RefreshTokenReuseInterval))
+				if time.Now().Before(refreshTokenReuseWindow) {
+					newToken = validToken
+				}
 			}
+			return nil
+		})
+		if err != nil {
+			return internalServerError("Error validating reuse interval").WithInternalError(err)
 		}
-	}
 
-	if token.Revoked {
-		a.clearCookieTokens(config, w)
-		if config.Security.RefreshTokenRotationEnabled {
-			// Revoke all tokens in token family
-			err = a.db.Transaction(func(tx *storage.Connection) error {
-				var terr error
-				if terr = models.RevokeTokenFamily(tx, token); terr != nil {
-					return terr
+		if newToken == nil {
+			if config.Security.RefreshTokenRotationEnabled {
+				// Revoke all tokens in token family
+				err = a.db.Transaction(func(tx *storage.Connection) error {
+					var terr error
+					if terr = models.RevokeTokenFamily(tx, token); terr != nil {
+						return terr
+					}
+					return nil
+				})
+				if err != nil {
+					return internalServerError(err.Error())
 				}
-				return nil
-			})
-			if err != nil {
-				return internalServerError(err.Error())
 			}
+			return oauthError("invalid_grant", "Invalid Refresh Token").WithInternalMessage("Possible abuse attempt: %v", r)
 		}
-		return oauthError("invalid_grant", "Invalid Refresh Token").WithInternalMessage("Possible abuse attempt: %v", r)
 	}
 
 	var tokenString string
-	var newToken *models.RefreshToken
 	var newTokenResponse *AccessTokenResponse
 
 	err = a.db.Transaction(func(tx *storage.Connection) error {
@@ -316,9 +325,11 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 			return terr
 		}
 
-		newToken, terr = models.GrantRefreshTokenSwap(tx, user, token)
-		if terr != nil {
-			return internalServerError(terr.Error())
+		if newToken == nil {
+			newToken, terr = models.GrantRefreshTokenSwap(tx, user, token)
+			if terr != nil {
+				return internalServerError(terr.Error())
+			}
 		}
 
 		tokenString, terr = generateAccessToken(user, time.Second*time.Duration(config.JWT.Exp), config.JWT.Secret)
diff --git a/conf/configuration.go b/conf/configuration.go
index c99e840e..b3a817e5 100644
--- a/conf/configuration.go
+++ b/conf/configuration.go
@@ -181,6 +181,7 @@ type CaptchaConfiguration struct {
 type SecurityConfiguration struct {
 	Captcha                               CaptchaConfiguration `json:"captcha"`
 	RefreshTokenRotationEnabled           bool                 `json:"refresh_token_rotation_enabled" split_words:"true" default:"true"`
+	RefreshTokenReuseInterval             int                  `json:"refresh_token_reuse_interval" split_words:"true"`
 	UpdatePasswordRequireReauthentication bool                 `json:"update_password_require_reauthentication" split_words:"true"`
 }
 
diff --git a/example.env b/example.env
index e38ef11a..be0e6312 100644
--- a/example.env
+++ b/example.env
@@ -190,7 +190,9 @@ GOTRUE_EXTERNAL_SAML_SIGNING_KEY=""
 
 # Additional Security config
 GOTRUE_LOG_LEVEL="debug"
-GOTRUE_REFRESH_TOKEN_ROTATION_ENABLED="false"
+GOTRUE_SECURITY_REFRESH_TOKEN_ROTATION_ENABLED="false"
+GOTRUE_SECURITY_REFRESH_TOKEN_REUSE_INTERVAL="0"
+GOTRUE_SECURITY_UPDATE_PASSWORD_REQUIRE_REAUTHENTICATION="false"
 GOTRUE_OPERATOR_TOKEN="unused-operator-token"
 GOTRUE_RATE_LIMIT_HEADER="X-Forwarded-For"
 GOTRUE_RATE_LIMIT_EMAIL_SENT="100"
diff --git a/models/refresh_token.go b/models/refresh_token.go
index 054806e4..2af42c50 100644
--- a/models/refresh_token.go
+++ b/models/refresh_token.go
@@ -1,6 +1,7 @@
 package models
 
 import (
+	"database/sql"
 	"time"
 
 	"github.com/gobuffalo/pop/v5"
@@ -57,19 +58,33 @@ func GrantRefreshTokenSwap(tx *storage.Connection, user *User, token *RefreshTok
 
 // RevokeTokenFamily revokes all refresh tokens that descended from the provided token.
 func RevokeTokenFamily(tx *storage.Connection, token *RefreshToken) error {
+	tablename := (&pop.Model{Value: RefreshToken{}}).TableName()
 	err := tx.RawQuery(`
 	with recursive token_family as (
-		select id, user_id, token, revoked, parent from refresh_tokens where parent = ?
+		select id, user_id, token, revoked, parent from `+tablename+` where parent = ?
 		union
-		select r.id, r.user_id, r.token, r.revoked, r.parent from `+(&pop.Model{Value: RefreshToken{}}).TableName()+` r inner join token_family t on t.token = r.parent
+		select r.id, r.user_id, r.token, r.revoked, r.parent from `+tablename+` r inner join token_family t on t.token = r.parent
 	)
-	update `+(&pop.Model{Value: RefreshToken{}}).TableName()+` r set revoked = true from token_family where token_family.id = r.id;`, token.Token).Exec()
+	update `+tablename+` r set revoked = true from token_family where token_family.id = r.id;`, token.Token).Exec()
 	if err != nil {
 		return err
 	}
 	return nil
 }
 
+// GetValidChildToken returns the child token of the token provided if the child is not revoked.
+func GetValidChildToken(tx *storage.Connection, token *RefreshToken) (*RefreshToken, error) {
+	refreshToken := &RefreshToken{}
+	err := tx.Q().Where("parent = ? and revoked = false", token.Token).First(refreshToken)
+	if err != nil {
+		if errors.Cause(err) == sql.ErrNoRows {
+			return nil, RefreshTokenNotFoundError{}
+		}
+		return nil, err
+	}
+	return refreshToken, nil
+}
+
 // Logout deletes all refresh tokens for a user.
 func Logout(tx *storage.Connection, instanceID uuid.UUID, id uuid.UUID) error {
 	return tx.RawQuery("DELETE FROM "+(&pop.Model{Value: RefreshToken{}}).TableName()+" WHERE instance_id = ? AND user_id = ?", instanceID, id).Exec()


Test patch:
diff --git a/api/token_test.go b/api/token_test.go
index eaa5dcb5..763cb49d 100644
--- a/api/token_test.go
+++ b/api/token_test.go
@@ -150,6 +150,95 @@ func (ts *TokenTestSuite) TestTokenRefreshTokenGrantFailure() {
 	assert.Equal(ts.T(), http.StatusBadRequest, w.Code)
 }
 
+func (ts *TokenTestSuite) TestTokenRefreshTokenRotation() {
+	u, err := models.NewUser(ts.instanceID, "foo@example.com", "password", ts.Config.JWT.Aud, nil)
+	require.NoError(ts.T(), err, "Error creating test user model")
+	t := time.Now()
+	u.EmailConfirmedAt = &t
+	require.NoError(ts.T(), ts.API.db.Create(u), "Error saving foo user")
+	first, err := models.GrantAuthenticatedUser(ts.API.db, u)
+	require.NoError(ts.T(), err)
+	second, err := models.GrantRefreshTokenSwap(ts.API.db, u, first)
+	require.NoError(ts.T(), err)
+	third, err := models.GrantRefreshTokenSwap(ts.API.db, u, second)
+	require.NoError(ts.T(), err)
+
+	cases := []struct {
+		desc                        string
+		refreshTokenRotationEnabled bool
+		reuseInterval               int
+		refreshToken                string
+		expectedCode                int
+		expectedBody                map[string]interface{}
+	}{
+		{
+			"Valid refresh within reuse interval",
+			true,
+			30,
+			second.Token,
+			http.StatusOK,
+			map[string]interface{}{
+				"refresh_token": third.Token,
+			},
+		},
+		{
+			"Invalid refresh, first token is not the previous revoked token",
+			true,
+			0,
+			first.Token,
+			http.StatusBadRequest,
+			map[string]interface{}{
+				"error":             "invalid_grant",
+				"error_description": "Invalid Refresh Token",
+			},
+		},
+		{
+			"Invalid refresh, revoked third token",
+			true,
+			0,
+			second.Token,
+			http.StatusBadRequest,
+			map[string]interface{}{
+				"error":             "invalid_grant",
+				"error_description": "Invalid Refresh Token",
+			},
+		},
+		{
+			"Invalid refresh, third token revoked by previous case",
+			true,
+			30,
+			third.Token,
+			http.StatusBadRequest,
+			map[string]interface{}{
+				"error":             "invalid_grant",
+				"error_description": "Invalid Refresh Token",
+			},
+		},
+	}
+
+	for _, c := range cases {
+		ts.Run(c.desc, func() {
+			ts.Config.Security.RefreshTokenRotationEnabled = c.refreshTokenRotationEnabled
+			ts.Config.Security.RefreshTokenReuseInterval = c.reuseInterval
+			var buffer bytes.Buffer
+			require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
+				"refresh_token": c.refreshToken,
+			}))
+			req := httptest.NewRequest(http.MethodPost, "http://localhost/token?grant_type=refresh_token", &buffer)
+			req.Header.Set("Content-Type", "application/json")
+			w := httptest.NewRecorder()
+			ts.API.handler.ServeHTTP(w, req)
+			assert.Equal(ts.T(), c.expectedCode, w.Code)
+
+			data := make(map[string]interface{})
+			require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data))
+			for k, v := range c.expectedBody {
+				require.Equal(ts.T(), v, data[k])
+			}
+		})
+	}
+}
+
 func (ts *TokenTestSuite) createBannedUser() *models.User {
 	u, err := models.NewUser(ts.instanceID, "banned@example.com", "password", ts.Config.JWT.Aud, nil)
 	require.NoError(ts.T(), err, "Error creating test user model")