From 97fbd8115fd35e8d27b1cd85d0bb9e831f4c87a4 Mon Sep 17 00:00:00 2001 From: Phavya Jayakumar Date: Mon, 27 Jul 2026 13:50:36 +0530 Subject: [PATCH 1/2] XRAY-156301 - Implement http HEAD GET security for pip and poetry --- commands/curation/curationaudit.go | 34 ++--- commands/curation/curationaudit_test.go | 163 +++++++++++++++++------- 2 files changed, 136 insertions(+), 61 deletions(-) diff --git a/commands/curation/curationaudit.go b/commands/curation/curationaudit.go index 087068904..a5fec4814 100644 --- a/commands/curation/curationaudit.go +++ b/commands/curation/curationaudit.go @@ -847,9 +847,9 @@ func (ca *CurationAuditCommand) getRtManagerAndAuth(tech techutils.Technology) ( return } -// pipenvBoundedRedirectManager must use zero retries: HttpClient.Send retries +// boundedRedirectManager must use zero retries: HttpClient.Send retries // on CheckRedirect errors, desyncing SendWithBoundedRedirects's hop counter. -func pipenvBoundedRedirectManager(serverDetails *config.ServerDetails) (artifactory.ArtifactoryServicesManager, error) { +func boundedRedirectManager(serverDetails *config.ServerDetails) (artifactory.ArtifactoryServicesManager, error) { return rtUtils.CreateServiceManager(serverDetails, 0, 0, false) } @@ -1007,8 +1007,8 @@ func (ca *CurationAuditCommand) auditTree(tech techutils.Technology, results map if err != nil { return err } - if tech == techutils.Pipenv { - rtManager, err = pipenvBoundedRedirectManager(serverDetails) + if tech == techutils.Pip || tech == techutils.Poetry || tech == techutils.Pipenv { + rtManager, err = boundedRedirectManager(serverDetails) if err != nil { return err } @@ -1743,8 +1743,8 @@ func (nc *treeAnalyzer) fetchNodeStatus(node xrayUtils.GraphNode, p *sync.Map) e requestDetails := nc.httpClientDetails.Clone() var resp *http.Response var err error - if nc.tech == techutils.Pipenv { - resp, _, err = nc.sendPipenvRequest(http.MethodHead, packageUrl, requestDetails) + if nc.tech == techutils.Pip || nc.tech == techutils.Poetry || nc.tech == techutils.Pipenv { + resp, _, err = nc.sendBoundedRequest(http.MethodHead, packageUrl, requestDetails) } else { resp, _, err = nc.rtManager.Client().SendHead(packageUrl, requestDetails) } @@ -1804,7 +1804,7 @@ func (nc *treeAnalyzer) fetchNodeStatus(node xrayUtils.GraphNode, p *sync.Map) e return nil } -func (nc *treeAnalyzer) sendPipenvRequest(method, requestURL string, details *httputils.HttpClientDetails) (*http.Response, []byte, error) { +func (nc *treeAnalyzer) sendBoundedRequest(method, requestURL string, details *httputils.HttpClientDetails) (*http.Response, []byte, error) { repositoryURL := fmt.Sprintf("%s/api/pypi/%s/", strings.TrimSuffix(nc.url, "/"), nc.repo) boundary, err := utils.NewEndpointBoundary(repositoryURL) if err != nil { @@ -1824,8 +1824,8 @@ func (ca *CurationAuditCommand) runCvsFallback(cvsErr *python.CvsBlockedError, t if err != nil { return fmt.Errorf("curation-blocked resolution fallback: failed to get Artifactory manager (%w); %s error: %w", err, tech, cvsErr) } - if tech == techutils.Pipenv { - rtManager, err = pipenvBoundedRedirectManager(serverDetails) + if tech == techutils.Pip || tech == techutils.Poetry || tech == techutils.Pipenv { + rtManager, err = boundedRedirectManager(serverDetails) if err != nil { return fmt.Errorf("curation-blocked resolution fallback: failed to create bounded HTTP manager: %w", err) } @@ -1873,8 +1873,8 @@ func (nc *treeAnalyzer) lookupPypiAllVersions(name string) ([]string, error) { var resp *http.Response var body []byte var err error - if nc.tech == techutils.Pipenv { - resp, body, err = nc.sendPipenvRequest(http.MethodGet, metadataURL, requestDetails) + if nc.tech == techutils.Pip || nc.tech == techutils.Poetry || nc.tech == techutils.Pipenv { + resp, body, err = nc.sendBoundedRequest(http.MethodGet, metadataURL, requestDetails) } else { resp, body, _, err = nc.rtManager.Client().SendGet(metadataURL, true, requestDetails) } @@ -1913,8 +1913,8 @@ func (nc *treeAnalyzer) lookupPypiNormalDownloadURL(name, ver string) (string, e var resp *http.Response var body []byte var err error - if nc.tech == techutils.Pipenv { - resp, body, err = nc.sendPipenvRequest(http.MethodGet, metadataURL, requestDetails) + if nc.tech == techutils.Pip || nc.tech == techutils.Poetry || nc.tech == techutils.Pipenv { + resp, body, err = nc.sendBoundedRequest(http.MethodGet, metadataURL, requestDetails) } else { resp, body, _, err = nc.rtManager.Client().SendGet(metadataURL, true, requestDetails) } @@ -2014,8 +2014,8 @@ func (nc *treeAnalyzer) fetchCvsBlockedStatus(pins []python.PinnedRequirement) [ headDetails := nc.httpClientDetails.Clone() var headResp *http.Response var headErr error - if nc.tech == techutils.Pipenv { - headResp, _, headErr = nc.sendPipenvRequest(http.MethodHead, dlURL, headDetails) + if nc.tech == techutils.Pip || nc.tech == techutils.Poetry || nc.tech == techutils.Pipenv { + headResp, _, headErr = nc.sendBoundedRequest(http.MethodHead, dlURL, headDetails) } else { headResp, _, headErr = nc.rtManager.Client().SendHead(dlURL, headDetails) } @@ -2114,8 +2114,8 @@ func (nc *treeAnalyzer) getBlockedPackageDetails(packageUrl string, name string, var getResp *http.Response var respBody []byte var err error - if nc.tech == techutils.Pipenv { - getResp, respBody, err = nc.sendPipenvRequest(http.MethodGet, packageUrl, requestDetails) + if nc.tech == techutils.Pip || nc.tech == techutils.Poetry || nc.tech == techutils.Pipenv { + getResp, respBody, err = nc.sendBoundedRequest(http.MethodGet, packageUrl, requestDetails) } else { getResp, respBody, _, err = nc.rtManager.Client().SendGet(packageUrl, true, requestDetails) } diff --git a/commands/curation/curationaudit_test.go b/commands/curation/curationaudit_test.go index ded6cfb2d..2717f5665 100644 --- a/commands/curation/curationaudit_test.go +++ b/commands/curation/curationaudit_test.go @@ -2310,12 +2310,16 @@ func TestGetBlockedPackageDetails_403UnparsableBodyReturnsBlocked(t *testing.T) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - serverMock, _, rtManager := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + serverMock, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusForbidden) _, _ = w.Write([]byte(tt.respBody)) }) defer serverMock.Close() + // Poetry (like Pip/Pipenv) routes through sendBoundedRequest, which requires a + // zero-retry client — mirrors the production boundedRedirectManager construction. + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) rtAuth := rtManager.GetConfig().GetServiceDetails() httpClientDetails := rtAuth.CreateHttpClientDetails() analyzer := treeAnalyzer{ @@ -2371,7 +2375,7 @@ func TestFetchCvsBlockedStatusTransitive(t *testing.T) { // Version-specific metadata JSON (returns the whl download URL). versionMetaJSON := fmt.Sprintf(`{"urls":[{"packagetype":"bdist_wheel","url":"../../%s"}]}`, whlRelativePath) - serverMock, _, rtManager := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + serverMock, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { switch { // All-versions metadata: /api/pypi//pypi//json case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/pypi/"+blockedPkg+"/json"): @@ -2398,6 +2402,10 @@ func TestFetchCvsBlockedStatusTransitive(t *testing.T) { }) defer serverMock.Close() + // Pip (like Poetry/Pipenv) routes through sendBoundedRequest, which requires a + // zero-retry client — mirrors the production boundedRedirectManager construction. + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) rtAuth := rtManager.GetConfig().GetServiceDetails() httpClientDetails := rtAuth.CreateHttpClientDetails() @@ -2467,7 +2475,7 @@ func TestFetchCvsBlockedStatusPoetry(t *testing.T) { blockResponse := fmt.Sprintf(`{"errors":[{"status":403,"message":%q}]}`, blockMsg) versionMetaJSON := fmt.Sprintf(`{"urls":[{"packagetype":"bdist_wheel","url":"../../%s"}]}`, whlRelativePath) - serverMock, _, rtManager := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + serverMock, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/pypi/"+blockedPkg+"/"+blockedVer+"/json"): w.WriteHeader(http.StatusOK) @@ -2483,6 +2491,10 @@ func TestFetchCvsBlockedStatusPoetry(t *testing.T) { }) defer serverMock.Close() + // Poetry (like Pip/Pipenv) routes through sendBoundedRequest, which requires a + // zero-retry client — mirrors the production boundedRedirectManager construction. + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) rtAuth := rtManager.GetConfig().GetServiceDetails() httpClientDetails := rtAuth.CreateHttpClientDetails() @@ -2525,7 +2537,7 @@ func TestFetchCvsBlockedStatusNotInMetadataNotRendered(t *testing.T) { ver = "4.87.1000" // not in the metadata API ) - serverMock, _, rtManager := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + serverMock, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/pypi/"+pkg+"/"+ver+"/json"): w.WriteHeader(http.StatusNotFound) @@ -2535,6 +2547,10 @@ func TestFetchCvsBlockedStatusNotInMetadataNotRendered(t *testing.T) { }) defer serverMock.Close() + // Pip (like Poetry/Pipenv) routes through sendBoundedRequest, which requires a + // zero-retry client — mirrors the production boundedRedirectManager construction. + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) rtAuth := rtManager.GetConfig().GetServiceDetails() analyzer := treeAnalyzer{ rtManager: rtManager, @@ -2566,7 +2582,7 @@ func TestFetchCvsBlockedStatusSetsDepRelation(t *testing.T) { whlRelativePath = "packages/ab/cd/langchain_core-1.4.7-py3-none-any.whl" ) - serverMock, _, rtManager := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + serverMock, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/pypi/"+blockedPkg+"/json"): w.WriteHeader(http.StatusOK) @@ -2583,6 +2599,10 @@ func TestFetchCvsBlockedStatusSetsDepRelation(t *testing.T) { }) defer serverMock.Close() + // Pip (like Poetry/Pipenv) routes through sendBoundedRequest, which requires a + // zero-retry client — mirrors the production boundedRedirectManager construction. + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) rtAuth := rtManager.GetConfig().GetServiceDetails() analyzer := treeAnalyzer{ rtManager: rtManager, @@ -2630,7 +2650,7 @@ func TestFetchCvsBlockedStatusHeadErrorNoFalsePositive(t *testing.T) { whlRelativePath = "packages/ab/cd/foo-1.0-py3-none-any.whl" ) - serverMock, _, rtManager := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + serverMock, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == http.MethodHead: w.WriteHeader(http.StatusInternalServerError) @@ -2643,6 +2663,10 @@ func TestFetchCvsBlockedStatusHeadErrorNoFalsePositive(t *testing.T) { }) defer serverMock.Close() + // Pip (like Poetry/Pipenv) routes through sendBoundedRequest, which requires a + // zero-retry client — mirrors the production boundedRedirectManager construction. + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) rtAuth := rtManager.GetConfig().GetServiceDetails() analyzer := treeAnalyzer{ rtManager: rtManager, @@ -2671,7 +2695,7 @@ func TestFetchCvsBlockedStatusHeadOKNoFalsePositive(t *testing.T) { whlRelativePath = "packages/ab/cd/foo-1.0-py3-none-any.whl" ) - serverMock, _, rtManager := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + serverMock, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == http.MethodHead: // Stale CVS cache cleared; package is now accessible. @@ -2685,6 +2709,10 @@ func TestFetchCvsBlockedStatusHeadOKNoFalsePositive(t *testing.T) { }) defer serverMock.Close() + // Pip (like Poetry/Pipenv) routes through sendBoundedRequest, which requires a + // zero-retry client — mirrors the production boundedRedirectManager construction. + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) rtAuth := rtManager.GetConfig().GetServiceDetails() analyzer := treeAnalyzer{ rtManager: rtManager, @@ -2873,41 +2901,45 @@ url = "https://user:token@acme.jfrog.io/artifactory/api/pypi/repo/simple" assert.Nil(t, ca.PackageManagerConfig) } -func TestSendPipenvRequestRejectsRedirectOutsideRepository(t *testing.T) { - var outsideRequested atomic.Bool - var requests atomic.Int32 - server, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { - requests.Add(1) - if r.URL.Path == "/api/system/configuration" { - outsideRequested.Store(true) - w.WriteHeader(http.StatusOK) - return - } - http.Redirect(w, r, "/api/system/configuration", http.StatusFound) - }) - defer server.Close() - rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) - require.NoError(t, err) - rtAuth := rtManager.GetConfig().GetServiceDetails() - analyzer := treeAnalyzer{ - rtManager: rtManager, - httpClientDetails: rtAuth.CreateHttpClientDetails(), - url: rtAuth.GetUrl(), - repo: "repo", - tech: techutils.Pipenv, - } - requestDetails := analyzer.httpClientDetails.Clone() - requestDetails.Headers["X-Artifactory-Curation-Request-Waiver"] = "syn" +func TestSendBoundedRequestRejectsRedirectOutsideRepository(t *testing.T) { + for _, tech := range []techutils.Technology{techutils.Pip, techutils.Poetry, techutils.Pipenv} { + t.Run(tech.String(), func(t *testing.T) { + var outsideRequested atomic.Bool + var requests atomic.Int32 + server, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + if r.URL.Path == "/api/system/configuration" { + outsideRequested.Store(true) + w.WriteHeader(http.StatusOK) + return + } + http.Redirect(w, r, "/api/system/configuration", http.StatusFound) + }) + defer server.Close() + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) + rtAuth := rtManager.GetConfig().GetServiceDetails() + analyzer := treeAnalyzer{ + rtManager: rtManager, + httpClientDetails: rtAuth.CreateHttpClientDetails(), + url: rtAuth.GetUrl(), + repo: "repo", + tech: tech, + } + requestDetails := analyzer.httpClientDetails.Clone() + requestDetails.Headers["X-Artifactory-Curation-Request-Waiver"] = "syn" - _, _, err = analyzer.sendPipenvRequest(http.MethodGet, - strings.TrimSuffix(analyzer.url, "/")+"/api/pypi/repo/packages/pkg.whl", requestDetails) - require.Error(t, err) - assert.Contains(t, err.Error(), "unsafe redirect") - assert.False(t, outsideRequested.Load()) - assert.Equal(t, int32(1), requests.Load()) + _, _, err = analyzer.sendBoundedRequest(http.MethodGet, + strings.TrimSuffix(analyzer.url, "/")+"/api/pypi/repo/packages/pkg.whl", requestDetails) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsafe redirect") + assert.False(t, outsideRequested.Load()) + assert.Equal(t, int32(1), requests.Load()) + }) + } } -func TestPipenvCvsMetadataRejectsRedirectOutsideRepository(t *testing.T) { +func TestCvsMetadataRejectsRedirectOutsideRepository(t *testing.T) { tests := []struct { name string call func(*treeAnalyzer) error @@ -2927,8 +2959,48 @@ func TestPipenvCvsMetadataRejectsRedirectOutsideRepository(t *testing.T) { }, }, } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { + for _, tech := range []techutils.Technology{techutils.Pip, techutils.Poetry, techutils.Pipenv} { + for _, test := range tests { + t.Run(tech.String()+"/"+test.name, func(t *testing.T) { + var outsideRequested atomic.Bool + var requests atomic.Int32 + server, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + if r.URL.Path == "/api/system/configuration" { + outsideRequested.Store(true) + w.WriteHeader(http.StatusOK) + return + } + http.Redirect(w, r, "/api/system/configuration", http.StatusFound) + }) + defer server.Close() + rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) + require.NoError(t, err) + rtAuth := rtManager.GetConfig().GetServiceDetails() + analyzer := &treeAnalyzer{ + rtManager: rtManager, + httpClientDetails: rtAuth.CreateHttpClientDetails(), + url: rtAuth.GetUrl(), + repo: "repo", + tech: tech, + } + + err = test.call(analyzer) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsafe redirect") + assert.False(t, outsideRequested.Load()) + assert.Equal(t, int32(1), requests.Load()) + }) + } + } +} + +// TestFetchNodeStatusRoutesPipAndPoetryThroughBoundedRedirects exercises the actual +// tech-branch in fetchNodeStatus (not sendBoundedRequest directly) to guard against a +// regression that silently narrows the bounded-redirect condition back to Pipenv only. +func TestFetchNodeStatusRoutesPipAndPoetryThroughBoundedRedirects(t *testing.T) { + for _, tech := range []techutils.Technology{techutils.Pip, techutils.Poetry, techutils.Pipenv} { + t.Run(tech.String(), func(t *testing.T) { var outsideRequested atomic.Bool var requests atomic.Int32 server, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { @@ -2944,15 +3016,18 @@ func TestPipenvCvsMetadataRejectsRedirectOutsideRepository(t *testing.T) { rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) require.NoError(t, err) rtAuth := rtManager.GetConfig().GetServiceDetails() - analyzer := &treeAnalyzer{ + nodeId := python.PythonPackageTypeIdentifier + "pkg:1.0.0" + packageUrl := strings.TrimSuffix(rtAuth.GetUrl(), "/") + "/api/pypi/repo/packages/pkg.whl" + analyzer := treeAnalyzer{ rtManager: rtManager, httpClientDetails: rtAuth.CreateHttpClientDetails(), url: rtAuth.GetUrl(), repo: "repo", - tech: techutils.Pipenv, + tech: tech, + downloadUrls: map[string]string{nodeId: packageUrl}, } - err = test.call(analyzer) + err = analyzer.fetchNodeStatus(xrayUtils.GraphNode{Id: nodeId}, &sync.Map{}) require.Error(t, err) assert.Contains(t, err.Error(), "unsafe redirect") assert.False(t, outsideRequested.Load()) From 7d5af3ec431b64e45fe70e7c8e896ca26609e561 Mon Sep 17 00:00:00 2001 From: Phavya Jayakumar Date: Thu, 30 Jul 2026 18:26:51 +0530 Subject: [PATCH 2/2] XRAY-156301 - Move redirect boundary validation to jfrog-client-go --- commands/curation/curationaudit.go | 7 +- go.mod | 2 + go.sum | 4 +- .../buildinfo/technologies/python/python.go | 7 +- utils/http_redirect.go | 125 -------------- utils/http_redirect_test.go | 160 ------------------ 6 files changed, 12 insertions(+), 293 deletions(-) delete mode 100644 utils/http_redirect.go delete mode 100644 utils/http_redirect_test.go diff --git a/commands/curation/curationaudit.go b/commands/curation/curationaudit.go index a5fec4814..ef41c7345 100644 --- a/commands/curation/curationaudit.go +++ b/commands/curation/curationaudit.go @@ -31,6 +31,7 @@ import ( "github.com/jfrog/jfrog-client-go/artifactory" "github.com/jfrog/jfrog-client-go/auth" + "github.com/jfrog/jfrog-client-go/http/redirect" clientutils "github.com/jfrog/jfrog-client-go/utils" "github.com/jfrog/jfrog-client-go/utils/errorutils" "github.com/jfrog/jfrog-client-go/utils/io/httputils" @@ -1806,12 +1807,12 @@ func (nc *treeAnalyzer) fetchNodeStatus(node xrayUtils.GraphNode, p *sync.Map) e func (nc *treeAnalyzer) sendBoundedRequest(method, requestURL string, details *httputils.HttpClientDetails) (*http.Response, []byte, error) { repositoryURL := fmt.Sprintf("%s/api/pypi/%s/", strings.TrimSuffix(nc.url, "/"), nc.repo) - boundary, err := utils.NewEndpointBoundary(repositoryURL) + boundary, err := redirect.NewEndpointBoundary(repositoryURL) if err != nil { return nil, nil, err } - return utils.SendWithBoundedRedirects(nc.rtManager.Client(), method, requestURL, details, - boundary, utils.MaxAuthenticatedRedirects) + return redirect.SendWithBoundedRedirects(nc.rtManager.Client(), method, requestURL, details, + boundary, redirect.MaxAuthenticatedRedirects) } // runCvsFallback is called when pip or poetry resolution failed because CVS diff --git a/go.mod b/go.mod index f2e4b3ca6..d5d1969c7 100644 --- a/go.mod +++ b/go.mod @@ -167,3 +167,5 @@ require ( // replace github.com/jfrog/build-info-go => github.com/jfrog/build-info-go dev // replace github.com/jfrog/froggit-go => github.com/jfrog/froggit-go master + +replace github.com/jfrog/jfrog-client-go => github.com/Phavya-jfrog/jfrog-client-go v0.0.0-20260730125004-28d1eaa0c7f1 diff --git a/go.sum b/go.sum index ea75a7eb3..5f9d77245 100644 --- a/go.sum +++ b/go.sum @@ -9,6 +9,8 @@ github.com/CycloneDX/cyclonedx-go v0.10.0/go.mod h1:vUvbCXQsEm48OI6oOlanxstwNByX github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= +github.com/Phavya-jfrog/jfrog-client-go v0.0.0-20260730125004-28d1eaa0c7f1 h1:Z/2IQxSeUhrmxzWQQAJXPOXOtd1VTfPw45w1aqtiRZw= +github.com/Phavya-jfrog/jfrog-client-go v0.0.0-20260730125004-28d1eaa0c7f1/go.mod h1:FHpjN1nTDoj96xd6obe27EOgGErqzU0rQgC96L3Ch9E= github.com/ProtonMail/go-crypto v1.4.1 h1:9RfcZHqEQUvP8RzecWEUafnZVtEvrBVL9BiF67IQOfM= github.com/ProtonMail/go-crypto v1.4.1/go.mod h1:e1OaTyu5SYVrO9gKOEhTc+5UcXtTUa+P3uLudwcgPqo= github.com/VividCortex/ewma v1.2.0 h1:f58SaIzcDXrSy3kWaHNvuJgJ3Nmz59Zji6XoJR/q1ow= @@ -173,8 +175,6 @@ github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260728121041-2227ac7420a0 h1:D github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260728121041-2227ac7420a0/go.mod h1:1vxzqW7jHBSuTNqO2vxEnhbniwq4dj5wveDuCMJX7Yo= github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260728123939-34b27f070f2e h1:K0IK3w5a5h6SIi9yoOJ6a7DL+kuFjs5acypOxKyT2OM= github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260728123939-34b27f070f2e/go.mod h1:MygQx8pekgPCXyXnejIAVG9S4ImGcDFmcfRPUug/0d0= -github.com/jfrog/jfrog-client-go v1.55.1-0.20260729072925-e1104f6b9e00 h1:QekOqpZ4c34Xb/eX9UCM1JMGipGoW/ZpkbwsWrx/kAQ= -github.com/jfrog/jfrog-client-go v1.55.1-0.20260729072925-e1104f6b9e00/go.mod h1:FHpjN1nTDoj96xd6obe27EOgGErqzU0rQgC96L3Ch9E= github.com/jhump/protoreflect v1.15.1 h1:HUMERORf3I3ZdX05WaQ6MIpd/NJ434hTp5YiKgfCL6c= github.com/jhump/protoreflect v1.15.1/go.mod h1:jD/2GMKKE6OqX8qTjhADU1e6DShO+gavG9e0Q693nKo= github.com/kevinburke/ssh_config v1.6.0 h1:J1FBfmuVosPHf5GRdltRLhPJtJpTlMdKTBjRgTaQBFY= diff --git a/sca/bom/buildinfo/technologies/python/python.go b/sca/bom/buildinfo/technologies/python/python.go index 477e5655f..8dfd2014e 100644 --- a/sca/bom/buildinfo/technologies/python/python.go +++ b/sca/bom/buildinfo/technologies/python/python.go @@ -22,6 +22,7 @@ import ( "github.com/jfrog/jfrog-cli-security/utils" "github.com/jfrog/jfrog-cli-security/utils/techutils" "github.com/jfrog/jfrog-client-go/artifactory" + "github.com/jfrog/jfrog-client-go/http/redirect" "github.com/jfrog/jfrog-client-go/utils/errorutils" "github.com/jfrog/jfrog-client-go/utils/io/fileutils" "github.com/jfrog/jfrog-client-go/utils/io/httputils" @@ -546,14 +547,14 @@ func resolvePipenvPackageURL(rtManager artifactory.ArtifactoryServicesManager, h func buildPipenvDownloadUrl(rtManager artifactory.ArtifactoryServicesManager, clientDetails *httputils.HttpClientDetails, artiUrl, repository string, pkg pipfileLockPackage) (string, error) { normalized := NormalizePypiName(pkg.Name) repositoryURL := fmt.Sprintf("%s/api/pypi/%s/", artiUrl, repository) - boundary, err := utils.NewEndpointBoundary(repositoryURL) + boundary, err := redirect.NewEndpointBoundary(repositoryURL) if err != nil { return "", err } simpleIndexUrl := repositoryURL + "simple/" + normalized + "/" log.Debug(fmt.Sprintf("Pipenv: GET simple-index %s (matching against %d hashes)", simpleIndexUrl, len(pkg.Hashes))) - resp, body, err := utils.SendWithBoundedRedirects(rtManager.Client(), http.MethodGet, simpleIndexUrl, - clientDetails, boundary, utils.MaxAuthenticatedRedirects) + resp, body, err := redirect.SendWithBoundedRedirects(rtManager.Client(), http.MethodGet, simpleIndexUrl, + clientDetails, boundary, redirect.MaxAuthenticatedRedirects) if err != nil { return "", err } diff --git a/utils/http_redirect.go b/utils/http_redirect.go deleted file mode 100644 index e62800a36..000000000 --- a/utils/http_redirect.go +++ /dev/null @@ -1,125 +0,0 @@ -package utils - -import ( - "fmt" - "net/http" - "net/url" - "path" - "strings" - - "github.com/jfrog/jfrog-client-go/http/jfroghttpclient" - "github.com/jfrog/jfrog-client-go/utils/io/httputils" -) - -// This logic will be shifted to jfrog-client-go under https://jfrog-int.atlassian.net/browse/XRAY-156301 -const MaxAuthenticatedRedirects = 3 - -type EndpointBoundary struct { - scheme string - host string - path string -} - -func NewEndpointBoundary(rawBaseURL string) (EndpointBoundary, error) { - u, err := url.Parse(rawBaseURL) - if err != nil || u.Host == "" || u.User != nil || u.RawPath != "" || u.RawQuery != "" || u.Fragment != "" || - (!strings.EqualFold(u.Scheme, "http") && !strings.EqualFold(u.Scheme, "https")) { - return EndpointBoundary{}, fmt.Errorf("invalid endpoint boundary") - } - cleanPath, valid := normalizedBoundaryPath(u.Path) - if !valid { - return EndpointBoundary{}, fmt.Errorf("invalid endpoint boundary") - } - return EndpointBoundary{ - scheme: strings.ToLower(u.Scheme), - host: normalizedHost(u), - path: cleanPath, - }, nil -} - -func (b EndpointBoundary) Validate(rawURL string) error { - u, err := url.Parse(rawURL) - if err != nil || u.Scheme == "" || u.Host == "" || u.User != nil { - return fmt.Errorf("invalid request URL") - } - if hasAmbiguousPathEscape(u.EscapedPath()) { - return fmt.Errorf("ambiguous escaped request path") - } - requestPath, valid := normalizedBoundaryPath(u.Path) - if !valid { - return fmt.Errorf("request URL escapes the configured endpoint") - } - if strings.ToLower(u.Scheme) != b.scheme || normalizedHost(u) != b.host || - !strings.HasPrefix(requestPath, b.path) { - return fmt.Errorf("request URL escapes the configured endpoint") - } - return nil -} - -func normalizedBoundaryPath(rawPath string) (string, bool) { - if rawPath == "" { - rawPath = "/" - } - cleanPath := strings.TrimSuffix(path.Clean(rawPath), "/") + "/" - return cleanPath, cleanPath == strings.TrimSuffix(rawPath, "/")+"/" -} - -func hasAmbiguousPathEscape(escapedPath string) bool { - for { - lowerPath := strings.ToLower(escapedPath) - if strings.Contains(lowerPath, "%2f") || strings.Contains(lowerPath, "%2e") || - strings.Contains(lowerPath, "%5c") { - return true - } - decoded, err := url.PathUnescape(escapedPath) - if err != nil { - return true - } - if decoded == escapedPath { - return false - } - escapedPath = decoded - } -} - -func normalizedHost(u *url.URL) string { - port := u.Port() - if port == "" { - if strings.EqualFold(u.Scheme, "http") { - port = "80" - } else if strings.EqualFold(u.Scheme, "https") { - port = "443" - } - } - return strings.ToLower(u.Hostname()) + ":" + port -} - -func SendWithBoundedRedirects(client *jfroghttpclient.JfrogHttpClient, method, requestURL string, - details *httputils.HttpClientDetails, boundary EndpointBoundary, maxRedirects int, -) (*http.Response, []byte, error) { - // JfrogHttpClient has no per-request redirect hook, so validate each hop before forwarding auth. - if maxRedirects < 0 { - return nil, nil, fmt.Errorf("redirect limit must be non-negative") - } - // HttpClient.Send retries on CheckRedirect errors, which would desync this hop counter. - if retries := client.GetHttpClient().GetRetries(); retries != 0 { - return nil, nil, fmt.Errorf("bounded redirects require a zero-retry client, got %d retries configured", retries) - } - currentURL := requestURL - for redirects := 0; ; redirects++ { - if err := boundary.Validate(currentURL); err != nil { - return nil, nil, err - } - resp, body, redirectURL, err := client.Send(method, currentURL, nil, false, true, details.Clone(), "") - if redirectURL == "" { - return resp, body, err - } - if redirects == maxRedirects { - return resp, body, fmt.Errorf("redirect limit of %d exceeded", maxRedirects) - } - if err := boundary.Validate(redirectURL); err != nil { - return resp, body, fmt.Errorf("unsafe redirect: %w", err) - } - currentURL = redirectURL - } -} diff --git a/utils/http_redirect_test.go b/utils/http_redirect_test.go deleted file mode 100644 index 1fabbf9e7..000000000 --- a/utils/http_redirect_test.go +++ /dev/null @@ -1,160 +0,0 @@ -package utils - -import ( - "fmt" - "net/http" - "sync/atomic" - "testing" - - rtUtils "github.com/jfrog/jfrog-cli-core/v2/artifactory/utils" - coreCommonTests "github.com/jfrog/jfrog-cli-core/v2/common/tests" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestNewEndpointBoundary(t *testing.T) { - t.Run("bare host covers root", func(t *testing.T) { - boundary, err := NewEndpointBoundary("https://example.com") - require.NoError(t, err) - assert.Equal(t, "/", boundary.path) - assert.NoError(t, boundary.Validate("https://example.com")) - assert.NoError(t, boundary.Validate("https://example.com/api/pypi/repo/file.whl")) - }) - - t.Run("trailing slash is normalized", func(t *testing.T) { - withoutSlash, err := NewEndpointBoundary("https://example.com/api/pypi/repo") - require.NoError(t, err) - withSlash, err := NewEndpointBoundary("https://example.com/api/pypi/repo/") - require.NoError(t, err) - assert.Equal(t, withoutSlash, withSlash) - }) - - t.Run("IPv6 and default port are normalized", func(t *testing.T) { - boundary, err := NewEndpointBoundary("https://[2001:db8::1]/api/pypi/repo/") - require.NoError(t, err) - assert.NoError(t, boundary.Validate("https://[2001:db8::1]:443/api/pypi/repo/pkg.whl")) - assert.Error(t, boundary.Validate("https://[2001:db8::2]/api/pypi/repo/pkg.whl")) - }) - - for _, rawURL := range []string{ - "https://user@example.com/api/pypi/repo/", - "https://example.com/api/pypi/repo/?token=value", - "https://example.com/api/pypi/repo/#fragment", - "ftp://example.com/api/pypi/repo/", - } { - t.Run("rejects invalid boundary "+rawURL, func(t *testing.T) { - _, err := NewEndpointBoundary(rawURL) - require.Error(t, err) - }) - } -} - -func TestEndpointBoundaryValidateRejectsEscapes(t *testing.T) { - boundary, err := NewEndpointBoundary("https://example.com/api/pypi/repo/") - require.NoError(t, err) - - for _, rawURL := range []string{ - "https://example.com/api/pypi/repo/%2Fsecret", - "https://example.com/api/pypi/repo/%252Fsecret", - "https://example.com/api/pypi/repo/%25252e%25252e%25252fsecret", - "https://example.com/api/pypi/repo/%255csecret", - } { - t.Run(rawURL, func(t *testing.T) { - err := boundary.Validate(rawURL) - require.Error(t, err) - assert.Contains(t, err.Error(), "ambiguous escaped request path") - }) - } -} - -func TestEndpointBoundaryValidateRejectsOutsideEndpoint(t *testing.T) { - boundary, err := NewEndpointBoundary("https://example.com/api/pypi/repo/") - require.NoError(t, err) - - for _, rawURL := range []string{ - "http://example.com/api/pypi/repo/pkg.whl", - "https://other.example.com/api/pypi/repo/pkg.whl", - "https://example.com/api/pypi/other/pkg.whl", - "https://user@example.com/api/pypi/repo/pkg.whl", - } { - t.Run(rawURL, func(t *testing.T) { - assert.Error(t, boundary.Validate(rawURL)) - }) - } -} - -func TestSendWithBoundedRedirectsLimit(t *testing.T) { - tests := []struct { - name string - redirects int32 - wantErr bool - wantRequests int32 - }{ - {name: "three redirects succeed", redirects: MaxAuthenticatedRedirects, wantRequests: 4}, - {name: "fourth redirect is rejected", redirects: MaxAuthenticatedRedirects + 1, wantErr: true, wantRequests: 4}, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - var requests atomic.Int32 - server, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { - requestNumber := requests.Add(1) - if requestNumber <= test.redirects { - http.Redirect(w, r, fmt.Sprintf("/api/pypi/repo/%d", requestNumber), http.StatusFound) - return - } - w.WriteHeader(http.StatusOK) - if _, err := w.Write([]byte("ok")); err != nil { - t.Errorf("failed writing response: %v", err) - } - }) - defer server.Close() - // Zero retries required; see SendWithBoundedRedirects. - rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) - require.NoError(t, err) - boundary, err := NewEndpointBoundary(server.URL + "/api/pypi/repo/") - require.NoError(t, err) - details := rtManager.GetConfig().GetServiceDetails().CreateHttpClientDetails() - - resp, body, err := SendWithBoundedRedirects(rtManager.Client(), http.MethodGet, - server.URL+"/api/pypi/repo/0", &details, boundary, MaxAuthenticatedRedirects) - if test.wantErr { - require.Error(t, err) - assert.Contains(t, err.Error(), "redirect limit") - } else { - require.NoError(t, err) - require.NotNil(t, resp) - assert.Equal(t, http.StatusOK, resp.StatusCode) - assert.Equal(t, "ok", string(body)) - } - assert.Equal(t, test.wantRequests, requests.Load()) - }) - } -} - -func TestSendWithBoundedRedirectsRejectsLateHopEscape(t *testing.T) { - // The first redirect is legitimate; only the second hop escapes the boundary. - // Every hop must be validated, not just the initial request. - var requests atomic.Int32 - server, serverDetails, _ := coreCommonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { - switch requests.Add(1) { - case 1: - http.Redirect(w, r, "/api/pypi/repo/inner", http.StatusFound) - case 2: - http.Redirect(w, r, "/api/pypi/other-repo/secret", http.StatusFound) - default: - w.WriteHeader(http.StatusOK) - } - }) - defer server.Close() - rtManager, err := rtUtils.CreateServiceManager(serverDetails, 0, 0, false) - require.NoError(t, err) - boundary, err := NewEndpointBoundary(server.URL + "/api/pypi/repo/") - require.NoError(t, err) - details := rtManager.GetConfig().GetServiceDetails().CreateHttpClientDetails() - - _, _, err = SendWithBoundedRedirects(rtManager.Client(), http.MethodGet, - server.URL+"/api/pypi/repo/start", &details, boundary, MaxAuthenticatedRedirects) - require.Error(t, err) - assert.Contains(t, err.Error(), "unsafe redirect") - assert.Equal(t, int32(2), requests.Load(), "must stop at the escaping hop, not follow it") -}