Repository navigation
Conversation
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 SUNET#846
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AB3apg5kCEGNPozrhSggum
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Browser handling, terminal-state protection, and persistence error handling remain incomplete.
5 open issues
What changed in this PR
Addresses #846 by recording wallet refusals as verifier session outcomes instead of attempting to parse them as presentations.
Changes:
- Accepts plain and encrypted authorization error responses and rejects empty requests with HTTP 400.
- Persists decline details and exposes a
declinedcompletion status. - Adds regression tests and updates the HTTP API interface.
| File | Description |
|---|---|
| pkg/cache/authcontext_types.go | Adds persisted error details. |
| internal/verifier/httpserver/endpoints_ui.go | Returns declined completion outcomes. |
| internal/verifier/httpserver/endpoints_session_preference_test.go | Updates the test API stub. |
| internal/verifier/httpserver/api.go | Exposes session outcome lookup. |
| internal/verifier/apiv1/handlers_verification.go | Handles wallet errors and records declines. |
| internal/verifier/apiv1/handlers_ui.go | Adds session outcome retrieval. |
| internal/verifier/apiv1/direct_post_decline_test.go | Adds decline and malformed-request tests. |
Review effort: Balanced
Comment on lines
+925
to
+929
| return SessionOutcome{ | ||
| ResponseCode: authCtx.VerifierResponseCode, | ||
| Error: authCtx.Error, | ||
| ErrorDescription: authCtx.ErrorDescription, | ||
| }, nil |
| } | ||
|
|
||
| authCtx, err := c.cacheService.AuthContext.Get(ctx, &cache.AuthorizationContext{State: state}) | ||
| if err != nil || authCtx == nil { |
Comment on lines
+171
to
+174
| authCtx.Status = cache.SessionStatusError | ||
| authCtx.Error = code | ||
| authCtx.ErrorDescription = description | ||
| if err := c.cacheService.AuthContext.Save(ctx, authCtx); err != nil { |
Comment on lines
+171
to
+174
| authCtx.Status = cache.SessionStatusError | ||
| authCtx.Error = code | ||
| authCtx.ErrorDescription = description | ||
| if err := c.cacheService.AuthContext.Save(ctx, authCtx); err != nil { |
Comment on lines
+115
to
+119
| if outcome.Declined() { | ||
| return gin.H{ | ||
| "status": "declined", | ||
| "error": outcome.Error, | ||
| "error_description": outcome.ErrorDescription, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #846.
A user who declines gets HTTP 500 and a session that never ends.
What happened
VerificationDirectPostRequestdeclared onlyresponse, so a Wallet's Authorization Error Response bound to an empty struct. The handler then calledGetKID()unconditionally and went looking for a JWE header in"":It was returned bare, so
NewErrorFromErrorcould not classify it — a 500 for the one outcome that is not a failure at all.Behind that, nothing recorded the refusal.
/ui/completionis driven byCompletedResponseCode, and a session with no response code reportspending: 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 whySessionOutcomereplaces a bare response code and/ui/completiongaineddeclined.The plain form is conformant, not a wallet quirk
The engine posts
error/error_description/stateas a form with no JWE (go-wallet-backend internal/engine/oid4vp.go:1906-1919), even though the request asked fordirect_post.jwt. OpenID4VP 1.0:access_deniedcovers lack of End-User consent.Both are accepted. The encrypted one is checked before the presentation parse, because an error legitimately has no
vp_tokenand would otherwise be reported as a malformed presentation.Security
stateis validated against a live session (§14.3.2: the Verifier SHOULD check thatstatecorresponds to a recent Authorization Request). An unknownstateis refused rather than ignored: accepting one would let anyone who can reachresponse_uriend a session they know nothing about, and ignoring one would answer 200 while leaving the real session pending. An absent and an unknownstateget the same answer, because which states are live is not something this endpoint should confirm to an unauthenticated caller. Nothing of the wallet's text is echoed to the 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
GetKIDand come back as the same 500.Deliberately not done
No
redirect_uriis returned. §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.200 {}is what the engine already tolerates.Testing
Every guarantee proved non-vacuous by restoring the defect:
stateacceptedDeclined()always falsego vet ./...clean;./internal/verifier/...,./pkg/openid4vp/...green. The only failures in a full run areTestRedisCache_*, which fail identically on a pristinemaincheckout (testcontainers cannot start Redis here) — verified, not assumed.Draft: the
statehandling and the choice to omitredirect_uriare the parts worth a second opinion.🤖 Generated with Claude Code
https://claude.ai/code/session_01AB3apg5kCEGNPozrhSggum