From 5460271bb51290c2b0acf2f00001096e2b12c3e2 Mon Sep 17 00:00:00 2001 From: Tobi Smethurst <31960611+tsmethurst@users.noreply.github.com> Date: Thu, 8 Jul 2021 11:32:31 +0200 Subject: [PATCH] Auth flow fixes (#82) * preliminary fixes to broken auth flow * fix some auth/cookie weirdness * fmt --- internal/api/client/auth/auth.go | 18 ++++++ internal/api/client/auth/authorize.go | 89 ++++++++++++++------------- internal/api/client/auth/signin.go | 2 +- internal/api/model/oauth.go | 10 +-- internal/router/session.go | 9 +++ 5 files changed, 79 insertions(+), 49 deletions(-) diff --git a/internal/api/client/auth/auth.go b/internal/api/client/auth/auth.go index 793c19f4e..7cddc3e74 100644 --- a/internal/api/client/auth/auth.go +++ b/internal/api/client/auth/auth.go @@ -36,8 +36,26 @@ const ( OauthTokenPath = "/oauth/token" // OauthAuthorizePath is the API path for authorization requests (eg., authorize this app to act on my behalf as a user) OauthAuthorizePath = "/oauth/authorize" + + sessionUserID = "userid" + sessionClientID = "client_id" + sessionRedirectURI = "redirect_uri" + sessionForceLogin = "force_login" + sessionResponseType = "response_type" + sessionCode = "code" + sessionScope = "scope" ) +var sessionKeys []string = []string{ + sessionUserID, + sessionClientID, + sessionRedirectURI, + sessionForceLogin, + sessionResponseType, + sessionCode, + sessionScope, +} + // Module implements the ClientAPIModule interface for type Module struct { config *config.Config diff --git a/internal/api/client/auth/authorize.go b/internal/api/client/auth/authorize.go index 7661019db..fb16b00fc 100644 --- a/internal/api/client/auth/authorize.go +++ b/internal/api/client/auth/authorize.go @@ -26,7 +26,6 @@ import ( "github.com/gin-contrib/sessions" "github.com/gin-gonic/gin" - "github.com/sirupsen/logrus" "github.com/superseriousbusiness/gotosocial/internal/api/model" "github.com/superseriousbusiness/gotosocial/internal/db" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" @@ -38,25 +37,30 @@ import ( func (m *Module) AuthorizeGETHandler(c *gin.Context) { l := m.log.WithField("func", "AuthorizeGETHandler") s := sessions.Default(c) - s.Options(sessions.Options{ - MaxAge: 120, // give the user 2 minutes to sign in before expiring their session - }) // UserID will be set in the session by AuthorizePOSTHandler if the caller has already gone through the authentication flow // If it's not set, then we don't know yet who the user is, so we need to redirect them to the sign in page. - userID, ok := s.Get("userid").(string) + userID, ok := s.Get(sessionUserID).(string) if !ok || userID == "" { l.Trace("userid was empty, parsing form then redirecting to sign in page") - if err := parseAuthForm(c, l); err != nil { - c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) - } else { - c.Redirect(http.StatusFound, AuthSignInPath) + form := &model.OAuthAuthorize{} + if err := c.Bind(form); err != nil { + l.Debugf("invalid auth form: %s", err) + return } + l.Tracef("parsed auth form: %+v", form) + + if err := extractAuthForm(s, form); err != nil { + l.Debugf(fmt.Sprintf("error parsing form at /oauth/authorize: %s", err)) + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + c.Redirect(http.StatusFound, AuthSignInPath) return } // We can use the client_id on the session to retrieve info about the app associated with the client_id - clientID, ok := s.Get("client_id").(string) + clientID, ok := s.Get(sessionClientID).(string) if !ok || clientID == "" { c.JSON(http.StatusInternalServerError, gin.H{"error": "no client_id found in session"}) return @@ -64,7 +68,7 @@ func (m *Module) AuthorizeGETHandler(c *gin.Context) { app := >smodel.Application{ ClientID: clientID, } - if err := m.db.GetWhere([]db.Where{{Key: "client_id", Value: app.ClientID}}, app); err != nil { + if err := m.db.GetWhere([]db.Where{{Key: sessionClientID, Value: app.ClientID}}, app); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("no application found for client id %s", clientID)}) return } @@ -88,12 +92,12 @@ func (m *Module) AuthorizeGETHandler(c *gin.Context) { } // Finally we should also get the redirect and scope of this particular request, as stored in the session. - redirect, ok := s.Get("redirect_uri").(string) + redirect, ok := s.Get(sessionRedirectURI).(string) if !ok || redirect == "" { c.JSON(http.StatusInternalServerError, gin.H{"error": "no redirect_uri found in session"}) return } - scope, ok := s.Get("scope").(string) + scope, ok := s.Get(sessionScope).(string) if !ok || scope == "" { c.JSON(http.StatusInternalServerError, gin.H{"error": "no scope found in session"}) return @@ -107,7 +111,7 @@ func (m *Module) AuthorizeGETHandler(c *gin.Context) { "appname": app.Name, "appwebsite": app.Website, "redirect": redirect, - "scope": scope, + sessionScope: scope, "user": acct.Username, }) } @@ -123,39 +127,47 @@ func (m *Module) AuthorizePOSTHandler(c *gin.Context) { // We need to retrieve the original form submitted to the authorizeGEThandler, and // recreate it on the request so that it can be used further by the oauth2 library. // So first fetch all the values from the session. - forceLogin, ok := s.Get("force_login").(string) + + forceLogin, ok := s.Get(sessionForceLogin).(string) if !ok { c.JSON(http.StatusBadRequest, gin.H{"error": "session missing force_login"}) return } - responseType, ok := s.Get("response_type").(string) + + responseType, ok := s.Get(sessionResponseType).(string) if !ok || responseType == "" { c.JSON(http.StatusBadRequest, gin.H{"error": "session missing response_type"}) return } - clientID, ok := s.Get("client_id").(string) + + clientID, ok := s.Get(sessionClientID).(string) if !ok || clientID == "" { c.JSON(http.StatusBadRequest, gin.H{"error": "session missing client_id"}) return } - redirectURI, ok := s.Get("redirect_uri").(string) + + redirectURI, ok := s.Get(sessionRedirectURI).(string) if !ok || redirectURI == "" { c.JSON(http.StatusBadRequest, gin.H{"error": "session missing redirect_uri"}) return } - scope, ok := s.Get("scope").(string) + + scope, ok := s.Get(sessionScope).(string) if !ok { c.JSON(http.StatusBadRequest, gin.H{"error": "session missing scope"}) return } - userID, ok := s.Get("userid").(string) + + userID, ok := s.Get(sessionUserID).(string) if !ok { c.JSON(http.StatusBadRequest, gin.H{"error": "session missing userid"}) return } // we're done with the session so we can clear it now - s.Clear() + for _, key := range sessionKeys { + s.Delete(key) + } if err := s.Save(); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return @@ -163,12 +175,12 @@ func (m *Module) AuthorizePOSTHandler(c *gin.Context) { // now set the values on the request values := url.Values{} - values.Set("force_login", forceLogin) - values.Set("response_type", responseType) - values.Set("client_id", clientID) - values.Set("redirect_uri", redirectURI) - values.Set("scope", scope) - values.Set("userid", userID) + values.Set(sessionForceLogin, forceLogin) + values.Set(sessionResponseType, responseType) + values.Set(sessionClientID, clientID) + values.Set(sessionRedirectURI, redirectURI) + values.Set(sessionScope, scope) + values.Set(sessionUserID, userID) c.Request.Form = values l.Tracef("values on request set to %+v", c.Request.Form) @@ -178,18 +190,9 @@ func (m *Module) AuthorizePOSTHandler(c *gin.Context) { } } -// parseAuthForm parses the OAuthAuthorize form in the gin context, and stores +// extractAuthForm checks the given OAuthAuthorize form, and stores // the values in the form into the session. -func parseAuthForm(c *gin.Context, l *logrus.Entry) error { - s := sessions.Default(c) - - // first make sure they've filled out the authorize form with the required values - form := &model.OAuthAuthorize{} - if err := c.ShouldBind(form); err != nil { - return err - } - l.Tracef("parsed form: %+v", form) - +func extractAuthForm(s sessions.Session, form *model.OAuthAuthorize) error { // these fields are *required* so check 'em if form.ResponseType == "" || form.ClientID == "" || form.RedirectURI == "" { return errors.New("missing one of: response_type, client_id or redirect_uri") @@ -201,10 +204,10 @@ func parseAuthForm(c *gin.Context, l *logrus.Entry) error { } // save these values from the form so we can use them elsewhere in the session - s.Set("force_login", form.ForceLogin) - s.Set("response_type", form.ResponseType) - s.Set("client_id", form.ClientID) - s.Set("redirect_uri", form.RedirectURI) - s.Set("scope", form.Scope) + s.Set(sessionForceLogin, form.ForceLogin) + s.Set(sessionResponseType, form.ResponseType) + s.Set(sessionClientID, form.ClientID) + s.Set(sessionRedirectURI, form.RedirectURI) + s.Set(sessionScope, form.Scope) return s.Save() } diff --git a/internal/api/client/auth/signin.go b/internal/api/client/auth/signin.go index 158cc5c4c..7974a8cfa 100644 --- a/internal/api/client/auth/signin.go +++ b/internal/api/client/auth/signin.go @@ -62,7 +62,7 @@ func (m *Module) SignInPOSTHandler(c *gin.Context) { return } - s.Set("userid", userid) + s.Set(sessionUserID, userid) if err := s.Save(); err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return diff --git a/internal/api/model/oauth.go b/internal/api/model/oauth.go index 250d2218f..10c150474 100644 --- a/internal/api/model/oauth.go +++ b/internal/api/model/oauth.go @@ -22,16 +22,16 @@ package model // See here: https://docs.joinmastodon.org/methods/apps/oauth/ type OAuthAuthorize struct { // Forces the user to re-login, which is necessary for authorizing with multiple accounts from the same instance. - ForceLogin string `form:"force_login,omitempty"` + ForceLogin string `form:"force_login" json:"force_login"` // Should be set equal to `code`. - ResponseType string `form:"response_type"` + ResponseType string `form:"response_type" json:"response_type"` // Client ID, obtained during app registration. - ClientID string `form:"client_id"` + ClientID string `form:"client_id" json:"client_id"` // Set a URI to redirect the user to. // If this parameter is set to urn:ietf:wg:oauth:2.0:oob then the authorization code will be shown instead. // Must match one of the redirect URIs declared during app registration. - RedirectURI string `form:"redirect_uri"` + RedirectURI string `form:"redirect_uri" json:"redirect_uri"` // List of requested OAuth scopes, separated by spaces (or by pluses, if using query parameters). // Must be a subset of scopes declared during app registration. If not provided, defaults to read. - Scope string `form:"scope,omitempty"` + Scope string `form:"scope" json:"scope"` } diff --git a/internal/router/session.go b/internal/router/session.go index a1ac09d28..2d00f7677 100644 --- a/internal/router/session.go +++ b/internal/router/session.go @@ -22,6 +22,7 @@ import ( "crypto/rand" "errors" "fmt" + "net/http" "github.com/gin-contrib/sessions" "github.com/gin-contrib/sessions/memstore" @@ -63,6 +64,14 @@ func useSession(cfg *config.Config, dbService db.DB, engine *gin.Engine) error { } store := memstore.NewStore(rs.Auth, rs.Crypt) + store.Options(sessions.Options{ + Path: "/", + Domain: cfg.Host, + MaxAge: 120, // 2 minutes + Secure: true, // only use cookie over https + HttpOnly: true, // exclude javascript from inspecting cookie + SameSite: http.SameSiteStrictMode, // https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-cookie-same-site-00#section-4.1.1 + }) sessionName := fmt.Sprintf("gotosocial-%s", cfg.Host) engine.Use(sessions.Sessions(sessionName, store)) return nil