gitproto: retry credential auth against the actual challenge URL

5ec540d→main·

Soph·1mo ago·3 files·+247 added/-46 removed

The 401-retry path replayed against c.EndpointURL even when the 401 came from a cross-host redirect (e.g. github.com → replica.example). Go's http.Client strips Authorization on cross-host redirects (per shouldCopyHeaderOnRedirect), so the retry hit the challenger without auth, got 401 again, and we Reject'd the user's valid credentials — locking them out on the next sync, since Lookup would then return nothing.

Production flow that triggers this:

1. GET origin/info/refs (anonymous) → 307 → challenger returns 401.
2. res.Request.URL.Host == challenger; challengeURL keyed correctly.
3. Lookup(challenger) returns the stored creds.
4. Retry builds URL from c.EndpointURL = origin, attaches auth.
5. http.Client follows 307 → strips Authorization → challenger 401.
6. We hit the reject branch → Reject(challenger, valid-creds). Lost.

Fix:

• tryHelperRetry captures res.Request.URL (the post-redirect URL the 401 actually came from) and passes it to the retry callback as an override target. The retry hits the challenger directly, no redirect to strip the header.

• On a successful cross-host retry we also rewrite c.EndpointURL's scheme/host to the challenger via adoptChallengeHost. Otherwise follow-up ops on the same conn would redirect again, lose auth, and have their freshly-Approved creds Reject'd inside resolvePendingHelperCreds when the next 401 came through.

• EnsureAuthForService had a related milder issue: Lookup ran against c.EndpointURL before the probe, but pendingHelperCreds.url was keyed on the post-probe challenge host. Cross-host: Lookup queried origin (probably no entry), then Approve/Reject targeted the challenger. Restructured to probe first, Lookup against challengeURL, then adopt the challenge host — same shape as the tryHelperRetry fix. Drops the pre-probe Lookup optimization (one fewer Lookup, one extra probe POST when the helper has no creds); necessary trade-off since a user may have creds for the challenger but not for the origin.

doInfoRefsRequest / doPostRPCRequest now accept an optional target *url.URL to support the override; nil keeps the previous "build from c.EndpointURL" behaviour for non-retry callers.

Regression tests:

• TestRequestInfoRefs_OnUnauthorizedAfterCrossHostRedirectRetries-AgainstChallenger — drives the production scenario end-to-end and asserts (a) the retry RoundTrip lands on the challenger with a Basic auth header, (b) Approve fires keyed on the challenger, (c) no Reject, and (d) c.EndpointURL has adopted the challenger so follow-ups don't redirect.
• TestEnsureAuthForService_CrossHostProbeLooksUpAndAdoptsChallenger — same idea on the EnsureAuthForService path.

Sessions

dcdc9414badfView transcript

Changes

3

• cmd/git-sync

• Mmain_test.go+15/-6

• internal/gitproto

• Msmarthttp.go+86/-27

• Msmarthttp_test.go+146/-13

// TestMain isolates the package's tests from the developer's local
// credential helper. Without this, `git credential fill` could find
// stored credentials for 127.0.0.1 (e.g. cached from an earlier test
// run) and turn EnsureAuthForService's would-be no-op into a real
// auth-probe POST, throwing off receive-pack POST counts.
// credential helper. EnsureAuthForService probes /git-receive-pack with a
// flush-packet POST unconditionally (required to discover cross-host
// auth challenges and auth-on-POST-only gates), so without stubbing the
// helper, `git credential fill` could find stored credentials for
// 127.0.0.1 (e.g. cached from an earlier test run) and attach them,
// changing the wire shape of the push the test under inspection.
// The probe itself still happens — receive-pack POST counts include it —
// but the stub guarantees no credentials are attached and the probe
// returns without further side effects on the helper.
// Tests that need to exercise helper behaviour explicitly should
// restore auth.GitCredentialCommand in their own setup.

Test EnsureAuthForService_SkipsProbeWhenHelperHasNoCredentials

This test avoids a wasted no-op POST when there are no credentials to attach anyway — the common shape for anonymous syncs and for syncs running in test/CI environments with no credential helper configured.