Skip to content

fix: parse error bodies before throwing on status (R3.7) - #99

Merged
stormer78 merged 1 commit into
mainfrom
fix/parse-error-bodies-before-status
Jul 19, 2026
Merged

fix: parse error bodies before throwing on status (R3.7)#99
stormer78 merged 1 commit into
mainfrom
fix/parse-error-bodies-before-status

Conversation

@stormer78

Copy link
Copy Markdown
Contributor

The last two R3.7 violations from the audit. Both silently discard the server's machine-readable error code.

1. register-gateway.ts — the error branch was unreachable

It threw on !res.ok before reading the body, on /trust-tasks — the dispatcher endpoint that emits consent refusals. The file already handled isTrustTaskErrorType, but that branch could never run for a rejected task: rejections arrive non-2xx, and the status check threw first. A consent requirement surfaced as an opaque push/register: ... failed (403): {...} string.

This is the same bug rest-channel.ts was previously fixed for, still live on a second path.

2. rest-channel.ts — R3.7 violated inside the fix for R3.7

res.json() consumes the stream. Both fallbacks then called errorFromResponse(res), whose own res.json() throws Body has already been read into a bare catch {}:

doc = await res.json();                  // consumes the body
...
throw await errorFromResponse(res);      // re-reads it → throws → swallowed

So body?.error?.code was always undefined on that path. The code degraded to a status-only guess, and details/suggestion were always lost — precisely what R3.7 forbids, in the code written to satisfy it.

Adds errorFromBody(body, status, statusText) for callers that have already parsed. errorFromResponse is unchanged for callers that have not.

Why decodeTrustTaskHttpReply was extracted

When I mutation-tested fix 2 by restoring the double-read, no test failed. The reply decoding was only reachable through the full bearer handshake, so it was unverifiable in place.

It encodes a security-relevant decision — whether a refusal keeps the details an approver renders a consent prompt from — and logic reachable only via a DIDComm authcrypt round-trip is logic that in practice never gets exercised. Extracting it made the bug detectable.

A note on the existing tests

The register-gateway tests used { ok, json } object literals rather than real Response objects, so they broke against a change that is correct for every real Response. A hand-rolled stub only implements whatever the code happened to call when it was written. Switched to real Response objects — I hit the same trap in my own wiring test from #98 and fixed that too.

Verification

12 new tests across vta.errors, vta.rest-reply and device.register-gateway, covering: details surviving a 403 refusal, a server code that disagrees with its status, non-JSON bodies on 2xx vs non-2xx, and an explicit "body is read exactly once" assertion.

Mutation-tested both fixes:

  • restoring the double-read → "a non-2xx that is NOT a trust-task document keeps the server's error code" fails
  • restoring status-first ordering → "a refusal at a non-2xx status still surfaces its machine-readable code" fails

Full suite 204/204 (was 198), clean build and lint.

Not verified: no run against a real VTA emitting a real refusal. The wire shapes come from the existing tests and the trust-task error type, not from observed VTA traffic.

Remaining audit finding

Persist-before-ack (R1.6/R4.1) — @openvtc/vti-didcomm-js acks before dispatching to onMessage, and the wallet persists only the message id, never the body. An offscreen teardown mid-prompt loses a task-consent request permanently. Not fixable in this repo; needs a library change or disabling auto-ack. Worth an issue in the shared repo rather than a patch here.

Pre-merge checklist

  • All clients have finite timeouts (R1.2) — unchanged from fix: bound every outbound fetch with a timeout (R1.2) #98
  • No lock held across a network await (R1.3) — n/a
  • No local state committed before its remote effect (R2.1) — n/a
  • Every retry bounded + backed off (R1.4) — the 401 retry is unchanged
  • Accept/poll/listen loops survive transient errors (R1.5) — n/a
  • Acks/deletes only after durable handoff (R1.6) — untouched; see remaining finding
  • New/changed wire types (R3.*) — none; this reads existing shapes correctly
  • Config absence = most restrictive (R5.*) — an unparseable body yields a typed error from the status rather than being treated as success
  • Logs/status claim only what was verified (R6.*) — this is the fix: errors now report the code the server actually sent
  • "Process dies on the next line" answered (R2.1) — no mutation semantics changed
  • Deviations flagged with rule numbers — none

Two remaining R3.7 violations from the audit, both losing the server's
machine-readable error code.

1. `register-gateway.ts` threw on `!res.ok` before reading the body — on
   `/trust-tasks`, the dispatcher endpoint that emits consent refusals. The
   file already handled `isTrustTaskErrorType`, but that branch was
   UNREACHABLE for every rejected task, because a rejection arrives non-2xx
   and the status check threw first. A consent requirement surfaced as an
   opaque `failed (403): {...}` string. Now parses first and matches the
   trust-task error document before considering status.

2. `rest-channel.ts` had the same class of bug INSIDE the fix for it.
   `res.json()` consumes the stream, and both fallbacks then called
   `errorFromResponse(res)`, whose own `res.json()` throws "Body has already
   been read" into a bare `catch {}`. So `body.error.code` was always
   undefined there: the code degraded to a status-only guess and `details` and
   `suggestion` were always lost — R3.7 violated by the code written to
   satisfy R3.7.

Adds `errorFromBody(body, status, statusText)` for callers that have already
parsed, since a Response body can only be read once. `errorFromResponse` is
unchanged for callers that have not.

Also extracts `decodeTrustTaskHttpReply` from `RestChannel.send`. The reply
decoding was only reachable through the full bearer handshake, so reverting
the fix broke NO test — the fix was unverifiable in place. It encodes a
security-relevant decision (whether a refusal keeps the `details` an approver
renders a consent prompt from), and logic that can only be exercised via a
DIDComm authcrypt round-trip is logic that never gets exercised.

The existing register-gateway tests used `{ ok, json }` literals rather than
real `Response` objects. A hand-rolled stub only implements what the code
happened to call when it was written, so it stopped representing a Response
the moment the code read the body a different way — it failed against a change
correct for every real Response. Switched to real `Response` objects.

Verified: 12 new tests. Mutation-tested both fixes — restoring the double-read
fails "keeps the server's error code"; the register-gateway status-first order
fails "a refusal at a non-2xx status still surfaces its machine-readable
code". Full suite 204/204, clean build and lint.

Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
@stormer78
stormer78 merged commit 1ce9dbb into main Jul 19, 2026
3 checks passed
@stormer78
stormer78 deleted the fix/parse-error-bodies-before-status branch July 19, 2026 05:19
stormer78 added a commit that referenced this pull request Jul 19, 2026
CLAUDE.md was untracked, so every contributor had their own copy or none.
Checks it in, and corrects the parts that had gone stale — its named defects
were the ones most likely to be trusted verbatim, and all three were fixed:

- R3.7's `consentRequiredFrom` example was fixed (it now matches
  `details.reason`). Replaced with the rule that actually bites now: a
  Response body reads once, so an already-parsed body must use
  `errorFromBody`, not a re-read of the spent Response (#99).
- R1.5's "one 2s retry from onClose" applied to the worker inbound path
  (fixed in #88) and then to the approver inbox (fixed in #97). Replaced with
  the invariant — cap the delay not the attempt count, re-arm on every
  failure including first-connect — and a pointer to `ReconnectScheduler`.
- R1.2's `handleApiGet`/`handleApiPost` example was fixed in #88 and is now
  the compliant reference. Replaced with the thing that actually hides these:
  fetch is injected, so `grep "fetch("` finds almost nothing and the timeout
  belongs at the injection point (#98).

Promotes the one genuinely open defect to its own section: R1.6
persist-before-ack, which is not fixable from this repo — vti-didcomm-js acks
before dispatching to `onMessage`, and the wallet persists only the message
id, so an offscreen teardown mid-prompt loses a task-consent request for good.

Adds a repo-mechanics section for the traps that cost time this week: build
`core` before typechecking dependents, lint is `tsc -b` (never `-b --noEmit`,
TS6310), cross-workspace imports need a `references` entry, what CI asserts,
stub with real `Response` objects, and Node unreffing the `AbortSignal.timeout`
timer (passes locally, fails in CI).

Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
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.

1 participant