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

Jump to bottom

fix: openid ignores missing redirect uri #762

Closed
wants to merge 5 commits into from
+179 −15
Closed

fix: openid ignores missing redirect uri #762

Show file tree
Hide file tree
Changes from 1 commit
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 @@ -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") {
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 @@ -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
}
Expand Down Expand Up @@ -358,27 +373,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
15 changes: 14 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 @@ -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",
Expand Down Expand Up @@ -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)
Expand Down