Skip to content

Commit

Permalink
fix: require redirect_uri for OpenID Connect calls (#814)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@aeneasr
aeneasr authored Sep 16, 2024
1 parent 8631deb commit aa7c79e
Show file tree
Hide file tree
Showing 8 changed files with 179 additions and 15 deletions.
35 changes: 27 additions & 8 deletions authorize_request_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -363,27 +378,31 @@ 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
}

if len(request.Form.Get("registration")) > 0 {
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
}

Expand Down
12 changes: 12 additions & 0 deletions handler/openid/flow_explicit_auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
14 changes: 14 additions & 0 deletions handler/openid/flow_explicit_auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package openid
import (
"context"
"fmt"
"net/url"
"testing"

"github.com/ory/fosite/internal/gen"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
12 changes: 12 additions & 0 deletions handler/openid/flow_hybrid.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
31 changes: 27 additions & 4 deletions handler/openid/flow_hybrid_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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{
Expand Down
12 changes: 12 additions & 0 deletions handler/openid/flow_implicit.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
21 changes: 19 additions & 2 deletions handler/openid/flow_implicit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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()
Expand Down
57 changes: 56 additions & 1 deletion integration/oidc_explicit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit aa7c79e

Please sign in to comment.