From f6464b4b6a8919a1f8bdfc2e478c6e64e9fa5a26 Mon Sep 17 00:00:00 2001 From: Joel Lee Date: Wed, 28 Aug 2024 09:11:18 +0800 Subject: [PATCH] fix: remove server side cookie token methods (#1742) ## What kind of change does this PR introduce? Remove set cookie tokens and clear cookie token methods as it looks like they aren't done server side. Task: https://www.notion.so/supabase/team-auth-113cb19144c1419ea5fb1d600281d959?p=ff083dad758e44ef9b4e230804e0fee7&pm=s --- example.env | 3 -- internal/api/anonymous.go | 3 -- internal/api/auth.go | 4 --- internal/api/external.go | 4 --- internal/api/logout.go | 3 -- internal/api/logout_test.go | 8 ----- internal/api/mfa.go | 6 ---- internal/api/middleware.go | 1 - internal/api/samlacs.go | 4 --- internal/api/signup.go | 4 --- internal/api/token.go | 58 ---------------------------------- internal/api/token_refresh.go | 5 --- internal/api/verify.go | 9 ------ internal/conf/configuration.go | 21 ++---------- 14 files changed, 2 insertions(+), 131 deletions(-) diff --git a/example.env b/example.env index e408dcb40..01371183e 100644 --- a/example.env +++ b/example.env @@ -216,9 +216,6 @@ GOTRUE_OPERATOR_TOKEN="unused-operator-token" GOTRUE_RATE_LIMIT_HEADER="X-Forwarded-For" GOTRUE_RATE_LIMIT_EMAIL_SENT="100" -# Cookie config -GOTRUE_COOKIE_KEY="sb" -GOTRUE_COOKIE_DOMAIN="localhost" GOTRUE_MAX_VERIFIED_FACTORS=10 # Auth Hook Configuration diff --git a/internal/api/anonymous.go b/internal/api/anonymous.go index fada3cb65..294f860cb 100644 --- a/internal/api/anonymous.go +++ b/internal/api/anonymous.go @@ -44,9 +44,6 @@ func (a *API) SignupAnonymously(w http.ResponseWriter, r *http.Request) error { if terr != nil { return terr } - if terr := a.setCookieTokens(config, token, false, w); terr != nil { - return terr - } return nil }) if err != nil { diff --git a/internal/api/auth.go b/internal/api/auth.go index e881865dd..b03767f02 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -16,21 +16,17 @@ import ( // requireAuthentication checks incoming requests for tokens presented using the Authorization header func (a *API) requireAuthentication(w http.ResponseWriter, r *http.Request) (context.Context, error) { token, err := a.extractBearerToken(r) - config := a.config if err != nil { - a.clearCookieTokens(config, w) return nil, err } ctx, err := a.parseJWTClaims(token, r) if err != nil { - a.clearCookieTokens(config, w) return ctx, err } ctx, err = a.maybeLoadUserOrSession(ctx) if err != nil { - a.clearCookieTokens(config, w) return ctx, err } return ctx, err diff --git a/internal/api/external.go b/internal/api/external.go index c609e53c0..d0cec1536 100644 --- a/internal/api/external.go +++ b/internal/api/external.go @@ -164,7 +164,6 @@ func (a *API) handleOAuthCallback(r *http.Request) (*OAuthProviderData, error) { func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Request) error { ctx := r.Context() db := a.db.WithContext(ctx) - config := a.config var grantParams models.GrantParams grantParams.FillGrantParams(r) @@ -264,9 +263,6 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re rurl = token.AsRedirectURL(rurl, q) - if err := a.setCookieTokens(config, token, false, w); err != nil { - return internalServerError("Failed to set JWT cookie. %s", err) - } } http.Redirect(w, r, rurl, http.StatusFound) diff --git a/internal/api/logout.go b/internal/api/logout.go index ea5b871f7..a2c31a312 100644 --- a/internal/api/logout.go +++ b/internal/api/logout.go @@ -20,8 +20,6 @@ const ( func (a *API) Logout(w http.ResponseWriter, r *http.Request) error { ctx := r.Context() db := a.db.WithContext(ctx) - config := a.config - scope := LogoutGlobal if r.URL.Query() != nil { @@ -64,7 +62,6 @@ func (a *API) Logout(w http.ResponseWriter, r *http.Request) error { return internalServerError("Error logging out user").WithInternalError(err) } - a.clearCookieTokens(config, w) w.WriteHeader(http.StatusNoContent) return nil diff --git a/internal/api/logout_test.go b/internal/api/logout_test.go index 3a4094109..b1a0fdbb6 100644 --- a/internal/api/logout_test.go +++ b/internal/api/logout_test.go @@ -71,13 +71,5 @@ func (ts *LogoutTestSuite) TestLogoutSuccess() { ts.API.handler.ServeHTTP(w, req) require.Equal(ts.T(), http.StatusNoContent, w.Code) - - accessTokenKey := fmt.Sprintf("%v-access-token", ts.Config.Cookie.Key) - refreshTokenKey := fmt.Sprintf("%v-refresh-token", ts.Config.Cookie.Key) - for _, c := range w.Result().Cookies() { - if c.Name == accessTokenKey || c.Name == refreshTokenKey { - require.Equal(ts.T(), "", c.Value) - } - } } } diff --git a/internal/api/mfa.go b/internal/api/mfa.go index 70c263fba..357eea34c 100644 --- a/internal/api/mfa.go +++ b/internal/api/mfa.go @@ -544,9 +544,6 @@ func (a *API) verifyTOTPFactor(w http.ResponseWriter, r *http.Request, params *V if terr != nil { return terr } - if terr = a.setCookieTokens(config, token, false, w); terr != nil { - return internalServerError("Failed to set JWT cookie. %s", terr) - } if terr = models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil { return internalServerError("Failed to update sessions. %s", terr) } @@ -663,9 +660,6 @@ func (a *API) verifyPhoneFactor(w http.ResponseWriter, r *http.Request, params * if terr != nil { return terr } - if terr = a.setCookieTokens(config, token, false, w); terr != nil { - return internalServerError("Failed to set JWT cookie. %s", terr) - } if terr = models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil { return internalServerError("Failed to update sessions. %s", terr) } diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 8caa5d885..972ac5ab3 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -143,7 +143,6 @@ func (a *API) requireAdminCredentials(w http.ResponseWriter, req *http.Request) ctx, err := a.parseJWTClaims(t, req) if err != nil { - a.clearCookieTokens(a.config, w) return nil, err } diff --git a/internal/api/samlacs.go b/internal/api/samlacs.go index 907efcd4c..cc99a5274 100644 --- a/internal/api/samlacs.go +++ b/internal/api/samlacs.go @@ -307,10 +307,6 @@ func (a *API) handleSamlAcs(w http.ResponseWriter, r *http.Request) error { return err } - if err := a.setCookieTokens(config, token, false, w); err != nil { - return internalServerError("Failed to set JWT cookie").WithInternalError(err) - } - if !utilities.IsRedirectURLValid(config, redirectTo) { redirectTo = config.SiteURL } diff --git a/internal/api/signup.go b/internal/api/signup.go index cc5e189e8..d7d946c8c 100644 --- a/internal/api/signup.go +++ b/internal/api/signup.go @@ -323,10 +323,6 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error { if terr != nil { return terr } - - if terr = a.setCookieTokens(config, token, false, w); terr != nil { - return internalServerError("Failed to set JWT cookie. %s", terr) - } return nil }) if err != nil { diff --git a/internal/api/token.go b/internal/api/token.go index f9a13ac00..d8790e679 100644 --- a/internal/api/token.go +++ b/internal/api/token.go @@ -2,7 +2,6 @@ package api import ( "context" - "errors" "net/http" "net/url" "strconv" @@ -14,7 +13,6 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/xeipuuv/gojsonschema" - "github.com/supabase/auth/internal/conf" "github.com/supabase/auth/internal/hooks" "github.com/supabase/auth/internal/metering" "github.com/supabase/auth/internal/models" @@ -224,9 +222,6 @@ func (a *API) ResourceOwnerPasswordGrant(ctx context.Context, w http.ResponseWri return terr } - if terr = a.setCookieTokens(config, token, false, w); terr != nil { - return internalServerError("Failed to set JWT cookie. %s", terr) - } return nil }) if err != nil { @@ -486,59 +481,6 @@ func (a *API) updateMFASessionAndClaims(r *http.Request, tx *storage.Connection, }, nil } -// setCookieTokens sets the access_token & refresh_token in the cookies -func (a *API) setCookieTokens(config *conf.GlobalConfiguration, token *AccessTokenResponse, session bool, w http.ResponseWriter) error { - // don't need to catch error here since we always set the cookie name - _ = a.setCookieToken(config, "access-token", token.Token, session, w) - _ = a.setCookieToken(config, "refresh-token", token.RefreshToken, session, w) - return nil -} - -func (a *API) setCookieToken(config *conf.GlobalConfiguration, name string, tokenString string, session bool, w http.ResponseWriter) error { - if name == "" { - return errors.New("failed to set cookie, invalid name") - } - cookieName := config.Cookie.Key + "-" + name - exp := time.Second * time.Duration(config.Cookie.Duration) - cookie := &http.Cookie{ - Name: cookieName, - Value: tokenString, - Secure: true, - HttpOnly: true, - Path: "/", - Domain: config.Cookie.Domain, - } - if !session { - cookie.Expires = time.Now().Add(exp) - cookie.MaxAge = config.Cookie.Duration - } - - http.SetCookie(w, cookie) - return nil -} - -func (a *API) clearCookieTokens(config *conf.GlobalConfiguration, w http.ResponseWriter) { - a.clearCookieToken(config, "access-token", w) - a.clearCookieToken(config, "refresh-token", w) -} - -func (a *API) clearCookieToken(config *conf.GlobalConfiguration, name string, w http.ResponseWriter) { - cookieName := config.Cookie.Key - if name != "" { - cookieName += "-" + name - } - http.SetCookie(w, &http.Cookie{ - Name: cookieName, - Value: "", - Expires: time.Now().Add(-1 * time.Hour * 10), - MaxAge: -1, - Secure: true, - HttpOnly: true, - Path: "/", - Domain: config.Cookie.Domain, - }) -} - func validateTokenClaims(outputClaims map[string]interface{}) error { schemaLoader := gojsonschema.NewStringLoader(hooks.MinimumViableTokenSchema) diff --git a/internal/api/token_refresh.go b/internal/api/token_refresh.go index 5c9d1984d..8f1ddd320 100644 --- a/internal/api/token_refresh.go +++ b/internal/api/token_refresh.go @@ -182,9 +182,7 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h time.Second * time.Duration(config.Security.RefreshTokenReuseInterval)) if a.Now().After(reuseUntil) { - a.clearCookieTokens(config, w) // not OK to reuse this token - if config.Security.RefreshTokenRotationEnabled { // Revoke all tokens in token family if err := models.RevokeTokenFamily(tx, token); err != nil { @@ -248,9 +246,6 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h RefreshToken: issuedToken.Token, User: user, } - if terr = a.setCookieTokens(config, newTokenResponse, false, w); terr != nil { - return internalServerError("Failed to set JWT cookie. %s", terr) - } return nil }) diff --git a/internal/api/verify.go b/internal/api/verify.go index 74a25bd85..5adc80744 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -117,7 +117,6 @@ func (a *API) Verify(w http.ResponseWriter, r *http.Request) error { func (a *API) verifyGet(w http.ResponseWriter, r *http.Request, params *VerifyParams) error { ctx := r.Context() db := a.db.WithContext(ctx) - config := a.config var ( user *models.User @@ -185,9 +184,6 @@ func (a *API) verifyGet(w http.ResponseWriter, r *http.Request, params *VerifyPa return terr } - if terr = a.setCookieTokens(config, token, false, w); terr != nil { - return internalServerError("Failed to set JWT cookie. %s", terr) - } } else if isPKCEFlow(flowType) { if authCode, terr = issueAuthCode(tx, user, authenticationMethod); terr != nil { return badRequestError(ErrorCodeFlowStateNotFound, "No associated flow state found. %s", terr) @@ -227,7 +223,6 @@ func (a *API) verifyGet(w http.ResponseWriter, r *http.Request, params *VerifyPa func (a *API) verifyPost(w http.ResponseWriter, r *http.Request, params *VerifyParams) error { ctx := r.Context() db := a.db.WithContext(ctx) - config := a.config var ( user *models.User @@ -285,10 +280,6 @@ func (a *API) verifyPost(w http.ResponseWriter, r *http.Request, params *VerifyP if terr != nil { return terr } - - if terr = a.setCookieTokens(config, token, false, w); terr != nil { - return internalServerError("Failed to set JWT cookie. %s", terr) - } return nil }) if err != nil { diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go index 8426616c2..7a0cffab4 100644 --- a/internal/conf/configuration.go +++ b/internal/conf/configuration.go @@ -257,13 +257,8 @@ type GlobalConfiguration struct { Security SecurityConfiguration `json:"security"` Sessions SessionsConfiguration `json:"sessions"` MFA MFAConfiguration `json:"MFA"` - Cookie struct { - Key string `json:"key"` - Domain string `json:"domain"` - Duration int `json:"duration"` - } `json:"cookies"` - SAML SAMLConfiguration `json:"saml"` - CORS CORSConfiguration `json:"cors"` + SAML SAMLConfiguration `json:"saml"` + CORS CORSConfiguration `json:"cors"` } type CORSConfiguration struct { @@ -844,18 +839,6 @@ func (config *GlobalConfiguration) ApplyDefaults() error { config.Sms.Template = "" } - if config.Cookie.Key == "" { - config.Cookie.Key = "sb" - } - - if config.Cookie.Domain == "" { - config.Cookie.Domain = "" - } - - if config.Cookie.Duration == 0 { - config.Cookie.Duration = 86400 - } - if config.URIAllowList == nil { config.URIAllowList = []string{} }