From 5fdddb2c69f5ffb3767f2e89524422eb5d2364a1 Mon Sep 17 00:00:00 2001 From: Leif Johansson Date: Wed, 21 Oct 2026 21:12:09 +0200 Subject: [PATCH] fix(httphelpers): a wrapped classified error keeps its status StatusCode decided the status with a type switch on *helpers.Error, which does not unwrap, while NewErrorFromError - which builds the response BODY - does. So a classified error returned through %w came out with the right code in the body and a status that did not come from the error at all. Not simply a 500 either. The wrapped error fell past the type switch, past the sentinel errors.Is checks, and into inferStatusFromErrorString, which keyword-matches the concatenated message - so the status was decided by whatever prose the wrapper happened to contain. Measured: refusal returned directly -> 400 fmt.Errorf("scope nym: %w", refusal) -> 500 fmt.Errorf("bad token: %w", refusal) -> 401 fmt.Errorf("missing field: %w", refusal) -> 404 The 401 is the one that does damage: a client acting on it re-authenticates in response to a request it should simply not repeat. This is the rule the four existing errors.Is lines were approximating. Each was added when one particular wrapped SENTINEL surfaced with the wrong status; an ad-hoc *helpers.Error has no sentinel to check against, so it was never covered. The unwrap states the rule once and leaves those four answering exactly as they did. No endpoint's status changes today. Every current producer of an ad-hoc *helpers.Error returns it bare to RegEndpoint - surveyed in the PR, file by file - so the defect was latent, and bites the first time a path wraps one. That is what the zkp_req presentation work hit: its refusals are produced several frames below the handler and travel up through a dispatcher. Tests pin the wrapper's WORDING as the thing that must not matter, and that unclassified errors are still 500 - the fix must not turn every unexpected failure into a client error. Proved non-vacuous by removing the unwrap and watching the wording cases fail. Refs #841 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AB3apg5kCEGNPozrhSggum --- CHANGELOG.md | 1 + pkg/httphelpers/status.go | 21 ++++++ pkg/httphelpers/status_unwrap_test.go | 93 +++++++++++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 pkg/httphelpers/status_unwrap_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 416e52e8b..79e6c93ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ ### Fixed +- **A wrapped classified error now keeps its HTTP status** (#841): `httphelpers.StatusCode` decided the status with a type switch that does not unwrap, while the response body (`NewErrorFromError`) does unwrap. A `*helpers.Error` returned through `%w` therefore carried the right code in the body and a status taken from `inferStatusFromErrorString`'s keyword match over the concatenated message - so a 400 refusal read as 401 when the wrapper said "token" and 404 when it said "missing". No endpoint's status changes today: every existing producer returns its classified error bare, so the defect was latent. It bites the first path that wraps one. - **W3C presentation verification picked a proof by accident.** A verifiable presentation holds two Data Integrity proofs - the holder's, over the presentation, and the embedded credential's, from the issuer - and both diff --git a/pkg/httphelpers/status.go b/pkg/httphelpers/status.go index 3bb0b8da1..a261d71d4 100644 --- a/pkg/httphelpers/status.go +++ b/pkg/httphelpers/status.go @@ -61,6 +61,27 @@ func StatusCode(ctx context.Context, err error) int { } } + // A *helpers.Error reached through %w, which the type switch above + // cannot see. + // + // NewErrorFromError already unwraps - it uses errors.AsType - so the + // RESPONSE BODY of a wrapped classified error is correct today while + // the status is decided down in inferStatusFromErrorString, from + // whatever words happen to appear in the wrapper's text. That is not + // a 500 by default so much as a status chosen by prose: a 400 refusal + // reads as 401 when the wrapper says "token" and as 404 when it says + // "missing". A client acting on 401 re-authenticates in response to a + // request it should simply not repeat. + // + // Placed after the sentinel checks below it would be equivalent, and + // before them is clearer: this is the general case those four + // errors.Is lines are specific instances of. They were each added + // when a particular wrapped sentinel surfaced wrong, one at a time; + // this is the rule they were approximating. + if classified, ok := errors.AsType[*helpers.Error](err); ok { + return StatusCode(ctx, classified) + } + // Check if it's wrapped in one of our known errors if errors.Is(err, helpers.ErrNoDocumentFound) || errors.Is(err, helpers.ErrNoIdentityFound) { return http.StatusNotFound diff --git a/pkg/httphelpers/status_unwrap_test.go b/pkg/httphelpers/status_unwrap_test.go new file mode 100644 index 000000000..3a3bdfc19 --- /dev/null +++ b/pkg/httphelpers/status_unwrap_test.go @@ -0,0 +1,93 @@ +package httphelpers + +import ( + "context" + "errors" + "fmt" + "net/http" + "testing" + + "github.com/SUNET/vc/pkg/helpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A classified error reached through %w must keep its status. +// +// The wrapper's WORDING is the thing under test. Before this unwrapped, +// a wrapped *helpers.Error fell past the type switch and all the way to +// inferStatusFromErrorString, which keyword-matches the concatenated +// message - so the status was decided by whatever prose the wrapper +// happened to contain, not by the error. A 400 refusal read as 401 when +// the wrapper said "token", and a client acting on 401 re-authenticates +// in response to a request it should simply not repeat. +func TestStatusCodeUnwrapsAClassifiedError(t *testing.T) { + refusal := helpers.NewErrorDetailsWithStatus("zkp_req_issuer_key_set_size", "two keys, one expected", http.StatusBadRequest) + + for _, tc := range []struct { + name string + err error + }{ + {"returned directly", refusal}, + {"wrapped with no keyword in the wrapper", fmt.Errorf("scope nym: %w", refusal)}, + {"wrapped where the wrapper says invalid", fmt.Errorf("scope nym: invalid presentation: %w", refusal)}, + {"wrapped where the wrapper says token", fmt.Errorf("scope nym: bad token: %w", refusal)}, + {"wrapped where the wrapper says missing", fmt.Errorf("scope nym: missing field: %w", refusal)}, + {"wrapped twice", fmt.Errorf("outer: %w", fmt.Errorf("inner: %w", refusal))}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, http.StatusBadRequest, StatusCode(context.Background(), tc.err), + "the status must come from the error, not from the wrapper's wording") + }) + } +} + +// Not only 400: whatever status the classified error carries survives. +func TestStatusCodeUnwrapsEveryClassifiedStatus(t *testing.T) { + for _, status := range []int{ + http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, + http.StatusNotFound, http.StatusConflict, http.StatusInternalServerError, + } { + classified := helpers.NewErrorDetailsWithStatus("some_code", "detail", status) + assert.Equal(t, status, StatusCode(context.Background(), fmt.Errorf("context: %w", classified))) + } +} + +// A classified error with no explicit status still infers from its TITLE +// when wrapped, the same way it does when returned directly - the title +// is a shape somebody chose, unlike the wrapper's prose. +func TestStatusCodeInfersFromTheTitleWhenWrapped(t *testing.T) { + noStatus := helpers.NewErrorDetails("document_not_found", "nope") + assert.Equal(t, + StatusCode(context.Background(), noStatus), + StatusCode(context.Background(), fmt.Errorf("loading the document: %w", noStatus)), + "wrapping must not change the answer") +} + +// An error nothing recognises is still a 500. The fix must not turn +// every unexpected failure into a client error. +func TestStatusCodeLeavesUnclassifiedErrorsAlone(t *testing.T) { + assert.Equal(t, http.StatusInternalServerError, + StatusCode(context.Background(), errors.New("something went wrong in a way nobody modelled"))) + assert.Equal(t, http.StatusInternalServerError, + StatusCode(context.Background(), fmt.Errorf("while doing a thing: %w", errors.New("nothing recognisable")))) +} + +// The four sentinel errors.Is checks this generalises are instances of +// the same rule, and must keep answering as they did. +func TestStatusCodeKeepsTheSentinelAnswers(t *testing.T) { + for _, tc := range []struct { + err error + want int + }{ + {helpers.ErrNoDocumentFound, http.StatusNotFound}, + {helpers.ErrNoIdentityFound, http.StatusNotFound}, + {helpers.ErrDocumentAlreadyExists, http.StatusConflict}, + {helpers.ErrDuplicateKey, http.StatusConflict}, + {helpers.ErrInternalServerError, http.StatusInternalServerError}, + } { + require.NotNil(t, tc.err) + assert.Equal(t, tc.want, StatusCode(context.Background(), tc.err), "%v direct", tc.err) + assert.Equal(t, tc.want, StatusCode(context.Background(), fmt.Errorf("context: %w", tc.err)), "%v wrapped", tc.err) + } +}