From e66758ed3cc426041f97523d59aaca98382f7e73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Tue, 20 Aug 2024 18:22:22 +0300 Subject: [PATCH 1/2] fix(op): do not redirect to unverified uri on error Backport of https://github.com/zitadel/oidc/pull/640 Related to https://github.com/zitadel/oidc/issues/627 --- pkg/op/auth_request.go | 49 ++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/pkg/op/auth_request.go b/pkg/op/auth_request.go index 7d9f264a..a27d86be 100644 --- a/pkg/op/auth_request.go +++ b/pkg/op/auth_request.go @@ -75,19 +75,27 @@ func Authorize(w http.ResponseWriter, r *http.Request, authorizer Authorizer) { if authReq.RequestParam != "" && authorizer.RequestObjectSupported() { authReq, err = ParseRequestObject(ctx, authReq, authorizer.Storage(), IssuerFromContext(ctx)) if err != nil { - AuthRequestError(w, r, authReq, err, authorizer.Encoder()) + AuthRequestError(w, r, nil, err, authorizer.Encoder()) return } } if authReq.ClientID == "" { - AuthRequestError(w, r, authReq, fmt.Errorf("auth request is missing client_id"), authorizer.Encoder()) + AuthRequestError(w, r, nil, fmt.Errorf("auth request is missing client_id"), authorizer.Encoder()) return } if authReq.RedirectURI == "" { - AuthRequestError(w, r, authReq, fmt.Errorf("auth request is missing redirect_uri"), authorizer.Encoder()) + AuthRequestError(w, r, nil, fmt.Errorf("auth request is missing redirect_uri"), authorizer.Encoder()) return } - validation := ValidateAuthRequest + + var client Client + validation := func(ctx context.Context, authReq *oidc.AuthRequest, storage Storage, verifier IDTokenHintVerifier) (sub string, err error) { + client, err = authorizer.Storage().GetClientByClientID(ctx, authReq.ClientID) + if err != nil { + return "", oidc.ErrInvalidRequestRedirectURI().WithDescription("unable to retrieve client by id").WithParent(err) + } + return ValidateAuthRequestClient(ctx, authReq, client, verifier) + } if validater, ok := authorizer.(AuthorizeValidator); ok { validation = validater.ValidateAuthRequest } @@ -105,11 +113,6 @@ func Authorize(w http.ResponseWriter, r *http.Request, authorizer Authorizer) { AuthRequestError(w, r, authReq, oidc.DefaultToServerError(err, "unable to save auth request"), authorizer.Encoder()) return } - client, err := authorizer.Storage().GetClientByClientID(ctx, req.GetClientID()) - if err != nil { - AuthRequestError(w, r, req, oidc.DefaultToServerError(err, "unable to retrieve client by id"), authorizer.Encoder()) - return - } RedirectToLogin(req.GetID(), client, w, r) } @@ -204,23 +207,37 @@ func CopyRequestObjectToAuthRequest(authReq *oidc.AuthRequest, requestObject *oi authReq.RequestParam = "" } -// ValidateAuthRequest validates the authorize parameters and returns the userID of the id_token_hint if passed +// ValidateAuthRequest validates the authorize parameters and returns the userID of the id_token_hint if passed. +// +// Deprecated: Use [ValidateAuthRequestClient] to prevent querying for the Client twice. func ValidateAuthRequest(ctx context.Context, authReq *oidc.AuthRequest, storage Storage, verifier IDTokenHintVerifier) (sub string, err error) { - authReq.MaxAge, err = ValidateAuthReqPrompt(authReq.Prompt, authReq.MaxAge) + ctx, span := tracer.Start(ctx, "ValidateAuthRequest") + defer span.End() + + client, err := storage.GetClientByClientID(ctx, authReq.ClientID) if err != nil { + return "", oidc.ErrInvalidRequestRedirectURI().WithDescription("unable to retrieve client by id").WithParent(err) + } + return ValidateAuthRequestClient(ctx, authReq, client, verifier) +} + +// ValidateAuthRequestClient validates the Auth request against the passed client. +// If id_token_hint is part of the request, the subject of the token is returned. +func ValidateAuthRequestClient(ctx context.Context, authReq *oidc.AuthRequest, client Client, verifier IDTokenHintVerifier) (sub string, err error) { + ctx, span := tracer.Start(ctx, "ValidateAuthRequestClient") + defer span.End() + + if err := ValidateAuthReqRedirectURI(client, authReq.RedirectURI, authReq.ResponseType); err != nil { return "", err } - client, err := storage.GetClientByClientID(ctx, authReq.ClientID) + authReq.MaxAge, err = ValidateAuthReqPrompt(authReq.Prompt, authReq.MaxAge) if err != nil { - return "", oidc.DefaultToServerError(err, "unable to retrieve client by id") + return "", err } authReq.Scopes, err = ValidateAuthReqScopes(client, authReq.Scopes) if err != nil { return "", err } - if err := ValidateAuthReqRedirectURI(client, authReq.RedirectURI, authReq.ResponseType); err != nil { - return "", err - } if err := ValidateAuthReqResponseType(client, authReq.ResponseType); err != nil { return "", err } From b5e7321cabea06de2180851778c2fbe1d0cfc2ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Tue, 20 Aug 2024 18:54:47 +0300 Subject: [PATCH 2/2] adjust tests --- pkg/op/auth_request_test.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/pkg/op/auth_request_test.go b/pkg/op/auth_request_test.go index e8c9085e..299e5b72 100644 --- a/pkg/op/auth_request_test.go +++ b/pkg/op/auth_request_test.go @@ -132,27 +132,22 @@ func TestValidateAuthRequest(t *testing.T) { }{ { "scope missing fails", - args{&oidc.AuthRequest{}, mock.NewMockStorageExpectValidClientID(t), nil}, + args{&oidc.AuthRequest{ClientID: "client_id", RedirectURI: "https://registered.com/callback"}, mock.NewMockStorageExpectValidClientID(t), nil}, oidc.ErrInvalidRequest(), }, { "scope openid missing fails", - args{&oidc.AuthRequest{Scopes: []string{"profile"}}, mock.NewMockStorageExpectValidClientID(t), nil}, + args{&oidc.AuthRequest{ClientID: "client_id", RedirectURI: "https://registered.com/callback", Scopes: []string{"profile"}}, mock.NewMockStorageExpectValidClientID(t), nil}, oidc.ErrInvalidScope(), }, { "response_type missing fails", - args{&oidc.AuthRequest{Scopes: []string{"openid"}}, mock.NewMockStorageExpectValidClientID(t), nil}, - oidc.ErrInvalidRequest(), - }, - { - "client_id missing fails", - args{&oidc.AuthRequest{Scopes: []string{"openid"}, ResponseType: oidc.ResponseTypeCode}, mock.NewMockStorageExpectValidClientID(t), nil}, + args{&oidc.AuthRequest{ClientID: "client_id", RedirectURI: "https://registered.com/callback", Scopes: []string{"openid"}}, mock.NewMockStorageExpectValidClientID(t), nil}, oidc.ErrInvalidRequest(), }, { "redirect_uri missing fails", - args{&oidc.AuthRequest{Scopes: []string{"openid"}, ResponseType: oidc.ResponseTypeCode, ClientID: "client_id"}, mock.NewMockStorageExpectValidClientID(t), nil}, + args{&oidc.AuthRequest{ClientID: "client_id", Scopes: []string{"openid"}, ResponseType: oidc.ResponseTypeCode}, mock.NewMockStorageExpectValidClientID(t), nil}, oidc.ErrInvalidRequest(), }, }