fix: delete the PKCE request session only after a successful verification

handler/pkce/handler.go

@@ -146,10 +146,6 @@ func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite
 		return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
 	}
 
-	if err := c.Storage.DeletePKCERequestSession(ctx, signature); err != nil {
-		return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
-	}
-
 	challenge := pkceRequest.GetRequestForm().Get("code_challenge")
 	method := pkceRequest.GetRequestForm().Get("code_challenge_method")
 	client := pkceRequest.GetClient()
@@ -160,7 +156,9 @@ func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite
 	nc := len(challenge)
 
 	if !c.Config.GetEnforcePKCE(ctx) && nc == 0 && nv == 0 {
-		return nil
+		// No challenge was bound and none is required, so this is a valid
+		// non-PKCE exchange. Consume the session before allowing it through.
+		return c.consumePKCERequestSession(ctx, signature)
 	}
 
 	// NOTE: The code verifier SHOULD have enough entropy to make it
@@ -180,6 +178,13 @@ func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite
 		return errorsx.WithStack(fosite.ErrInvalidGrant.
 			WithHint("The PKCE code verifier must only contain [a-Z], [0-9], '-', '.', '_', '~'."))
 	} else if nc == 0 {
+		// A verifier was presented against a session that never had a challenge
+		// bound to it. There is nothing here a downgrade could exploit, so the
+		// session is consumed before rejecting the request.
+		if err := c.consumePKCERequestSession(ctx, signature); err != nil {
+			return err
+		}
+
 		return errorsx.WithStack(fosite.ErrInvalidGrant.
 			WithHint("The PKCE code verifier was provided but the code challenge was absent from the authorization request."))
 	}
@@ -205,6 +210,12 @@ func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite
 	// as normal (as defined by OAuth 2.0 [RFC6749]).  If the values are not
 	// equal, an error response indicating "invalid_grant" as described in
 	// Section 5.2 of [RFC6749] MUST be returned.
+	//
+	// The session is deleted only once the verifier is confirmed to match the
+	// bound challenge below -- never beforehand, and never on a failed match.
+	// Deleting it on a failed attempt would strip the challenge, after which
+	// the same code could be replayed with no verifier at all, downgrading the
+	// exchange to a non-PKCE one.
 	switch method {
 	case "S256":
 		hash := sha256.New()
@@ -226,6 +237,17 @@ func (c *Handler) HandleTokenEndpointRequest(ctx context.Context, request fosite
 		}
 	}
 
+	return c.consumePKCERequestSession(ctx, signature)
+}
+
+// consumePKCERequestSession deletes the PKCE request session tied to
+// signature. Call this only once a request has been fully validated, or
+// determined not to need PKCE at all -- see the downgrade note in
+// HandleTokenEndpointRequest.
+func (c *Handler) consumePKCERequestSession(ctx context.Context, signature string) error {
+	if err := c.Storage.DeletePKCERequestSession(ctx, signature); err != nil {
+		return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
+	}
 	return nil
 }
 

handler/pkce/handler_test.go

@@ -391,6 +391,109 @@ func TestPKCEHandleTokenEndpointRequest(t *testing.T) {
 	}
 }
 
+// sessionExists reports whether a PKCE request session is still stored under
+// signature.
+func sessionExists(t *testing.T, s PKCERequestStorage, signature string) bool {
+	t.Helper()
+	_, err := s.GetPKCERequestSession(context.Background(), signature, nil)
+	if err == nil {
+		return true
+	}
+	require.ErrorIs(t, err, fosite.ErrNotFound)
+	return false
+}
+
+// TestHandleTokenEndpointRequest_SessionLifecycle locks in when
+// HandleTokenEndpointRequest consumes (deletes) the PKCE request session tied
+// to a code, per the downgrade note on that method: never on a failed
+// verification, so a failed attempt can't strip a bound challenge and let the
+// same code be replayed with no verifier as a non-PKCE exchange.
+func TestHandleTokenEndpointRequest_SessionLifecycle(t *testing.T) {
+	s256verifier := "KGCt4m8AmjUvIR5ArTByrmehjtbxn1A49YpTZhsH8N7fhDr7LQayn9xx6mck"
+	hash := sha256.New()
+	hash.Write([]byte(s256verifier))
+	s256challenge := base64.RawURLEncoding.EncodeToString(hash.Sum([]byte{}))
+
+	for _, tc := range []struct {
+		d               string
+		force           bool
+		challenge       string
+		method          string
+		verifier        string
+		wantErr         bool
+		wantSessionLeft bool
+	}{
+		{
+			d:         "match: session is consumed",
+			challenge: s256challenge,
+			method:    "S256",
+			verifier:  s256verifier,
+		},
+		{
+			d:               "mismatch: session survives so a retry can't downgrade",
+			challenge:       s256challenge,
+			method:          "S256",
+			verifier:        "wrong-verifier-wrong-verifier-wrong-verifier",
+			wantErr:         true,
+			wantSessionLeft: true,
+		},
+		{
+			d:               "verifier missing when a challenge was bound: session survives",
+			challenge:       s256challenge,
+			method:          "S256",
+			wantErr:         true,
+			wantSessionLeft: true,
+		},
+		{
+			d:        "no challenge or verifier, not enforced: session is consumed",
+			verifier: "",
+		},
+		{
+			d:        "verifier presented against a session with no bound challenge: session is consumed",
+			method:   "S256",
+			verifier: s256verifier,
+			wantErr:  true,
+		},
+	} {
+		t.Run(tc.d, func(t *testing.T) {
+			s := storage.NewMemoryStore()
+			ms := &mockCodeStrategy{signature: "code-under-test"}
+			h := &Handler{
+				Storage:               s,
+				AuthorizeCodeStrategy: ms,
+				Config:                &fosite.Config{EnforcePKCE: tc.force},
+			}
+			client := &fosite.DefaultClient{}
+
+			ar := fosite.NewAuthorizeRequest()
+			ar.Client = client
+			if tc.challenge != "" {
+				ar.Form.Add("code_challenge", tc.challenge)
+			}
+			if tc.method != "" {
+				ar.Form.Add("code_challenge_method", tc.method)
+			}
+			require.NoError(t, s.CreatePKCERequestSession(context.Background(), ms.signature, ar))
+
+			r := fosite.NewAccessRequest(nil)
+			r.Client = client
+			r.GrantTypes = fosite.Arguments{"authorization_code"}
+			if tc.verifier != "" {
+				r.Form.Add("code_verifier", tc.verifier)
+			}
+
+			err := h.HandleTokenEndpointRequest(context.Background(), r)
+			if tc.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+
+			assert.Equal(t, tc.wantSessionLeft, sessionExists(t, s, ms.signature))
+		})
+	}
+}
+
 func newtesterr(err error) error {
 	if err == nil {
 		return nil