diff --git a/app/handlers/oauth.go b/app/handlers/oauth.go index 4a2ffd959..3e8092191 100644 --- a/app/handlers/oauth.go +++ b/app/handlers/oauth.go @@ -163,14 +163,17 @@ func OAuthCallback() web.HandlerFunc { provider := c.Param("provider") state := c.QueryParam("state") - parts := strings.Split(state, "|") + claims, err := jwt.DecodeOAuthStateClaims(state) + if err != nil { + return c.Forbidden() + } - if parts[0] == "" { + if claims.Redirect == "" { log.Warnf(c, "Missing redirect URL in OAuth callback state for provider @{Provider}.", dto.Props{"Provider": provider}) return c.NotFound() } - redirectURL, err := url.ParseRequestURI(parts[0]) + redirectURL, err := url.ParseRequestURI(claims.Redirect) if err != nil { return c.Failure(err) } @@ -184,7 +187,7 @@ func OAuthCallback() web.HandlerFunc { if redirectURL.Path == fmt.Sprintf("/oauth/%s/echo", provider) { var query = redirectURL.Query() query.Set("code", code) - query.Set("identifier", parts[1]) + query.Set("identifier", claims.Identifier) redirectURL.RawQuery = query.Encode() return c.Redirect(redirectURL.String()) } @@ -221,7 +224,7 @@ func OAuthCallback() web.HandlerFunc { var query = redirectURL.Query() query.Set("code", code) query.Set("redirect", redirectURL.RequestURI()) - query.Set("identifier", parts[1]) + query.Set("identifier", claims.Identifier) redirectURL.RawQuery = query.Encode() redirectURL.Path = fmt.Sprintf("/oauth/%s/token", provider) return c.Redirect(redirectURL.String()) diff --git a/app/handlers/oauth_test.go b/app/handlers/oauth_test.go index 51d795812..58adcf1ca 100644 --- a/app/handlers/oauth_test.go +++ b/app/handlers/oauth_test.go @@ -60,6 +60,11 @@ func TestSignInByOAuthHandler_InvalidURL(t *testing.T) { RegisterT(t) bus.Init(&oauth.Service{}) + state, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://avengers.test.fider.io", + Identifier: "MY_SESSION_ID", + }) + server := mock.NewServer() code, response := server. AddParam("provider", app.FacebookProvider). @@ -69,7 +74,7 @@ func TestSignInByOAuthHandler_InvalidURL(t *testing.T) { Execute(handlers.SignInByOAuth()) Expect(code).Equals(http.StatusTemporaryRedirect) - Expect(response.Header().Get("Location")).Equals("https://www.facebook.com/v3.2/dialog/oauth?client_id=FB_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%2Foauth%2Ffacebook%2Fcallback&response_type=code&scope=public_profile+email&state=http%3A%2F%2Favengers.test.fider.io%7CMY_SESSION_ID") + Expect(response.Header().Get("Location")).Equals("https://www.facebook.com/v3.2/dialog/oauth?client_id=FB_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%2Foauth%2Ffacebook%2Fcallback&response_type=code&scope=public_profile+email&state="+state) } func TestSignInByOAuthHandler_AuthenticatedUser(t *testing.T) { @@ -102,8 +107,13 @@ func TestSignInByOAuthHandler_AuthenticatedUser_UsingEcho(t *testing.T) { Use(middlewares.Session()). Execute(handlers.SignInByOAuth()) + state, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://avengers.test.fider.io/oauth/facebook/echo", + Identifier: "MY_SESSION_ID", + }) + Expect(code).Equals(http.StatusTemporaryRedirect) - Expect(response.Header().Get("Location")).Equals("https://www.facebook.com/v3.2/dialog/oauth?client_id=FB_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%2Foauth%2Ffacebook%2Fcallback&response_type=code&scope=public_profile+email&state=http%3A%2F%2Favengers.test.fider.io%2Foauth%2Ffacebook%2Fecho%7CMY_SESSION_ID") + Expect(response.Header().Get("Location")).Equals("https://www.facebook.com/v3.2/dialog/oauth?client_id=FB_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%2Foauth%2Ffacebook%2Fcallback&response_type=code&scope=public_profile+email&state="+state) } func TestCallbackHandler_InvalidState(t *testing.T) { @@ -115,16 +125,20 @@ func TestCallbackHandler_InvalidState(t *testing.T) { AddParam("provider", app.FacebookProvider). Execute(handlers.OAuthCallback()) - Expect(code).Equals(http.StatusInternalServerError) + Expect(code).Equals(http.StatusForbidden) } func TestCallbackHandler_InvalidCode(t *testing.T) { RegisterT(t) server := mock.NewServer() + state, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://avengers.test.fider.io", + Identifier: "", + }) code, response := server. - WithURL("http://login.test.fider.io/oauth/callback?state=http://avengers.test.fider.io"). + WithURL("http://login.test.fider.io/oauth/callback?state="+state). AddParam("provider", app.FacebookProvider). Execute(handlers.OAuthCallback()) @@ -135,9 +149,14 @@ func TestCallbackHandler_InvalidCode(t *testing.T) { func TestCallbackHandler_SignIn(t *testing.T) { RegisterT(t) + state, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://avengers.test.fider.io", + Identifier: "888", + }) + server := mock.NewServer() code, response := server. - WithURL("http://login.test.fider.io/oauth/callback?state=http://avengers.test.fider.io|888&code=123"). + WithURL("http://login.test.fider.io/oauth/callback?state="+state+"&code=123"). AddParam("provider", app.FacebookProvider). Execute(handlers.OAuthCallback()) @@ -149,8 +168,13 @@ func TestCallbackHandler_SignIn_WithPath(t *testing.T) { RegisterT(t) server := mock.NewServer() + state, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://avengers.test.fider.io/some-page", + Identifier: "888", + }) + code, response := server. - WithURL("http://login.test.fider.io/oauth/callback?state=http://avengers.test.fider.io/some-page|888&code=123"). + WithURL("http://login.test.fider.io/oauth/callback?state="+state+"&code=123"). AddParam("provider", app.FacebookProvider). Execute(handlers.OAuthCallback()) @@ -175,9 +199,14 @@ func TestCallbackHandler_SignUp(t *testing.T) { return app.ErrNotFound }) + state, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://demo.test.fider.io/signup", + Identifier: "", + }) + server := mock.NewServer() code, response := server. - WithURL("http://login.test.fider.io/oauth/callback?state=http://demo.test.fider.io/signup&code=123"). + WithURL("http://login.test.fider.io/oauth/callback?state="+state+"&code=123"). AddParam("provider", app.FacebookProvider). Execute(handlers.OAuthCallback()) Expect(code).Equals(http.StatusTemporaryRedirect) diff --git a/app/pkg/jwt/jwt.go b/app/pkg/jwt/jwt.go index 8ba5f1126..33e27fe85 100644 --- a/app/pkg/jwt/jwt.go +++ b/app/pkg/jwt/jwt.go @@ -43,6 +43,13 @@ type OAuthClaims struct { Metadata } +// OAuthStateClaims represents what goes into JWT tokens used for OAuth state parameter +type OAuthStateClaims struct { + Redirect string `json:"oauthstate/redirect"` + Identifier string `json:"oauthstate/identifier"` + Metadata +} + // Encode creates new JWT token with given claims func Encode(claims jwtgo.Claims) (string, error) { jwtToken := jwtgo.NewWithClaims(jwtgo.GetSigningMethod("HS256"), claims) @@ -73,6 +80,16 @@ func DecodeOAuthClaims(token string) (*OAuthClaims, error) { return claims, nil } +// DecodeOAuthStateClaims extract OAuthClaims from given JWT token +func DecodeOAuthStateClaims(token string) (*OAuthStateClaims, error) { + claims := &OAuthStateClaims{} + err := decode(token, claims) + if err != nil { + return nil, errors.Wrap(err, "failed to decode OAuthState claims") + } + return claims, nil +} + func decode(token string, claims jwtgo.Claims) error { jwtToken, err := jwtgo.ParseWithClaims(token, claims, func(t *jwtgo.Token) (any, error) { if _, ok := t.Method.(*jwtgo.SigningMethodHMAC); !ok { diff --git a/app/services/oauth/oauth.go b/app/services/oauth/oauth.go index d94ee3eb3..70af7906b 100644 --- a/app/services/oauth/oauth.go +++ b/app/services/oauth/oauth.go @@ -17,6 +17,7 @@ import ( "github.com/getfider/fider/app/pkg/env" "github.com/getfider/fider/app/pkg/errors" "github.com/getfider/fider/app/pkg/jsonq" + "github.com/getfider/fider/app/pkg/jwt" "github.com/getfider/fider/app/pkg/validate" "github.com/getfider/fider/app/pkg/web" "golang.org/x/oauth2" @@ -155,7 +156,17 @@ func getOAuthAuthorizationURL(ctx context.Context, q *query.GetOAuthAuthorizatio parameters.Add("scope", config.Scope) parameters.Add("redirect_uri", fmt.Sprintf("%s/oauth/%s/callback", oauthBaseURL, q.Provider)) parameters.Add("response_type", "code") - parameters.Add("state", q.Redirect+"|"+q.Identifier) + + state, err := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: q.Redirect, + Identifier: q.Identifier, + }) + + if err != nil { + return err + } + + parameters.Add("state", state) authURL.RawQuery = parameters.Encode() q.Result = authURL.String() diff --git a/app/services/oauth/oauth_test.go b/app/services/oauth/oauth_test.go index 56f7c46ee..83f5efcb1 100644 --- a/app/services/oauth/oauth_test.go +++ b/app/services/oauth/oauth_test.go @@ -16,6 +16,7 @@ import ( "github.com/getfider/fider/app/pkg/bus" "github.com/getfider/fider/app/pkg/errors" + "github.com/getfider/fider/app/pkg/jwt" "github.com/getfider/fider/app/pkg/web" . "github.com/getfider/fider/app/pkg/assert" @@ -46,9 +47,14 @@ func TestGetAuthURL_Facebook(t *testing.T) { Identifier: "456", } + expectedState, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://example.org", + Identifier: "456", + }) + err := bus.Dispatch(ctx, authURL) Expect(err).IsNil() - Expect(authURL.Result).Equals("https://www.facebook.com/v3.2/dialog/oauth?client_id=FB_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2Ffacebook%2Fcallback&response_type=code&scope=public_profile+email&state=http%3A%2F%2Fexample.org%7C456") + Expect(authURL.Result).Equals("https://www.facebook.com/v3.2/dialog/oauth?client_id=FB_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2Ffacebook%2Fcallback&response_type=code&scope=public_profile+email&state="+expectedState) } func TestGetAuthURL_Google(t *testing.T) { @@ -63,9 +69,14 @@ func TestGetAuthURL_Google(t *testing.T) { Identifier: "123", } + expectedState, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://example.org", + Identifier: "123", + }) + err := bus.Dispatch(ctx, authURL) Expect(err).IsNil() - Expect(authURL.Result).Equals("https://accounts.google.com/o/oauth2/v2/auth?client_id=GO_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2Fgoogle%2Fcallback&response_type=code&scope=profile+email&state=http%3A%2F%2Fexample.org%7C123") + Expect(authURL.Result).Equals("https://accounts.google.com/o/oauth2/v2/auth?client_id=GO_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2Fgoogle%2Fcallback&response_type=code&scope=profile+email&state="+expectedState) } func TestGetAuthURL_GitHub(t *testing.T) { @@ -80,9 +91,14 @@ func TestGetAuthURL_GitHub(t *testing.T) { Identifier: "456", } + expectedState, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://example.org", + Identifier: "456", + }) + err := bus.Dispatch(ctx, authURL) Expect(err).IsNil() - Expect(authURL.Result).Equals("https://github.com/login/oauth/authorize?client_id=GH_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2Fgithub%2Fcallback&response_type=code&scope=user%3Aemail&state=http%3A%2F%2Fexample.org%7C456") + Expect(authURL.Result).Equals("https://github.com/login/oauth/authorize?client_id=GH_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2Fgithub%2Fcallback&response_type=code&scope=user%3Aemail&state="+expectedState) } func TestGetAuthURL_Custom(t *testing.T) { @@ -108,9 +124,14 @@ func TestGetAuthURL_Custom(t *testing.T) { Identifier: "456", } + expectedState, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://example.org", + Identifier: "456", + }) + err := bus.Dispatch(ctx, authURL) Expect(err).IsNil() - Expect(authURL.Result).Equals("https://example.org/oauth/authorize?client_id=CU_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2F_custom%2Fcallback&response_type=code&scope=profile+email&state=http%3A%2F%2Fexample.org%7C456") + Expect(authURL.Result).Equals("https://example.org/oauth/authorize?client_id=CU_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2F_custom%2Fcallback&response_type=code&scope=profile+email&state="+expectedState) } func TestGetAuthURL_Twitch(t *testing.T) { @@ -136,9 +157,14 @@ func TestGetAuthURL_Twitch(t *testing.T) { Identifier: "456", } + expectedState, _ := jwt.Encode(jwt.OAuthStateClaims{ + Redirect: "http://example.org", + Identifier: "456", + }) + err := bus.Dispatch(ctx, authURL) Expect(err).IsNil() - Expect(authURL.Result).Equals("https://id.twitch.tv/oauth/authorize?claims=%7B%22userinfo%22%3A%7B%22preferred_username%22%3Anull%2C%22email%22%3Anull%2C%22email_verified%22%3Anull%7D%7D&client_id=CU_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2F_custom%2Fcallback&response_type=code&scope=openid&state=http%3A%2F%2Fexample.org%7C456") + Expect(authURL.Result).Equals("https://id.twitch.tv/oauth/authorize?claims=%7B%22userinfo%22%3A%7B%22preferred_username%22%3Anull%2C%22email%22%3Anull%2C%22email_verified%22%3Anull%7D%7D&client_id=CU_CL_ID&redirect_uri=http%3A%2F%2Flogin.test.fider.io%3A3000%2Foauth%2F_custom%2Fcallback&response_type=code&scope=openid&state="+expectedState) } func TestParseProfileResponse_AllFields(t *testing.T) {