- Fix AllowInsecureHTTP doc comment: say '_test.go file in the same
package' instead of 'export_test.go' (MINOR #1)
- Remove dead u.Fragment = "" from redactURL: HTTP requests never
carry fragments over the wire per RFC 7230 §5.1 (MINOR #2)
- Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer
that no network call should occur (NIT #3)
- Restore timer.Stop() no-op in case <-timer.C for symmetry with ctx.Done
- Add missing blank line between TestNoInsecureOption_RejectsHTTP and
TestNoInsecureOption_RejectsUppercaseHTTP
- Remove double blank line before TestAllowInsecureHTTP_WithoutEnvVar_Rejected
Resolves review comments from sonnet-review-bot on PR #113.
Address review feedback on PR #113:
- MAJOR (both reviews): Replace strings.HasPrefix(reqURL, "http://")
with url.Parse + strings.EqualFold for case-insensitive scheme
comparison per RFC 3986. Prevents bypass via HTTP:// or Http://.
- MINOR (security): Enhance redactURL to strip userinfo component
(user:pass@host) in addition to query params, preventing credential
leakage in error messages and logs.
- NIT (gpt): Remove redundant timer.Stop() after timer.C fires —
it's a no-op and the comment was misleading.
- Add tests for uppercase/mixed-case HTTP scheme rejection and
userinfo redaction.
Three-layer defense for the AllowInsecureHTTP client option:
1. Environment gate: AllowInsecureHTTP() requires REVIEW_BOT_ALLOW_INSECURE=1
env var. Without it, the option is silently ignored with a slog.Warn.
2. Warning log on activation: When the env gate IS satisfied, a slog.Warn
fires at client construction time so operators notice in production logs.
3. Test bypass: AllowInsecureHTTPForTest() skips the env gate entirely,
keeping test code clean (no t.Setenv needed in every test).
Additionally, doRequest now rejects HTTP URLs unless allowInsecureHTTP is set
on the client, providing defense-in-depth against credential leakage.
Closes#96
claw