From aa7c79e2d3e80d28611c72e47a9919c73041c530 Mon Sep 17 00:00:00 2001 From: hackerman <3372410+aeneasr@users.noreply.github.com> Date: Mon, 16 Sep 2024 13:17:55 +0200 Subject: [PATCH] fix: require redirect_uri for OpenID Connect calls (#814) 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. --- authorize_request_handler.go | 35 ++++++++++---- 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 | 57 ++++++++++++++++++++++- 8 files changed, 179 insertions(+), 15 deletions(-) diff --git a/authorize_request_handler.go b/authorize_request_handler.go index 89f22a2f..675983fc 100644 --- a/authorize_request_handler.go +++ b/authorize_request_handler.go @@ -165,6 +165,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 { @@ -176,14 +187,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 } @@ -363,15 +378,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 } @@ -379,11 +398,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/handler/openid/flow_explicit_auth.go b/handler/openid/flow_explicit_auth.go index 071327fa..da973f8e 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 2373444b..756f3d82 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 925070a1..c1dc6c64 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 943fa665..afe4bbe3 100644 --- a/handler/openid/flow_hybrid_test.go +++ b/handler/openid/flow_hybrid_test.go @@ -90,6 +90,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 @@ -113,7 +114,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"}, @@ -128,7 +132,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"}, @@ -143,7 +150,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"}, @@ -178,7 +188,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"}, @@ -255,12 +269,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 e71113be..caf9fa9b 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 baf282c9..a833e51b 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 a2df1e1e..5fa06e52 100644 --- a/integration/oidc_explicit_test.go +++ b/integration/oidc_explicit_test.go @@ -56,6 +56,30 @@ 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 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", @@ -66,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", @@ -89,6 +128,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", @@ -122,7 +176,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)