fix(npm): scrub the relay failure response body
What does this MR do and why?
relayReadFailure scrubbed one of its two user-controlled fields and not the other. RequestURL went through redact.RedactURLUserinfo; ResponseBody was assigned raw off the *HTTPError.
Following that through: on main the npm client scans a response body for the five secret patterns in newHTTPErrorFromResponse but does not redact URL userinfo, and a credentialed URL carries no keyword for that scan to match. A refusal body echoing https://user:pass@upstream therefore reached Detail.HTTP.ResponseBody with the credential intact.
This MR closes the gap in two places. newHTTPErrorFromResponse now runs RedactURLUserinfo alongside ScanContent over the full body before truncation, so every caller of the constructor receives a scrubbed ResponseBody, and a credentialed URL no longer leaves a password prefix behind at either read boundary. The first is the 4081-byte truncation cut, where a URL's whole userinfo sits in the retained prefix while the @ is clipped; redacting before truncation, while the @ is still present, closes it. The second is the read cap: a URL whose @ sits at or past the cap never has its @ read, so when the read fills the cap the constructor redacts a trailing unterminated scheme:// authority before the scans. (pkg/client/oci's constructor has the same read-cap gap, tracked in #90, so this is the npm fix only.) relayReadFailure repeats both passes over the field it owns as defense in depth, matching the scrub it already applied to RequestURL.
Both renderers apply scanFreeText on the way out, so the exposure was limited to consumers importing Report.Cases[i].Detail.HTTP. That is the same reasoning the function already gives for redacting RequestURL at this site rather than leaving it to the renderer.
References
Closes #74 (closed)
Raised in review of !263 (comment 3812404387)
Differences
Before: detail.ResponseBody = httpErr.ResponseBody. A body reading {"error":"upstream unreachable: https://ci:hunter2@upstream.example/pkg"} reached the importable field with hunter2 in the clear.
After: the same body reaches it as {"error":"upstream unreachable: https://[REDACTED]@upstream.example/pkg"}. The host and path survive, so the field still names what was read.
Why both passes and not only the userinfo one
The constructor now scrubs the body, so relayReadFailure repeating both passes is defense in depth rather than the only line holding. Applying both keeps this site correct on its own terms rather than resting on which passes that constructor happens to apply, which is the argument scrubForMessage makes in the same package.
Both passes are idempotent, so the constructor and the renderer repeating them change nothing. ScanContent rewrites keyword<sep>value to keyword<sep>[REDACTED] and re-matching yields the same string; RedactURLUserinfo matches scheme://[REDACTED]@ and replaces it with itself. scrubForMessage's doc already records the second of those.
Nothing re-parses Detail.HTTP.ResponseBody. The error-shape verdicts read HTTPError.ErrorShapeReason, taken from the raw bytes upstream of any scan, so the false-FAIL hazard that doc warns about does not apply here.
How to set up and validate locally
-
Run both packages:
go test -count=1 ./pkg/conformance/npm/ ./pkg/client/npm/ -
Confirm each redaction layer is pinned on its own. Three independent reverts each turn one case red:
-
Drop
RedactURLUserinfofromnewHTTPErrorFromResponseinpkg/client/npm/client.go(leavingScanContentalone) and rungo test -count=1 -run TestNewHTTPErrorFromResponse_redactsCredentialedURLStraddlingTheCap ./pkg/client/npm/. It fails: a credentialed URL straddling the 4081-byte truncation cut leaks a password prefix. -
Drop the trailing-authority redaction (the
if len(body) == int(readCap)branch in the same constructor) and rungo test -count=1 -run TestNewHTTPErrorFromResponse_redactsCredentialedURLPastTheReadCap ./pkg/client/npm/. It fails: a credentialed URL whose@lands at the read cap leaks a password prefix. -
Change the assignment in
relayReadFailureback todetail.ResponseBody = httpErr.ResponseBodyand rungo test -count=1 -run TestRelayReadFailure_ScrubsACredentialedURLTheConstructorNeverBuilt ./pkg/conformance/npm/. It fails with:"{\"error\":\"upstream unreachable: https://ci:hunter2@upstream.example/pkg\"}" should not contain "hunter2"
-
MR acceptance checklist
- Both user-controlled fields on this detail are scrubbed at construction
- Each redaction layer has a test that fails when only that layer is reverted
- Idempotency of the doubled passes reasoned through and recorded in the comment
- Conforms to the code review guidelines