Redact the request URL from a transport error before it becomes the hint

Merged
JE
2mo ago
+690-12

The Go side of #174, which strips the URL from the Rust SDK's reqwest errors.

The leak. net/http reports every transport failure as a *url.Error, and url.Error.Error() renders the whole request URL (Get "https://…/blob?sig=…": dial tcp …). ErrNetwork copied that rendering into Hint and chained the error as Cause, so a refused connection, timeout or TLS failure against a signed URL — an Active Storage direct-upload or blob URL, where the query is the credential — put the signature into the SDK error's Error(), %+v, hint and cause chain: everywhere the error gets logged. Client.Get accepts an absolute https:// URL, so a caller-supplied signed URL reaches this path today.

The fix. One helper, redactTransportError in security.go next to RedactHeaders, projects every *url.Error's URL before any text is built, and keeps a projected *url.Error in the chain so errors.Is (context sentinels) and errors.As (*url.Error, net.Error) classify the failure as before. On the API origin the projection is scheme, host and path (the query dropped); anywhere else it is the origin alone, because a storage service can sign the query or the path (Active Storage's disk service puts its token in the path) — that follows basecamp-sdk SPEC §9's "credential-bearing URL is rendered as its origin only" rather than this card's keep-the-path default, and this repo has no spec of its own to say otherwise. The projection rebuilds the whole error tree. Every *url.Error has its URL projected. A URL on the SDK's own API origin carrying no userinfo is trusted — the SDK's request paths build their network error as networkError(err, c.cfg.BaseURL) — and beneath it the cause is kept, projected in turn: those URLs carry no credential (the token rides in the Authorization header; the query is paging and filtering) and the transport's diagnostic is what a reader needs. A *url.Error anywhere else — a storage host, a caller-supplied absolute URL on any origin, an API-origin URL with userinfo — is built from parts this package chose: Op kept only as the method token net/http writes there (else the fixed Request, on an ancestor of a projected one too), the URL as its origin, and in place of the cause a transportFailureError carrying the net.Error flags the transport error delegated and unwrapping to the context sentinels it wrapped (one or both), so errors.Is still sees a cancellation or a deadline. Whatever a transport put in those fields is text this package did not build and cannot show free of the URL; a join beside a projected member keeps its projected members and, of a dropped sibling, only its classification; an error exposing a *url.Error through As alone is replaced by that transport error projected. Exported ErrNetwork has no origin to trust and treats every URL as one that can be signed. A *url.Error with nothing to drop keeps its cause, projected in turn (a transport built on another http.Client nests one inside another), a multi-error as errors.Join of its projected members, and a wrapper dropped in favour of the projected cause (its text can carry the URL on its own, in any spelling, and nothing built from that text can be shown not to — no SDK-owned site wraps the transport error before ErrNetwork, so the text lost is a custom transport's own prefix). An error with nothing to drop is returned unchanged, at every level. Sites: ErrNetwork (every transport-failure site funnels through it: singleRequest, and both MagicLinkFlow requests) and AsError (a raw transport error handed to it took the same text into Message). No spec section governs error rendering in this repo, so the projection is query+userinfo only, as the card asks.

The test. TestNetworkErrorRendersNoSignedQuery dials 127.0.0.1:1 through the shipped client with ?sig=SECRETVALUE, on the API origin and as a caller-supplied absolute URL off it, and asserts SECRETVALUE is absent from Error(), Hint, Message, %+v and every link of the cause chain, that on the API origin the hint keeps http://127.0.0.1:1/x and the transport's connection refused, that off it the hint is https://127.0.0.1:1 and the fixed transport failure, and that the chain still yields a *url.Error carrying the projection. On main it fails with:

the signed query leaked into "Network error: Get \"http://127.0.0.1:1/x?sig=SECRETVALUE\": dial tcp 127.0.0.1:1: connect: connection refused"

TestRedactTransportError covers the bare, wrapped, opaque-wrapper, nothing-to-drop and unparsable cases; TestAsErrorRendersNoSignedQuery covers AsError.

Review rounds. Copilot and Codex found the nested *url.Error a custom transport produces and the branches of an errors.Join the first shape dropped; both are fixed by the tree walk above and covered in TestRedactTransportError; Codex then found a wrapper interpolating the URL in its own prefix, and an opaque cause doing the same beneath the *url.Error — the point where searching text had to give way to building a projected transport error from fixed parts alone (later rounds found Op, opaque join siblings, a cause that is cancelled and a timeout at once, userinfo on the API origin, an ancestor's Op, and both sentinels beneath one URL — all covered by construction now that nothing beneath or beside a projected URL is transport-owned text). Declined: a conformance fixture — the fixtures are JSON-driven mock-server cases with no way to force a transport failure, and the fix is Go-only until the other languages get the same audit, so a cross-language fixture would fail four runners for behaviour their SDKs do not yet have. A later round found GetAll taking a caller's absolute URL without the marker Get applies, and buildURL and RequireSecureEndpoint rendering the rejected URL whole; fixed, and TestCallerAbsoluteURLRendersNoSignedURL runs Get and GetAll with a signed URL on the API origin. Declined: looking for a custom As target on a type that also unwraps — the fall-through replaces a wrapper with nothing to drop by its trusted cause, and the shape takes a custom transport authoring it on purpose.

Hooks and logs. A request built from a caller's absolute URL — Client.Get("https://…"), the SDK's one way to reach a URL it did not build, and the shape a signed storage URL takes — carries a context mark: the request hooks, the retry hook and the debug logger see its origin alone, its transport failure reaches OnRequestEnd as its classification, and its network error trusts no origin (c.trustedOrigin(ctx)). The mark survives a hook that hands back a context of its own. API requests still reach the hooks whole. This is the shape hey-sdk#179 has for its storage requests and basecamp-sdk's download hop 1 already had (basecamp-sdk#837).

Gates run locally: make go-check, Go conformance runner (127 passed).

Watching the review loop on this one through to convergence.