From 106651d55abc4258d12a4903fc4b96684cb9e472 Mon Sep 17 00:00:00 2001 From: Leif Johansson Date: Fri, 23 Oct 2026 02:42:38 +0200 Subject: [PATCH 1/2] fix(verifier): a declined presentation ends the session A user who declines gets HTTP 500 and a session that never ends. VerificationDirectPostRequest declared only `response`, so a Wallet's Authorization Error Response bound to an empty struct; the handler then called GetKID() unconditionally and went looking for a JWE header in "". Observed live as ERROR "failed to get KID from request" {"error":"failed to parse JWT header: unexpected end of JSON input"} returned bare, so NewErrorFromError could not classify it and it went out as a 500 - for the one outcome that is not a failure at all. Behind that, nothing recorded the refusal. /ui/completion is driven by CompletedResponseCode, and a session with no response code reports "pending": there was no terminal state for a refusal to reach. So the UI polled until the cache evicted the context and the relying party never learned of a decision the user had already made. Recording the decline alone would not have fixed it, which is why SessionOutcome replaces a bare response code and /ui/completion gained "declined". The plain form the engine sends is conformant, not a wallet quirk. OpenID4VP 1.0 section 8.3.1: "If a Wallet is unable to generate an encrypted response, it MAY send an error response without encryption as per Section 8.2" - so no JWE even though the request asked for direct_post.jwt, and section 8.2's own example is exactly these three form parameters. Section 5.6 says both successful and error responses SHOULD use the supplied Response Mode, so an error may ALSO arrive encrypted; both are accepted, the encrypted one checked before the presentation parse because an error legitimately has no vp_token and would otherwise be reported as a malformed presentation. state is validated against a live session (section 14.3.2: the Verifier SHOULD check that state corresponds to a recent Authorization Request). An unknown state is refused rather than ignored: accepting one would let anyone who can reach response_uri end a session they know nothing about, and ignoring one would answer 200 while leaving the real session pending. An absent and an unknown state get the same answer, because which states are live is not something this endpoint should confirm to an unauthenticated caller. A request carrying neither a presentation nor an error is now a classified 400. It is the caller's to fix, and it used to reach GetKID and come back as the same 500. No redirect_uri is returned. Section 8.2 permits one and the engine reads it back, but where a declined flow should send the browser is a UX decision nobody has made, and inventing one would land users on a page that does not exist. Fixes #846 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AB3apg5kCEGNPozrhSggum --- .../apiv1/direct_post_decline_test.go | 150 ++++++++++++++++++ internal/verifier/apiv1/handlers_ui.go | 45 +++++- .../verifier/apiv1/handlers_verification.go | 125 +++++++++++++++ internal/verifier/httpserver/api.go | 1 + .../endpoints_session_preference_test.go | 4 + internal/verifier/httpserver/endpoints_ui.go | 18 ++- pkg/cache/authcontext_types.go | 26 ++- 7 files changed, 356 insertions(+), 13 deletions(-) create mode 100644 internal/verifier/apiv1/direct_post_decline_test.go diff --git a/internal/verifier/apiv1/direct_post_decline_test.go b/internal/verifier/apiv1/direct_post_decline_test.go new file mode 100644 index 000000000..270cda56c --- /dev/null +++ b/internal/verifier/apiv1/direct_post_decline_test.go @@ -0,0 +1,150 @@ +package apiv1 + +import ( + "context" + "net/http" + "testing" + + "github.com/SUNET/vc/internal/verifier/notify" + "github.com/SUNET/vc/pkg/cache" + "github.com/SUNET/vc/pkg/helpers" + "github.com/SUNET/vc/pkg/logger" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// declineClient is a verifier with one live session, which is all an +// error response needs: it carries no presentation to verify. +func declineClient(t *testing.T, state string) (*Client, *cache.AuthorizationContext) { + t.Helper() + ctx := context.Background() + + client, _ := CreateTestClientWithMock(t, nil) + notifyService, _ := notify.New(ctx, client.cfg, logger.NewSimple("test")) + client.notify = notifyService + + authCtx := &cache.AuthorizationContext{ + SessionID: "declining-session", + State: state, + Nonce: "nonce", + ClientID: "x509_san_dns:verifier.example.com", + Status: cache.SessionStatusAwaitingPresentation, + } + require.NoError(t, client.cacheService.AuthContext.Save(ctx, authCtx)) + return client, authCtx +} + +// The live failure: the user declines, the engine POSTs a plain +// error form to response_uri, and the verifier answered 500 "failed to +// get KID from request: failed to parse JWT header: unexpected end of +// JSON input" because it looked for a JWE header in an empty string. +// +// OpenID4VP 1.0 section 8.3.1 permits an unencrypted error response even +// for direct_post.jwt, and section 8.2's example is exactly this form. +func TestDirectPostAcceptsAPlainDecline(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + + reply, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Error: "access_denied", + ErrorDescription: "consent-declined", + State: "declined-state", + }) + require.NoError(t, err, "a declined presentation is an outcome, not a server failure") + require.NotNil(t, reply) + + stored, err := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: "declined-state"}) + require.NoError(t, err) + assert.Equal(t, cache.SessionStatusError, stored.Status, "the session must be terminal") + assert.Equal(t, "access_denied", stored.Error) + assert.Equal(t, "consent-declined", stored.ErrorDescription) +} + +// The half that actually unsticks the UI. /ui/completion reports a +// missing response code as "pending", so recording the decline is not +// enough on its own - the session has to have an outcome that is +// neither pending nor complete. +func TestDeclinedSessionHasATerminalOutcome(t *testing.T) { + ctx := context.Background() + client, authCtx := declineClient(t, "declined-state") + + before, err := client.SessionOutcome(ctx, authCtx.SessionID) + require.NoError(t, err) + require.False(t, before.Declined(), "the session has not been answered yet") + require.Empty(t, before.ResponseCode) + + _, err = client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Error: "access_denied", State: "declined-state", + }) + require.NoError(t, err) + + after, err := client.SessionOutcome(ctx, authCtx.SessionID) + require.NoError(t, err) + assert.True(t, after.Declined(), "the UI would poll pending forever") + assert.Equal(t, "access_denied", after.Error) + assert.Empty(t, after.ResponseCode, "a declined session has no response code") +} + +// state is what ties an error response to a request. OpenID4VP 1.0 +// section 14.3.2: the Verifier SHOULD check that it corresponds to a +// recent Authorization Request. +func TestDirectPostDeclineRequiresAKnownState(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct{ name, state string }{ + {"no state at all", ""}, + {"a state this verifier is not waiting for", "someone-elses-state"}, + } { + t.Run(tc.name, func(t *testing.T) { + client, authCtx := declineClient(t, "declined-state") + + _, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Error: "access_denied", State: tc.state, + }) + require.Error(t, err, "accepting this would let anyone who can reach response_uri end a session") + + shaped := helpers.NewErrorFromError(err) + assert.Equal(t, http.StatusBadRequest, shaped.HTTPStatus) + + // And the real session is untouched: silently answering 200 + // would leave it pending while reporting success. + stored, getErr := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: authCtx.State}) + require.NoError(t, getErr) + assert.Empty(t, stored.Error, "an unrelated session must not be ended by this") + }) + } +} + +// Neither a presentation nor an error is the caller's mistake, and it +// used to reach GetKID and come back as a 500. +func TestDirectPostWithNeitherPresentationNorErrorIsABadRequest(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + + _, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{}) + require.Error(t, err) + + shaped := helpers.NewErrorFromError(err) + assert.Equal(t, http.StatusBadRequest, shaped.HTTPStatus, "a malformed request is not a server failure") + assert.False(t, shaped.IsUnclassified(), "the caller gets a req_id and no idea what to fix") + assert.NotContains(t, err.Error(), "JWT header", + "the old message described a JWE the caller never claimed to send") +} + +// A wallet that CAN encrypt returns its error the way section 5.6 says +// it SHOULD - "Both successful and error responses SHOULD be returned +// using the supplied Response Mode" - so the same outcome arrives inside +// the envelope. It has no vp_token, and would otherwise be reported as a +// malformed presentation. +func TestDirectPostAcceptsAnEncryptedDecline(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + + reply, err := client.directPostError(ctx, "access_denied", "consent-declined", "declined-state") + require.NoError(t, err) + require.NotNil(t, reply) + + stored, err := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: "declined-state"}) + require.NoError(t, err) + assert.Equal(t, cache.SessionStatusError, stored.Status) +} diff --git a/internal/verifier/apiv1/handlers_ui.go b/internal/verifier/apiv1/handlers_ui.go index 47794a0d8..5c2790dfb 100644 --- a/internal/verifier/apiv1/handlers_ui.go +++ b/internal/verifier/apiv1/handlers_ui.go @@ -878,20 +878,55 @@ func (c *Client) IsActiveAuthSession(ctx context.Context, sessionID string) (act // client can retry instead of latching onto a bogus "pending" and // never reconciling the lost completion. func (c *Client) CompletedResponseCode(ctx context.Context, sessionID string) (string, error) { + outcome, err := c.SessionOutcome(ctx, sessionID) + if err != nil { + return "", err + } + return outcome.ResponseCode, nil +} + +// SessionOutcome is what a polling UI needs to stop polling: the +// response code of a completed session, or the error of a declined one. +// +// The two used to be one value, and only one of them existed. A wallet +// that answered with an Authorization Error Response - a user pressing +// "no", which OpenID4VP 1.0 section 8.5 calls access_denied - left the +// session with no response code and no other way to say so, and +// /ui/completion reports a missing response code as "pending". The UI +// then polled until the cache evicted the context, and the relying +// party never learned of a decision the user had already made. +type SessionOutcome struct { + // ResponseCode is set when the wallet presented. + ResponseCode string + // Error and ErrorDescription are set when it refused. The + // description is the wallet's own text and is shown to an operator, + // never acted on. + Error string + ErrorDescription string +} + +// Declined reports whether the wallet answered with an error. +func (o SessionOutcome) Declined() bool { return o.Error != "" } + +func (c *Client) SessionOutcome(ctx context.Context, sessionID string) (SessionOutcome, error) { if sessionID == "" { - return "", nil + return SessionOutcome{}, nil } authCtx, err := c.cacheService.AuthContext.GetByID(ctx, sessionID) if err != nil { if errors.Is(err, cache.ErrNoDocuments) { - return "", nil + return SessionOutcome{}, nil } - return "", err + return SessionOutcome{}, err } if authCtx == nil { - return "", nil + return SessionOutcome{}, nil } - return authCtx.VerifierResponseCode, nil + return SessionOutcome{ + ResponseCode: authCtx.VerifierResponseCode, + Error: authCtx.Error, + ErrorDescription: authCtx.ErrorDescription, + }, nil } // UIResumeStatus names the possible outcomes of a /ui/resume call. diff --git a/internal/verifier/apiv1/handlers_verification.go b/internal/verifier/apiv1/handlers_verification.go index d68035b9b..6c1117be4 100644 --- a/internal/verifier/apiv1/handlers_verification.go +++ b/internal/verifier/apiv1/handlers_verification.go @@ -8,11 +8,13 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "net/url" "slices" "strings" "github.com/SUNET/vc/pkg/cache" + "github.com/SUNET/vc/pkg/helpers" "github.com/SUNET/vc/pkg/jose" "github.com/SUNET/vc/pkg/mdoc" "github.com/SUNET/vc/pkg/model" @@ -75,6 +77,29 @@ func (c *Client) VerificationRequestObject(ctx context.Context, req *Verificatio type VerificationDirectPostRequest struct { Response string `json:"response" form:"response"` + + // Error, ErrorDescription and State carry an Authorization Error + // Response - the Wallet answering the request with a refusal + // instead of a presentation. + // + // OpenID4VP 1.0 section 8.3.1: "If a Wallet is unable to generate an + // encrypted response, it MAY send an error response without + // encryption as per Section 8.2." So a PLAIN error form is + // conformant even when the request asked for direct_post.jwt, and + // section 8.2's own example is exactly these three parameters + // posted as a form. + // + // Section 5.6 says both successful and error responses SHOULD use + // the supplied Response Mode, so an error may also arrive encrypted + // inside Response. Both are accepted; see directPostError. + // + // Until these existed a decline bound to an empty struct and the + // handler went looking for a JWE header in "", which failed as + // "unexpected end of JSON input" - a 500 for the one outcome that + // is not an error at all, with the session left pending forever. + Error string `json:"error,omitempty" form:"error"` + ErrorDescription string `json:"error_description,omitempty" form:"error_description"` + State string `json:"state,omitempty" form:"state"` // DCAPI marks a response returned inside a navigator.credentials.get // call and forwarded here by the verifier's own page, rather than POSTed // by a wallet reached through request_uri. The two bind their mdoc @@ -100,9 +125,92 @@ type VerificationDirectPostResponse struct { RedirectURI string `json:"redirect_uri,omitempty"` } +// directPostError records an Authorization Error Response and ends the +// session. +// +// The point is the ENDING. A declined request is an outcome the relying +// party is entitled to, and before this the only thing that happened was +// a log line: the session stayed pending until the cache evicted it, so +// a user who pressed "no" left the verifier waiting for an answer they +// had already given. +// +// state is validated against a live session (OpenID4VP 1.0 section +// 14.3.2: the Verifier SHOULD check that state corresponds to a recent +// Authorization Request). An unknown state is refused rather than +// ignored - accepting one would let anyone who can reach response_uri +// end a session they know nothing about, and silently ignoring one +// would leave the real session pending while answering 200. +// +// Nothing of the wallet's text is echoed back to the caller: the reply +// carries only the redirect, and the error reaches the operator through +// the session, not through this response body. +func (c *Client) directPostError(ctx context.Context, code, description, state string) (*VerificationDirectPostResponse, error) { + if state == "" { + return nil, helpers.NewErrorDetailsWithStatus("invalid_request", + "an error response must carry the state of the request it answers", http.StatusBadRequest) + } + + authCtx, err := c.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: state}) + if err != nil || authCtx == nil { + // Deliberately the same answer as a state that never existed: + // whether a given state is live is not something this endpoint + // should confirm to an unauthenticated caller. + c.log.Warn("direct_post error response for an unknown state", "error", code) + return nil, helpers.NewErrorDetailsWithStatus("invalid_request", + "state does not correspond to a request this verifier is waiting for", http.StatusBadRequest) + } + + if sessionExpired(authCtx) { + c.log.Info("direct_post error response for an expired session", "state", state, "error", code) + return nil, ErrSessionExpired + } + + c.log.Warn("the wallet returned an error response instead of a presentation", + "state", state, "error", code, "session_id", authCtx.SessionID) + + authCtx.Status = cache.SessionStatusError + authCtx.Error = code + authCtx.ErrorDescription = description + if err := c.cacheService.AuthContext.Save(ctx, authCtx); err != nil { + c.log.Error(err, "failed to record the wallet's error response", "state", state) + return nil, err + } + + // The same nudge a presentation sends, so a waiting UI learns now + // rather than on its next poll. The browser is told the outcome, not + // the wallet's prose. + c.notify.Submit(authCtx.SessionID, map[string]string{"error": code}) + + // OpenID4VP 1.0 section 8.2 allows the Response URI to return + // redirect_uri for an error response, and the engine reads it back + // (go-wallet-backend internal/engine/oid4vp.go submitErrorResponse), + // so a same-device decline can still land the user somewhere. + // Omitted here: where a declined flow should send the browser is a + // verifier UX decision, and inventing one would send users to a page + // nobody designed. 200 with an empty object is what the engine + // already tolerates. + return &VerificationDirectPostResponse{}, nil +} + func (c *Client) VerificationDirectPost(ctx context.Context, req *VerificationDirectPostRequest) (*VerificationDirectPostResponse, error) { c.log.Debug("Verification direct-post") + // A refusal, before anything looks for a JWE. An error response + // carries no `response`, so every step below it - the KID, the + // ephemeral key, the decryption - is looking for something that was + // never sent. + if req.Error != "" && req.Response == "" { + return c.directPostError(ctx, req.Error, req.ErrorDescription, req.State) + } + + // Neither a presentation nor an error is a malformed request, and + // it is the caller's to fix. It used to reach GetKID and come back + // as a 500. + if req.Response == "" { + return nil, helpers.NewErrorDetailsWithStatus("invalid_request", + "the response carries neither a presentation nor an error", http.StatusBadRequest) + } + // Extract KID from JWE header kid, err := req.GetKID() if err != nil { @@ -126,6 +234,23 @@ func (c *Client) VerificationDirectPost(ctx context.Context, req *VerificationDi return nil, err } + // An error response returned the way section 5.6 says it SHOULD be - + // "Both successful and error responses SHOULD be returned using the + // supplied Response Mode" - so for direct_post.jwt it arrives + // encrypted, inside the same envelope a presentation would use. + // + // Checked before the presentation parse rather than after it fails, + // because an error response legitimately has no vp_token and would + // otherwise be reported as a malformed presentation. + var encryptedError struct { + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + State string `json:"state"` + } + if err := json.Unmarshal(decryptedJWE, &encryptedError); err == nil && encryptedError.Error != "" { + return c.directPostError(ctx, encryptedError.Error, encryptedError.ErrorDescription, encryptedError.State) + } + // Parse response parameters using openid4vp vpResponse := openid4vp.VPResponse{} if err := json.Unmarshal(decryptedJWE, &vpResponse); err != nil { diff --git a/internal/verifier/httpserver/api.go b/internal/verifier/httpserver/api.go index a6747495b..a8bdee1b2 100644 --- a/internal/verifier/httpserver/api.go +++ b/internal/verifier/httpserver/api.go @@ -26,6 +26,7 @@ type Apiv1 interface { UIMetadata(ctx context.Context) (*apiv1.UIMetadataReply, error) IsActiveAuthSession(ctx context.Context, sessionID string) (active bool, expiresAt int64, err error) CompletedResponseCode(ctx context.Context, sessionID string) (string, error) + SessionOutcome(ctx context.Context, sessionID string) (apiv1.SessionOutcome, error) UIResume(ctx context.Context, sessionID string) (*apiv1.UIResumeReply, error) // OIDC Provider diff --git a/internal/verifier/httpserver/endpoints_session_preference_test.go b/internal/verifier/httpserver/endpoints_session_preference_test.go index 64179d769..7ef08b3e3 100644 --- a/internal/verifier/httpserver/endpoints_session_preference_test.go +++ b/internal/verifier/httpserver/endpoints_session_preference_test.go @@ -268,6 +268,10 @@ func (unimplementedApiv1) UIMetadata(ctx context.Context) (*apiv1.UIMetadataRepl func (unimplementedApiv1) IsActiveAuthSession(ctx context.Context, sessionID string) (bool, int64, error) { panic("IsActiveAuthSession not implemented in test") } +func (unimplementedApiv1) SessionOutcome(ctx context.Context, sessionID string) (apiv1.SessionOutcome, error) { + panic("SessionOutcome not implemented in test") +} + func (unimplementedApiv1) CompletedResponseCode(ctx context.Context, sessionID string) (string, error) { panic("CompletedResponseCode not implemented in test") } diff --git a/internal/verifier/httpserver/endpoints_ui.go b/internal/verifier/httpserver/endpoints_ui.go index 6845d1fe0..9c767f4cc 100644 --- a/internal/verifier/httpserver/endpoints_ui.go +++ b/internal/verifier/httpserver/endpoints_ui.go @@ -98,7 +98,7 @@ func (s *Service) endpointUICompletion(ctx context.Context, c *gin.Context) (any return nil, nil } - responseCode, err := s.apiv1.CompletedResponseCode(ctx, sessionID) + outcome, err := s.apiv1.SessionOutcome(ctx, sessionID) if err != nil { // A backend error (e.g. transient Mongo outage) must not be // collapsed to "pending" - the browser would stop retrying and @@ -107,11 +107,23 @@ func (s *Service) endpointUICompletion(ctx context.Context, c *gin.Context) (any span.SetStatus(codes.Error, err.Error()) return nil, err } - if responseCode == "" { + // A refusal is terminal. Reported before the response code, which a + // declined session does not have: without this the UI reads the + // missing code as "pending" and polls until the context is evicted, + // so a user who pressed "no" leaves the relying party waiting for an + // answer they already gave. + if outcome.Declined() { + return gin.H{ + "status": "declined", + "error": outcome.Error, + "error_description": outcome.ErrorDescription, + }, nil + } + if outcome.ResponseCode == "" { c.JSON(http.StatusOK, gin.H{"status": "pending"}) return nil, nil } - return gin.H{"status": "complete", "response_code": responseCode}, nil + return gin.H{"status": "complete", "response_code": outcome.ResponseCode}, nil } // endpointUIResume rebuilds the state of a verifier UI reloaded mid-flow. diff --git a/pkg/cache/authcontext_types.go b/pkg/cache/authcontext_types.go index 667bf7919..10b620b82 100644 --- a/pkg/cache/authcontext_types.go +++ b/pkg/cache/authcontext_types.go @@ -226,11 +226,27 @@ type AuthorizationContext struct { PresentationSubmission any `json:"presentation_submission,omitempty" bson:"presentation_submission,omitempty"` // OpenID4VP fields (wallet interaction) - EphemeralEncryptionKeyID string `json:"ephemeral_encryption_key_id,omitempty" bson:"ephemeral_encryption_key_id,omitempty" validate:"omitempty,max=128,printascii"` - VerifierResponseCode string `json:"verifier_response_code,omitempty" bson:"verifier_response_code,omitempty" validate:"omitempty,max=128,printascii"` - RequestObjectID string `json:"request_object_id,omitempty" bson:"request_object_id,omitempty" validate:"omitempty,max=128,printascii"` - RequestObjectNonce string `json:"request_object_nonce,omitempty" bson:"request_object_nonce,omitempty" validate:"omitempty,max=128,printascii"` - DCQLQuery *openid4vp.DCQL `json:"dcql_query,omitempty" bson:"dcql_query,omitempty"` + EphemeralEncryptionKeyID string `json:"ephemeral_encryption_key_id,omitempty" bson:"ephemeral_encryption_key_id,omitempty" validate:"omitempty,max=128,printascii"` + VerifierResponseCode string `json:"verifier_response_code,omitempty" bson:"verifier_response_code,omitempty" validate:"omitempty,max=128,printascii"` + + // Error and ErrorDescription record an Authorization Error Response + // the Wallet returned instead of a presentation - most often + // access_denied, which OpenID4VP 1.0 section 8.5 defines as covering + // a lack of End-User consent. + // + // Recorded rather than merely logged, because a declined request is + // an OUTCOME. Without it the session has no terminal state at all: + // /ui/completion reports pending until the cache evicts the context, + // so the relying party waits for an answer the user already gave. + // + // ErrorDescription is the Wallet's own text and is bounded and + // printable-checked like every other field here: it crosses from a + // wallet into a verifier's UI. + Error string `json:"error,omitempty" bson:"error,omitempty" validate:"omitempty,max=64,printascii"` + ErrorDescription string `json:"error_description,omitempty" bson:"error_description,omitempty" validate:"omitempty,max=512"` + RequestObjectID string `json:"request_object_id,omitempty" bson:"request_object_id,omitempty" validate:"omitempty,max=128,printascii"` + RequestObjectNonce string `json:"request_object_nonce,omitempty" bson:"request_object_nonce,omitempty" validate:"omitempty,max=128,printascii"` + DCQLQuery *openid4vp.DCQL `json:"dcql_query,omitempty" bson:"dcql_query,omitempty"` // ScopeQueryIDs maps a requested scope to the id of the DCQL credential // query that stands for it, for the scopes where the two differ. // From 9272f38c853dd6f99af96bcc46a980ae2ad5fbf5 Mon Sep 17 00:00:00 2001 From: Leif Johansson Date: Sat, 24 Oct 2026 13:38:46 +0200 Subject: [PATCH 2/2] fix(verifier): a decline must not undo an outcome, and must end the tab Five things the review found, each with a test that fails without it. The first outcome wins, in both directions. A late or replayed error response used to overwrite a completed session's status while its VerifierResponseCode survived, and a presentation that landed after a refusal wrote a response code onto a session whose Error and terminal status stayed put. Either way /ui/completion reported "declined" and /ui/resume "complete" for the same session - two answers, both read from stored state, with no way to tell which happened. An error response for an answered session is now accepted and recorded nowhere; a presentation for one is refused, because the wallet is handing over credentials and a 200 would say they were taken. A backend that cannot answer is not an unknown state. The lookup used `err != nil` for both, so a Mongo outage told the wallet its own request was malformed and left the real session pending with nobody retrying. ErrNoDocuments keeps the deliberate 400 - whether a state is live is not something this endpoint confirms to an unauthenticated caller - and anything else is now 503, naming no backend detail. Mutate a copy. MemoryStore.Get hands back the stored pointer itself and Save validates after the fields are assigned, so an error_description past the model's 512-byte bound failed the save with the live session already carrying the rejected value and a terminal status, while a Mongo-backed deployment kept the old one: the same request, two outcomes, decided by the cache backend. The description is wallet-controlled input, which is the kind that finds that edge. A refused context is answered, so isReusableAuthContext rejects it. Reusing one would hand a fresh interaction the state and nonce the user had just declined, and the decline recorded on it would then describe a request nobody made. The browser half: /ui/notify's refusal carries no redirect_uri, and the listener filtered every message on that field, so a decline was indistinguishable from a keep-alive and the page went on showing the QR until the deadline. The decision is now notify-decline.js, a module with no EventSource in it, and the page tears the QR screen down the way it does for an expiry. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AB3apg5kCEGNPozrhSggum --- .../apiv1/direct_post_decline_test.go | 236 ++++++++++++++++++ internal/verifier/apiv1/errors.go | 16 ++ internal/verifier/apiv1/handlers_ui.go | 26 ++ .../verifier/apiv1/handlers_verification.go | 53 +++- .../verifier/staticembed/notify-decline.js | 77 ++++++ .../staticembed/presentation-definition.html | 17 +- .../staticembed/presentation-definition.js | 66 ++++- .../staticembed/tests/notify-decline.test.js | 88 +++++++ 8 files changed, 568 insertions(+), 11 deletions(-) create mode 100644 internal/verifier/staticembed/notify-decline.js create mode 100644 internal/verifier/staticembed/tests/notify-decline.test.js diff --git a/internal/verifier/apiv1/direct_post_decline_test.go b/internal/verifier/apiv1/direct_post_decline_test.go index 270cda56c..c77290aa0 100644 --- a/internal/verifier/apiv1/direct_post_decline_test.go +++ b/internal/verifier/apiv1/direct_post_decline_test.go @@ -2,8 +2,11 @@ package apiv1 import ( "context" + "errors" "net/http" + "strings" "testing" + "time" "github.com/SUNET/vc/internal/verifier/notify" "github.com/SUNET/vc/pkg/cache" @@ -148,3 +151,236 @@ func TestDirectPostAcceptsAnEncryptedDecline(t *testing.T) { require.NoError(t, err) assert.Equal(t, cache.SessionStatusError, stored.Status) } + +// unreachableAuthContextStore answers every lookup the way a store whose +// backend is down does: with an operational error, not a miss. The +// distinction is the whole point - ErrNoDocuments means "no such state", +// anything else means "I cannot tell you". +type unreachableAuthContextStore struct { + cache.AuthContextStore + err error +} + +func (s unreachableAuthContextStore) Get(context.Context, *cache.AuthorizationContext) (*cache.AuthorizationContext, error) { + return nil, s.err +} + +// A wallet's error response must not be able to undo an outcome the +// session already has. Without the guard a late or replayed error +// overwrote a completed session's status while its VerifierResponseCode +// survived, so /ui/completion said "declined" and /ui/resume said +// "complete" for the same session - two answers, both read from stored +// state, with no way to tell which actually happened. +func TestDirectPostDeclineDoesNotOverwriteAnOutcome(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct { + name string + arrange func(*cache.AuthorizationContext) + status cache.SessionStatus + wantErr string + }{ + { + name: "a session that has already completed", + arrange: func(a *cache.AuthorizationContext) { + a.Status = cache.SessionStatusCompleted + a.VerifierResponseCode = "response-code" + }, + status: cache.SessionStatusCompleted, + wantErr: "", + }, + { + name: "a session that has already been declined", + arrange: func(a *cache.AuthorizationContext) { + a.Status = cache.SessionStatusError + a.Error = "access_denied" + a.ErrorDescription = "the first refusal" + }, + status: cache.SessionStatusError, + wantErr: "access_denied", + }, + } { + t.Run(tc.name, func(t *testing.T) { + client, authCtx := declineClient(t, "declined-state") + tc.arrange(authCtx) + require.NoError(t, client.cacheService.AuthContext.Update(ctx, authCtx)) + + // Answered, not refused: the wallet is told its POST was + // received. Returning an error would invite it to retry + // against a session that is over. + reply, err := client.directPostError(ctx, "server_error", "a later, losing refusal", "declined-state") + require.NoError(t, err) + require.NotNil(t, reply) + + stored, err := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: "declined-state"}) + require.NoError(t, err) + assert.Equal(t, tc.status, stored.Status, "the first outcome must stand") + assert.Equal(t, tc.wantErr, stored.Error, "a later error response must not rewrite the recorded one") + assert.NotEqual(t, "a later, losing refusal", stored.ErrorDescription) + }) + } +} + +// A backend that cannot answer is not an unknown state. Answering 400 +// here tells a wallet its own request was malformed during an outage, +// and the real session stays pending with nobody retrying. +func TestDirectPostDeclineOnAnUnreachableBackendIsNotABadRequest(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + client.cacheService.AuthContext = unreachableAuthContextStore{ + AuthContextStore: client.cacheService.AuthContext, + err: errors.New("connection refused"), + } + + _, err := client.directPostError(ctx, "access_denied", "consent-declined", "declined-state") + require.Error(t, err) + + shaped := helpers.NewErrorFromError(err) + assert.Equal(t, http.StatusServiceUnavailable, shaped.HTTPStatus, + "an outage must be retryable, not reported as the caller's mistake") + assert.NotEqual(t, http.StatusBadRequest, shaped.HTTPStatus) + assert.NotContains(t, err.Error(), "connection refused", + "the wallet learns the verifier is unavailable, not what its backend is") +} + +// The description is wallet-controlled input, and the model bounds it at +// 512 bytes. Save validates AFTER the fields are assigned, so mutating +// the stored document in place left a memory-backed deployment with the +// rejected value and a terminal status on the live session while a +// Mongo-backed one kept the old one - the same request, two outcomes, +// decided by the cache backend. +func TestDirectPostDeclineWithAnOversizedDescriptionLeavesTheSessionIntact(t *testing.T) { + ctx := context.Background() + client, _ := declineClient(t, "declined-state") + + _, err := client.directPostError(ctx, "access_denied", strings.Repeat("x", 513), "declined-state") + require.Error(t, err, "a value the model refuses to store must not be reported as stored") + + stored, getErr := client.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: "declined-state"}) + require.NoError(t, getErr) + assert.Equal(t, cache.SessionStatusAwaitingPresentation, stored.Status, + "a failed save must not leave the session terminal") + assert.Empty(t, stored.Error, "a failed save must not leave the session carrying a refusal") + assert.Empty(t, stored.ErrorDescription, "the rejected value must not be readable from the live session") +} + +// /ui/resume is what a reloaded tab asks. Before this it answered +// "pending" with the original QR for a request the user had already +// declined, so the two endpoints disagreed: /ui/completion said declined +// while /ui/resume offered the code again. +func TestUIResumeReportsADeclinedSession(t *testing.T) { + ctx := context.Background() + client, authCtx := declineClient(t, "declined-state") + + _, err := client.directPostError(ctx, "access_denied", "consent-declined", "declined-state") + require.NoError(t, err) + + reply, err := client.UIResume(ctx, authCtx.SessionID) + require.NoError(t, err) + require.NotNil(t, reply) + assert.Equal(t, UIResumeDeclined, reply.Status, "a reload must not rebuild the QR for a refused request") + assert.Equal(t, "access_denied", reply.Error) + assert.Equal(t, "consent-declined", reply.ErrorDescription) + assert.Empty(t, reply.AuthorizationRequest, "there is nothing left to scan") + assert.Empty(t, reply.ResponseCode, "a declined session has no response code") +} + +// A declined context has been answered. Handing it to a fresh +// interaction would reuse the state and nonce the user just refused, and +// the decline recorded on it would then describe a request nobody made. +func TestDeclinedSessionIsNotReusedByANewInteraction(t *testing.T) { + ctx := t.Context() + client := newSigningTestClient(t) + // A real interaction is needed here (not declineClient's bare + // context) because the reuse path runs through UIInteraction, and + // the decline it answers broadcasts on the notify service. + notifyService, err := notify.New(ctx, client.cfg, logger.NewSimple("test")) + require.NoError(t, err) + client.notify = notifyService + + first, err := client.UIInteraction(ctx, &UIInteractionRequest{DCQLQuery: createTestDCQLForVP(t)}) + require.NoError(t, err) + require.NotEmpty(t, first.SessionID) + + authCtx, err := client.cacheService.AuthContext.GetByID(ctx, first.SessionID) + require.NoError(t, err) + require.NotEmpty(t, authCtx.State) + require.True(t, isReusableAuthContext(authCtx), "the premise: it was reusable before the refusal") + + _, err = client.directPostError(ctx, "access_denied", "consent-declined", authCtx.State) + require.NoError(t, err) + + declined, err := client.cacheService.AuthContext.GetByID(ctx, first.SessionID) + require.NoError(t, err) + assert.False(t, isReusableAuthContext(declined), "a refused session is answered, not in flight") + + second, err := client.UIInteraction(ctx, &UIInteractionRequest{ + DCQLQuery: createTestDCQLForVP(t), + SessionID: first.SessionID, + }) + require.NoError(t, err) + assert.NotEqual(t, first.SessionID, second.SessionID, + "a new request after a refusal must get its own session") + + // And the refusal still describes the request it was given, not the + // new one. + stillDeclined, err := client.cacheService.AuthContext.GetByID(ctx, first.SessionID) + require.NoError(t, err) + assert.Equal(t, "access_denied", stillDeclined.Error) +} + +// The other direction of "first outcome wins". The deadline was the +// only thing checked before a presentation was accepted, so a wallet +// that posted credentials after the user had declined - a refusal and a +// presentation racing, or a replay - wrote a VerifierResponseCode onto a +// session whose Error and terminal status stayed put. /ui/completion +// then reported "declined" and /ui/resume "complete" for one session. +func TestVerificationDirectPostRefusesAnAlreadyAnsweredSession(t *testing.T) { + ctx := t.Context() + + answeredSession := func(t *testing.T, decline bool) (*Client, string, string) { + t.Helper() + client, _ := CreateTestClientWithMock(t, nil) + notifyService, err := notify.New(ctx, client.cfg, logger.NewSimple("test")) + require.NoError(t, err) + client.notify = notifyService + + sessionID, state := "answered-verification-session", "answered-state" + session := pendingSession(sessionID, state, time.Now().Add(10*time.Minute).Unix()) + session.EphemeralEncryptionKeyID = sessionID + require.NoError(t, client.cacheService.AuthContext.Save(ctx, session)) + + if decline { + _, err = client.directPostError(ctx, "access_denied", "consent-declined", state) + require.NoError(t, err) + } + return client, sessionID, state + } + + // The premise, so the test below is about the guard and not about a + // verifier that refuses every presentation: the same session, the + // same deadline, is not refused as answered while it is still live. + t.Run("a live session is not refused as answered", func(t *testing.T) { + client, sessionID, state := answeredSession(t, false) + _, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Response: encryptedVPResponse(t, client, sessionID, state), + }) + assert.NotErrorIs(t, err, ErrSessionAlreadyAnswered) + }) + + t.Run("a declined session refuses the presentation that follows", func(t *testing.T) { + client, sessionID, state := answeredSession(t, true) + + _, err := client.VerificationDirectPost(ctx, &VerificationDirectPostRequest{ + Response: encryptedVPResponse(t, client, sessionID, state), + }) + require.Error(t, err) + assert.ErrorIs(t, err, ErrSessionAlreadyAnswered) + assert.NotErrorIs(t, err, ErrSessionExpired, "the deadline has not passed; the session was answered") + + stored, err := client.cacheService.AuthContext.GetByID(ctx, sessionID) + require.NoError(t, err) + assert.Equal(t, "access_denied", stored.Error, "the refusal must still be what this session says happened") + assert.Empty(t, stored.VerifierResponseCode, "accepting it would have made the two UI endpoints disagree") + }) +} diff --git a/internal/verifier/apiv1/errors.go b/internal/verifier/apiv1/errors.go index 810334316..c84755adf 100644 --- a/internal/verifier/apiv1/errors.go +++ b/internal/verifier/apiv1/errors.go @@ -152,6 +152,22 @@ var ( HTTPStatus: http.StatusBadRequest, } + // ErrSessionAlreadyAnswered is a presentation for a session that + // already reached an outcome - most plainly, one the user declined + // a moment earlier, with the wallet's POST still in flight. + // + // Separate from ErrSessionExpired because it is a different fact + // and the wallet should be able to tell them apart: a deadline + // passed, versus an answer this session already has. Accepting it + // would write a response code onto a session whose Error and + // terminal status stay put, so /ui/completion would report + // "declined" and /ui/resume "complete" for the same session. + ErrSessionAlreadyAnswered = &OAuthError{ + ErrorCode: ErrCodeInvalidGrant, + ErrorDescription: "The session has already been answered", + HTTPStatus: http.StatusBadRequest, + } + ErrInvalidVP = &OAuthError{ ErrorCode: ErrCodeInvalidRequest, ErrorDescription: "Invalid verifiable presentation", diff --git a/internal/verifier/apiv1/handlers_ui.go b/internal/verifier/apiv1/handlers_ui.go index 5c2790dfb..a98182542 100644 --- a/internal/verifier/apiv1/handlers_ui.go +++ b/internal/verifier/apiv1/handlers_ui.go @@ -807,6 +807,13 @@ func isReusableAuthContext(authCtx *cache.AuthorizationContext) bool { if authCtx.VerifierResponseCode != "" { return false } + // A refused session has been answered too. Reusing one would hand a + // fresh interaction the state and nonce the user already declined, + // and the decline recorded on it would then describe a request + // nobody made. + if authSessionIsTerminal(authCtx) { + return false + } if authCtx.ExpiresAt != 0 && authCtx.ExpiresAt <= time.Now().Unix() { return false } @@ -945,6 +952,9 @@ const ( // UIResumeExpired means the auth context exists but its request object // has been evicted, so the QR / request_uri is no longer resolvable. UIResumeExpired UIResumeStatus = "expired" + // UIResumeDeclined means the wallet refused: the user said no, and + // there is nothing left to wait for. + UIResumeDeclined UIResumeStatus = "declined" ) // UIResumeReply carries one of three alternatives selected by Status: @@ -965,6 +975,10 @@ type UIResumeReply struct { // Complete-only field. ResponseCode string `json:"response_code,omitempty"` + + // Declined-only fields. + Error string `json:"error,omitempty"` + ErrorDescription string `json:"error_description,omitempty"` } // UIResume reports whether the wallet interaction named by sessionID is @@ -993,6 +1007,18 @@ func (c *Client) UIResume(ctx context.Context, sessionID string) (*UIResumeReply ResponseCode: authCtx.VerifierResponseCode, }, nil } + // A refusal is an outcome, so a reload must not rebuild the QR for + // it. Without this a reloaded tab resumed "pending" with the + // original QR for a request the user had already declined, and the + // two endpoints disagreed: /ui/completion said declined while + // /ui/resume offered the code again. + if authCtx.Error != "" { + return &UIResumeReply{ + Status: UIResumeDeclined, + Error: authCtx.Error, + ErrorDescription: authCtx.ErrorDescription, + }, nil + } // Past the presentation deadline there is nothing to resume. Checked // after the completed case, so a finished session still hands back its // response code, and before the request object, which outlives the diff --git a/internal/verifier/apiv1/handlers_verification.go b/internal/verifier/apiv1/handlers_verification.go index 6c1117be4..05e1f03a3 100644 --- a/internal/verifier/apiv1/handlers_verification.go +++ b/internal/verifier/apiv1/handlers_verification.go @@ -151,13 +151,23 @@ func (c *Client) directPostError(ctx context.Context, code, description, state s } authCtx, err := c.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: state}) - if err != nil || authCtx == nil { + switch { + case errors.Is(err, cache.ErrNoDocuments) || (err == nil && authCtx == nil): // Deliberately the same answer as a state that never existed: // whether a given state is live is not something this endpoint // should confirm to an unauthenticated caller. c.log.Warn("direct_post error response for an unknown state", "error", code) return nil, helpers.NewErrorDetailsWithStatus("invalid_request", "state does not correspond to a request this verifier is waiting for", http.StatusBadRequest) + case err != nil: + // A backend that cannot answer is NOT an unknown state. Saying + // 400 here would tell a wallet its own request was malformed + // during a Mongo outage, and the real session would stay + // pending with nobody retrying. 503 is the honest answer and + // names no backend detail. + c.log.Error(err, "could not look up the session for a direct_post error response", "error", code) + return nil, helpers.NewErrorDetailsWithStatus("temporarily_unavailable", + "the session could not be looked up; retry", http.StatusServiceUnavailable) } if sessionExpired(authCtx) { @@ -165,16 +175,36 @@ func (c *Client) directPostError(ctx context.Context, code, description, state s return nil, ErrSessionExpired } + // First outcome wins. Without this a late error overwrites a + // completed session while its VerifierResponseCode survives, so + // /ui/completion reports "declined" and /ui/resume reports + // "complete" for the same session - two answers, both from stored + // state, with no way to tell which happened. + if authSessionIsTerminal(authCtx) { + c.log.Info("direct_post error response for a session that already reached an outcome", + "state", state, "error", code, "status", authCtx.Status) + return &VerificationDirectPostResponse{}, nil + } + c.log.Warn("the wallet returned an error response instead of a presentation", "state", state, "error", code, "session_id", authCtx.SessionID) - authCtx.Status = cache.SessionStatusError - authCtx.Error = code - authCtx.ErrorDescription = description - if err := c.cacheService.AuthContext.Save(ctx, authCtx); err != nil { + // Mutate a COPY. MemoryStore.Get hands back the stored pointer + // itself, and Save validates after these assignments - so a wallet + // sending an error_description past the model's 512-byte bound + // would fail the save with the live session already carrying the + // rejected value and a terminal status, while a Mongo-backed + // deployment kept the old one. The description is wallet-controlled + // input, which is exactly the kind that finds that edge. + updated := *authCtx + updated.Status = cache.SessionStatusError + updated.Error = code + updated.ErrorDescription = description + if err := c.cacheService.AuthContext.Save(ctx, &updated); err != nil { c.log.Error(err, "failed to record the wallet's error response", "state", state) return nil, err } + authCtx = &updated // The same nudge a presentation sends, so a waiting UI learns now // rather than on its next poll. The browser is told the outcome, not @@ -276,6 +306,19 @@ func (c *Client) VerificationDirectPost(ctx context.Context, req *VerificationDi return nil, ErrSessionExpired } + // The other direction of "first outcome wins". A presentation that + // lands after the session was answered - the user declined and the + // wallet sent anyway, or a response is replayed - would otherwise + // write a VerifierResponseCode while Error and the terminal status + // stay put, and the two UI endpoints would then disagree about what + // happened. Refused rather than ignored: the wallet is handing over + // credentials, and a 200 would say they were accepted. + if authSessionIsTerminal(authCtx) { + c.log.Info("Verification direct post for a session that already reached an outcome", + "state", vpResponse.State, "status", authCtx.Status) + return nil, ErrSessionAlreadyAnswered + } + // Generate response code responseCode := uuid.NewString() callbackURL, err := url.JoinPath(c.cfg.Verifier.PublicURL, "/verification/callback") diff --git a/internal/verifier/staticembed/notify-decline.js b/internal/verifier/staticembed/notify-decline.js new file mode 100644 index 000000000..df2cf3bbb --- /dev/null +++ b/internal/verifier/staticembed/notify-decline.js @@ -0,0 +1,77 @@ +/** + * Reading a refusal out of an /ui/notify message. + * + * The notify stream carries two kinds of terminal news for a waiting + * page: a completion, which carries a redirect_uri, and a refusal, which + * does not. The listener used to filter on `redirect_uri` alone, so a + * refusal was dropped and the page waited for a wallet that had already + * answered. This is the half that decides, kept separate so it can be + * tested without an EventSource. + */ + +/** A refusal with no error code of its own. Section 8.5 of OpenID4VP + * 1.0 names access_denied for a user who did not consent, and that is + * what a wallet with nothing more specific to say sends. */ +const DEFAULT_DECLINE_ERROR = "access_denied"; + +/** + * The error code of a refusal carried by one SSE message, or "" when the + * message is not a refusal. + * + * A completion is never a refusal, even if some field of it happens to + * contain the substring "error": the presence of redirect_uri settles + * it, and that check comes first. + * + * The payload SHOULD be JSON. It is parsed leniently because this is the + * last hop of a flow the user is watching: a message that says "error" + * in a shape we did not predict is still better treated as the refusal + * it plainly is than left to time out. + * + * @param {unknown} data One SSE message's data field. + * @returns {string} The error code, or "" if this message is not a refusal. + */ +export function declineFromNotifyMessage(data) { + if (!data || typeof data !== "string") return ""; + if (data.includes("redirect_uri")) return ""; + if (!data.includes("error")) return ""; + + try { + const parsed = JSON.parse(data); + if (parsed && typeof parsed === "object") { + if (typeof parsed.error === "string" && parsed.error) return parsed.error; + // A well-formed object that named no code is still a + // refusal - "error_description only" is a shape wallets do + // send - and treating it as noise would hang the page. + if (typeof parsed.error_description === "string" && parsed.error_description) { + return DEFAULT_DECLINE_ERROR; + } + return ""; + } + } catch { + // Not JSON. Fall through to the textual form below. + } + + const match = data.match(/error[=:]\s*["']?([^"'\s,}]+)/); + if (match && match[1]) return match[1]; + return ""; +} + +/** + * The description a refusal carried, or "" when it carried none. Shown + * as supporting text and never acted on: it is the wallet's own prose. + * + * @param {unknown} data One SSE message's data field. + * @returns {string} + */ +export function declineDescriptionFromNotifyMessage(data) { + if (!data || typeof data !== "string") return ""; + try { + const parsed = JSON.parse(data); + if (parsed && typeof parsed === "object" && typeof parsed.error_description === "string") { + return parsed.error_description; + } + } catch { + return ""; + } + return ""; +} diff --git a/internal/verifier/staticembed/presentation-definition.html b/internal/verifier/staticembed/presentation-definition.html index dbd4a72b7..10d95935d 100644 --- a/internal/verifier/staticembed/presentation-definition.html +++ b/internal/verifier/staticembed/presentation-definition.html @@ -40,7 +40,7 @@

+ + -