fix: include query params in HTTP getter cache key (#2399)

* fix: include query params in HTTP getter cache key (#2103) When helmfile caches remote HTTP files fetched via the "normal" getter (plain https:// URLs without a git:: prefix), the cache key did not include query parameters. This caused URLs that differ only in query params (e.g. ?ref=commit1 vs ?ref=commit2) to share the same cache directory, silently returning the wrong file version. The root cause was in Fetch() where the "normal" getter branch overwrote the cache key with only scheme + host, discarding query params that were correctly computed earlier. Fix: extract the query-params suffix into a reusable variable and apply it in both the default and "normal" getter cache key paths. Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * Update pkg/remote/remote.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> * Update pkg/remote/remote_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> --------- Signed-off-by: Aditya Menon <amenon@canarytechnologies.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2026-10-19 10:19:05 +02:00 · 2026-02-14 09:31:53 +08:00
parent 5c43fa6465
commit c6b962dbbf
6 changed files with 303 additions and 13 deletions
@@ -2,6 +2,8 @@ package remote

 import (
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"errors"
 	"fmt"
 	"io"
@@ -235,16 +237,36 @@ func (r *Remote) Fetch(path string, cacheDirOpt ...string) (string, error) {
 	should_cache := query.Get("cache") != "false"
 	delete(query, "cache")

+	// Precompute a redacted copy of the query for cache keys and debug logs.
+	// Sensitive query parameters are hashed so that different credentials produce
+	// distinct cache keys without writing the raw secret to disk or log output.
+	// Note: go-getter may still log the full URL internally.
+	var paramsSuffix string
+	var redactedQuery neturl.Values
+	if len(query) > 0 {
+		redactedQuery = maps.Clone(query)
+		for key := range redactedQuery {
+			lk := strings.ToLower(key)
+			if strings.Contains(lk, "token") || strings.Contains(lk, "password") ||
+				strings.Contains(lk, "secret") || strings.Contains(lk, "key") ||
+				strings.Contains(lk, "signature") {
+				orig := redactedQuery[key]
+				hashed := make([]string, len(orig))
+				for i, v := range orig {
+					h := sha256.Sum256([]byte(v))
+					hashed[i] = hex.EncodeToString(h[:])[:8]
+				}
+				redactedQuery[key] = hashed
+			}
+		}
+		paramsSuffix = strings.ReplaceAll(redactedQuery.Encode(), "&", "_")
+	}
+
 	var cacheKey string
 	replacer := strings.NewReplacer(":", "", "//", "_", "/", "_", ".", "_")
 	dirKey := replacer.Replace(srcDir)
-	if len(query) > 0 {
-		q := maps.Clone(query)
-		if q.Has("sshkey") {
-			q.Set("sshkey", "redacted")
-		}
-		paramsKey := strings.ReplaceAll(q.Encode(), "&", "_")
-		cacheKey = fmt.Sprintf("%s.%s", dirKey, paramsKey)
+	if paramsSuffix != "" {
+		cacheKey = fmt.Sprintf("%s.%s", dirKey, paramsSuffix)
 	} else {
 		cacheKey = dirKey
 	}
@@ -258,7 +280,13 @@ func (r *Remote) Fetch(path string, cacheDirOpt ...string) (string, error) {
 	cacheDirPath := filepath.Join(r.Home, getterDst)
 	if u.Getter == "normal" {
 		srcDir = fmt.Sprintf("%s://%s", u.Scheme, u.Host)
-		cacheKey = replacer.Replace(srcDir)
+		normalDirKey := replacer.Replace(srcDir)
+		if paramsSuffix != "" {
+			cacheKey = fmt.Sprintf("%s.%s", normalDirKey, paramsSuffix)
+		} else {
+			cacheKey = normalDirKey
+		}
+		getterDst = filepath.Join(cacheBaseDir, cacheKey)
 		cacheDirPath = filepath.Join(r.Home, cacheKey, u.Dir)
 	}

@@ -268,7 +296,7 @@ func (r *Remote) Fetch(path string, cacheDirOpt ...string) (string, error) {

 	{
 		if r.fs.FileExistsAt(cacheDirPath) {
-			return "", fmt.Errorf("%s is not directory. please remove it so that variant could use it for dependency caching", getterDst)
+			return "", fmt.Errorf("%s is not a directory. Please remove it so that helmfile can use it for dependency caching", cacheDirPath)
 		}

 		if u.Getter == "normal" {
@@ -294,7 +322,11 @@ func (r *Remote) Fetch(path string, cacheDirOpt ...string) (string, error) {
 			getterSrc = strings.Join([]string{getterSrc, query.Encode()}, "?")
 		}

-		r.Logger.Debugf("remote> downloading %s to %s", getterSrc, getterDst)
+		logSrc := path
+		if redactedQuery != nil {
+			logSrc = strings.Join([]string{strings.SplitN(path, "?", 2)[0], redactedQuery.Encode()}, "?")
+		}
+		r.Logger.Debugf("remote> downloading %s to %s", logSrc, cacheDirPath)

 		switch {
 		case u.Getter == "normal" && u.Scheme == "s3":
@@ -157,8 +157,11 @@ func TestRemote_SShGitHub_WithSshKey(t *testing.T) {
 	cleanfs := map[string]string{
 		CacheDir(): "",
 	}
+	// Note: "0b44c081" is the first 8 characters of the SHA256 hash of the test SSH key.
+	// It is intentionally hardcoded here as part of the expected cache key format and replaces
+	// the previous "redacted" placeholder to reflect the actual hashing behavior.
 	cachefs := map[string]string{
-		filepath.Join(CacheDir(), "ssh_github_com_helmfile_helmfiles_git.ref=0.40.0_sshkey=redacted/releases/kiam.yaml"): "foo: bar",
+		filepath.Join(CacheDir(), "ssh_github_com_helmfile_helmfiles_git.ref=0.40.0_sshkey=0b44c081/releases/kiam.yaml"): "foo: bar",
 	}

 	testcases := []struct {
@@ -205,7 +208,7 @@ func TestRemote_SShGitHub_WithSshKey(t *testing.T) {
 				t.Fatalf("unexpected error: %v", err)
 			}

-			expectedFile := filepath.Join(CacheDir(), "ssh_github_com_helmfile_helmfiles_git.ref=0.40.0_sshkey=redacted/releases/kiam.yaml")
+			expectedFile := filepath.Join(CacheDir(), "ssh_github_com_helmfile_helmfiles_git.ref=0.40.0_sshkey=0b44c081/releases/kiam.yaml")
 			if file != expectedFile {
 				t.Errorf("unexpected file located: %s vs expected: %s", file, expectedFile)
 			}
@@ -497,6 +500,15 @@ func TestParse(t *testing.T) {
 			file:   "values.yaml",
 			query:  "",
 		},
+		{
+			name:   "https scheme normal with query params",
+			input:  "https://gitlab.example.com/api/v4/projects/test/repository/files/values.yaml/raw?ref=abc123",
+			getter: "normal",
+			scheme: "https",
+			dir:    "api/v4/projects/test/repository/files/values.yaml",
+			file:   "raw",
+			query:  "ref=abc123",
+		},
 	}

 	for _, tt := range testcases {
@@ -685,3 +697,110 @@ func TestAWSSDKLogLevelInit(t *testing.T) {
 		})
 	}
 }
+
+func TestRemote_HttpUrlWithQueryParams(t *testing.T) {
+	cacheDir := CacheDir()
+	cleanfs := map[string]string{
+		cacheDir: "",
+	}
+	cachefs := map[string]string{
+		filepath.Join(cacheDir, "https_gitlab_example_com.ref=abc123/api/v4/projects/test/repository/files/values.yaml", "raw"): "cached: content",
+	}
+
+	testcases := []struct {
+		name           string
+		files          map[string]string
+		expectCacheHit bool
+	}{
+		{name: "cache miss", files: cleanfs, expectCacheHit: false},
+		{name: "cache hit", files: cachefs, expectCacheHit: true},
+	}
+
+	for _, tt := range testcases {
+		t.Run(tt.name, func(t *testing.T) {
+			testfs := testhelper.NewTestFs(tt.files)
+
+			hit := true
+
+			get := func(wd, src, dst string) error {
+				hit = false
+				return nil
+			}
+
+			getter := &testGetter{get: get}
+			remote := &Remote{
+				Logger:     helmexec.NewLogger(io.Discard, "debug"),
+				Home:       cacheDir,
+				Getter:     getter,
+				S3Getter:   getter,
+				HttpGetter: getter,
+				fs:         testfs.ToFileSystem(),
+			}
+
+			url := "https://gitlab.example.com/api/v4/projects/test/repository/files/values.yaml/raw?ref=abc123"
+			file, err := remote.Fetch(url)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			expectedFile := filepath.Join(cacheDir, "https_gitlab_example_com.ref=abc123/api/v4/projects/test/repository/files/values.yaml", "raw")
+			if file != expectedFile {
+				t.Errorf("unexpected file located: %s vs expected: %s", file, expectedFile)
+			}
+
+			if tt.expectCacheHit && !hit {
+				t.Errorf("unexpected cache miss")
+			}
+			if !tt.expectCacheHit && hit {
+				t.Errorf("unexpected cache hit")
+			}
+		})
+	}
+}
+
+func TestRemote_HttpUrlQueryParamsAvoidCacheCollision(t *testing.T) {
+	cacheDir := CacheDir()
+	cleanfs := map[string]string{
+		cacheDir: "",
+	}
+	testfs := testhelper.NewTestFs(cleanfs)
+
+	get := func(wd, src, dst string) error {
+		return nil
+	}
+
+	getter := &testGetter{get: get}
+	remote := &Remote{
+		Logger:     helmexec.NewLogger(io.Discard, "debug"),
+		Home:       cacheDir,
+		Getter:     getter,
+		S3Getter:   getter,
+		HttpGetter: getter,
+		fs:         testfs.ToFileSystem(),
+	}
+
+	url1 := "https://gitlab.example.com/api/v4/projects/test/repository/files/values.yaml/raw?ref=29b5609"
+	url2 := "https://gitlab.example.com/api/v4/projects/test/repository/files/values.yaml/raw?ref=d80839c"
+
+	file1, err := remote.Fetch(url1)
+	if err != nil {
+		t.Fatalf("unexpected error fetching url1: %v", err)
+	}
+
+	file2, err := remote.Fetch(url2)
+	if err != nil {
+		t.Fatalf("unexpected error fetching url2: %v", err)
+	}
+
+	if file1 == file2 {
+		t.Errorf("expected different cache paths for different query params, but both resolved to: %s", file1)
+	}
+
+	// Verify both contain the ref in the path
+	if !strings.Contains(file1, "ref=29b5609") {
+		t.Errorf("expected file1 path to contain ref=29b5609, got: %s", file1)
+	}
+	if !strings.Contains(file2, "ref=d80839c") {
+		t.Errorf("expected file2 path to contain ref=d80839c, got: %s", file2)
+	}
+}