diff --git a/internal/git/git.go b/internal/git/git.go index 514ee05..2e3dfb5 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -11,8 +11,10 @@ import ( "errors" "fmt" "log/slog" + "net/url" "os" "path/filepath" + "strings" "github.com/go-git/go-git/v5" gitconfig "github.com/go-git/go-git/v5/config" @@ -61,31 +63,126 @@ func (s *RepositoryService) SyncBareRepository(ctx context.Context, remoteURL, l return fmt.Errorf("unsupported repository URL '%s'. Only http and https clone URLs are allowed.", remoteURL) } + syncedURL := remoteURL + err := mirrorRepository(ctx, remoteURL, localPath, credential, cache) + // A host may serve the modern protocol on one form of a repository URL and + // the older one on the other, and this client cannot read the modern one. + // The same repository is then reachable at the other form, so a failure + // that is only about the protocol is retried there rather than reported. + // The retry becomes the URL the rest of the sync uses, so a host that + // serves LFS on the answered form only is not failed after the mirror + // succeeded. + if alternate := versionTwoAlternate(remoteURL, err); alternate != "" { + slog.Info("Remote answered with Git protocol v2, which this client cannot read; retrying the other URL form.", + "repository", paths.RedactURL(remoteURL), "retrying", paths.RedactURL(alternate)) + if err = mirrorRepository(ctx, alternate, localPath, credential, cache); err == nil { + syncedURL = alternate + } + } + if err != nil { + return err + } + + if includeLFS { + return s.fetchLFS(ctx, syncedURL, localPath, credential) + } + return nil +} + +// mirrorRepository clones or updates the mirror at localPath from remoteURL. +func mirrorRepository(ctx context.Context, remoteURL, localPath string, credential *Credential, cache bool) error { if cache && isBareRepository(localPath) { // Update the existing mirror. The mirror refspec (+refs/*:refs/*) // force-updates rewritten branches and prune drops refs deleted // upstream, so the mirror tracks the remote exactly. if err := fetchMirror(ctx, remoteURL, localPath, credential); err != nil { - if ctx.Err() != nil { - // A shutdown is not a corrupt mirror. Re-cloning here would - // delete the cached mirror and then fail on the same cancelled - // context, leaving nothing behind. + if ctx.Err() != nil || isProtocolVersionTwo(err) { + // A shutdown or a protocol v2 advertisement is not a corrupt + // mirror. Re-cloning would delete a mirror that is intact — and + // on the protocol path it would delete the mirror and then fail + // on the same form, turning a retry that could have fetched + // incrementally into a full clone. return err } slog.Warn("Incremental mirror fetch failed; re-cloning from scratch.", "localPath", localPath, "error", err.Error()) return freshClone(ctx, remoteURL, localPath, credential) } - } else { - if err := freshClone(ctx, remoteURL, localPath, credential); err != nil { - return err - } + return nil } + return freshClone(ctx, remoteURL, localPath, credential) +} - if includeLFS { - return s.fetchLFS(ctx, remoteURL, localPath, credential) +// pktLineTooShort and cannotReadHash are the two fragments go-git's v0/v1 ref +// decoder produces when it is handed a protocol v2 advertisement: the version +// announcement is read as a ref, and "version 2" is too short to be a hash. +// Both are required so an unrelated decode failure cannot be mistaken for it. +// +// The failure is recognised by its text because go-git raises it from deep +// inside the decoder and wraps it in no error this package can match on; there +// is no sentinel to compare against. +const ( + pktLineTooShort = "pkt-line too short" + cannotReadHash = "cannot read hash" +) + +// versionTwoAlternate returns the other spelling of remoteURL when err says the +// remote answered with a protocol v2 advertisement, and "" when there is +// nothing to retry. +// +// A host decides whether a repository is served at its bare path or at the path +// ending in ".git", and some hosts answer one of the two with the modern +// protocol only. Since go-git v5 cannot read that protocol, the other form is +// the same repository reached a way this client understands. A URL with no path +// to alternate has nothing to offer, so it is reported rather than retried. +func versionTwoAlternate(remoteURL string, err error) string { + if err == nil || !isProtocolVersionTwo(err) { + return "" } - return nil + return alternateURLForm(remoteURL) +} + +func isProtocolVersionTwo(err error) bool { + if err == nil { + return false + } + message := err.Error() + return strings.Contains(message, cannotReadHash) && strings.Contains(message, pktLineTooShort) +} + +// alternateURLForm returns remoteURL with its ".git" suffix flipped, or "" when +// there is no path to flip. +// +// The suffix is matched case-insensitively, as the rest of the codebase does, so +// a URL that already ends in ".GIT" is recognised as suffixed rather than given +// a second one. A trailing slash is normalised away first, because otherwise it +// would end up inside the rewritten path. A fragment is dropped: it is a +// client-side marker that never forms part of what is fetched, and carrying it +// onto the other form would append it to a request the server never sent one +// for. A query is kept, since it can be part of what the remote is asked for. +// +// The rewrite is driven by the decoded path, so an escaped separator is no +// longer distinguished from a real one. A repository path containing a literal +// slash cannot be spelled in a git URL anyway — the separator is structural — +// so decoding is the more faithful reading of the URL, not a loss. +func alternateURLForm(remoteURL string) string { + parsed, err := url.Parse(strings.TrimSpace(remoteURL)) + if err != nil { + return "" + } + + withoutSlash := strings.TrimSuffix(parsed.Path, "/") + if withoutSlash == "" { + return "" + } + + parsed.Path = paths.TrimGitSuffix(withoutSlash) + if parsed.Path == withoutSlash { + parsed.Path += ".git" + } + parsed.Fragment = "" + + return parsed.String() } // fetchLFS mirrors the remote's LFS objects. A remote can have Git LFS turned @@ -100,7 +197,7 @@ func (s *RepositoryService) fetchLFS(ctx context.Context, remoteURL, localPath s err := s.lfs.FetchAll(ctx, localPath, remoteURL, username, password) if errors.Is(err, lfs.ErrDisabled) { - slog.Info("Skipped Git LFS fetch because it is disabled on the remote.", "repository", remoteURL) + slog.Info("Skipped Git LFS fetch because it is disabled on the remote.", "repository", paths.RedactURL(remoteURL)) return nil } if err != nil { diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 3fd4c34..3885899 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -28,19 +28,36 @@ import ( // exercised without any external binary or network. const sourceURL = "http://gitbackup.test/source.git" -var testLoader = &armedLoader{repositories: make(map[string]storer.Storer)} +// protocolTwoMessage is the error go-git's v0/v1 decoder reports when a host +// answers with a Git protocol v2 advertisement, built from the same fragments the +// production check matches so the two cannot drift apart. +var protocolTwoMessage = fmt.Sprintf("pkt-line 3: %s, %s (version 2)", cannotReadHash, pktLineTooShort) + +var testLoader = &armedLoader{ + repositories: make(map[string]storer.Storer), + protocolTwo: make(map[string]error), +} // armedLoader serves the registered repositories but can be told to fail, so -// tests can exercise the incremental-fetch failure and self-heal paths. +// tests can exercise the incremental-fetch failure and self-heal paths. It can +// also answer one endpoint the way a host does when it serves Git protocol v2 +// there: go-git's v0/v1 decoder reports the version announcement as a hash it +// cannot read. type armedLoader struct { repositories map[string]storer.Storer fail atomic.Bool + // protocolTwo maps an endpoint to the error its v2 answer produces, so a + // test can serve the modern protocol on one URL form only. + protocolTwo map[string]error } func (l *armedLoader) Load(ep *transport.Endpoint) (storer.Storer, error) { if l.fail.Load() { return nil, errors.New("server unavailable") } + if err, ok := l.protocolTwo[ep.String()]; ok { + return nil, err + } repository, ok := l.repositories[ep.String()] if !ok { return nil, transport.ErrRepositoryNotFound @@ -140,6 +157,155 @@ func TestSyncBareRepositoryRejectsUnsupportedTransport(t *testing.T) { } } +// TestSyncBareRepositoryRetriesTheOtherURLForm covers a host that serves Git +// protocol v2 at one form of a repository URL while the repository itself is +// reachable at the other. This client speaks v0/v1 only, so the v2 answer is not +// something it can read; the same repository at the other form is, so the sync +// is retried there instead of failing the repository. +func TestSyncBareRepositoryRetriesTheOtherURLForm(t *testing.T) { + const configuredURL = "http://gitbackup.test/protocol-two" + const servedURL = configuredURL + ".git" + + source, _ := createSourceRepositoryAt(t, servedURL) + head, err := source.Head() + if err != nil { + t.Fatal(err) + } + + // The configured form answers with the modern protocol, which go-git + // reports the way v5's decoder does. + testLoader.protocolTwo[configuredURL] = errors.New(protocolTwoMessage) + t.Cleanup(func() { delete(testLoader.protocolTwo, configuredURL) }) + + mirrorPath := filepath.Join(t.TempDir(), "repositories", "mirror") + service := NewRepositoryService() + if err := service.SyncBareRepository(context.Background(), configuredURL, mirrorPath, nil, false, false); err != nil { + t.Fatalf("sync should have retried the other URL form: %v", err) + } + if !mirrorHasCommit(t, mirrorPath, head.Hash()) { + t.Error("mirror should contain the source commit from the form that answers") + } +} + +// TestSyncBareRepositoryReportsProtocolTwoWithoutAnAlternate covers a URL with +// no other form to try: the failure is reported rather than retried into the +// same answer. +func TestSyncBareRepositoryReportsProtocolTwoWithoutAnAlternate(t *testing.T) { + const configuredURL = "http://gitbackup.test/" + + testLoader.protocolTwo[configuredURL] = errors.New(protocolTwoMessage) + t.Cleanup(func() { delete(testLoader.protocolTwo, configuredURL) }) + + mirrorPath := filepath.Join(t.TempDir(), "repositories", "mirror") + service := NewRepositoryService() + err := service.SyncBareRepository(context.Background(), configuredURL, mirrorPath, nil, false, false) + if err == nil { + t.Fatal("a host that only answers with protocol v2 should be reported") + } + if !strings.Contains(err.Error(), pktLineTooShort) { + t.Errorf("err = %v, want the protocol failure reported", err) + } +} + +// TestSyncBareRepositoryRetriesWithoutDestroyingTheMirror covers a cached mirror +// whose host starts answering with protocol v2. The mirror is intact and the +// other URL form still serves the repository, so the retry must fetch into the +// mirror rather than re-cloning: a destructive self-heal would delete a usable +// mirror and then fail on the same form first, costing a full clone every run. +func TestSyncBareRepositoryRetriesWithoutDestroyingTheMirror(t *testing.T) { + const configuredURL = "http://gitbackup.test/cached-two" + const servedURL = configuredURL + ".git" + + createSourceRepositoryAt(t, servedURL) + mirrorPath := filepath.Join(t.TempDir(), "repositories", "mirror") + service := NewRepositoryService() + + // Seed the cached mirror through the form that answers. + if err := service.SyncBareRepository(context.Background(), servedURL, mirrorPath, nil, true, false); err != nil { + t.Fatalf("seeding the mirror failed: %v", err) + } + + // A file inside the mirror stands in for its contents: re-cloning removes + // the directory, so its survival is what separates a fetch from a clone. + marker := filepath.Join(mirrorPath, "seeded-marker") + if err := os.WriteFile(marker, []byte("seed"), 0o644); err != nil { + t.Fatalf("write marker: %v", err) + } + + // The host now answers the configured form with the modern protocol while + // the other form still serves the repository. + testLoader.protocolTwo[configuredURL] = errors.New(protocolTwoMessage) + t.Cleanup(func() { delete(testLoader.protocolTwo, configuredURL) }) + + if err := service.SyncBareRepository(context.Background(), configuredURL, mirrorPath, nil, true, false); err != nil { + t.Fatalf("sync should have retried the other URL form: %v", err) + } + if _, err := os.Stat(marker); err != nil { + t.Errorf("the cached mirror was rebuilt instead of fetched into: %v", err) + } + if !isBareRepository(mirrorPath) { + t.Error("the cached mirror should still be a bare repository") + } +} + +// TestAlternateURLForm covers how the other form of a repository URL is derived. +func TestAlternateURLForm(t *testing.T) { + cases := []struct { + name string + remote string + expected string + }{ + {"appends the suffix", "https://host/owner/repo", "https://host/owner/repo.git"}, + {"removes the suffix", "https://host/owner/repo.git", "https://host/owner/repo"}, + {"keeps the port", "https://host:8443/owner/repo", "https://host:8443/owner/repo.git"}, + {"keeps userinfo", "https://user@host/owner/repo", "https://user@host/owner/repo.git"}, + {"keeps a nested path", "https://host/a/b/c/repo", "https://host/a/b/c/repo.git"}, + {"no path to alternate", "https://host", ""}, + {"root path has nothing to alternate", "https://host/", ""}, + {"only the last suffix is flipped", "https://host/owner/repo.git.git", "https://host/owner/repo.git"}, + {"keeps a query", "https://host/owner/repo?foo=1", "https://host/owner/repo.git?foo=1"}, + {"recognises an upper-case suffix", "https://host/owner/repo.GIT", "https://host/owner/repo"}, + {"drops a trailing slash", "https://host/owner/repo/", "https://host/owner/repo.git"}, + {"drops a trailing slash after the suffix", "https://host/owner/repo.git/", "https://host/owner/repo"}, + {"drops a fragment", "https://host/owner/repo#main", "https://host/owner/repo.git"}, + {"keeps an escaped path escaped", "https://host/owner/re%20po", "https://host/owner/re%20po.git"}, + {"decodes an escaped separator", "https://host/owner/re%2Fpo", "https://host/owner/re/po.git"}, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + if got := alternateURLForm(testCase.remote); got != testCase.expected { + t.Errorf("alternateURLForm(%q) = %q, want %q", testCase.remote, got, testCase.expected) + } + }) + } +} + +// TestVersionTwoAlternate covers the decision to retry: only a protocol v2 +// advertisement leaves anything to try, and only when there is another form. +func TestVersionTwoAlternate(t *testing.T) { + protocolTwo := errors.New(protocolTwoMessage) + + cases := []struct { + name string + remote string + err error + expected string + }{ + {"retries the other form", "https://host/owner/repo", protocolTwo, "https://host/owner/repo.git"}, + {"nothing to retry without a path", "https://host/", protocolTwo, ""}, + {"a successful sync is not retried", "https://host/owner/repo", nil, ""}, + {"an unrelated failure is not retried", "https://host/owner/repo", errors.New("connection refused"), ""}, + {"an authentication failure is not retried", "https://host/owner/repo", transport.ErrAuthenticationRequired, ""}, + } + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + if got := versionTwoAlternate(testCase.remote, testCase.err); got != testCase.expected { + t.Errorf("versionTwoAlternate(%q, %v) = %q, want %q", testCase.remote, testCase.err, got, testCase.expected) + } + }) + } +} + func TestSyncBareRepositoryFreshClone(t *testing.T) { source, _ := createSourceRepository(t) head, err := source.Head() diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index 1fff8d5..2341ddc 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -51,6 +51,35 @@ func TestTrimGitSuffix(t *testing.T) { } } +func TestRedactURL(t *testing.T) { + tests := []struct { + name string + raw string + want string + }{ + {name: "masks a password", raw: "https://user:token@host/owner/repo", want: "https://user:xxxxx@host/owner/repo"}, + {name: "masks only the password", raw: "https://user:tok@host:8443/owner/repo.git", want: "https://user:xxxxx@host:8443/owner/repo.git"}, + {name: "keeps a username", raw: "https://user@host/owner/repo", want: "https://user@host/owner/repo"}, + {name: "leaves a url without userinfo alone", raw: "https://host/owner/repo", want: "https://host/owner/repo"}, + {name: "masks a password in a value the parser rejects", raw: "https://user:token@exa mple.com/repo", want: "https://user:xxxxx@exa mple.com/repo"}, + {name: "masks a password before a bad port", raw: "https://user:token@host:notaport/repo", want: "https://user:xxxxx@host:notaport/repo"}, + {name: "masks a password with a space in it", raw: "https://user:p@ss word@host/repo", want: "https://user:xxxxx@host/repo"}, + {name: "masks a password under another scheme", raw: "ssh://user:token@host:22/repo", want: "ssh://user:xxxxx@host:22/repo"}, + {name: "masks a password containing a slash", raw: "https://user:tok/en@host/repo", want: "https://user:xxxxx@host/repo"}, + {name: "masks a password containing a question mark", raw: "https://user:tok?en@host/repo", want: "https://user:xxxxx@host/repo"}, + {name: "masks a password containing a hash", raw: "https://user:tok#en@host/repo", want: "https://user:xxxxx@host/repo"}, + {name: "leaves a host and port without userinfo alone", raw: "https://host:8080/repo", want: "https://host:8080/repo"}, + {name: "leaves an unparseable value without userinfo alone", raw: "http://[::1", want: "http://[::1"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := RedactURL(tt.raw); got != tt.want { + t.Errorf("RedactURL(%q) = %q, want %q", tt.raw, got, tt.want) + } + }) + } +} + func TestParseHTTPURL(t *testing.T) { ok := []string{"https://example.com/x", "http://10.0.0.1:9000", "HTTPS://EXAMPLE.COM", " https://padded.example.com "} for _, value := range ok { diff --git a/internal/paths/urls.go b/internal/paths/urls.go index bdfb7cf..c43750a 100644 --- a/internal/paths/urls.go +++ b/internal/paths/urls.go @@ -44,6 +44,58 @@ func TrimGitSuffix(value string) string { return value } +// RedactURL renders a URL with any embedded password masked, for safe inclusion +// in logs and error messages. A clone URL may carry userinfo such as +// https://user:token@host/owner/repo, and a token written to a log is a leaked +// credential. +// +// A value the parser rejects is masked textually rather than returned as-is: +// url.Parse refuses hosts with spaces or malformed ports, and those are exactly +// the values whose userinfo cannot be isolated by parsing. Failing open there +// would leak the credential the function exists to hide. +func RedactURL(rawURL string) string { + trimmed := strings.TrimSpace(rawURL) + + parsed, err := url.Parse(trimmed) + if err != nil { + return maskUserinfo(trimmed) + } + return parsed.Redacted() +} + +// maskUserinfo replaces the password of a URL's userinfo section, keeping the +// scheme and username so a log line still says what was fetched and which +// credential was used. It is applied to values the URL parser rejected, so it +// locates the section itself rather than asking the parser for it. +// +// The section runs from the first ":" after the scheme to the last "@" in the +// remainder: a password may itself contain "@", "/", "?" or "#", so the last +// "@" is what separates userinfo from the host, and cutting at a separator +// first would hide that "@" and hand the credential back unchanged. The text +// after that "@" is kept, since it is the part of the value most likely to +// identify where the request was going. +func maskUserinfo(rawURL string) string { + schemeEnd := strings.Index(rawURL, "://") + if schemeEnd < 0 { + return rawURL + } + prefix := rawURL[:schemeEnd+3] + remainder := rawURL[len(prefix):] + + at := strings.LastIndex(remainder, "@") + if at < 0 { + // No userinfo at all: any ":" belongs to the host and port. + return rawURL + } + colon := strings.Index(remainder[:at], ":") + if colon < 0 { + // A username with no password carries no secret to mask. + return rawURL + } + + return prefix + remainder[:colon+1] + "xxxxx" + remainder[at:] +} + // IsHTTPOrHTTPS reports whether the parsed URL uses the http or https scheme. // It is the single predicate behind the transport allowlist, settings // validation, and the storage-key parser.