Skip to content

Commit

Permalink
fix: remove server side cookie token methods (#1742)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
J0 authored Aug 28, 2024
1 parent 2d51956 commit c6efec4
Show file tree
Hide file tree
Showing 14 changed files with 2 additions and 131 deletions.
3 changes: 0 additions & 3 deletions example.env
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 0 additions & 3 deletions internal/api/anonymous.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 0 additions & 4 deletions internal/api/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions internal/api/external.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 0 additions & 3 deletions internal/api/logout.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
8 changes: 0 additions & 8 deletions internal/api/logout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
}
6 changes: 0 additions & 6 deletions internal/api/mfa.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
1 change: 0 additions & 1 deletion internal/api/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
4 changes: 0 additions & 4 deletions internal/api/samlacs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
4 changes: 0 additions & 4 deletions internal/api/signup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
58 changes: 0 additions & 58 deletions internal/api/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package api

import (
"context"
"errors"
"net/http"
"net/url"
"strconv"
Expand All @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)

Expand Down
5 changes: 0 additions & 5 deletions internal/api/token_refresh.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
})
Expand Down
9 changes: 0 additions & 9 deletions internal/api/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
21 changes: 2 additions & 19 deletions internal/conf/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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{}
}
Expand Down

0 comments on commit c6efec4

Please sign in to comment.