gitproto: defer credential approval until response passes validation

Both credential-approval paths previously called CredentialHelper.Approve
as soon as the retried request returned 2xx — before RequestInfoRefs
validated the advertisement content-type and applied its size cap. A
misleading 2xx (e.g. a captive-portal HTML body) would persist the
credentials in the helper even though RequestInfoRefs still returned
an error.

Unify around the existing pendingHelperCreds slot:

  - tryHelperRetry no longer calls Approve directly on a 2xx retry. It
    sets c.Auth (so follow-ups on the same conn reuse the creds) and
    records the creds as pending, leaving approval to the caller.
    Rejection on 401/403/transport-error stays immediate — a definite
    auth failure is unambiguous.

  - resolvePendingHelperCreds gains a success bool. Approve only fires
    when the operation actually succeeded (status 2xx *and* body
    validation passed); Reject still fires on 401/403; everything else
    leaves the creds pending and c.Auth in place.

  - RequestInfoRefs now reads/validates first (httpError, content-type,
    redirect, size cap — extracted into readInfoRefsResponse) and only
    then settles helper state with success=(err==nil).

  - PostRPCStreamBody settles with success=(httpError==nil), which is
    the full success signal for the POST path.

Adds TestRequestInfoRefs_OnUnauthorizedRetry2xxBadContentTypeDoesNotApprove
to pin the new contract: a 2xx retry with a non-advertisement body must
error out and must NOT call Approve (or Reject — it isn't an auth
failure).

Addresses Cursor Bugbot review comment on PR #65: "Premature credential
helper approve".

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

Author: Soph

internal/gitproto/smarthttp.go

@@ -202,9 +202,25 @@ func (c *HTTPConn) RequestInfoRefs(ctx context.Context, service string, gitProto
 	if err != nil {
 		return nil, err
 	}
-	c.resolvePendingHelperCreds(ctx, res)
-
 	defer res.Body.Close()
+
+	data, err := c.readInfoRefsResponse(res, service)
+	// Settle helper credentials on the fully-validated outcome: approve only
+	// once the advertisement parsed and read within limits, reject on 401/403.
+	// Running this after validation stops a misleading 2xx (wrong content-type
+	// or oversized body) from persisting credentials for an operation that
+	// ultimately failed.
+	c.resolvePendingHelperCreds(ctx, res, err == nil)
+	if err != nil {
+		return nil, err
+	}
+	return data, nil
+}
+
+// readInfoRefsResponse validates and reads an /info/refs response: it checks
+// the HTTP status and advertisement content-type, applies any redirect to the
+// endpoint, and reads the body under a size cap. The caller closes res.Body.
+func (c *HTTPConn) readInfoRefsResponse(res *http.Response, service string) ([]byte, error) {
 	if err := httpError(res); err != nil {
 		return nil, err
 	}
@@ -299,10 +315,14 @@ func (c *HTTPConn) PostRPCStreamBody(ctx context.Context, service string, body i
 			return nil, err
 		}
 	}
-	c.resolvePendingHelperCreds(ctx, res)
-	if err := httpError(res); err != nil {
+	httpErr := httpError(res)
+	// Settle helper credentials on the validated status: approve on a 2xx,
+	// reject on 401/403. For the POST path the HTTP status is the whole
+	// success signal — there's no advertisement body to validate further.
+	c.resolvePendingHelperCreds(ctx, res, httpErr == nil)
+	if httpErr != nil {
 		_ = res.Body.Close()
-		return nil, err
+		return nil, httpErr
 	}
 	return res.Body, nil
 }
@@ -360,9 +380,10 @@ func ApplyAuth(req *http.Request, auth AuthMethod) {
 //     servers that 404/405 GET while requiring auth on POST.
 //  3. If the probe gets 401, attach the helper credentials tentatively.
 //     The next real operation (PostRPCStreamBody or RequestInfoRefs)
-//     calls resolvePendingHelperCreds, which Approves them on 2xx or
-//     Rejects them on 401/403 — helper state only changes based on the
-//     actual outcome, never on the probe response alone.
+//     calls resolvePendingHelperCreds, which Approves them only once that
+//     operation fully succeeds or Rejects them on 401/403 — helper state
+//     only changes based on the actual outcome, never on the probe response
+//     alone.
 //
 // If the probe doesn't 401 (200, 404, 405, etc.) we don't attach; the
 // server either accepts anonymous POSTs here or returns ambiguously,
@@ -388,26 +409,35 @@ func (c *HTTPConn) EnsureAuthForService(ctx context.Context, service string) {
 	c.pendingHelperCreds = &helperCreds{user: user, pass: pass, url: challengeURL}
 }
 
-// resolvePendingHelperCreds settles credentials that EnsureAuthForService
-// attached tentatively, based on the outcome of a real operation. Called
-// from RequestInfoRefs and PostRPCStreamBody. No-op if nothing pending.
-func (c *HTTPConn) resolvePendingHelperCreds(ctx context.Context, res *http.Response) {
+// resolvePendingHelperCreds settles credentials that were attached tentatively
+// — either by EnsureAuthForService's probe or by a tryHelperRetry that got a
+// 2xx — based on the fully-validated outcome of a real operation. Called from
+// RequestInfoRefs and PostRPCStreamBody. No-op if nothing is pending.
+//
+// success reports whether the operation actually succeeded (a 2xx whose body
+// also passed any service-specific validation), as opposed to merely returning
+// a 2xx status. We approve only on success, so a misleading 2xx — e.g. an
+// /info/refs response with the wrong content-type or an oversized body — can't
+// persist credentials for an operation the caller still reports as failed.
+func (c *HTTPConn) resolvePendingHelperCreds(ctx context.Context, res *http.Response, success bool) {
 	if c.pendingHelperCreds == nil || c.CredentialHelper == nil {
 		return
 	}
 	creds := c.pendingHelperCreds
 	switch {
-	case res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusMultipleChoices:
+	case success:
 		c.pendingHelperCreds = nil
 		c.CredentialHelper.Approve(ctx, creds.url, creds.user, creds.pass)
 	case res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden:
 		c.pendingHelperCreds = nil
 		c.Auth = nil
 		c.CredentialHelper.Reject(ctx, creds.url, creds.user, creds.pass)
 	}
-	// Other status: leave pending. A later op on this conn may resolve;
-	// the conn is short-lived (one sync), so leftover pending state at
-	// end of life is harmless.
+	// Otherwise (e.g. a 2xx with a malformed/oversized body): leave the creds
+	// pending and c.Auth in place. We must not approve credentials for an
+	// operation that failed validation, but a non-auth failure isn't proof
+	// they're bad either. The conn is short-lived (one sync), so leftover
+	// pending state at end of life is harmless.
 }
 
 // flushPacket is the smart-HTTP pkt-line "flush" marker. A request body
@@ -437,10 +467,12 @@ func (c *HTTPConn) doServiceProbe(ctx context.Context, service string) (*http.Re
 // (explicit auth must surface its own failures rather than be quietly papered
 // over). retry attempts the same request with helper-supplied credentials.
 //
-// On retry success the credentials are stored on c.Auth so follow-up calls
-// on the same connection reuse them. On retry failure (401, 403, or transport
-// error) the helper is told to reject the credentials so a stale stored token
-// self-heals on the next run.
+// On a 2xx retry the credentials are stored on c.Auth (so follow-up calls on
+// the same connection reuse them) and recorded as pending — the caller then
+// approves them via resolvePendingHelperCreds once the response passes full
+// validation, never on the 2xx status alone. On retry failure (401, 403, or
+// transport error) the helper is told to reject the credentials immediately so
+// a stale stored token self-heals on the next run.
 //
 // Caller is responsible for closing the returned response body.
 func (c *HTTPConn) tryHelperRetry(ctx context.Context, res *http.Response, retry func(AuthMethod) (*http.Response, error)) (*http.Response, error) {
@@ -469,8 +501,11 @@ func (c *HTTPConn) tryHelperRetry(ctx context.Context, res *http.Response, retry
 		// surface "Invalid or expired token" as 403 rather than 401.
 		c.CredentialHelper.Reject(ctx, challengeURL, user, pass)
 	case res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusMultipleChoices:
+		// Attach tentatively and defer approval to resolvePendingHelperCreds,
+		// which runs after the caller validates the response body — a 2xx
+		// status alone isn't proof the operation succeeded.
 		c.Auth = retryAuth
-		c.CredentialHelper.Approve(ctx, challengeURL, user, pass)
+		c.pendingHelperCreds = &helperCreds{user: user, pass: pass, url: challengeURL}
 	}
 	return res, nil
 }

internal/gitproto/smarthttp_test.go

@@ -642,6 +642,46 @@ func TestRequestInfoRefs_OnUnauthorizedRetryStill401CallsReject(t *testing.T) {
 	}
 }
 
+// TestRequestInfoRefs_OnUnauthorizedRetry2xxBadContentTypeDoesNotApprove
+// guards the deferred-approval contract: a retry that authenticates (HTTP 200)
+// but returns a non-advertisement body must surface a content-type error and
+// must NOT persist credentials in the helper — the operation didn't actually
+// succeed, so a misleading 2xx shouldn't approve the creds. It's also not an
+// auth failure, so the helper isn't told to reject them either.
+func TestRequestInfoRefs_OnUnauthorizedRetry2xxBadContentTypeDoesNotApprove(t *testing.T) {
+	helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true}
+	attempts := 0
+	conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) {
+		attempts++
+		if attempts == 1 {
+			return newUnauthorizedResponse(req), nil
+		}
+		res := &http.Response{
+			StatusCode: http.StatusOK,
+			Request:    req,
+			Header:     make(http.Header),
+			Body:       io.NopCloser(strings.NewReader("<html>login</html>")),
+		}
+		res.Header.Set("Content-Type", "text/html")
+		return res, nil
+	}))
+	conn.CredentialHelper = helper
+
+	_, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "")
+	if err == nil {
+		t.Fatal("expected content-type error after a 2xx retry with a non-advertisement body")
+	}
+	if !strings.Contains(err.Error(), "unexpected info/refs content-type") {
+		t.Fatalf("error = %v, want content-type error", err)
+	}
+	if got := helper.count("approve"); got != 0 {
+		t.Errorf("must not approve credentials for an operation that failed validation, got %d approve calls", got)
+	}
+	if got := helper.count("reject"); got != 0 {
+		t.Errorf("a 2xx-but-invalid response is not an auth failure, got %d reject calls", got)
+	}
+}
+
 // TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject documents that some
 // token services (notably Cloudflare) return 403 "Invalid or expired token"
 // instead of 401 when stored credentials have expired.