diff --git a/handler/pkce/handler.go b/handler/pkce/handler.go index f457b8bea..91d0305bd 100644 --- a/handler/pkce/handler.go +++ b/handler/pkce/handler.go @@ -47,6 +47,11 @@ func (c *Handler) HandleAuthorizeEndpointRequest(ctx context.Context, ar fosite. return err } + // We don't need a session if it's not enforced and the PKCE parameters are not provided by the client. + if challenge == "" && method == "" { + return nil + } + code := resp.GetCode() if len(code) == 0 { return errorsx.WithStack(fosite.ErrServerError.WithDebug("The PKCE handler must be loaded after the authorize code handler.")) @@ -64,24 +69,14 @@ func (c *Handler) HandleAuthorizeEndpointRequest(ctx context.Context, ar fosite. } func (c *Handler) validate(ctx context.Context, challenge, method string, client fosite.Client) error { - if challenge == "" { + if len(challenge) == 0 { // If the server requires Proof Key for Code Exchange (PKCE) by OAuth // clients and the client does not send the "code_challenge" in // the request, the authorization endpoint MUST return the authorization // error response with the "error" value set to "invalid_request". The // "error_description" or the response of "error_uri" SHOULD explain the // nature of error, e.g., code challenge required. - if c.Config.GetEnforcePKCE(ctx) { - return errorsx.WithStack(fosite.ErrInvalidRequest. - WithHint("Clients must include a code_challenge when performing the authorize code flow, but it is missing."). - WithDebug("The server is configured in a way that enforces PKCE for clients.")) - } - if c.Config.GetEnforcePKCEForPublicClients(ctx) && client.IsPublic() { - return errorsx.WithStack(fosite.ErrInvalidRequest. - WithHint("This client must include a code_challenge when performing the authorize code flow, but it is missing."). - WithDebug("The server is configured in a way that enforces PKCE for this client.")) - } - return nil + return c.validateNoPKCE(ctx, client) } // If the server supporting PKCE does not support the requested @@ -108,6 +103,20 @@ func (c *Handler) validate(ctx context.Context, challenge, method string, client return nil } +func (c *Handler) validateNoPKCE(ctx context.Context, client fosite.Client) error { + if c.Config.GetEnforcePKCE(ctx) { + return errorsx.WithStack(fosite.ErrInvalidRequest. + WithHint("Clients must include a code_challenge when performing the authorize code flow, but it is missing."). + WithDebug("The server is configured in a way that enforces PKCE for clients.")) + } + if c.Config.GetEnforcePKCEForPublicClients(ctx) && client.IsPublic() { + return errorsx.WithStack(fosite.ErrInvalidRequest. + WithHint("This client must include a code_challenge when performing the authorize code flow, but it is missing."). + WithDebug("The server is configured in a way that enforces PKCE for this client.")) + } + return nil +} + func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite.AccessRequester) error { if !c.CanHandleTokenEndpointRequest(ctx, request) { return errorsx.WithStack(fosite.ErrUnknownRequest) @@ -123,8 +132,15 @@ func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite code := request.GetRequestForm().Get("code") signature := c.AuthorizeCodeStrategy.AuthorizeCodeSignature(ctx, code) - authorizeRequest, err := c.Storage.GetPKCERequestSession(ctx, signature, request.GetSession()) + pkceRequest, err := c.Storage.GetPKCERequestSession(ctx, signature, request.GetSession()) + + nv := len(verifier) + if errors.Is(err, fosite.ErrNotFound) { + if nv == 0 { + return c.validateNoPKCE(ctx, request.GetClient()) + } + return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint("Unable to find initial PKCE data tied to this request").WithWrap(err).WithDebug(err.Error())) } else if err != nil { return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error())) @@ -134,14 +150,16 @@ func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error())) } - challenge := authorizeRequest.GetRequestForm().Get("code_challenge") - method := authorizeRequest.GetRequestForm().Get("code_challenge_method") - client := authorizeRequest.GetClient() + challenge := pkceRequest.GetRequestForm().Get("code_challenge") + method := pkceRequest.GetRequestForm().Get("code_challenge_method") + client := pkceRequest.GetClient() if err := c.validate(ctx, challenge, method, client); err != nil { return err } - if !c.Config.GetEnforcePKCE(ctx) && challenge == "" && verifier == "" { + nc := len(challenge) + + if !c.Config.GetEnforcePKCE(ctx) && nc == 0 && nv == 0 { return nil } @@ -152,15 +170,18 @@ func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite // 43-octet URL safe string to use as the code verifier. // Validation - if len(verifier) < 43 { + if nv < 43 { return errorsx.WithStack(fosite.ErrInvalidGrant. WithHint("The PKCE code verifier must be at least 43 characters.")) - } else if len(verifier) > 128 { + } else if nv > 128 { return errorsx.WithStack(fosite.ErrInvalidGrant. WithHint("The PKCE code verifier can not be longer than 128 characters.")) } else if verifierWrongFormat.MatchString(verifier) { return errorsx.WithStack(fosite.ErrInvalidGrant. WithHint("The PKCE code verifier must only contain [a-Z], [0-9], '-', '.', '_', '~'.")) + } else if nc == 0 { + return errorsx.WithStack(fosite.ErrInvalidGrant. + WithHint("The PKCE code verifier was provided but the code challenge was absent from the authorization request.")) } // Upon receipt of the request at the token endpoint, the server diff --git a/handler/pkce/handler_test.go b/handler/pkce/handler_test.go index 5058cc7c2..f087cad89 100644 --- a/handler/pkce/handler_test.go +++ b/handler/pkce/handler_test.go @@ -10,6 +10,7 @@ import ( "fmt" "testing" + "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -93,21 +94,23 @@ func TestPKCEHandlerValidate(t *testing.T) { s256challenge := base64.RawURLEncoding.EncodeToString(hash.Sum([]byte{})) for k, tc := range []struct { - d string - grant string - force bool - enablePlain bool - challenge string - method string - verifier string - code string - expectErr error - client *fosite.DefaultClient + d string + grant string + force bool + enablePlain bool + challenge string + method string + verifier string + code string + expectErr error + expectErrDesc string + client *fosite.DefaultClient }{ { - d: "fails because not auth code flow", - grant: "not_authorization_code", - expectErr: fosite.ErrUnknownRequest, + d: "fails because not auth code flow", + grant: "not_authorization_code", + expectErr: fosite.ErrUnknownRequest, + expectErrDesc: "The handler is not responsible for this request.", }, { d: "passes with private client", @@ -121,11 +124,13 @@ func TestPKCEHandlerValidate(t *testing.T) { code: "valid-code-1", }, { - d: "fails because invalid code", - grant: "authorization_code", - expectErr: fosite.ErrInvalidGrant, - client: pc, - code: "invalid-code-2", + d: "fails because invalid code", + grant: "authorization_code", + client: pc, + code: "invalid-code-2", + verifier: "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo", + expectErr: fosite.ErrInvalidGrant, + expectErrDesc: "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. Unable to find initial PKCE data tied to this request not_found", }, { d: "passes because auth code flow but pkce is not forced and no challenge given", @@ -134,12 +139,13 @@ func TestPKCEHandlerValidate(t *testing.T) { code: "valid-code-3", }, { - d: "fails because auth code flow and pkce challenge given but plain is disabled", - grant: "authorization_code", - challenge: "foo", - client: pc, - expectErr: fosite.ErrInvalidRequest, - code: "valid-code-4", + d: "fails because auth code flow and pkce challenge given but plain is disabled", + grant: "authorization_code", + challenge: "foo", + client: pc, + code: "valid-code-4", + expectErr: fosite.ErrInvalidRequest, + expectErrDesc: "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. Clients must use code_challenge_method=S256, plain is not allowed. The server is configured in a way that enforces PKCE S256 as challenge method for clients.", }, { d: "passes", @@ -163,69 +169,75 @@ func TestPKCEHandlerValidate(t *testing.T) { code: "valid-code-6", }, { - d: "fails because challenge and verifier do not match", - grant: "authorization_code", - challenge: "not-foo", - verifier: "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo", - method: "plain", - client: pc, - enablePlain: true, - code: "valid-code-7", - expectErr: fosite.ErrInvalidGrant, + d: "fails because challenge and verifier do not match", + grant: "authorization_code", + challenge: "not-foo", + verifier: "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo", + method: "plain", + client: pc, + enablePlain: true, + code: "valid-code-7", + expectErr: fosite.ErrInvalidGrant, + expectErrDesc: "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The PKCE code challenge did not match the code verifier.", }, { - d: "fails because challenge and verifier do not match", - grant: "authorization_code", - challenge: "not-foonot-foonot-foonot-foonot-foonot-foonot-foonot-foo", - verifier: "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo", - client: pc, - enablePlain: true, - code: "valid-code-8", - expectErr: fosite.ErrInvalidGrant, + d: "fails because challenge and verifier do not match", + grant: "authorization_code", + challenge: "not-foonot-foonot-foonot-foonot-foonot-foonot-foonot-foo", + verifier: "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo", + client: pc, + enablePlain: true, + code: "valid-code-8", + expectErr: fosite.ErrInvalidGrant, + expectErrDesc: "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The PKCE code challenge did not match the code verifier.", }, { - d: "fails because verifier is too short", - grant: "authorization_code", - challenge: "foo", - verifier: "foo", - method: "S256", - client: pc, - force: true, - code: "valid-code-9a", - expectErr: fosite.ErrInvalidGrant, + d: "fails because verifier is too short", + grant: "authorization_code", + challenge: "foo", + verifier: "foo", + method: "S256", + client: pc, + force: true, + code: "valid-code-9", + expectErr: fosite.ErrInvalidGrant, + expectErrDesc: "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The PKCE code verifier must be at least 43 characters.", }, { - d: "fails because verifier is too long", - grant: "authorization_code", - challenge: "foo", - verifier: "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo", - method: "S256", - client: pc, - force: true, - code: "valid-code-10", - expectErr: fosite.ErrInvalidGrant, + d: "fails because verifier is too long", + grant: "authorization_code", + challenge: "foo", + verifier: "foofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoofoo", + method: "S256", + client: pc, + force: true, + code: "valid-code-10", + expectErr: fosite.ErrInvalidGrant, + expectErrDesc: "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The PKCE code verifier can not be longer than 128 characters.", }, { - d: "fails because verifier is malformed", - grant: "authorization_code", - challenge: "foo", - verifier: `(!"/$%Z&$T()/)OUZI>$"&=/T(PUOI>"%/)TUOI&/(O/()RGTE>=/(%"/()="$/)(=()=/R/()=))`, - method: "S256", - client: pc, - force: true, - code: "valid-code-11", - expectErr: fosite.ErrInvalidGrant, + d: "fails because verifier is malformed", + grant: "authorization_code", + challenge: "foo", + verifier: `(!"/$%Z&$T()/)OUZI>$"&=/T(PUOI>"%/)TUOI&/(O/()RGTE>=/(%"/()="$/)(=()=/R/()=))`, + method: "S256", + client: pc, + force: true, + code: "valid-code-11", + expectErr: fosite.ErrInvalidGrant, + expectErrDesc: "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The PKCE code verifier must only contain [a-Z], [0-9], '-', '.', '_', '~'.", }, { - d: "fails because challenge and verifier do not match", - grant: "authorization_code", - challenge: "Zm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9v", - verifier: "Zm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9v", - method: "S256", - client: pc, - force: true, - code: "valid-code-12", - expectErr: fosite.ErrInvalidGrant, + d: "fails because challenge and verifier do not match", + grant: "authorization_code", + challenge: "Zm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9v", + verifier: "Zm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9v", + method: "S256", + client: pc, + force: true, + code: "valid-code-12", + expectErr: fosite.ErrInvalidGrant, + expectErrDesc: "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The PKCE code challenge did not match the code verifier.", }, { d: "passes because challenge and verifier match", @@ -237,25 +249,58 @@ func TestPKCEHandlerValidate(t *testing.T) { force: true, code: "valid-code-13", }, + { + d: "passes when not forced because no challenge or verifier", + grant: "authorization_code", + client: pc, + code: "valid-code-14", + }, + { + d: "fails when not forced because verifier provided when no challenge", + grant: "authorization_code", + client: pc, + code: "valid-code-15", + verifier: "Zm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9vZm9v", + expectErr: fosite.ErrInvalidGrant, + expectErrDesc: "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. The PKCE code verifier was provided but the code challenge was absent from the authorization request.", + }, } { t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { config.EnablePKCEPlainChallengeMethod = tc.enablePlain config.EnforcePKCE = tc.force ms.signature = tc.code ar := fosite.NewAuthorizeRequest() - ar.Form.Add("code_challenge", tc.challenge) - ar.Form.Add("code_challenge_method", tc.method) + + if len(tc.challenge) != 0 { + ar.Form.Add("code_challenge", tc.challenge) + } + + if len(tc.method) != 0 { + ar.Form.Add("code_challenge_method", tc.method) + } + + ar.Client = tc.client + require.NoError(t, s.CreatePKCERequestSession(nil, fmt.Sprintf("valid-code-%d", k), ar)) r := fosite.NewAccessRequest(nil) r.Client = tc.client r.GrantTypes = fosite.Arguments{tc.grant} - r.Form.Add("code_verifier", tc.verifier) + + if len(tc.verifier) != 0 { + r.Form.Add("code_verifier", tc.verifier) + } + + err := h.HandleTokenEndpointRequest(context.Background(), r) + if tc.expectErr == nil { - require.NoError(t, h.HandleTokenEndpointRequest(context.Background(), r)) + assert.NoError(t, err) } else { - err := h.HandleTokenEndpointRequest(context.Background(), r) - require.EqualError(t, err, tc.expectErr.Error(), "%+v", err) + assert.EqualError(t, err, tc.expectErr.Error(), "%+v", err) + + if len(tc.expectErrDesc) != 0 { + assert.EqualError(t, newtesterr(err), tc.expectErrDesc) + } } }) } @@ -345,3 +390,24 @@ func TestPKCEHandleTokenEndpointRequest(t *testing.T) { }) } } + +func newtesterr(err error) error { + if err == nil { + return nil + } + + var e *fosite.RFC6749Error + if errors.As(err, &e) { + return &testerr{e} + } + + return err +} + +type testerr struct { + *fosite.RFC6749Error +} + +func (e *testerr) Error() string { + return e.RFC6749Error.WithExposeDebug(true).GetDescription() +}