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) + } +}