Redact the request URL from a transport error before it becomes the hint - #179
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Nested or custom transport errors can still expose signed URLs, and the regression test assumes port 1 is unused.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Redacts signed attachment URLs from Go SDK transport errors and observability hooks.
Changes:
- Adds URL projection and transport-error redaction.
- Marks storage uploads for hook-safe URL reporting.
- Adds regression tests for error chains and hooks.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
go/pkg/hey/security.go |
Implements URL and transport-error redaction. |
go/pkg/hey/errors.go |
Applies redaction to SDK errors. |
go/pkg/hey/http.go |
Projects marked storage URLs for hooks. |
go/pkg/hey/attachments.go |
Marks signed upload requests. |
go/pkg/hey/observability.go |
Documents hook URL behavior. |
go/pkg/hey/transport_error_test.go |
Tests redaction and classification. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 43e62c837d
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7f5439979
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afc54498d2
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6430af93e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4184d6e75
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2b7c7c800
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1d472ac17
ℹ️ 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".
net/http reports every transport failure as a *url.Error carrying the whole request URL, and ErrNetwork copied its rendering into the hint and chained it as the cause, so a dial failure on the attachment upload's storage hop — a signed URL, whose query is the credential — put the signature into the SDK error everywhere it is rendered or logged. The cause now passes through one helper that projects the URL to its scheme, host and path before any text is built, keeping the *url.Error in the chain so errors.Is and errors.As classify the failure as before; AsError takes the same path. The same storage request reached the request hooks with the signed URL whole in RequestInfo.URL, on success as much as on failure. It is now marked on its context and the transport hands the hooks the same projection; API requests, whose URL carries no credential, still reach the hooks whole. A test dials a closed port on the storage hop of an upload through the shipped client with a signed query and checks the hint, Error, %+v, every link of the cause chain and both hook arguments for it.
A transport built on another http.Client nests one *url.Error inside another, and errors.Join can hold two beside each other; projecting the first alone left the nested URL in the rendering and the unwrap chain, and returned a chain untouched when only the nested URL had anything to drop. The projection now recurses through the found error's own chain, and a wrapper's text is rebuilt only when it holds no second transport error the one substitution could miss; otherwise the projected transport error alone is kept.
Projecting the first *url.Error errors.As found, then guarding the wrapper text against a second one, dropped every other branch of a multi-error: errors.Join(signedURLError, context.Canceled) through ErrNetwork lost errors.Is(err, context.Canceled) once the URL was projected. The projection now rebuilds the tree node by node instead — a *url.Error projected with its own chain, a multi-error as errors.Join of its projected members, a wrapper with its text re-rendered around the projected cause or dropped when it rendered that cause in a form the substitution cannot find — and the guard goes away with the case it guarded. The storage request's failure reaches OnRequestEnd projected too: the default transport reports it as a net.OpError, which carries no URL, but a custom transport can report a *url.Error of its own, which renders the same signed URL. A test with such a transport checks the hook result.
HEY answers GetBlob and DownloadBlob with a redirect to a signed storage URL, which net/http follows on the request's own context, so the storage hop reached OnRequestStart and OnRequestEnd with the signature whole — on a successful download, where there is no error for ErrNetwork to project. The marker the attachment upload sets is now set on the blob download's context as well, so the transport projects every hop of it, and is named for what it does rather than for the storage request alone.
…ring it
Re-rendering a wrapper's text with the projection substituted for the
rendering it embedded assumed the wrapper carried the URL nowhere else; a
wrapper like fmt.Errorf("request %s failed: %w", req.URL, err) carries it in
its own prefix, in whatever spelling, and nothing built from that text can be
shown not to. The wrapper is now dropped in favour of the projected cause,
which is the projection-not-redaction rule applied to the last place text
was still being searched, and the re-rendering type goes with it.
Whatever a transport reports beneath a signed URL is text this package did not build — a custom transport's own message can interpolate the request URL with no *url.Error beneath it for the walk to find — and cannot be shown free of the URL in any spelling. A projected *url.Error now carries, in place of that cause, the context sentinel it wrapped, so errors.Is still sees a cancellation or a deadline, or a fixed transport failure carrying the net.Error flags the *url.Error delegated to it, so a timeout still classifies as one. Ordinary transport failures, whose URL has nothing to drop, keep their cause and its text as before.
Severing the cause beneath every projected URL threw away the diagnostic that matters most — a refused connection, a DNS or TLS failure — on the SDK's own API requests, whose query string is paging and filtering, not a credential: the token rides in the Authorization header. The SDK's request paths now build their network error knowing the API origin; beneath a URL on it the cause is kept, projected in turn, and only beneath a projected URL anywhere else — the storage host, a redirect's target — does the classification stand in for it. ErrNetwork, with no origin to trust, keeps treating every URL as one that can be signed, which is what the attachment upload and the hook result on a marked request want.
A *url.Error has three fields, and a custom transport can put the signed URL in any of them or beside them: in Op, where net/http writes only the method; in an opaque cause with no *url.Error beneath it; in an opaque sibling of an errors.Join. The projection now builds a *url.Error off the API origin from parts this package chose: Op as the method token or the fixed "Request", the URL projected, and in place of the cause a stand-in carrying the net.Error flags the transport error delegated and unwrapping to the context sentinel it wrapped — both at once, where a cause that was cancelled and a timeout lost one of them before. A join keeps its projected members and its sentinel siblings and drops the opaque ones. Nothing beneath or beside a projected URL is text this package did not build, which closes the class rather than the case. A request the hooks see projected trusts no origin for its network error: a blob download's redirect can land on a signed URL of HEY's own origin. Its retry hook sees the same projection the request hooks do.
A storage service can put its credential anywhere in the URL it signs — a query, a token in the path, a password in the userinfo — and Active Storage's disk service, which the route snapshot carries, puts its token in the path. Keeping the path of a URL off the API origin kept a place a credential can ride. Such a URL now projects to its scheme and host, which is all a reader needs to place the failure, and its cause is the fixed stand-in as before; the hooks and the retry hook see a marked request's hops the same way; a URL on the API origin carrying userinfo is no longer trusted for the same reason. An ancestor *url.Error of a projected one keeps only the method token as its Op, and a cause that wrapped both context sentinels keeps both.
Trusting every URL on the API origin trusted a signed storage URL served from it — Active Storage's disk service puts its token in the path of the app's own origin — when a caller handed one to Get. A request built from a caller's absolute URL now carries the mark the storage requests carry: the hooks see its origin alone and its network error trusts no origin. The mark survives a hook that hands back a context of its own, so the redirect net/http derives from the request keeps it, and a marked request's transport failure reaches the hooks as its classification whatever shape the transport reported it in. Two more places transport-owned text could still reach a rendering close with it: an error exposing a *url.Error through As alone, with no Unwrap, is replaced by that transport error projected; and a join member dropped for its text leaves its net.Error classification behind, so a timeout beside a projected member still classifies as one.
Get marked a caller's absolute URL as one that can be signed; PostForm, PatchForm, DeleteForm, PostMultipart and GetAll take one through the same buildURL and did not, so a signed URL handed to them reached the hooks whole and, on the API origin, kept its path beneath the network error. One helper, markCallerURL, now marks all of them, and sendBodyRequest trusts the origin singleRequest does. Two pieces of the SDK's own text still carried the URL on such a request. RequireSecureEndpoint's rejection rendered the direct-upload URL HEY returned — a signed URL, before any request existed — and buildURL's rejection of an http URL on another host rendered the caller's; both name the origin alone now, and a parse failure is rendered without the URL net/url puts in its own error. ErrNotFound's identifier takes the same projection on a marked request, since an expired disk-service URL answers 404: a blob download's 404 names the origin rather than the blob path.
4b0fa07 to
c49e7fc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c49e7fc047
ℹ️ 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".
…te form is A credential can sit in a URL here in two shapes: a caller's absolute URL, on any origin, and one of HEY's own Active Storage routes whose path is the credential — the disk service's encoded key, the signed ids of blob redirects, proxies and representations. The marker covered the first; a same-origin signed path handed to Get in relative form was trusted, so the hooks, the 404 identifier and a transport error kept the token. The marker now names both shapes, which is the whole class, and an ordinary API path keeps its detail; a direct upload is created under the same mount with nothing in its path and stays one.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acee267317
ℹ️ 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".
|
Review threads: 7 resolved (2 fixed, 5 declined with the reasoning in each thread). Copilot re-reviewed acee267 on request and its four threads folded into the two decisions below, both closed on 432a886; Codex's one finding on that head is fixed in 053a7b1. Main was merged in (15ad43b, no conflicts) because CodeQL's javascript-typescript job, added to the matrix when the TypeScript SDK landed on main, fails on a head with no TypeScript to scan. Fixed:
Declined:
|
There was a problem hiding this comment.
🟡 Changes recommended
Malformed URLs and omitted signed email-storage paths can still expose credentials.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
…llerURL keeps A prefix list cannot observe what it guards: HEY's credential-bearing routes include paths with no fixed prefix at all, so each round would name the one the list missed. Every signed URL HEY hands out is absolute and marked as such, and a relative blob path goes through GetBlob or DownloadBlob, which mark their own; a signed path in relative form handed to a raw helper is the caller's own text.
…a fixed token buildURL is the one entry for a caller's string, so it parses what it returns once: accountScopedURL and http.NewRequestWithContext re-parse a string that has already parsed and can no longer fail on it. net/url's error quotes the input, or the component it rejected, either of which can be the signed value; buildURL and requireHTTPS now render "invalid URL" alone.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 432a886cb1
ℹ️ 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".
net/url reads a scheme case-insensitively, so HTTPS://… is a URL a request can be sent to; the prefix check that decides whether a caller's string is an absolute URL or an API path read it in lower case alone, and sent the upper-case form under the base URL, unmarked.
…-typescript job now scans * origin/main: Add generated TypeScript HEY SDK with safely gated npm releases (#144) Install mbx from its 1.10.1 release instead of building it from git (#190) Try mr-boxington objects mode again, with mbx pinned to a fix on its main (#188) Build CI's debug profiles with line tables only (#186) Run the conformance jobs alongside the unit tests instead of after them (#184) Revert "Try mr-boxington as the Rust test job's build cache (#182)" (#187) Save the cargo cache from main only (#181)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dbb9f3a1e
ℹ️ 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".
| case hasScheme(path, "https"): | ||
| resolved = path |
There was a problem hiding this comment.
With a caller-supplied off-origin URL whose scheme is mixed-case (for example, HTTPS://storage.example/...), this new case-insensitive branch now passes it through, after which singleRequest and sendBodyRequest call prepareAPIRequest, whose default authentication strategy adds the HEY bearer token to the initial request. The redirect policy strips authorization only from redirected hops, so Get, GetAll, and the form helpers can disclose API credentials directly to the off-origin server. This is fresh evidence beyond the prior mixed-case hook-redaction comment because the authentication wiring was not accounted for; reject off-origin absolute requests or omit API authentication for them.
Useful? React with 👍 / 👎.
The Go side of the fix that strips the URL from the Rust SDKs' reqwest errors (fizzy-sdk#174, basecamp-sdk#869).
The leak. net/http reports every transport failure as a
*url.Error, andurl.Error.Error()renders the whole request URL (Put "https://…/blob?signature=…": dial tcp …).ErrNetworkcopied that rendering intoHintand chained the error asCause, so a refused connection, timeout or TLS failure on the one request the SDK issues to a signed URL — the attachment upload's PUT to the Active Storage direct-upload URL, where the query is the credential — put the signature into the SDK error'sError(),%+v, hint and cause chain: everywhere the error gets logged. The same request reached the request hooks with the signed URL whole inRequestInfo.URL, on success as much as on failure.The fix. One helper,
redactTransportErrorinsecurity.gonext toRedactHeaders, projects every*url.Error's URL before any text is built, and keeps a projected*url.Errorin the chain soerrors.Is(context sentinels) anderrors.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, in this repo's route snapshot, puts its token in the path — which follows basecamp-sdk SPEC §9's "credential-bearing URL is rendered as its origin only" rather than this card's keep-the-path default, this repo having no spec of its own to say otherwise. The projection rebuilds the whole error tree. Every*url.Errorhas 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 asnetworkError(err, c.cfg.BaseURL)— and beneath it the cause is kept, projected in turn: those URLs carry no credential (the token rides in theAuthorizationheader; the query is paging and filtering) and the transport's diagnostic is what a reader needs. A*url.Erroranywhere else — the storage host, a redirect's target, an API-origin URL with userinfo — is built from parts this package chose:Opkept only as the method token net/http writes there (else the fixedRequest, on an ancestor of a projected one too), the URL as its origin, and in place of the cause atransportFailureErrorcarrying thenet.Errorflags the transport error delegated and unwrapping to the context sentinels it wrapped (one or both), soerrors.Isstill 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.ErrorthroughAsalone is replaced by that transport error projected. A request the hooks see projected trusts no origin at all (c.trustedOrigin(ctx)): a blob download's redirect can land on a signed URL of HEY's own origin, and its retry hook sees the same projection the request hooks do. ExportedErrNetworkhas no origin to trust and treats every URL as one that can be signed, which is whatAttachments.Uploadand the hook result on a marked request use. A*url.Errorwith nothing to drop keeps its cause, projected in turn (a transport built on anotherhttp.Clientnests one inside another), a multi-error aserrors.Joinof 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 — no SDK-owned site wraps the transport error beforeErrNetwork, 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,doFormRequest,retryCausefor the generated client's resends, the token refresh inauth.go, andAttachments.Upload) andAsError(a raw transport error handed to it took the same text intoMessage).The hooks get the same projection on the requests whose URL can be signed:
Attachments.Uploadmarks the storage PUT's context,GetBlobandDownloadBlobmark theirs, and so doesdoRequestfor a caller's absolute URL (the SDK's one way to reach a URL it did not build) (HEY answers them with a redirect to a signed storage URL, which net/http follows on the same context, so the storage hop reached the hooks with the signature whole on every successful download), andloggingTransporthandsRequestInfo.URLthe origin alone for every hop of a marked request andRequestResult.Errorthe failure's classification alone (a customWithTransportcan report a*url.Errorof its own, or a plain error interpolating the URL). The mark survives a hook that hands back a context of its own, so the redirect net/http derives from the request keeps it. API requests carry no credential in their URL (the token is in theAuthorizationheader) and still reach the hooks whole, asRequestInfo's doc now says. A signed storage path handed to a raw helper in relative form is the caller's own text and is not recognised: every signed URL HEY hands out is absolute, a relative blob path goes throughGetBlob/DownloadBlob, which mark their own, and no fixed prefix covers HEY's credential-bearing routes;markCallerURL's comment states that boundary. This is the shape basecamp-sdk's download hop 1 already has (basecamp-sdk#837).Review rounds. Copilot and Codex found the nested
*url.Errora custom transport produces (the projection now rebuilds the whole error tree: nested chains,errors.Joinmembers, wrappers), the hook result on a custom transport, the blob-download redirect hop, and a wrapper interpolating the URL in its own prefix; on the fizzy side Codex then showed an opaque cause doing the same beneath the*url.Error, the point where searching text had to give way to keeping only the classification beneath a projected URL. All are fixed and tested here too. Declined: deriving the closed port from a freed ephemeral listener instead of port 1 (the freed port is what a parallel test'shttptest.NewServeris handed next). A later round found the form helpers andGetAlltaking a caller's absolute URL without the markerGetapplies,RequireSecureEndpointandbuildURLrendering the rejected URL whole, andErrNotFoundnaming a signed URL that answered 404; all fixed. Declined: replacing a custom transport's deferred body-read errors with their classification (net/http builds no*url.Errorthere; the default transport's read failures carry no URL, and the wrapper would drop theunexpected EOFa real truncated download reports), and looking for a customAstarget on a type that also unwraps (the fall-through replaces a wrapper with nothing to drop by its trusted cause). The last round removed the relative-path prefix allowlist an earlier round had added, for the reason above, and bounded a caller URL's parse atbuildURL, the one entry for a caller's string: a URL net/url rejects is rendered as the fixed tokeninvalid URLthere and inRequireSecureEndpoint, never as net/url's text, which quotes the input or the component it rejected, soaccountScopedURLandhttp.NewRequestWithContextcan no longer fail on a stringbuildURLreturned.The test.
TestAttachmentsUploadTransportErrorRendersNoSignedURLserves a direct-upload URL ofhttp://127.0.0.1:1/blob?signature=SECRETVALUEand runsAttachments().Uploadthrough the shipped client, assertingSECRETVALUEis absent fromError(),Hint,Message,%+v, every link of the cause chain,RequestInfo.URLfor the storage request and everyRequestResult.Error; that the hint keepshttp://127.0.0.1:1; that the chain still yields a*url.Errorcarrying the projection; and that the API request's URL still reaches the hooks whole. Onmainit fails with:TestRedactTransportErrorcovers the bare, wrapped, opaque-wrapper, nothing-to-drop and unparsable cases;TestAsErrorRendersNoSignedQuerycoversAsError.TestCallerAbsoluteURLReachesHooksProjectedandTestCallerAbsoluteURLTransportErrorRendersNoSignedURLrunGet,GetAll,PostFormandPostMultipartwith a signed URL on the API origin;TestAttachmentsUploadInsecureTargetRendersNoSignedURLandTestCallerInsecureURLRendersNoSignedURLcover the two rejections.TestCallerMalformedURLRendersNoSignedValueruns the same four methods with a bad escape and a signed value in the port position, on an unscoped and an account-scoped client, andTestAttachmentsUploadMalformedTargetRendersNoSignedValuecovers the direct-upload URL: neither value appears in any rendering and no request reaches the hooks.Gates run locally:
make go-check,make conformance-go(195 passed).Watching the review loop on this one through to convergence.