From 7ec9f9cf660678f84cef1217b9dbe09c1d1772dd Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Tue, 28 Jul 2026 16:19:36 +0300 Subject: [PATCH 01/15] RTECO-0000 - Limit jf setup go GOPROXY fallback to 404/410 `jf setup go` wrote `GOPROXY=|direct`. In Go, a pipe makes the go command fall through to the next entry after ANY error, while a comma limits it to 404 and 410 responses. Because `direct` fetches from the module's public source, the pipe meant a 403 from Artifactory Curation was silently satisfied from the public internet instead of failing. Verified against go1.26.5 with a local proxy returning a fixed status: 403 + ,direct -> exit 1, module not fetched (Curation holds) 403 + |direct -> exit 0, module fetched from public VCS (Curation bypassed) 404 + either -> falls through, as intended This matches cmd/go/internal/modfetch/proxy.go, which sets fallBackOnError from the separator and only breaks early when the error is fs.ErrNotExist. Go's own default, https://proxy.golang.org,direct, uses a comma for the same reason. `jf go` keeps the pipe: it resolves through the CLI for a single invocation and already exposes --no-fallback, so falling back on any error is a deliberate feature there. Only `jf setup`, which writes a persistent global GOPROXY for the native go command with no opt-out, switches to the comma. - Add GoProxyUrlParams.Separator with the pipe as the default, so existing callers - including the curation-audit path - are unaffected. - Harden addDirect against appending a second fallback entry for either separator, so re-running setup cannot produce "...|direct,direct". - Fix the configureGo docstring, which already documented the comma the code never wrote, and explain why the two commands differ. - Assert the separator in TestSetupCommand_Go (both that the comma is present and that the pipe is absent), which previously matched only the url prefix. - Cover the separator matrix and addDirect idempotency in go_test.go. Co-Authored-By: Claude Opus 5 --- artifactory/commands/golang/go.go | 42 ++++++++++++++++++-- artifactory/commands/golang/go_test.go | 50 ++++++++++++++++++++++++ artifactory/commands/setup/setup.go | 10 ++++- artifactory/commands/setup/setup_test.go | 6 +++ 4 files changed, 103 insertions(+), 5 deletions(-) diff --git a/artifactory/commands/golang/go.go b/artifactory/commands/golang/go.go index 39ac40ae..4f59d122 100644 --- a/artifactory/commands/golang/go.go +++ b/artifactory/commands/golang/go.go @@ -356,12 +356,31 @@ func logGoVersion() error { return nil } +const ( + // GoProxyAnyErrorSeparator ('|') makes the go command fall through to the + // next GOPROXY entry after ANY error, including 403 responses. + GoProxyAnyErrorSeparator = "|" + // GoProxyNotFoundSeparator (',') makes the go command fall through only + // after a 404 or 410 response. Any other error - notably a 403 from + // Artifactory Curation - is a hard failure, so a blocked module is not + // silently fetched from its public source. This matches Go's own default + // of "https://proxy.golang.org,direct". + GoProxyNotFoundSeparator = "," +) + type GoProxyUrlParams struct { // Fallback to retrieve the modules directly from the source if // the module failed to be retrieved from the proxy. - // add |direct to the end of the url. + // Appends direct to the end of the url. // example: https://gocenter.io|direct Direct bool + // Separator between the proxy url and the "direct" fallback entry, which + // decides WHEN the go command falls through to the public source: + // "|" (default) - on any error, including a Curation 403. + // "," - only on 404/410, so a blocked module fails instead of leaking. + // Empty means GoProxyAnyErrorSeparator, preserving the previous behavior + // for existing callers. + Separator string // The path from baseUrl to the standard Go repository path // URL structure: //api/go/ EndpointPrefix string @@ -373,11 +392,26 @@ func (gdu *GoProxyUrlParams) BuildUrl(url *url.URL, repoName string) string { return gdu.addDirect(url.String()) } +// separator returns the configured fallback separator, defaulting to the +// any-error form so callers that predate this field keep their behavior. +func (gdu *GoProxyUrlParams) separator() string { + if gdu.Separator == GoProxyNotFoundSeparator { + return GoProxyNotFoundSeparator + } + return GoProxyAnyErrorSeparator +} + func (gdu *GoProxyUrlParams) addDirect(url string) string { - if gdu.Direct && !strings.HasSuffix(url, "|direct") { - return url + "|direct" + if !gdu.Direct { + return url + } + // Guard against both spellings so an already-suffixed url is never + // appended to twice, whichever separator it carries. + if strings.HasSuffix(url, GoProxyAnyErrorSeparator+"direct") || + strings.HasSuffix(url, GoProxyNotFoundSeparator+"direct") { + return url } - return url + return url + gdu.separator() + "direct" } func GetArtifactoryRemoteRepoUrl(serverDetails *config.ServerDetails, repo string, goProxyParams GoProxyUrlParams) (string, error) { diff --git a/artifactory/commands/golang/go_test.go b/artifactory/commands/golang/go_test.go index 0ffe406e..20334faa 100644 --- a/artifactory/commands/golang/go_test.go +++ b/artifactory/commands/golang/go_test.go @@ -140,6 +140,7 @@ func TestGoProxyUrlParams_BuildUrl(t *testing.T) { name string RepoName string Direct bool + Separator string EndpointPrefix string ExpectedUrl string }{ @@ -160,6 +161,33 @@ func TestGoProxyUrlParams_BuildUrl(t *testing.T) { EndpointPrefix: "prefix", ExpectedUrl: "https://test/prefix/api/go/go", }, + { + name: "Empty separator keeps the any-error default", + RepoName: "go", + Direct: true, + Separator: "", + ExpectedUrl: "https://test/api/go/go|direct", + }, + { + name: "Comma separator limits fallback to 404/410", + RepoName: "go", + Direct: true, + Separator: GoProxyNotFoundSeparator, + ExpectedUrl: "https://test/api/go/go,direct", + }, + { + name: "Explicit pipe separator", + RepoName: "go", + Direct: true, + Separator: GoProxyAnyErrorSeparator, + ExpectedUrl: "https://test/api/go/go|direct", + }, + { + name: "Separator ignored when Direct is false", + RepoName: "go", + Separator: GoProxyNotFoundSeparator, + ExpectedUrl: "https://test/api/go/go", + }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { @@ -167,9 +195,31 @@ func TestGoProxyUrlParams_BuildUrl(t *testing.T) { require.NoError(t, err) gdu := &GoProxyUrlParams{ Direct: testCase.Direct, + Separator: testCase.Separator, EndpointPrefix: testCase.EndpointPrefix, } assert.Equalf(t, testCase.ExpectedUrl, gdu.BuildUrl(remoteUrl, testCase.RepoName), "BuildUrl(%v, %v)", remoteUrl, testCase.RepoName) }) } } + +// addDirect must never append a second fallback entry, whichever separator the +// url already carries - otherwise re-running setup would produce "...|direct,direct". +func TestGoProxyUrlParams_AddDirectIsIdempotent(t *testing.T) { + testCases := []struct { + name string + url string + separator string + }{ + {name: "pipe url, pipe separator", url: "https://test/api/go/go|direct", separator: GoProxyAnyErrorSeparator}, + {name: "comma url, comma separator", url: "https://test/api/go/go,direct", separator: GoProxyNotFoundSeparator}, + {name: "pipe url, comma separator", url: "https://test/api/go/go|direct", separator: GoProxyNotFoundSeparator}, + {name: "comma url, pipe separator", url: "https://test/api/go/go,direct", separator: GoProxyAnyErrorSeparator}, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + gdu := &GoProxyUrlParams{Direct: true, Separator: testCase.separator} + assert.Equal(t, testCase.url, gdu.addDirect(testCase.url)) + }) + } +} diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index 85651aa6..5d89d799 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -352,6 +352,13 @@ func (sc *SetupCommand) configureYarn() (err error) { // Runs the following command: // // go env -w GOPROXY=https://:@/artifactory/go/,direct +// +// The comma is deliberate. Unlike `jf go`, which resolves through the CLI for a +// single invocation and exposes --no-fallback, this writes a persistent global +// GOPROXY consumed by the native go command with no opt-out. A comma limits the +// fallback to 404/410 (module not proxied); a pipe would fall through on ANY +// error, so a 403 from Artifactory Curation would be silently satisfied from the +// module's public source. func (sc *SetupCommand) configureGo() error { if goProxyVal := os.Getenv("GOPROXY"); goProxyVal != "" { // Remove the variable so it won't override the newly configured proxy (temporarily). @@ -366,7 +373,8 @@ func (sc *SetupCommand) configureGo() error { log.Warn(fmt.Sprintf("A local GOPROXY='%s' is set and will override the global setting.\n"+ "Unset it in your shell config (e.g., .zshrc, .bashrc).", goProxyVal)) } - repoWithCredsUrl, err := golang.GetArtifactoryRemoteRepoUrl(sc.serverDetails, sc.repoName, golang.GoProxyUrlParams{Direct: true}) + repoWithCredsUrl, err := golang.GetArtifactoryRemoteRepoUrl(sc.serverDetails, sc.repoName, + golang.GoProxyUrlParams{Direct: true, Separator: golang.GoProxyNotFoundSeparator}) if err != nil { return fmt.Errorf("failed to get Go repository URL: %w", err) } diff --git a/artifactory/commands/setup/setup_test.go b/artifactory/commands/setup/setup_test.go index c02ec8d2..64eae30b 100644 --- a/artifactory/commands/setup/setup_test.go +++ b/artifactory/commands/setup/setup_test.go @@ -358,6 +358,12 @@ func TestSetupCommand_Go(t *testing.T) { assert.Contains(t, goProxy, "https://acme.jfrog.io/artifactory/api/go/test-repo") } + // The fallback must be comma-separated. A pipe would make the go command + // fall through to the module's public source on ANY error, including a 403 + // from Artifactory Curation, silently defeating the block. + assert.Contains(t, goProxy, ",direct", "jf setup must limit the direct fallback to 404/410") + assert.NotContains(t, goProxy, "|direct", "a pipe separator would fall back on any error, including a Curation 403") + // Clean up the global GOPROXY setting after each test case err = exec.Command("go", "env", "-u", goProxyEnv).Run() assert.NoError(t, err, "Failed to unset GOPROXY after test case") From 94f74ba415e080a10bb9a2e434dfe336590e2221 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Tue, 28 Jul 2026 16:33:10 +0300 Subject: [PATCH 02/15] RTECO-0000 - Match Go's own GOPROXY parsing in the direct-fallback guard The idempotency guard used strings.HasSuffix against ",direct" and "|direct", which misses forms the go command accepts. Go trims each GOPROXY entry before comparing it, so "url , direct" and "url | direct" are valid fallbacks - the old guard would not recognise them and would append a second entry. Replace it with hasDirectFallback, which splits on the last separator and trims before comparing, mirroring cmd/go/internal/modfetch/proxy.go. The comparison stays case-sensitive on purpose. Verified against go1.26.5 with a local proxy returning 404: url,direct / url, direct / url , direct / url | direct -> module resolved url,DIRECT / url,Direct -> "invalid proxy URL missing scheme" So "DIRECT" is not the keyword to Go either; treating it as "no fallback present" matches the go command rather than silently accepting a config Go rejects. Note this guard is defensive only: the sole production path builds the url from url.Parse(details.GetUrl()), an Artifactory base url that cannot carry a fallback suffix. Hardened anyway so the guard actually holds for any future caller. Adds TestHasDirectFallback covering the whitespace and case variants, "off", "directory" as a substring, and a comma inside the path. Co-Authored-By: Claude Opus 5 --- artifactory/commands/golang/go.go | 28 +++++++++++++----- artifactory/commands/golang/go_test.go | 41 ++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/artifactory/commands/golang/go.go b/artifactory/commands/golang/go.go index 4f59d122..d989c752 100644 --- a/artifactory/commands/golang/go.go +++ b/artifactory/commands/golang/go.go @@ -357,6 +357,10 @@ func logGoVersion() error { } const ( + // goProxyDirect is Go's keyword for fetching straight from the module's + // source. Case-sensitive: the go command compares entries with == against + // "direct", so "DIRECT" is treated as a proxy URL, not the keyword. + goProxyDirect = "direct" // GoProxyAnyErrorSeparator ('|') makes the go command fall through to the // next GOPROXY entry after ANY error, including 403 responses. GoProxyAnyErrorSeparator = "|" @@ -401,17 +405,25 @@ func (gdu *GoProxyUrlParams) separator() string { return GoProxyAnyErrorSeparator } +// hasDirectFallback reports whether the url already ends in a "direct" fallback +// entry, mirroring how the go command itself parses GOPROXY: entries are split +// on "," or "|" and trimmed before comparison, so "url , direct" counts. The +// comparison is case-sensitive for the same reason Go's is - "DIRECT" is not the +// direct keyword to Go either, it is parsed as a proxy URL and rejected with +// "invalid proxy URL missing scheme". +func hasDirectFallback(url string) bool { + i := strings.LastIndexAny(url, GoProxyAnyErrorSeparator+GoProxyNotFoundSeparator) + if i < 0 { + return false + } + return strings.TrimSpace(url[i+1:]) == goProxyDirect +} + func (gdu *GoProxyUrlParams) addDirect(url string) string { - if !gdu.Direct { - return url - } - // Guard against both spellings so an already-suffixed url is never - // appended to twice, whichever separator it carries. - if strings.HasSuffix(url, GoProxyAnyErrorSeparator+"direct") || - strings.HasSuffix(url, GoProxyNotFoundSeparator+"direct") { + if !gdu.Direct || hasDirectFallback(url) { return url } - return url + gdu.separator() + "direct" + return url + gdu.separator() + goProxyDirect } func GetArtifactoryRemoteRepoUrl(serverDetails *config.ServerDetails, repo string, goProxyParams GoProxyUrlParams) (string, error) { diff --git a/artifactory/commands/golang/go_test.go b/artifactory/commands/golang/go_test.go index 20334faa..e152bb09 100644 --- a/artifactory/commands/golang/go_test.go +++ b/artifactory/commands/golang/go_test.go @@ -223,3 +223,44 @@ func TestGoProxyUrlParams_AddDirectIsIdempotent(t *testing.T) { }) } } + +// hasDirectFallback must agree with the go command's own GOPROXY parsing, which +// trims each entry before comparing it against "direct" and is case-sensitive. +// Verified against go1.26.5: "url , direct" and "url | direct" resolve normally, +// while "url,DIRECT" fails with "invalid proxy URL missing scheme". +func TestHasDirectFallback(t *testing.T) { + testCases := []struct { + name string + url string + expected bool + }{ + // Go accepts these, so they already carry a fallback. + {name: "comma, no space", url: "https://test/api/go/go,direct", expected: true}, + {name: "pipe, no space", url: "https://test/api/go/go|direct", expected: true}, + {name: "space after comma", url: "https://test/api/go/go, direct", expected: true}, + {name: "spaces around comma", url: "https://test/api/go/go , direct", expected: true}, + {name: "spaces around pipe", url: "https://test/api/go/go | direct", expected: true}, + {name: "tab after comma", url: "https://test/api/go/go,\tdirect", expected: true}, + // Go rejects these as proxy URLs, so they are not a fallback. + {name: "uppercase DIRECT", url: "https://test/api/go/go,DIRECT", expected: false}, + {name: "mixed case Direct", url: "https://test/api/go/go,Direct", expected: false}, + // No fallback entry at all. + {name: "no separator", url: "https://test/api/go/go", expected: false}, + {name: "separator but other entry", url: "https://test/api/go/go,https://other", expected: false}, + {name: "off is not direct", url: "https://test/api/go/go,off", expected: false}, + {name: "direct as a substring only", url: "https://test/api/go/go,directory", expected: false}, + {name: "comma in the path, no fallback", url: "https://test/api/go/go,v2", expected: false}, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + assert.Equal(t, testCase.expected, hasDirectFallback(testCase.url)) + // A url Go already treats as having a fallback must not gain a second one. + gdu := &GoProxyUrlParams{Direct: true, Separator: GoProxyNotFoundSeparator} + if testCase.expected { + assert.Equal(t, testCase.url, gdu.addDirect(testCase.url)) + } else { + assert.Equal(t, testCase.url+",direct", gdu.addDirect(testCase.url)) + } + }) + } +} From 6b8f2645f6d4d113b2ff1a97fe2d665bed6de136 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Tue, 28 Jul 2026 16:38:06 +0300 Subject: [PATCH 03/15] RTECO-0000 - Address review: bool instead of a separator string, drop dead guard Acting on review feedback. All six points were valid; four changed the code. Replace Separator string with FallbackOnlyIfNotFound bool. The string form whitelisted "," and returned the pipe for everything else, so a typo like ";" silently selected the permissive separator - fail-open on the very switch that closes the bypass. A bool makes that state unrepresentable, and removes two exported constants, the separator() accessor and the "empty means..." paragraph for a field with exactly two states and one non-default caller. Revert addDirect to the pre-existing single-suffix guard. The dual-suffix guard and its hasDirectFallback helper were unreachable: the only production path is BuildUrl, which rebuilds the path with path.Join and can never yield a url ending in a fallback entry. Re-running setup constructs a new url rather than appending to the old one, so the "...|direct,direct" scenario the previous commit described cannot occur. Dropping it also removes a test case that asserted a |direct url stays |direct even when the caller asked for the comma - an insecure outcome codified in a test named "idempotent". Add a log.Info in configureGo. Limiting the fallback is a behavior change with no opt-out: an unreachable Artifactory, a 5xx or a 401 previously fell through to the module's public source and the build kept working, and now fails. Nothing in the docs mentions GOPROXY, so the command output is the only place a user learns this. Retain from the earlier commit: the comma for jf setup, the corrected docstring, and the setup_test assertions that ,direct is present and |direct is not. Co-Authored-By: Claude Opus 5 --- artifactory/commands/golang/go.go | 65 ++++---------- artifactory/commands/golang/go_test.go | 115 +++++-------------------- artifactory/commands/setup/setup.go | 8 +- 3 files changed, 45 insertions(+), 143 deletions(-) diff --git a/artifactory/commands/golang/go.go b/artifactory/commands/golang/go.go index d989c752..cde142fe 100644 --- a/artifactory/commands/golang/go.go +++ b/artifactory/commands/golang/go.go @@ -356,35 +356,18 @@ func logGoVersion() error { return nil } -const ( - // goProxyDirect is Go's keyword for fetching straight from the module's - // source. Case-sensitive: the go command compares entries with == against - // "direct", so "DIRECT" is treated as a proxy URL, not the keyword. - goProxyDirect = "direct" - // GoProxyAnyErrorSeparator ('|') makes the go command fall through to the - // next GOPROXY entry after ANY error, including 403 responses. - GoProxyAnyErrorSeparator = "|" - // GoProxyNotFoundSeparator (',') makes the go command fall through only - // after a 404 or 410 response. Any other error - notably a 403 from - // Artifactory Curation - is a hard failure, so a blocked module is not - // silently fetched from its public source. This matches Go's own default - // of "https://proxy.golang.org,direct". - GoProxyNotFoundSeparator = "," -) - type GoProxyUrlParams struct { // Fallback to retrieve the modules directly from the source if // the module failed to be retrieved from the proxy. - // Appends direct to the end of the url. + // add |direct to the end of the url. // example: https://gocenter.io|direct Direct bool - // Separator between the proxy url and the "direct" fallback entry, which - // decides WHEN the go command falls through to the public source: - // "|" (default) - on any error, including a Curation 403. - // "," - only on 404/410, so a blocked module fails instead of leaking. - // Empty means GoProxyAnyErrorSeparator, preserving the previous behavior - // for existing callers. - Separator string + // FallbackOnlyIfNotFound joins the "direct" entry with a comma instead of a + // pipe, so the go command falls through to the module's public source only + // after a 404/410 and a 403 from Artifactory Curation is a hard failure. + // The zero value keeps the previous any-error (pipe) behavior for existing + // callers. Ignored when Direct is false. + FallbackOnlyIfNotFound bool // The path from baseUrl to the standard Go repository path // URL structure: //api/go/ EndpointPrefix string @@ -396,34 +379,16 @@ func (gdu *GoProxyUrlParams) BuildUrl(url *url.URL, repoName string) string { return gdu.addDirect(url.String()) } -// separator returns the configured fallback separator, defaulting to the -// any-error form so callers that predate this field keep their behavior. -func (gdu *GoProxyUrlParams) separator() string { - if gdu.Separator == GoProxyNotFoundSeparator { - return GoProxyNotFoundSeparator - } - return GoProxyAnyErrorSeparator -} - -// hasDirectFallback reports whether the url already ends in a "direct" fallback -// entry, mirroring how the go command itself parses GOPROXY: entries are split -// on "," or "|" and trimmed before comparison, so "url , direct" counts. The -// comparison is case-sensitive for the same reason Go's is - "DIRECT" is not the -// direct keyword to Go either, it is parsed as a proxy URL and rejected with -// "invalid proxy URL missing scheme". -func hasDirectFallback(url string) bool { - i := strings.LastIndexAny(url, GoProxyAnyErrorSeparator+GoProxyNotFoundSeparator) - if i < 0 { - return false - } - return strings.TrimSpace(url[i+1:]) == goProxyDirect -} - func (gdu *GoProxyUrlParams) addDirect(url string) string { - if !gdu.Direct || hasDirectFallback(url) { - return url + if gdu.Direct && !strings.HasSuffix(url, "|direct") { + // A comma limits the fallback to 404/410, so a Curation 403 fails + // instead of being satisfied from the module's public source. + if gdu.FallbackOnlyIfNotFound { + return url + ",direct" + } + return url + "|direct" } - return url + gdu.separator() + goProxyDirect + return url } func GetArtifactoryRemoteRepoUrl(serverDetails *config.ServerDetails, repo string, goProxyParams GoProxyUrlParams) (string, error) { diff --git a/artifactory/commands/golang/go_test.go b/artifactory/commands/golang/go_test.go index e152bb09..364acf74 100644 --- a/artifactory/commands/golang/go_test.go +++ b/artifactory/commands/golang/go_test.go @@ -137,12 +137,12 @@ func TestGetArtifactoryApiUrl(t *testing.T) { func TestGoProxyUrlParams_BuildUrl(t *testing.T) { testCases := []struct { - name string - RepoName string - Direct bool - Separator string - EndpointPrefix string - ExpectedUrl string + name string + RepoName string + Direct bool + FallbackOnlyIfNotFound bool + EndpointPrefix string + ExpectedUrl string }{ { name: "Url Without direct or Prefix", @@ -162,31 +162,24 @@ func TestGoProxyUrlParams_BuildUrl(t *testing.T) { ExpectedUrl: "https://test/prefix/api/go/go", }, { - name: "Empty separator keeps the any-error default", - RepoName: "go", - Direct: true, - Separator: "", - ExpectedUrl: "https://test/api/go/go|direct", - }, - { - name: "Comma separator limits fallback to 404/410", - RepoName: "go", - Direct: true, - Separator: GoProxyNotFoundSeparator, - ExpectedUrl: "https://test/api/go/go,direct", + name: "Zero value keeps the any-error pipe for existing callers", + RepoName: "go", + Direct: true, + FallbackOnlyIfNotFound: false, + ExpectedUrl: "https://test/api/go/go|direct", }, { - name: "Explicit pipe separator", - RepoName: "go", - Direct: true, - Separator: GoProxyAnyErrorSeparator, - ExpectedUrl: "https://test/api/go/go|direct", + name: "FallbackOnlyIfNotFound limits the fallback to 404/410", + RepoName: "go", + Direct: true, + FallbackOnlyIfNotFound: true, + ExpectedUrl: "https://test/api/go/go,direct", }, { - name: "Separator ignored when Direct is false", - RepoName: "go", - Separator: GoProxyNotFoundSeparator, - ExpectedUrl: "https://test/api/go/go", + name: "Ignored when Direct is false", + RepoName: "go", + FallbackOnlyIfNotFound: true, + ExpectedUrl: "https://test/api/go/go", }, } for _, testCase := range testCases { @@ -194,73 +187,11 @@ func TestGoProxyUrlParams_BuildUrl(t *testing.T) { remoteUrl, err := url.Parse("https://test") require.NoError(t, err) gdu := &GoProxyUrlParams{ - Direct: testCase.Direct, - Separator: testCase.Separator, - EndpointPrefix: testCase.EndpointPrefix, + Direct: testCase.Direct, + FallbackOnlyIfNotFound: testCase.FallbackOnlyIfNotFound, + EndpointPrefix: testCase.EndpointPrefix, } assert.Equalf(t, testCase.ExpectedUrl, gdu.BuildUrl(remoteUrl, testCase.RepoName), "BuildUrl(%v, %v)", remoteUrl, testCase.RepoName) }) } } - -// addDirect must never append a second fallback entry, whichever separator the -// url already carries - otherwise re-running setup would produce "...|direct,direct". -func TestGoProxyUrlParams_AddDirectIsIdempotent(t *testing.T) { - testCases := []struct { - name string - url string - separator string - }{ - {name: "pipe url, pipe separator", url: "https://test/api/go/go|direct", separator: GoProxyAnyErrorSeparator}, - {name: "comma url, comma separator", url: "https://test/api/go/go,direct", separator: GoProxyNotFoundSeparator}, - {name: "pipe url, comma separator", url: "https://test/api/go/go|direct", separator: GoProxyNotFoundSeparator}, - {name: "comma url, pipe separator", url: "https://test/api/go/go,direct", separator: GoProxyAnyErrorSeparator}, - } - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - gdu := &GoProxyUrlParams{Direct: true, Separator: testCase.separator} - assert.Equal(t, testCase.url, gdu.addDirect(testCase.url)) - }) - } -} - -// hasDirectFallback must agree with the go command's own GOPROXY parsing, which -// trims each entry before comparing it against "direct" and is case-sensitive. -// Verified against go1.26.5: "url , direct" and "url | direct" resolve normally, -// while "url,DIRECT" fails with "invalid proxy URL missing scheme". -func TestHasDirectFallback(t *testing.T) { - testCases := []struct { - name string - url string - expected bool - }{ - // Go accepts these, so they already carry a fallback. - {name: "comma, no space", url: "https://test/api/go/go,direct", expected: true}, - {name: "pipe, no space", url: "https://test/api/go/go|direct", expected: true}, - {name: "space after comma", url: "https://test/api/go/go, direct", expected: true}, - {name: "spaces around comma", url: "https://test/api/go/go , direct", expected: true}, - {name: "spaces around pipe", url: "https://test/api/go/go | direct", expected: true}, - {name: "tab after comma", url: "https://test/api/go/go,\tdirect", expected: true}, - // Go rejects these as proxy URLs, so they are not a fallback. - {name: "uppercase DIRECT", url: "https://test/api/go/go,DIRECT", expected: false}, - {name: "mixed case Direct", url: "https://test/api/go/go,Direct", expected: false}, - // No fallback entry at all. - {name: "no separator", url: "https://test/api/go/go", expected: false}, - {name: "separator but other entry", url: "https://test/api/go/go,https://other", expected: false}, - {name: "off is not direct", url: "https://test/api/go/go,off", expected: false}, - {name: "direct as a substring only", url: "https://test/api/go/go,directory", expected: false}, - {name: "comma in the path, no fallback", url: "https://test/api/go/go,v2", expected: false}, - } - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - assert.Equal(t, testCase.expected, hasDirectFallback(testCase.url)) - // A url Go already treats as having a fallback must not gain a second one. - gdu := &GoProxyUrlParams{Direct: true, Separator: GoProxyNotFoundSeparator} - if testCase.expected { - assert.Equal(t, testCase.url, gdu.addDirect(testCase.url)) - } else { - assert.Equal(t, testCase.url+",direct", gdu.addDirect(testCase.url)) - } - }) - } -} diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index 5d89d799..326e3c8a 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -374,13 +374,19 @@ func (sc *SetupCommand) configureGo() error { "Unset it in your shell config (e.g., .zshrc, .bashrc).", goProxyVal)) } repoWithCredsUrl, err := golang.GetArtifactoryRemoteRepoUrl(sc.serverDetails, sc.repoName, - golang.GoProxyUrlParams{Direct: true, Separator: golang.GoProxyNotFoundSeparator}) + golang.GoProxyUrlParams{Direct: true, FallbackOnlyIfNotFound: true}) if err != nil { return fmt.Errorf("failed to get Go repository URL: %w", err) } if err := biutils.RunGo([]string{"env", "-w", "GOPROXY=" + repoWithCredsUrl}, ""); err != nil { return fmt.Errorf("failed to set GOPROXY environment variable: %w", err) } + // This is a behavior change worth surfacing: previously any proxy error fell + // back to the module's public source, so an unreachable Artifactory still + // produced a working build. + log.Info("GOPROXY falls back to the module's source only for modules the repository does not serve (404/410). " + + "Any other error, including a Curation block or an unreachable Artifactory, now fails the command instead of " + + "resolving from the public internet.") return nil } From 7cb4cfdd6641c660d2c7b91a59ec8f5eedfee0d0 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Tue, 28 Jul 2026 16:48:30 +0300 Subject: [PATCH 04/15] RTECO-0000 - Cover the parameter shapes both commands actually pass The separator was only asserted through BuildUrl and the end-to-end TestSetupCommand_Go, which needs the `go` binary and writes a global env var, so it cannot run everywhere. Add unit coverage for the shapes the two callers really use, and for the combination that was missing: - GetArtifactoryRemoteRepoUrl with {Direct: true, FallbackOnlyIfNotFound: true}, the exact call from configureGo, asserting the credentials are embedded, the url ends in ",direct", and no "|direct" appears. - GetArtifactoryRemoteRepoUrl with {Direct: true}, the `jf go` shape, asserting it still ends in "|direct" so the unchanged path is pinned. - getArtifactoryApiUrl with an endpoint prefix and the comma, so prefix and separator are exercised together on the credential-embedding path. - A BuildUrl case combining the prefix with the comma. Verified by mutation: reverting the comma in addDirect fails 5 subtests, dropping FallbackOnlyIfNotFound from configureGo fails TestSetupCommand_Go on all three auth cases, and forcing the comma for every caller fails 6 subtests - so the tests pin the fix in both directions. Co-Authored-By: Claude Opus 5 --- artifactory/commands/golang/go_test.go | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/artifactory/commands/golang/go_test.go b/artifactory/commands/golang/go_test.go index 364acf74..d84e0d52 100644 --- a/artifactory/commands/golang/go_test.go +++ b/artifactory/commands/golang/go_test.go @@ -93,6 +93,19 @@ func TestGetArtifactoryRemoteRepoUrl(t *testing.T) { repoUrl, err := GetArtifactoryRemoteRepoUrl(server, repoName, GoProxyUrlParams{}) assert.NoError(t, err) assert.Equal(t, "https://testuser:"+testFakeToken+"@server.com/artifactory/api/go/test-repo", repoUrl) + + // The exact parameters `jf setup go` passes. Asserted here because the only + // other coverage of this shape is TestSetupCommand_Go, which needs the `go` + // binary and writes a global env var, so it cannot run everywhere. + repoUrl, err = GetArtifactoryRemoteRepoUrl(server, repoName, GoProxyUrlParams{Direct: true, FallbackOnlyIfNotFound: true}) + assert.NoError(t, err) + assert.Equal(t, "https://testuser:"+testFakeToken+"@server.com/artifactory/api/go/test-repo,direct", repoUrl) + assert.NotContains(t, repoUrl, "|direct", "a pipe would fall back on any error, including a Curation 403") + + // The parameters `jf go` passes must keep falling back on any error. + repoUrl, err = GetArtifactoryRemoteRepoUrl(server, repoName, GoProxyUrlParams{Direct: true}) + assert.NoError(t, err) + assert.Equal(t, "https://testuser:"+testFakeToken+"@server.com/artifactory/api/go/test-repo|direct", repoUrl) } func TestGetArtifactoryApiUrl(t *testing.T) { @@ -117,6 +130,12 @@ func TestGetArtifactoryApiUrl(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "https://frog:testpass@test.com/artifactory/test/api/go/test-repo|direct", url) + // Same, but with the fallback limited to 404/410 - the credentials must still + // be embedded and the comma must survive alongside the endpoint prefix. + url, err = getArtifactoryApiUrl("test-repo", details, GoProxyUrlParams{EndpointPrefix: "test", Direct: true, FallbackOnlyIfNotFound: true}) + assert.NoError(t, err) + assert.Equal(t, "https://frog:testpass@test.com/artifactory/test/api/go/test-repo,direct", url) + // Test access token // Set fake access token with username "test" details.SetUser("testuser") @@ -181,6 +200,14 @@ func TestGoProxyUrlParams_BuildUrl(t *testing.T) { FallbackOnlyIfNotFound: true, ExpectedUrl: "https://test/api/go/go", }, + { + name: "Comma fallback together with an endpoint prefix", + RepoName: "go", + Direct: true, + FallbackOnlyIfNotFound: true, + EndpointPrefix: "prefix", + ExpectedUrl: "https://test/prefix/api/go/go,direct", + }, } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { From 3a611773dec72c5c0b756ce0732e0cd17c1df825 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 11:05:05 +0300 Subject: [PATCH 05/15] Say which configuration jf setup changed and how widely it applies jf setup writes user-level package manager configuration, so a run in one project silently changes resolution for every other project the user builds. The only output was "Successfully configured to use JFrog repository ''", which says nothing about the file that changed or its scope, so unrelated builds could start resolving elsewhere with nothing to trace it back to. Add a note naming the configuration and stating that it applies beyond the current directory. Container logins get a credentials-only wording, since docker/podman/helm authenticate rather than redirect resolution and an unqualified 'docker pull alpine' still reaches Docker Hub. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/setup.go | 51 ++++++++++++++++++++++++ artifactory/commands/setup/setup_test.go | 36 +++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index 326e3c8a..d44c9aef 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -33,6 +33,54 @@ import ( "golang.org/x/exp/maps" ) +// packageManagerToConfigLocation names the configuration each package manager is +// written to, so the command can say what it changed. Every entry is user-level: +// the settings live in the user's home directory, not in the current project, so +// they take effect for every project this user builds. Saying so matters because +// nothing else in the output reveals that the change is not scoped to the +// directory the command was run in. +var packageManagerToConfigLocation = map[project.ProjectType]string{ + project.Npm: "your user-level npm configuration (.npmrc)", + project.Pnpm: "your user-level pnpm configuration (.npmrc)", + project.Yarn: "your user-level Yarn configuration (.yarnrc)", + project.Pip: "your user-level pip configuration (pip.conf)", + project.Pipenv: "your user-level pip configuration (pip.conf)", + project.Poetry: "your user-level Poetry configuration", + project.Twine: "your user-level Twine configuration (.pypirc)", + project.UV: "your user-level uv configuration (uv.toml)", + project.Nuget: "your user-level NuGet configuration (NuGet.Config)", + project.Dotnet: "your user-level NuGet configuration (NuGet.Config)", + project.Go: "your Go environment (GOPROXY)", + project.Gradle: "a Gradle init script in your Gradle user home", + project.Maven: "your user-level Maven settings (settings.xml)", + project.Docker: "your Docker credential store", + project.Podman: "your Podman credential store", + project.Helm: "your Helm registry credential store", +} + +// containerLoginPackageManagers authenticate to the platform instead of redirecting +// resolution, so the note must not claim that their projects now resolve through +// Artifactory: an unqualified `docker pull alpine` still goes to Docker Hub. +var containerLoginPackageManagers = map[project.ProjectType]bool{ + project.Docker: true, + project.Podman: true, + project.Helm: true, +} + +// configScopeNote describes what the command changed and how widely it applies, or +// an empty string for a package manager we have nothing accurate to say about. +func configScopeNote(packageManager project.ProjectType) string { + location, ok := packageManagerToConfigLocation[packageManager] + if !ok { + return "" + } + if containerLoginPackageManagers[packageManager] { + return fmt.Sprintf("Credentials were saved to %s for your user account.", location) + } + return fmt.Sprintf("This updated %s, so it applies to every %s project for this user, not only the current directory.", + location, packageManager.String()) +} + // packageManagerToRepositoryPackageType maps project types to corresponding Artifactory repository package types. var packageManagerToRepositoryPackageType = map[project.ProjectType]string{ // Npm package managers @@ -195,6 +243,9 @@ func (sc *SetupCommand) Run() (err error) { repoPrefix = coreutils.PrintBoldTitle(fmt.Sprintf(" repository '%s'", sc.repoName)) } log.Output(fmt.Sprintf("Successfully configured %s to use JFrog%s.", coreutils.PrintBoldTitle(sc.packageManager.String()), repoPrefix)) + if note := configScopeNote(sc.packageManager); note != "" { + log.Output(note) + } return nil } diff --git a/artifactory/commands/setup/setup_test.go b/artifactory/commands/setup/setup_test.go index 64eae30b..15f8ba18 100644 --- a/artifactory/commands/setup/setup_test.go +++ b/artifactory/commands/setup/setup_test.go @@ -929,3 +929,39 @@ func TestSetupCommand_MavenCorrupted(t *testing.T) { assert.NotContains(t, content, testCredential(), "Old token should be replaced") }) } + +// Every supported package manager must describe what it changed: the output is the +// only place a user learns the configuration is user-level rather than scoped to the +// directory they ran the command in. +func TestConfigScopeNote_CoversEverySupportedPackageManager(t *testing.T) { + for _, name := range GetSupportedPackageManagersList() { + packageManager := project.FromString(name) + note := configScopeNote(packageManager) + assert.NotEmptyf(t, note, "no scope note for supported package manager %q", name) + assert.Containsf(t, note, "your", "scope note for %q should name the configuration it changed: %s", name, note) + } +} + +// Container logins authenticate rather than redirect resolution, so their note must +// not promise that projects now resolve through Artifactory — an unqualified +// `docker pull alpine` still reaches Docker Hub after `jf setup docker`. +func TestConfigScopeNote_ContainerLoginsDoNotClaimResolution(t *testing.T) { + for _, packageManager := range []project.ProjectType{project.Docker, project.Podman, project.Helm} { + note := configScopeNote(packageManager) + assert.Contains(t, note, "Credentials were saved", packageManager.String()) + assert.NotContains(t, note, "applies to every", packageManager.String()) + } + + // Resolution-changing package managers must state the scope explicitly. + for _, packageManager := range []project.ProjectType{project.Npm, project.Maven, project.Go, project.Pip} { + note := configScopeNote(packageManager) + assert.Contains(t, note, "applies to every", packageManager.String()) + assert.Contains(t, note, "not only the current directory", packageManager.String()) + } +} + +// An unsupported package manager has nothing accurate to say, so it must stay silent +// rather than print a misleading note. +func TestConfigScopeNote_UnknownPackageManagerIsSilent(t *testing.T) { + assert.Empty(t, configScopeNote(project.Cocoapods)) +} From 073c5c596a25c42200e68e6a37e216a566e43fa6 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 11:35:29 +0300 Subject: [PATCH 06/15] Do not claim user-level scope when the configuration was redirected Four package managers honor an environment variable that moves their config somewhere other than the user-level default: PIP_CONFIG_FILE (pip, pipenv), NPM_CONFIG_USERCONFIG (npm, pnpm), POETRY_CONFIG_DIR and UV_CONFIG_FILE. The redirected file can sit anywhere the user pointed it, including inside the current project, so stating that the change applies to every project for this user was wrong in exactly those cases. Report the path and the variable that redirected it instead, and defer the scope to that file. Also replace the container-login set with a predicate, and assert the two note shapes per package manager rather than matching a shared word. Note the existing Docker/Podman checks are deliberately left alone: Helm shares the credentials-only wording but still goes through repository selection, so folding those call sites into the predicate would change its behavior. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/setup.go | 38 ++++++++++++++---- artifactory/commands/setup/setup_test.go | 51 +++++++++++++++++++++++- 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index d44c9aef..8bf1376c 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -58,13 +58,27 @@ var packageManagerToConfigLocation = map[project.ProjectType]string{ project.Helm: "your Helm registry credential store", } -// containerLoginPackageManagers authenticate to the platform instead of redirecting -// resolution, so the note must not claim that their projects now resolve through -// Artifactory: an unqualified `docker pull alpine` still goes to Docker Hub. -var containerLoginPackageManagers = map[project.ProjectType]bool{ - project.Docker: true, - project.Podman: true, - project.Helm: true, +// packageManagerConfigOverrideEnv names the environment variable that redirects a +// package manager's configuration away from its user-level default. When one is set +// the file can live anywhere the user pointed it, including inside the current +// project, so the note must describe that path instead of claiming user-wide scope. +// Only package managers whose configure function actually honors the variable are +// listed here. +var packageManagerConfigOverrideEnv = map[project.ProjectType]string{ + project.Npm: "NPM_CONFIG_USERCONFIG", + project.Pnpm: "NPM_CONFIG_USERCONFIG", + project.Pip: "PIP_CONFIG_FILE", + project.Pipenv: "PIP_CONFIG_FILE", + project.Poetry: "POETRY_CONFIG_DIR", + project.UV: "UV_CONFIG_FILE", +} + +// isContainerLogin reports whether the package manager authenticates to the platform +// instead of redirecting resolution. Their projects do not start resolving through +// Artifactory — an unqualified `docker pull alpine` still goes to Docker Hub — so the +// note must not claim otherwise. +func isContainerLogin(packageManager project.ProjectType) bool { + return packageManager == project.Docker || packageManager == project.Podman || packageManager == project.Helm } // configScopeNote describes what the command changed and how widely it applies, or @@ -74,9 +88,17 @@ func configScopeNote(packageManager project.ProjectType) string { if !ok { return "" } - if containerLoginPackageManagers[packageManager] { + if isContainerLogin(packageManager) { return fmt.Sprintf("Credentials were saved to %s for your user account.", location) } + // A redirected configuration is not user-level, so report where it actually went + // rather than promising a scope that may not hold. + if envVar, hasOverride := packageManagerConfigOverrideEnv[packageManager]; hasOverride { + if overridePath := os.Getenv(envVar); overridePath != "" { + return fmt.Sprintf("This updated the %s configuration at %s, because %s is set, so its scope follows that path rather than your user-level configuration.", + packageManager.String(), overridePath, envVar) + } + } return fmt.Sprintf("This updated %s, so it applies to every %s project for this user, not only the current directory.", location, packageManager.String()) } diff --git a/artifactory/commands/setup/setup_test.go b/artifactory/commands/setup/setup_test.go index 15f8ba18..7e232dc8 100644 --- a/artifactory/commands/setup/setup_test.go +++ b/artifactory/commands/setup/setup_test.go @@ -936,9 +936,56 @@ func TestSetupCommand_MavenCorrupted(t *testing.T) { func TestConfigScopeNote_CoversEverySupportedPackageManager(t *testing.T) { for _, name := range GetSupportedPackageManagersList() { packageManager := project.FromString(name) + require.NotEqualf(t, project.ProjectType(-1), packageManager, "%q is not a known project type", name) + + // Clear any override so this asserts the default, user-level wording. + if envVar, hasOverride := packageManagerConfigOverrideEnv[packageManager]; hasOverride { + t.Setenv(envVar, "") + } + + note := configScopeNote(packageManager) + require.NotEmptyf(t, note, "no scope note for supported package manager %q", name) + + // Each note must take exactly one of the two valid shapes, and a resolution + // note must name the package manager it is talking about. + if isContainerLogin(packageManager) { + assert.Containsf(t, note, "Credentials were saved", "%q: %s", name, note) + assert.NotContainsf(t, note, "applies to every", "%q must not claim resolution: %s", name, note) + continue + } + assert.Containsf(t, note, fmt.Sprintf("applies to every %s project", name), "%q: %s", name, note) + assert.NotContainsf(t, note, "Credentials were saved", "%q: %s", name, note) + } +} + +// A configuration redirected by an environment variable is not user-level — it can sit +// inside the current project — so the note must report that path instead of promising +// a scope that does not hold. +func TestConfigScopeNote_RedirectedConfigDoesNotClaimUserScope(t *testing.T) { + require.NotEmpty(t, packageManagerConfigOverrideEnv, "expected package managers with a config override") + + for packageManager, envVar := range packageManagerConfigOverrideEnv { + overridePath := filepath.Join(t.TempDir(), "redirected-config") + t.Setenv(envVar, overridePath) + + note := configScopeNote(packageManager) + assert.Containsf(t, note, overridePath, "%s: note should name the redirected path: %s", packageManager.String(), note) + assert.Containsf(t, note, envVar, "%s: note should name the variable that redirected it: %s", packageManager.String(), note) + // The scope claim itself is what must not survive a redirect; the note may still + // mention user-level configuration to contrast against it. + assert.NotContainsf(t, note, "applies to every", "%s must not claim project-wide scope when redirected: %s", packageManager.String(), note) + assert.Containsf(t, note, "scope follows that path", "%s should defer scope to the redirected file: %s", packageManager.String(), note) + } +} + +// With no override set the default user-level wording applies, so the two branches are +// covered in both directions. +func TestConfigScopeNote_WithoutOverrideClaimsUserScope(t *testing.T) { + for packageManager, envVar := range packageManagerConfigOverrideEnv { + t.Setenv(envVar, "") note := configScopeNote(packageManager) - assert.NotEmptyf(t, note, "no scope note for supported package manager %q", name) - assert.Containsf(t, note, "your", "scope note for %q should name the configuration it changed: %s", name, note) + assert.Containsf(t, note, "user-level", "%s: %s", packageManager.String(), note) + assert.NotContainsf(t, note, envVar, "%s: %s", packageManager.String(), note) } } From 1e96470298b0eabdfc6f5b5f2fc70f7e42758baf Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 12:11:57 +0300 Subject: [PATCH 07/15] Assert the command prints the scope note, not just that it can build one Every configScopeNote test called the builder directly, so deleting the log call in Run would have left them all passing while the user saw nothing. Capture the command output and assert the note reaches it. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/setup_test.go | 26 ++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/artifactory/commands/setup/setup_test.go b/artifactory/commands/setup/setup_test.go index 7e232dc8..89815308 100644 --- a/artifactory/commands/setup/setup_test.go +++ b/artifactory/commands/setup/setup_test.go @@ -1,6 +1,7 @@ package setup import ( + "bytes" "fmt" "net/http" "net/http/httptest" @@ -20,6 +21,7 @@ import ( "github.com/jfrog/jfrog-cli-core/v2/utils/ioutils" "github.com/jfrog/jfrog-client-go/auth" "github.com/jfrog/jfrog-client-go/utils/io" + "github.com/jfrog/jfrog-client-go/utils/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/exp/slices" @@ -1012,3 +1014,27 @@ func TestConfigScopeNote_ContainerLoginsDoNotClaimResolution(t *testing.T) { func TestConfigScopeNote_UnknownPackageManagerIsSilent(t *testing.T) { assert.Empty(t, configScopeNote(project.Cocoapods)) } + +// The note is only useful if the command actually prints it, so assert the wiring +// rather than just the string builder: removing the log call would otherwise leave +// every configScopeNote test passing. +func TestSetupCommand_PrintsConfigScopeNote(t *testing.T) { + // Maven writes only settings.xml, so a temporary home keeps the run self-contained. + // Both variables are set for cross-platform parity with the other Maven tests. + tempHome := t.TempDir() + t.Setenv("HOME", tempHome) + t.Setenv("USERPROFILE", tempHome) + + var output bytes.Buffer + previousLogger := log.Logger + log.SetLogger(log.NewLogger(log.INFO, &output)) + defer log.SetLogger(previousLogger) + + setupCmd := createTestSetupCommand(project.Maven) + setupCmd.repoName = "test-repo" + require.NoError(t, setupCmd.Run()) + + assert.Contains(t, output.String(), "Successfully configured", "expected the success message") + assert.Contains(t, output.String(), configScopeNote(project.Maven), + "the command must print the scope note, not just be able to build it") +} From 90b430092fc831db86d736088ad68cf6247d09f5 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 14:22:09 +0300 Subject: [PATCH 08/15] Guard the direct fallback for both separators and name them The suffix check only recognised "|direct", so a url that already carried the comma form got a second entry appended when FallbackOnlyIfNotFound was set. Check both forms in one helper, and give the entry and the two separators names so the difference between them - fall back on any error versus only on 404/410 - is stated once instead of implied by punctuation at the call site. Co-Authored-By: Claude Opus 5 --- artifactory/commands/golang/go.go | 36 ++++++++++++++++++-------- artifactory/commands/golang/go_test.go | 19 ++++++++++++++ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/artifactory/commands/golang/go.go b/artifactory/commands/golang/go.go index cde142fe..db7070a9 100644 --- a/artifactory/commands/golang/go.go +++ b/artifactory/commands/golang/go.go @@ -356,6 +356,16 @@ func logGoVersion() error { return nil } +// The "direct" GOPROXY entry tells the go command to fetch the module from its own +// source. The separator in front of it decides when that happens: a pipe falls back +// on any proxy error, a comma only on 404/410 - so with a comma a Curation 403 is a +// hard failure instead of being quietly satisfied from the module's public source. +const ( + directFallbackEntry = "direct" + anyErrorFallbackSeparator = "|" + notFoundOnlyFallbackSeparator = "," +) + type GoProxyUrlParams struct { // Fallback to retrieve the modules directly from the source if // the module failed to be retrieved from the proxy. @@ -364,9 +374,8 @@ type GoProxyUrlParams struct { Direct bool // FallbackOnlyIfNotFound joins the "direct" entry with a comma instead of a // pipe, so the go command falls through to the module's public source only - // after a 404/410 and a 403 from Artifactory Curation is a hard failure. - // The zero value keeps the previous any-error (pipe) behavior for existing - // callers. Ignored when Direct is false. + // after a 404/410. The zero value keeps the previous any-error (pipe) + // behavior for existing callers. Ignored when Direct is false. FallbackOnlyIfNotFound bool // The path from baseUrl to the standard Go repository path // URL structure: //api/go/ @@ -380,15 +389,20 @@ func (gdu *GoProxyUrlParams) BuildUrl(url *url.URL, repoName string) string { } func (gdu *GoProxyUrlParams) addDirect(url string) string { - if gdu.Direct && !strings.HasSuffix(url, "|direct") { - // A comma limits the fallback to 404/410, so a Curation 403 fails - // instead of being satisfied from the module's public source. - if gdu.FallbackOnlyIfNotFound { - return url + ",direct" - } - return url + "|direct" + if !gdu.Direct || hasDirectFallback(url) { + return url } - return url + if gdu.FallbackOnlyIfNotFound { + return url + notFoundOnlyFallbackSeparator + directFallbackEntry + } + return url + anyErrorFallbackSeparator + directFallbackEntry +} + +// hasDirectFallback reports whether the url already ends with a "direct" entry, with +// either separator, so that an already-configured fallback is never appended twice. +func hasDirectFallback(url string) bool { + return strings.HasSuffix(url, anyErrorFallbackSeparator+directFallbackEntry) || + strings.HasSuffix(url, notFoundOnlyFallbackSeparator+directFallbackEntry) } func GetArtifactoryRemoteRepoUrl(serverDetails *config.ServerDetails, repo string, goProxyParams GoProxyUrlParams) (string, error) { diff --git a/artifactory/commands/golang/go_test.go b/artifactory/commands/golang/go_test.go index d84e0d52..d0022816 100644 --- a/artifactory/commands/golang/go_test.go +++ b/artifactory/commands/golang/go_test.go @@ -154,6 +154,25 @@ func TestGetArtifactoryApiUrl(t *testing.T) { assert.Equal(t, "https://frog:"+testFakeToken+"@test.com/artifactory/api/go/test-repo", url) } +func TestGoProxyUrlParams_AddDirectIsIdempotentForBothSeparators(t *testing.T) { + testCases := []struct { + name string + url string + FallbackOnlyIfNotFound bool + }{ + {name: "Pipe fallback is not appended twice", url: "https://test/api/go/go|direct"}, + {name: "Comma fallback is not appended twice", url: "https://test/api/go/go,direct", FallbackOnlyIfNotFound: true}, + {name: "Comma fallback is left alone when a pipe was requested", url: "https://test/api/go/go,direct"}, + {name: "Pipe fallback is left alone when a comma was requested", url: "https://test/api/go/go|direct", FallbackOnlyIfNotFound: true}, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + gdu := &GoProxyUrlParams{Direct: true, FallbackOnlyIfNotFound: testCase.FallbackOnlyIfNotFound} + assert.Equal(t, testCase.url, gdu.addDirect(testCase.url)) + }) + } +} + func TestGoProxyUrlParams_BuildUrl(t *testing.T) { testCases := []struct { name string From ecbbad005770fc556e2605c5e0e79857165e97a2 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 14:23:39 +0300 Subject: [PATCH 09/15] Describe each package manager's configuration in one verified table Three maps keyed by the same package manager described the same configuration between them, and the override map was partly wrong. Fold them into one entry per package manager carrying the location, whether the setup only stores credentials, and the environment variable that redirects it, with the evidence for each override recorded next to it. The corrections that came out of checking every entry against the tool that consumes it: - pnpm claimed NPM_CONFIG_USERCONFIG and a .npmrc. `pnpm config set` writes auth.ini in pnpm's own config directory and ignores that variable, which it reads only as a credentials fallback, so the note pointed at a file jf setup never touched. - GOENV and GRADLE_USER_HOME were missing even though both are honored: `go env -w` writes where GOENV points, and WriteInitScript reads GRADLE_USER_HOME before falling back to the user's Gradle home. - npm, pip, pipenv, poetry and uv were confirmed against their documentation and by running each tool; yarn, twine, nuget, dotnet and maven take their config path per invocation, not from the environment, so they stay without an entry. The override set is asserted exactly, in both directions, because an entry that looks consistent by name - pnpm next to npm - is the way a wrong path gets added back. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/setup.go | 112 ++++++++++++----------- artifactory/commands/setup/setup_test.go | 55 ++++++++++- 2 files changed, 111 insertions(+), 56 deletions(-) diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index 8bf1376c..95bf564b 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -33,74 +33,84 @@ import ( "golang.org/x/exp/maps" ) -// packageManagerToConfigLocation names the configuration each package manager is -// written to, so the command can say what it changed. Every entry is user-level: -// the settings live in the user's home directory, not in the current project, so -// they take effect for every project this user builds. Saying so matters because -// nothing else in the output reveals that the change is not scoped to the -// directory the command was run in. -var packageManagerToConfigLocation = map[project.ProjectType]string{ - project.Npm: "your user-level npm configuration (.npmrc)", - project.Pnpm: "your user-level pnpm configuration (.npmrc)", - project.Yarn: "your user-level Yarn configuration (.yarnrc)", - project.Pip: "your user-level pip configuration (pip.conf)", - project.Pipenv: "your user-level pip configuration (pip.conf)", - project.Poetry: "your user-level Poetry configuration", - project.Twine: "your user-level Twine configuration (.pypirc)", - project.UV: "your user-level uv configuration (uv.toml)", - project.Nuget: "your user-level NuGet configuration (NuGet.Config)", - project.Dotnet: "your user-level NuGet configuration (NuGet.Config)", - project.Go: "your Go environment (GOPROXY)", - project.Gradle: "a Gradle init script in your Gradle user home", - project.Maven: "your user-level Maven settings (settings.xml)", - project.Docker: "your Docker credential store", - project.Podman: "your Podman credential store", - project.Helm: "your Helm registry credential store", +// packageManagerConfig describes the configuration `jf setup` writes for one package +// manager, so the command can say what it changed and how far the change reaches. +// Saying so matters because nothing else in the output reveals that the change is not +// scoped to the directory the command was run in. +type packageManagerConfig struct { + // location names the configuration in the user's terms rather than as an absolute + // path: the real path is platform dependent (pip alone has three per-user + // locations) and the package manager resolves it itself, so a path spelled out + // here would be wrong on some machines. + location string + // credentialsOnly marks the package managers that only store credentials instead + // of redirecting resolution. Their projects do not start resolving through + // Artifactory — an unqualified `docker pull alpine` still goes to Docker Hub — so + // the note must not claim otherwise. + credentialsOnly bool + // overrideEnv is the environment variable that moves this configuration off its + // user-level default. When it is set the configuration can live anywhere the user + // pointed it, including inside the current project, so the note has to describe + // that path instead of claiming user-wide scope. Only set where the configure + // function or the tool it drives really honors the variable — the per-entry + // comments record what was verified. + overrideEnv string } -// packageManagerConfigOverrideEnv names the environment variable that redirects a -// package manager's configuration away from its user-level default. When one is set -// the file can live anywhere the user pointed it, including inside the current -// project, so the note must describe that path instead of claiming user-wide scope. -// Only package managers whose configure function actually honors the variable are -// listed here. -var packageManagerConfigOverrideEnv = map[project.ProjectType]string{ - project.Npm: "NPM_CONFIG_USERCONFIG", - project.Pnpm: "NPM_CONFIG_USERCONFIG", - project.Pip: "PIP_CONFIG_FILE", - project.Pipenv: "PIP_CONFIG_FILE", - project.Poetry: "POETRY_CONFIG_DIR", - project.UV: "UV_CONFIG_FILE", -} - -// isContainerLogin reports whether the package manager authenticates to the platform -// instead of redirecting resolution. Their projects do not start resolving through -// Artifactory — an unqualified `docker pull alpine` still goes to Docker Hub — so the -// note must not claim otherwise. -func isContainerLogin(packageManager project.ProjectType) bool { - return packageManager == project.Docker || packageManager == project.Podman || packageManager == project.Helm +// One entry per package manager in packageManagerToRepositoryPackageType; a test +// asserts the two stay in step. +var packageManagerConfigs = map[project.ProjectType]packageManagerConfig{ + // npm resolves the file `npm config set` writes through NPM_CONFIG_USERCONFIG. + project.Npm: {location: "your user-level npm configuration (.npmrc)", overrideEnv: "NPM_CONFIG_USERCONFIG"}, + // pnpm is deliberately left without an override: `pnpm config set` writes to + // pnpm's own config directory (auth.ini) and ignores NPM_CONFIG_USERCONFIG, which + // it consults only as a fallback when reading credentials. + project.Pnpm: {location: "your user-level pnpm configuration (auth.ini in pnpm's config directory)"}, + // Yarn Classic writes ~/.yarnrc, and YARN_RC_FILENAME does not redirect it. + project.Yarn: {location: "your user-level Yarn configuration (.yarnrc)"}, + // configurePip writes the file itself when PIP_CONFIG_FILE is set, because + // `pip config set` does not support that variable. + project.Pip: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE"}, + project.Pipenv: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE"}, + // `poetry config` writes config.toml into POETRY_CONFIG_DIR when it is set. + project.Poetry: {location: "your user-level Poetry configuration (config.toml)", overrideEnv: "POETRY_CONFIG_DIR"}, + // Twine's .pypirc path is chosen per invocation (--config-file), not by the environment. + project.Twine: {location: "your user-level Twine configuration (.pypirc)"}, + // ConfigureUVIndex writes to UV_CONFIG_FILE when it is set. + project.UV: {location: "your user-level uv configuration (uv.toml)", overrideEnv: "UV_CONFIG_FILE"}, + project.Nuget: {location: "your user-level NuGet configuration (NuGet.Config)"}, + project.Dotnet: {location: "your user-level NuGet configuration (NuGet.Config)"}, + // `go env -w` writes to the file GOENV points at, defaulting to the per-user Go env file. + project.Go: {location: "your user-level Go environment (GOPROXY in your Go env file)", overrideEnv: "GOENV"}, + // gradle.WriteInitScript drops the script under GRADLE_USER_HOME when it is set. + project.Gradle: {location: "your user-level Gradle configuration (an init script in your Gradle user home)", overrideEnv: gradle.UserHomeEnv}, + // Maven picks the settings file per invocation (-s), not from the environment. + project.Maven: {location: "your user-level Maven settings (settings.xml)"}, + project.Docker: {location: "your Docker credential store", credentialsOnly: true}, + project.Podman: {location: "your Podman credential store", credentialsOnly: true}, + project.Helm: {location: "your Helm registry credential store", credentialsOnly: true}, } // configScopeNote describes what the command changed and how widely it applies, or // an empty string for a package manager we have nothing accurate to say about. func configScopeNote(packageManager project.ProjectType) string { - location, ok := packageManagerToConfigLocation[packageManager] + packageManagerConfig, ok := packageManagerConfigs[packageManager] if !ok { return "" } - if isContainerLogin(packageManager) { - return fmt.Sprintf("Credentials were saved to %s for your user account.", location) + if packageManagerConfig.credentialsOnly { + return fmt.Sprintf("Credentials were saved to %s for your user account.", packageManagerConfig.location) } // A redirected configuration is not user-level, so report where it actually went // rather than promising a scope that may not hold. - if envVar, hasOverride := packageManagerConfigOverrideEnv[packageManager]; hasOverride { - if overridePath := os.Getenv(envVar); overridePath != "" { + if packageManagerConfig.overrideEnv != "" { + if overridePath := os.Getenv(packageManagerConfig.overrideEnv); overridePath != "" { return fmt.Sprintf("This updated the %s configuration at %s, because %s is set, so its scope follows that path rather than your user-level configuration.", - packageManager.String(), overridePath, envVar) + packageManager.String(), overridePath, packageManagerConfig.overrideEnv) } } return fmt.Sprintf("This updated %s, so it applies to every %s project for this user, not only the current directory.", - location, packageManager.String()) + packageManagerConfig.location, packageManager.String()) } // packageManagerToRepositoryPackageType maps project types to corresponding Artifactory repository package types. diff --git a/artifactory/commands/setup/setup_test.go b/artifactory/commands/setup/setup_test.go index 89815308..e275dc83 100644 --- a/artifactory/commands/setup/setup_test.go +++ b/artifactory/commands/setup/setup_test.go @@ -941,7 +941,7 @@ func TestConfigScopeNote_CoversEverySupportedPackageManager(t *testing.T) { require.NotEqualf(t, project.ProjectType(-1), packageManager, "%q is not a known project type", name) // Clear any override so this asserts the default, user-level wording. - if envVar, hasOverride := packageManagerConfigOverrideEnv[packageManager]; hasOverride { + if envVar := packageManagerConfigs[packageManager].overrideEnv; envVar != "" { t.Setenv(envVar, "") } @@ -950,7 +950,7 @@ func TestConfigScopeNote_CoversEverySupportedPackageManager(t *testing.T) { // Each note must take exactly one of the two valid shapes, and a resolution // note must name the package manager it is talking about. - if isContainerLogin(packageManager) { + if packageManagerConfigs[packageManager].credentialsOnly { assert.Containsf(t, note, "Credentials were saved", "%q: %s", name, note) assert.NotContainsf(t, note, "applies to every", "%q must not claim resolution: %s", name, note) continue @@ -964,9 +964,10 @@ func TestConfigScopeNote_CoversEverySupportedPackageManager(t *testing.T) { // inside the current project — so the note must report that path instead of promising // a scope that does not hold. func TestConfigScopeNote_RedirectedConfigDoesNotClaimUserScope(t *testing.T) { - require.NotEmpty(t, packageManagerConfigOverrideEnv, "expected package managers with a config override") + overrides := packageManagersWithConfigOverride() + require.NotEmpty(t, overrides, "expected package managers with a config override") - for packageManager, envVar := range packageManagerConfigOverrideEnv { + for packageManager, envVar := range overrides { overridePath := filepath.Join(t.TempDir(), "redirected-config") t.Setenv(envVar, overridePath) @@ -983,7 +984,7 @@ func TestConfigScopeNote_RedirectedConfigDoesNotClaimUserScope(t *testing.T) { // With no override set the default user-level wording applies, so the two branches are // covered in both directions. func TestConfigScopeNote_WithoutOverrideClaimsUserScope(t *testing.T) { - for packageManager, envVar := range packageManagerConfigOverrideEnv { + for packageManager, envVar := range packageManagersWithConfigOverride() { t.Setenv(envVar, "") note := configScopeNote(packageManager) assert.Containsf(t, note, "user-level", "%s: %s", packageManager.String(), note) @@ -991,6 +992,50 @@ func TestConfigScopeNote_WithoutOverrideClaimsUserScope(t *testing.T) { } } +// Each override was verified against the tool that consumes it, so the set is pinned in +// both directions: a new entry added without that verification fails here, and dropping +// one that works silently downgrades the note to a scope claim that may be wrong. +func TestPackageManagerConfigs_OverridesAreExactlyTheVerifiedSet(t *testing.T) { + expected := map[project.ProjectType]string{ + project.Npm: "NPM_CONFIG_USERCONFIG", + project.Pip: "PIP_CONFIG_FILE", + project.Pipenv: "PIP_CONFIG_FILE", + project.Poetry: "POETRY_CONFIG_DIR", + project.UV: "UV_CONFIG_FILE", + project.Go: "GOENV", + project.Gradle: "GRADLE_USER_HOME", + } + assert.Equal(t, expected, packageManagersWithConfigOverride()) +} + +// pnpm looks like it should follow npm here, and it does not: `pnpm config set` writes to +// pnpm's own config directory and ignores NPM_CONFIG_USERCONFIG (verified against pnpm +// 11 — the file it wrote was auth.ini under the pnpm config directory, both with and +// without the variable set). Claiming the redirect would send users to a file that +// `jf setup pnpm` never touched, so the absence is asserted rather than left to chance. +func TestPackageManagerConfigs_PnpmHasNoConfigOverride(t *testing.T) { + assert.Empty(t, packageManagerConfigs[project.Pnpm].overrideEnv, + "pnpm config set does not honor an environment override") + + customConfig := filepath.Join(t.TempDir(), "custom.npmrc") + t.Setenv("NPM_CONFIG_USERCONFIG", customConfig) + note := configScopeNote(project.Pnpm) + assert.NotContains(t, note, customConfig, "the note must not point at a file pnpm does not write: "+note) + assert.Contains(t, note, "applies to every pnpm project", note) +} + +// packageManagersWithConfigOverride returns only the entries that declare an override +// variable, so the override tests do not have to skip the rest. +func packageManagersWithConfigOverride() map[project.ProjectType]string { + overrides := map[project.ProjectType]string{} + for packageManager, packageManagerConfig := range packageManagerConfigs { + if packageManagerConfig.overrideEnv != "" { + overrides[packageManager] = packageManagerConfig.overrideEnv + } + } + return overrides +} + // Container logins authenticate rather than redirect resolution, so their note must // not promise that projects now resolve through Artifactory — an unqualified // `docker pull alpine` still reaches Docker Hub after `jf setup docker`. From 061a1b9949ab8185a71bba13b0e910aed37cb77d Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 14:24:20 +0300 Subject: [PATCH 10/15] Keep the npm and pnpm setup tests out of the developer's real configuration The shared test redirected NPM_CONFIG_USERCONFIG into a temp directory and read the .npmrc back from it. pnpm ignores that variable, so the pnpm cases configured the developer's real pnpm configuration - writing the test repository and its fake token there - and then failed on a .npmrc that was never created. Redirect every home and config directory variable the two package managers consult into the temp directory, and assert against whichever configuration file was actually written: npm produces the file NPM_CONFIG_USERCONFIG names, pnpm produces auth.ini under a config directory whose path differs per platform. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/setup_test.go | 45 ++++++++++++++++++++---- 1 file changed, 38 insertions(+), 7 deletions(-) diff --git a/artifactory/commands/setup/setup_test.go b/artifactory/commands/setup/setup_test.go index e275dc83..f09b55d8 100644 --- a/artifactory/commands/setup/setup_test.go +++ b/artifactory/commands/setup/setup_test.go @@ -3,6 +3,7 @@ package setup import ( "bytes" "fmt" + "io/fs" "net/http" "net/http/httptest" "os" @@ -93,6 +94,14 @@ func testSetupCommandNpmPnpm(t *testing.T, packageManager project.ProjectType) { // Set NPM_CONFIG_USERCONFIG to point to the temporary npmrc file path. t.Setenv("NPM_CONFIG_USERCONFIG", npmrcFilePath) + // `pnpm config set` ignores NPM_CONFIG_USERCONFIG and writes to its own config + // directory instead, so every variable that locates a home or config directory + // is redirected into tempDir as well. Without this the pnpm cases write into + // the developer's real pnpm configuration and then fail on the missing .npmrc. + t.Setenv("HOME", tempDir) + t.Setenv("USERPROFILE", tempDir) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tempDir, "xdg")) + t.Setenv("LOCALAPPDATA", filepath.Join(tempDir, "localappdata")) // Set up server details for the current test case's authentication type. loginCmd := createTestSetupCommand(packageManager) @@ -103,10 +112,7 @@ func testSetupCommandNpmPnpm(t *testing.T, packageManager project.ProjectType) { // Run the login command and ensure no errors occur. require.NoError(t, loginCmd.Run()) - // Read the contents of the temporary npmrc file. - npmrcContentBytes, err := os.ReadFile(npmrcFilePath) - assert.NoError(t, err) - npmrcContent := string(npmrcContentBytes) + npmrcContent := readPackageManagerConfigs(t, tempDir) // Validate that the registry URL was set correctly in .npmrc. assert.Contains(t, npmrcContent, fmt.Sprintf("%s=%s", cmdutils.NpmConfigRegistryKey, "https://acme.jfrog.io/artifactory/api/npm/test-repo/")) @@ -120,13 +126,38 @@ func testSetupCommandNpmPnpm(t *testing.T, packageManager project.ProjectType) { expectedBasicAuth := fmt.Sprintf("//acme.jfrog.io/artifactory/api/npm/test-repo/:%s=\"bXlVc2VyOm15UGFzc3dvcmQ=\"", cmdutils.NpmConfigAuthKey) assert.Contains(t, npmrcContent, expectedBasicAuth) } - - // Clean up the temporary npmrc file. - assert.NoError(t, os.Remove(npmrcFilePath)) }) } } +// readPackageManagerConfigs returns the contents of every npm-family configuration file +// written under root. npm writes the file NPM_CONFIG_USERCONFIG names, while pnpm writes +// auth.ini inside its own config directory, whose path differs per platform, so the files +// are found by walking rather than assumed. Only known configuration file names are read, +// to keep caches and log files out of the assertions. +func readPackageManagerConfigs(t *testing.T, root string) string { + configFileNames := []string{".npmrc", "auth.ini", "rc", "config.yaml"} + // The walk only collects paths; the files are read afterwards, so no filesystem + // operation runs inside the callback. + var configPaths []string + require.NoError(t, filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { + if err != nil || entry.IsDir() || !slices.Contains(configFileNames, entry.Name()) { + return err + } + configPaths = append(configPaths, path) + return nil + })) + require.NotEmptyf(t, configPaths, "no package manager configuration was written under %s", root) + + var contents []string + for _, configPath := range configPaths { + content, err := os.ReadFile(configPath) + require.NoError(t, err) + contents = append(contents, string(content)) + } + return strings.Join(contents, "\n") +} + func TestSetupCommand_Yarn(t *testing.T) { // Retrieve the home directory and construct the .yarnrc file path. homeDir, err := os.UserHomeDir() From a3fe583b4721842a45945628037d83d1e81213b5 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 16:00:28 +0300 Subject: [PATCH 11/15] Publish the jf setup package-manager capability table Four downstream consumers each hand-maintain their own copy of what `jf setup` supports, and none of them can be checked against this package: - Fly Desktop (TypeScript): SUPPORTED_PACKAGE_MANAGERS - binary names, aliases, version commands, minimum versions. - Fly client (Go): handlers/register.go - names, aliases, binaries, repo types, shared-config groups. - JFrog agent hooks: TYPE_TO_PMS + PM_BINARIES + PMS_SETUP_WITHOUT_CLIENT - package type to package manager family, binary names, and which setups need no client. - JFrog skills: the manifest table in SKILL.md. Each drifts silently the day a package manager is added here. Recent examples: the agent hooks gated maven and gradle on a PATH lookup even though their setup writes the configuration file directly, and their pip probe tries pip3 before pip while getExecutable tries pip first. GetCapabilities exposes the table those copies were approximating, built from packageManagerToRepositoryPackageType and packageManagerConfigs so there is no second table to keep in step. MarshalCapabilities renders it as JSON for callers that read it as command output rather than as a Go value. Beyond what was already here, entries now carry: - clientBinaries, defaulting to the package manager's own name (maven is mvn; pip is pip then pip3, matching getExecutable). - versionCmd, because nuget and dotnet reject --version and print it via `help`, go uses `version`, and helm uses `version --short`. - minVersion, set only where a release introduced the mechanism setup depends on: helm 3.8.0, where OCI support reached GA and without which `helm registry login` cannot work. - configOnly for maven and gradle, whose configure functions write the file directly, so callers stop gating them on a client that wrapper-only projects (./mvnw, ./gradlew) never install. - publishOnly for twine, which is upload configuration rather than install-time routing. - configGroup for the package managers that write the SAME file - pip with pipenv, nuget with dotnet - so callers know those setups race and must be serialized. npm, pnpm and Yarn each write their own file and are deliberately not grouped, as are Docker and Podman. The tests pin the special cases the downstream copies got wrong, and TestGetCapabilities_EveryEntryIsFullyPopulated is now what actually keeps packageManagerConfigs in step with packageManagerToRepositoryPackageType - the previous comment claimed a test did that, but the only one covered the override-env subset. One divergence is pinned deliberately rather than resolved: gradle maps to the "gradle" repository package type here, while the Fly client registers it under maven and the agent hooks fold it into the maven type. `jf setup gradle` will not offer a maven-type repository when it prompts, so those copies contradict this mapping. uv's minVersion is left unset: configureUV needs `uv auth login` and the release that introduced it is unconfirmed. Fly Desktop asserts 0.8.15. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/capabilities.go | 145 ++++++++++++ .../commands/setup/capabilities_test.go | 207 ++++++++++++++++++ artifactory/commands/setup/setup.go | 74 +++++-- 3 files changed, 413 insertions(+), 13 deletions(-) create mode 100644 artifactory/commands/setup/capabilities.go create mode 100644 artifactory/commands/setup/capabilities_test.go diff --git a/artifactory/commands/setup/capabilities.go b/artifactory/commands/setup/capabilities.go new file mode 100644 index 00000000..c4a65b8f --- /dev/null +++ b/artifactory/commands/setup/capabilities.go @@ -0,0 +1,145 @@ +package setup + +import ( + "encoding/json" + + "github.com/jfrog/jfrog-cli-core/v2/common/project" + "github.com/jfrog/jfrog-client-go/utils/errorutils" + "golang.org/x/exp/maps" + "golang.org/x/exp/slices" +) + +// Roles a package manager can be configured for. +const ( + // RoleInstall means setup redirects dependency resolution through Artifactory. + RoleInstall = "install" + // RolePublish means setup configures uploads, not resolution. + RolePublish = "publish" +) + +// Configuration-file groups. Every package manager in one group writes the SAME +// file, so a caller setting up more than one of them must serialize those runs. +const ( + ConfigGroupPipConf = "pip-conf" + ConfigGroupNugetConfig = "nuget-config" +) + +// defaultVersionArg is what most clients accept to print their version. +var defaultVersionArg = []string{"--version"} + +// Capability is the machine-readable description of one package manager that +// `jf setup` supports. +// +// It exists because every downstream consumer had grown its own copy of this +// table — Fly Desktop (TypeScript), the Fly client (Go) and the JFrog agent +// hooks each hand-maintain a package-manager list, binary names, shared-config +// groupings and which package managers need a client on PATH. None of those +// copies can be checked against this package, so each one drifts silently the +// day a package manager is added here. Publishing the table makes those copies +// deletable. +type Capability struct { + // PackageManager is the token accepted by `jf setup `. + PackageManager string `json:"packageManager"` + // RepoPackageType is the Artifactory repository package type it resolves + // against, e.g. "pypi" for pip, pipenv, poetry, twine and uv. + RepoPackageType string `json:"repoPackageType"` + // Aliases are other accepted names for the same package manager. + Aliases []string `json:"aliases,omitempty"` + // ClientBinaries are the executables to look for on PATH, in preference order. + ClientBinaries []string `json:"clientBinaries"` + // VersionCmd is the argument list that makes the client print its version. + // Not every client accepts --version. + VersionCmd []string `json:"versionCmd"` + // MinVersion, when set, is the oldest client release setup can configure. + MinVersion string `json:"minVersion,omitempty"` + // RequiresClient reports whether setup runs the client. When false the + // configuration file is written directly and setup works with no client + // installed — which is what wrapper-only projects (./mvnw, ./gradlew) rely on. + RequiresClient bool `json:"requiresClient"` + // Role is RoleInstall or RolePublish. + Role string `json:"role"` + // CredentialsOnly reports that setup stores credentials without redirecting + // resolution, so an unqualified `docker pull alpine` still goes to Docker Hub. + CredentialsOnly bool `json:"credentialsOnly"` + // ConfigGroup, when set, names the configuration file shared with other + // package managers. Setups within one group must not run concurrently. + ConfigGroup string `json:"configGroup,omitempty"` + // ConfigLocation describes the configuration in the user's terms. It is not an + // absolute path: the real path is platform dependent and the package manager + // resolves it itself. + ConfigLocation string `json:"configLocation"` + // OverrideEnv, when set, is the environment variable that moves the + // configuration off its user-level default. + OverrideEnv string `json:"overrideEnv,omitempty"` +} + +// GetCapabilities returns one Capability per supported package manager, in the +// same order as GetSupportedPackageManagersList. +func GetCapabilities() []Capability { + packageManagers := maps.Keys(packageManagerToRepositoryPackageType) + slices.SortFunc(packageManagers, func(a, b project.ProjectType) int { + return int(a) - int(b) + }) + + capabilities := make([]Capability, 0, len(packageManagers)) + for _, packageManager := range packageManagers { + capabilities = append(capabilities, buildCapability(packageManager)) + } + return capabilities +} + +// GetCapability returns the Capability for one package manager, or an error when +// `jf setup` does not support it. +func GetCapability(packageManager project.ProjectType) (Capability, error) { + if !IsSupportedPackageManager(packageManager) { + return Capability{}, errorutils.CheckErrorf("unsupported package manager: %s", packageManager) + } + return buildCapability(packageManager), nil +} + +// MarshalCapabilities renders the whole table as indented JSON, for callers that +// read it as the output of a command rather than as a Go value. +func MarshalCapabilities() (string, error) { + encoded, err := json.MarshalIndent(GetCapabilities(), "", " ") + if err != nil { + return "", errorutils.CheckError(err) + } + return string(encoded), nil +} + +func buildCapability(packageManager project.ProjectType) Capability { + name := packageManager.String() + // A package manager present in packageManagerToRepositoryPackageType but absent + // from packageManagerConfigs yields the zero config, which would publish empty + // binaries and an empty location. TestGetCapabilities_EveryEntryIsFullyPopulated + // keeps the two maps in step, so that is a test failure rather than bad output. + config := packageManagerConfigs[packageManager] + + clientBinaries := config.clientBinaries + if len(clientBinaries) == 0 { + clientBinaries = []string{name} + } + versionCmd := config.versionCmd + if len(versionCmd) == 0 { + versionCmd = defaultVersionArg + } + role := RoleInstall + if config.publishOnly { + role = RolePublish + } + + return Capability{ + PackageManager: name, + RepoPackageType: packageManagerToRepositoryPackageType[packageManager], + Aliases: slices.Clone(config.aliases), + ClientBinaries: slices.Clone(clientBinaries), + VersionCmd: slices.Clone(versionCmd), + MinVersion: config.minVersion, + RequiresClient: !config.configOnly, + Role: role, + CredentialsOnly: config.credentialsOnly, + ConfigGroup: config.configGroup, + ConfigLocation: config.location, + OverrideEnv: config.overrideEnv, + } +} diff --git a/artifactory/commands/setup/capabilities_test.go b/artifactory/commands/setup/capabilities_test.go new file mode 100644 index 00000000..09490534 --- /dev/null +++ b/artifactory/commands/setup/capabilities_test.go @@ -0,0 +1,207 @@ +package setup + +import ( + "encoding/json" + "testing" + + "github.com/jfrog/jfrog-cli-core/v2/common/project" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The point of publishing the table: adding a package manager to `jf setup` +// without describing it here would ship an incomplete contract to the consumers +// that replaced their own copies with it. +func TestGetCapabilities_CoversEverySupportedPackageManager(t *testing.T) { + capabilities := GetCapabilities() + supported := GetSupportedPackageManagersList() + + require.Len(t, capabilities, len(supported), + "every package manager `jf setup` supports needs exactly one Capability") + + byName := make(map[string]Capability, len(capabilities)) + for _, capability := range capabilities { + byName[capability.PackageManager] = capability + } + for _, packageManager := range supported { + require.Contains(t, byName, packageManager, + "`jf setup %s` is supported but has no Capability entry", packageManager) + } + + // Order has to match, so callers can zip the two lists. + for i, packageManager := range supported { + assert.Equal(t, packageManager, capabilities[i].PackageManager, + "GetCapabilities must use the same order as GetSupportedPackageManagersList") + } +} + +// Every field a consumer is told it can rely on must actually be populated — +// an entry added with only a location would otherwise pass the coverage test +// above while publishing empty binaries and version arguments. +func TestGetCapabilities_EveryEntryIsFullyPopulated(t *testing.T) { + for _, capability := range GetCapabilities() { + t.Run(capability.PackageManager, func(t *testing.T) { + assert.NotEmpty(t, capability.RepoPackageType, "repoPackageType must be set") + assert.NotEmpty(t, capability.ConfigLocation, "configLocation must be set") + assert.NotEmpty(t, capability.ClientBinaries, "clientBinaries must never be empty") + assert.NotEmpty(t, capability.VersionCmd, "versionCmd must never be empty") + assert.Contains(t, []string{RoleInstall, RolePublish}, capability.Role) + for _, binary := range capability.ClientBinaries { + assert.NotEmpty(t, binary, "clientBinaries must not contain empty strings") + } + }) + } +} + +// Defaults exist so entries only spell out what differs. Assert the defaults are +// applied rather than silently left empty. +func TestGetCapabilities_Defaults(t *testing.T) { + npm, err := GetCapability(project.Npm) + require.NoError(t, err) + assert.Equal(t, []string{"npm"}, npm.ClientBinaries, "clientBinaries defaults to the package manager's own name") + assert.Equal(t, []string{"--version"}, npm.VersionCmd, "versionCmd defaults to --version") + assert.Empty(t, npm.Aliases) + assert.Equal(t, RoleInstall, npm.Role) + assert.True(t, npm.RequiresClient) +} + +// The three facts downstream copies got wrong, pinned here. +func TestGetCapabilities_KnownSpecialCases(t *testing.T) { + t.Run("maven and gradle need no client on PATH", func(t *testing.T) { + // configureMaven goes through NewSettingsXmlManager and configureGradle through + // WriteInitScript, so a wrapper-only project must not be gated on a PATH lookup. + for _, packageManager := range []project.ProjectType{project.Maven, project.Gradle} { + capability, err := GetCapability(packageManager) + require.NoError(t, err) + assert.False(t, capability.RequiresClient, + "%s setup writes its configuration directly", capability.PackageManager) + } + }) + + t.Run("maven is reachable as mvn", func(t *testing.T) { + maven, err := GetCapability(project.Maven) + require.NoError(t, err) + assert.Equal(t, []string{"mvn"}, maven.Aliases) + assert.Equal(t, []string{"mvn"}, maven.ClientBinaries) + }) + + t.Run("pip prefers pip over pip3", func(t *testing.T) { + // Matches getExecutable, which tries pip first and falls back to pip3. + pip, err := GetCapability(project.Pip) + require.NoError(t, err) + assert.Equal(t, []string{"pip", "pip3"}, pip.ClientBinaries) + }) + + t.Run("twine is publish-only", func(t *testing.T) { + twine, err := GetCapability(project.Twine) + require.NoError(t, err) + assert.Equal(t, RolePublish, twine.Role) + assert.Equal(t, "pypi", twine.RepoPackageType, "publish-only does not mean a different repo type") + }) + + t.Run("clients that reject --version", func(t *testing.T) { + for packageManager, expected := range map[project.ProjectType][]string{ + project.Nuget: {"help"}, + project.Dotnet: {"help"}, + project.Go: {"version"}, + project.Helm: {"version", "--short"}, + } { + capability, err := GetCapability(packageManager) + require.NoError(t, err) + assert.Equal(t, expected, capability.VersionCmd, capability.PackageManager) + } + }) + + t.Run("helm requires OCI support", func(t *testing.T) { + // configureHelm runs `helm registry login`; OCI reached GA in Helm 3.8.0. + helm, err := GetCapability(project.Helm) + require.NoError(t, err) + assert.Equal(t, "3.8.0", helm.MinVersion) + }) +} + +// A shared config file is a concurrency constraint, so the grouping has to be +// exactly the set of package managers that write the same file — no more. +func TestGetCapabilities_ConfigGroupsMatchSharedFiles(t *testing.T) { + grouped := make(map[string][]string) + for _, capability := range GetCapabilities() { + if capability.ConfigGroup != "" { + grouped[capability.ConfigGroup] = append(grouped[capability.ConfigGroup], capability.PackageManager) + } + } + + assert.Equal(t, map[string][]string{ + ConfigGroupPipConf: {"pip", "pipenv"}, + ConfigGroupNugetConfig: {"nuget", "dotnet"}, + }, grouped) + + // npm, pnpm and Yarn each write their own file, so grouping them would + // needlessly serialize three independent setups. + for _, packageManager := range []project.ProjectType{project.Npm, project.Pnpm, project.Yarn} { + capability, err := GetCapability(packageManager) + require.NoError(t, err) + assert.Empty(t, capability.ConfigGroup, "%s writes its own file", capability.PackageManager) + } + // Docker and Podman keep separate credential stores. + for _, packageManager := range []project.ProjectType{project.Docker, project.Podman} { + capability, err := GetCapability(packageManager) + require.NoError(t, err) + assert.Empty(t, capability.ConfigGroup, "%s has its own credential store", capability.PackageManager) + } + + // Every group must have more than one member, or it is not a constraint. + for group, packageManagers := range grouped { + assert.Greater(t, len(packageManagers), 1, "config group %q has a single member", group) + } +} + +func TestGetCapability_UnsupportedPackageManager(t *testing.T) { + _, err := GetCapability(project.Cocoapods) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported package manager") +} + +func TestMarshalCapabilities(t *testing.T) { + encoded, err := MarshalCapabilities() + require.NoError(t, err) + + var decoded []Capability + require.NoError(t, json.Unmarshal([]byte(encoded), &decoded)) + assert.Equal(t, GetCapabilities(), decoded, "the JSON form must round-trip") + + // Optional fields must stay absent rather than emitting empty values that a + // consumer would have to distinguish from a real one. + var raw []map[string]any + require.NoError(t, json.Unmarshal([]byte(encoded), &raw)) + for _, entry := range raw { + if entry["packageManager"] == "npm" { + assert.NotContains(t, entry, "aliases") + assert.NotContains(t, entry, "minVersion") + assert.NotContains(t, entry, "configGroup") + assert.Contains(t, entry, "requiresClient", "non-optional fields must always be present") + } + } +} + +// Callers group by repo type to decide which package managers a governed +// repository covers, so the mapping has to survive being inverted. +func TestGetCapabilities_GroupByRepoPackageType(t *testing.T) { + byRepoType := make(map[string][]string) + for _, capability := range GetCapabilities() { + byRepoType[capability.RepoPackageType] = append(byRepoType[capability.RepoPackageType], capability.PackageManager) + } + + assert.ElementsMatch(t, []string{"pip", "pipenv", "poetry", "twine", "uv"}, byRepoType["pypi"]) + assert.ElementsMatch(t, []string{"npm", "pnpm", "yarn"}, byRepoType["npm"]) + assert.ElementsMatch(t, []string{"docker", "podman"}, byRepoType["docker"]) + assert.ElementsMatch(t, []string{"nuget", "dotnet"}, byRepoType["nuget"]) + + // Gradle is its own Artifactory package type here, NOT part of "maven". + // Pinned deliberately: downstream copies of this table disagree — the Fly client + // registers gradle under its maven repo name, and the agent hooks map the maven + // package type to [maven, gradle]. Anything resolving a gradle repository from + // the "maven" type is contradicting this mapping, and `jf setup gradle` will not + // offer a maven-type repository when it prompts. + assert.Equal(t, []string{"maven"}, byRepoType["maven"]) + assert.Equal(t, []string{"gradle"}, byRepoType["gradle"]) +} diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index 95bf564b..9b8db686 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -55,12 +55,41 @@ type packageManagerConfig struct { // function or the tool it drives really honors the variable — the per-entry // comments record what was verified. overrideEnv string + // aliases are the other names a caller may use for this package manager, so + // downstream tools do not each keep their own alias table. + aliases []string + // clientBinaries are the executables to look for on PATH, in preference order. + // Defaults to the package manager's own name, so only set it where they differ. + clientBinaries []string + // versionCmd is the argument list that makes the client print its version. + // Defaults to --version; several clients do not accept that flag at all. + versionCmd []string + // minVersion is the oldest client release `jf setup` can actually configure. + // Only set where a specific release introduced the mechanism setup depends on, + // and record which one in the entry's comment — this is published to callers + // that will refuse to run below it. + minVersion string + // configOnly marks the package managers whose configure function writes the + // configuration file itself and never runs the client. Their setup succeeds on a + // machine that has no client installed, which matters for wrapper-only projects + // (`./mvnw`, `./gradlew`), so callers must not gate them on a PATH lookup. + configOnly bool + // publishOnly marks the package managers configured for uploading rather than + // resolving. They are not part of install-time routing. + publishOnly bool + // configGroup names the configuration file this package manager shares with + // others. Everything in one group writes the SAME file, so concurrent `jf setup` + // runs across a group race each other and callers have to serialize them. + configGroup string } -// One entry per package manager in packageManagerToRepositoryPackageType; a test -// asserts the two stay in step. +// One entry per package manager in packageManagerToRepositoryPackageType. +// TestGetCapabilities_EveryEntryIsFullyPopulated keeps the two maps in step: a +// package manager added there without an entry here publishes an empty location +// and empty client binaries through GetCapabilities, and fails that test. var packageManagerConfigs = map[project.ProjectType]packageManagerConfig{ // npm resolves the file `npm config set` writes through NPM_CONFIG_USERCONFIG. + // npm, pnpm and Yarn are NOT one config group: each writes its own file. project.Npm: {location: "your user-level npm configuration (.npmrc)", overrideEnv: "NPM_CONFIG_USERCONFIG"}, // pnpm is deliberately left without an override: `pnpm config set` writes to // pnpm's own config directory (auth.ini) and ignores NPM_CONFIG_USERCONFIG, which @@ -69,26 +98,45 @@ var packageManagerConfigs = map[project.ProjectType]packageManagerConfig{ // Yarn Classic writes ~/.yarnrc, and YARN_RC_FILENAME does not redirect it. project.Yarn: {location: "your user-level Yarn configuration (.yarnrc)"}, // configurePip writes the file itself when PIP_CONFIG_FILE is set, because - // `pip config set` does not support that variable. - project.Pip: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE"}, - project.Pipenv: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE"}, + // `pip config set` does not support that variable. requiresClient stays true: + // the default path does shell out, and only the override path does not. + // pip and pipenv share pip.conf, so their setups must not run concurrently. + project.Pip: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE", + clientBinaries: []string{"pip", "pip3"}, configGroup: ConfigGroupPipConf}, + project.Pipenv: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE", + configGroup: ConfigGroupPipConf}, // `poetry config` writes config.toml into POETRY_CONFIG_DIR when it is set. project.Poetry: {location: "your user-level Poetry configuration (config.toml)", overrideEnv: "POETRY_CONFIG_DIR"}, // Twine's .pypirc path is chosen per invocation (--config-file), not by the environment. - project.Twine: {location: "your user-level Twine configuration (.pypirc)"}, + project.Twine: {location: "your user-level Twine configuration (.pypirc)", publishOnly: true}, // ConfigureUVIndex writes to UV_CONFIG_FILE when it is set. - project.UV: {location: "your user-level uv configuration (uv.toml)", overrideEnv: "UV_CONFIG_FILE"}, - project.Nuget: {location: "your user-level NuGet configuration (NuGet.Config)"}, - project.Dotnet: {location: "your user-level NuGet configuration (NuGet.Config)"}, + // minVersion is deliberately unset: configureUV needs `uv auth login`, and the + // release that introduced it has not been confirmed. Fly Desktop asserts 0.8.15. + project.UV: {location: "your user-level uv configuration (uv.toml)", overrideEnv: "UV_CONFIG_FILE"}, + // nuget and dotnet both write NuGet.Config, so their setups must not run concurrently. + // Neither client accepts --version; both print it through `help`. + project.Nuget: {location: "your user-level NuGet configuration (NuGet.Config)", + versionCmd: []string{"help"}, configGroup: ConfigGroupNugetConfig}, + project.Dotnet: {location: "your user-level NuGet configuration (NuGet.Config)", + versionCmd: []string{"help"}, configGroup: ConfigGroupNugetConfig}, // `go env -w` writes to the file GOENV points at, defaulting to the per-user Go env file. - project.Go: {location: "your user-level Go environment (GOPROXY in your Go env file)", overrideEnv: "GOENV"}, + project.Go: {location: "your user-level Go environment (GOPROXY in your Go env file)", overrideEnv: "GOENV", + versionCmd: []string{"version"}}, // gradle.WriteInitScript drops the script under GRADLE_USER_HOME when it is set. - project.Gradle: {location: "your user-level Gradle configuration (an init script in your Gradle user home)", overrideEnv: gradle.UserHomeEnv}, + // configOnly: WriteInitScript writes the file directly, so `gradle` need not exist. + project.Gradle: {location: "your user-level Gradle configuration (an init script in your Gradle user home)", + overrideEnv: gradle.UserHomeEnv, configOnly: true}, // Maven picks the settings file per invocation (-s), not from the environment. - project.Maven: {location: "your user-level Maven settings (settings.xml)"}, + // configOnly: configureMaven goes through NewSettingsXmlManager, so `mvn` need not exist. + project.Maven: {location: "your user-level Maven settings (settings.xml)", configOnly: true, + aliases: []string{"mvn"}, clientBinaries: []string{"mvn"}}, + // Docker and Podman keep separate credential stores, so they are not one group. project.Docker: {location: "your Docker credential store", credentialsOnly: true}, project.Podman: {location: "your Podman credential store", credentialsOnly: true}, - project.Helm: {location: "your Helm registry credential store", credentialsOnly: true}, + // configureHelm runs `helm registry login` against an OCI registry. OCI support + // reached general availability in Helm 3.8.0, so older clients cannot be configured. + project.Helm: {location: "your Helm registry credential store", credentialsOnly: true, + versionCmd: []string{"version", "--short"}, minVersion: "3.8.0"}, } // configScopeNote describes what the command changed and how widely it applies, or From b9634e05a8d12c6e0334d682e440f777f9299cc4 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 16:04:44 +0300 Subject: [PATCH 12/15] Record that Artifactory has real Gradle repositories Resolved: the gradle repository package type is correct here, and the downstream copies that fold gradle into maven are the ones to change. The Fly client used its maven repo name because it had no Gradle repositories to point at, not because the type model says so. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/capabilities_test.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/artifactory/commands/setup/capabilities_test.go b/artifactory/commands/setup/capabilities_test.go index 09490534..24701b88 100644 --- a/artifactory/commands/setup/capabilities_test.go +++ b/artifactory/commands/setup/capabilities_test.go @@ -196,12 +196,13 @@ func TestGetCapabilities_GroupByRepoPackageType(t *testing.T) { assert.ElementsMatch(t, []string{"docker", "podman"}, byRepoType["docker"]) assert.ElementsMatch(t, []string{"nuget", "dotnet"}, byRepoType["nuget"]) - // Gradle is its own Artifactory package type here, NOT part of "maven". - // Pinned deliberately: downstream copies of this table disagree — the Fly client - // registers gradle under its maven repo name, and the agent hooks map the maven - // package type to [maven, gradle]. Anything resolving a gradle repository from - // the "maven" type is contradicting this mapping, and `jf setup gradle` will not - // offer a maven-type repository when it prompts. + // Gradle is its own Artifactory package type, NOT part of "maven" — Artifactory + // has real Gradle repositories. Pinned because downstream copies of this table + // fold gradle into maven: the Fly client registers it under its maven repo name + // (it had no Gradle repositories to point at), and the agent hooks map the maven + // package type to [maven, gradle]. Since this map drives + // promptUserToSelectRepository, `jf setup gradle` will not offer a maven-type + // repository, so those copies have to change rather than this mapping. assert.Equal(t, []string{"maven"}, byRepoType["maven"]) assert.Equal(t, []string{"gradle"}, byRepoType["gradle"]) } From e4c7387f8b88a2b28524efa59ef2c7507c3a3e01 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 16:10:35 +0300 Subject: [PATCH 13/15] Mask credentials in every GOPROXY entry, not just the first configureGo echoes a pre-existing GOPROXY back to the user so they can unset it. GOPROXY is a separator-delimited list, but the masking stopped at the first '@', so with more than one proxy configured every later entry's token was printed verbatim: before: ****@host1/api/go/r1,https://u:TOKEN_TWO@host2/api/go/r2 after: https://****@host1/api/go/r1,https://****@host2/api/go/r2 Two other cases it got wrong: pipe-separated lists, and a password containing '@' (the first-'@' index cut inside the password and printed the rest of it). Now masks per entry across both separators, using the LAST '@' within an entry to delimit the credentials, and keeps the scheme so the warning still identifies which proxy is set. Bare keywords like `direct` and `off` are untouched. Spotted by comparing against fly-client's stringutils.MaskCredsFromURL, which already splits the list before masking. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/setup.go | 46 ++++++++++++++-- artifactory/commands/setup/setup_test.go | 68 ++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index 9b8db686..83ede8e6 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -479,6 +479,46 @@ func (sc *SetupCommand) configureYarn() (err error) { return nil } +// goProxySeparators are the two characters that delimit GOPROXY entries: a comma +// falls through only on 404/410, a pipe on any error. +const goProxySeparators = ",|" + +// maskGoProxyCredentials replaces the credentials of every entry in a GOPROXY +// value, keeping the scheme and host so the message still says which proxy is set. +// +// GOPROXY is a separator-delimited list, so masking only up to the first '@' +// would print every later entry's token verbatim. Within one entry the LAST '@' +// delimits the credentials, because a password may itself contain '@'. +func maskGoProxyCredentials(goProxy string) string { + var masked strings.Builder + entryStart := 0 + for i, char := range goProxy { + if !strings.ContainsRune(goProxySeparators, char) { + continue + } + masked.WriteString(maskGoProxyEntry(goProxy[entryStart:i])) + masked.WriteRune(char) + entryStart = i + len(string(char)) + } + masked.WriteString(maskGoProxyEntry(goProxy[entryStart:])) + return masked.String() +} + +// maskGoProxyEntry masks the credentials of a single GOPROXY entry. Entries +// without credentials — including the bare `direct` and `off` keywords — are +// returned unchanged. +func maskGoProxyEntry(entry string) string { + credentialsEnd := strings.LastIndex(entry, "@") + if credentialsEnd == -1 { + return entry + } + scheme := "" + if schemeEnd := strings.Index(entry, "://"); schemeEnd != -1 && schemeEnd < credentialsEnd { + scheme = entry[:schemeEnd+len("://")] + } + return scheme + "****" + entry[credentialsEnd:] +} + // configureGo configures Go to use the Artifactory repository for GOPROXY. // Runs the following command: // @@ -496,13 +536,9 @@ func (sc *SetupCommand) configureGo() error { if err := os.Unsetenv("GOPROXY"); err != nil { return errorutils.CheckErrorf("failed to unset GOPROXY environment variable: %w", err) } - // Mask credentials in the GOPROXY value - if i := strings.Index(goProxyVal, "@"); i != -1 { - goProxyVal = "****" + goProxyVal[i:] - } // Log a warning about the existing GOPROXY environment variable so the user can unset it permanently log.Warn(fmt.Sprintf("A local GOPROXY='%s' is set and will override the global setting.\n"+ - "Unset it in your shell config (e.g., .zshrc, .bashrc).", goProxyVal)) + "Unset it in your shell config (e.g., .zshrc, .bashrc).", maskGoProxyCredentials(goProxyVal))) } repoWithCredsUrl, err := golang.GetArtifactoryRemoteRepoUrl(sc.serverDetails, sc.repoName, golang.GoProxyUrlParams{Direct: true, FallbackOnlyIfNotFound: true}) diff --git a/artifactory/commands/setup/setup_test.go b/artifactory/commands/setup/setup_test.go index f09b55d8..f26e809d 100644 --- a/artifactory/commands/setup/setup_test.go +++ b/artifactory/commands/setup/setup_test.go @@ -1114,3 +1114,71 @@ func TestSetupCommand_PrintsConfigScopeNote(t *testing.T) { assert.Contains(t, output.String(), configScopeNote(project.Maven), "the command must print the scope note, not just be able to build it") } + +// A pre-existing GOPROXY is echoed back to the user, and GOPROXY is a +// separator-delimited list, so masking has to cover every entry rather than +// stopping at the first set of credentials. +func TestMaskGoProxyCredentials(t *testing.T) { + const tokenOne = "TOKEN_ONE" + const tokenTwo = "TOKEN_TWO" + testCases := []struct { + name string + goProxy string + expected string + }{ + { + name: "Single entry with direct fallback", + goProxy: "https://u:" + tokenOne + "@art.example.com/artifactory/api/go/repo,direct", + expected: "https://****@art.example.com/artifactory/api/go/repo,direct", + }, + { + name: "Comma-separated entries both masked", + goProxy: "https://u:" + tokenOne + "@host1/api/go/r1,https://u:" + tokenTwo + "@host2/api/go/r2", + expected: "https://****@host1/api/go/r1,https://****@host2/api/go/r2", + }, + { + name: "Pipe-separated entries both masked", + goProxy: "https://u:" + tokenOne + "@host1/api/go/r1|https://u:" + tokenTwo + "@host2/api/go/r2", + expected: "https://****@host1/api/go/r1|https://****@host2/api/go/r2", + }, + { + name: "Mixed separators", + goProxy: "https://u:" + tokenOne + "@host1/r1,https://u:" + tokenTwo + "@host2/r2|direct", + expected: "https://****@host1/r1,https://****@host2/r2|direct", + }, + { + name: "Password containing an at sign masks the whole password", + goProxy: "https://user:p@ssw0rd@art.example.com/artifactory/api/go/repo", + expected: "https://****@art.example.com/artifactory/api/go/repo", + }, + { + name: "No credentials is left untouched", + goProxy: "https://proxy.golang.org,direct", + expected: "https://proxy.golang.org,direct", + }, + { + name: "Keywords are left untouched", + goProxy: "off", + expected: "off", + }, + { + name: "Empty value", + goProxy: "", + expected: "", + }, + { + name: "Entry without a scheme still loses its credentials", + goProxy: "u:" + tokenOne + "@host/api/go/repo", + expected: "****@host/api/go/repo", + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + masked := maskGoProxyCredentials(testCase.goProxy) + assert.Equal(t, testCase.expected, masked) + assert.NotContains(t, masked, tokenOne, "no entry's credentials may survive masking") + assert.NotContains(t, masked, tokenTwo, "no entry's credentials may survive masking") + assert.NotContains(t, masked, "ssw0rd", "a password containing '@' must not leak") + }) + } +} From 2ca188bae6ce2df77520b279a20642568374af15 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 16:34:20 +0300 Subject: [PATCH 14/15] Record that pnpm keeps its own repository after a later npm setup pnpm reads its own configuration ahead of ~/.npmrc, so the two setups drift apart silently: with no pnpm setup pnpm follows npm's file through the fallback, but once `jf setup pnpm` has written auth.ini a later `jf setup npm` moves npm alone. Verified against pnpm 11 by writing both files and reading the value back - pnpm reported its own repository, npm reported the other. Recorded next to the entry that already explains why pnpm has no config override, because the two facts have the same cause. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/setup.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index 83ede8e6..2981175e 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -94,6 +94,12 @@ var packageManagerConfigs = map[project.ProjectType]packageManagerConfig{ // pnpm is deliberately left without an override: `pnpm config set` writes to // pnpm's own config directory (auth.ini) and ignores NPM_CONFIG_USERCONFIG, which // it consults only as a fallback when reading credentials. + // That file also outranks ~/.npmrc for pnpm, which matters across two setups: a + // machine with no pnpm setup inherits npm's ~/.npmrc through the fallback, but + // once `jf setup pnpm` has written auth.ini, a later `jf setup npm` moves npm + // alone and pnpm keeps resolving from the repository it was given. Both tools are + // individually correct, and nothing in either command's output says they now + // disagree. Verified against pnpm 11. project.Pnpm: {location: "your user-level pnpm configuration (auth.ini in pnpm's config directory)"}, // Yarn Classic writes ~/.yarnrc, and YARN_RC_FILENAME does not redirect it. project.Yarn: {location: "your user-level Yarn configuration (.yarnrc)"}, From f787a4292b2e628470aa0a13cbc1b385a17b5589 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Wed, 29 Jul 2026 16:43:26 +0300 Subject: [PATCH 15/15] Drop the capability table from this PR Removes GetCapabilities/GetCapability/MarshalCapabilities and the extra packageManagerConfig fields they needed. The premise was wrong: I took this for an addition to an existing `--list` surface, and there is no such flag - it was a new public API, which is too much to decide inside a PR about the GOPROXY fallback and the config-scope note. It needs its own discussion, including whether a machine-readable capability output should be a flag at all. Kept from that work, because both stand on their own: - The GOPROXY credential masking fix (e4c7387). Independent of the table; it closed a real leak of every entry after the first. - A coverage test for packageManagerConfigs. The map comment claimed a test kept it in step with packageManagerToRepositoryPackageType, and none did - the only one covered the override-env subset, so a package manager added without an entry here would silently print no scope note. Ten lines to make the existing claim true, verified by removing the helm entry and watching it fail. The findings the table encoded are not lost: the shared-config-file clobbering, maven and gradle needing no client, and helm 3.8.0 are all documented in the setup help text in jfrog-cli#3627. Co-Authored-By: Claude Opus 5 --- artifactory/commands/setup/capabilities.go | 145 ------------ .../commands/setup/capabilities_test.go | 208 ------------------ artifactory/commands/setup/setup.go | 75 ++----- artifactory/commands/setup/setup_test.go | 13 ++ 4 files changed, 27 insertions(+), 414 deletions(-) delete mode 100644 artifactory/commands/setup/capabilities.go delete mode 100644 artifactory/commands/setup/capabilities_test.go diff --git a/artifactory/commands/setup/capabilities.go b/artifactory/commands/setup/capabilities.go deleted file mode 100644 index c4a65b8f..00000000 --- a/artifactory/commands/setup/capabilities.go +++ /dev/null @@ -1,145 +0,0 @@ -package setup - -import ( - "encoding/json" - - "github.com/jfrog/jfrog-cli-core/v2/common/project" - "github.com/jfrog/jfrog-client-go/utils/errorutils" - "golang.org/x/exp/maps" - "golang.org/x/exp/slices" -) - -// Roles a package manager can be configured for. -const ( - // RoleInstall means setup redirects dependency resolution through Artifactory. - RoleInstall = "install" - // RolePublish means setup configures uploads, not resolution. - RolePublish = "publish" -) - -// Configuration-file groups. Every package manager in one group writes the SAME -// file, so a caller setting up more than one of them must serialize those runs. -const ( - ConfigGroupPipConf = "pip-conf" - ConfigGroupNugetConfig = "nuget-config" -) - -// defaultVersionArg is what most clients accept to print their version. -var defaultVersionArg = []string{"--version"} - -// Capability is the machine-readable description of one package manager that -// `jf setup` supports. -// -// It exists because every downstream consumer had grown its own copy of this -// table — Fly Desktop (TypeScript), the Fly client (Go) and the JFrog agent -// hooks each hand-maintain a package-manager list, binary names, shared-config -// groupings and which package managers need a client on PATH. None of those -// copies can be checked against this package, so each one drifts silently the -// day a package manager is added here. Publishing the table makes those copies -// deletable. -type Capability struct { - // PackageManager is the token accepted by `jf setup `. - PackageManager string `json:"packageManager"` - // RepoPackageType is the Artifactory repository package type it resolves - // against, e.g. "pypi" for pip, pipenv, poetry, twine and uv. - RepoPackageType string `json:"repoPackageType"` - // Aliases are other accepted names for the same package manager. - Aliases []string `json:"aliases,omitempty"` - // ClientBinaries are the executables to look for on PATH, in preference order. - ClientBinaries []string `json:"clientBinaries"` - // VersionCmd is the argument list that makes the client print its version. - // Not every client accepts --version. - VersionCmd []string `json:"versionCmd"` - // MinVersion, when set, is the oldest client release setup can configure. - MinVersion string `json:"minVersion,omitempty"` - // RequiresClient reports whether setup runs the client. When false the - // configuration file is written directly and setup works with no client - // installed — which is what wrapper-only projects (./mvnw, ./gradlew) rely on. - RequiresClient bool `json:"requiresClient"` - // Role is RoleInstall or RolePublish. - Role string `json:"role"` - // CredentialsOnly reports that setup stores credentials without redirecting - // resolution, so an unqualified `docker pull alpine` still goes to Docker Hub. - CredentialsOnly bool `json:"credentialsOnly"` - // ConfigGroup, when set, names the configuration file shared with other - // package managers. Setups within one group must not run concurrently. - ConfigGroup string `json:"configGroup,omitempty"` - // ConfigLocation describes the configuration in the user's terms. It is not an - // absolute path: the real path is platform dependent and the package manager - // resolves it itself. - ConfigLocation string `json:"configLocation"` - // OverrideEnv, when set, is the environment variable that moves the - // configuration off its user-level default. - OverrideEnv string `json:"overrideEnv,omitempty"` -} - -// GetCapabilities returns one Capability per supported package manager, in the -// same order as GetSupportedPackageManagersList. -func GetCapabilities() []Capability { - packageManagers := maps.Keys(packageManagerToRepositoryPackageType) - slices.SortFunc(packageManagers, func(a, b project.ProjectType) int { - return int(a) - int(b) - }) - - capabilities := make([]Capability, 0, len(packageManagers)) - for _, packageManager := range packageManagers { - capabilities = append(capabilities, buildCapability(packageManager)) - } - return capabilities -} - -// GetCapability returns the Capability for one package manager, or an error when -// `jf setup` does not support it. -func GetCapability(packageManager project.ProjectType) (Capability, error) { - if !IsSupportedPackageManager(packageManager) { - return Capability{}, errorutils.CheckErrorf("unsupported package manager: %s", packageManager) - } - return buildCapability(packageManager), nil -} - -// MarshalCapabilities renders the whole table as indented JSON, for callers that -// read it as the output of a command rather than as a Go value. -func MarshalCapabilities() (string, error) { - encoded, err := json.MarshalIndent(GetCapabilities(), "", " ") - if err != nil { - return "", errorutils.CheckError(err) - } - return string(encoded), nil -} - -func buildCapability(packageManager project.ProjectType) Capability { - name := packageManager.String() - // A package manager present in packageManagerToRepositoryPackageType but absent - // from packageManagerConfigs yields the zero config, which would publish empty - // binaries and an empty location. TestGetCapabilities_EveryEntryIsFullyPopulated - // keeps the two maps in step, so that is a test failure rather than bad output. - config := packageManagerConfigs[packageManager] - - clientBinaries := config.clientBinaries - if len(clientBinaries) == 0 { - clientBinaries = []string{name} - } - versionCmd := config.versionCmd - if len(versionCmd) == 0 { - versionCmd = defaultVersionArg - } - role := RoleInstall - if config.publishOnly { - role = RolePublish - } - - return Capability{ - PackageManager: name, - RepoPackageType: packageManagerToRepositoryPackageType[packageManager], - Aliases: slices.Clone(config.aliases), - ClientBinaries: slices.Clone(clientBinaries), - VersionCmd: slices.Clone(versionCmd), - MinVersion: config.minVersion, - RequiresClient: !config.configOnly, - Role: role, - CredentialsOnly: config.credentialsOnly, - ConfigGroup: config.configGroup, - ConfigLocation: config.location, - OverrideEnv: config.overrideEnv, - } -} diff --git a/artifactory/commands/setup/capabilities_test.go b/artifactory/commands/setup/capabilities_test.go deleted file mode 100644 index 24701b88..00000000 --- a/artifactory/commands/setup/capabilities_test.go +++ /dev/null @@ -1,208 +0,0 @@ -package setup - -import ( - "encoding/json" - "testing" - - "github.com/jfrog/jfrog-cli-core/v2/common/project" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// The point of publishing the table: adding a package manager to `jf setup` -// without describing it here would ship an incomplete contract to the consumers -// that replaced their own copies with it. -func TestGetCapabilities_CoversEverySupportedPackageManager(t *testing.T) { - capabilities := GetCapabilities() - supported := GetSupportedPackageManagersList() - - require.Len(t, capabilities, len(supported), - "every package manager `jf setup` supports needs exactly one Capability") - - byName := make(map[string]Capability, len(capabilities)) - for _, capability := range capabilities { - byName[capability.PackageManager] = capability - } - for _, packageManager := range supported { - require.Contains(t, byName, packageManager, - "`jf setup %s` is supported but has no Capability entry", packageManager) - } - - // Order has to match, so callers can zip the two lists. - for i, packageManager := range supported { - assert.Equal(t, packageManager, capabilities[i].PackageManager, - "GetCapabilities must use the same order as GetSupportedPackageManagersList") - } -} - -// Every field a consumer is told it can rely on must actually be populated — -// an entry added with only a location would otherwise pass the coverage test -// above while publishing empty binaries and version arguments. -func TestGetCapabilities_EveryEntryIsFullyPopulated(t *testing.T) { - for _, capability := range GetCapabilities() { - t.Run(capability.PackageManager, func(t *testing.T) { - assert.NotEmpty(t, capability.RepoPackageType, "repoPackageType must be set") - assert.NotEmpty(t, capability.ConfigLocation, "configLocation must be set") - assert.NotEmpty(t, capability.ClientBinaries, "clientBinaries must never be empty") - assert.NotEmpty(t, capability.VersionCmd, "versionCmd must never be empty") - assert.Contains(t, []string{RoleInstall, RolePublish}, capability.Role) - for _, binary := range capability.ClientBinaries { - assert.NotEmpty(t, binary, "clientBinaries must not contain empty strings") - } - }) - } -} - -// Defaults exist so entries only spell out what differs. Assert the defaults are -// applied rather than silently left empty. -func TestGetCapabilities_Defaults(t *testing.T) { - npm, err := GetCapability(project.Npm) - require.NoError(t, err) - assert.Equal(t, []string{"npm"}, npm.ClientBinaries, "clientBinaries defaults to the package manager's own name") - assert.Equal(t, []string{"--version"}, npm.VersionCmd, "versionCmd defaults to --version") - assert.Empty(t, npm.Aliases) - assert.Equal(t, RoleInstall, npm.Role) - assert.True(t, npm.RequiresClient) -} - -// The three facts downstream copies got wrong, pinned here. -func TestGetCapabilities_KnownSpecialCases(t *testing.T) { - t.Run("maven and gradle need no client on PATH", func(t *testing.T) { - // configureMaven goes through NewSettingsXmlManager and configureGradle through - // WriteInitScript, so a wrapper-only project must not be gated on a PATH lookup. - for _, packageManager := range []project.ProjectType{project.Maven, project.Gradle} { - capability, err := GetCapability(packageManager) - require.NoError(t, err) - assert.False(t, capability.RequiresClient, - "%s setup writes its configuration directly", capability.PackageManager) - } - }) - - t.Run("maven is reachable as mvn", func(t *testing.T) { - maven, err := GetCapability(project.Maven) - require.NoError(t, err) - assert.Equal(t, []string{"mvn"}, maven.Aliases) - assert.Equal(t, []string{"mvn"}, maven.ClientBinaries) - }) - - t.Run("pip prefers pip over pip3", func(t *testing.T) { - // Matches getExecutable, which tries pip first and falls back to pip3. - pip, err := GetCapability(project.Pip) - require.NoError(t, err) - assert.Equal(t, []string{"pip", "pip3"}, pip.ClientBinaries) - }) - - t.Run("twine is publish-only", func(t *testing.T) { - twine, err := GetCapability(project.Twine) - require.NoError(t, err) - assert.Equal(t, RolePublish, twine.Role) - assert.Equal(t, "pypi", twine.RepoPackageType, "publish-only does not mean a different repo type") - }) - - t.Run("clients that reject --version", func(t *testing.T) { - for packageManager, expected := range map[project.ProjectType][]string{ - project.Nuget: {"help"}, - project.Dotnet: {"help"}, - project.Go: {"version"}, - project.Helm: {"version", "--short"}, - } { - capability, err := GetCapability(packageManager) - require.NoError(t, err) - assert.Equal(t, expected, capability.VersionCmd, capability.PackageManager) - } - }) - - t.Run("helm requires OCI support", func(t *testing.T) { - // configureHelm runs `helm registry login`; OCI reached GA in Helm 3.8.0. - helm, err := GetCapability(project.Helm) - require.NoError(t, err) - assert.Equal(t, "3.8.0", helm.MinVersion) - }) -} - -// A shared config file is a concurrency constraint, so the grouping has to be -// exactly the set of package managers that write the same file — no more. -func TestGetCapabilities_ConfigGroupsMatchSharedFiles(t *testing.T) { - grouped := make(map[string][]string) - for _, capability := range GetCapabilities() { - if capability.ConfigGroup != "" { - grouped[capability.ConfigGroup] = append(grouped[capability.ConfigGroup], capability.PackageManager) - } - } - - assert.Equal(t, map[string][]string{ - ConfigGroupPipConf: {"pip", "pipenv"}, - ConfigGroupNugetConfig: {"nuget", "dotnet"}, - }, grouped) - - // npm, pnpm and Yarn each write their own file, so grouping them would - // needlessly serialize three independent setups. - for _, packageManager := range []project.ProjectType{project.Npm, project.Pnpm, project.Yarn} { - capability, err := GetCapability(packageManager) - require.NoError(t, err) - assert.Empty(t, capability.ConfigGroup, "%s writes its own file", capability.PackageManager) - } - // Docker and Podman keep separate credential stores. - for _, packageManager := range []project.ProjectType{project.Docker, project.Podman} { - capability, err := GetCapability(packageManager) - require.NoError(t, err) - assert.Empty(t, capability.ConfigGroup, "%s has its own credential store", capability.PackageManager) - } - - // Every group must have more than one member, or it is not a constraint. - for group, packageManagers := range grouped { - assert.Greater(t, len(packageManagers), 1, "config group %q has a single member", group) - } -} - -func TestGetCapability_UnsupportedPackageManager(t *testing.T) { - _, err := GetCapability(project.Cocoapods) - require.Error(t, err) - assert.Contains(t, err.Error(), "unsupported package manager") -} - -func TestMarshalCapabilities(t *testing.T) { - encoded, err := MarshalCapabilities() - require.NoError(t, err) - - var decoded []Capability - require.NoError(t, json.Unmarshal([]byte(encoded), &decoded)) - assert.Equal(t, GetCapabilities(), decoded, "the JSON form must round-trip") - - // Optional fields must stay absent rather than emitting empty values that a - // consumer would have to distinguish from a real one. - var raw []map[string]any - require.NoError(t, json.Unmarshal([]byte(encoded), &raw)) - for _, entry := range raw { - if entry["packageManager"] == "npm" { - assert.NotContains(t, entry, "aliases") - assert.NotContains(t, entry, "minVersion") - assert.NotContains(t, entry, "configGroup") - assert.Contains(t, entry, "requiresClient", "non-optional fields must always be present") - } - } -} - -// Callers group by repo type to decide which package managers a governed -// repository covers, so the mapping has to survive being inverted. -func TestGetCapabilities_GroupByRepoPackageType(t *testing.T) { - byRepoType := make(map[string][]string) - for _, capability := range GetCapabilities() { - byRepoType[capability.RepoPackageType] = append(byRepoType[capability.RepoPackageType], capability.PackageManager) - } - - assert.ElementsMatch(t, []string{"pip", "pipenv", "poetry", "twine", "uv"}, byRepoType["pypi"]) - assert.ElementsMatch(t, []string{"npm", "pnpm", "yarn"}, byRepoType["npm"]) - assert.ElementsMatch(t, []string{"docker", "podman"}, byRepoType["docker"]) - assert.ElementsMatch(t, []string{"nuget", "dotnet"}, byRepoType["nuget"]) - - // Gradle is its own Artifactory package type, NOT part of "maven" — Artifactory - // has real Gradle repositories. Pinned because downstream copies of this table - // fold gradle into maven: the Fly client registers it under its maven repo name - // (it had no Gradle repositories to point at), and the agent hooks map the maven - // package type to [maven, gradle]. Since this map drives - // promptUserToSelectRepository, `jf setup gradle` will not offer a maven-type - // repository, so those copies have to change rather than this mapping. - assert.Equal(t, []string{"maven"}, byRepoType["maven"]) - assert.Equal(t, []string{"gradle"}, byRepoType["gradle"]) -} diff --git a/artifactory/commands/setup/setup.go b/artifactory/commands/setup/setup.go index 2981175e..476bf246 100644 --- a/artifactory/commands/setup/setup.go +++ b/artifactory/commands/setup/setup.go @@ -55,41 +55,13 @@ type packageManagerConfig struct { // function or the tool it drives really honors the variable — the per-entry // comments record what was verified. overrideEnv string - // aliases are the other names a caller may use for this package manager, so - // downstream tools do not each keep their own alias table. - aliases []string - // clientBinaries are the executables to look for on PATH, in preference order. - // Defaults to the package manager's own name, so only set it where they differ. - clientBinaries []string - // versionCmd is the argument list that makes the client print its version. - // Defaults to --version; several clients do not accept that flag at all. - versionCmd []string - // minVersion is the oldest client release `jf setup` can actually configure. - // Only set where a specific release introduced the mechanism setup depends on, - // and record which one in the entry's comment — this is published to callers - // that will refuse to run below it. - minVersion string - // configOnly marks the package managers whose configure function writes the - // configuration file itself and never runs the client. Their setup succeeds on a - // machine that has no client installed, which matters for wrapper-only projects - // (`./mvnw`, `./gradlew`), so callers must not gate them on a PATH lookup. - configOnly bool - // publishOnly marks the package managers configured for uploading rather than - // resolving. They are not part of install-time routing. - publishOnly bool - // configGroup names the configuration file this package manager shares with - // others. Everything in one group writes the SAME file, so concurrent `jf setup` - // runs across a group race each other and callers have to serialize them. - configGroup string } -// One entry per package manager in packageManagerToRepositoryPackageType. -// TestGetCapabilities_EveryEntryIsFullyPopulated keeps the two maps in step: a -// package manager added there without an entry here publishes an empty location -// and empty client binaries through GetCapabilities, and fails that test. +// One entry per package manager in packageManagerToRepositoryPackageType; +// TestPackageManagerConfigs_CoversEverySupportedPackageManager asserts the two +// stay in step. var packageManagerConfigs = map[project.ProjectType]packageManagerConfig{ // npm resolves the file `npm config set` writes through NPM_CONFIG_USERCONFIG. - // npm, pnpm and Yarn are NOT one config group: each writes its own file. project.Npm: {location: "your user-level npm configuration (.npmrc)", overrideEnv: "NPM_CONFIG_USERCONFIG"}, // pnpm is deliberately left without an override: `pnpm config set` writes to // pnpm's own config directory (auth.ini) and ignores NPM_CONFIG_USERCONFIG, which @@ -104,45 +76,26 @@ var packageManagerConfigs = map[project.ProjectType]packageManagerConfig{ // Yarn Classic writes ~/.yarnrc, and YARN_RC_FILENAME does not redirect it. project.Yarn: {location: "your user-level Yarn configuration (.yarnrc)"}, // configurePip writes the file itself when PIP_CONFIG_FILE is set, because - // `pip config set` does not support that variable. requiresClient stays true: - // the default path does shell out, and only the override path does not. - // pip and pipenv share pip.conf, so their setups must not run concurrently. - project.Pip: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE", - clientBinaries: []string{"pip", "pip3"}, configGroup: ConfigGroupPipConf}, - project.Pipenv: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE", - configGroup: ConfigGroupPipConf}, + // `pip config set` does not support that variable. + project.Pip: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE"}, + project.Pipenv: {location: "your user-level pip configuration (pip.conf)", overrideEnv: "PIP_CONFIG_FILE"}, // `poetry config` writes config.toml into POETRY_CONFIG_DIR when it is set. project.Poetry: {location: "your user-level Poetry configuration (config.toml)", overrideEnv: "POETRY_CONFIG_DIR"}, // Twine's .pypirc path is chosen per invocation (--config-file), not by the environment. - project.Twine: {location: "your user-level Twine configuration (.pypirc)", publishOnly: true}, + project.Twine: {location: "your user-level Twine configuration (.pypirc)"}, // ConfigureUVIndex writes to UV_CONFIG_FILE when it is set. - // minVersion is deliberately unset: configureUV needs `uv auth login`, and the - // release that introduced it has not been confirmed. Fly Desktop asserts 0.8.15. - project.UV: {location: "your user-level uv configuration (uv.toml)", overrideEnv: "UV_CONFIG_FILE"}, - // nuget and dotnet both write NuGet.Config, so their setups must not run concurrently. - // Neither client accepts --version; both print it through `help`. - project.Nuget: {location: "your user-level NuGet configuration (NuGet.Config)", - versionCmd: []string{"help"}, configGroup: ConfigGroupNugetConfig}, - project.Dotnet: {location: "your user-level NuGet configuration (NuGet.Config)", - versionCmd: []string{"help"}, configGroup: ConfigGroupNugetConfig}, + project.UV: {location: "your user-level uv configuration (uv.toml)", overrideEnv: "UV_CONFIG_FILE"}, + project.Nuget: {location: "your user-level NuGet configuration (NuGet.Config)"}, + project.Dotnet: {location: "your user-level NuGet configuration (NuGet.Config)"}, // `go env -w` writes to the file GOENV points at, defaulting to the per-user Go env file. - project.Go: {location: "your user-level Go environment (GOPROXY in your Go env file)", overrideEnv: "GOENV", - versionCmd: []string{"version"}}, + project.Go: {location: "your user-level Go environment (GOPROXY in your Go env file)", overrideEnv: "GOENV"}, // gradle.WriteInitScript drops the script under GRADLE_USER_HOME when it is set. - // configOnly: WriteInitScript writes the file directly, so `gradle` need not exist. - project.Gradle: {location: "your user-level Gradle configuration (an init script in your Gradle user home)", - overrideEnv: gradle.UserHomeEnv, configOnly: true}, + project.Gradle: {location: "your user-level Gradle configuration (an init script in your Gradle user home)", overrideEnv: gradle.UserHomeEnv}, // Maven picks the settings file per invocation (-s), not from the environment. - // configOnly: configureMaven goes through NewSettingsXmlManager, so `mvn` need not exist. - project.Maven: {location: "your user-level Maven settings (settings.xml)", configOnly: true, - aliases: []string{"mvn"}, clientBinaries: []string{"mvn"}}, - // Docker and Podman keep separate credential stores, so they are not one group. + project.Maven: {location: "your user-level Maven settings (settings.xml)"}, project.Docker: {location: "your Docker credential store", credentialsOnly: true}, project.Podman: {location: "your Podman credential store", credentialsOnly: true}, - // configureHelm runs `helm registry login` against an OCI registry. OCI support - // reached general availability in Helm 3.8.0, so older clients cannot be configured. - project.Helm: {location: "your Helm registry credential store", credentialsOnly: true, - versionCmd: []string{"version", "--short"}, minVersion: "3.8.0"}, + project.Helm: {location: "your Helm registry credential store", credentialsOnly: true}, } // configScopeNote describes what the command changed and how widely it applies, or diff --git a/artifactory/commands/setup/setup_test.go b/artifactory/commands/setup/setup_test.go index f26e809d..48edac77 100644 --- a/artifactory/commands/setup/setup_test.go +++ b/artifactory/commands/setup/setup_test.go @@ -1182,3 +1182,16 @@ func TestMaskGoProxyCredentials(t *testing.T) { }) } } + +// packageManagerConfigs drives the note printed after every successful setup, so a +// package manager added to packageManagerToRepositoryPackageType without an entry +// here would silently print nothing. The map comment promises these stay in step. +func TestPackageManagerConfigs_CoversEverySupportedPackageManager(t *testing.T) { + assert.Len(t, packageManagerConfigs, len(packageManagerToRepositoryPackageType)) + for packageManager := range packageManagerToRepositoryPackageType { + config, ok := packageManagerConfigs[packageManager] + if assert.True(t, ok, "%s is supported by jf setup but has no packageManagerConfigs entry", packageManager) { + assert.NotEmpty(t, config.location, "%s has an entry with no location", packageManager) + } + } +}