Skip to content

feat(apiclient): RFC 9457 Problem Details on v2 error responses#687

Merged
jhagberg merged 6 commits into
mainfrom
feat/v2-rfc9457-errors
May 28, 2026
Merged

feat(apiclient): RFC 9457 Problem Details on v2 error responses#687
jhagberg merged 6 commits into
mainfrom
feat/v2-rfc9457-errors

Conversation

@jhagberg
Copy link
Copy Markdown
Contributor

@jhagberg jhagberg commented Apr 24, 2026

Related issue(s) and PR(s)
Closes #678. Stacks on #686. Fifth of five PRs for #663.

Description
Replaces V2Client's ad-hoc "server returned status N: {body}" error format with RFC 9457 Problem Details parsing, adds a typed APIError with StatusCode for errors.As-based callers, and drops the substring-match hack in V2Client.DownloadFile's 403-flatten logic. Small PR (+205 / −13), focused on the error surface; success path unchanged.

Key design points

  • apiclient.APIError is a typed error with StatusCode (so callers can errors.As), an optional *ProblemDetails populated when the body parses, and a truncated Body for diagnostics. The msg field stays unexported so the formatted message always goes through Error().
  • parseErrorResponse tries Problem Details when Content-Type is application/problem+json or application/json (case-insensitive via mime.ParseMediaType). Falls back to "server returned status N: <truncated>" for HTML / plain / empty / malformed. Body read capped at 8 KiB.
  • V2Client.DownloadFile's resolve-step 403 flatten moves from strings.Contains(err.Error(), "status 403") to errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusForbidden. The old substring match silently stopped firing the moment the server started returning Problem Details — the new message contains "Forbidden (403)", never "status 403" — so the 403 would have leaked. This PR is what getJSON's // #678 replaces error wrapping... comment was waiting for.
  • Both 403 flatten sites still produce "dataset/file does not exist or access denied: <UserArg>"; the typed *APIError message is discarded on that path so the leakage contract can't leak Problem Details detail.

User-visible CLI message change (worth flagging at review)

Non-403 v2 server responses change shape. Dev stack returns application/problem+json, so:

  • Old: Error: failed to get datasets, reason: server returned status 401: {"detail":"Missing, invalid, or expired bearer token.",...}
  • New: Error: failed to get datasets, reason: Unauthorized (401): Missing, invalid, or expired bearer token.

403 flattening is unchanged. Shell scripts or log filters keyed on the literal "server returned status N" need updating for the Problem-Details path.

Out of scope (tracked separately)

How to test

Local:

  • go build ./..., go vet ./..., gofmt -l .: clean.
  • go test ./...: 191 pass.
  • golangci-lint run --timeout 5m: 0 issues.

Integration (build tag integration, against the live v2 dev stack):

  • TestV2_ListDatasets_Smoke, TestV2_ListFiles_Smoke, TestV2_ListFiles_ExactPath_Smoke, TestV2_ListFiles_PathPrefix_NoMatch, TestV2_DatasetInfo_Smoke, TestV2_DownloadFile_EndToEnd, TestV2_DownloadFile_NotFound403: 7/7 pass.

Manual against the live dev stack:

  • list --api-version v2 --dataset EGAD_FORBIDDEN"Forbidden (403): access denied" (typed; list has no leakage flatten).
  • list --api-version v2 --datasets with fake-signature JWT → "Unauthorized (401): Missing, invalid, or expired bearer token."
  • download --api-version v2 --dataset-id EGAD_DOES_NOT_EXIST --pubkey <pem> some-file.c4gh"dataset/file does not exist or access denied: some-file.c4gh" (403 on list-resolve via the typed errors.As path — would have leaked under the old substring hack).
  • Same with bare fileId → same flatten message.
  • Happy path by-path and by-fileId still produces canonical filename.

Test plan

  • go test ./...: 191 pass
  • go vet ./...: clean
  • golangci-lint run: clean
  • gofmt -l .: clean
  • Integration: 7/7 pass against live dev stack
  • Manual: typed 401/403, ambiguous-403 flatten via errors.As, happy path unchanged
  • CI: golint, test (ubuntu + windows), integration-v2, integration (s3), codeql-analysis

@jhagberg jhagberg requested a review from a team as a code owner April 24, 2026 12:24
@jhagberg jhagberg force-pushed the feat/v2-rfc9457-errors branch from 0ab1547 to d9727a8 Compare April 24, 2026 14:37
@jhagberg jhagberg force-pushed the feat/v2-download branch from 3805aeb to 7cac102 Compare May 13, 2026 13:22
@jhagberg jhagberg force-pushed the feat/v2-rfc9457-errors branch from d9727a8 to ff645f7 Compare May 13, 2026 13:24
@jhagberg jhagberg force-pushed the feat/v2-download branch from 7cac102 to 492a6fe Compare May 19, 2026 07:03
@jhagberg jhagberg force-pushed the feat/v2-rfc9457-errors branch from ff645f7 to 83ea2ca Compare May 19, 2026 07:04
@jhagberg jhagberg force-pushed the feat/v2-download branch from 492a6fe to e7d0917 Compare May 21, 2026 12:27
@jhagberg jhagberg force-pushed the feat/v2-rfc9457-errors branch from 83ea2ca to 2dc5663 Compare May 21, 2026 12:27
@jhagberg jhagberg force-pushed the feat/v2-download branch from e7d0917 to 797cc89 Compare May 21, 2026 12:57
@jhagberg jhagberg force-pushed the feat/v2-rfc9457-errors branch from 2dc5663 to 4bc874c Compare May 21, 2026 12:57
@jhagberg jhagberg force-pushed the feat/v2-download branch from 797cc89 to 6b3b300 Compare May 21, 2026 14:24
@jhagberg jhagberg force-pushed the feat/v2-rfc9457-errors branch from 4bc874c to 3807b5e Compare May 21, 2026 14:24
Comment thread downloadclient/errors.go
@jhagberg jhagberg force-pushed the feat/v2-download branch from 6b3b300 to 5d384e0 Compare May 26, 2026 13:19
@jhagberg jhagberg force-pushed the feat/v2-rfc9457-errors branch 2 times, most recently from e7afd82 to 9e5eabc Compare May 26, 2026 13:26
jhagberg added 4 commits May 27, 2026 10:58
Adds the RFC 9457 ProblemDetails shape, a typed APIError exposing
StatusCode so callers can errors.As on it instead of substring-matching
error messages, and parseErrorResponse which converts a non-2xx
*http.Response into *APIError.

Successful Problem Details bodies format as '<Title> (<status>):
<Detail>'; status-only or type-only bodies still populate apiErr.Problem
and fall through to a body-sample message. Non-Problem bodies (HTML,
plain text, empty, malformed JSON) fall back to 'server returned status
N: <truncated-body>'.

Content-Type matching goes through mime.ParseMediaType so it honours
RFC 7231's case-insensitivity rule. The body read is capped at 8 KiB
(errorBodyReadLimit) to bound allocation from a hostile or misconfigured
server.

Wiring into V2Client is in the next commit.
getJSON and DownloadFile non-2xx paths now delegate to parseErrorResponse,
replacing the inline 'server returned status N: {body}' formatting that
getJSON previously used. Server errors now surface as 'Title (status):
Detail' for Problem-compliant responses.

DownloadFile's resolve-step 403 flatten switches from substring-matching
on 'status 403' to errors.As against the typed *APIError. Substring
matching silently stopped working once the server returned Problem
Details (the formatted message contains 'Forbidden (403)', never
'status 403'); the typed check is the spec-correct way to discriminate
on status code. Both the resolve-step 403 and the download-GET 403
still flatten to 'dataset/file does not exist or access denied:
<UserArg>' to preserve the server's existence-leakage contract.
…ated]

Appends " [truncated]" suffix when the response body exceeds
maxErrorBodyBytes so the user knows the output was cut off.

Addresses KarlG-nbis review feedback on #687.
@jhagberg jhagberg force-pushed the feat/v2-rfc9457-errors branch from 9e5eabc to 25fc9bd Compare May 27, 2026 09:32
Comment thread downloadclient/errors.go Outdated
Comment thread downloadclient/errors.go Outdated
Comment thread downloadclient/errors.go
Copy link
Copy Markdown
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Added a minor comment regarding potential problems when truncating byte strings.

Back up to the last valid rune boundary before slicing so we never
split a multi-byte character. Keeps the byte-based limit for
terminal output sizing.

Addresses nanjiangshu review feedback on #687.
Base automatically changed from feat/v2-download to main May 28, 2026 08:23
@jhagberg jhagberg enabled auto-merge May 28, 2026 08:24
@jhagberg jhagberg added this pull request to the merge queue May 28, 2026
Merged via the queue into main with commit 1ec42e0 May 28, 2026
7 checks passed
@jhagberg jhagberg deleted the feat/v2-rfc9457-errors branch May 28, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(download): RFC 9457 Problem Details error parsing on v2 calls

3 participants