From 415558ac677b853d5b87f76106082df7b8849883 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Tue, 10 Nov 2026 22:54:59 +0800 Subject: [PATCH 1/4] fix(server): check the DPoP nonce before consuming the request object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PushAuthorizationRequest and BeginBackchannelAuthentication verified the request object (or signed authentication request), recording its jti as used, before verifying the request's DPoP proof and running the nonce check. With Dependencies.Nonces set, a first request without a nonce was refused with use_dpop_nonce after its request object had already been consumed, so the client's retry of the same request with a nonce-bearing proof (RFC 9449 §8 asks it to retry with the nonce, not to re-sign) was refused as a request object replay. Both endpoints now verify the optional DPoP proof, including its own jti replay record and the nonce, right after client authentication, in a shared verifyOptionalDPoPProof. The dpop_jkt binding steps stay where they were and take the verified proof's thumbprint. Error codes and messages are unchanged. Co-Authored-By: Claude Opus 5.5 --- .github/mutation/mutants.json | 6 +-- ARCHITECTURE.md | 2 +- conformance/server/oidf-config/README.md | 2 +- server/backchannel_authentication.go | 51 +++++++----------- server/backchannel_dpop_jkt_test.go | 53 ++++++++++++++++++ server/nonce_test.go | 60 +++++++++++++++++++-- server/par.go | 68 ++++++++++++++++-------- 7 files changed, 180 insertions(+), 62 deletions(-) diff --git a/.github/mutation/mutants.json b/.github/mutation/mutants.json index 7570806d..0a634ef5 100644 --- a/.github/mutation/mutants.json +++ b/.github/mutation/mutants.json @@ -193,15 +193,15 @@ }, { "id": "srv-ciba-dpop-nonce", - "file": "server/backchannel_authentication.go", + "file": "server/par.go", "old": "if s.deps.Nonces != nil {\n\t\tif challenge := s.checkDPoPNonce(", "new": "if s.deps.Nonces != nil && false {\n\t\tif challenge := s.checkDPoPNonce(", "module": "root", "packages": [ "./server" ], - "run": "^TestBackchannelAuthenticationChallengesDPoPNonce$", - "why": "the backchannel authentication endpoint enforces DPoP nonces" + "run": "^(TestBackchannelAuthenticationChallengesDPoPNonce|TestPushAuthorizationRequestChallengesMissingNonceWhenDPoPPresented)$", + "why": "the backchannel authentication and PAR endpoints enforce DPoP nonces" }, { "id": "srv-access-token-size-cap", diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 9c1cc631..a91840d4 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1180,7 +1180,7 @@ whenever `Config.MTLSEndpoints` is configured, the suite's own (confirmed by disassembly); (2) a DPoP proof's `htu` claim was checked against this server's plain endpoint URL only, in both `verifyTokenRequestDPoP` (`server/token.go`) and -`reconcileParDPoPBinding` (`server/par.go`) — but a client +`verifyOptionalDPoPProof` (`server/par.go`) — but a client authenticating via certificate must call the mTLS-alias URL to present it at all, so its DPoP proof legitimately names that alias instead; confirmed live via the token endpoint's `dpop: htu does not match diff --git a/conformance/server/oidf-config/README.md b/conformance/server/oidf-config/README.md index e4b8e0f5..08d78f89 100644 --- a/conformance/server/oidf-config/README.md +++ b/conformance/server/oidf-config/README.md @@ -324,7 +324,7 @@ run: at all without that listener existing. - **Fixed: a DPoP proof presented at the mTLS-alias URL was rejected outright.** Both `verifyTokenRequestDPoP` (`server/token.go`) and - `reconcileParDPoPBinding` (`server/par.go`) checked a proof's `htu` + `verifyOptionalDPoPProof` (`server/par.go`) checked a proof's `htu` against this server's *plain* endpoint URL only. That's fine for every scenario tried before this profile — `sender_constrain=mtls` never uses DPoP at all, and every DPoP-sender-constrained client diff --git a/server/backchannel_authentication.go b/server/backchannel_authentication.go index f1399073..1a60b9cb 100644 --- a/server/backchannel_authentication.go +++ b/server/backchannel_authentication.go @@ -16,7 +16,6 @@ import ( fapi "github.com/idfoundry/fapigo" "github.com/idfoundry/fapigo/extension" - "github.com/idfoundry/fapigo/internal/dpop" "github.com/idfoundry/fapigo/internal/requestobject" "github.com/idfoundry/fapigo/keys" "github.com/idfoundry/fapigo/storage" @@ -170,6 +169,14 @@ func (s *Server) BeginBackchannelAuthentication(ctx context.Context, req BeginBa if authErr != nil { return s.backchannelBeginFail(ctx, "", authErr), nil } + // Before the signed authentication request, which records its jti: + // see verifyOptionalDPoPProof. A client authenticating with its + // certificate calls the mTLS alias (RFC 8705 §5), and its proof names + // that URL, as at PAR and the token endpoint. + dpopThumbprint, dpopErr := s.verifyOptionalDPoPProof(ctx, dpopProof, s.cfg.Endpoints.BackchannelAuthentication, s.cfg.MTLSEndpoints.BackchannelAuthentication) + if dpopErr != nil { + return s.backchannelBeginFail(ctx, client.ID(), dpopErr), nil + } verified, resolveErr := s.resolveBackchannelAuthenticationParameters(ctx, params, client) if resolveErr != nil { @@ -186,9 +193,9 @@ func (s *Server) BeginBackchannelAuthentication(ctx context.Context, req BeginBa return s.backchannelBeginFail(ctx, client.ID(), subjectErr), nil } - dpopJKT, dpopErr := s.reconcileBackchannelDPoPBinding(ctx, dpopProof, validated.params) - if dpopErr != nil { - return s.backchannelBeginFail(ctx, client.ID(), dpopErr), nil + dpopJKT, bindErr := reconcileBackchannelDPoPBinding(dpopThumbprint, validated.params) + if bindErr != nil { + return s.backchannelBeginFail(ctx, client.ID(), bindErr), nil } interaction, err := s.backchannelInteraction(client, validated.params, validated.resources) @@ -623,38 +630,20 @@ func hasVisibleContent(msg string) bool { // counterpart: a DPoP proof at the backchannel authentication endpoint // is optional, exactly like PAR — see Open Question 1 in this feature's // design plan for why this mirrors PAR rather than the token endpoint's -// always-required proof. It returns the key thumbprint the request is -// bound to, which the token exchange then requires its DPoP proof's key -// to match: the proof's key, or an explicit dpop_jkt parameter (RFC 9449 -// §10, applied here as at PAR) when there's no proof. A dpop_jkt that -// doesn't match a proof sent with it is refused. -func (s *Server) reconcileBackchannelDPoPBinding(ctx context.Context, proof string, params map[string]json.RawMessage) (string, *Error) { +// always-required proof. thumbprint is the key thumbprint of the proof +// verifyOptionalDPoPProof already verified, or "" when there was none. +// It returns the key thumbprint the request is bound to, which the token +// exchange then requires its DPoP proof's key to match: the proof's key, +// or an explicit dpop_jkt parameter (RFC 9449 §10, applied here as at +// PAR) when there's no proof. A dpop_jkt that doesn't match a proof sent +// with it is refused. +func reconcileBackchannelDPoPBinding(thumbprint string, params map[string]json.RawMessage) (string, *Error) { // A dpop_jkt that isn't a non-empty string was refused with the // request's other parameters (backchannelParameterTypes). declared, _ := jsonString(params, "dpop_jkt") - if proof == "" { + if thumbprint == "" { return declared, nil } - // A client authenticating with its certificate calls the mTLS alias - // (RFC 8705 §5), and its proof names that URL, as at PAR and the - // token endpoint. - verified, err := verifyDPoPAtEitherEndpoint(ctx, dpop.VerifyRequest{ - Proof: proof, - Method: "POST", - Now: s.deps.Clock.Now(), - MaxProofAge: s.cfg.Limits.MaxDPoPProofAge, - MaxClockSkew: s.cfg.Limits.MaxClockSkew, - Replay: s.dpopReplayChecker(), - }, s.cfg.Endpoints.BackchannelAuthentication, s.cfg.MTLSEndpoints.BackchannelAuthentication) - if err != nil { - return "", newError(ErrorInvalidDPoPProof, 400, "DPoP proof verification failed", err) - } - if s.deps.Nonces != nil { - if challenge := s.checkDPoPNonce(ctx, verified.Nonce, s.deps.Clock.Now()); challenge != nil { - return "", challenge - } - } - thumbprint := verified.Thumbprint.String() if declared != "" && declared != thumbprint { return "", newError(ErrorInvalidRequest, 400, "dpop_jkt does not match the DPoP proof presented to the backchannel authentication endpoint", nil) } diff --git a/server/backchannel_dpop_jkt_test.go b/server/backchannel_dpop_jkt_test.go index 560720d9..5b837162 100644 --- a/server/backchannel_dpop_jkt_test.go +++ b/server/backchannel_dpop_jkt_test.go @@ -190,3 +190,56 @@ func TestBackchannelAuthenticationChallengesDPoPNonce(t *testing.T) { t.Fatalf("proof with the issued nonce: action = %T, want BackchannelInteractionRequired", action) } } + +// TestBackchannelAuthenticationNonceChallengeKeepsRequestUsable: a +// use_dpop_nonce challenge must not consume the signed authentication +// request's jti. The client retries the same signed request with a fresh +// proof carrying the nonce, and that retry succeeds; only a second +// successful use is a replay. +func TestBackchannelAuthenticationNonceChallengeKeepsRequestUsable(t *testing.T) { + endpoint, err := url.Parse(testBackchannelAuthenticationEndpoint) + if err != nil { + t.Fatalf("url.Parse: %v", err) + } + h := newHarnessWithBackchannelOptions(t, memstore.NewBackchannelAuthenticationStore(), + func(cfg *server.Config) { cfg.Limits.DPoPNonceLifetime = time.Minute }, + func(deps *server.Dependencies) { deps.Nonces = memstore.NewNonceStore() }, + ) + dpopKey := generateKey(t) + requestObj := h.backchannelRequestObject(t, standardBackchannelParams(t)) + begin := func(nonce string) server.BackchannelAuthenticationAction { + t.Helper() + proof, err := dpop.CreateProof(dpop.ProofRequest{Signer: dpopKey, Algorithm: fapi.ES256, Method: "POST", URL: endpoint, Now: h.now, Nonce: nonce}) + if err != nil { + t.Fatalf("CreateProof: %v", err) + } + action, err := h.server.BeginBackchannelAuthentication(context.Background(), server.BeginBackchannelAuthenticationRequest{ + HTTP: server.FormRequest{Parameters: backchannelFormParams(h.clientAssertion(t), requestObj)}, + DPoPProofs: []string{proof}, + }) + if err != nil { + t.Fatalf("BeginBackchannelAuthentication: %v", err) + } + return action + } + + localErr, ok := begin("").(server.BackchannelAuthenticationLocalError) + if !ok || localErr.Error.Code() != server.ErrorUseDPoPNonce { + t.Fatalf("proof without a nonce: want a use_dpop_nonce BackchannelAuthenticationLocalError") + } + issued := localErr.Error.Nonce() + + if action := begin(issued); !isBackchannelInteractionRequired(action) { + t.Fatalf("retry of the same signed request with the issued nonce: action = %#v, want BackchannelInteractionRequired", action) + } + + localErr, ok = begin(issued).(server.BackchannelAuthenticationLocalError) + if !ok || localErr.Error.Code() != server.ErrorInvalidRequest { + t.Fatalf("signed request used a second time: want an invalid_request BackchannelAuthenticationLocalError") + } +} + +func isBackchannelInteractionRequired(action server.BackchannelAuthenticationAction) bool { + _, ok := action.(server.BackchannelInteractionRequired) + return ok +} diff --git a/server/nonce_test.go b/server/nonce_test.go index 6d15a1a5..68c1befa 100644 --- a/server/nonce_test.go +++ b/server/nonce_test.go @@ -56,19 +56,24 @@ func TestNewRejectsInvalidNonceConfig(t *testing.T) { // newHarnessWithNonces mirrors newHarnessWithIdentityClaims's shape // (identity_claims_test.go), but wires a memstore.NonceStore and // Limits.DPoPNonceLifetime so token/PAR nonce-challenge tests don't have -// to duplicate every other piece of a valid harness. -func newHarnessWithNonces(t *testing.T) (harness, *memstore.NonceStore) { +// to duplicate every other piece of a valid harness. mutateClient, if +// given, adjusts the registered client's configuration. +func newHarnessWithNonces(t *testing.T, mutateClient ...func(*storage.RegisteredClientConfig)) (harness, *memstore.NonceStore) { t.Helper() now := time.Now() key := generateKey(t) serverKey := generateKey(t) - client, err := storage.NewRegisteredClient(storage.RegisteredClientConfig{ + clientCfg := storage.RegisteredClientConfig{ ID: testClientID, RedirectURIs: []fapi.RegisteredRedirectURI{testRedirectURI}, ClientAssertionAlgorithm: fapi.ES256, AllowedScopes: []string{"openid", "accounts", "offline_access"}, - }) + } + for _, mutate := range mutateClient { + mutate(&clientCfg) + } + client, err := storage.NewRegisteredClient(clientCfg) if err != nil { t.Fatalf("NewRegisteredClient: %v", err) } @@ -519,6 +524,53 @@ func TestPushAuthorizationRequestAcceptsValidNonceAndHandsOutNext(t *testing.T) } } +// TestPushAuthorizationRequestNonceChallengeKeepsRequestObjectUsable: +// a use_dpop_nonce challenge must not consume the request object's jti. +// The client retries the same signed request object with a fresh proof +// carrying the nonce (RFC 9449 §8 asks it to re-send, not re-sign), and +// that retry succeeds; only a second successful use is a replay. +func TestPushAuthorizationRequestNonceChallengeKeepsRequestObjectUsable(t *testing.T) { + h, _ := newHarnessWithNonces(t, func(cfg *storage.RegisteredClientConfig) { cfg.RequestObjectAlgorithm = fapi.ES256 }) + dpopKey := generateKey(t) + parURL, err := url.Parse(testPAREndpoint) + if err != nil { + t.Fatalf("url.Parse: %v", err) + } + requestObj := h.requestObject(t, standardAuthParams(t)) + push := func(nonce string) error { + t.Helper() + proof, err := dpop.CreateProof(dpop.ProofRequest{ + Signer: dpopKey, Algorithm: fapi.ES256, Method: "POST", URL: parURL, Now: h.now, Nonce: nonce, + }) + if err != nil { + t.Fatalf("dpop.CreateProof: %v", err) + } + _, err = h.server.PushAuthorizationRequest(context.Background(), server.PushAuthorizationRequest{ + HTTP: server.FormRequest{Parameters: []server.FormParameter{ + formParam("client_assertion", h.clientAssertion(t)), + formParam("client_assertion_type", clientassertion.AssertionType), + formParam("request", requestObj), + }}, + DPoPProofs: []string{proof}, + }) + return err + } + + err = push("") + if code := serverErrorCode(t, err); code != server.ErrorUseDPoPNonce { + t.Fatalf("proof without a nonce: error code = %q, want %q", code, server.ErrorUseDPoPNonce) + } + issued := err.(*server.Error).Nonce() + + if err := push(issued); err != nil { + t.Fatalf("retry of the same request object with the issued nonce: %v, want success", err) + } + + if code := serverErrorCode(t, push(issued)); code != server.ErrorInvalidRequestObject { + t.Fatalf("request object used a second time: error code = %q, want %q", code, server.ErrorInvalidRequestObject) + } +} + // TestNonceIssuedAtPARIsValidAtTokenEndpoint confirms one shared nonce // store covers everything this server verifies (Dependencies.Nonces's // own doc comment) — a nonce issued from a PAR challenge must also be diff --git a/server/par.go b/server/par.go index f9c5a9f0..3624dec1 100644 --- a/server/par.go +++ b/server/par.go @@ -237,6 +237,12 @@ func (s *Server) PushAuthorizationRequest(ctx context.Context, req PushAuthoriza if subjectErr := s.checkClientSubjectIssuable(client); subjectErr != nil { return s.parFail(ctx, client.ID(), subjectErr) } + // Before the request object, which records its jti: see + // verifyOptionalDPoPProof. + dpopThumbprint, dpopErr := s.verifyOptionalDPoPProof(ctx, dpopProof, s.cfg.Endpoints.PushedAuthorizationRequest, s.cfg.MTLSEndpoints.PushedAuthorizationRequest) + if dpopErr != nil { + return s.parFail(ctx, client.ID(), dpopErr) + } authParams, tokenClaims, requestedResources, resolveErr := s.resolveAuthorizationParameters(ctx, params, formResources, client) if resolveErr != nil { @@ -257,9 +263,9 @@ func (s *Server) PushAuthorizationRequest(ctx context.Context, req PushAuthoriza return s.parFail(ctx, client.ID(), mapErr) } - validated, dpopErr := s.reconcileParDPoPBinding(ctx, dpopProof, validated) - if dpopErr != nil { - return s.parFail(ctx, client.ID(), dpopErr) + validated, bindErr := reconcileParDPoPBinding(dpopThumbprint, validated) + if bindErr != nil { + return s.parFail(ctx, client.ID(), bindErr) } requestURI, err := par.GenerateRequestURI(s.deps.Random) @@ -815,26 +821,26 @@ func (s *Server) validateAuthorizationParameters(params map[string]json.RawMessa return params, nil } -// reconcileParDPoPBinding handles an optional DPoP proof presented -// alongside the pushed authorization request (RFC 9449 §10). A DPoP -// proof at the PAR endpoint is not itself required, but if the client -// sends one, this server: -// - verifies it (proof of possession of the key, matching htm/htu), -// - rejects the request outright if an explicit dpop_jkt parameter -// was also given and doesn't match the proof's key thumbprint, and -// - otherwise derives an implicit dpop_jkt from the proof's own key -// when the request carried none, so the resulting authorization -// code ends up bound to whichever key the client actually -// demonstrated possession of at PAR time — not just whichever key -// later shows up at the token endpoint. +// verifyOptionalDPoPProof verifies the optional DPoP proof presented +// to the PAR or backchannel authentication endpoint (RFC 9449 §10) — +// proof of possession of the key, matching htm/htu, its own jti replay +// record — and, when Dependencies.Nonces is set, its nonce (§8). It +// returns the proof key's thumbprint, or "" when no proof was sent. +// +// Both endpoints call it straight after client authentication, before +// the request object (or signed authentication request) is verified: +// verifying that records its single-use jti, so a nonce challenge +// issued after it would make the client's legitimate retry of the same +// signed request fail as a replay (§8 asks the client only to retry with +// the nonce, not to re-sign its request). // -// The proof's htu is checked against either this endpoint's plain URL +// The proof's htu is checked against either the endpoint's plain URL // or its mTLS alias (RFC 8705 §5) — see verifyDPoPAtEitherEndpoint's own // doc comment for why a DPoP-sender-constrained client may legitimately // call the alias here too. -func (s *Server) reconcileParDPoPBinding(ctx context.Context, proof string, params map[string]json.RawMessage) (map[string]json.RawMessage, *Error) { +func (s *Server) verifyOptionalDPoPProof(ctx context.Context, proof string, endpoint, mtlsAlias fapi.URL) (string, *Error) { if proof == "" { - return params, nil + return "", nil } verified, err := verifyDPoPAtEitherEndpoint(ctx, dpop.VerifyRequest{ Proof: proof, @@ -843,16 +849,34 @@ func (s *Server) reconcileParDPoPBinding(ctx context.Context, proof string, para MaxProofAge: s.cfg.Limits.MaxDPoPProofAge, MaxClockSkew: s.cfg.Limits.MaxClockSkew, Replay: s.dpopReplayChecker(), - }, s.cfg.Endpoints.PushedAuthorizationRequest, s.cfg.MTLSEndpoints.PushedAuthorizationRequest) + }, endpoint, mtlsAlias) if err != nil { - return nil, newError(ErrorInvalidDPoPProof, 400, "DPoP proof verification failed", err) + return "", newError(ErrorInvalidDPoPProof, 400, "DPoP proof verification failed", err) } if s.deps.Nonces != nil { if challenge := s.checkDPoPNonce(ctx, verified.Nonce, s.deps.Clock.Now()); challenge != nil { - return nil, challenge + return "", challenge } } - thumbprint := verified.Thumbprint.String() + return verified.Thumbprint.String(), nil +} + +// reconcileParDPoPBinding binds the pushed authorization request to the +// key of the optional DPoP proof presented alongside it (RFC 9449 §10), +// already verified by verifyOptionalDPoPProof — thumbprint is that +// proof's key thumbprint, or "" when the client sent no proof. With a +// proof, this server: +// - rejects the request outright if an explicit dpop_jkt parameter +// was also given and doesn't match the proof's key thumbprint, and +// - otherwise derives an implicit dpop_jkt from the proof's own key +// when the request carried none, so the resulting authorization +// code ends up bound to whichever key the client actually +// demonstrated possession of at PAR time — not just whichever key +// later shows up at the token endpoint. +func reconcileParDPoPBinding(thumbprint string, params map[string]json.RawMessage) (map[string]json.RawMessage, *Error) { + if thumbprint == "" { + return params, nil + } if declared, jsonErr := jsonString(params, "dpop_jkt"); jsonErr == nil && declared != "" { if declared != thumbprint { From 770c1b5b0003a25278d0e98c740a3dcd5fb12044 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Tue, 10 Nov 2026 22:56:03 +0800 Subject: [PATCH 2/4] test(interop): run the Message Signing instance with DPoP nonces on The Message Signing conformance-as instance ran with -dpop-nonce-challenge=false because the server consumed a pushed request object before its DPoP nonce challenge, so openid-client's nonce retry of the same signed request was refused as a replay. With the server now checking the nonce first, leg 3 runs with nonces on and the README's known-disagreement entry for it is gone. Co-Authored-By: Claude Opus 5.5 --- interop/README.md | 14 +------------- interop/run-openid-client.sh | 7 +------ 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/interop/README.md b/interop/README.md index 3e9719b2..93067b79 100644 --- a/interop/README.md +++ b/interop/README.md @@ -92,16 +92,4 @@ the one at fault. ## Known disagreements -- **DPoP nonce retry of a pushed signed request object (FAPIgo).** With - DPoP nonces on, `server.PushAuthorizationRequest` verifies the request - object, recording its `jti` as used, before it checks the DPoP proof's - nonce. The request is then refused with `use_dpop_nonce`, and - openid-client retries it with the nonce, as RFC 9449 §8 expects of a - client, resending the same signed request object; FAPIgo refuses the - retry as a replay (`invalid_request_object`). A request the server - refused shouldn't use up its request object: RFC 9449 §8 makes the - retry part of the normal flow, and nothing in RFC 9101 or FAPI 2.0 - Message Signing asks a client to re-sign it. Until FAPIgo checks the - nonce first (or records the `jti` only once the request succeeds), the - Message Signing instance runs with `-dpop-nonce-challenge=false`; leg 1 - covers nonce handling. +None known. diff --git a/interop/run-openid-client.sh b/interop/run-openid-client.sh index 61933a14..25edf2d0 100755 --- a/interop/run-openid-client.sh +++ b/interop/run-openid-client.sh @@ -79,13 +79,8 @@ echo "interop: starting conformance-as" "${work_dir}/conformance-as" -config "${work_dir}/as-baseline.json" \ -mtls -ciba -client-credentials-grant >"${work_dir}/as-baseline.log" 2>&1 & server_pids+=("$!") -# -dpop-nonce-challenge=false: with nonces on, the PAR request that gets -# the use_dpop_nonce challenge has already recorded its request object's -# jti, so openid-client's retry of the same request (RFC 9449 §8) is -# refused as a replay. A FAPIgo issue: see interop/README.md, "Known -# disagreements". Leg 1 covers nonce handling on the baseline instance. "${work_dir}/conformance-as" -config "${work_dir}/as-message-signing.json" \ - -dpop-nonce-challenge=false >"${work_dir}/as-message-signing.log" 2>&1 & + >"${work_dir}/as-message-signing.log" 2>&1 & server_pids+=("$!") for port in "${INTEROP_AS_PORT}" "${INTEROP_AS_MTLS_PORT}" "${INTEROP_AS_MS_PORT}"; do From 725d3c2a703c40a56ac5b380f1cb39f7b7f34998 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Wed, 11 Nov 2026 00:11:43 +0800 Subject: [PATCH 3/4] refactor(server): move PAR's PKCE parameter checks into a helper validateAuthorizationParameters' cognitive complexity was just over the limit; the PKCE checks move into validatePKCEParameters unchanged. Co-Authored-By: Claude Opus 5.5 --- server/par.go | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/server/par.go b/server/par.go index 3624dec1..be0be773 100644 --- a/server/par.go +++ b/server/par.go @@ -780,19 +780,8 @@ func (s *Server) validateAuthorizationParameters(params map[string]json.RawMessa return nil, newError(ErrorInvalidRequest, 400, "redirect_uri is not an acceptable redirect destination", err) } - codeChallenge, err := jsonString(params, "code_challenge") - if err != nil { - return nil, newError(ErrorInvalidRequest, 400, "code_challenge is required", err) - } - if codeChallenge == "" { - return nil, newError(ErrorInvalidRequest, 400, "code_challenge must not be empty", nil) - } - codeChallengeMethod, err := jsonString(params, "code_challenge_method") - if err != nil { - return nil, newError(ErrorInvalidRequest, 400, "code_challenge_method is required", err) - } - if _, err := pkce.ParseMethod(codeChallengeMethod); err != nil { - return nil, newError(ErrorInvalidRequest, 400, "code_challenge_method must be S256", err) + if err := validatePKCEParameters(params); err != nil { + return nil, err } if _, _, err := requestedMaxAge(params); err != nil { @@ -821,6 +810,27 @@ func (s *Server) validateAuthorizationParameters(params map[string]json.RawMessa return params, nil } +// validatePKCEParameters checks the request's PKCE parameters (RFC 7636 +// §4.3): a non-empty code_challenge, and code_challenge_method S256, the +// only method FAPI 2.0 allows. +func validatePKCEParameters(params map[string]json.RawMessage) *Error { + codeChallenge, err := jsonString(params, "code_challenge") + if err != nil { + return newError(ErrorInvalidRequest, 400, "code_challenge is required", err) + } + if codeChallenge == "" { + return newError(ErrorInvalidRequest, 400, "code_challenge must not be empty", nil) + } + codeChallengeMethod, err := jsonString(params, "code_challenge_method") + if err != nil { + return newError(ErrorInvalidRequest, 400, "code_challenge_method is required", err) + } + if _, err := pkce.ParseMethod(codeChallengeMethod); err != nil { + return newError(ErrorInvalidRequest, 400, "code_challenge_method must be S256", err) + } + return nil +} + // verifyOptionalDPoPProof verifies the optional DPoP proof presented // to the PAR or backchannel authentication endpoint (RFC 9449 §10) — // proof of possession of the key, matching htm/htu, its own jti replay From 847009d342b6081024e2fe8ad9f7379548383e9a Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Wed, 11 Nov 2026 00:28:32 +0800 Subject: [PATCH 4/4] refactor(server): move PAR's response_type check into a helper validateAuthorizationParameters was still just over the cognitive complexity limit; the response_type checks move into validateResponseType unchanged. Co-Authored-By: Claude Opus 5.5 --- server/par.go | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/server/par.go b/server/par.go index be0be773..5e3e644d 100644 --- a/server/par.go +++ b/server/par.go @@ -760,13 +760,8 @@ func (s *Server) validateAuthorizationParameters(params map[string]json.RawMessa if err := s.checkResponseMode(params); err != nil { return nil, err } - responseType, err := jsonString(params, "response_type") - if err != nil || responseType == "" { - return nil, newError(ErrorInvalidRequest, 400, "response_type is required", err) - } - if responseType != "code" { - // RFC 6749 §4.1.2.1. - return nil, newError(ErrorUnsupportedResponseType, 400, `response_type must be "code"`, nil) + if err := validateResponseType(params); err != nil { + return nil, err } redirectURI, err := jsonString(params, "redirect_uri") @@ -810,6 +805,20 @@ func (s *Server) validateAuthorizationParameters(params map[string]json.RawMessa return params, nil } +// validateResponseType checks the request's response_type: present, +// and "code", the only response type this server supports. +func validateResponseType(params map[string]json.RawMessage) *Error { + responseType, err := jsonString(params, "response_type") + if err != nil || responseType == "" { + return newError(ErrorInvalidRequest, 400, "response_type is required", err) + } + if responseType != "code" { + // RFC 6749 §4.1.2.1. + return newError(ErrorUnsupportedResponseType, 400, `response_type must be "code"`, nil) + } + return nil +} + // validatePKCEParameters checks the request's PKCE parameters (RFC 7636 // §4.3): a non-empty code_challenge, and code_challenge_method S256, the // only method FAPI 2.0 allows.