fix: Limit token auth to configured hosts

[huynhtrungcsc]

Summary

• Only attach WithAuthToken credentials to requests for the configured API or upload hosts.
• Preserve unauthenticated cross-host requests created from absolute URLs.
• Add regression coverage for API and upload request host boundaries.

Fixes #4366

[alexandear]

Could you create an issue first with the problem you're trying to resolve, and once we've confirmed it, we can proceed with a review?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.51%. Comparing base (90075ee) to head (92eef19).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4363   +/-   ##
=======================================
  Coverage   97.51%   97.51%           
=======================================
  Files         193      193           
  Lines       19519    19549   +30     
=======================================
+ Hits        19033    19063   +30     
  Misses        269      269           
  Partials      217      217           

☔ 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.

[huynhtrungcsc]

Thanks for the guidance. I opened an issue describing the behavior this PR addresses:

• WithAuthToken authorizes requests outside configured hosts #4366

I also added focused coverage for the host-boundary helper and the configured-host authorization behavior. I’m happy to adjust the PR based on the maintainers’ preferred direction.

[gmlewis]

Thank you, @huynhtrungcsc.

I understand that this is a hypothetical situation and has not been reported as an actual problem "in the wild" but I also understand that with the increased proliferation of "vibe coding" and not properly understanding how to use packages, that this kind of change will help reduce the possibility of users of this package from shooting themselves in the foot.

LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @Not-Dhananjay-Mishra

[alexandear]

@huynhtrungcsc please update the PR's description with "Fixes #4366" and remove the unnecessary "Tests" section.

[huynhtrungcsc]

Updated the PR description to include "Fixes #4366" and removed the Tests section. Thank you.

[alexandear]

It looks like this PR was created with AI assistance, so I reviewed it with AI as well. Here's what I found:

1. github/github.go:638 — Clone() silently loses authentication (CONFIRMED, reproduced). The token closure captures c and checks the original client's URLs; Clone at github.go:783 copies the wrapped transport without re-binding it. NewClient(WithAuthToken(...)).Clone(WithEnterpriseURLs(...)) — a pattern that worked before this change — now sends every request unauthenticated. This is the must-fix.

2. github/github.go:723 — no default-port normalization (CONFIRMED). https://ghe.corp:443/ in config vs https://ghe.corp/... in a request URL (same RFC 6454 origin) fails the match and drops the token deterministically.

3. github/github.go:718 — three divergent boundary predicates (CONFIRMED). sameOrigin (scheme+host, case-insensitive) vs bareDoUntilFound:1391 and checkRedirectHost:2182 (host-only, case-sensitive). Concrete effect: a same-host https→http redirect is followed but not authenticated — no data, no error. Consolidate into one shared predicate.

4. github/repos_contents.go:200 — DownloadContents on private repos (PLAUSIBLE). The download_url fetch (raw.githubusercontent.com / GHES raw host) goes through the token transport and now loses the header; usually rescued by the embedded ?token=, but worth a deliberate decision and a test whose fake download_url is cross-origin.

5. github/repos_releases.go:328 — DownloadReleaseAsset + client.Client() (PLAUSIBLE). The documented private-repo pattern no longer authenticates cross-host redirect targets. A fix for github.com's pre-signed S3 URLs, a possible regression for GHES storage topologies.

6. github/github.go:258 — stale docs/comments (CONFIRMED). Client(), WithAuthToken (:436), and the bareDoUntilFound rationale comment (:1389) all still describe unconditional token attachment.

7. github/github.go:636 — wasted work in the hot path (cleanup). Early-exit before req.Clone, and hoist the "Bearer "+token string out of the closure.

8. github/github_test.go:714 — redundant tests / dead nil checks (cleanup). Fold TestSameOrigin's two unique cases into TestClientShouldAuthorizeRequest and drop the unreachable nil branches.

Findings 1–3 all point at one design question worth settling before merging: should the closure capture resolved URL values, or should the origin check live at a client level (e.g. in BareDo) where the correct client — including clones — is always in hand? Fixing it there resolves the Clone bug and gives the redirect guards a single predicate to share.

[huynhtrungcsc]

Thanks for the detailed review. I addressed the confirmed regression and consistency issues in the latest push:

• Fixed Clone() by preserving the WithAuthToken token in the client config and re-wrapping the clone's transport against the clone's configured API/upload URLs, instead of reusing the original client's auth wrapper.
• Normalized origin matching with a shared sameOrigin helper, including default port handling (https://host == https://host:443).
• Updated the redirect guards to use the same origin predicate and reject cross-origin redirects consistently.
• Updated the Client() / WithAuthToken comments and cleaned up the hot path by avoiding request cloning unless the token is actually attached.
• Added regression coverage for the clone behavior and default-port origin matching, and simplified the helper tests.

I left the DownloadContents / DownloadReleaseAsset cases unchanged in this PR because those are broader product-behavior decisions around cross-origin GitHub/GHES download URLs. I’m happy to follow the maintainers’ preferred direction there if you want this PR to cover those paths too.

Verified locally with:

go test ./github -run "TestWithAuthToken|TestShouldAuthorizeURL|TestBareDoUntilFound_RejectsCrossOriginRedirect|TestRoundTripWithOptionalFollowRedirect_RejectsCrossOriginRedirect" -count=1
go test ./... -count=1

[gmlewis]

Same comment here about your code coverage dropping. Please fix.

[huynhtrungcsc]

Thanks, I added coverage for the remaining helper branches so the new code paths are exercised directly, including nil URL/configured origins, default HTTP port handling, and non-HTTP schemes.

Verified locally with:

go test ./github -run "TestShouldAuthorizeURL|TestWithAuthTokenCloneAuthorizesCloneHosts" -count=1
go test ./... -count=1

[stevehipwell]

I don't think we should be making these changes without actually discussing the issue as there are other related issues that should be looked at together with this. When I rewrote the client auth code I did so to keep it aligned with the existing behaviours, if we're looking to change that I'd like to make sure we carefully consider how to do it.

[stevehipwell]

Wy has this code been moved? It makes it much harder to review the actual changes?

[huynhtrungcsc]

Thanks, that makes sense. I agree this should not be changed piecemeal if there are related auth, redirect, and download-url behaviors that need to be considered together.

I will stop pushing implementation changes on this PR for now and use #4366 as the place to settle the expected behavior first. Once there is maintainer consensus on the boundary this should enforce, I can either adjust this PR to match that direction or close it if a different approach is preferred.