Strip the request URL from a reqwest error before it becomes the SDK's

Merged
JE
2mo ago
+84-3

Origin: Codex's second review round on basecamp/sdk#15, fixed in the seed at basecamp/sdk@5c45cb3. This crate carries the same shape as the seed did.

The leak. reqwest::Error writes the request URL into its own Display (error sending request for url (…)) and Debug. Error::network copies that Display into the hint and chains the reqwest error as the source, so a transport failure — refused connection, timeout, TLS error, reset mid-stream — against a pre-signed storage URL put the signature from the query into the SDK error's hint, Display, Debug and source chain: everywhere the error gets logged. A signed URL — an Active Storage direct-upload or blob URL — is the case where the query is the credential.

The fix. A network helper — the shape hey-sdk already has — calls reqwest::Error::without_url() before Error::network, and all three conversions go through it. Three conversion sites, all in src/http/reqwest.rs: the try_from of the request, the execute, and the body chunk() stream.

The test. tests/reqwest_client.rs sends through the shipped client to a closed port on 127.0.0.1 with ?signature=… in the URL and asserts the error is network, still chains its cause and still carries a hint, and that the signature appears in none of Display, Debug, the hint, or any link of the source chain by either rendering. On main it fails with:

the signed query leaked into "Network error: error sending request for url (http://127.0.0.1:56749/storage/blob?signature=sig-4f3c2a1b-never-logged)"

The chunk() path is stripped for uniformity; reqwest 0.13.5 attaches the URL only on its whole-body readers (bytes(), json()), not on chunk(), so no test can currently fail there.

Gates run locally: make rs-check, make conformance-rust.

Watching the review loop on this one through to convergence.

Summary by cubic

Prevents signed request URLs from leaking into SDK error messages. reqwest::Error writes the request URL into its Display and Debug, and Error::network copied that text into the hint and chained the error as source, so a transport failure against a signed URL (like an Active Storage direct upload) exposed the query signature everywhere the error is rendered or logged. All three conversion sites now pass errors through a helper that calls without_url() first.

Test

  • Adds an integration test that sends a request to a closed port with a ?signature=... query and asserts the signature is absent from Display, Debug, hint, and all source-chain renderings.
  • The chunk() path is stripped for uniformity, though reqwest only attaches URLs to whole-body readers, so no test currently fails there.