[AdamMagued]

Fixes #4651

Summary

sanitizeURL(uri *url.URL) *url.URL previously mutated uri.RawQuery directly on the input pointer. When formatting error messages (ErrorResponse.Error(), RateLimitError.Error(), AbuseRateLimitError.Error(), or RedirectionError.Error()), the live r.Response.Request.URL pointer was passed to sanitizeURL. This caused the caller's active request URL to be overwritten in place, redacting sensitive tokens to REDACTED. Consequently, subsequent request retries failed due to redacted credentials, and concurrent error formatting on the same request introduced a data race.

Changes

• Updated sanitizeURL in github/github.go to clone the incoming *url.URL struct prior to mutating RawQuery, returning a pointer to the clone while keeping the original pointer unchanged.
• Added regression tests in github/github_test.go:
  • TestSanitizeURL_DoesNotMutateInput verifies that calling sanitizeURL leaves the input pointer's RawQuery and String() intact.
  • TestErrorResponse_Error_DoesNotMutateRequestURL verifies that formatting an ErrorResponse redacts credentials in the formatted string while preserving original credentials in r.Response.Request.URL.
  • Added companion regression tests for RateLimitError, AbuseRateLimitError, and RedirectionError.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.63%. Comparing base (40091e2) to head (dd130ea).
⚠️ Report is 15 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4654      +/-   ##
==========================================
+ Coverage   98.61%   98.63%   +0.01%     
==========================================
  Files         199      201       +2     
  Lines       18693    18868     +175     
==========================================
+ Hits        18435    18610     +175     
  Misses        258      258              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

Thank you, @AdamMagued, but this PR as written doesn't solve #4651, as Error() continues to mutate state.

Additionally,

• The library never puts these params in URLs. Auth is applied as an Authorization header (github/github.go:661), and since #4564 even UnauthenticatedRateLimitedTransport uses Basic auth via setCredentialsAsHeaders (github/github.go:2217-2231). A grep of non-test code finds zero constructions of ?access_token=, ?client_secret=, or ?token=. Notably, the existing TestDo_sanitizeURL (github_test.go:2602) is now effectively vacuous — the secret is sent as a header, so it could never appear in the URL.
• The internal retry is unaffected. The only retry in bareDo (github/github.go:1528) clones the request and never calls Error(). So "subsequent retries fail with redacted credentials" only happens if caller code manually put a legacy query-token in a URL, then logged the error, then reused that request. Possible, but caller-induced and not something the repo does.
• The data race is narrow. It requires a sensitive param to be present and the same error formatted concurrently. Error() is otherwise a pure read, so ordinary header-auth users never trigger it.

If you wish to try again, that is fine, otherwise this PR will be closed.