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 3412675a..61270c77 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 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)