From 8106b6985620956ce8cfa4126143a95ca87ea976 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Thu, 28 Jul 2022 16:43:27 +0200 Subject: [PATCH] [feature] add 'state' oauth2 param to /oauth/authorize (#730) --- internal/api/client/auth/auth.go | 15 ++++++++------- internal/api/client/auth/authorize.go | 12 +++++++++++- internal/api/client/auth/callback.go | 14 +++++++------- internal/api/client/auth/signin.go | 8 ++++---- internal/api/client/status/statuscreate_test.go | 2 +- internal/api/model/oauth.go | 4 ++++ 6 files changed, 35 insertions(+), 20 deletions(-) diff --git a/internal/api/client/auth/auth.go b/internal/api/client/auth/auth.go index a097f8004..b12b6d24f 100644 --- a/internal/api/client/auth/auth.go +++ b/internal/api/client/auth/auth.go @@ -55,13 +55,14 @@ callbackStateParam = "state" callbackCodeParam = "code" - sessionUserID = "userid" - sessionClientID = "client_id" - sessionRedirectURI = "redirect_uri" - sessionForceLogin = "force_login" - sessionResponseType = "response_type" - sessionScope = "scope" - sessionState = "state" + sessionUserID = "userid" + sessionClientID = "client_id" + sessionRedirectURI = "redirect_uri" + sessionForceLogin = "force_login" + sessionResponseType = "response_type" + sessionScope = "scope" + sessionInternalState = "internal_state" + sessionClientState = "client_state" ) // Module implements the ClientAPIModule interface for diff --git a/internal/api/client/auth/authorize.go b/internal/api/client/auth/authorize.go index 233dacfd2..1a594a319 100644 --- a/internal/api/client/auth/authorize.go +++ b/internal/api/client/auth/authorize.go @@ -189,6 +189,11 @@ func (m *Module) AuthorizePOSTHandler(c *gin.Context) { errs = append(errs, fmt.Sprintf("key %s was not found in session", sessionScope)) } + var clientState string + if s, ok := s.Get(sessionClientState).(string); ok { + clientState = s + } + userID, ok := s.Get(sessionUserID).(string) if !ok { errs = append(errs, fmt.Sprintf("key %s was not found in session", sessionUserID)) @@ -246,6 +251,10 @@ func (m *Module) AuthorizePOSTHandler(c *gin.Context) { sessionUserID: {userID}, } + if clientState != "" { + c.Request.Form.Set("state", clientState) + } + if err := m.processor.OAuthHandleAuthorizeRequest(c.Writer, c.Request); err != nil { api.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error(), helpfulAdvice), m.processor.InstanceGet) } @@ -285,7 +294,8 @@ func saveAuthFormToSession(s sessions.Session, form *model.OAuthAuthorize) gtser s.Set(sessionClientID, form.ClientID) s.Set(sessionRedirectURI, form.RedirectURI) s.Set(sessionScope, form.Scope) - s.Set(sessionState, uuid.NewString()) + s.Set(sessionInternalState, uuid.NewString()) + s.Set(sessionClientState, form.State) if err := s.Save(); err != nil { err := fmt.Errorf("error saving form values onto session: %s", err) diff --git a/internal/api/client/auth/callback.go b/internal/api/client/auth/callback.go index 34a4995c8..96a73a52f 100644 --- a/internal/api/client/auth/callback.go +++ b/internal/api/client/auth/callback.go @@ -45,26 +45,26 @@ func (m *Module) CallbackGETHandler(c *gin.Context) { // check the query vs session state parameter to mitigate csrf // https://auth0.com/docs/secure/attack-protection/state-parameters - state := c.Query(callbackStateParam) - if state == "" { + returnedInternalState := c.Query(callbackStateParam) + if returnedInternalState == "" { m.clearSession(s) err := fmt.Errorf("%s parameter not found on callback query", callbackStateParam) api.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGet) return } - savedStateI := s.Get(sessionState) - savedState, ok := savedStateI.(string) + savedInternalStateI := s.Get(sessionInternalState) + savedInternalState, ok := savedInternalStateI.(string) if !ok { m.clearSession(s) - err := fmt.Errorf("key %s was not found in session", sessionState) + err := fmt.Errorf("key %s was not found in session", sessionInternalState) api.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGet) return } - if state != savedState { + if returnedInternalState != savedInternalState { m.clearSession(s) - err := errors.New("mismatch between query state and session state") + err := errors.New("mismatch between callback state and saved state") api.ErrorHandler(c, gtserror.NewErrorUnauthorized(err, err.Error()), m.processor.InstanceGet) return } diff --git a/internal/api/client/auth/signin.go b/internal/api/client/auth/signin.go index b8f267f54..f9541d4c5 100644 --- a/internal/api/client/auth/signin.go +++ b/internal/api/client/auth/signin.go @@ -58,16 +58,16 @@ func (m *Module) SignInGETHandler(c *gin.Context) { // idp provider is in use, so redirect to it s := sessions.Default(c) - stateI := s.Get(sessionState) - state, ok := stateI.(string) + internalStateI := s.Get(sessionInternalState) + internalState, ok := internalStateI.(string) if !ok { m.clearSession(s) - err := fmt.Errorf("key %s was not found in session", sessionState) + err := fmt.Errorf("key %s was not found in session", sessionInternalState) api.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGet) return } - c.Redirect(http.StatusSeeOther, m.idp.AuthCodeURL(state)) + c.Redirect(http.StatusSeeOther, m.idp.AuthCodeURL(internalState)) } // SignInPOSTHandler should be served at https://example.org/auth/sign_in. diff --git a/internal/api/client/status/statuscreate_test.go b/internal/api/client/status/statuscreate_test.go index 2a1516d9b..93fe74175 100644 --- a/internal/api/client/status/statuscreate_test.go +++ b/internal/api/client/status/statuscreate_test.go @@ -312,7 +312,7 @@ func (suite *StatusCreateTestSuite) TestAttachNewMediaSuccess() { ctx.Request = httptest.NewRequest(http.MethodPost, fmt.Sprintf("http://localhost:8080/%s", status.BasePath), nil) // the endpoint we're hitting ctx.Request.Header.Set("accept", "application/json") ctx.Request.Form = url.Values{ - "status": {"here's an image attachment"}, + "status": {"here's an image attachment"}, "media_ids[]": {attachment.ID}, } suite.statusModule.StatusCreatePOSTHandler(ctx) diff --git a/internal/api/model/oauth.go b/internal/api/model/oauth.go index c86e4723e..e6dc0d42c 100644 --- a/internal/api/model/oauth.go +++ b/internal/api/model/oauth.go @@ -33,4 +33,8 @@ type OAuthAuthorize struct { // 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" json:"scope"` + // State is used by the application to store request-specific data and/or prevent CSRF attacks. + // The authorization server must return the unmodified state value back to the application. + // See https://www.oauth.com/oauth2-servers/authorization/the-authorization-request/ + State string `form:"state" json:"state"` }