From b2f1fc773a9b16c35944c0fa2e649b773a413789 Mon Sep 17 00:00:00 2001 From: Isaque Pinheiro Date: Mon, 3 Aug 2026 11:11:48 -0300 Subject: [PATCH 1/2] fix(lint): clear the two findings #268 left on main The Lint job was already red when #268 merged, on two findings from that PR: setup/migrations.go:59: cognitive complexity 25 of func `seven` (gocognit) pkg/env/legacy_auth_test.go:76: declaration of "err" shadows line 70 (govet) `seven` grew past the threshold because the rewrite dropped the //nolint the original carried. Rather than put the suppression back, the per-entry work moves into migrateLegacyAuth and the decrypt-or-warn step into decryptLegacy, which reads better than the three near-identical blocks it replaces and takes the complexity down on its own. No behaviour change: `go test ./...` stays green. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/env/legacy_auth_test.go | 4 +-- setup/migrations.go | 72 ++++++++++++++++++++++--------------- 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/pkg/env/legacy_auth_test.go b/pkg/env/legacy_auth_test.go index 725f646..2f965b5 100644 --- a/pkg/env/legacy_auth_test.go +++ b/pkg/env/legacy_auth_test.go @@ -73,8 +73,8 @@ func TestLegacyAuthFieldsSurviveLoadAndSave(t *testing.T) { } configPath := filepath.Join(tempDir, consts.BossConfigFile) - if err := os.WriteFile(configPath, data, 0600); err != nil { - t.Fatalf("Failed to write config file: %v", err) + if writeErr := os.WriteFile(configPath, data, 0600); writeErr != nil { + t.Fatalf("Failed to write config file: %v", writeErr) } config, err := env.LoadConfiguration(tempDir) diff --git a/setup/migrations.go b/setup/migrations.go index 2137821..68748e1 100644 --- a/setup/migrations.go +++ b/setup/migrations.go @@ -61,43 +61,57 @@ func seven() { migrated := false for repo, auth := range configuration.Auth { - if auth == nil { - continue + if migrateLegacyAuth(repo, auth) { + migrated = true } + } - if auth.LegacyUser != "" { - if decrypted, err := oldDecrypt(auth.LegacyUser); err != nil { - msg.Warn("⚠️ Migration 7: could not migrate the user for %s: %v", repo, err) - } else { - auth.SetUser(decrypted) - } - } + if migrated { + configuration.SaveConfiguration() + } +} - if auth.LegacyPass != "" { - if decrypted, err := oldDecrypt(auth.LegacyPass); err != nil { - msg.Warn("⚠️ Migration 7: could not migrate the password for %s: %v", repo, err) - } else { - auth.SetPass(decrypted) - } - } +// migrateLegacyAuth converts one entry's legacy credentials and clears them, +// reporting whether anything was converted. +func migrateLegacyAuth(repo string, auth *env.Auth) bool { + if auth == nil { + return false + } + if auth.LegacyUser == "" && auth.LegacyPass == "" && auth.LegacyPassPhrase == "" { + return false + } - if auth.LegacyPassPhrase != "" { - if decrypted, err := oldDecrypt(auth.LegacyPassPhrase); err != nil { - msg.Warn("⚠️ Migration 7: could not migrate the passphrase for %s: %v", repo, err) - } else if decrypted != "" { - auth.SetPassPhrase(decrypted) - } - } + if decrypted, ok := decryptLegacy(repo, "user", auth.LegacyUser); ok { + auth.SetUser(decrypted) + } + if decrypted, ok := decryptLegacy(repo, "password", auth.LegacyPass); ok { + auth.SetPass(decrypted) + } + // An empty passphrase means the key has none, and storing it back would only + // re-create the value migration 7 exists to retire. + if decrypted, ok := decryptLegacy(repo, "passphrase", auth.LegacyPassPhrase); ok && decrypted != "" { + auth.SetPassPhrase(decrypted) + } - if auth.LegacyUser != "" || auth.LegacyPass != "" || auth.LegacyPassPhrase != "" { - auth.LegacyUser, auth.LegacyPass, auth.LegacyPassPhrase = "", "", "" - migrated = true - } + auth.LegacyUser, auth.LegacyPass, auth.LegacyPassPhrase = "", "", "" + + return true +} + +// decryptLegacy decrypts one legacy value, warning instead of aborting when it +// cannot be read. It reports false when there was nothing to convert. +func decryptLegacy(repo, field, value string) (string, bool) { + if value == "" { + return "", false } - if migrated { - configuration.SaveConfiguration() + decrypted, err := oldDecrypt(value) + if err != nil { + msg.Warn("⚠️ Migration 7: could not migrate the %s for %s: %v", field, repo, err) + return "", false } + + return decrypted, true } // cleanup cleans up the internal global directory. From 2afa9adfe8891a18a577a41174e184dfd4457408 Mon Sep 17 00:00:00 2001 From: Isaque Pinheiro Date: Mon, 3 Aug 2026 10:47:14 -0300 Subject: [PATCH 2/2] fix(git): pick the credential for the transport the remote actually uses The credential is looked up by host prefix, but the transport comes from the URL the repository fetches from. Those disagree whenever a cache was cloned over HTTPS before an SSH login was configured for that host: dep.GetURL() returns the SSH form once auth.UseSSH is set, while the cached remote stays HTTPS. go-git v5.4.2 ignored an SSH credential handed to the HTTP transport and fetched anonymously. Since the upgrade to v5.19.1 it returns transport.ErrInvalidAuthMethod instead, so every fetch for that host fails -- and because a failed fetch only warned, Boss still reported "Installation completed successfully" and exited 0. The project silently stayed on whatever the cache last held. Measured against v3.0.12 on the same cache, same auth and same project, with a cache missing the newest tag of hashload/cqlbr: without this change: 4x "invalid auth method", cache untouched, cqlbr 1.1.6 with this change: no warning, tag restored, cqlbr 1.1.51 (v3.0.12 parity) GetAuthForURL resolves the credential against the effective remote URL and returns nil when it does not fit the transport, which restores the anonymous fetch go-git used to perform on its own. A private repository reached over the wrong transport still fails, exactly as it did before. The fetch failure in UpdateCacheEmbedded was logged at debug level, so it was invisible at normal verbosity. It now warns and says the cached copy is being used, which is the difference between "up to date" and "whatever was cached". Co-Authored-By: Claude Opus 5 (1M context) --- internal/adapters/secondary/git/git.go | 21 ++- .../adapters/secondary/git/git_embedded.go | 11 +- pkg/env/auth_transport_test.go | 143 ++++++++++++++++++ pkg/env/configuration.go | 47 ++++++ pkg/env/interfaces.go | 1 + 5 files changed, 217 insertions(+), 6 deletions(-) create mode 100644 pkg/env/auth_transport_test.go diff --git a/internal/adapters/secondary/git/git.go b/internal/adapters/secondary/git/git.go index 952464e..44e8c9c 100644 --- a/internal/adapters/secondary/git/git.go +++ b/internal/adapters/secondary/git/git.go @@ -33,6 +33,23 @@ func UpdateCache(config env.ConfigProvider, dep domain.Dependency) (*goGit.Repos return UpdateCacheNative(dep) } +// remoteURL returns the URL the repository actually fetches from. +// +// This is not always dep.GetURL(): a cache cloned before an SSH login was +// configured for the host keeps its original HTTPS remote, and the credential +// has to be picked for the transport in use, not for the one the manifest +// would produce today. +func remoteURL(repository *goGit.Repository, dep domain.Dependency) string { + if repository != nil { + if remote, err := repository.Remote(goGit.DefaultRemoteName); err == nil { + if urls := remote.Config().URLs; len(urls) > 0 { + return urls[0] + } + } + } + return dep.GetURL() +} + func initSubmodules(config env.ConfigProvider, dep domain.Dependency, repository *goGit.Repository) error { worktree, err := repository.Worktree() if err != nil { @@ -46,7 +63,7 @@ func initSubmodules(config env.ConfigProvider, dep domain.Dependency, repository err = submodules.Update(&goGit.SubmoduleUpdateOptions{ Init: true, RecurseSubmodules: goGit.DefaultSubmoduleRecursionDepth, - Auth: config.GetAuth(dep.GetURLPrefix()), + Auth: config.GetAuthForURL(dep.GetURLPrefix(), remoteURL(repository, dep)), }) if err != nil { return err @@ -70,7 +87,7 @@ func GetVersions(config env.ConfigProvider, repository *goGit.Repository, dep do err := repository.Fetch(&goGit.FetchOptions{ Force: true, Prune: true, - Auth: config.GetAuth(dep.GetURLPrefix()), + Auth: config.GetAuthForURL(dep.GetURLPrefix(), remoteURL(repository, dep)), RefSpecs: []gitConfig.RefSpec{ "refs/*:refs/*", "HEAD:refs/heads/HEAD", diff --git a/internal/adapters/secondary/git/git_embedded.go b/internal/adapters/secondary/git/git_embedded.go index 95b5a5b..5434ffd 100644 --- a/internal/adapters/secondary/git/git_embedded.go +++ b/internal/adapters/secondary/git/git_embedded.go @@ -26,7 +26,7 @@ func CloneCacheEmbedded(config env.ConfigProvider, dep domain.Dependency) (*git. storageCache := makeStorageCache(config, dep) worktreeFileSystem := createWorktreeFs(config, dep) url := dep.GetURL() - auth := config.GetAuth(dep.GetURLPrefix()) + auth := config.GetAuthForURL(dep.GetURLPrefix(), url) cloneOpts := &git.CloneOptions{ URL: url, @@ -73,9 +73,12 @@ func UpdateCacheEmbedded(config env.ConfigProvider, dep domain.Dependency) (*git err = repository.Fetch(&git.FetchOptions{ Force: true, - Auth: config.GetAuth(dep.GetURLPrefix())}) + Auth: config.GetAuthForURL(dep.GetURLPrefix(), remoteURL(repository, dep))}) if err != nil && err.Error() != "already up-to-date" { - msg.Debug("Error to fetch repository of %s: %s", dep.Repository, err) + // The cached copy is still usable, so this is not fatal -- but it is the + // difference between "up to date" and "whatever was cached last time", + // which the user has to be able to see. + msg.Warn("⚠️ Could not update %s, using the cached copy: %s", dep.Repository, err) } if err := initSubmodules(config, dep, repository); err != nil { return nil, err @@ -133,6 +136,6 @@ func PullEmbedded(config env.ConfigProvider, dep domain.Dependency) error { } return worktree.Pull(&git.PullOptions{ Force: true, - Auth: config.GetAuth(dep.GetURLPrefix()), + Auth: config.GetAuthForURL(dep.GetURLPrefix(), remoteURL(repository, dep)), }) } diff --git a/pkg/env/auth_transport_test.go b/pkg/env/auth_transport_test.go new file mode 100644 index 0000000..da7922a --- /dev/null +++ b/pkg/env/auth_transport_test.go @@ -0,0 +1,143 @@ +package env_test + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/pem" + "os" + "path/filepath" + "testing" + + "github.com/go-git/go-git/v5/plumbing/transport/http" + "github.com/hashload/boss/pkg/env" + "golang.org/x/crypto/ssh" +) + +// A cache cloned over HTTPS keeps its HTTPS remote even after `boss login -s` +// configures SSH for the host. Handing the SSH credential to the HTTP transport +// makes go-git reject the fetch with "invalid auth method", so every dependency +// of that host silently stops updating. +func TestGetAuthForURLDropsSSHCredentialOnHTTPRemote(t *testing.T) { + config := newConfigWithAuth(t, &env.Auth{UseSSH: true, Path: "/home/user/.ssh/id_ed25519"}) + + if got := config.GetAuthForURL("github.com", "https://github.com/hashload/horse"); got != nil { + t.Errorf("GetAuthForURL() = %v, want nil for an SSH credential on an HTTPS remote", got) + } +} + +func TestGetAuthForURLDropsBasicCredentialOnSSHRemote(t *testing.T) { + auth := &env.Auth{} + auth.SetUser("octocat") + auth.SetPass("s3cr3t") + config := newConfigWithAuth(t, auth) + + if got := config.GetAuthForURL("github.com", "git@github.com:hashload/horse"); got != nil { + t.Errorf("GetAuthForURL() = %v, want nil for a basic credential on an SSH remote", got) + } +} + +func TestGetAuthForURLKeepsMatchingCredential(t *testing.T) { + auth := &env.Auth{} + auth.SetUser("octocat") + auth.SetPass("s3cr3t") + config := newConfigWithAuth(t, auth) + + got := config.GetAuthForURL("github.com", "https://github.com/hashload/horse") + basic, ok := got.(*http.BasicAuth) + if !ok { + t.Fatalf("GetAuthForURL() = %T, want *http.BasicAuth", got) + } + if basic.Username != "octocat" || basic.Password != "s3cr3t" { + t.Errorf("GetAuthForURL() returned %q/%q, want octocat/s3cr3t", basic.Username, basic.Password) + } +} + +func TestGetAuthForURLWithoutStoredCredential(t *testing.T) { + config := newConfigWithAuth(t, nil) + + if got := config.GetAuthForURL("gitlab.com", "https://gitlab.com/group/project"); got != nil { + t.Errorf("GetAuthForURL() = %v, want nil when nothing is stored for the host", got) + } +} + +// An empty URL means the caller could not determine the transport. Falling back +// to the stored credential keeps the previous behaviour rather than silently +// dropping authentication. +func TestGetAuthForURLFallsBackWhenURLIsUnknown(t *testing.T) { + auth := &env.Auth{} + auth.SetUser("octocat") + auth.SetPass("s3cr3t") + config := newConfigWithAuth(t, auth) + + if got := config.GetAuthForURL("github.com", ""); got == nil { + t.Error("GetAuthForURL() = nil, want the stored credential when the URL is unknown") + } +} + +func TestGetAuthForURLTransportDetection(t *testing.T) { + sshAuth := &env.Auth{UseSSH: true, Path: writeTestSSHKey(t)} + sshAuth.SetPassPhrase(testKeyPassphrase) + config := newConfigWithAuth(t, sshAuth) + + sshRemotes := []string{ + "git@github.com:hashload/horse", + "ssh://git@github.com/hashload/horse", + "git@mygitlab.domain.de:delphi/libraries/mylib.git", + } + for _, remote := range sshRemotes { + if got := config.GetAuthForURL("github.com", remote); got == nil { + t.Errorf("GetAuthForURL(%q) = nil, want the SSH credential", remote) + } + } + + httpRemotes := []string{ + "https://github.com/hashload/horse", + "http://github.com/hashload/horse", + // A user in an HTTPS URL must not be mistaken for scp-like syntax. + "https://octocat@github.com/hashload/horse", + } + for _, remote := range httpRemotes { + if got := config.GetAuthForURL("github.com", remote); got != nil { + t.Errorf("GetAuthForURL(%q) = %v, want nil", remote, got) + } + } +} + +// testKeyPassphrase protects the generated key. Using an encrypted key keeps +// this test independent of how an empty passphrase is handled. +const testKeyPassphrase = "test-passphrase" + +// writeTestSSHKey writes an encrypted ed25519 key and returns its path. +// The SSH branch of GetAuth parses the key file, so it needs a real one. +func writeTestSSHKey(t *testing.T) string { + t.Helper() + + _, private, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("Failed to generate the test key: %v", err) + } + + block, err := ssh.MarshalPrivateKeyWithPassphrase(private, "", []byte(testKeyPassphrase)) + if err != nil { + t.Fatalf("Failed to marshal the test key: %v", err) + } + + path := filepath.Join(t.TempDir(), "id_ed25519") + if err := os.WriteFile(path, pem.EncodeToMemory(block), 0600); err != nil { + t.Fatalf("Failed to write the test key: %v", err) + } + return path +} + +func newConfigWithAuth(t *testing.T, auth *env.Auth) *env.Configuration { + t.Helper() + + config, err := env.LoadConfiguration(t.TempDir()) + if config == nil { + t.Fatalf("LoadConfiguration() returned no configuration: %v", err) + } + if auth != nil { + config.Auth["github.com"] = auth + } + return config +} diff --git a/pkg/env/configuration.go b/pkg/env/configuration.go index 5b5dc55..687db5d 100644 --- a/pkg/env/configuration.go +++ b/pkg/env/configuration.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "time" "github.com/go-git/go-git/v5/plumbing/transport" @@ -127,7 +128,53 @@ func (a *Auth) SetPassPhrase(passphrase string) { } } +// usesSSHTransport reports whether rawURL is fetched over SSH. +func usesSSHTransport(rawURL string) bool { + switch { + case strings.HasPrefix(rawURL, "ssh://"): + return true + case strings.HasPrefix(rawURL, "http://"), + strings.HasPrefix(rawURL, "https://"), + strings.HasPrefix(rawURL, "git://"), + strings.HasPrefix(rawURL, "file://"): + return false + default: + // scp-like syntax: [user@]host:path + return strings.Contains(rawURL, "@") + } +} + +// GetAuthForURL returns the authentication method for a repository, but only +// when the stored credential fits the transport rawURL will use. +// +// The credential is keyed by host, while the transport comes from the URL the +// repository actually fetches from. Those two disagree whenever a cache was +// cloned over HTTPS before an SSH login was configured for that host. Handing +// an SSH credential to the HTTP transport makes go-git fail the fetch with +// "invalid auth method" -- older versions silently ignored it instead, which is +// why this only started biting after the go-git upgrade. +// +// Returning nil means "fetch anonymously", which is what go-git used to do on +// its own and is correct for the public repositories this affects. A private +// repository reached over the wrong transport still fails, as it always did. +func (c *Configuration) GetAuthForURL(repo, rawURL string) transport.AuthMethod { + auth := c.Auth[repo] + if auth == nil { + return nil + } + + if rawURL != "" && auth.UseSSH != usesSSHTransport(rawURL) { + msg.Debug("Skipping the credential stored for %s: it does not fit the transport of %s", repo, rawURL) + return nil + } + + return c.GetAuth(repo) +} + // GetAuth returns the authentication method for a repository. +// +// Prefer GetAuthForURL when the URL being reached is known: this one cannot +// tell whether the credential fits the transport. func (c *Configuration) GetAuth(repo string) transport.AuthMethod { auth := c.Auth[repo] diff --git a/pkg/env/interfaces.go b/pkg/env/interfaces.go index 653b9b5..bf1c621 100644 --- a/pkg/env/interfaces.go +++ b/pkg/env/interfaces.go @@ -12,6 +12,7 @@ type ConfigProvider interface { GetDelphiPath() string GetGitEmbedded() bool GetAuth(repo string) transport.AuthMethod + GetAuthForURL(repo, rawURL string) transport.AuthMethod GetPurgeTime() int GetInternalRefreshRate() int GetLastPurge() time.Time