Repository navigation
Conversation
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 SUNET#841
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.
🟢 Approval recommended
The narrow change preserves existing status mapping, has focused regression coverage, and has no identified blocking issues.
No open issues 🎉
What changed in this PR
Fixes #841 by preserving classified HTTP error statuses through wrapping instead of deriving them from wrapper text.
Changes:
- Unwraps
*helpers.Errorand reuses existing status mapping. - Adds regression coverage for wrapper wording, classified statuses, and sentinel errors.
- Documents the fix in the changelog.
| File | Description |
|---|---|
| pkg/httphelpers/status.go | Preserves wrapped classified error statuses. |
| pkg/httphelpers/status_unwrap_test.go | Tests wrapping and existing fallback behavior. |
| CHANGELOG.md | Records the fix and its current impact. |
Review effort: Balanced
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 #841.
StatusCodedecided the HTTP status with a type switch on*helpers.Error, which does not unwrap.NewErrorFromError, which builds the response body, does unwrap. So a classified error returned throughfmt.Errorf("…: %w", err)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
The wrapped error fell past the type switch, past the sentinel
errors.Ischecks, and intoinferStatusFromErrorString, which keyword-matches the concatenated message. The status was therefore decided by whatever prose the wrapper happened to contain. Measured before and after, same inputs:refusalreturned directlyfmt.Errorf("scope nym: %w", refusal)fmt.Errorf("invalid presentation: %w", refusal)fmt.Errorf("bad token: %w", refusal)fmt.Errorf("missing field: %w", refusal)notFoundreturned directlyfmt.Errorf("loading: %w", notFound)The
401is the one that does damage: a client acting on it re-authenticates in response to a request it should simply not repeat.What changes in production: nothing, today
This is the part reviewers should check hardest, so here is the survey, file by file. Every current producer of an ad-hoc
*helpers.Errorreturns it bare toRegEndpoint, so no existing endpoint's status changes:internal/apigw/apiv1/handlers_verifier_presentation.gohandlers_verifier.go:646,:673internal/apigw/apiv1/handlers_issuance_session.go:180internal/apigw/apiv1/handlers_datastore.go(requirePreAuthScope):594internal/apigw/httpserver/endpoints.go:177internal/apigw/httpserver/endpoints_oauth.go:740pkg/httphelpers/{server,rendering,middleware}.goStatusCodeSo the defect is latent. It bites the first time a path wraps a classified error — which is what the
zkp_reqpresentation work hit: those refusals are produced several frames below the handler and travel up through a dispatcher.Nothing here should "stay 500 deliberately". The one shape that might look like it — an internal failure wrapped around a classified cause — does not arise: a handler that wants a 500 either returns an unclassified error (still 500, covered by a test) or constructs
helpers.ErrInternalServerError, whose status this preserves. The fix only changes cases where a classified error was already going to be rendered with its own code in the body, and makes the status agree with it.Why the existing
errors.Islines did not cover itStatusCodealready had four expliciterrors.Is(err, helpers.Err…)checks for wrapped sentinels, each added when one particular wrapped sentinel surfaced wrong. They are instances of the general rule the function never stated. An ad-hoc*helpers.Errorhas no sentinel to check against. The unwrap states the rule once; a test pins that all four sentinels still answer exactly as before, direct and wrapped.Testing
go build ./...andgo test ./pkg/... ./internal/...green. Tests pin the wrapper's wording as the thing that must not matter, every classified status (400/401/403/404/409/500), title-inference when no explicit status is set, 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: the wording cases fail with "the status must come from the error, not from the wrapper's wording".
Draft because it is a shared helper on the HTTP boundary and the survey above is the thing worth a second pair of eyes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AB3apg5kCEGNPozrhSggum