From 25830de501e277f45ec470743b3a4968aaea41b3 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 17:37:54 -0700 Subject: [PATCH 1/4] fix(git): retry the other URL form when a host answers with protocol v2 A host may serve a repository at its bare path and at the path ending in ".git", and some answer only one of the two with the Git wire protocol v2 advertisement. go-git v5 speaks v0/v1 only, so that answer is a hard failure: pkt-line 3: cannot read hash, pkt-line too short (version 2) The same repository is reachable at the other form, so a sync that fails for that reason is retried there instead of failing the repository outright. A URL with no path to alternate is reported as before, and no other failure is retried. The failure is recognised by its text because go-git raises it from inside the decoder with no error to match on. Both fragments of the message are required so an unrelated decode failure is not mistaken for it. The retry only costs anything on a failure this client cannot otherwise recover from, and the mirror is removed and re-cloned on that path, so nothing partial survives the attempt. Closes #30. --- internal/git/git.go | 85 ++++++++++++++++++++++++--- internal/git/git_test.go | 123 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 199 insertions(+), 9 deletions(-) diff --git a/internal/git/git.go b/internal/git/git.go index 514ee05..8186a66 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,6 +63,28 @@ 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) } + 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. + 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", remoteURL, "retrying", alternate) + err = mirrorRepository(ctx, alternate, localPath, credential, cache) + } + if err != nil { + return err + } + + if includeLFS { + return s.fetchLFS(ctx, remoteURL, 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 @@ -76,16 +100,63 @@ func (s *RepositoryService) SyncBareRepository(ctx context.Context, remoteURL, l "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. +func alternateURLForm(remoteURL string) string { + parsed, err := url.Parse(remoteURL) + if err != nil || parsed.Path == "" || parsed.Path == "/" { + return "" + } + + suffix := strings.HasSuffix(parsed.Path, ".git") + if suffix { + parsed.Path = strings.TrimSuffix(parsed.Path, ".git") + } else { + parsed.Path += ".git" + } + return parsed.String() } // fetchLFS mirrors the remote's LFS objects. A remote can have Git LFS turned diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 3fd4c34..eefef09 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,108 @@ 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) + } +} + +// 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"}, + } + 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() From 7019a8e22bab17476c7242983cfed05602f7f3b6 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 18:20:39 -0700 Subject: [PATCH 2/4] fix(git): keep the mirror, the credentials, and the URL form a retry depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the protocol v2 retry found ways it could go wrong. The v2 log line wrote the clone URL verbatim, and a clone URL may embed a password (`https://user:token@host/owner/repo`), which put a live credential in the logs. Both URLs now go through paths.RedactURL, which is the shared helper the URL parsing this package already uses lives beside; the LFS skip line had the same leak and is fixed with it. A cached mirror was deleted before the retry could use it. The self-heal treats every incremental fetch failure as a corrupt mirror and re-clones from scratch, so a host answering v2 destroyed an intact mirror and then failed on the same form — every scheduled run paid for a full clone. A protocol advertisement is not corruption, so it is no longer self-healed, and the retry now fetches incrementally into the mirror that is already there. The retry's URL was not carried to the LFS step, which still derived its batch endpoint from the configured form. On a host that serves LFS only on the form that answered, the mirror succeeded and the sync failed immediately after, so the recovery was half-applied. The URL that actually cloned is now the one LFS uses. The suffix was matched case-sensitively and only the exact path "/" was special, so `repo.GIT` gained a second suffix and `repo/` produced `repo/.git` — URLs that cannot exist, on which the retry silently did nothing. The suffix is now matched case-insensitively through paths.TrimGitSuffix, as the rest of the codebase does, and a trailing slash is normalised away first. A fragment is also dropped: it is a client-side marker that never forms part of what is fetched, and carrying it onto the other form appends it to a request the server never received one for. A query is deliberately kept, since it can be part of what the remote is asked for. Coverage: redaction including the unparseable case, an upper-case suffix, trailing slashes, fragments, escaped paths, and a cached mirror surviving a protocol v2 retry. The mirror test fails against the previous self-heal, and the suffix test fails against the previous case-sensitive match. --- internal/git/git.go | 54 ++++++++++++++++++++++++++---------- internal/git/git_test.go | 47 +++++++++++++++++++++++++++++++ internal/paths/paths_test.go | 21 ++++++++++++++ internal/paths/urls.go | 13 +++++++++ 4 files changed, 121 insertions(+), 14 deletions(-) diff --git a/internal/git/git.go b/internal/git/git.go index 8186a66..2e3dfb5 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -63,22 +63,28 @@ 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", remoteURL, "retrying", alternate) - err = mirrorRepository(ctx, alternate, localPath, credential, cache) + "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, remoteURL, localPath, credential) + return s.fetchLFS(ctx, syncedURL, localPath, credential) } return nil } @@ -90,10 +96,12 @@ func mirrorRepository(ctx context.Context, remoteURL, localPath string, credenti // 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.", @@ -144,18 +152,36 @@ func isProtocolVersionTwo(err error) bool { // 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(remoteURL) - if err != nil || parsed.Path == "" || parsed.Path == "/" { + parsed, err := url.Parse(strings.TrimSpace(remoteURL)) + if err != nil { return "" } - suffix := strings.HasSuffix(parsed.Path, ".git") - if suffix { - parsed.Path = strings.TrimSuffix(parsed.Path, ".git") - } else { + 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() } @@ -171,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 eefef09..3885899 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -207,6 +207,47 @@ func TestSyncBareRepositoryReportsProtocolTwoWithoutAnAlternate(t *testing.T) { } } +// 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 { @@ -223,6 +264,12 @@ func TestAlternateURLForm(t *testing.T) { {"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) { diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index 1fff8d5..3ea3a24 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -51,6 +51,27 @@ 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: "returns an unparseable value unchanged", 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..924b4ac 100644 --- a/internal/paths/urls.go +++ b/internal/paths/urls.go @@ -44,6 +44,19 @@ 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 that does not parse is returned unchanged, because there +// is nothing to mask and dropping it would hide the failure being reported. +func RedactURL(rawURL string) string { + parsed, err := url.Parse(strings.TrimSpace(rawURL)) + if err != nil { + return rawURL + } + return parsed.Redacted() +} + // 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. From 2e98e82dcb6302148c1a2ff006c865a818c92f73 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 18:34:30 -0700 Subject: [PATCH 3/4] fix(paths): mask userinfo when a URL cannot be parsed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RedactURL returned a value the parser rejected unchanged, so a credential in a host the parser refuses — a space in the hostname, a malformed port — was handed back verbatim to be written to a log. That is the one case the function exists for, so it failed open exactly where it mattered. Unparseable values are now masked by locating the userinfo section directly: everything between the scheme and the first separator is the authority, and the last "@" within it splits userinfo from host, since a password may itself contain one. The scheme, username and host are kept so a log line still says what was fetched and which credential was used; the path is dropped, because once the value has failed to parse the separator that would end the authority cannot be told apart from part of it. Coverage: a password in a rejected host, before a malformed port, containing a space, under a non-http scheme, and an unparseable value with no userinfo left alone. A regexp was tried first and abandoned — Go's regexp has no lookahead, so the last-"@" rule cannot be expressed with one. --- internal/paths/paths_test.go | 6 ++++- internal/paths/urls.go | 45 ++++++++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index 3ea3a24..e017c42 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -61,7 +61,11 @@ func TestRedactURL(t *testing.T) { {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: "returns an unparseable value unchanged", raw: "http://[::1", want: "http://[::1"}, + {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"}, + {name: "masks a password before a bad port", raw: "https://user:token@host:notaport/repo", want: "https://user:xxxxx@host:notaport"}, + {name: "masks a password with a space in it", raw: "https://user:p@ss word@host/repo", want: "https://user:xxxxx@host"}, + {name: "masks a password under another scheme", raw: "ssh://user:token@host:22/repo", want: "ssh://user:xxxxx@host:22/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) { diff --git a/internal/paths/urls.go b/internal/paths/urls.go index 924b4ac..de4a191 100644 --- a/internal/paths/urls.go +++ b/internal/paths/urls.go @@ -47,16 +47,53 @@ func TrimGitSuffix(value string) string { // 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 that does not parse is returned unchanged, because there -// is nothing to mask and dropping it would hide the failure being reported. +// 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 { - parsed, err := url.Parse(strings.TrimSpace(rawURL)) + trimmed := strings.TrimSpace(rawURL) + + parsed, err := url.Parse(trimmed) if err != nil { - return rawURL + 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: everything +// between the scheme and the first separator is the authority, and within that +// the last "@" splits userinfo from host, because a password may contain one. +func maskUserinfo(rawURL string) string { + schemeEnd := strings.Index(rawURL, "://") + if schemeEnd < 0 { + return rawURL + } + prefix := rawURL[:schemeEnd+3] + + authority := rawURL[len(prefix):] + if cut := strings.IndexAny(authority, "/?#"); cut >= 0 { + authority = authority[:cut] + } + + at := strings.LastIndex(authority, "@") + if at < 0 { + return rawURL + } + colon := strings.Index(authority[:at], ":") + if colon < 0 { + // A username with no password carries no secret to mask. + return rawURL + } + + return prefix + authority[:colon+1] + "xxxxx" + authority[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. From d9b4e312128cae75061eae3e1aa94a92affff767 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 18:46:11 -0700 Subject: [PATCH 4/4] fix(paths): mask a password that contains a URL separator maskUserinfo cut the authority at the first "/", "?" or "#" before looking for the "@" that ends the userinfo section. A password containing one of those hid that "@" from the search, and the unparseable value was returned with the credential intact: RedactURL("https://user:tok/en@host/repo") == "https://user:tok/en@host/repo" The "@" is now located in the whole remainder after the scheme, before anything is cut, so the separator only bounds the userinfo rather than hiding where it ends. Because nothing is discarded, the path, port and fragment of the rejected value survive in the masked form, which is more useful in a log line than the authority-only rendering this replaces. Coverage: passwords containing "/", "?" and "#", plus a host and port with no userinfo to ensure a ":" alone is not mistaken for one. --- internal/paths/paths_test.go | 10 +++++++--- internal/paths/urls.go | 24 +++++++++++++----------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index e017c42..2341ddc 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -61,10 +61,14 @@ func TestRedactURL(t *testing.T) { {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"}, - {name: "masks a password before a bad port", raw: "https://user:token@host:notaport/repo", want: "https://user:xxxxx@host:notaport"}, - {name: "masks a password with a space in it", raw: "https://user:p@ss word@host/repo", want: "https://user:xxxxx@host"}, + {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 { diff --git a/internal/paths/urls.go b/internal/paths/urls.go index de4a191..c43750a 100644 --- a/internal/paths/urls.go +++ b/internal/paths/urls.go @@ -66,32 +66,34 @@ func RedactURL(rawURL string) string { // 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: everything -// between the scheme and the first separator is the authority, and within that -// the last "@" splits userinfo from host, because a password may contain one. +// 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):] - authority := rawURL[len(prefix):] - if cut := strings.IndexAny(authority, "/?#"); cut >= 0 { - authority = authority[:cut] - } - - at := strings.LastIndex(authority, "@") + at := strings.LastIndex(remainder, "@") if at < 0 { + // No userinfo at all: any ":" belongs to the host and port. return rawURL } - colon := strings.Index(authority[:at], ":") + colon := strings.Index(remainder[:at], ":") if colon < 0 { // A username with no password carries no secret to mask. return rawURL } - return prefix + authority[:colon+1] + "xxxxx" + authority[at:] + return prefix + remainder[:colon+1] + "xxxxx" + remainder[at:] } // IsHTTPOrHTTPS reports whether the parsed URL uses the http or https scheme.