feat: clearer OHTTP client error messages - #785
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8e415cd35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if matches!( | ||
| resp.status, | ||
| StatusCode::BAD_GATEWAY | StatusCode::SERVICE_UNAVAILABLE | StatusCode::GATEWAY_TIMEOUT | ||
| ) { | ||
| return AuthenticatorError::ServiceUnavailable { |
There was a problem hiding this comment.
When the gateway or indexer themselves time out, their routers install timeout_layer(... request_timeout(...)), which returns a structured JSON RequestTimeout body with HTTP 504, and the gateway also uses HTTP 503 for its typed BatcherUnavailable response. This new status-only shortcut runs before decoding the body, so those first-party service errors are now collapsed into ServiceUnavailable and callers lose the precise error code/message that was previously available via GatewayError/IndexerError. Consider parsing the structured service body first, or only falling back to ServiceUnavailable for opaque infra responses.
Useful? React with 👍 / 👎.
2 participants
Summary
Improves the accuracy of error messages surfaced on the OHTTP request path in the authenticator. Today, non-crypto failures are misattributed to OHTTP crypto — e.g. a target service being unreachable surfaces as a cryptic "decapsulation" error, and a captive portal returning a 200 HTML page also fails as an opaque decapsulation error.
All changes are server-agnostic and do not depend on matching the server's error body (so they remain portable across any RFC 9458 gateway). No server change is required.
Changes
OhttpEncapsulationErrorpreviously printed "encapsulation error" for both encrypt and decrypt failures. Added a dedicatedOhttpDecapsulationErrorso a failed decrypt is labelled accurately.message/ohttp-res(loose, case-insensitive match) now returnsOhttpRelayInvalidResponseinstead of a cryptic decap failure. The gateway only sets that content-type on success, so this is provably a relay-leg condition — most commonly a captive portal / transparent proxy intercepting the client network.OhttpRelayErrornow includes theservicescope andrelay_url, and its message reflects that a non-2xx outer response means the request was rejected before decapsulation (relay leg, or the gateway's RFC 9458 §5.2 pre-decap plaintext rejections such as a 401 key/config mismatch).ServiceUnavailableerror rather than a raw 5xx body. RFC 9458 §5.2 uses 504 for a gateway that cannot reach its target; this installs the correct client behaviour now (no effect on the current gateway, which returns 500, until a server follow-up lands).Out of scope
The headline "target-unreachable shows as decapsulation" symptom is not client-fixable: RFC 9458 §5.2 mandates that post-decapsulation and target errors be encapsulated (inner response, outer 200), so there is no outer-layer signal. The portable fix is server-side — emit a distinct inner status (504, not 500) for target-unreachable — and is recommended as a follow-up on the gateway.
Tests
message/ohttp-resmatcher.OhttpRelayError(scope + URL populated); 2xx non-OHTTP content-type →OhttpRelayInvalidResponse; correct content-type with an undecryptable body →OhttpDecapsulationError.