Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: openid ignores missing redirect uri #762

Closed
wants to merge 5 commits into from
Closed

fix: openid ignores missing redirect uri #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4273501
fix: openid ignores missing redirect uri
james-d-elliott Aug 4, 2023
63c797c
test: add tdd test
james-d-elliott Aug 30, 2023
37f4ada
Merge branch 'master' into fix-oidc-redirect-uri
james-d-elliott Dec 2, 2023
14f50b3
Merge branch 'master' into fix-oidc-redirect-uri
james-d-elliott Feb 13, 2024
86c8ea5
fix: require `redirect_uri` for OpenID Connect calls
aeneasr Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions authorize_request_handler.go
View file
Open in desktop
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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not already handled by the openid handler itself?

Copy link
Contributor Author

@james-d-elliott james-d-elliott Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added an integration test which shows this is unfortunately not the case. It only returns an error if the client has multiple redirect URI's. The issue is that OAuth 2.0 allows assuming the Redirect URI if absent provided there is only one, however OpenID Connect 1.0 explicitly requires the redirect URI parameter and has no such leeway.

It should also be noted that OAuth 2.1 does not alter this behavior and instead elaborates / clarifies the behavior for better or worse. The only alteration by my reading is in OpenID Connect 1.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see the linked issue, you were actually the person to raise the issue. Maybe that's helpful?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this and I agree with @james-d-elliott: OAuth 2.0 says that if you included Redirect URI in authentication request, then it has also be included in the token request. And Fosite currently checks that.

But OIDC spec says that Redirect URI is required in the authentication request. And this PR adds this check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both - I checked and indeed the expectation is that the redirect-url is properly validated before we call HandleAuthorizeEndpointRequest. Given that this is openID specific though, this should be moved to the openid handler instead. I'll do that now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm creating a separate PR as this is a breaking change for existing clients and it requires dedicated care for merging that to production on our infrastructure!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this implementation does not respect hybrid or implicit flows of OIDC, so it definitely makes sense to move that to the appropriate handlers in my view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a solid plan, glad this will get merged regardless.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that instead of splitting validateAuthorizeScope into parseAuthorizeScope and validateAuthorizeScope, it would be easier if you swapped the order of validation, first call (original) validateAuthorizeScope and then (updated) validateAuthorizeRedirectURI. In a way, if validateAuthorizeRedirectURI depends on parsed scopes, it is better if validateAuthorizeRedirectURI depends on parsed and validated scopes.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why these changes? I think they are unrelated and just change the style?

return request, err
}

Expand Down
12 changes: 12 additions & 0 deletions handler/openid/flow_explicit_auth.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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{
Expand Down
12 changes: 12 additions & 0 deletions handler/openid/flow_implicit.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading