From 15b80ac519ca914fbd2e19208ce0b4fee0d8f88c Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 15:15:43 +0200 Subject: [PATCH 1/6] auth: defer credential helper until 401, match git's behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git-sync used to call `git credential fill` proactively whenever an HTTP endpoint had no explicit auth. Two problems with that: - On hosts the user had never authenticated against, git fell back to an interactive `Username:`/`Password:` prompt — turning git-sync into an interactive command and breaking non-interactive runs (issue #63). - For hosts where the helper *did* have credentials, we'd send a token to public repos that didn't need one — leaking a credential to a request the server hadn't actually challenged. This change makes git-sync follow git's own HTTP auth flow: - `auth.Resolve` no longer consults the credential helper. Anonymous (or explicit token / Entire DB token) is what comes back. - `HTTPConn` gains a `CredentialHelper` interface. On a 401 it calls `Lookup`, retries the request with the returned credentials, and stores the auth on the conn so follow-up `PostRPC` calls reuse it. - `auth.GitCredentialHelper` shells out to `git credential fill / approve / reject` with `GIT_TERMINAL_PROMPT=0`, so a misconfigured helper fails fast rather than blocking on a tty prompt. - On a successful retry we tell the helper `approve`; on 401 *or* 403 (Cloudflare-style "Invalid or expired token") we tell it `reject` so stale credentials self-heal across runs. Tests cover the full lifecycle: anonymous success skips the helper, 401 triggers Lookup + retry + Approve, retry-still-401 and retry-403 both trigger Reject, helper-with-no-credentials surfaces the original 401 cleanly, and explicit auth disables the helper fallback entirely. Closes #63. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 84b7af2c6388 --- internal/auth/auth.go | 126 ++++++++--- internal/auth/auth_test.go | 255 ++++++++++++++++++--- internal/gitproto/smarthttp.go | 107 +++++++-- internal/gitproto/smarthttp_test.go | 335 ++++++++++++++++++++++++++++ internal/syncer/auth_test.go | 16 +- internal/syncer/integration_test.go | 17 +- internal/syncer/syncer.go | 6 + 7 files changed, 784 insertions(+), 78 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 02778eda..efcf8902 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/url" + "os" "os/exec" "strings" @@ -29,15 +30,13 @@ type Endpoint struct { } // Resolve resolves the auth method for the given endpoint configuration. -// Order: explicit flags → Entire DB token → git credential helper → anonymous. +// Order: explicit flags → Entire DB token → anonymous (with the git credential +// helper deferred until the server returns 401, matching git's own behaviour). func Resolve(raw Endpoint, ep *url.URL) (Method, error) { if auth := explicitAuth(raw); auth != nil { return auth, nil } - if ep == nil { - return nil, nil //nolint:nilnil // nil signals no auth method found at this stage - } - if ep.Scheme != "http" && ep.Scheme != "https" { + if !isHTTPEndpoint(ep) { return nil, nil //nolint:nilnil // nil signals no auth method found at this stage } if username, password, ok, err := LookupEntireDBCredential(raw, ep); err != nil { @@ -45,9 +44,12 @@ func Resolve(raw Endpoint, ep *url.URL) (Method, error) { } else if ok { return &transporthttp.BasicAuth{Username: username, Password: password}, nil } - if username, password, ok := lookupGitCredential(ep); ok { - return &transporthttp.BasicAuth{Username: username, Password: password}, nil - } + // Note: we deliberately do not consult the git credential helper here. + // Doing so eagerly would leak stored credentials to public repos that + // don't require auth, and previously caused interactive prompts when + // no helper had credentials (issue #63). The credential helper is now + // consulted on demand when an HTTP request returns 401 — see + // GitCredentialHelper, wired up by the HTTP connection layer. return nil, nil //nolint:nilnil // nil signals no auth method found at this stage } @@ -65,28 +67,65 @@ func explicitAuth(raw Endpoint) Method { return nil } -// GitCredentialFillCommand is replaceable for testing. -var GitCredentialFillCommand = func(ctx context.Context, input string) ([]byte, error) { - cmd := exec.CommandContext(ctx, "git", "credential", "fill") +// newGitCredentialCmd builds the `git credential ` invocation used by +// GitCredentialCommand. Extracted so tests can inspect the command's +// environment without exec'ing git. +func newGitCredentialCmd(ctx context.Context, op, input string) *exec.Cmd { + cmd := exec.CommandContext(ctx, "git", "credential", op) cmd.Stdin = strings.NewReader(input) - return cmd.Output() + // Disable git's interactive terminal prompt fallback. When no credential + // helper has credentials for the host (e.g. a public repo on a server + // the user has never authenticated against), git would otherwise drop + // to an interactive username/password prompt on /dev/tty. git-sync is a + // non-interactive tool — failing here lets us cleanly surface a 401 + // rather than block waiting for input. See issue #63. + cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") + return cmd +} + +// GitCredentialCommand invokes `git credential ` with the given input +// (in the git-credential text format). op is one of "fill", "approve", or +// "reject". Replaceable for testing. +var GitCredentialCommand = func(ctx context.Context, op, input string) ([]byte, error) { + return newGitCredentialCmd(ctx, op, input).Output() } -func lookupGitCredential(ep *url.URL) (string, string, bool) { - input := credentialFillInput(ep) +// GitCredentialHelper bridges Git's credential helper protocol to HTTP auth. +// It looks up credentials on demand (typically in response to a 401) and +// signals back to the helper whether the credentials worked. +// +// Implementations are best-effort: a missing or misbehaving helper must not +// fail the surrounding sync, only deny credentials. Errors from approve/reject +// are silently swallowed since those steps are advisory. +type GitCredentialHelper struct{} + +// Lookup queries the git credential helper for credentials for ep. Returns +// ok=false if no credentials are available (so the caller can surface a +// clean 401 rather than block). +// +//nolint:unparam // err is always nil today but kept for the CredentialHelper interface. +func (GitCredentialHelper) Lookup(ctx context.Context, ep *url.URL) (username, password string, ok bool, err error) { + if !isHTTPEndpoint(ep) { + // git's credential helper protocol only knows about HTTP. + return "", "", false, nil + } + input := credentialInput(ep, "", "") if input == "" { - return "", "", false + return "", "", false, nil } - output, err := GitCredentialFillCommand(context.Background(), input) - if err != nil { - return "", "", false + output, helperErr := GitCredentialCommand(ctx, "fill", input) + if helperErr != nil { + // Helper exited non-zero — typically means "no credentials found" + // or "terminal prompts disabled" (when no helper has creds). Treat + // both as "no credentials available" so the original 401 surfaces. + return "", "", false, nil //nolint:nilerr // intentional: swallow helper failure as "no credentials" } values := parseCredentialOutput(output) - password := values["password"] + password = values["password"] if password == "" { - return "", "", false + return "", "", false, nil } - username := values["username"] + username = values["username"] if username == "" { if ep.User != nil && ep.User.Username() != "" { username = ep.User.Username() @@ -94,10 +133,40 @@ func lookupGitCredential(ep *url.URL) (string, string, bool) { username = defaultGitUsername } } - return username, password, true + return username, password, true, nil +} + +// isHTTPEndpoint reports whether ep is a non-nil HTTP or HTTPS endpoint. +func isHTTPEndpoint(ep *url.URL) bool { + return ep != nil && (ep.Scheme == "http" || ep.Scheme == "https") +} + +// Approve tells the helper the credentials worked, so it can persist them. +// Best-effort: helper failures are swallowed. +func (GitCredentialHelper) Approve(ctx context.Context, ep *url.URL, username, password string) { + input := credentialInput(ep, username, password) + if input == "" { + return + } + _, _ = GitCredentialCommand(ctx, "approve", input) //nolint:errcheck // best-effort signal +} + +// Reject tells the helper the credentials failed, so it can forget them. +// Best-effort: helper failures are swallowed. +func (GitCredentialHelper) Reject(ctx context.Context, ep *url.URL, username, password string) { + input := credentialInput(ep, username, password) + if input == "" { + return + } + _, _ = GitCredentialCommand(ctx, "reject", input) //nolint:errcheck // best-effort signal } -func credentialFillInput(ep *url.URL) string { +// credentialInput builds a git-credential format request body for the given +// endpoint. When username/password are set, they are appended (for use with +// `git credential approve`/`reject`). When both are empty, the result is a +// query body suitable for `git credential fill`. Explicit username overrides +// any user embedded in the endpoint URL. +func credentialInput(ep *url.URL, username, password string) string { if ep == nil || ep.Hostname() == "" { return "" } @@ -106,8 +175,15 @@ func credentialFillInput(ep *url.URL) string { if path := strings.TrimPrefix(ep.Path, "/"); path != "" { fmt.Fprintf(&b, "path=%s\n", path) } - if ep.User != nil && ep.User.Username() != "" { - fmt.Fprintf(&b, "username=%s\n", ep.User.Username()) + user := username + if user == "" && ep.User != nil { + user = ep.User.Username() + } + if user != "" { + fmt.Fprintf(&b, "username=%s\n", user) + } + if password != "" { + fmt.Fprintf(&b, "password=%s\n", password) } b.WriteString("\n") return b.String() diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 1fc69e7d..26b3e2b6 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -119,7 +119,7 @@ func TestTokenExpiredOrExpiring(t *testing.T) { } } -func TestCredentialFillInput(t *testing.T) { +func TestCredentialInput_FillQueryWithEmbeddedUser(t *testing.T) { ep := &url.URL{ Scheme: "https", Host: "github.com", @@ -127,38 +127,66 @@ func TestCredentialFillInput(t *testing.T) { User: url.User("myuser"), } - got := credentialFillInput(ep) + got := credentialInput(ep, "", "") want := "protocol=https\nhost=github.com\npath=owner/repo.git\nusername=myuser\n\n" if got != want { - t.Errorf("credentialFillInput returned:\n%q\nwant:\n%q", got, want) + t.Errorf("credentialInput returned:\n%q\nwant:\n%q", got, want) } } -func TestCredentialFillInputNilEndpoint(t *testing.T) { - got := credentialFillInput(nil) +func TestCredentialInput_NilEndpoint(t *testing.T) { + got := credentialInput(nil, "", "") if got != "" { t.Errorf("expected empty string for nil endpoint, got %q", got) } } -func TestCredentialFillInputEmptyHost(t *testing.T) { +func TestCredentialInput_EmptyHost(t *testing.T) { ep := &url.URL{Scheme: "https"} - got := credentialFillInput(ep) + got := credentialInput(ep, "", "") if got != "" { t.Errorf("expected empty string for empty host, got %q", got) } } -func TestCredentialFillInputNoUser(t *testing.T) { +func TestCredentialInput_FillQueryNoUser(t *testing.T) { ep := &url.URL{ Scheme: "https", Host: "example.com", Path: "/repo.git", } - got := credentialFillInput(ep) + got := credentialInput(ep, "", "") want := "protocol=https\nhost=example.com\npath=repo.git\n\n" if got != want { - t.Errorf("credentialFillInput returned:\n%q\nwant:\n%q", got, want) + t.Errorf("credentialInput returned:\n%q\nwant:\n%q", got, want) + } +} + +func TestCredentialInput_ApproveRejectFormatIncludesUserAndPassword(t *testing.T) { + ep := &url.URL{ + Scheme: "https", + Host: "example.com", + Path: "/owner/repo.git", + } + got := credentialInput(ep, "alice", "s3cret") + want := "protocol=https\nhost=example.com\npath=owner/repo.git\nusername=alice\npassword=s3cret\n\n" + if got != want { + t.Errorf("credentialInput returned:\n%q\nwant:\n%q", got, want) + } +} + +func TestCredentialInput_ExplicitUserOverridesURLUser(t *testing.T) { + ep := &url.URL{ + Scheme: "https", + Host: "example.com", + User: url.User("from-url"), + } + got := credentialInput(ep, "explicit", "") + if !strings.Contains(got, "username=explicit\n") { + t.Errorf("expected explicit username to win, got:\n%q", got) + } + if strings.Contains(got, "username=from-url") { + t.Errorf("URL-embedded username should be overridden, got:\n%q", got) } } @@ -234,7 +262,6 @@ func TestResolve(t *testing.T) { name string raw Endpoint ep *url.URL - mockCred func(ctx context.Context, input string) ([]byte, error) wantType string // "token", "basic", "nil" wantUser string wantPass string @@ -270,29 +297,23 @@ func TestResolve(t *testing.T) { wantType: "nil", }, { - name: "nothing set HTTP endpoint no credential helper returns nil", - raw: Endpoint{}, - ep: ep, - mockCred: func(_ context.Context, _ string) ([]byte, error) { - return nil, errors.New("no helper") - }, + name: "nothing set HTTP endpoint returns nil (defer to helper on 401)", + raw: Endpoint{}, + ep: ep, wantType: "nil", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Save and restore GitCredentialFillCommand. - origCmd := GitCredentialFillCommand - defer func() { GitCredentialFillCommand = origCmd }() - - if tt.mockCred != nil { - GitCredentialFillCommand = tt.mockCred - } else { - // Default mock: no credential helper. - GitCredentialFillCommand = func(_ context.Context, _ string) ([]byte, error) { - return nil, errors.New("no helper") - } + // Resolve must never consult the git credential helper — + // that lookup is deferred to a 401 response. If anything + // here invokes the helper, fail loudly. + origCmd := GitCredentialCommand + defer func() { GitCredentialCommand = origCmd }() + GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + t.Fatalf("unexpected GitCredentialCommand(%q, %q) call during Resolve", op, input) + return nil, nil } // Also ensure ENTIRE_CONFIG_DIR points nowhere so EntireDB lookup @@ -407,6 +428,184 @@ func TestExplicitAuth(t *testing.T) { } } +// recordedCredCall captures one invocation of GitCredentialCommand for assertion. +type recordedCredCall struct { + op string + input string +} + +// withRecordingHelper replaces GitCredentialCommand with one that appends +// each call to calls and delegates to handler for the response. +func withRecordingHelper(t *testing.T, calls *[]recordedCredCall, handler func(op, input string) ([]byte, error)) { + t.Helper() + orig := GitCredentialCommand + t.Cleanup(func() { GitCredentialCommand = orig }) + GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + *calls = append(*calls, recordedCredCall{op: op, input: input}) + if handler == nil { + return nil, nil + } + return handler(op, input) + } +} + +func TestGitCredentialHelper_Lookup_ReturnsCredentials(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com", Path: "/owner/repo.git"} + var calls []recordedCredCall + withRecordingHelper(t, &calls, func(op, _ string) ([]byte, error) { + if op != "fill" { + t.Fatalf("expected fill, got %q", op) + } + return []byte("username=alice\npassword=s3cret\n"), nil + }) + + user, pass, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected ok=true") + } + if user != "alice" || pass != "s3cret" { + t.Errorf("got user=%q pass=%q, want alice/s3cret", user, pass) + } + if len(calls) != 1 { + t.Fatalf("expected 1 helper call, got %d", len(calls)) + } + if !strings.Contains(calls[0].input, "protocol=https\nhost=example.com\n") { + t.Errorf("fill input missing host/protocol:\n%q", calls[0].input) + } +} + +func TestGitCredentialHelper_Lookup_HelperFailsReturnsNotFound(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + withRecordingHelper(t, new([]recordedCredCall), func(_, _ string) ([]byte, error) { + return nil, errors.New("no helper") + }) + + _, _, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Errorf("expected no error when helper has no credentials, got %v", err) + } + if ok { + t.Error("expected ok=false when helper fails") + } +} + +func TestGitCredentialHelper_Lookup_EmptyPasswordReturnsNotFound(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + withRecordingHelper(t, new([]recordedCredCall), func(_, _ string) ([]byte, error) { + return []byte("username=alice\n"), nil + }) + + _, _, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if ok { + t.Error("expected ok=false when password is empty") + } +} + +func TestGitCredentialHelper_Lookup_UsernameFallsBackToGit(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + withRecordingHelper(t, new([]recordedCredCall), func(_, _ string) ([]byte, error) { + return []byte("password=tok\n"), nil + }) + + user, _, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !ok { + t.Fatal("expected ok=true") + } + if user != "git" { + t.Errorf("expected username fallback to 'git', got %q", user) + } +} + +func TestGitCredentialHelper_Lookup_NonHTTPEndpointReturnsNotFound(t *testing.T) { + ep := &url.URL{Scheme: "ssh", Host: "example.com"} + calls := new([]recordedCredCall) + withRecordingHelper(t, calls, func(_, _ string) ([]byte, error) { + return []byte("password=tok\n"), nil + }) + + _, _, ok, err := GitCredentialHelper{}.Lookup(context.Background(), ep) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ok { + t.Error("expected ok=false for SSH endpoint — credential helper protocol is HTTP-only") + } + if len(*calls) != 0 { + t.Errorf("expected no helper call for SSH endpoint, got %d", len(*calls)) + } +} + +func TestGitCredentialHelper_Approve_SendsCredentialsToHelper(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com", Path: "/owner/repo.git"} + var calls []recordedCredCall + withRecordingHelper(t, &calls, nil) + + GitCredentialHelper{}.Approve(context.Background(), ep, "alice", "s3cret") + + if len(calls) != 1 || calls[0].op != "approve" { + t.Fatalf("expected one 'approve' call, got %+v", calls) + } + want := "protocol=https\nhost=example.com\npath=owner/repo.git\nusername=alice\npassword=s3cret\n\n" + if calls[0].input != want { + t.Errorf("approve input:\n%q\nwant:\n%q", calls[0].input, want) + } +} + +func TestGitCredentialHelper_Reject_SendsCredentialsToHelper(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + var calls []recordedCredCall + withRecordingHelper(t, &calls, nil) + + GitCredentialHelper{}.Reject(context.Background(), ep, "alice", "bad") + + if len(calls) != 1 || calls[0].op != "reject" { + t.Fatalf("expected one 'reject' call, got %+v", calls) + } + if !strings.Contains(calls[0].input, "username=alice\npassword=bad\n") { + t.Errorf("reject input missing creds:\n%q", calls[0].input) + } +} + +func TestGitCredentialHelper_ApproveRejectSwallowHelperErrors(t *testing.T) { + ep := &url.URL{Scheme: "https", Host: "example.com"} + withRecordingHelper(t, new([]recordedCredCall), func(_, _ string) ([]byte, error) { + return nil, errors.New("helper unavailable") + }) + + // Best-effort: helper unavailability must not panic or propagate + // (logged at most). The signal-to-helper step is advisory. + GitCredentialHelper{}.Approve(context.Background(), ep, "u", "p") + GitCredentialHelper{}.Reject(context.Background(), ep, "u", "p") +} + +// TestGitCredentialCmdDisablesTerminalPrompt verifies that the real +// `git credential` invocation sets GIT_TERMINAL_PROMPT=0 so it never +// drops into an interactive username/password prompt when no helper +// has credentials. Regression test for issue #63. +func TestGitCredentialCmdDisablesTerminalPrompt(t *testing.T) { + cmd := newGitCredentialCmd(context.Background(), "fill", "protocol=https\nhost=example.com\n\n") + + var found bool + for _, kv := range cmd.Env { + if kv == "GIT_TERMINAL_PROMPT=0" { + found = true + break + } + } + if !found { + t.Errorf("expected GIT_TERMINAL_PROMPT=0 in cmd.Env, got %v", cmd.Env) + } +} + func TestEndpointBaseURL(t *testing.T) { tests := []struct { name string diff --git a/internal/gitproto/smarthttp.go b/internal/gitproto/smarthttp.go index 90c5fe45..886a716d 100644 --- a/internal/gitproto/smarthttp.go +++ b/internal/gitproto/smarthttp.go @@ -70,6 +70,23 @@ type AuthMethod interface { Authorizer(req *http.Request) error } +// CredentialHelper provides on-demand credentials when an HTTP request is +// rejected with 401. Implementations typically wrap git's credential helper +// protocol; see auth.GitCredentialHelper. +// +// Lookup must not block on user interaction — if no credentials are +// available, return ok=false so the surrounding sync can surface a clean +// 401 rather than hang. +// +// Approve and Reject are advisory signals the helper uses to persist or +// forget credentials. Errors are intentionally not returned: failures here +// must not poison the outer request flow. +type CredentialHelper interface { + Lookup(ctx context.Context, ep *url.URL) (username, password string, ok bool, err error) + Approve(ctx context.Context, ep *url.URL, username, password string) + Reject(ctx context.Context, ep *url.URL, username, password string) +} + // HTTPConn represents a connection to a remote Git HTTP endpoint. type HTTPConn struct { Label string @@ -77,6 +94,13 @@ type HTTPConn struct { HTTP *http.Client Auth AuthMethod + // CredentialHelper, if set, is consulted on a 401 response when no + // initial Auth was configured. The retry happens once on /info/refs; + // on success the resolved credentials are stored in Auth for the + // remaining requests on this connection. Setting Auth up front + // disables the helper fallback — explicit auth wins. + CredentialHelper CredentialHelper + // FollowInfoRefsRedirect, when true, rewrites Endpoint.Scheme and // Endpoint.Host to the final URL returned by RequestInfoRefs after // HTTP redirects. Subsequent PostRPC* calls then target the @@ -161,23 +185,45 @@ func RequestInfoRefs(ctx context.Context, conn Conn, service string, gitProtocol // RequestInfoRefs fetches /info/refs for the given service. func (c *HTTPConn) RequestInfoRefs(ctx context.Context, service string, gitProtocol string) ([]byte, error) { - reqURL := fmt.Sprintf("%s/info/refs?service=%s", c.EndpointURL.String(), service) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) + res, err := c.doInfoRefsRequest(ctx, service, gitProtocol, c.Auth) if err != nil { - return nil, fmt.Errorf("create info-refs request: %w", err) - } - req.Header.Set("Accept", "*/*") - req.Header.Set("User-Agent", capability.DefaultAgent()) - req.Header.Set(StatsPhaseHeader, service+" info-refs") - if gitProtocol != "" { - req.Header.Set("Git-Protocol", gitProtocol) + return nil, err } - ApplyAuth(req, c.Auth) - res, err := c.HTTP.Do(req) - if err != nil { - return nil, fmt.Errorf("request info-refs: %w", err) + // On 401, consult the credential helper as a fallback — but only when + // no explicit auth was configured up front. Explicit auth that fails + // is a real error the user needs to see, not something to paper over. + if res.StatusCode == http.StatusUnauthorized && c.Auth == nil && c.CredentialHelper != nil { + user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, c.EndpointURL) + if lookupErr != nil { + _ = res.Body.Close() + return nil, fmt.Errorf("look up credentials: %w", lookupErr) + } + if ok { + _ = res.Body.Close() + retryAuth := basicAuth{username: user, password: pass} + res, err = c.doInfoRefsRequest(ctx, service, gitProtocol, retryAuth) + if err != nil { + c.CredentialHelper.Reject(ctx, c.EndpointURL, user, pass) + return nil, err + } + switch { + case res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden: + // 401: server rejected the credentials. + // 403: some token services (e.g. Cloudflare) return + // "Invalid or expired token" as 403 rather than 401. + // Since we only reach this branch when the initial + // response was 401 (so the server requires auth), + // a 403 on retry means the credentials themselves + // didn't validate — reject them. + c.CredentialHelper.Reject(ctx, c.EndpointURL, user, pass) + case res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusMultipleChoices: + c.Auth = retryAuth + c.CredentialHelper.Approve(ctx, c.EndpointURL, user, pass) + } + } } + defer res.Body.Close() if err := httpError(res); err != nil { return nil, err @@ -287,3 +333,38 @@ func ApplyAuth(req *http.Request, auth AuthMethod) { } _ = auth.Authorizer(req) //nolint:errcheck // BasicAuth and TokenAuth never error; future authorizers should surface 401s instead } + +// doInfoRefsRequest issues a single /info/refs GET and returns the raw +// response. Caller is responsible for closing the body. Extracted so the +// 401-retry path can reissue the same request with different auth. +func (c *HTTPConn) doInfoRefsRequest(ctx context.Context, service, gitProtocol string, auth AuthMethod) (*http.Response, error) { + reqURL := fmt.Sprintf("%s/info/refs?service=%s", c.EndpointURL.String(), service) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) + if err != nil { + return nil, fmt.Errorf("create info-refs request: %w", err) + } + req.Header.Set("Accept", "*/*") + req.Header.Set("User-Agent", capability.DefaultAgent()) + req.Header.Set(StatsPhaseHeader, service+" info-refs") + if gitProtocol != "" { + req.Header.Set("Git-Protocol", gitProtocol) + } + ApplyAuth(req, auth) + res, err := c.HTTP.Do(req) + if err != nil { + return nil, fmt.Errorf("request info-refs: %w", err) + } + return res, nil +} + +// basicAuth is an internal AuthMethod that injects a username/password via +// HTTP Basic auth. Used to wrap credential-helper output for the 401-retry +// path without taking a dependency on go-git's transporthttp package. +type basicAuth struct { + username, password string +} + +func (b basicAuth) Authorizer(req *http.Request) error { + req.SetBasicAuth(b.username, b.password) + return nil +} diff --git a/internal/gitproto/smarthttp_test.go b/internal/gitproto/smarthttp_test.go index 4ba264eb..0f0f1ae8 100644 --- a/internal/gitproto/smarthttp_test.go +++ b/internal/gitproto/smarthttp_test.go @@ -447,6 +447,341 @@ func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } +// newAdvertisementResponse returns a 200 response shaped like a smart-HTTP +// /info/refs advertisement, suitable for round-tripper fakes. +func newAdvertisementResponse(req *http.Request, service string) *http.Response { + res := &http.Response{ + StatusCode: http.StatusOK, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("0000")), + } + res.Header.Set("Content-Type", "application/x-"+service+"-advertisement") + return res +} + +func newUnauthorizedResponse(req *http.Request) *http.Response { + res := &http.Response{ + StatusCode: http.StatusUnauthorized, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("authentication required")), + } + res.Header.Set("WWW-Authenticate", `Basic realm="git"`) + return res +} + +// TestRequestInfoRefs_AnonymousSucceedsWithoutConsultingHelper verifies the +// happy path: when the server accepts an unauthenticated request, we never +// touch the credential helper. +func TestRequestInfoRefs_AnonymousSucceedsWithoutConsultingHelper(t *testing.T) { + helper := &fakeCredentialHelper{lookupOK: true, lookupUser: "x", lookupPass: "y"} + var authHeaders []string + conn := NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", nil, + roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + return newAdvertisementResponse(req, "git-upload-pack"), nil + }), + ) + conn.CredentialHelper = helper + + if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { + t.Fatalf("RequestInfoRefs: %v", err) + } + if helper.lookupCalls != 0 { + t.Errorf("expected 0 helper lookups on anonymous success, got %d", helper.lookupCalls) + } + if len(authHeaders) != 1 || authHeaders[0] != "" { + t.Errorf("expected exactly one anonymous request, got headers %v", authHeaders) + } +} + +// TestRequestInfoRefs_OnUnauthorizedRetriesWithHelperCredentials verifies +// the core fix: a 401 triggers a helper lookup, the request is retried with +// those credentials, the helper is told the creds worked, and the conn +// remembers the credentials for subsequent calls. +func TestRequestInfoRefs_OnUnauthorizedRetriesWithHelperCredentials(t *testing.T) { + helper := &fakeCredentialHelper{lookupUser: "alice", lookupPass: "s3cret", lookupOK: true} + + var authHeaders []string + attempts := 0 + conn := NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", nil, + roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + return newAdvertisementResponse(req, "git-upload-pack"), nil + }), + ) + conn.CredentialHelper = helper + + if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { + t.Fatalf("RequestInfoRefs: %v", err) + } + + if helper.lookupCalls != 1 { + t.Errorf("expected 1 helper lookup, got %d", helper.lookupCalls) + } + if helper.approveCalls != 1 { + t.Errorf("expected 1 approve call on retry success, got %d", helper.approveCalls) + } + if helper.rejectCalls != 0 { + t.Errorf("expected 0 reject calls on retry success, got %d", helper.rejectCalls) + } + if helper.lastApproveUser != "alice" || helper.lastApprovePass != "s3cret" { + t.Errorf("approve called with wrong creds: user=%q pass=%q", helper.lastApproveUser, helper.lastApprovePass) + } + if len(authHeaders) != 2 { + t.Fatalf("expected 2 requests (anon then auth retry), got %d: %v", len(authHeaders), authHeaders) + } + if authHeaders[0] != "" { + t.Errorf("first request should be anonymous, got Authorization=%q", authHeaders[0]) + } + if !strings.HasPrefix(authHeaders[1], "Basic ") { + t.Errorf("retry should have Basic auth header, got %q", authHeaders[1]) + } + if conn.Auth == nil { + t.Error("expected conn.Auth to be stored after successful auth retry") + } +} + +// TestRequestInfoRefs_OnUnauthorizedReusesStoredAuthOnNextCall confirms that +// once a 401 retry succeeds, subsequent requests on the same connection use +// the stored auth instead of going through anonymous → 401 → retry again. +func TestRequestInfoRefs_OnUnauthorizedReusesStoredAuthOnNextCall(t *testing.T) { + helper := &fakeCredentialHelper{lookupUser: "alice", lookupPass: "s3cret", lookupOK: true} + + var authHeaders []string + attempts := 0 + conn := NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", nil, + roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + if req.Method == http.MethodGet { + return newAdvertisementResponse(req, "git-upload-pack"), nil + } + res := &http.Response{ + StatusCode: http.StatusOK, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + } + res.Header.Set("Content-Type", "application/x-git-upload-pack-result") + return res, nil + }), + ) + conn.CredentialHelper = helper + + if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { + t.Fatalf("RequestInfoRefs: %v", err) + } + if _, err := PostRPC(context.Background(), conn, "git-upload-pack", []byte("0000"), false, "phase"); err != nil { + t.Fatalf("PostRPC: %v", err) + } + + if helper.lookupCalls != 1 { + t.Errorf("expected only 1 helper lookup across both requests, got %d", helper.lookupCalls) + } + if len(authHeaders) != 3 { + t.Fatalf("expected 3 requests, got %d: %v", len(authHeaders), authHeaders) + } + if authHeaders[1] == "" || authHeaders[2] == "" { + t.Errorf("retry GET and follow-up POST should both carry auth: %v", authHeaders) + } +} + +// TestRequestInfoRefs_OnUnauthorizedSurfaces401WithoutHelper verifies that +// a 401 without a configured credential helper just returns the error. +func TestRequestInfoRefs_OnUnauthorizedSurfaces401WithoutHelper(t *testing.T) { + conn := NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", nil, + roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + }), + ) + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "401") { + t.Errorf("expected 401 error, got %v", err) + } +} + +// TestRequestInfoRefs_OnUnauthorizedSurfaces401WhenHelperHasNoCredentials +// verifies that an opaque "helper has nothing" response (ok=false) surfaces +// the 401 cleanly — no retry, no approve, no reject. +func TestRequestInfoRefs_OnUnauthorizedSurfaces401WhenHelperHasNoCredentials(t *testing.T) { + helper := &fakeCredentialHelper{lookupOK: false} + conn := NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", nil, + roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + }), + ) + conn.CredentialHelper = helper + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "401") { + t.Errorf("expected 401 error, got %v", err) + } + if helper.lookupCalls != 1 { + t.Errorf("expected 1 lookup attempt, got %d", helper.lookupCalls) + } + if helper.approveCalls != 0 || helper.rejectCalls != 0 { + t.Errorf("expected no approve/reject when helper had no creds, got approve=%d reject=%d", + helper.approveCalls, helper.rejectCalls) + } +} + +// TestRequestInfoRefs_OnUnauthorizedRetryStill401CallsReject verifies that +// when the helper-supplied credentials are themselves rejected by the +// server, we tell the helper so it can forget them. +func TestRequestInfoRefs_OnUnauthorizedRetryStill401CallsReject(t *testing.T) { + helper := &fakeCredentialHelper{lookupUser: "alice", lookupPass: "bad", lookupOK: true} + conn := NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", nil, + roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + }), + ) + conn.CredentialHelper = helper + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if helper.rejectCalls != 1 { + t.Errorf("expected 1 reject call, got %d", helper.rejectCalls) + } + if helper.approveCalls != 0 { + t.Errorf("expected 0 approve calls, got %d", helper.approveCalls) + } + if helper.lastRejectUser != "alice" || helper.lastRejectPass != "bad" { + t.Errorf("reject called with wrong creds: user=%q pass=%q", helper.lastRejectUser, helper.lastRejectPass) + } +} + +// TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject verifies that some +// token services (notably Cloudflare) return 403 "Invalid or expired token" +// instead of 401 when stored credentials have expired. Since we only reach +// the retry path when the initial response was 401 (server required auth), +// a 403 on retry indicates the helper's credentials themselves didn't +// validate — reject them so the next run starts clean. +func TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject(t *testing.T) { + helper := &fakeCredentialHelper{lookupUser: "user", lookupPass: "expired-token", lookupOK: true} + attempts := 0 + conn := NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", nil, + roundTripperFunc(func(req *http.Request) (*http.Response, error) { + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + // Retry with helper creds — server says 403, expired token. + res := &http.Response{ + StatusCode: http.StatusForbidden, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("Invalid or expired token")), + } + return res, nil + }), + ) + conn.CredentialHelper = helper + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if helper.rejectCalls != 1 { + t.Errorf("expected 1 reject call on retry 403, got %d", helper.rejectCalls) + } + if helper.approveCalls != 0 { + t.Errorf("expected 0 approve calls, got %d", helper.approveCalls) + } + if helper.lastRejectUser != "user" || helper.lastRejectPass != "expired-token" { + t.Errorf("reject called with wrong creds: user=%q pass=%q", helper.lastRejectUser, helper.lastRejectPass) + } +} + +// TestRequestInfoRefs_DoesNotRetryWhenConnAlreadyAuthenticated verifies +// that if the caller explicitly configured auth (via Resolve), we don't +// override it with helper credentials on 401 — that's the caller's +// problem to debug, not ours to silently paper over. +func TestRequestInfoRefs_DoesNotRetryWhenConnAlreadyAuthenticated(t *testing.T) { + helper := &fakeCredentialHelper{lookupUser: "alice", lookupPass: "s3cret", lookupOK: true} + initialAuth := &transporthttp.BasicAuth{Username: "explicit", Password: "tok"} + + conn := NewHTTPConn( + &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, + "src", initialAuth, + roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + }), + ) + conn.CredentialHelper = helper + + _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") + if err == nil { + t.Fatal("expected error, got nil") + } + if helper.lookupCalls != 0 { + t.Errorf("expected 0 helper lookups when auth was preconfigured, got %d", helper.lookupCalls) + } +} + +// fakeCredentialHelper is a CredentialHelper used in tests. Configure lookup +// behaviour via lookupUser/lookupPass/lookupOK/lookupErr; the call counters +// let tests assert the helper's Approve/Reject lifecycle was driven correctly. +type fakeCredentialHelper struct { + lookupUser string + lookupPass string + lookupOK bool + lookupErr error + + lookupCalls int + approveCalls int + rejectCalls int + + lastApproveUser, lastApprovePass string + lastRejectUser, lastRejectPass string +} + +func (h *fakeCredentialHelper) Lookup(_ context.Context, _ *url.URL) (string, string, bool, error) { + h.lookupCalls++ + return h.lookupUser, h.lookupPass, h.lookupOK, h.lookupErr +} + +func (h *fakeCredentialHelper) Approve(_ context.Context, _ *url.URL, user, pass string) { + h.approveCalls++ + h.lastApproveUser, h.lastApprovePass = user, pass +} + +func (h *fakeCredentialHelper) Reject(_ context.Context, _ *url.URL, user, pass string) { + h.rejectCalls++ + h.lastRejectUser, h.lastRejectPass = user, pass +} + type roundTripReader struct { remaining int } diff --git a/internal/syncer/auth_test.go b/internal/syncer/auth_test.go index 7e2019a6..6d11dae8 100644 --- a/internal/syncer/auth_test.go +++ b/internal/syncer/auth_test.go @@ -24,10 +24,10 @@ func TestResolveAuthMethodPrefersExplicitToken(t *testing.T) { t.Fatalf("new endpoint: %v", err) } - originalFill := auth.GitCredentialFillCommand - t.Cleanup(func() { auth.GitCredentialFillCommand = originalFill }) - auth.GitCredentialFillCommand = func(_ context.Context, input string) ([]byte, error) { - t.Fatalf("unexpected git credential fill call with input %q", input) + originalCred := auth.GitCredentialCommand + t.Cleanup(func() { auth.GitCredentialCommand = originalCred }) + auth.GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + t.Fatalf("unexpected git credential %s call with input %q", op, input) return nil, nil } @@ -118,10 +118,10 @@ func TestResolveAuthMethodUsesEntireDBStoredToken(t *testing.T) { t.Fatalf("write token: %v", err) } - originalFill := auth.GitCredentialFillCommand - t.Cleanup(func() { auth.GitCredentialFillCommand = originalFill }) - auth.GitCredentialFillCommand = func(_ context.Context, input string) ([]byte, error) { - t.Fatalf("unexpected git credential fill call with input %q", input) + originalCred := auth.GitCredentialCommand + t.Cleanup(func() { auth.GitCredentialCommand = originalCred }) + auth.GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + t.Fatalf("unexpected git credential %s call with input %q", op, input) return nil, nil } diff --git a/internal/syncer/integration_test.go b/internal/syncer/integration_test.go index 43da3598..19a572a6 100644 --- a/internal/syncer/integration_test.go +++ b/internal/syncer/integration_test.go @@ -1700,11 +1700,11 @@ func TestRun_IntegrationUsesGitCredentialHelperFallback(t *testing.T) { defer sourceServer.Close() defer targetServer.Close() - originalFill := auth.GitCredentialFillCommand + originalCred := auth.GitCredentialCommand t.Cleanup(func() { - auth.GitCredentialFillCommand = originalFill + auth.GitCredentialCommand = originalCred }) - auth.GitCredentialFillCommand = func(_ context.Context, input string) ([]byte, error) { + auth.GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { if !strings.Contains(input, "protocol=http\n") { t.Fatalf("expected protocol in credential input, got %q", input) } @@ -1714,7 +1714,16 @@ func TestRun_IntegrationUsesGitCredentialHelperFallback(t *testing.T) { if !strings.Contains(input, "path=repo.git\n") { t.Fatalf("expected repo path in credential input, got %q", input) } - return []byte("username=" + username + "\npassword=" + password + "\n\n"), nil + switch op { + case "fill": + return []byte("username=" + username + "\npassword=" + password + "\n\n"), nil + case "approve", "reject": + // Best-effort signaling — accept and return empty. + return nil, nil + default: + t.Fatalf("unexpected git credential op %q", op) + return nil, nil + } } result, err := Run(context.Background(), Config{ diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index 6f990235..ad31f2e7 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -377,6 +377,12 @@ func newConn(raw Endpoint, label string, stats *statsCollector, httpClient *http client := instrumentHTTPClient(httpClient, raw.SkipTLSVerify, label, stats) conn := gitproto.NewHTTPConnWithClient(ep, label, authMethod, client) conn.FollowInfoRefsRedirect = raw.FollowInfoRefsRedirect + // When no explicit auth is configured, wire up the git credential helper + // as a fallback that fires only on a 401 response. This matches git's + // own behaviour and avoids leaking stored credentials to public repos. + if authMethod == nil { + conn.CredentialHelper = auth.GitCredentialHelper{} + } return conn, nil } From 2477551b8a8101379f9b1c18d4da4b90e76def36 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 17:41:38 +0200 Subject: [PATCH 2/6] code review cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop internal basicAuth duplicate; use transporthttp.BasicAuth directly in the gitproto retry path (the package already pulls in go-git types via the AuthMethod interface contract). - Collapse GitCredentialHelper.Approve/Reject into a single signal() helper; they only differed in the op string. - Type the credential op as auth.CredentialOp with named constants (CredentialOpFill / Approve / Reject), removing magic strings from production code and the integration test switch. - Collapse fakeCredentialHelper's six counters/last-X fields into a single calls []credCall slice with count(op)/last(op) accessors. - Add a newTestConn(t, rt) helper to deduplicate the 8 new tests. - Trim narrating comments throughout (per code review). Kept the CredentialHelper interface contract, the GIT_TERMINAL_PROMPT=0 rationale, the 403-Cloudflare anecdote, and the explicit-auth-wins invariant — these encode non-obvious WHY. Trimmed the rest. No behaviour change; full test suite + race detector + golangci-lint pass clean. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 4f0fc9b99fdc --- internal/auth/auth.go | 85 ++++---- internal/auth/auth_test.go | 40 ++-- internal/gitproto/smarthttp.go | 48 ++--- internal/gitproto/smarthttp_test.go | 314 +++++++++++++--------------- internal/syncer/auth_test.go | 4 +- internal/syncer/integration_test.go | 7 +- internal/syncer/syncer.go | 3 - 7 files changed, 210 insertions(+), 291 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index efcf8902..aead1e09 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -44,12 +44,6 @@ func Resolve(raw Endpoint, ep *url.URL) (Method, error) { } else if ok { return &transporthttp.BasicAuth{Username: username, Password: password}, nil } - // Note: we deliberately do not consult the git credential helper here. - // Doing so eagerly would leak stored credentials to public repos that - // don't require auth, and previously caused interactive prompts when - // no helper had credentials (issue #63). The credential helper is now - // consulted on demand when an HTTP request returns 401 — see - // GitCredentialHelper, wired up by the HTTP connection layer. return nil, nil //nolint:nilnil // nil signals no auth method found at this stage } @@ -67,58 +61,54 @@ func explicitAuth(raw Endpoint) Method { return nil } -// newGitCredentialCmd builds the `git credential ` invocation used by -// GitCredentialCommand. Extracted so tests can inspect the command's -// environment without exec'ing git. -func newGitCredentialCmd(ctx context.Context, op, input string) *exec.Cmd { - cmd := exec.CommandContext(ctx, "git", "credential", op) +// CredentialOp identifies a `git credential` subcommand. +type CredentialOp string + +const ( + CredentialOpFill CredentialOp = "fill" + CredentialOpApprove CredentialOp = "approve" + CredentialOpReject CredentialOp = "reject" +) + +// newGitCredentialCmd builds the `git credential ` invocation. Extracted +// so tests can inspect the command's environment without exec'ing git. +func newGitCredentialCmd(ctx context.Context, op CredentialOp, input string) *exec.Cmd { + cmd := exec.CommandContext(ctx, "git", "credential", string(op)) cmd.Stdin = strings.NewReader(input) - // Disable git's interactive terminal prompt fallback. When no credential - // helper has credentials for the host (e.g. a public repo on a server - // the user has never authenticated against), git would otherwise drop - // to an interactive username/password prompt on /dev/tty. git-sync is a - // non-interactive tool — failing here lets us cleanly surface a 401 - // rather than block waiting for input. See issue #63. + // Suppress git's interactive username/password fallback. Without this, + // a host with no configured helper drops to a /dev/tty prompt and turns + // git-sync into an interactive command (issue #63). cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") return cmd } // GitCredentialCommand invokes `git credential ` with the given input -// (in the git-credential text format). op is one of "fill", "approve", or -// "reject". Replaceable for testing. -var GitCredentialCommand = func(ctx context.Context, op, input string) ([]byte, error) { +// (git-credential text format). Replaceable for testing. +var GitCredentialCommand = func(ctx context.Context, op CredentialOp, input string) ([]byte, error) { return newGitCredentialCmd(ctx, op, input).Output() } // GitCredentialHelper bridges Git's credential helper protocol to HTTP auth. -// It looks up credentials on demand (typically in response to a 401) and -// signals back to the helper whether the credentials worked. -// -// Implementations are best-effort: a missing or misbehaving helper must not -// fail the surrounding sync, only deny credentials. Errors from approve/reject -// are silently swallowed since those steps are advisory. +// Best-effort: a missing or misbehaving helper denies credentials rather +// than failing the surrounding sync. type GitCredentialHelper struct{} // Lookup queries the git credential helper for credentials for ep. Returns -// ok=false if no credentials are available (so the caller can surface a -// clean 401 rather than block). +// ok=false if no credentials are available so the caller can surface a +// clean 401 rather than block. // //nolint:unparam // err is always nil today but kept for the CredentialHelper interface. func (GitCredentialHelper) Lookup(ctx context.Context, ep *url.URL) (username, password string, ok bool, err error) { if !isHTTPEndpoint(ep) { - // git's credential helper protocol only knows about HTTP. return "", "", false, nil } input := credentialInput(ep, "", "") if input == "" { return "", "", false, nil } - output, helperErr := GitCredentialCommand(ctx, "fill", input) + output, helperErr := GitCredentialCommand(ctx, CredentialOpFill, input) if helperErr != nil { - // Helper exited non-zero — typically means "no credentials found" - // or "terminal prompts disabled" (when no helper has creds). Treat - // both as "no credentials available" so the original 401 surfaces. - return "", "", false, nil //nolint:nilerr // intentional: swallow helper failure as "no credentials" + return "", "", false, nil //nolint:nilerr // helper failure means "no credentials available" } values := parseCredentialOutput(output) password = values["password"] @@ -136,29 +126,26 @@ func (GitCredentialHelper) Lookup(ctx context.Context, ep *url.URL) (username, p return username, password, true, nil } -// isHTTPEndpoint reports whether ep is a non-nil HTTP or HTTPS endpoint. -func isHTTPEndpoint(ep *url.URL) bool { - return ep != nil && (ep.Scheme == "http" || ep.Scheme == "https") +// Approve tells the helper the credentials worked. +func (h GitCredentialHelper) Approve(ctx context.Context, ep *url.URL, username, password string) { + h.signal(ctx, CredentialOpApprove, ep, username, password) } -// Approve tells the helper the credentials worked, so it can persist them. -// Best-effort: helper failures are swallowed. -func (GitCredentialHelper) Approve(ctx context.Context, ep *url.URL, username, password string) { - input := credentialInput(ep, username, password) - if input == "" { - return - } - _, _ = GitCredentialCommand(ctx, "approve", input) //nolint:errcheck // best-effort signal +// Reject tells the helper the credentials failed. +func (h GitCredentialHelper) Reject(ctx context.Context, ep *url.URL, username, password string) { + h.signal(ctx, CredentialOpReject, ep, username, password) } -// Reject tells the helper the credentials failed, so it can forget them. -// Best-effort: helper failures are swallowed. -func (GitCredentialHelper) Reject(ctx context.Context, ep *url.URL, username, password string) { +func (GitCredentialHelper) signal(ctx context.Context, op CredentialOp, ep *url.URL, username, password string) { input := credentialInput(ep, username, password) if input == "" { return } - _, _ = GitCredentialCommand(ctx, "reject", input) //nolint:errcheck // best-effort signal + _, _ = GitCredentialCommand(ctx, op, input) //nolint:errcheck // advisory signal; helper failures swallowed +} + +func isHTTPEndpoint(ep *url.URL) bool { + return ep != nil && (ep.Scheme == "http" || ep.Scheme == "https") } // credentialInput builds a git-credential format request body for the given diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 26b3e2b6..ede9aad3 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -311,7 +311,7 @@ func TestResolve(t *testing.T) { // here invokes the helper, fail loudly. origCmd := GitCredentialCommand defer func() { GitCredentialCommand = origCmd }() - GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + GitCredentialCommand = func(_ context.Context, op CredentialOp, input string) ([]byte, error) { t.Fatalf("unexpected GitCredentialCommand(%q, %q) call during Resolve", op, input) return nil, nil } @@ -428,19 +428,16 @@ func TestExplicitAuth(t *testing.T) { } } -// recordedCredCall captures one invocation of GitCredentialCommand for assertion. type recordedCredCall struct { - op string + op CredentialOp input string } -// withRecordingHelper replaces GitCredentialCommand with one that appends -// each call to calls and delegates to handler for the response. -func withRecordingHelper(t *testing.T, calls *[]recordedCredCall, handler func(op, input string) ([]byte, error)) { +func withRecordingHelper(t *testing.T, calls *[]recordedCredCall, handler func(op CredentialOp, input string) ([]byte, error)) { t.Helper() orig := GitCredentialCommand t.Cleanup(func() { GitCredentialCommand = orig }) - GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + GitCredentialCommand = func(_ context.Context, op CredentialOp, input string) ([]byte, error) { *calls = append(*calls, recordedCredCall{op: op, input: input}) if handler == nil { return nil, nil @@ -452,8 +449,8 @@ func withRecordingHelper(t *testing.T, calls *[]recordedCredCall, handler func(o func TestGitCredentialHelper_Lookup_ReturnsCredentials(t *testing.T) { ep := &url.URL{Scheme: "https", Host: "example.com", Path: "/owner/repo.git"} var calls []recordedCredCall - withRecordingHelper(t, &calls, func(op, _ string) ([]byte, error) { - if op != "fill" { + withRecordingHelper(t, &calls, func(op CredentialOp, _ string) ([]byte, error) { + if op != CredentialOpFill { t.Fatalf("expected fill, got %q", op) } return []byte("username=alice\npassword=s3cret\n"), nil @@ -479,7 +476,7 @@ func TestGitCredentialHelper_Lookup_ReturnsCredentials(t *testing.T) { func TestGitCredentialHelper_Lookup_HelperFailsReturnsNotFound(t *testing.T) { ep := &url.URL{Scheme: "https", Host: "example.com"} - withRecordingHelper(t, new([]recordedCredCall), func(_, _ string) ([]byte, error) { + withRecordingHelper(t, new([]recordedCredCall), func(_ CredentialOp, _ string) ([]byte, error) { return nil, errors.New("no helper") }) @@ -494,7 +491,7 @@ func TestGitCredentialHelper_Lookup_HelperFailsReturnsNotFound(t *testing.T) { func TestGitCredentialHelper_Lookup_EmptyPasswordReturnsNotFound(t *testing.T) { ep := &url.URL{Scheme: "https", Host: "example.com"} - withRecordingHelper(t, new([]recordedCredCall), func(_, _ string) ([]byte, error) { + withRecordingHelper(t, new([]recordedCredCall), func(_ CredentialOp, _ string) ([]byte, error) { return []byte("username=alice\n"), nil }) @@ -509,7 +506,7 @@ func TestGitCredentialHelper_Lookup_EmptyPasswordReturnsNotFound(t *testing.T) { func TestGitCredentialHelper_Lookup_UsernameFallsBackToGit(t *testing.T) { ep := &url.URL{Scheme: "https", Host: "example.com"} - withRecordingHelper(t, new([]recordedCredCall), func(_, _ string) ([]byte, error) { + withRecordingHelper(t, new([]recordedCredCall), func(_ CredentialOp, _ string) ([]byte, error) { return []byte("password=tok\n"), nil }) @@ -528,7 +525,7 @@ func TestGitCredentialHelper_Lookup_UsernameFallsBackToGit(t *testing.T) { func TestGitCredentialHelper_Lookup_NonHTTPEndpointReturnsNotFound(t *testing.T) { ep := &url.URL{Scheme: "ssh", Host: "example.com"} calls := new([]recordedCredCall) - withRecordingHelper(t, calls, func(_, _ string) ([]byte, error) { + withRecordingHelper(t, calls, func(_ CredentialOp, _ string) ([]byte, error) { return []byte("password=tok\n"), nil }) @@ -551,7 +548,7 @@ func TestGitCredentialHelper_Approve_SendsCredentialsToHelper(t *testing.T) { GitCredentialHelper{}.Approve(context.Background(), ep, "alice", "s3cret") - if len(calls) != 1 || calls[0].op != "approve" { + if len(calls) != 1 || calls[0].op != CredentialOpApprove { t.Fatalf("expected one 'approve' call, got %+v", calls) } want := "protocol=https\nhost=example.com\npath=owner/repo.git\nusername=alice\npassword=s3cret\n\n" @@ -567,7 +564,7 @@ func TestGitCredentialHelper_Reject_SendsCredentialsToHelper(t *testing.T) { GitCredentialHelper{}.Reject(context.Background(), ep, "alice", "bad") - if len(calls) != 1 || calls[0].op != "reject" { + if len(calls) != 1 || calls[0].op != CredentialOpReject { t.Fatalf("expected one 'reject' call, got %+v", calls) } if !strings.Contains(calls[0].input, "username=alice\npassword=bad\n") { @@ -577,22 +574,19 @@ func TestGitCredentialHelper_Reject_SendsCredentialsToHelper(t *testing.T) { func TestGitCredentialHelper_ApproveRejectSwallowHelperErrors(t *testing.T) { ep := &url.URL{Scheme: "https", Host: "example.com"} - withRecordingHelper(t, new([]recordedCredCall), func(_, _ string) ([]byte, error) { + withRecordingHelper(t, new([]recordedCredCall), func(_ CredentialOp, _ string) ([]byte, error) { return nil, errors.New("helper unavailable") }) - // Best-effort: helper unavailability must not panic or propagate - // (logged at most). The signal-to-helper step is advisory. + // Approve/Reject must not panic when the helper is broken. GitCredentialHelper{}.Approve(context.Background(), ep, "u", "p") GitCredentialHelper{}.Reject(context.Background(), ep, "u", "p") } -// TestGitCredentialCmdDisablesTerminalPrompt verifies that the real -// `git credential` invocation sets GIT_TERMINAL_PROMPT=0 so it never -// drops into an interactive username/password prompt when no helper -// has credentials. Regression test for issue #63. +// TestGitCredentialCmdDisablesTerminalPrompt is a regression test for issue +// #63 — without GIT_TERMINAL_PROMPT=0 git drops into an interactive prompt. func TestGitCredentialCmdDisablesTerminalPrompt(t *testing.T) { - cmd := newGitCredentialCmd(context.Background(), "fill", "protocol=https\nhost=example.com\n\n") + cmd := newGitCredentialCmd(context.Background(), CredentialOpFill, "protocol=https\nhost=example.com\n\n") var found bool for _, kv := range cmd.Env { diff --git a/internal/gitproto/smarthttp.go b/internal/gitproto/smarthttp.go index 886a716d..a5b73d10 100644 --- a/internal/gitproto/smarthttp.go +++ b/internal/gitproto/smarthttp.go @@ -12,6 +12,7 @@ import ( "strings" "github.com/go-git/go-git/v6/plumbing/protocol/capability" + transporthttp "github.com/go-git/go-git/v6/plumbing/transport/http" ) const maxHTTPErrorBody = 64 * 1024 @@ -71,16 +72,10 @@ type AuthMethod interface { } // CredentialHelper provides on-demand credentials when an HTTP request is -// rejected with 401. Implementations typically wrap git's credential helper -// protocol; see auth.GitCredentialHelper. -// -// Lookup must not block on user interaction — if no credentials are -// available, return ok=false so the surrounding sync can surface a clean -// 401 rather than hang. -// -// Approve and Reject are advisory signals the helper uses to persist or -// forget credentials. Errors are intentionally not returned: failures here -// must not poison the outer request flow. +// rejected with 401. Lookup must not block on user interaction — return +// ok=false instead, so the sync can surface a clean 401 rather than hang. +// Approve/Reject are advisory and intentionally have no error return: +// failures must not poison the outer request flow. type CredentialHelper interface { Lookup(ctx context.Context, ep *url.URL) (username, password string, ok bool, err error) Approve(ctx context.Context, ep *url.URL, username, password string) @@ -190,9 +185,9 @@ func (c *HTTPConn) RequestInfoRefs(ctx context.Context, service string, gitProto return nil, err } - // On 401, consult the credential helper as a fallback — but only when - // no explicit auth was configured up front. Explicit auth that fails - // is a real error the user needs to see, not something to paper over. + // On 401, fall back to the credential helper — but only when no + // explicit auth was configured. Explicit auth that fails is a real + // error the user needs to see. if res.StatusCode == http.StatusUnauthorized && c.Auth == nil && c.CredentialHelper != nil { user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, c.EndpointURL) if lookupErr != nil { @@ -201,7 +196,7 @@ func (c *HTTPConn) RequestInfoRefs(ctx context.Context, service string, gitProto } if ok { _ = res.Body.Close() - retryAuth := basicAuth{username: user, password: pass} + retryAuth := &transporthttp.BasicAuth{Username: user, Password: pass} res, err = c.doInfoRefsRequest(ctx, service, gitProtocol, retryAuth) if err != nil { c.CredentialHelper.Reject(ctx, c.EndpointURL, user, pass) @@ -209,13 +204,8 @@ func (c *HTTPConn) RequestInfoRefs(ctx context.Context, service string, gitProto } switch { case res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden: - // 401: server rejected the credentials. - // 403: some token services (e.g. Cloudflare) return - // "Invalid or expired token" as 403 rather than 401. - // Since we only reach this branch when the initial - // response was 401 (so the server requires auth), - // a 403 on retry means the credentials themselves - // didn't validate — reject them. + // 403 included because some token services (e.g. Cloudflare) + // surface "Invalid or expired token" as 403 rather than 401. c.CredentialHelper.Reject(ctx, c.EndpointURL, user, pass) case res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusMultipleChoices: c.Auth = retryAuth @@ -334,9 +324,7 @@ func ApplyAuth(req *http.Request, auth AuthMethod) { _ = auth.Authorizer(req) //nolint:errcheck // BasicAuth and TokenAuth never error; future authorizers should surface 401s instead } -// doInfoRefsRequest issues a single /info/refs GET and returns the raw -// response. Caller is responsible for closing the body. Extracted so the -// 401-retry path can reissue the same request with different auth. +// doInfoRefsRequest issues a single /info/refs GET. Caller closes res.Body. func (c *HTTPConn) doInfoRefsRequest(ctx context.Context, service, gitProtocol string, auth AuthMethod) (*http.Response, error) { reqURL := fmt.Sprintf("%s/info/refs?service=%s", c.EndpointURL.String(), service) req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) @@ -356,15 +344,3 @@ func (c *HTTPConn) doInfoRefsRequest(ctx context.Context, service, gitProtocol s } return res, nil } - -// basicAuth is an internal AuthMethod that injects a username/password via -// HTTP Basic auth. Used to wrap credential-helper output for the 401-retry -// path without taking a dependency on go-git's transporthttp package. -type basicAuth struct { - username, password string -} - -func (b basicAuth) Authorizer(req *http.Request) error { - req.SetBasicAuth(b.username, b.password) - return nil -} diff --git a/internal/gitproto/smarthttp_test.go b/internal/gitproto/smarthttp_test.go index 0f0f1ae8..83557a29 100644 --- a/internal/gitproto/smarthttp_test.go +++ b/internal/gitproto/smarthttp_test.go @@ -447,8 +447,6 @@ func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } -// newAdvertisementResponse returns a 200 response shaped like a smart-HTTP -// /info/refs advertisement, suitable for round-tripper fakes. func newAdvertisementResponse(req *http.Request, service string) *http.Response { res := &http.Response{ StatusCode: http.StatusOK, @@ -471,71 +469,63 @@ func newUnauthorizedResponse(req *http.Request) *http.Response { return res } -// TestRequestInfoRefs_AnonymousSucceedsWithoutConsultingHelper verifies the -// happy path: when the server accepts an unauthenticated request, we never -// touch the credential helper. -func TestRequestInfoRefs_AnonymousSucceedsWithoutConsultingHelper(t *testing.T) { - helper := &fakeCredentialHelper{lookupOK: true, lookupUser: "x", lookupPass: "y"} - var authHeaders []string - conn := NewHTTPConn( +func newTestConn(_ *testing.T, rt http.RoundTripper) *HTTPConn { + return NewHTTPConn( &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, - "src", nil, - roundTripperFunc(func(req *http.Request) (*http.Response, error) { - authHeaders = append(authHeaders, req.Header.Get("Authorization")) - return newAdvertisementResponse(req, "git-upload-pack"), nil - }), + "src", nil, rt, ) +} + +func TestRequestInfoRefs_AnonymousSucceedsWithoutConsultingHelper(t *testing.T) { + helper := &fakeCredentialHelper{user: "x", pass: "y", ok: true} + var authHeaders []string + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + return newAdvertisementResponse(req, "git-upload-pack"), nil + })) conn.CredentialHelper = helper if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { t.Fatalf("RequestInfoRefs: %v", err) } - if helper.lookupCalls != 0 { - t.Errorf("expected 0 helper lookups on anonymous success, got %d", helper.lookupCalls) + if helper.count("lookup") != 0 { + t.Errorf("expected 0 helper lookups on anonymous success, got %d", helper.count("lookup")) } if len(authHeaders) != 1 || authHeaders[0] != "" { t.Errorf("expected exactly one anonymous request, got headers %v", authHeaders) } } -// TestRequestInfoRefs_OnUnauthorizedRetriesWithHelperCredentials verifies -// the core fix: a 401 triggers a helper lookup, the request is retried with -// those credentials, the helper is told the creds worked, and the conn -// remembers the credentials for subsequent calls. func TestRequestInfoRefs_OnUnauthorizedRetriesWithHelperCredentials(t *testing.T) { - helper := &fakeCredentialHelper{lookupUser: "alice", lookupPass: "s3cret", lookupOK: true} + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} var authHeaders []string attempts := 0 - conn := NewHTTPConn( - &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, - "src", nil, - roundTripperFunc(func(req *http.Request) (*http.Response, error) { - authHeaders = append(authHeaders, req.Header.Get("Authorization")) - attempts++ - if attempts == 1 { - return newUnauthorizedResponse(req), nil - } - return newAdvertisementResponse(req, "git-upload-pack"), nil - }), - ) + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + return newAdvertisementResponse(req, "git-upload-pack"), nil + })) conn.CredentialHelper = helper if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { t.Fatalf("RequestInfoRefs: %v", err) } - if helper.lookupCalls != 1 { - t.Errorf("expected 1 helper lookup, got %d", helper.lookupCalls) + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 helper lookup, got %d", got) } - if helper.approveCalls != 1 { - t.Errorf("expected 1 approve call on retry success, got %d", helper.approveCalls) + if got := helper.count("approve"); got != 1 { + t.Errorf("expected 1 approve call, got %d", got) } - if helper.rejectCalls != 0 { - t.Errorf("expected 0 reject calls on retry success, got %d", helper.rejectCalls) + if got := helper.count("reject"); got != 0 { + t.Errorf("expected 0 reject calls, got %d", got) } - if helper.lastApproveUser != "alice" || helper.lastApprovePass != "s3cret" { - t.Errorf("approve called with wrong creds: user=%q pass=%q", helper.lastApproveUser, helper.lastApprovePass) + if last := helper.last("approve"); last == nil || last.user != "alice" || last.pass != "s3cret" { + t.Errorf("approve called with wrong creds: %+v", last) } if len(authHeaders) != 2 { t.Fatalf("expected 2 requests (anon then auth retry), got %d: %v", len(authHeaders), authHeaders) @@ -551,36 +541,29 @@ func TestRequestInfoRefs_OnUnauthorizedRetriesWithHelperCredentials(t *testing.T } } -// TestRequestInfoRefs_OnUnauthorizedReusesStoredAuthOnNextCall confirms that -// once a 401 retry succeeds, subsequent requests on the same connection use -// the stored auth instead of going through anonymous → 401 → retry again. func TestRequestInfoRefs_OnUnauthorizedReusesStoredAuthOnNextCall(t *testing.T) { - helper := &fakeCredentialHelper{lookupUser: "alice", lookupPass: "s3cret", lookupOK: true} + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} var authHeaders []string attempts := 0 - conn := NewHTTPConn( - &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, - "src", nil, - roundTripperFunc(func(req *http.Request) (*http.Response, error) { - authHeaders = append(authHeaders, req.Header.Get("Authorization")) - attempts++ - if attempts == 1 { - return newUnauthorizedResponse(req), nil - } - if req.Method == http.MethodGet { - return newAdvertisementResponse(req, "git-upload-pack"), nil - } - res := &http.Response{ - StatusCode: http.StatusOK, - Request: req, - Header: make(http.Header), - Body: io.NopCloser(strings.NewReader("")), - } - res.Header.Set("Content-Type", "application/x-git-upload-pack-result") - return res, nil - }), - ) + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + if req.Method == http.MethodGet { + return newAdvertisementResponse(req, "git-upload-pack"), nil + } + res := &http.Response{ + StatusCode: http.StatusOK, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + } + res.Header.Set("Content-Type", "application/x-git-upload-pack-result") + return res, nil + })) conn.CredentialHelper = helper if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { @@ -590,8 +573,8 @@ func TestRequestInfoRefs_OnUnauthorizedReusesStoredAuthOnNextCall(t *testing.T) t.Fatalf("PostRPC: %v", err) } - if helper.lookupCalls != 1 { - t.Errorf("expected only 1 helper lookup across both requests, got %d", helper.lookupCalls) + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected only 1 helper lookup across both requests, got %d", got) } if len(authHeaders) != 3 { t.Fatalf("expected 3 requests, got %d: %v", len(authHeaders), authHeaders) @@ -601,16 +584,10 @@ func TestRequestInfoRefs_OnUnauthorizedReusesStoredAuthOnNextCall(t *testing.T) } } -// TestRequestInfoRefs_OnUnauthorizedSurfaces401WithoutHelper verifies that -// a 401 without a configured credential helper just returns the error. func TestRequestInfoRefs_OnUnauthorizedSurfaces401WithoutHelper(t *testing.T) { - conn := NewHTTPConn( - &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, - "src", nil, - roundTripperFunc(func(req *http.Request) (*http.Response, error) { - return newUnauthorizedResponse(req), nil - }), - ) + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") if err == nil { @@ -621,18 +598,11 @@ func TestRequestInfoRefs_OnUnauthorizedSurfaces401WithoutHelper(t *testing.T) { } } -// TestRequestInfoRefs_OnUnauthorizedSurfaces401WhenHelperHasNoCredentials -// verifies that an opaque "helper has nothing" response (ok=false) surfaces -// the 401 cleanly — no retry, no approve, no reject. func TestRequestInfoRefs_OnUnauthorizedSurfaces401WhenHelperHasNoCredentials(t *testing.T) { - helper := &fakeCredentialHelper{lookupOK: false} - conn := NewHTTPConn( - &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, - "src", nil, - roundTripperFunc(func(req *http.Request) (*http.Response, error) { - return newUnauthorizedResponse(req), nil - }), - ) + helper := &fakeCredentialHelper{ok: false} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) conn.CredentialHelper = helper _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") @@ -642,144 +612,140 @@ func TestRequestInfoRefs_OnUnauthorizedSurfaces401WhenHelperHasNoCredentials(t * if !strings.Contains(err.Error(), "401") { t.Errorf("expected 401 error, got %v", err) } - if helper.lookupCalls != 1 { - t.Errorf("expected 1 lookup attempt, got %d", helper.lookupCalls) + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 lookup attempt, got %d", got) } - if helper.approveCalls != 0 || helper.rejectCalls != 0 { - t.Errorf("expected no approve/reject when helper had no creds, got approve=%d reject=%d", - helper.approveCalls, helper.rejectCalls) + if got := helper.count("approve") + helper.count("reject"); got != 0 { + t.Errorf("expected no approve/reject when helper had no creds, got %d", got) } } -// TestRequestInfoRefs_OnUnauthorizedRetryStill401CallsReject verifies that -// when the helper-supplied credentials are themselves rejected by the -// server, we tell the helper so it can forget them. func TestRequestInfoRefs_OnUnauthorizedRetryStill401CallsReject(t *testing.T) { - helper := &fakeCredentialHelper{lookupUser: "alice", lookupPass: "bad", lookupOK: true} - conn := NewHTTPConn( - &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, - "src", nil, - roundTripperFunc(func(req *http.Request) (*http.Response, error) { - return newUnauthorizedResponse(req), nil - }), - ) + helper := &fakeCredentialHelper{user: "alice", pass: "bad", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) conn.CredentialHelper = helper _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") if err == nil { t.Fatal("expected error, got nil") } - if helper.rejectCalls != 1 { - t.Errorf("expected 1 reject call, got %d", helper.rejectCalls) + if got := helper.count("reject"); got != 1 { + t.Errorf("expected 1 reject call, got %d", got) } - if helper.approveCalls != 0 { - t.Errorf("expected 0 approve calls, got %d", helper.approveCalls) + if got := helper.count("approve"); got != 0 { + t.Errorf("expected 0 approve calls, got %d", got) } - if helper.lastRejectUser != "alice" || helper.lastRejectPass != "bad" { - t.Errorf("reject called with wrong creds: user=%q pass=%q", helper.lastRejectUser, helper.lastRejectPass) + if last := helper.last("reject"); last == nil || last.user != "alice" || last.pass != "bad" { + t.Errorf("reject called with wrong creds: %+v", last) } } -// TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject verifies that some +// TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject documents that some // token services (notably Cloudflare) return 403 "Invalid or expired token" -// instead of 401 when stored credentials have expired. Since we only reach -// the retry path when the initial response was 401 (server required auth), -// a 403 on retry indicates the helper's credentials themselves didn't -// validate — reject them so the next run starts clean. +// instead of 401 when stored credentials have expired. func TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject(t *testing.T) { - helper := &fakeCredentialHelper{lookupUser: "user", lookupPass: "expired-token", lookupOK: true} + helper := &fakeCredentialHelper{user: "user", pass: "expired-token", ok: true} attempts := 0 - conn := NewHTTPConn( - &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, - "src", nil, - roundTripperFunc(func(req *http.Request) (*http.Response, error) { - attempts++ - if attempts == 1 { - return newUnauthorizedResponse(req), nil - } - // Retry with helper creds — server says 403, expired token. - res := &http.Response{ - StatusCode: http.StatusForbidden, - Request: req, - Header: make(http.Header), - Body: io.NopCloser(strings.NewReader("Invalid or expired token")), - } - return res, nil - }), - ) + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + attempts++ + if attempts == 1 { + return newUnauthorizedResponse(req), nil + } + res := &http.Response{ + StatusCode: http.StatusForbidden, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("Invalid or expired token")), + } + return res, nil + })) conn.CredentialHelper = helper _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") if err == nil { t.Fatal("expected error, got nil") } - if helper.rejectCalls != 1 { - t.Errorf("expected 1 reject call on retry 403, got %d", helper.rejectCalls) + if got := helper.count("reject"); got != 1 { + t.Errorf("expected 1 reject call on retry 403, got %d", got) } - if helper.approveCalls != 0 { - t.Errorf("expected 0 approve calls, got %d", helper.approveCalls) + if got := helper.count("approve"); got != 0 { + t.Errorf("expected 0 approve calls, got %d", got) } - if helper.lastRejectUser != "user" || helper.lastRejectPass != "expired-token" { - t.Errorf("reject called with wrong creds: user=%q pass=%q", helper.lastRejectUser, helper.lastRejectPass) + if last := helper.last("reject"); last == nil || last.user != "user" || last.pass != "expired-token" { + t.Errorf("reject called with wrong creds: %+v", last) } } -// TestRequestInfoRefs_DoesNotRetryWhenConnAlreadyAuthenticated verifies -// that if the caller explicitly configured auth (via Resolve), we don't -// override it with helper credentials on 401 — that's the caller's -// problem to debug, not ours to silently paper over. +// TestRequestInfoRefs_DoesNotRetryWhenConnAlreadyAuthenticated: explicit auth +// must win over the helper, so users debugging a bad token they passed see +// the real error rather than a silent fallback. func TestRequestInfoRefs_DoesNotRetryWhenConnAlreadyAuthenticated(t *testing.T) { - helper := &fakeCredentialHelper{lookupUser: "alice", lookupPass: "s3cret", lookupOK: true} + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} initialAuth := &transporthttp.BasicAuth{Username: "explicit", Password: "tok"} - conn := NewHTTPConn( - &url.URL{Scheme: "https", Host: "example.com", Path: "/repo.git"}, - "src", initialAuth, - roundTripperFunc(func(req *http.Request) (*http.Response, error) { - return newUnauthorizedResponse(req), nil - }), - ) + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + conn.Auth = initialAuth conn.CredentialHelper = helper _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", "") if err == nil { t.Fatal("expected error, got nil") } - if helper.lookupCalls != 0 { - t.Errorf("expected 0 helper lookups when auth was preconfigured, got %d", helper.lookupCalls) + if got := helper.count("lookup"); got != 0 { + t.Errorf("expected 0 helper lookups when auth was preconfigured, got %d", got) } } -// fakeCredentialHelper is a CredentialHelper used in tests. Configure lookup -// behaviour via lookupUser/lookupPass/lookupOK/lookupErr; the call counters -// let tests assert the helper's Approve/Reject lifecycle was driven correctly. -type fakeCredentialHelper struct { - lookupUser string - lookupPass string - lookupOK bool - lookupErr error +type credCall struct { + op string // "lookup", "approve", "reject" + user string + pass string +} - lookupCalls int - approveCalls int - rejectCalls int +// fakeCredentialHelper is a test CredentialHelper. Set user/pass/ok/err to +// configure Lookup; inspect calls (via count/last) to assert lifecycle. +type fakeCredentialHelper struct { + user, pass string + ok bool + err error - lastApproveUser, lastApprovePass string - lastRejectUser, lastRejectPass string + calls []credCall } func (h *fakeCredentialHelper) Lookup(_ context.Context, _ *url.URL) (string, string, bool, error) { - h.lookupCalls++ - return h.lookupUser, h.lookupPass, h.lookupOK, h.lookupErr + h.calls = append(h.calls, credCall{op: "lookup"}) + return h.user, h.pass, h.ok, h.err } func (h *fakeCredentialHelper) Approve(_ context.Context, _ *url.URL, user, pass string) { - h.approveCalls++ - h.lastApproveUser, h.lastApprovePass = user, pass + h.calls = append(h.calls, credCall{op: "approve", user: user, pass: pass}) } func (h *fakeCredentialHelper) Reject(_ context.Context, _ *url.URL, user, pass string) { - h.rejectCalls++ - h.lastRejectUser, h.lastRejectPass = user, pass + h.calls = append(h.calls, credCall{op: "reject", user: user, pass: pass}) +} + +func (h *fakeCredentialHelper) count(op string) int { + n := 0 + for _, c := range h.calls { + if c.op == op { + n++ + } + } + return n +} + +func (h *fakeCredentialHelper) last(op string) *credCall { + for i := len(h.calls) - 1; i >= 0; i-- { + if h.calls[i].op == op { + return &h.calls[i] + } + } + return nil } type roundTripReader struct { diff --git a/internal/syncer/auth_test.go b/internal/syncer/auth_test.go index 6d11dae8..d22a51ac 100644 --- a/internal/syncer/auth_test.go +++ b/internal/syncer/auth_test.go @@ -26,7 +26,7 @@ func TestResolveAuthMethodPrefersExplicitToken(t *testing.T) { originalCred := auth.GitCredentialCommand t.Cleanup(func() { auth.GitCredentialCommand = originalCred }) - auth.GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + auth.GitCredentialCommand = func(_ context.Context, op auth.CredentialOp, input string) ([]byte, error) { t.Fatalf("unexpected git credential %s call with input %q", op, input) return nil, nil } @@ -120,7 +120,7 @@ func TestResolveAuthMethodUsesEntireDBStoredToken(t *testing.T) { originalCred := auth.GitCredentialCommand t.Cleanup(func() { auth.GitCredentialCommand = originalCred }) - auth.GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + auth.GitCredentialCommand = func(_ context.Context, op auth.CredentialOp, input string) ([]byte, error) { t.Fatalf("unexpected git credential %s call with input %q", op, input) return nil, nil } diff --git a/internal/syncer/integration_test.go b/internal/syncer/integration_test.go index 19a572a6..35487c8e 100644 --- a/internal/syncer/integration_test.go +++ b/internal/syncer/integration_test.go @@ -1704,7 +1704,7 @@ func TestRun_IntegrationUsesGitCredentialHelperFallback(t *testing.T) { t.Cleanup(func() { auth.GitCredentialCommand = originalCred }) - auth.GitCredentialCommand = func(_ context.Context, op, input string) ([]byte, error) { + auth.GitCredentialCommand = func(_ context.Context, op auth.CredentialOp, input string) ([]byte, error) { if !strings.Contains(input, "protocol=http\n") { t.Fatalf("expected protocol in credential input, got %q", input) } @@ -1715,10 +1715,9 @@ func TestRun_IntegrationUsesGitCredentialHelperFallback(t *testing.T) { t.Fatalf("expected repo path in credential input, got %q", input) } switch op { - case "fill": + case auth.CredentialOpFill: return []byte("username=" + username + "\npassword=" + password + "\n\n"), nil - case "approve", "reject": - // Best-effort signaling — accept and return empty. + case auth.CredentialOpApprove, auth.CredentialOpReject: return nil, nil default: t.Fatalf("unexpected git credential op %q", op) diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index ad31f2e7..8b6a9085 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -377,9 +377,6 @@ func newConn(raw Endpoint, label string, stats *statsCollector, httpClient *http client := instrumentHTTPClient(httpClient, raw.SkipTLSVerify, label, stats) conn := gitproto.NewHTTPConnWithClient(ep, label, authMethod, client) conn.FollowInfoRefsRedirect = raw.FollowInfoRefsRedirect - // When no explicit auth is configured, wire up the git credential helper - // as a fallback that fires only on a 401 response. This matches git's - // own behaviour and avoids leaking stored credentials to public repos. if authMethod == nil { conn.CredentialHelper = auth.GitCredentialHelper{} } From 6247df31eefc8f2ae923a64cfe840abc392359a1 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 18:15:03 +0200 Subject: [PATCH 3/6] key helper on the challenged host; retry POSTs too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness gaps caught in review: 1. After a 3xx on /info/refs, the host that actually returns 401 is in res.Request.URL, which can differ from c.EndpointURL. Querying the helper with c.EndpointURL fetched (and on success approved) credentials under the wrong key — a miss on the next run, or a poisoned key if the retry happened to work via a follow-up redirect. New challengeURLFor() preserves the original repo path but swaps in the post-redirect scheme/host before consulting the helper. 2. Helper fallback existed only on GET /info/refs. Servers that allow anonymous discovery but require auth on the actual pack POST (e.g. Gerrit anonymous-readable + authenticated push) would fail hard. PostRPCStreamBody now does the same lookup → retry → approve/reject dance, gated on the body being io.Seeker so we can rewind it for the second attempt. PostRPC / PostRPCStream always pass bytes.NewReader, which is seekable; a caller that hands in a raw non-seekable Reader sees the 401 surface as-is (documented in the doc comment). The retry lifecycle is now in tryHelperRetry, shared between GET and POST. Four new tests cover redirect-host keying and the POST 401-retry permutations. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 991254d4f73a --- internal/gitproto/smarthttp.go | 135 +++++++++++++++++++------- internal/gitproto/smarthttp_test.go | 143 +++++++++++++++++++++++++--- 2 files changed, 232 insertions(+), 46 deletions(-) diff --git a/internal/gitproto/smarthttp.go b/internal/gitproto/smarthttp.go index a5b73d10..0504aeec 100644 --- a/internal/gitproto/smarthttp.go +++ b/internal/gitproto/smarthttp.go @@ -184,34 +184,11 @@ func (c *HTTPConn) RequestInfoRefs(ctx context.Context, service string, gitProto if err != nil { return nil, err } - - // On 401, fall back to the credential helper — but only when no - // explicit auth was configured. Explicit auth that fails is a real - // error the user needs to see. - if res.StatusCode == http.StatusUnauthorized && c.Auth == nil && c.CredentialHelper != nil { - user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, c.EndpointURL) - if lookupErr != nil { - _ = res.Body.Close() - return nil, fmt.Errorf("look up credentials: %w", lookupErr) - } - if ok { - _ = res.Body.Close() - retryAuth := &transporthttp.BasicAuth{Username: user, Password: pass} - res, err = c.doInfoRefsRequest(ctx, service, gitProtocol, retryAuth) - if err != nil { - c.CredentialHelper.Reject(ctx, c.EndpointURL, user, pass) - return nil, err - } - switch { - case res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden: - // 403 included because some token services (e.g. Cloudflare) - // surface "Invalid or expired token" as 403 rather than 401. - c.CredentialHelper.Reject(ctx, c.EndpointURL, user, pass) - case res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusMultipleChoices: - c.Auth = retryAuth - c.CredentialHelper.Approve(ctx, c.EndpointURL, user, pass) - } - } + res, err = c.tryHelperRetry(ctx, res, func(auth AuthMethod) (*http.Response, error) { + return c.doInfoRefsRequest(ctx, service, gitProtocol, auth) + }) + if err != nil { + return nil, err } defer res.Body.Close() @@ -287,7 +264,37 @@ func PostRPCStreamBody(ctx context.Context, conn Conn, service string, body io.R // PostRPCStreamBody sends a POST to the given service using a streaming request body. // Caller must close the returned ReadCloser. +// +// On a 401 we consult the credential helper and retry, mirroring git's +// own behaviour for servers that allow anonymous /info/refs but gate the +// actual upload-pack/receive-pack POST behind auth. Retry is only possible +// when body is an io.Seeker (so we can rewind it); callers that pass a raw +// non-seekable Reader will see the 401 surface as-is. func (c *HTTPConn) PostRPCStreamBody(ctx context.Context, service string, body io.Reader, v2 bool, phase string) (io.ReadCloser, error) { + res, err := c.doPostRPCRequest(ctx, service, body, v2, phase, c.Auth) + if err != nil { + return nil, err + } + if seeker, ok := body.(io.Seeker); ok { + res, err = c.tryHelperRetry(ctx, res, func(auth AuthMethod) (*http.Response, error) { + if _, seekErr := seeker.Seek(0, io.SeekStart); seekErr != nil { + return nil, fmt.Errorf("rewind RPC body for credential-helper retry: %w", seekErr) + } + return c.doPostRPCRequest(ctx, service, body, v2, phase, auth) + }) + if err != nil { + return nil, err + } + } + if err := httpError(res); err != nil { + _ = res.Body.Close() + return nil, err + } + return res.Body, nil +} + +// doPostRPCRequest issues a single POST to /. Caller closes res.Body. +func (c *HTTPConn) doPostRPCRequest(ctx context.Context, service string, body io.Reader, v2 bool, phase string, auth AuthMethod) (*http.Response, error) { reqURL := fmt.Sprintf("%s/%s", c.EndpointURL.String(), service) req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, body) if err != nil { @@ -300,17 +307,12 @@ func (c *HTTPConn) PostRPCStreamBody(ctx context.Context, service string, body i if v2 { req.Header.Set("Git-Protocol", GitProtocolV2) } - ApplyAuth(req, c.Auth) - + ApplyAuth(req, auth) res, err := c.HTTP.Do(req) if err != nil { return nil, fmt.Errorf("post RPC: %w", err) } - if err := httpError(res); err != nil { - _ = res.Body.Close() - return nil, err - } - return res.Body, nil + return res, nil } // ApplyAuth applies the given auth method to an HTTP request. Errors from @@ -324,6 +326,69 @@ func ApplyAuth(req *http.Request, auth AuthMethod) { _ = auth.Authorizer(req) //nolint:errcheck // BasicAuth and TokenAuth never error; future authorizers should surface 401s instead } +// tryHelperRetry handles the 401 → lookup → retry → approve/reject lifecycle +// when a CredentialHelper is configured and no explicit Auth was set up front +// (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. +// +// 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) { + if res.StatusCode != http.StatusUnauthorized || c.Auth != nil || c.CredentialHelper == nil { + return res, nil + } + challengeURL := challengeURLFor(c.EndpointURL, res) + user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, challengeURL) + if lookupErr != nil { + _ = res.Body.Close() + return nil, fmt.Errorf("look up credentials: %w", lookupErr) + } + if !ok { + return res, nil + } + _ = res.Body.Close() + retryAuth := &transporthttp.BasicAuth{Username: user, Password: pass} + res, err := retry(retryAuth) + if err != nil { + c.CredentialHelper.Reject(ctx, challengeURL, user, pass) + return nil, err + } + switch { + case res.StatusCode == http.StatusUnauthorized || res.StatusCode == http.StatusForbidden: + // 403 included because some token services (e.g. Cloudflare) + // 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: + c.Auth = retryAuth + c.CredentialHelper.Approve(ctx, challengeURL, user, pass) + } + return res, nil +} + +// challengeURLFor returns the URL key used to query the credential helper +// for an auth challenge. After a 3xx the actually-challenged host is in +// res.Request.URL, which may differ from c.EndpointURL — using the wrong +// one would query (and possibly approve/reject) credentials under the +// wrong helper key. The original repo path is preserved so the key still +// matches what the user configured. +func challengeURLFor(orig *url.URL, res *http.Response) *url.URL { + if res == nil || res.Request == nil || res.Request.URL == nil { + return orig + } + final := res.Request.URL + if final.Host == orig.Host && final.Scheme == orig.Scheme { + return orig + } + out := *orig + out.Scheme = final.Scheme + out.Host = final.Host + return &out +} + // doInfoRefsRequest issues a single /info/refs GET. Caller closes res.Body. func (c *HTTPConn) doInfoRefsRequest(ctx context.Context, service, gitProtocol string, auth AuthMethod) (*http.Response, error) { reqURL := fmt.Sprintf("%s/info/refs?service=%s", c.EndpointURL.String(), service) diff --git a/internal/gitproto/smarthttp_test.go b/internal/gitproto/smarthttp_test.go index 83557a29..37e7f00a 100644 --- a/internal/gitproto/smarthttp_test.go +++ b/internal/gitproto/smarthttp_test.go @@ -447,14 +447,14 @@ func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } -func newAdvertisementResponse(req *http.Request, service string) *http.Response { +func newAdvertisementResponse(req *http.Request) *http.Response { res := &http.Response{ StatusCode: http.StatusOK, Request: req, Header: make(http.Header), Body: io.NopCloser(strings.NewReader("0000")), } - res.Header.Set("Content-Type", "application/x-"+service+"-advertisement") + res.Header.Set("Content-Type", "application/x-git-upload-pack-advertisement") return res } @@ -481,7 +481,7 @@ func TestRequestInfoRefs_AnonymousSucceedsWithoutConsultingHelper(t *testing.T) var authHeaders []string conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { authHeaders = append(authHeaders, req.Header.Get("Authorization")) - return newAdvertisementResponse(req, "git-upload-pack"), nil + return newAdvertisementResponse(req), nil })) conn.CredentialHelper = helper @@ -507,7 +507,7 @@ func TestRequestInfoRefs_OnUnauthorizedRetriesWithHelperCredentials(t *testing.T if attempts == 1 { return newUnauthorizedResponse(req), nil } - return newAdvertisementResponse(req, "git-upload-pack"), nil + return newAdvertisementResponse(req), nil })) conn.CredentialHelper = helper @@ -553,7 +553,7 @@ func TestRequestInfoRefs_OnUnauthorizedReusesStoredAuthOnNextCall(t *testing.T) return newUnauthorizedResponse(req), nil } if req.Method == http.MethodGet { - return newAdvertisementResponse(req, "git-upload-pack"), nil + return newAdvertisementResponse(req), nil } res := &http.Response{ StatusCode: http.StatusOK, @@ -700,10 +700,131 @@ func TestRequestInfoRefs_DoesNotRetryWhenConnAlreadyAuthenticated(t *testing.T) } } +// TestRequestInfoRefs_OnUnauthorizedAfterRedirectKeysHelperOnFinalHost +// covers the case where /info/refs is 307'd to a different host and the +// replica returns 401: the helper must be queried for the host that +// actually challenged us, not the original endpoint. +func TestRequestInfoRefs_OnUnauthorizedAfterRedirectKeysHelperOnFinalHost(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 { + // Simulate that Go's HTTP client followed a 3xx to replica.example + // before getting the 401 — res.Request.URL is the post-redirect URL. + res := newUnauthorizedResponse(req) + res.Request = &http.Request{URL: &url.URL{ + Scheme: "https", Host: "replica.example", Path: "/repo.git/info/refs", + }} + return res, nil + } + return newAdvertisementResponse(req), nil + })) + conn.CredentialHelper = helper + + if _, err := conn.RequestInfoRefs(context.Background(), "git-upload-pack", ""); err != nil { + t.Fatalf("RequestInfoRefs: %v", err) + } + + lookup := helper.last("lookup") + if lookup == nil { + t.Fatal("expected helper lookup") + } + if !strings.Contains(lookup.url, "replica.example") { + t.Errorf("helper Lookup keyed on %q, want replica.example", lookup.url) + } + if strings.Contains(lookup.url, "/info/refs") { + t.Errorf("helper Lookup URL should carry the repo path, not /info/refs: %q", lookup.url) + } + approve := helper.last("approve") + if approve == nil || !strings.Contains(approve.url, "replica.example") { + t.Errorf("helper Approve keyed on wrong URL: %+v", approve) + } +} + +func TestPostRPC_OnUnauthorizedRetriesWithHelperCredentials(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var authHeaders []string + attempts := 0 + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + 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("")), + } + res.Header.Set("Content-Type", "application/x-git-upload-pack-result") + return res, nil + })) + conn.CredentialHelper = helper + + if _, err := PostRPC(context.Background(), conn, "git-upload-pack", []byte("0000"), false, "phase"); err != nil { + t.Fatalf("PostRPC: %v", err) + } + + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 helper lookup, got %d", got) + } + if got := helper.count("approve"); got != 1 { + t.Errorf("expected 1 approve call, got %d", got) + } + if len(authHeaders) != 2 { + t.Fatalf("expected 2 requests (anon then auth), got %d: %v", len(authHeaders), authHeaders) + } + if authHeaders[0] != "" { + t.Errorf("first POST should be anonymous, got %q", authHeaders[0]) + } + if !strings.HasPrefix(authHeaders[1], "Basic ") { + t.Errorf("retry POST should have Basic auth, got %q", authHeaders[1]) + } + if conn.Auth == nil { + t.Error("expected conn.Auth to be stored after successful POST retry") + } +} + +func TestPostRPC_OnUnauthorizedSurfaces401WithoutHelper(t *testing.T) { + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + + _, err := PostRPC(context.Background(), conn, "git-upload-pack", []byte("0000"), false, "phase") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "401") { + t.Errorf("expected 401 error, got %v", err) + } +} + +func TestPostRPC_OnUnauthorizedRetryStill401CallsReject(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "bad", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + _, err := PostRPC(context.Background(), conn, "git-upload-pack", []byte("0000"), false, "phase") + if err == nil { + t.Fatal("expected error, got nil") + } + if got := helper.count("reject"); got != 1 { + t.Errorf("expected 1 reject call, got %d", got) + } + if got := helper.count("approve"); got != 0 { + t.Errorf("expected 0 approve calls, got %d", got) + } +} + type credCall struct { op string // "lookup", "approve", "reject" user string pass string + url string // the *url.URL passed to the helper, stringified } // fakeCredentialHelper is a test CredentialHelper. Set user/pass/ok/err to @@ -716,17 +837,17 @@ type fakeCredentialHelper struct { calls []credCall } -func (h *fakeCredentialHelper) Lookup(_ context.Context, _ *url.URL) (string, string, bool, error) { - h.calls = append(h.calls, credCall{op: "lookup"}) +func (h *fakeCredentialHelper) Lookup(_ context.Context, ep *url.URL) (string, string, bool, error) { + h.calls = append(h.calls, credCall{op: "lookup", url: ep.String()}) return h.user, h.pass, h.ok, h.err } -func (h *fakeCredentialHelper) Approve(_ context.Context, _ *url.URL, user, pass string) { - h.calls = append(h.calls, credCall{op: "approve", user: user, pass: pass}) +func (h *fakeCredentialHelper) Approve(_ context.Context, ep *url.URL, user, pass string) { + h.calls = append(h.calls, credCall{op: "approve", user: user, pass: pass, url: ep.String()}) } -func (h *fakeCredentialHelper) Reject(_ context.Context, _ *url.URL, user, pass string) { - h.calls = append(h.calls, credCall{op: "reject", user: user, pass: pass}) +func (h *fakeCredentialHelper) Reject(_ context.Context, ep *url.URL, user, pass string) { + h.calls = append(h.calls, credCall{op: "reject", user: user, pass: pass, url: ep.String()}) } func (h *fakeCredentialHelper) count(op string) int { From 6f9c8de65b535e3b9489fa203fcd964e5e09b3fa Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 18:33:42 +0200 Subject: [PATCH 4/6] probe for auth before streaming pack pushes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit's io.Seeker gate didn't help the production push path. internal/gitproto/push.go builds the receive-pack body as io.MultiReader(header, packData), where packData is the live pipe from upload-pack. That body satisfies neither io.Seeker nor any other replayable contract, so PostRPCStreamBody's on-the-fly 401 retry was a no-op for real pushes. Fix: add HTTPConn.EnsureAuthForService(ctx, service) and call it from sendReceivePack before the body is constructed. It issues an anonymous GET to /; if the server 401s, the helper is consulted, retried with credentials, and (on a non-401/403 response) c.Auth is stored so the streaming POST that follows is pre-authenticated. The probe handles 2xx, 404, 405 etc. all as "credentials accepted" — any non-401/403 means the server didn't reject the creds we sent. 405 is the typical response for GET /git-receive-pack on smart-HTTP servers, so this is the common shape. Limitation made explicit in the doc comment: servers that allow GET anonymously but only 401 on POST will slip past this probe. For those, callers must pass explicit credentials (--target-token). Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: f6b8e1586278 --- internal/gitproto/push.go | 8 ++ internal/gitproto/smarthttp.go | 69 +++++++++++++++ internal/gitproto/smarthttp_test.go | 129 ++++++++++++++++++++++++++++ 3 files changed, 206 insertions(+) diff --git a/internal/gitproto/push.go b/internal/gitproto/push.go index 704601da..55b99e83 100644 --- a/internal/gitproto/push.go +++ b/internal/gitproto/push.go @@ -150,6 +150,14 @@ func sendReceivePack( if err := req.Encode(&header); err != nil { return fmt.Errorf("encode update-request: %w", err) } + // The push body is io.MultiReader(header, packData); packData comes + // from a live upload-pack pipe and isn't rewindable, so a mid-stream + // 401 can't trigger PostRPCStreamBody's normal helper retry. Probe + // for auth requirements up front instead — for HTTP conns that have + // a CredentialHelper configured but no Auth resolved yet. + if hc, ok := conn.(*HTTPConn); ok { + hc.EnsureAuthForService(ctx, transport.ReceivePackService) + } body := io.Reader(bytes.NewReader(header.Bytes())) if packData != nil { body = io.MultiReader(body, packData) diff --git a/internal/gitproto/smarthttp.go b/internal/gitproto/smarthttp.go index 0504aeec..4bfc9e14 100644 --- a/internal/gitproto/smarthttp.go +++ b/internal/gitproto/smarthttp.go @@ -326,6 +326,75 @@ func ApplyAuth(req *http.Request, auth AuthMethod) { _ = auth.Authorizer(req) //nolint:errcheck // BasicAuth and TokenAuth never error; future authorizers should surface 401s instead } +// EnsureAuthForService resolves credentials via the helper before a non- +// rewindable request body is committed. It's a no-op when no helper is +// configured or Auth is already set. +// +// Use this from push.go (and other streaming-body POST paths) where the +// body is built from a live upstream stream (e.g. io.MultiReader over a +// pack reader) and so can't be replayed on a mid-stream 401. The probe +// is a GET to /; servers that gate the service on auth return +// 401 here, letting us resolve credentials before the real POST starts. +// +// Probe failures are non-fatal — the subsequent real request will +// surface any actual problem. Servers that allow GET anonymously but +// 401 on POST will still slip past this probe; for those, callers must +// pass explicit credentials. +func (c *HTTPConn) EnsureAuthForService(ctx context.Context, service string) { + if c.Auth != nil || c.CredentialHelper == nil { + return + } + res, err := c.doServiceProbe(ctx, service, nil) + if err != nil { + return + } + if res.StatusCode != http.StatusUnauthorized { + _ = res.Body.Close() + return + } + challengeURL := challengeURLFor(c.EndpointURL, res) + user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, challengeURL) + _ = res.Body.Close() + if lookupErr != nil || !ok { + return + } + retryAuth := &transporthttp.BasicAuth{Username: user, Password: pass} + res, err = c.doServiceProbe(ctx, service, retryAuth) + if err != nil { + c.CredentialHelper.Reject(ctx, challengeURL, user, pass) + return + } + defer res.Body.Close() + switch res.StatusCode { + case http.StatusUnauthorized, http.StatusForbidden: + c.CredentialHelper.Reject(ctx, challengeURL, user, pass) + default: + // Anything else with credentials attached — 2xx, 405 Method Not + // Allowed (the common shape for GET /git-receive-pack), 404, etc. + // — means the server didn't reject the credentials. Trust them + // for the upcoming real POST; if they actually fail there, the + // caller surfaces that 401/403 directly. + c.Auth = retryAuth + c.CredentialHelper.Approve(ctx, challengeURL, user, pass) + } +} + +func (c *HTTPConn) doServiceProbe(ctx context.Context, service string, auth AuthMethod) (*http.Response, error) { + reqURL := fmt.Sprintf("%s/%s", c.EndpointURL.String(), service) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) + if err != nil { + return nil, fmt.Errorf("create auth-probe request: %w", err) + } + req.Header.Set("User-Agent", capability.DefaultAgent()) + req.Header.Set(StatsPhaseHeader, service+" auth-probe") + ApplyAuth(req, auth) + res, err := c.HTTP.Do(req) + if err != nil { + return nil, fmt.Errorf("auth-probe request: %w", err) + } + return res, nil +} + // tryHelperRetry handles the 401 → lookup → retry → approve/reject lifecycle // when a CredentialHelper is configured and no explicit Auth was set up front // (explicit auth must surface its own failures rather than be quietly papered diff --git a/internal/gitproto/smarthttp_test.go b/internal/gitproto/smarthttp_test.go index 37e7f00a..49d258df 100644 --- a/internal/gitproto/smarthttp_test.go +++ b/internal/gitproto/smarthttp_test.go @@ -820,6 +820,135 @@ func TestPostRPC_OnUnauthorizedRetryStill401CallsReject(t *testing.T) { } } +// TestEnsureAuthForService_ResolvesAuthBeforePost simulates a server that +// allows anonymous /info/refs but requires auth on the service endpoint +// itself — the case where the streaming push body (io.MultiReader over +// packData) can't trigger an on-the-fly 401 retry. The probe must +// resolve credentials so the upcoming POST is pre-authenticated. +func TestEnsureAuthForService_ResolvesAuthBeforePost(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var methods []string + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + methods = append(methods, req.Method+" "+req.URL.Path+" auth="+req.Header.Get("Authorization")) + if req.Header.Get("Authorization") == "" { + return newUnauthorizedResponse(req), nil + } + // With creds: server happens to return 405 for GET on the + // receive-pack endpoint (the normal shape for smart-HTTP servers). + return &http.Response{ + StatusCode: http.StatusMethodNotAllowed, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if conn.Auth == nil { + t.Fatal("expected conn.Auth to be set after probe") + } + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 helper lookup, got %d", got) + } + if got := helper.count("approve"); got != 1 { + t.Errorf("expected 1 approve call after non-auth-error retry, got %d", got) + } + if len(methods) != 2 { + t.Errorf("expected 2 probe attempts (anon then authed), got %d: %v", len(methods), methods) + } +} + +func TestEnsureAuthForService_NoHelperIsNoOp(t *testing.T) { + called := false + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + called = true + return newAdvertisementResponse(req), nil + })) + // No CredentialHelper, no Auth. + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if called { + t.Error("expected EnsureAuthForService to be a no-op without a helper") + } + if conn.Auth != nil { + t.Error("expected conn.Auth to remain nil") + } +} + +func TestEnsureAuthForService_AnonymousServiceLeavesAuthNil(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + // Public server: probe succeeds without auth. + return &http.Response{ + StatusCode: http.StatusMethodNotAllowed, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if conn.Auth != nil { + t.Error("expected conn.Auth to remain nil when probe gets non-401") + } + if got := helper.count("lookup"); got != 0 { + t.Errorf("expected 0 helper lookups when probe didn't 401, got %d", got) + } +} + +// TestPostRPCStreamBody_NonSeekableBodyAfterProbeAuth covers the +// production push shape: body is io.MultiReader (not seekable), so the +// in-place retry path is skipped — but EnsureAuthForService having run +// already means c.Auth is set on the first attempt. +func TestPostRPCStreamBody_NonSeekableBodyAfterProbeAuth(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var authHeaders []string + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + authHeaders = append(authHeaders, req.Header.Get("Authorization")) + if req.Header.Get("Authorization") == "" { + return newUnauthorizedResponse(req), nil + } + if req.Method == http.MethodGet { + return &http.Response{ + StatusCode: http.StatusMethodNotAllowed, Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + } + res := &http.Response{ + StatusCode: http.StatusOK, Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + } + res.Header.Set("Content-Type", "application/x-git-receive-pack-result") + return res, nil + })) + conn.CredentialHelper = helper + + // Simulate push.go: probe first, then stream a non-seekable body. + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + body := io.MultiReader(strings.NewReader("0000"), strings.NewReader("")) + reader, err := PostRPCStreamBody(context.Background(), conn, "git-receive-pack", body, false, "phase") + if err != nil { + t.Fatalf("PostRPCStreamBody: %v", err) + } + _ = reader.Close() + + // 3 requests: probe-anon (401), probe-authed (405), POST-authed (200). + if len(authHeaders) != 3 { + t.Fatalf("expected 3 requests, got %d: %v", len(authHeaders), authHeaders) + } + if authHeaders[0] != "" { + t.Errorf("probe should start anonymous, got %q", authHeaders[0]) + } + if authHeaders[2] == "" { + t.Error("real POST should carry auth resolved by the probe") + } +} + type credCall struct { op string // "lookup", "approve", "reject" user string From f80bd6fc581f75a521a419c86bc688b2fb38ce72 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 18:56:00 +0200 Subject: [PATCH 5/6] defer probe credential approval to the real operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous probe logic ran a second authenticated GET / and approved the credentials whenever that response wasn't 401/403. But many servers return 405 to GET /git-receive-pack without ever validating the Authorization header — so a 405 with attached credentials proves nothing. Approving in that case was a false positive: stale helper creds got blessed, c.Auth was set, and the streaming POST that followed skipped the helper retry path. The push then failed with no way to recover the bad helper state. Fix: the probe is now strictly anonymous. Its only job is to detect whether the server requires auth here (the 401 signal). If it does, the helper-supplied credentials are attached to c.Auth tentatively and recorded as pendingHelperCreds. The next real operation (PostRPCStreamBody or RequestInfoRefs) calls resolvePendingHelperCreds on its response: Approve on 2xx, Reject + clear c.Auth on 401/403, no-op otherwise. So helper state only changes when the real operation provides a definitive signal — which is what git itself does. Six new/updated tests: - TentativelyAttachesHelperCredsOnAnonymous401: c.Auth set, no Approve - 405ProbeWithCredsDoesNotPoisonHelper: explicit regression for the bug - RealPostApprovesTentativeCreds: 2xx on the real POST → Approve - RealPostRejectsTentativeCreds: 401 on the real POST → Reject + clear - NoHelperIsNoOp, AnonymousServiceLeavesAuthNil: existing semantics preserved Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: ce0a3f0780c7 --- internal/gitproto/smarthttp.go | 84 ++++++++++------- internal/gitproto/smarthttp_test.go | 134 +++++++++++++++++++--------- 2 files changed, 144 insertions(+), 74 deletions(-) diff --git a/internal/gitproto/smarthttp.go b/internal/gitproto/smarthttp.go index 4bfc9e14..a0f8e019 100644 --- a/internal/gitproto/smarthttp.go +++ b/internal/gitproto/smarthttp.go @@ -113,6 +113,18 @@ type HTTPConn struct { // coordinated writer here so server-side progress lines don't // clobber the in-place ticker frame. ProgressOut io.Writer + + // pendingHelperCreds tracks credentials supplied by the helper via + // EnsureAuthForService but not yet validated against a real operation. + // The next RequestInfoRefs/PostRPCStreamBody approves on 2xx or rejects + // on 401/403, ensuring helper state reflects the actual outcome rather + // than an ambiguous probe response. + pendingHelperCreds *helperCreds +} + +type helperCreds struct { + user, pass string + url *url.URL } // NewHTTPConn creates a new connection to the given endpoint. @@ -190,6 +202,7 @@ func (c *HTTPConn) RequestInfoRefs(ctx context.Context, service string, gitProto if err != nil { return nil, err } + c.resolvePendingHelperCreds(ctx, res) defer res.Body.Close() if err := httpError(res); err != nil { @@ -286,6 +299,7 @@ func (c *HTTPConn) PostRPCStreamBody(ctx context.Context, service string, body i return nil, err } } + c.resolvePendingHelperCreds(ctx, res) if err := httpError(res); err != nil { _ = res.Body.Close() return nil, err @@ -326,20 +340,26 @@ func ApplyAuth(req *http.Request, auth AuthMethod) { _ = auth.Authorizer(req) //nolint:errcheck // BasicAuth and TokenAuth never error; future authorizers should surface 401s instead } -// EnsureAuthForService resolves credentials via the helper before a non- -// rewindable request body is committed. It's a no-op when no helper is -// configured or Auth is already set. +// EnsureAuthForService tentatively attaches helper credentials before a +// non-rewindable request body is committed. It's a no-op when no helper +// is configured or Auth is already set. // -// Use this from push.go (and other streaming-body POST paths) where the -// body is built from a live upstream stream (e.g. io.MultiReader over a -// pack reader) and so can't be replayed on a mid-stream 401. The probe -// is a GET to /; servers that gate the service on auth return -// 401 here, letting us resolve credentials before the real POST starts. +// Used from push.go (and other streaming-body POST paths) where the body +// is built from a live upstream stream (e.g. io.MultiReader over a pack +// reader) and can't be replayed on a mid-stream 401. An anonymous GET +// / probe asks the server whether it requires auth here. If it +// 401s, the helper is consulted and the returned credentials are stored +// in c.Auth tentatively — the next real operation (PostRPCStreamBody or +// RequestInfoRefs) then Approves them on 2xx or Rejects them on 401/403. // -// Probe failures are non-fatal — the subsequent real request will -// surface any actual problem. Servers that allow GET anonymously but -// 401 on POST will still slip past this probe; for those, callers must -// pass explicit credentials. +// Approval is deliberately not done from the probe itself: many servers +// return 405 Method Not Allowed for GET /git-receive-pack without ever +// checking the Authorization header, so a 405 with credentials attached +// proves nothing about credential validity. Letting the real operation +// validate keeps helper state honest. +// +// Servers that allow anonymous GET but only 401 on POST will slip past +// this probe; for those, callers must pass explicit credentials. func (c *HTTPConn) EnsureAuthForService(ctx context.Context, service string) { if c.Auth != nil || c.CredentialHelper == nil { return @@ -348,35 +368,39 @@ func (c *HTTPConn) EnsureAuthForService(ctx context.Context, service string) { if err != nil { return } + defer res.Body.Close() if res.StatusCode != http.StatusUnauthorized { - _ = res.Body.Close() return } challengeURL := challengeURLFor(c.EndpointURL, res) user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, challengeURL) - _ = res.Body.Close() if lookupErr != nil || !ok { return } - retryAuth := &transporthttp.BasicAuth{Username: user, Password: pass} - res, err = c.doServiceProbe(ctx, service, retryAuth) - if err != nil { - c.CredentialHelper.Reject(ctx, challengeURL, user, pass) + c.Auth = &transporthttp.BasicAuth{Username: user, Password: pass} + 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) { + if c.pendingHelperCreds == nil || c.CredentialHelper == nil { return } - defer res.Body.Close() - switch res.StatusCode { - case http.StatusUnauthorized, http.StatusForbidden: - c.CredentialHelper.Reject(ctx, challengeURL, user, pass) - default: - // Anything else with credentials attached — 2xx, 405 Method Not - // Allowed (the common shape for GET /git-receive-pack), 404, etc. - // — means the server didn't reject the credentials. Trust them - // for the upcoming real POST; if they actually fail there, the - // caller surfaces that 401/403 directly. - c.Auth = retryAuth - c.CredentialHelper.Approve(ctx, challengeURL, user, pass) + creds := c.pendingHelperCreds + switch { + case res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusMultipleChoices: + 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. } func (c *HTTPConn) doServiceProbe(ctx context.Context, service string, auth AuthMethod) (*http.Response, error) { diff --git a/internal/gitproto/smarthttp_test.go b/internal/gitproto/smarthttp_test.go index 49d258df..35cc1a6d 100644 --- a/internal/gitproto/smarthttp_test.go +++ b/internal/gitproto/smarthttp_test.go @@ -820,43 +820,37 @@ func TestPostRPC_OnUnauthorizedRetryStill401CallsReject(t *testing.T) { } } -// TestEnsureAuthForService_ResolvesAuthBeforePost simulates a server that -// allows anonymous /info/refs but requires auth on the service endpoint -// itself — the case where the streaming push body (io.MultiReader over -// packData) can't trigger an on-the-fly 401 retry. The probe must -// resolve credentials so the upcoming POST is pre-authenticated. -func TestEnsureAuthForService_ResolvesAuthBeforePost(t *testing.T) { +// TestEnsureAuthForService_TentativelyAttachesHelperCredsOnAnonymous401: +// the anonymous probe gets 401, the helper supplies credentials, and they +// are attached to the conn for the upcoming streaming POST — but NOT +// approved yet. Approval is deferred until the real operation validates +// them; otherwise a server that returns 405 to GET /git-receive-pack +// without checking auth would bless stale creds. +func TestEnsureAuthForService_TentativelyAttachesHelperCredsOnAnonymous401(t *testing.T) { helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} - var methods []string + probes := 0 conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { - methods = append(methods, req.Method+" "+req.URL.Path+" auth="+req.Header.Get("Authorization")) - if req.Header.Get("Authorization") == "" { - return newUnauthorizedResponse(req), nil - } - // With creds: server happens to return 405 for GET on the - // receive-pack endpoint (the normal shape for smart-HTTP servers). - return &http.Response{ - StatusCode: http.StatusMethodNotAllowed, - Request: req, - Header: make(http.Header), - Body: io.NopCloser(strings.NewReader("")), - }, nil + probes++ + return newUnauthorizedResponse(req), nil })) conn.CredentialHelper = helper conn.EnsureAuthForService(context.Background(), "git-receive-pack") if conn.Auth == nil { - t.Fatal("expected conn.Auth to be set after probe") + t.Fatal("expected conn.Auth to be tentatively set from helper") } if got := helper.count("lookup"); got != 1 { t.Errorf("expected 1 helper lookup, got %d", got) } - if got := helper.count("approve"); got != 1 { - t.Errorf("expected 1 approve call after non-auth-error retry, got %d", got) + if got := helper.count("approve"); got != 0 { + t.Errorf("probe must not Approve — let the real operation validate. got %d", got) } - if len(methods) != 2 { - t.Errorf("expected 2 probe attempts (anon then authed), got %d: %v", len(methods), methods) + if got := helper.count("reject"); got != 0 { + t.Errorf("probe must not Reject either. got %d", got) + } + if probes != 1 { + t.Errorf("expected exactly 1 probe (no retry), got %d", probes) } } @@ -866,7 +860,6 @@ func TestEnsureAuthForService_NoHelperIsNoOp(t *testing.T) { called = true return newAdvertisementResponse(req), nil })) - // No CredentialHelper, no Auth. conn.EnsureAuthForService(context.Background(), "git-receive-pack") @@ -881,7 +874,6 @@ func TestEnsureAuthForService_NoHelperIsNoOp(t *testing.T) { func TestEnsureAuthForService_AnonymousServiceLeavesAuthNil(t *testing.T) { helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { - // Public server: probe succeeds without auth. return &http.Response{ StatusCode: http.StatusMethodNotAllowed, Request: req, @@ -901,11 +893,11 @@ func TestEnsureAuthForService_AnonymousServiceLeavesAuthNil(t *testing.T) { } } -// TestPostRPCStreamBody_NonSeekableBodyAfterProbeAuth covers the -// production push shape: body is io.MultiReader (not seekable), so the -// in-place retry path is skipped — but EnsureAuthForService having run -// already means c.Auth is set on the first attempt. -func TestPostRPCStreamBody_NonSeekableBodyAfterProbeAuth(t *testing.T) { +// TestEnsureAuthForService_RealPostApprovesTentativeCreds covers the +// production push shape: probe attaches helper creds tentatively; the +// real POST succeeds, which is the actual proof creds are valid. Only +// then do we Approve in the helper. +func TestEnsureAuthForService_RealPostApprovesTentativeCreds(t *testing.T) { helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} var authHeaders []string conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { @@ -913,12 +905,6 @@ func TestPostRPCStreamBody_NonSeekableBodyAfterProbeAuth(t *testing.T) { if req.Header.Get("Authorization") == "" { return newUnauthorizedResponse(req), nil } - if req.Method == http.MethodGet { - return &http.Response{ - StatusCode: http.StatusMethodNotAllowed, Request: req, Header: make(http.Header), - Body: io.NopCloser(strings.NewReader("")), - }, nil - } res := &http.Response{ StatusCode: http.StatusOK, Request: req, Header: make(http.Header), Body: io.NopCloser(strings.NewReader("")), @@ -928,7 +914,6 @@ func TestPostRPCStreamBody_NonSeekableBodyAfterProbeAuth(t *testing.T) { })) conn.CredentialHelper = helper - // Simulate push.go: probe first, then stream a non-seekable body. conn.EnsureAuthForService(context.Background(), "git-receive-pack") body := io.MultiReader(strings.NewReader("0000"), strings.NewReader("")) reader, err := PostRPCStreamBody(context.Background(), conn, "git-receive-pack", body, false, "phase") @@ -937,15 +922,76 @@ func TestPostRPCStreamBody_NonSeekableBodyAfterProbeAuth(t *testing.T) { } _ = reader.Close() - // 3 requests: probe-anon (401), probe-authed (405), POST-authed (200). - if len(authHeaders) != 3 { - t.Fatalf("expected 3 requests, got %d: %v", len(authHeaders), authHeaders) + // 2 requests: probe-anon (401) + POST-authed (200). No second probe. + if len(authHeaders) != 2 { + t.Fatalf("expected 2 requests, got %d: %v", len(authHeaders), authHeaders) } if authHeaders[0] != "" { - t.Errorf("probe should start anonymous, got %q", authHeaders[0]) + t.Errorf("probe should be anonymous, got %q", authHeaders[0]) + } + if !strings.HasPrefix(authHeaders[1], "Basic ") { + t.Errorf("real POST should carry the helper creds, got %q", authHeaders[1]) + } + if got := helper.count("approve"); got != 1 { + t.Errorf("expected 1 Approve after the real POST succeeded, got %d", got) + } +} + +// TestEnsureAuthForService_RealPostRejectsTentativeCreds: helper supplied +// stale credentials, the real POST 401s, which is the definitive signal +// to reject them and clear c.Auth. +func TestEnsureAuthForService_RealPostRejectsTentativeCreds(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "stale", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + // Every request 401s (helper creds are stale). + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + if conn.Auth == nil { + t.Fatal("expected conn.Auth to be tentatively set after probe 401") } - if authHeaders[2] == "" { - t.Error("real POST should carry auth resolved by the probe") + body := io.MultiReader(strings.NewReader("0000"), strings.NewReader("")) + _, err := PostRPCStreamBody(context.Background(), conn, "git-receive-pack", body, false, "phase") + if err == nil { + t.Fatal("expected error from POST with stale creds") + } + if got := helper.count("reject"); got != 1 { + t.Errorf("expected 1 Reject after POST 401, got %d", got) + } + if got := helper.count("approve"); got != 0 { + t.Errorf("expected 0 Approve calls, got %d", got) + } + if conn.Auth != nil { + t.Error("expected conn.Auth to be cleared after rejecting bad creds") + } +} + +// TestEnsureAuthForService_405ProbeWithCredsDoesNotPoisonHelper is the +// specific regression: previously, a 405 to the authenticated probe was +// interpreted as "creds accepted" and Approve was called — even though +// the server may have rejected the method before reading Authorization. +// The new contract never approves from a probe response at all. +func TestEnsureAuthForService_405ProbeWithCredsDoesNotPoisonHelper(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "stale", ok: true} + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + if req.Header.Get("Authorization") == "" { + return newUnauthorizedResponse(req), nil + } + // Server returns 405 without checking auth. + return &http.Response{ + StatusCode: http.StatusMethodNotAllowed, + Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if got := helper.count("approve"); got != 0 { + t.Errorf("405 probe response must not Approve stale creds (got %d Approve calls)", got) } } From 91a1224aade88d5cbbcca4ac8d7046afa534342f Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 20:12:17 +0200 Subject: [PATCH 6/6] probe with POST so auth-on-POST-only gates are detected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /git-receive-pack doesn't reliably exercise the same auth path as the real push. Servers that 404/405 the GET method while requiring auth on POST were slipping past EnsureAuthForService — the streaming push then 401'd with no way to retry. Cloudflare-style "Invalid or expired token" servers fall in this bucket, as do some Gerrit configurations. Two changes: 1. Probe with POST and the smart-HTTP flush packet "0000" as body — a valid no-op (zero ref updates, zero pack data) by spec. The auth layer challenges this POST identically to the real push, so the 401 signal is reliable. On the anonymous-allowed case the server processes a no-op that touches no ref state. 2. Lookup helper credentials *before* probing, and skip the probe entirely when the helper has nothing to attach. Without this every anonymous sync would do a wasted no-op POST per push. With it, only syncs that have a configured helper with stored credentials for the host pay the round-trip cost. cmd/git-sync's TestMain now overrides auth.GitCredentialCommand with a "no helper configured" stub. Without isolation, the developer's local credential store (osxkeychain etc.) could return cached credentials for 127.0.0.1 left over from a previous test run, turning the probe into a real POST and inflating receive-pack POST counts. The internal/syncer integration tests are unaffected — their server filters by metricPack which the probe's no-op body doesn't carry. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 47657919e49a --- cmd/git-sync/main_test.go | 17 +++++ internal/gitproto/push.go | 3 +- internal/gitproto/smarthttp.go | 55 +++++++++------ internal/gitproto/smarthttp_test.go | 100 +++++++++++++++++++++++++++- 4 files changed, 150 insertions(+), 25 deletions(-) diff --git a/cmd/git-sync/main_test.go b/cmd/git-sync/main_test.go index 532c6f1e..d4d360d5 100644 --- a/cmd/git-sync/main_test.go +++ b/cmd/git-sync/main_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -14,6 +15,7 @@ import ( "testing" "time" + "entire.io/entire/git-sync/internal/auth" "entire.io/entire/git-sync/internal/syncertest" "entire.io/entire/git-sync/unstable" billy "github.com/go-git/go-billy/v6" @@ -29,6 +31,21 @@ import ( "github.com/go-git/go-git/v6/storage/memory" ) +// 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. +// +// Tests that need to exercise helper behaviour explicitly should +// restore auth.GitCredentialCommand in their own setup. +func TestMain(m *testing.M) { + auth.GitCredentialCommand = func(_ context.Context, _ auth.CredentialOp, _ string) ([]byte, error) { + return nil, errors.New("no helper configured (test default)") + } + os.Exit(m.Run()) +} + const testBranch = "master" const modeReplicate = "replicate" diff --git a/internal/gitproto/push.go b/internal/gitproto/push.go index 55b99e83..ae7d1e31 100644 --- a/internal/gitproto/push.go +++ b/internal/gitproto/push.go @@ -153,8 +153,7 @@ func sendReceivePack( // The push body is io.MultiReader(header, packData); packData comes // from a live upload-pack pipe and isn't rewindable, so a mid-stream // 401 can't trigger PostRPCStreamBody's normal helper retry. Probe - // for auth requirements up front instead — for HTTP conns that have - // a CredentialHelper configured but no Auth resolved yet. + // for auth requirements with a same-shape POST first. if hc, ok := conn.(*HTTPConn); ok { hc.EnsureAuthForService(ctx, transport.ReceivePackService) } diff --git a/internal/gitproto/smarthttp.go b/internal/gitproto/smarthttp.go index a0f8e019..89458d4d 100644 --- a/internal/gitproto/smarthttp.go +++ b/internal/gitproto/smarthttp.go @@ -342,29 +342,40 @@ func ApplyAuth(req *http.Request, auth AuthMethod) { // EnsureAuthForService tentatively attaches helper credentials before a // non-rewindable request body is committed. It's a no-op when no helper -// is configured or Auth is already set. +// is configured, when Auth is already set, or when the helper has no +// credentials to offer. // // Used from push.go (and other streaming-body POST paths) where the body // is built from a live upstream stream (e.g. io.MultiReader over a pack -// reader) and can't be replayed on a mid-stream 401. An anonymous GET -// / probe asks the server whether it requires auth here. If it -// 401s, the helper is consulted and the returned credentials are stored -// in c.Auth tentatively — the next real operation (PostRPCStreamBody or -// RequestInfoRefs) then Approves them on 2xx or Rejects them on 401/403. +// reader) and can't be replayed on a mid-stream 401. // -// Approval is deliberately not done from the probe itself: many servers -// return 405 Method Not Allowed for GET /git-receive-pack without ever -// checking the Authorization header, so a 405 with credentials attached -// proves nothing about credential validity. Letting the real operation -// validate keeps helper state honest. +// The flow is: +// 1. Ask the helper if it has credentials for this endpoint. If not, +// bail — no point probing for an auth requirement we can't satisfy. +// (This also keeps anonymous syncs from doing a wasted no-op POST.) +// 2. Probe with a POST to / using the smart-HTTP flush packet +// "0000" as body — a valid no-op (zero ref updates, zero pack data) +// by spec. We probe with POST rather than GET because the auth layer +// may only gate the POST handler; a GET probe would slip past on +// 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. // -// Servers that allow anonymous GET but only 401 on POST will slip past -// this probe; for those, callers must pass explicit credentials. +// If the probe doesn't 401 (200, 404, 405, etc.) we don't attach; the +// server either accepts anonymous POSTs here or returns ambiguously, +// and either way attaching unvalidated credentials could leak them. func (c *HTTPConn) EnsureAuthForService(ctx context.Context, service string) { if c.Auth != nil || c.CredentialHelper == nil { return } - res, err := c.doServiceProbe(ctx, service, nil) + user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, c.EndpointURL) + if lookupErr != nil || !ok { + return + } + res, err := c.doServiceProbe(ctx, service) if err != nil { return } @@ -373,10 +384,6 @@ func (c *HTTPConn) EnsureAuthForService(ctx context.Context, service string) { return } challengeURL := challengeURLFor(c.EndpointURL, res) - user, pass, ok, lookupErr := c.CredentialHelper.Lookup(ctx, challengeURL) - if lookupErr != nil || !ok { - return - } c.Auth = &transporthttp.BasicAuth{Username: user, Password: pass} c.pendingHelperCreds = &helperCreds{user: user, pass: pass, url: challengeURL} } @@ -403,15 +410,21 @@ func (c *HTTPConn) resolvePendingHelperCreds(ctx context.Context, res *http.Resp // end of life is harmless. } -func (c *HTTPConn) doServiceProbe(ctx context.Context, service string, auth AuthMethod) (*http.Response, error) { +// flushPacket is the smart-HTTP pkt-line "flush" marker. A request body +// containing only a flush packet is a valid no-op for both upload-pack +// (no wants/haves) and receive-pack (no ref updates, no pack data). +var flushPacket = []byte("0000") + +func (c *HTTPConn) doServiceProbe(ctx context.Context, service string) (*http.Response, error) { reqURL := fmt.Sprintf("%s/%s", c.EndpointURL.String(), service) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(flushPacket)) if err != nil { return nil, fmt.Errorf("create auth-probe request: %w", err) } + req.Header.Set("Content-Type", fmt.Sprintf("application/x-%s-request", service)) + req.Header.Set("Accept", fmt.Sprintf("application/x-%s-result", service)) req.Header.Set("User-Agent", capability.DefaultAgent()) req.Header.Set(StatsPhaseHeader, service+" auth-probe") - ApplyAuth(req, auth) res, err := c.HTTP.Do(req) if err != nil { return nil, fmt.Errorf("auth-probe request: %w", err) diff --git a/internal/gitproto/smarthttp_test.go b/internal/gitproto/smarthttp_test.go index 35cc1a6d..f4eb3ec6 100644 --- a/internal/gitproto/smarthttp_test.go +++ b/internal/gitproto/smarthttp_test.go @@ -888,8 +888,31 @@ func TestEnsureAuthForService_AnonymousServiceLeavesAuthNil(t *testing.T) { if conn.Auth != nil { t.Error("expected conn.Auth to remain nil when probe gets non-401") } - if got := helper.count("lookup"); got != 0 { - t.Errorf("expected 0 helper lookups when probe didn't 401, got %d", got) + if got := helper.count("approve"); got != 0 { + t.Errorf("must not Approve when probe didn't 401, got %d", got) + } +} + +// TestEnsureAuthForService_SkipsProbeWhenHelperHasNoCredentials 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. +func TestEnsureAuthForService_SkipsProbeWhenHelperHasNoCredentials(t *testing.T) { + helper := &fakeCredentialHelper{ok: false} + called := false + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + called = true + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if called { + t.Error("expected no probe when the helper has no credentials") + } + if conn.Auth != nil { + t.Error("expected conn.Auth to remain nil") } } @@ -968,6 +991,79 @@ func TestEnsureAuthForService_RealPostRejectsTentativeCreds(t *testing.T) { } } +// TestEnsureAuthForService_ProbesWithPOSTAndFlushPacketBody verifies the +// probe uses the same HTTP method as the real operation (POST), with a +// minimal "0000" flush packet body — a valid no-op receive-pack push by +// the smart-HTTP spec. Probing with POST is essential: servers that +// gate only the POST handler (not GET) would otherwise slip past us. +func TestEnsureAuthForService_ProbesWithPOSTAndFlushPacketBody(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var probeMethod string + var probeBody []byte + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + probeMethod = req.Method + if req.Body != nil { + b, err := io.ReadAll(req.Body) + if err != nil { + t.Fatalf("read probe body: %v", err) + } + probeBody = b + } + return newUnauthorizedResponse(req), nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if probeMethod != http.MethodPost { + t.Errorf("expected probe method POST, got %q", probeMethod) + } + if string(probeBody) != "0000" { + t.Errorf("expected probe body to be the flush packet '0000', got %q", probeBody) + } +} + +// TestEnsureAuthForService_DetectsAuthGatedPostEvenWhenGetIsAnonymous: +// the gap a GET-based probe would miss — server returns 404 to GET +// (the receive-pack endpoint isn't a GET resource) but 401 to POST. +// A POST probe correctly detects the auth requirement. +func TestEnsureAuthForService_DetectsAuthGatedPostEvenWhenGetIsAnonymous(t *testing.T) { + helper := &fakeCredentialHelper{user: "alice", pass: "s3cret", ok: true} + var methods []string + conn := newTestConn(t, roundTripperFunc(func(req *http.Request) (*http.Response, error) { + methods = append(methods, req.Method) + if req.Method == http.MethodGet { + return &http.Response{ + StatusCode: http.StatusNotFound, Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + } + // POST: server requires auth. + if req.Header.Get("Authorization") == "" { + return newUnauthorizedResponse(req), nil + } + return &http.Response{ + StatusCode: http.StatusOK, Request: req, Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + }, nil + })) + conn.CredentialHelper = helper + + conn.EnsureAuthForService(context.Background(), "git-receive-pack") + + if conn.Auth == nil { + t.Fatal("expected probe to detect POST auth requirement and attach helper creds") + } + if got := helper.count("lookup"); got != 1 { + t.Errorf("expected 1 helper lookup, got %d", got) + } + for _, m := range methods { + if m == http.MethodGet { + t.Errorf("probe should not use GET — server may serve GET differently than POST") + } + } +} + // TestEnsureAuthForService_405ProbeWithCredsDoesNotPoisonHelper is the // specific regression: previously, a 405 to the authenticated probe was // interpreted as "creds accepted" and Approve was called — even though