Skip to content

Commit

Permalink
fix: duplicate session token issue when cookieDomain is changed
Browse files Browse the repository at this point in the history
  • Loading branch information
furkansenharputlu committed Apr 30, 2024
1 parent b827deb commit 197c6be
Show file tree
Hide file tree
Showing 10 changed files with 394 additions and 40 deletions.
34 changes: 34 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `session.CreateNewSession` now defaults to the value of the `st-auth-mode` header (if available) if the configured `config.GetTokenTransferMethod` returns `any`.
- Enable smooth switching between `useDynamicAccessTokenSigningKey` settings by allowing refresh calls to change the signing key type of a session.
- Make session required during signout.
- Added `OlderCookieDomain` config option in the session recipe. This will allow users to clear cookies from the older domain when the `CookieDomain` is changed.
- If `VerifySession` detects multiple access tokens in the request, it will return a 401 error, prompting a refresh, even if one of the tokens is valid.
- `RefreshPOST` (`/auth/session/refresh` by default) API changes:
- now returns 500 error if multiple access tokens are present in the request and `config.OlderCookieDomain` is not set.
- now clears the access token cookie if it was called without a refresh token (if an access token cookie exists and if using cookie-based sessions).
- now clears cookies from the old domain if `OlderCookieDomain` is specified and multiple refresh/access token cookies exist, without updating the front-token or any of the tokens.
- now a 200 response may not include new session tokens.

### Rationale

This update addresses an edge case where changing the `CookieDomain` config on the server can lead to session integrity issues. For instance, if the API server URL is 'api.example.com' with a cookie domain of '.example.com', and the server updates the cookie domain to 'api.example.com', the client may retain cookies with both '.example.com' and 'api.example.com' domains, resulting in multiple sets of session token cookies existing.

Previously, verifySession would select one of the access tokens from the incoming request. If it chose the older cookie, it would return a 401 status code, prompting a refresh request. However, the `RefreshPOST` API would then set new session token cookies with the updated `CookieDomain`, but older cookies will persist, leading to repeated 401 errors and refresh loops.

With this update, verifySession will return a 401 error if it detects multiple access tokens in the request, prompting a refresh request. The `RefreshPOST` API will clear cookies from the old domain if `OlderCookieDomain` is specified in the configuration, then return a 200 status. If `OlderCookieDomain` is not configured, the `RefreshPOST` API will return a 500 error with a message instructing to set `OlderCookieDomain`.


**Example:**

- `APIDomain`: 'api.example.com'
- `CookieDomain`: 'api.example.com'

**Flow:**

1. After authentication, the frontend has cookies set with `domain=api.example.com`, but the access token has expired.
2. The server updates `CookieDomain` to `.example.com`.
3. An API call requiring session with an expired access token (cookie with `domain=api.example.com`) results in a 401 response.
4. The frontend attempts to refresh the session, generating a new access token saved with `domain=.example.com`.
5. The original API call is retried, but because it sends both the old and new cookies, it again results in a 401 response.
6. The frontend tries to refresh the session with multiple access tokens:
- If `OlderCookieDomain` is not set, the refresh fails with a 500 error.
- The user remains stuck until they clear cookies manually or `OlderCookieDomain` is set.
- If `OlderCookieDomain` is set, the refresh clears the older cookie, returning a 200 response.
- The frontend retries the original API call, sending only the new cookie (`domain=.example.com`), resulting in a successful request.

## [0.17.5] - 2024-03-14
- Adds a type uint64 to the `accessTokenCookiesExpiryDurationMillis` local variable in `recipe/session/utils.go`. It also removes the redundant `uint64` type forcing needed because of the untyped variable.
Expand Down
5 changes: 5 additions & 0 deletions recipe/session/apiImplementation.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ import (

func MakeAPIImplementation() sessmodels.APIInterface {
refreshPOST := func(options sessmodels.APIOptions, userContext supertokens.UserContext) (sessmodels.SessionContainer, error) {
err := ClearSessionCookiesFromOlderCookieDomain(options.Req, options.Res, options.Config, userContext)
if err != nil {
return nil, err
}

return RefreshSessionInRequest(options.Req, options.Res, options.Config, options.RecipeImplementation, userContext)
}

Expand Down
67 changes: 67 additions & 0 deletions recipe/session/cookieAndHeaders.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"strings"
"time"

errors2 "github.com/supertokens/supertokens-golang/recipe/session/errors"

"github.com/supertokens/supertokens-golang/supertokens"

"github.com/supertokens/supertokens-golang/recipe/session/sessmodels"
Expand Down Expand Up @@ -316,3 +318,68 @@ func getCookieName(cookie string) string {
}
return kv[0]
}

// ClearSessionCookiesFromOlderCookieDomain addresses an edge case where changing the cookieDomain config on the server can
// lead to session integrity issues. For instance, if the API server URL is 'api.example.com'
// with a cookie domain of '.example.com', and the server updates the cookie domain to 'api.example.com',
// the client may retain cookies with both '.example.com' and 'api.example.com' domains.
//
// Consequently, if the server chooses the older cookie, session invalidation occurs, potentially
// resulting in an infinite refresh loop. To fix this, users are asked to specify "OlderCookieDomain" in
// the config.
//
// This function checks for multiple cookies with the same name and clears the cookies for the older domain.
func ClearSessionCookiesFromOlderCookieDomain(req *http.Request, res http.ResponseWriter, config sessmodels.TypeNormalisedInput, userContext supertokens.UserContext) error {
allowedTransferMethod := config.GetTokenTransferMethod(req, false, userContext)

// If the transfer method is 'header', there's no need to clear cookies immediately, even if there are multiple in the request.
if allowedTransferMethod == sessmodels.HeaderTransferMethod {
return nil
}

didClearCookies := false

tokenTypes := []sessmodels.TokenType{sessmodels.AccessToken, sessmodels.RefreshToken}
for _, token := range tokenTypes {
if hasMultipleCookiesForTokenType(req, token) {
// If a request has multiple session cookies and 'olderCookieDomain' is
// unset, we can't identify the correct cookie for refreshing the session.
// Using the wrong cookie can cause an infinite refresh loop. To avoid this,
// we throw a 500 error asking the user to set 'olderCookieDomain'.
if config.OlderCookieDomain == nil {
return errors.New(`The request contains multiple session cookies. This may happen if you've changed the 'cookieDomain' value in your configuration. To clear tokens from the previous domain, set 'olderCookieDomain' in your config.`)
}

supertokens.LogDebugMessage(fmt.Sprint("ClearSessionCookiesFromOlderCookieDomain: Clearing duplicate ", token, " cookie with domain ", config.OlderCookieDomain))
config.CookieDomain = config.OlderCookieDomain
setToken(config, res, token, "", 0, sessmodels.CookieTransferMethod, req, userContext)

didClearCookies = true
}
}

if didClearCookies {
return errors2.ClearDuplicateSessionCookiesError{
Msg: "The request contains multiple session cookies. We are clearing the cookie from OlderCookieDomain. Session will be refreshed in the next refresh call.",
}
}

return nil
}

func hasMultipleCookiesForTokenType(req *http.Request, tokenType sessmodels.TokenType) bool {
// Count of cookies with the specified token type
count := 0

// Loop through each cookie in the request
for _, cookie := range req.Cookies() {
// Check if the cookie's name matches the token type
cookieName, _ := getCookieNameFromTokenType(tokenType)
if cookie.Name == cookieName {
count++
}
}

// If count is greater than 1, then there are multiple cookies with the given token type
return count > 1
}
17 changes: 13 additions & 4 deletions recipe/session/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ package errors
import "github.com/supertokens/supertokens-golang/recipe/session/claims"

const (
UnauthorizedErrorStr = "UNAUTHORISED"
TryRefreshTokenErrorStr = "TRY_REFRESH_TOKEN"
TokenTheftDetectedErrorStr = "TOKEN_THEFT_DETECTED"
InvalidClaimsErrorStr = "INVALID_CLAIMS"
UnauthorizedErrorStr = "UNAUTHORISED"
TryRefreshTokenErrorStr = "TRY_REFRESH_TOKEN"
TokenTheftDetectedErrorStr = "TOKEN_THEFT_DETECTED"
InvalidClaimsErrorStr = "INVALID_CLAIMS"
ClearDuplicateSessionCookiesErrorStr = "CLEAR_DUPLICATE_SESSION_COOKIES"
)

// TryRefreshTokenError used for when the refresh API needs to be called
Expand Down Expand Up @@ -66,3 +67,11 @@ type InvalidClaimError struct {
func (err InvalidClaimError) Error() string {
return err.Msg
}

type ClearDuplicateSessionCookiesError struct {
Msg string
}

func (err ClearDuplicateSessionCookiesError) Error() string {
return err.Msg
}
7 changes: 7 additions & 0 deletions recipe/session/recipe.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,13 @@ func (r *Recipe) handleError(err error, req *http.Request, res http.ResponseWrit
supertokens.LogDebugMessage("errorHandler: returning INVALID_CLAIMS")
errs := err.(errors.InvalidClaimError)
return true, r.Config.ErrorHandlers.OnInvalidClaim(errs.InvalidClaims, req, res)
} else if defaultErrors.As(err, &errors.ClearDuplicateSessionCookiesError{}) {
supertokens.LogDebugMessage("errorHandler: returning CLEAR_DUPLICATE_SESSION_COOKIES")
// This error occurs in the `refreshPOST` API when multiple session
// cookies are found in the request and the user has set `olderCookieDomain`.
// We remove session cookies from the olderCookieDomain. The response must return `200 OK`
// to avoid logging out the user, allowing the session to continue with the valid cookie.
return true, r.Config.ErrorHandlers.OnClearDuplicateSessionCookies(err.Error(), req, res)
} else {
return r.OpenIdRecipe.RecipeModule.HandleError(err, req, res, userContext)
}
Expand Down
27 changes: 26 additions & 1 deletion recipe/session/sessionRequestFunctions.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,18 @@ func GetSessionFromRequest(req *http.Request, res http.ResponseWriter, config se
accessToken = accessTokens[sessmodels.HeaderTransferMethod]
} else if (allowedTokenTransferMethod == sessmodels.AnyTransferMethod || allowedTokenTransferMethod == sessmodels.CookieTransferMethod) && (accessTokens[sessmodels.CookieTransferMethod] != nil) {
supertokens.LogDebugMessage("getSession: using cookie transfer method")

// If multiple access tokens exist in the request cookie, throw TRY_REFRESH_TOKEN.
// This prompts the client to call the refresh endpoint, clearing olderCookieDomain cookies (if set).
// ensuring outdated token payload isn't used.
if hasMultipleCookiesForTokenType(req, sessmodels.AccessToken) {
supertokens.LogDebugMessage("getSession: Throwing TRY_REFRESH_TOKEN because multiple access tokens are present in request cookies")

return nil, errors.TryRefreshTokenError{
Msg: "Multiple access tokens present in the request cookies.",
}
}

cookieMethod := sessmodels.CookieTransferMethod
requestTokenTransferMethod = &cookieMethod
accessToken = accessTokens[sessmodels.CookieTransferMethod]
Expand Down Expand Up @@ -344,7 +356,20 @@ func RefreshSessionInRequest(req *http.Request, res http.ResponseWriter, config
setCookie(config, res, legacyIdRefreshTokenCookieName, "", 0, "accessTokenPath", req, userContext)
}

supertokens.LogDebugMessage("refreshSession: UNAUTHORISED because refresh token in request is undefined")
// We need to clear the access token cookie if
// - the refresh token is not found, and
// - the allowedTransferMethod is 'cookie' or 'any', and
// - an access token cookie exists (otherwise it'd be a no-op)
// See: https://github.com/supertokens/supertokens-node/issues/790
token, err := GetToken(req, sessmodels.AccessToken, sessmodels.CookieTransferMethod)
if err != nil {
return nil, err
}
if (allowedTokenTransferMethod == sessmodels.AnyTransferMethod || allowedTokenTransferMethod == sessmodels.CookieTransferMethod) && token != nil {
setCookie(config, res, "sAccessToken", "", 0, "accessTokenPath", req, userContext)
supertokens.LogDebugMessage("refreshSession: cleared access token and returning UNAUTHORISED because refresh token in request is undefined")
}

False := false
return nil, errors.UnauthorizedError{
Msg: "Refresh token not found. Are you sending the refresh token in the request as a cookie?",
Expand Down
Loading

0 comments on commit 197c6be

Please sign in to comment.