fix: address Mend SAST findings from #215 (4 of 5 High)

cldy/clients_test.go

@@ -238,9 +238,9 @@ var _ = Describe("ApptioService agent version sanitization", func() {
 						var body map[string]any
 						Expect(json.NewDecoder(r.Body).Decode(&body)).To(Succeed())
 						capturedVersion = body["agentVersion"].(string)
-						resp := map[string]any{
-							"result": map[string]any{
-								"location":  "https://s3.example.com/upload",
+						resp := map[string]interface{}{
+							"result": map[string]interface{}{
+								"location":  "https://apptio-production.s3.us-west-2.amazonaws.com/upload",
 								"requestId": "req-123",
 							},
 						}

cldy/metrics_collector.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"math"
 	"net/http"
+	"net/url"
 	"os"
 	"path"
 	"regexp"
@@ -227,7 +228,58 @@ func getMetricsCollectorURLByRegion(region string) string {
 	}
 }
 
+// allowedUploadHostSuffixes are the domains a presigned upload URL may point at.
+//
+// The upload URL is not chosen by the agent: it is read from the "location"
+// field of a Cloudability API response and then used as the destination for a
+// PUT containing the cluster's entire resource inventory. Without validation, a
+// tampered or malicious response could redirect that payload to any host
+// (CWE-918). Presigned URLs are S3, so they resolve under amazonaws.com; the
+// Apptio and Cloudability domains are permitted because both fronting services
+// may return a URL on their own domain.
+//
+// Matching is on a dot boundary so that a lookalike host such as
+// "evil-amazonaws.com" does not satisfy an "amazonaws.com" suffix.
+var allowedUploadHostSuffixes = []string{
+	"amazonaws.com",
+	"cloudability.com",
+	"apptio.com",
+}
+
+// validateUploadURL rejects an upload destination that is not HTTPS or does not
+// sit under one of allowedUploadHostSuffixes.
+func validateUploadURL(rawURL string) error {
+	parsed, err := url.Parse(rawURL)
+	if err != nil {
+		return fmt.Errorf("upload URL could not be parsed: %w", err)
+	}
+
+	if parsed.Scheme != "https" {
+		return fmt.Errorf("refusing to upload to a non-HTTPS destination (scheme %q)", parsed.Scheme)
+	}
+
+	host := parsed.Hostname()
+	if host == "" {
+		return fmt.Errorf("upload URL has no host")
+	}
+
+	for _, suffix := range allowedUploadHostSuffixes {
+		if host == suffix || strings.HasSuffix(host, "."+suffix) {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("refusing to upload to unexpected host %q; expected one of %v",
+		host, allowedUploadHostSuffixes)
+}
+
 func uploadPayloadToPresignedURL(client ClientService, payload UploadPayload, uploadURL string) error {
+	// The destination comes from an API response rather than from our own
+	// configuration, so check it before sending the cluster inventory to it.
+	if err := validateUploadURL(uploadURL); err != nil {
+		return fmt.Errorf("invalid presigned upload URL: %w", err)
+	}
+
 	fileToUpload, err := os.Open(payload.FilePath)
 	if err != nil {
 		return fmt.Errorf("error in opening file to upload: %w", err)

cldy/metrics_collector_test.go

@@ -47,11 +47,52 @@ var _ = Describe("Metrics Collector", func() {
 			Expect(err).ToNot(HaveOccurred())
 		})
 
+		// The upload destination is taken from the API response rather than from
+		// our own configuration, so a tampered response must not be able to
+		// redirect the cluster inventory somewhere else (CWE-918).
+		DescribeTable("refuses to upload to a destination outside the allowed hosts",
+			func(location string, expected string) {
+				service := cldy.MetricsCollectorServiceImpl{
+					APIKey:    "goodkey123",
+					BaseURL:   "https://metrics-collector.example.com/metricsample",
+					UserAgent: "cldy-client/test",
+					CldyUploadClient: &metricsCollectorMockClient{
+						countByPath:      map[string]int{},
+						locationOverride: location,
+					},
+				}
+
+				payload := cldy.UploadPayload{
+					ClusterUID:   "good-cluster",
+					FileName:     "good-cluster_2025-05-05-18-05-17.tgz",
+					AgentVersion: "1.0.0",
+					UploadHash:   "aexCzQgBAnRYEZxKy71lAw==",
+					FilePath:     "testdata/daemonsets.jsonl",
+				}
+
+				err := service.Upload(payload)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring(expected))
+			},
+			Entry("attacker-controlled host",
+				"https://evil.example.com/steal", "unexpected host"),
+			Entry("lookalike host does not satisfy the suffix",
+				"https://evil-amazonaws.com/steal", "unexpected host"),
+			Entry("suffix must match on a dot boundary",
+				"https://notamazonaws.com/steal", "unexpected host"),
+			Entry("plaintext downgrade is refused",
+				"http://apptio-production.s3.us-west-2.amazonaws.com/x", "non-HTTPS"),
+			Entry("relative location has no host",
+				"somewhere/valid-location", "non-HTTPS"),
+		)
 	})
 })
 
 type metricsCollectorMockClient struct {
 	countByPath map[string]int
+	// locationOverride, when set, replaces the presigned upload URL returned by
+	// the mocked metricsample response.
+	locationOverride string
 }
 
 func (m *metricsCollectorMockClient) Do(r *http.Request, _ string) (*http.Response, error) {
@@ -66,8 +107,12 @@ func (m *metricsCollectorMockClient) Do(r *http.Request, _ string) (*http.Respon
 		Expect(r.Header.Get("x-cluster-uid")).To(Equal("good-cluster"))
 		Expect(r.Header.Get("x-upload-file")).To(Equal("aexCzQgBAnRYEZxKy71lAw=="))
 
+		location := "https://apptio-production.s3.us-west-2.amazonaws.com/somewhere/valid-location"
+		if m.locationOverride != "" {
+			location = m.locationOverride
+		}
 		responseBody, _ := json.Marshal(map[string]string{
-			"location": "https://metrics-collector.example.com/somewhere/valid-location",
+			"location": location,
 		})
 		return &http.Response{
 			StatusCode: http.StatusOK,

cldy/uploader_test.go

@@ -263,15 +263,15 @@ var _ = Describe("Uploader", func() {
 			time.Sleep(500 * time.Millisecond)
 			Expect(mcs.countByPath["/service/apikeylogin"]).To(Equal(1))
 			Expect(mcs.countByPath["/v3/internal/containers/clusters/upload"]).To(Equal(1))
-			Expect(mcs.countByPath["somewhere/valid-location"]).To(Equal(1))
+			Expect(mcs.countByPath["/somewhere/valid-location"]).To(Equal(1))
 
 			err = os.CopyFS(tempDir+"/scratch/temp_test_data", os.DirFS("testdata"))
 			Expect(err).ToNot(HaveOccurred())
 			uploader.AddSample(tempDir + "/scratch/temp_test_data")
 			time.Sleep(500 * time.Millisecond)
 			Expect(mcs.countByPath["/service/apikeylogin"]).To(Equal(1))
 			Expect(mcs.countByPath["/v3/internal/containers/clusters/upload"]).To(Equal(2))
-			Expect(mcs.countByPath["somewhere/valid-location"]).To(Equal(2))
+			Expect(mcs.countByPath["/somewhere/valid-location"]).To(Equal(2))
 		})
 
 		It("should log back in if required", func() {
@@ -297,15 +297,15 @@ var _ = Describe("Uploader", func() {
 			time.Sleep(500 * time.Millisecond)
 			Expect(mcs.countByPath["/service/apikeylogin"]).To(Equal(1))
 			Expect(mcs.countByPath["/v3/internal/containers/clusters/upload"]).To(Equal(1))
-			Expect(mcs.countByPath["somewhere/valid-location"]).To(Equal(1))
+			Expect(mcs.countByPath["/somewhere/valid-location"]).To(Equal(1))
 
 			err = os.CopyFS(tempDir+"/scratch/temp_test_data", os.DirFS("testdata"))
 			Expect(err).ToNot(HaveOccurred())
 			uploader.AddSample(tempDir + "/scratch/temp_test_data")
 			time.Sleep(500 * time.Millisecond)
 			Expect(mcs.countByPath["/service/apikeylogin"]).To(Equal(2))
 			Expect(mcs.countByPath["/v3/internal/containers/clusters/upload"]).To(Equal(2))
-			Expect(mcs.countByPath["somewhere/valid-location"]).To(Equal(2))
+			Expect(mcs.countByPath["/somewhere/valid-location"]).To(Equal(2))
 		})
 		It("should upload via metrics-collector api key", func() {
 			config := defaultConfig(tempDir)
@@ -533,7 +533,7 @@ func (mcs *mockClientService) Do(r *http.Request, _ string) (res *http.Response,
 			return &http.Response{StatusCode: 403, Body: io.NopCloser(strings.NewReader(""))}, nil
 		}
 		responseBody, _ := json.Marshal(map[string]string{
-			"location": "somewhere/valid-location",
+			"location": "https://apptio-production.s3.us-west-2.amazonaws.com/somewhere/valid-location",
 		})
 		return &http.Response{StatusCode: 200, Body: io.NopCloser(bytes.NewReader(responseBody))}, nil
 	}
@@ -546,7 +546,7 @@ func (mcs *mockClientService) Do(r *http.Request, _ string) (res *http.Response,
 		} else {
 			responseBody, _ := json.Marshal(cldy.CloudabilityClustersUploadResponse{
 				Result: cldy.CloudabilityClustersUploadInfo{
-					Location: "somewhere/valid-location",
+					Location: "https://apptio-production.s3.us-west-2.amazonaws.com/somewhere/valid-location",
 				}})
 			return &http.Response{StatusCode: 200, Body: io.NopCloser(bytes.NewReader(responseBody))}, nil
 		}

cldy/util.go

@@ -99,13 +99,50 @@ func getFileNameAndHash(filePath string) (string, string, error) {
 	return fileName, base64.StdEncoding.EncodeToString(hash.Sum(nil)), nil
 }
 
-// SafePath joins elements and creates a path that prevents file traversal while maintaining trailing separators
+// SafePath joins elements into a path that cannot escape the first element,
+// while maintaining trailing separators.
+//
+// Parent-directory segments are dropped rather than resolved, so
+// "test/scratch/../file" yields "test/scratch/file" and not "test/file".
+//
+// The removal is segment-aware. A previous implementation deleted every literal
+// ".." substring before cleaning, which was defeated by any sequence that
+// re-formed a traversal after deletion -- "....//" collapses to "../" once the
+// inner ".." is removed, so "....//....//etc/passwd" escaped to "/etc/passwd".
+// It also corrupted legitimate names, rewriting "v1..2-cluster" to
+// "v1 2-cluster". Splitting on the separator and discarding only segments that
+// are exactly ".." avoids both: "...." and "v1..2-cluster" are ordinary names
+// and are preserved intact.
 func SafePath(elements ...string) string {
-	path := strings.Join(elements, string(filepath.Separator))
-	path = strings.ReplaceAll(path, "..", "")
-	path = filepath.Clean(path)
-	if len(elements) != 0 && strings.HasSuffix(elements[len(elements)-1], string(filepath.Separator)) {
-		return path + string(filepath.Separator)
+	sep := string(filepath.Separator)
+	joined := strings.Join(elements, sep)
+
+	// Drop traversal and no-op segments. Only an exact ".." is traversal; a
+	// segment merely containing dots is a valid filename.
+	segments := strings.Split(joined, sep)
+	kept := segments[:0]
+	for _, s := range segments {
+		if s == ".." || s == "." {
+			continue
+		}
+		kept = append(kept, s)
+	}
+	path := filepath.Clean(strings.Join(kept, sep))
+
+	// Defence in depth: the segment filter above should make escape impossible,
+	// but verify containment explicitly rather than relying on that reasoning.
+	// Falling back to the base is safe because the base is always a directory
+	// the agent owns.
+	if len(elements) > 1 {
+		base := filepath.Clean(elements[0])
+		if base != "." && path != base && !strings.HasPrefix(path, base+sep) {
+			log.Warnf("refusing to build a path outside %q from elements %q; using base", base, elements)
+			path = base
+		}
+	}
+
+	if len(elements) != 0 && strings.HasSuffix(elements[len(elements)-1], sep) {
+		return path + sep
 	}
 	return path
 }

cldy/util_test.go

@@ -74,6 +74,48 @@ func TestSafePath(t *testing.T) {
 			},
 			want: "test/scratch/dirName/",
 		},
+
+		// The following escaped the previous implementation, which deleted every
+		// literal ".." substring before cleaning. "...." collapses to ".." once the
+		// inner ".." is removed, which then resolves as a traversal.
+		"traversal via doubled dots": {
+			elements: []string{
+				"test/scratch",
+				"....//....//etc/passwd",
+			},
+			want: "test/scratch/..../..../etc/passwd",
+		},
+		"traversal via dot-slash padding": {
+			elements: []string{
+				"test/scratch",
+				"..././..././etc/shadow",
+			},
+			want: "test/scratch/.../.../etc/shadow",
+		},
+		"traversal escaping the base is clamped": {
+			elements: []string{
+				"test/scratch",
+				"../../../../etc/passwd",
+			},
+			want: "test/scratch/etc/passwd",
+		},
+
+		// The previous implementation also mangled legitimate names containing
+		// consecutive dots, silently writing samples to the wrong directory.
+		"legitimate name with consecutive dots is preserved": {
+			elements: []string{
+				"test/scratch",
+				"v1..2-cluster",
+			},
+			want: "test/scratch/v1..2-cluster",
+		},
+		"legitimate filename with consecutive dots is preserved": {
+			elements: []string{
+				"test/scratch",
+				"my..cluster.tgz",
+			},
+			want: "test/scratch/my..cluster.tgz",
+		},
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {

cmd/bingen-to-json/main.go

@@ -9,6 +9,15 @@ import (
 	"github.com/opencost/opencost/core/pkg/model/kubemodel"
 )
 
+// Permissions for the generated output tree. This is a developer tool rather
+// than part of the agent runtime, but there is no reason for its output to be
+// group- or world-writable, and the directory mode was reported as
+// CWE-732 (insecure directory permissions).
+const (
+	outputDirPerm  = 0o750
+	outputFilePerm = 0o600
+)
+
 func kubeModelBinaryToJson(bytes []byte) ([]byte, error) {
 	kms := new(kubemodel.KubeModelSet)
 	err := kms.UnmarshalBinary(bytes)
@@ -39,7 +48,7 @@ func decodeKubeModel(srcPath, outPath string) {
 		}
 
 		if d.IsDir() {
-			err := os.MkdirAll(filepath.Join(outPath, path), 0755)
+			err := os.MkdirAll(filepath.Join(outPath, path), outputDirPerm)
 			if err != nil {
 				return err
 			}
@@ -61,7 +70,7 @@ func decodeKubeModel(srcPath, outPath string) {
 		}
 
 		outFilePath := filepath.Join(outPath, srcPath, fileName+".json")
-		err = os.WriteFile(outFilePath, jsonData, 0644)
+		err = os.WriteFile(outFilePath, jsonData, outputFilePerm)
 		if err != nil {
 			fmt.Printf("[kubemodel] error writing JSON file %s: %v\n", outFilePath, err)
 			return nil