gitproto: track resolved endpoint separately from user input · Entire

gitproto: track resolved endpoint separately from user input

c01a2da→main·

Addresses review feedback: the auth path was mutating c.EndpointURL when adopting a cross-host challenger, conflating "the URL the user typed" with "where we ended up after redirects." The mutation also fired without checking FollowInfoRefsRedirect, so the auth flow was effectively following redirects even when the user had opted out of that.

Two changes in one:

No more EndpointURL mutation. A new c.resolvedEndpoint field carries the post-redirect scheme/host when one's been discovered; c.EndpointURL stays exactly as the caller passed it in. URL-building reads c.requestURL(), which returns resolvedEndpoint when set and EndpointURL otherwise. The user-typed URL is now stable for display, logging, telemetry, the SSH-scheme check in refs.go, and the github.com check in bootstrap.go — none of which want the resolved value.

The existing FollowInfoRefsRedirect block in RequestInfoRefs also stops mutating EndpointURL and writes to resolvedEndpoint instead, for consistency.

adoptChallengeHost is now gated on FollowInfoRefsRedirect. The user's explicit opt-in is the trigger for the conn's effective endpoint changing. With the flag off the immediate retry still hits the challenger directly (so the current op succeeds and the helper Approves valid creds on the right key — the production bug stays fixed), but follow-up ops on the same conn stay pointed at the user-typed URL. They'll surface a 401 the user can address by setting the flag.

The two TLS-off cross-host gates compare against c.requestURL().Host rather than c.EndpointURL.Host, so they remain correct on a conn that has already adopted a previous redirect (otherwise a same-host 401 from the resolved endpoint would look "cross-host" to the gate).

Tests:

TestRequestInfoRefs_FollowInfoRefsRedirect and the cross-host auth / EnsureAuthForService tests now assert (a) EndpointURL is unchanged and (b) resolvedEndpoint carries the discovered host.
The two adoption tests set FollowInfoRefsRedirect=true to opt in; a new TestRequestInfoRefs_CrossHostRetryWithoutFollowFlagSucceeds-ButDoesNotAdopt covers the gated half — flag off, retry still works, resolvedEndpoint stays nil.
TestRequestInfoRefs_DoesNotFollowByDefault and both TLS-off tests now also assert resolvedEndpoint == nil.

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

Sessions

Changes

internal/gitproto
Msmarthttp.go+69/-20
- Msmarthttp_test.go+110/-10

// on 401/403, ensuring helper state reflects the actual outcome rather
// than an ambiguous probe response.
pendingHelperCreds *helperCreds

// resolvedEndpoint, when non-nil, supersedes EndpointURL.Scheme/Host
// for outgoing requests on this conn. Populated when one of two
// redirect-following paths fires:
//
// - The FollowInfoRefsRedirect block in RequestInfoRefs, after a
//   successful /info/refs that landed on a different host.
// - adoptChallengeHost, after credential-helper auth resolves
//   against a cross-host challenge.
//
// Both are gated on FollowInfoRefsRedirect, so resolvedEndpoint only
// diverges from EndpointURL when the user has opted into following
// redirects. EndpointURL itself is never mutated — display, logging,
// telemetry, and the user-typed-URL accessors (Endpoint()) keep
// returning what the caller passed in.
//
// Path/userinfo are copied from EndpointURL; only Scheme/Host differ.
resolvedEndpoint *url.URL

func (c *HTTPConn) requestURL() *url.URL {
    if c.resolvedEndpoint != nil {
        return c.resolvedEndpoint
    }
    return c.EndpointURL
}

// adoptChallengeHost rewrites c.EndpointURL's scheme/host to match the host
// that just successfully authenticated. Required when the challenge came from
// a cross-host redirect: subsequent requests on this conn would otherwise be
// sent to the original host, redirected again, and stripped of their
// Authorization header — turning every follow-up into a fresh 401. Path is
// preserved on the assumption that the redirect target serves the same repo
// path (the same assumption FollowInfoRefsRedirect makes after a successful
// /info/refs).
// adoptChallengeHost records the host that just successfully authenticated
// as this conn's resolved endpoint, so subsequent ops on the same conn go
// there directly instead of replaying through c.EndpointURL — which would
// redirect again and have its Authorization header stripped on the cross-
// host hop, turning every follow-up into a fresh 401.
//
// Gated on FollowInfoRefsRedirect: the user explicitly opting into redirect-
// following is the trigger for the conn's effective endpoint changing. With
// the flag off the immediate retry still hits the challenger directly (so
// the current op succeeds and creds get Approved on the right key), but the
// next op stays pointed at the user-typed URL. EndpointURL itself is never
// mutated; we set resolvedEndpoint instead, leaving EndpointURL as user
// input for display/logging/telemetry to read.
//
// Path/userinfo are copied from EndpointURL — same assumption
// FollowInfoRefsRedirect's /info/refs block makes about the redirect target
// serving the same repo path.
func (c *HTTPConn) adoptChallengeHost(challengeURL *url.URL) {
    if challengeURL == nil {
        return
    }
    if !c.FollowInfoRefsRedirect || challengeURL == nil {
        return
    }
    if challengeURL.Host == c.EndpointURL.Host && challengeURL.Scheme == c.EndpointURL.Scheme {
        return
    }
    
    current := c.requestURL()
    if challengeURL.Host == current.Host && challengeURL.Scheme == current.Scheme {
        return
    }

c.EndpointURL.Scheme = challengeURL.Scheme
    c.EndpointURL.Host = challengeURL.Host

resolved := *c.EndpointURL
    resolved.Scheme = challengeURL.Scheme
    resolved.Host = challengeURL.Host
    c.resolvedEndpoint = &resolved
}