From 42735017227e07a442bc3efa5d24c67c76affc68 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Fri, 4 Aug 2023 15:15:42 +1000 Subject: [PATCH 1/3] fix: openid ignores missing redirect uri Fixes an issue where Authorize Requests which were intended for an OpenID Connect 1.0 client would incorrectly be allowed when missing the redirect URI when it's required by the specification. --- authorize_request_handler.go | 35 ++++++++++++++++++++++++------- integration/oidc_explicit_test.go | 15 ++++++++++++- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/authorize_request_handler.go b/authorize_request_handler.go index 42d24ad89..f2599a103 100644 --- a/authorize_request_handler.go +++ b/authorize_request_handler.go @@ -163,6 +163,17 @@ func (f *Fosite) validateAuthorizeRedirectURI(_ *http.Request, request *Authoriz // Fetch redirect URI from request rawRedirURI := request.Form.Get("redirect_uri") + // This ensures that the 'redirect_uri' parameter is present for OpenID Connect 1.0 authorization requests as per: + // + // Authorization Code Flow - https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // Implicit Flow - https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest + // Hybrid Flow - https://openid.net/specs/openid-connect-core-1_0.html#HybridAuthRequest + // + // Note: as per the Hybrid Flow documentation the Hybrid Flow has the same requirements as the Authorization Code Flow. + if len(rawRedirURI) == 0 && request.GetRequestedScopes().Has("openid") { + return errorsx.WithStack(ErrInvalidRequest.WithHint("The 'redirect_uri' parameter is required when using OpenID Connect 1.0.")) + } + // Validate redirect uri redirectURI, err := MatchRedirectURIWithClientRedirectURIs(rawRedirURI, request.Client) if err != nil { @@ -174,14 +185,18 @@ func (f *Fosite) validateAuthorizeRedirectURI(_ *http.Request, request *Authoriz return nil } +func (f *Fosite) parseAuthorizeScope(_ *http.Request, request *AuthorizeRequest) error { + request.SetRequestedScopes(RemoveEmpty(strings.Split(request.Form.Get("scope"), " "))) + + return nil +} + func (f *Fosite) validateAuthorizeScope(ctx context.Context, _ *http.Request, request *AuthorizeRequest) error { - scope := RemoveEmpty(strings.Split(request.Form.Get("scope"), " ")) - for _, permission := range scope { + for _, permission := range request.GetRequestedScopes() { if !f.Config.GetScopeStrategy(ctx)(request.Client.GetScopes(), permission) { return errorsx.WithStack(ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", permission)) } } - request.SetRequestedScopes(scope) return nil } @@ -358,15 +373,19 @@ func (f *Fosite) newAuthorizeRequest(ctx context.Context, r *http.Request, isPAR return request, err } - if err := f.validateAuthorizeRedirectURI(r, request); err != nil { + if err = f.parseAuthorizeScope(r, request); err != nil { + return request, err + } + + if err = f.validateAuthorizeRedirectURI(r, request); err != nil { return request, err } - if err := f.validateAuthorizeScope(ctx, r, request); err != nil { + if err = f.validateAuthorizeScope(ctx, r, request); err != nil { return request, err } - if err := f.validateAuthorizeAudience(ctx, r, request); err != nil { + if err = f.validateAuthorizeAudience(ctx, r, request); err != nil { return request, err } @@ -374,11 +393,11 @@ func (f *Fosite) newAuthorizeRequest(ctx context.Context, r *http.Request, isPAR return request, errorsx.WithStack(ErrRegistrationNotSupported) } - if err := f.validateResponseTypes(r, request); err != nil { + if err = f.validateResponseTypes(r, request); err != nil { return request, err } - if err := f.validateResponseMode(r, request); err != nil { + if err = f.validateResponseMode(r, request); err != nil { return request, err } diff --git a/integration/oidc_explicit_test.go b/integration/oidc_explicit_test.go index b9b0ee27e..78ef73b9d 100644 --- a/integration/oidc_explicit_test.go +++ b/integration/oidc_explicit_test.go @@ -55,6 +55,18 @@ func TestOpenIDConnectExplicitFlow(t *testing.T) { }, authStatusCode: http.StatusOK, }, + { + session: newIDSession(&jwt.IDTokenClaims{Subject: "peter"}), + description: "should fail registered single redirect uri but no redirect uri in request", + setup: func(oauthClient *oauth2.Config) string { + oauthClient.Scopes = []string{"openid"} + oauthClient.RedirectURL = "" + + return oauthClient.AuthCodeURL("12345678901234567890") + "&nonce=11234123" + }, + authStatusCode: http.StatusBadRequest, + expectAuthErr: `{"error":"invalid_request","error_description":"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. The 'redirect_uri' parameter is required when using OpenID Connect 1.0."}`, + }, { session: newIDSession(&jwt.IDTokenClaims{Subject: "peter"}), description: "should fail because nonce is not long enough", @@ -121,7 +133,8 @@ func TestOpenIDConnectExplicitFlow(t *testing.T) { defer ts.Close() oauthClient := newOAuth2Client(ts) - fositeStore.Clients["my-client"].(*fosite.DefaultClient).RedirectURIs[0] = ts.URL + "/callback" + + fositeStore.Clients["my-client"].(*fosite.DefaultClient).RedirectURIs = []string{ts.URL + "/callback"} resp, err := http.Get(c.setup(oauthClient)) require.NoError(t, err) From 63c797c662dd07ffdf8210d05e229a420d957bdc Mon Sep 17 00:00:00 2001 From: James Elliott Date: Wed, 30 Aug 2023 20:52:41 +1000 Subject: [PATCH 2/3] test: add tdd test --- integration/oidc_explicit_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/integration/oidc_explicit_test.go b/integration/oidc_explicit_test.go index 78ef73b9d..2992e6767 100644 --- a/integration/oidc_explicit_test.go +++ b/integration/oidc_explicit_test.go @@ -100,6 +100,21 @@ func TestOpenIDConnectExplicitFlow(t *testing.T) { }, authStatusCode: http.StatusOK, }, + { + session: newIDSession(&jwt.IDTokenClaims{ + Subject: "peter", + RequestedAt: time.Now().UTC(), + AuthTime: time.Now().Add(time.Second).UTC(), + }), + description: "should not pass missing redirect uri", + setup: func(oauthClient *oauth2.Config) string { + oauthClient.RedirectURL = "" + oauthClient.Scopes = []string{"openid"} + return oauthClient.AuthCodeURL("12345678901234567890") + "&nonce=1234567890&prompt=login" + }, + expectAuthErr: `{"error":"invalid_request","error_description":"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. The 'redirect_uri' parameter is required when using OpenID Connect 1.0."}`, + authStatusCode: http.StatusBadRequest, + }, { session: newIDSession(&jwt.IDTokenClaims{ Subject: "peter", From 86c8ea516dc35d229a0fb6bb784091aa1d7cf5c9 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Tue, 9 Jul 2024 12:27:02 +0200 Subject: [PATCH 3/3] fix: require `redirect_uri` for OpenID Connect calls Fixes an issue where Authorize Requests which were intended for an OpenID Connect 1.0 client would incorrectly be allowed when missing the redirect URI when it's required by the specification. Closes #685 Closes #762 BREAKING CHANGES: Going forward, calls to `/oauth2/auth` which trigger OpenID Connect require the `redirect_uri` query parameter to be set. --- handler/openid/flow_explicit_auth.go | 12 +++++++++ handler/openid/flow_explicit_auth_test.go | 14 ++++++++++ handler/openid/flow_hybrid.go | 12 +++++++++ handler/openid/flow_hybrid_test.go | 31 ++++++++++++++++++++--- handler/openid/flow_implicit.go | 12 +++++++++ handler/openid/flow_implicit_test.go | 21 +++++++++++++-- integration/oidc_explicit_test.go | 27 ++++++++++++++++++++ 7 files changed, 123 insertions(+), 6 deletions(-) diff --git a/handler/openid/flow_explicit_auth.go b/handler/openid/flow_explicit_auth.go index 071327faf..da973f8e0 100644 --- a/handler/openid/flow_explicit_auth.go +++ b/handler/openid/flow_explicit_auth.go @@ -47,6 +47,18 @@ func (c *OpenIDConnectExplicitHandler) HandleAuthorizeEndpointRequest(ctx contex return errorsx.WithStack(fosite.ErrMisconfiguration.WithDebug("The authorization code has not been issued yet, indicating a broken code configuration.")) } + // This ensures that the 'redirect_uri' parameter is present for OpenID Connect 1.0 authorization requests as per: + // + // Authorization Code Flow - https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // Implicit Flow - https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest + // Hybrid Flow - https://openid.net/specs/openid-connect-core-1_0.html#HybridAuthRequest + // + // Note: as per the Hybrid Flow documentation the Hybrid Flow has the same requirements as the Authorization Code Flow. + rawRedirectURI := ar.GetRequestForm().Get("redirect_uri") + if len(rawRedirectURI) == 0 { + return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("The 'redirect_uri' parameter is required when using OpenID Connect 1.0.")) + } + if err := c.OpenIDConnectRequestValidator.ValidatePrompt(ctx, ar); err != nil { return err } diff --git a/handler/openid/flow_explicit_auth_test.go b/handler/openid/flow_explicit_auth_test.go index 2373444b7..756f3d82f 100644 --- a/handler/openid/flow_explicit_auth_test.go +++ b/handler/openid/flow_explicit_auth_test.go @@ -6,6 +6,7 @@ package openid import ( "context" "fmt" + "net/url" "testing" "github.com/ory/fosite/internal/gen" @@ -55,6 +56,9 @@ func TestExplicit_HandleAuthorizeEndpointRequest(t *testing.T) { session := NewDefaultSession() session.Claims.Subject = "foo" areq.Session = session + areq.Form = url.Values{ + "redirect_uri": {"https://foobar.com"}, + } for k, c := range []struct { description string @@ -110,6 +114,16 @@ func TestExplicit_HandleAuthorizeEndpointRequest(t *testing.T) { return h }, }, + { + description: "should fail because redirect url is missing", + setup: func() OpenIDConnectExplicitHandler { + areq.Form.Del("redirect_uri") + h, store := makeOpenIDConnectExplicitHandler(ctrl, fosite.MinParameterEntropy) + store.EXPECT().CreateOpenIDConnectSession(gomock.Any(), "codeexample", gomock.Eq(areq.Sanitize(oidcParameters))).AnyTimes().Return(nil) + return h + }, + expectErr: fosite.ErrInvalidRequest, + }, } { t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { h := c.setup() diff --git a/handler/openid/flow_hybrid.go b/handler/openid/flow_hybrid.go index 925070a18..c1dc6c645 100644 --- a/handler/openid/flow_hybrid.go +++ b/handler/openid/flow_hybrid.go @@ -63,6 +63,18 @@ func (c *OpenIDConnectHybridHandler) HandleAuthorizeEndpointRequest(ctx context. return errorsx.WithStack(fosite.ErrInsufficientEntropy.WithHintf("Parameter 'nonce' is set but does not satisfy the minimum entropy of %d characters.", c.Config.GetMinParameterEntropy(ctx))) } + // This ensures that the 'redirect_uri' parameter is present for OpenID Connect 1.0 authorization requests as per: + // + // Authorization Code Flow - https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // Implicit Flow - https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest + // Hybrid Flow - https://openid.net/specs/openid-connect-core-1_0.html#HybridAuthRequest + // + // Note: as per the Hybrid Flow documentation the Hybrid Flow has the same requirements as the Authorization Code Flow. + rawRedirectURI := ar.GetRequestForm().Get("redirect_uri") + if len(rawRedirectURI) == 0 { + return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("The 'redirect_uri' parameter is required when using OpenID Connect 1.0.")) + } + sess, ok := ar.GetSession().(Session) if !ok { return errorsx.WithStack(ErrInvalidSession) diff --git a/handler/openid/flow_hybrid_test.go b/handler/openid/flow_hybrid_test.go index 3412675aa..61270c77c 100644 --- a/handler/openid/flow_hybrid_test.go +++ b/handler/openid/flow_hybrid_test.go @@ -93,6 +93,7 @@ func TestHybrid_HandleAuthorizeEndpointRequest(t *testing.T) { aresp := fosite.NewAuthorizeResponse() areq := fosite.NewAuthorizeRequest() + areq.Form = url.Values{"redirect_uri": {"https://foobar.com"}} for k, c := range []struct { description string @@ -116,7 +117,10 @@ func TestHybrid_HandleAuthorizeEndpointRequest(t *testing.T) { { description: "should fail because nonce set but too short", setup: func() OpenIDConnectHybridHandler { - areq.Form = url.Values{"nonce": {"short"}} + areq.Form = url.Values{ + "redirect_uri": {"https://foobar.com"}, + "nonce": {"short"}, + } areq.ResponseTypes = fosite.Arguments{"token", "code"} areq.Client = &fosite.DefaultClient{ GrantTypes: fosite.Arguments{"authorization_code", "implicit"}, @@ -131,7 +135,10 @@ func TestHybrid_HandleAuthorizeEndpointRequest(t *testing.T) { { description: "should fail because nonce set but too short for non-default min entropy", setup: func() OpenIDConnectHybridHandler { - areq.Form = url.Values{"nonce": {"some-foobar-nonce-win"}} + areq.Form = url.Values{ + "nonce": {"some-foobar-nonce-win"}, + "redirect_uri": {"https://foobar.com"}, + } areq.ResponseTypes = fosite.Arguments{"token", "code"} areq.Client = &fosite.DefaultClient{ GrantTypes: fosite.Arguments{"authorization_code", "implicit"}, @@ -146,7 +153,10 @@ func TestHybrid_HandleAuthorizeEndpointRequest(t *testing.T) { { description: "should fail because session not given", setup: func() OpenIDConnectHybridHandler { - areq.Form = url.Values{"nonce": {"long-enough"}} + areq.Form = url.Values{ + "nonce": {"long-enough"}, + "redirect_uri": {"https://foobar.com"}, + } areq.ResponseTypes = fosite.Arguments{"token", "code"} areq.Client = &fosite.DefaultClient{ GrantTypes: fosite.Arguments{"authorization_code", "implicit"}, @@ -181,7 +191,11 @@ func TestHybrid_HandleAuthorizeEndpointRequest(t *testing.T) { { description: "should pass with exact one state parameter in response", setup: func() OpenIDConnectHybridHandler { - areq.Form = url.Values{"nonce": {"long-enough"}, "state": {""}} + areq.Form = url.Values{ + "redirect_uri": {"https://foobar.com"}, + "nonce": {"long-enough"}, + "state": {""}, + } areq.Client = &fosite.DefaultClient{ GrantTypes: fosite.Arguments{"authorization_code", "implicit"}, ResponseTypes: fosite.Arguments{"token", "code", "id_token"}, @@ -258,12 +272,21 @@ func TestHybrid_HandleAuthorizeEndpointRequest(t *testing.T) { internal.RequireEqualTime(t, time.Now().Add(time.Hour).UTC(), areq.GetSession().GetExpiresAt(fosite.AuthorizeCode), time.Second) }, }, + { + description: "should fail if redirect_uri is missing", + setup: func() OpenIDConnectHybridHandler { + areq.Form.Del("redirect_uri") + return makeOpenIDConnectHybridHandler(fosite.MinParameterEntropy) + }, + expectErr: fosite.ErrInvalidRequest, + }, { description: "should pass with custom client lifespans", setup: func() OpenIDConnectHybridHandler { aresp = fosite.NewAuthorizeResponse() areq = fosite.NewAuthorizeRequest() areq.Form.Set("nonce", "some-foobar-nonce-win") + areq.Form.Set("redirect_uri", "https://foobar.com") areq.ResponseTypes = fosite.Arguments{"token", "code", "id_token"} areq.Client = &fosite.DefaultClientWithCustomTokenLifespans{ DefaultClient: &fosite.DefaultClient{ diff --git a/handler/openid/flow_implicit.go b/handler/openid/flow_implicit.go index e71113be0..caf9fa9b0 100644 --- a/handler/openid/flow_implicit.go +++ b/handler/openid/flow_implicit.go @@ -48,6 +48,18 @@ func (c *OpenIDConnectImplicitHandler) HandleAuthorizeEndpointRequest(ctx contex // return errorsx.WithStack(fosite.ErrInvalidGrant.WithDebug("The client is not allowed to use response type token and id_token")) //} + // This ensures that the 'redirect_uri' parameter is present for OpenID Connect 1.0 authorization requests as per: + // + // Authorization Code Flow - https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // Implicit Flow - https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest + // Hybrid Flow - https://openid.net/specs/openid-connect-core-1_0.html#HybridAuthRequest + // + // Note: as per the Hybrid Flow documentation the Hybrid Flow has the same requirements as the Authorization Code Flow. + rawRedirectURI := ar.GetRequestForm().Get("redirect_uri") + if len(rawRedirectURI) == 0 { + return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("The 'redirect_uri' parameter is required when using OpenID Connect 1.0.")) + } + if nonce := ar.GetRequestForm().Get("nonce"); len(nonce) == 0 { return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("Parameter 'nonce' must be set when using the OpenID Connect Implicit Flow.")) } else if len(nonce) < c.Config.GetMinParameterEntropy(ctx) { diff --git a/handler/openid/flow_implicit_test.go b/handler/openid/flow_implicit_test.go index baf282c96..a833e51b1 100644 --- a/handler/openid/flow_implicit_test.go +++ b/handler/openid/flow_implicit_test.go @@ -67,6 +67,9 @@ func TestImplicit_HandleAuthorizeEndpointRequest(t *testing.T) { aresp := fosite.NewAuthorizeResponse() areq := fosite.NewAuthorizeRequest() + areq.Form = url.Values{ + "redirect_uri": {"https://foobar.com"}, + } areq.Session = new(fosite.DefaultSession) for k, c := range []struct { @@ -150,7 +153,10 @@ func TestImplicit_HandleAuthorizeEndpointRequest(t *testing.T) { { description: "should not do anything because request requirements are not met", setup: func() OpenIDConnectImplicitHandler { - areq.Form = url.Values{"nonce": {"short"}} + areq.Form = url.Values{ + "nonce": {"short"}, + "redirect_uri": {"https://foobar.com"}, + } areq.ResponseTypes = fosite.Arguments{"id_token"} areq.RequestedScope = fosite.Arguments{"openid"} areq.Client = &fosite.DefaultClient{ @@ -165,7 +171,10 @@ func TestImplicit_HandleAuthorizeEndpointRequest(t *testing.T) { { description: "should fail because session not set", setup: func() OpenIDConnectImplicitHandler { - areq.Form = url.Values{"nonce": {"long-enough"}} + areq.Form = url.Values{ + "nonce": {"long-enough"}, + "redirect_uri": {"https://foobar.com"}, + } areq.ResponseTypes = fosite.Arguments{"id_token"} areq.RequestedScope = fosite.Arguments{"openid"} areq.Client = &fosite.DefaultClient{ @@ -282,6 +291,14 @@ func TestImplicit_HandleAuthorizeEndpointRequest(t *testing.T) { assert.NotEmpty(t, aresp.GetParameters().Get("access_token")) }, }, + { + description: "should fail without redirect_uri", + setup: func() OpenIDConnectImplicitHandler { + areq.Form.Del("redirect_uri") + return makeOpenIDConnectImplicitHandler(4) + }, + expectErr: fosite.ErrInvalidRequest, + }, } { t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { h := c.setup() diff --git a/integration/oidc_explicit_test.go b/integration/oidc_explicit_test.go index 13806bf29..5fa06e526 100644 --- a/integration/oidc_explicit_test.go +++ b/integration/oidc_explicit_test.go @@ -68,6 +68,18 @@ func TestOpenIDConnectExplicitFlow(t *testing.T) { authStatusCode: http.StatusBadRequest, expectAuthErr: `{"error":"invalid_request","error_description":"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. The 'redirect_uri' parameter is required when using OpenID Connect 1.0."}`, }, + { + session: newIDSession(&jwt.IDTokenClaims{Subject: "peter"}), + description: "should fail registered single redirect uri but no redirect uri in request", + setup: func(oauthClient *oauth2.Config) string { + oauthClient.Scopes = []string{"openid"} + oauthClient.RedirectURL = "" + + return oauthClient.AuthCodeURL("12345678901234567890") + "&nonce=11234123" + }, + authStatusCode: http.StatusBadRequest, + expectAuthErr: `{"error":"invalid_request","error_description":"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. The 'redirect_uri' parameter is required when using OpenID Connect 1.0."}`, + }, { session: newIDSession(&jwt.IDTokenClaims{Subject: "peter"}), description: "should fail because nonce is not long enough", @@ -78,6 +90,21 @@ func TestOpenIDConnectExplicitFlow(t *testing.T) { authStatusCode: http.StatusOK, expectTokenErr: "insufficient_entropy", }, + { + session: newIDSession(&jwt.IDTokenClaims{ + Subject: "peter", + RequestedAt: time.Now().UTC(), + AuthTime: time.Now().Add(time.Second).UTC(), + }), + description: "should not pass missing redirect uri", + setup: func(oauthClient *oauth2.Config) string { + oauthClient.RedirectURL = "" + oauthClient.Scopes = []string{"openid"} + return oauthClient.AuthCodeURL("12345678901234567890") + "&nonce=1234567890&prompt=login" + }, + expectAuthErr: `{"error":"invalid_request","error_description":"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. The 'redirect_uri' parameter is required when using OpenID Connect 1.0."}`, + authStatusCode: http.StatusBadRequest, + }, { session: newIDSession(&jwt.IDTokenClaims{Subject: "peter"}), description: "should fail because state is not long enough",