From af8ec87c4b5afc62aa9f476218d8833db157c91a Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Sun, 8 Nov 2026 12:33:31 +0200 Subject: [PATCH 1/3] fix releases repo url construction bug --- utils/paths.go | 13 ++++--- utils/paths_test.go | 40 +++++++++++++++++++++ utils/utils.go | 12 +++++-- utils/utils_test.go | 86 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 9 deletions(-) diff --git a/utils/paths.go b/utils/paths.go index 24efd8581..f6e57fa21 100644 --- a/utils/paths.go +++ b/utils/paths.go @@ -301,12 +301,12 @@ func ToURI(path string) string { return u.String() } -func GetReleasesRemoteDetails(artifact, downloadPath, remoteRepo string, remoteServerDetails *config.ServerDetails) (server *config.ServerDetails, fullRemotePath string, err error) { +// For a configured remote repository, the resource is expected under '/artifactory/' (a remote that proxies https://releases.jfrog.io). +// fallbackRemotePath is '/' directly (a remote that proxies https://releases.jfrog.io/artifactory, or a repository holding the resources in its root). It is empty when there is no fallback. +func GetReleasesRemoteDetails(artifact, downloadPath, remoteRepo string, remoteServerDetails *config.ServerDetails) (server *config.ServerDetails, remotePath, fallbackRemotePath string, err error) { if remoteServerDetails != nil && remoteRepo != "" { // Config profile server and repo details are provided - server = remoteServerDetails - fullRemotePath = path.Join(remoteRepo, downloadPath) - return + return remoteServerDetails, path.Join(remoteRepo, "artifactory", downloadPath), path.Join(remoteRepo, downloadPath), nil } // Try to get releases remote details from environment variable server, remoteRepo, err = dependencies.GetRemoteDetails(coreutils.ReleasesRemoteEnv) @@ -314,12 +314,11 @@ func GetReleasesRemoteDetails(artifact, downloadPath, remoteRepo string, remoteS return } if remoteRepo != "" { - fullRemotePath = path.Join(remoteRepo, "artifactory", downloadPath) - return + return server, path.Join(remoteRepo, "artifactory", downloadPath), path.Join(remoteRepo, downloadPath), nil } log.Debug(fmt.Sprintf("'"+coreutils.ReleasesRemoteEnv+"' environment variable is not configured. The '%s' will be downloaded directly from releases.jfrog.io if needed.", artifact)) // If not configured to download through a remote repository in Artifactory, download from releases.jfrog.io. - return &config.ServerDetails{ArtifactoryUrl: coreutils.JfrogReleasesUrl}, downloadPath, nil + return &config.ServerDetails{ArtifactoryUrl: coreutils.JfrogReleasesUrl}, downloadPath, "", nil } // RepositoryScansListLink is a deep link into Xray's Scans List for one artifact. diff --git a/utils/paths_test.go b/utils/paths_test.go index 95e568977..c94e0861b 100644 --- a/utils/paths_test.go +++ b/utils/paths_test.go @@ -5,8 +5,11 @@ import ( "path/filepath" "testing" + "github.com/jfrog/jfrog-cli-core/v2/utils/config" + "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" "github.com/jfrog/jfrog-client-go/utils/log" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestIsPathExcludedFromRoot(t *testing.T) { @@ -163,3 +166,40 @@ func TestGetFullPathsWorkingDirs(t *testing.T) { }) } } + +func TestGetReleasesRemoteDetails(t *testing.T) { + const downloadPath = "xray-scan-lib/xray-scan-lib-1.6.0-linux-amd64.tar.gz" + profileServer := &config.ServerDetails{ArtifactoryUrl: "https://profile.example.com/artifactory/"} + testCases := []struct { + name string + remoteRepo string + remoteServer *config.ServerDetails + expectedServerUrl string + expectedRemotePath string + expectedFallbackRemotePath string + }{ + { + name: "Config profile remote repository", + remoteRepo: "releases-remote", + remoteServer: profileServer, + expectedServerUrl: profileServer.ArtifactoryUrl, + expectedRemotePath: "releases-remote/artifactory/" + downloadPath, + expectedFallbackRemotePath: "releases-remote/" + downloadPath, + }, + { + name: "Not configured, download from releases.jfrog.io", + expectedServerUrl: coreutils.JfrogReleasesUrl, + expectedRemotePath: downloadPath, + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + t.Setenv(coreutils.ReleasesRemoteEnv, "") + server, remotePath, fallbackRemotePath, err := GetReleasesRemoteDetails("Resource", downloadPath, testCase.remoteRepo, testCase.remoteServer) + require.NoError(t, err) + assert.Equal(t, testCase.expectedServerUrl, server.ArtifactoryUrl) + assert.Equal(t, testCase.expectedRemotePath, remotePath) + assert.Equal(t, testCase.expectedFallbackRemotePath, fallbackRemotePath) + }) + } +} diff --git a/utils/utils.go b/utils/utils.go index c73547612..2a9acb1c2 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "io" + "net/http" "os" "path/filepath" "regexp" @@ -456,7 +457,7 @@ type RemoteResource struct { // Get the details of the resource from the JPD / Releases remote it is downloaded from. func GetRemoteResource(resourceName, downloadPath, remoteRepo string, remoteServerDetails *config.ServerDetails) (*RemoteResource, error) { - rtDetails, remotePath, err := GetReleasesRemoteDetails(resourceName, downloadPath, remoteRepo, remoteServerDetails) + rtDetails, remotePath, fallbackRemotePath, err := GetReleasesRemoteDetails(resourceName, downloadPath, remoteRepo, remoteServerDetails) if err != nil { return nil, err } @@ -466,7 +467,14 @@ func GetRemoteResource(resourceName, downloadPath, remoteRepo string, remoteServ } downloadUrl := rtDetails.ArtifactoryUrl + remotePath remoteFileDetails, _, err := client.GetRemoteFileDetails(downloadUrl, &httpClientDetails) - if err != nil { + var httpErr *errorutils.HttpResponseError + if err != nil && fallbackRemotePath != "" && errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + fallbackUrl := rtDetails.ArtifactoryUrl + fallbackRemotePath + if remoteFileDetails, _, err = client.GetRemoteFileDetails(fallbackUrl, &httpClientDetails); err != nil { + return nil, fmt.Errorf("couldn't get remote file details for %s or %s: %s", downloadUrl, fallbackUrl, err.Error()) + } + remotePath = fallbackRemotePath + } else if err != nil { return nil, fmt.Errorf("couldn't get remote file details for %s: %s", downloadUrl, err.Error()) } return &RemoteResource{name: resourceName, serverDetails: rtDetails, path: remotePath, details: remoteFileDetails}, nil diff --git a/utils/utils_test.go b/utils/utils_test.go index 2fb55fa76..6312ef2b8 100644 --- a/utils/utils_test.go +++ b/utils/utils_test.go @@ -11,8 +11,11 @@ import ( "net/http" "net/http/httptest" "os" + "path" "path/filepath" "slices" + "strings" + "sync" "testing" "github.com/stretchr/testify/assert" @@ -299,6 +302,89 @@ func TestRemoteResourceDownloadEncrypted(t *testing.T) { } } +func TestGetRemoteResourceFallsBackToPathWithoutArtifactory(t *testing.T) { + const ( + repoPrefix = "/artifactory/" + resourcesRemoteRepo + "/" + withArtifactory = repoPrefix + "artifactory/" + encryptedArchivePath + withoutArtifactory = repoPrefix + encryptedArchivePath + ) + testCases := []struct { + name string + statusByPath map[string]int + expectedPath string + expectedRequests []string + expectedErrUrls []string + expectedErrCode string + }{ + { + name: "Found under artifactory", + statusByPath: map[string]int{withArtifactory: http.StatusOK}, + expectedPath: path.Join(resourcesRemoteRepo, "artifactory", encryptedArchivePath), + expectedRequests: []string{withArtifactory}, + }, + { + name: "Not found under artifactory, found in root", + statusByPath: map[string]int{withoutArtifactory: http.StatusOK}, + expectedPath: path.Join(resourcesRemoteRepo, encryptedArchivePath), + expectedRequests: []string{withArtifactory, withoutArtifactory}, + }, + { + name: "Not found in both", + statusByPath: map[string]int{}, + expectedRequests: []string{withArtifactory, withoutArtifactory}, + expectedErrUrls: []string{withArtifactory, withoutArtifactory}, + expectedErrCode: "404", + }, + { + name: "Non 404 error under artifactory is not retried", + statusByPath: map[string]int{withArtifactory: http.StatusForbidden, withoutArtifactory: http.StatusOK}, + expectedRequests: []string{withArtifactory}, + expectedErrUrls: []string{withArtifactory}, + expectedErrCode: "403", + }, + { + name: "Not found under artifactory, non 404 error in root", + statusByPath: map[string]int{withoutArtifactory: http.StatusForbidden}, + expectedRequests: []string{withArtifactory, withoutArtifactory}, + expectedErrUrls: []string{withArtifactory, withoutArtifactory}, + expectedErrCode: "403", + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + var mutex sync.Mutex + var requests []string + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mutex.Lock() + requests = append(requests, r.URL.Path) + mutex.Unlock() + status, exists := testCase.statusByPath[r.URL.Path] + if !exists { + status = http.StatusNotFound + } + w.Header().Set("X-Checksum-Sha256", "checksum") + w.WriteHeader(status) + })) + defer mockServer.Close() + + resource, err := GetRemoteResource("Resource", encryptedArchivePath, resourcesRemoteRepo, &config.ServerDetails{ArtifactoryUrl: mockServer.URL + "/artifactory/"}) + + assert.Equal(t, testCase.expectedRequests, requests) + if testCase.expectedErrCode != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), testCase.expectedErrCode) + for _, expectedUrl := range testCase.expectedErrUrls { + assert.Contains(t, err.Error(), mockServer.URL+expectedUrl) + } + assert.Equal(t, len(testCase.expectedErrUrls), strings.Count(err.Error(), mockServer.URL)) + return + } + require.NoError(t, err) + assert.Equal(t, testCase.expectedPath, resource.path) + }) + } +} + func createZip(t *testing.T, fileName, content string) []byte { var buffer bytes.Buffer zipWriter := zip.NewWriter(&buffer) From 295fc4ae6e78950bef9e0cd2ed9493871ac9e4f7 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Sun, 8 Nov 2026 15:05:58 +0200 Subject: [PATCH 2/3] add test --- utils/paths_test.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/utils/paths_test.go b/utils/paths_test.go index c94e0861b..0fff2956b 100644 --- a/utils/paths_test.go +++ b/utils/paths_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + commonCommands "github.com/jfrog/jfrog-cli-core/v2/common/commands" "github.com/jfrog/jfrog-cli-core/v2/utils/config" "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" "github.com/jfrog/jfrog-client-go/utils/log" @@ -12,6 +13,11 @@ import ( "github.com/stretchr/testify/require" ) +const ( + downloadPath = "xray-scan-lib/xray-scan-lib-1.6.0-linux-amd64.tar.gz" + envServerId = "envServer" +) + func TestIsPathExcludedFromRoot(t *testing.T) { root := filepath.Join(string(filepath.Separator), "ci", "proj") ancestorRoot := filepath.Join(string(filepath.Separator), "ci", "attestation", "main") @@ -168,12 +174,15 @@ func TestGetFullPathsWorkingDirs(t *testing.T) { } func TestGetReleasesRemoteDetails(t *testing.T) { - const downloadPath = "xray-scan-lib/xray-scan-lib-1.6.0-linux-amd64.tar.gz" profileServer := &config.ServerDetails{ArtifactoryUrl: "https://profile.example.com/artifactory/"} + envServer := &config.ServerDetails{ServerId: envServerId, Url: "https://env.example.com/", ArtifactoryUrl: "https://env.example.com/artifactory/"} + t.Setenv(coreutils.HomeDir, t.TempDir()) + require.NoError(t, commonCommands.NewConfigCommand(commonCommands.AddOrEdit, envServerId).SetDetails(envServer).SetInteractive(false).SetEncPassword(false).Run()) testCases := []struct { name string remoteRepo string remoteServer *config.ServerDetails + envValue string expectedServerUrl string expectedRemotePath string expectedFallbackRemotePath string @@ -186,6 +195,13 @@ func TestGetReleasesRemoteDetails(t *testing.T) { expectedRemotePath: "releases-remote/artifactory/" + downloadPath, expectedFallbackRemotePath: "releases-remote/" + downloadPath, }, + { + name: "Env var remote repository", + envValue: envServerId + "/releases-remote", + expectedServerUrl: envServer.ArtifactoryUrl, + expectedRemotePath: "releases-remote/artifactory/" + downloadPath, + expectedFallbackRemotePath: "releases-remote/" + downloadPath, + }, { name: "Not configured, download from releases.jfrog.io", expectedServerUrl: coreutils.JfrogReleasesUrl, @@ -194,7 +210,7 @@ func TestGetReleasesRemoteDetails(t *testing.T) { } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - t.Setenv(coreutils.ReleasesRemoteEnv, "") + t.Setenv(coreutils.ReleasesRemoteEnv, testCase.envValue) server, remotePath, fallbackRemotePath, err := GetReleasesRemoteDetails("Resource", downloadPath, testCase.remoteRepo, testCase.remoteServer) require.NoError(t, err) assert.Equal(t, testCase.expectedServerUrl, server.ArtifactoryUrl) From ef98a2fa58e7c06e64bfee1950240e9ccc44a739 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Sun, 8 Nov 2026 17:16:41 +0200 Subject: [PATCH 3/3] CR notes fixes --- utils/paths.go | 22 +++++++++++----------- utils/utils.go | 14 ++++++++++---- utils/utils_test.go | 24 ++++++++++++++++-------- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/utils/paths.go b/utils/paths.go index f6e57fa21..d9e441bb8 100644 --- a/utils/paths.go +++ b/utils/paths.go @@ -306,19 +306,19 @@ func ToURI(path string) string { func GetReleasesRemoteDetails(artifact, downloadPath, remoteRepo string, remoteServerDetails *config.ServerDetails) (server *config.ServerDetails, remotePath, fallbackRemotePath string, err error) { if remoteServerDetails != nil && remoteRepo != "" { // Config profile server and repo details are provided - return remoteServerDetails, path.Join(remoteRepo, "artifactory", downloadPath), path.Join(remoteRepo, downloadPath), nil - } - // Try to get releases remote details from environment variable - server, remoteRepo, err = dependencies.GetRemoteDetails(coreutils.ReleasesRemoteEnv) - if err != nil { - return + server = remoteServerDetails + } else { + // Try to get releases remote details from environment variable + if server, remoteRepo, err = dependencies.GetRemoteDetails(coreutils.ReleasesRemoteEnv); err != nil { + return + } } - if remoteRepo != "" { - return server, path.Join(remoteRepo, "artifactory", downloadPath), path.Join(remoteRepo, downloadPath), nil + if remoteRepo == "" { + log.Debug(fmt.Sprintf("'"+coreutils.ReleasesRemoteEnv+"' environment variable is not configured. The '%s' will be downloaded directly from releases.jfrog.io if needed.", artifact)) + // If not configured to download through a remote repository in Artifactory, download from releases.jfrog.io. + return &config.ServerDetails{ArtifactoryUrl: coreutils.JfrogReleasesUrl}, downloadPath, "", nil } - log.Debug(fmt.Sprintf("'"+coreutils.ReleasesRemoteEnv+"' environment variable is not configured. The '%s' will be downloaded directly from releases.jfrog.io if needed.", artifact)) - // If not configured to download through a remote repository in Artifactory, download from releases.jfrog.io. - return &config.ServerDetails{ArtifactoryUrl: coreutils.JfrogReleasesUrl}, downloadPath, "", nil + return server, path.Join(remoteRepo, "artifactory", downloadPath), path.Join(remoteRepo, downloadPath), nil } // RepositoryScansListLink is a deep link into Xray's Scans List for one artifact. diff --git a/utils/utils.go b/utils/utils.go index 2a9acb1c2..9e3bebedf 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -467,11 +467,11 @@ func GetRemoteResource(resourceName, downloadPath, remoteRepo string, remoteServ } downloadUrl := rtDetails.ArtifactoryUrl + remotePath remoteFileDetails, _, err := client.GetRemoteFileDetails(downloadUrl, &httpClientDetails) - var httpErr *errorutils.HttpResponseError - if err != nil && fallbackRemotePath != "" && errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + if err != nil && fallbackRemotePath != "" && isLayoutMismatchError(err) { fallbackUrl := rtDetails.ArtifactoryUrl + fallbackRemotePath - if remoteFileDetails, _, err = client.GetRemoteFileDetails(fallbackUrl, &httpClientDetails); err != nil { - return nil, fmt.Errorf("couldn't get remote file details for %s or %s: %s", downloadUrl, fallbackUrl, err.Error()) + var fallbackErr error + if remoteFileDetails, _, fallbackErr = client.GetRemoteFileDetails(fallbackUrl, &httpClientDetails); fallbackErr != nil { + return nil, fmt.Errorf("couldn't get remote file details for %s: %w; and for %s: %w", downloadUrl, err, fallbackUrl, fallbackErr) } remotePath = fallbackRemotePath } else if err != nil { @@ -480,6 +480,12 @@ func GetRemoteResource(resourceName, downloadPath, remoteRepo string, remoteServ return &RemoteResource{name: resourceName, serverDetails: rtDetails, path: remotePath, details: remoteFileDetails}, nil } +// A wrong repository layout returns 404, or 403 when the user's permissions cover only specific folders of the repository. +func isLayoutMismatchError(err error) bool { + var httpErr *errorutils.HttpResponseError + return errors.As(err, &httpErr) && (httpErr.StatusCode == http.StatusNotFound || httpErr.StatusCode == http.StatusForbidden) +} + // Returns true when targetDir already holds the latest version of the resource. func (rr *RemoteResource) IsCached(targetDir string) (bool, error) { return isArtifactChecksumsMatch(rr.details, filepath.Join(targetDir, dependencies.ChecksumFileName), true) diff --git a/utils/utils_test.go b/utils/utils_test.go index 6312ef2b8..37d4246d7 100644 --- a/utils/utils_test.go +++ b/utils/utils_test.go @@ -314,7 +314,7 @@ func TestGetRemoteResourceFallsBackToPathWithoutArtifactory(t *testing.T) { expectedPath string expectedRequests []string expectedErrUrls []string - expectedErrCode string + expectedErrCodes []string }{ { name: "Found under artifactory", @@ -333,21 +333,27 @@ func TestGetRemoteResourceFallsBackToPathWithoutArtifactory(t *testing.T) { statusByPath: map[string]int{}, expectedRequests: []string{withArtifactory, withoutArtifactory}, expectedErrUrls: []string{withArtifactory, withoutArtifactory}, - expectedErrCode: "404", + expectedErrCodes: []string{"404", "404"}, }, { - name: "Non 404 error under artifactory is not retried", + name: "Forbidden under artifactory, found in root", statusByPath: map[string]int{withArtifactory: http.StatusForbidden, withoutArtifactory: http.StatusOK}, + expectedPath: path.Join(resourcesRemoteRepo, encryptedArchivePath), + expectedRequests: []string{withArtifactory, withoutArtifactory}, + }, + { + name: "Unauthorized under artifactory is not retried", + statusByPath: map[string]int{withArtifactory: http.StatusUnauthorized, withoutArtifactory: http.StatusOK}, expectedRequests: []string{withArtifactory}, expectedErrUrls: []string{withArtifactory}, - expectedErrCode: "403", + expectedErrCodes: []string{"401"}, }, { - name: "Not found under artifactory, non 404 error in root", + name: "Both errors are reported when the fallback fails differently", statusByPath: map[string]int{withoutArtifactory: http.StatusForbidden}, expectedRequests: []string{withArtifactory, withoutArtifactory}, expectedErrUrls: []string{withArtifactory, withoutArtifactory}, - expectedErrCode: "403", + expectedErrCodes: []string{"404", "403"}, }, } for _, testCase := range testCases { @@ -370,9 +376,11 @@ func TestGetRemoteResourceFallsBackToPathWithoutArtifactory(t *testing.T) { resource, err := GetRemoteResource("Resource", encryptedArchivePath, resourcesRemoteRepo, &config.ServerDetails{ArtifactoryUrl: mockServer.URL + "/artifactory/"}) assert.Equal(t, testCase.expectedRequests, requests) - if testCase.expectedErrCode != "" { + if len(testCase.expectedErrCodes) > 0 { require.Error(t, err) - assert.Contains(t, err.Error(), testCase.expectedErrCode) + for _, expectedCode := range testCase.expectedErrCodes { + assert.Contains(t, err.Error(), expectedCode) + } for _, expectedUrl := range testCase.expectedErrUrls { assert.Contains(t, err.Error(), mockServer.URL+expectedUrl) }