fix: tighten GitLab host matching and request handling in helm values fetch

internal/provisioner/utility/helm_utils.go

@@ -13,13 +13,24 @@ import (
 	"net/url"
 	"os"
 	"strings"
+	"time"
 
 	"github.com/mattermost/mattermost-cloud/internal/tools/helm"
 	"github.com/mattermost/mattermost-cloud/model"
 	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 )
 
+// gitlabFetchClient is the HTTP client used to download Helm values files
+// from the configured GitLab instance. Redirects are not followed and a
+// timeout is applied to keep the supervisor goroutine responsive.
+var gitlabFetchClient = &http.Client{
+	Timeout: 30 * time.Second,
+	CheckRedirect: func(req *http.Request, via []*http.Request) error {
+		return http.ErrUseLastResponse
+	},
+}
+
 const (
 	defaultKubeConfigPath            = ""
 	defaultHelmDeploymentSetArgument = ""
@@ -264,13 +275,28 @@ type gitlabValuesFileResponse struct {
 	Content string `json:"content"`
 }
 
+// isConfiguredGitlabHost reports whether valPathURL points at the GitLab
+// instance configured via --utilities-git-url. The hostname comparison is
+// exact and case-insensitive.
+func isConfiguredGitlabHost(valPathURL *url.URL) bool {
+	configured := model.GetGitopsRepoURL()
+	if configured == "" {
+		return false
+	}
+	configuredURL, err := url.Parse(configured)
+	if err != nil || configuredURL.Hostname() == "" {
+		return false
+	}
+	return strings.EqualFold(valPathURL.Hostname(), configuredURL.Hostname())
+}
+
 // fetchFromGitlabIfNecessary returns the path of the values file. If
 // this is a local path or a non-Gitlab URL, the path is simply
-// returned unchanged. If a Gitlab URL is provided, the values file is
-// fetched and stored in the OS's temp dir and the filename of the
-// file is returned. If a temp file is created, a cleanup routine will
-// be returned as the second return value, otherwise that value will
-// be nil
+// returned unchanged. If the configured Gitlab host is provided over
+// HTTPS, the values file is fetched and stored in the OS's temp dir and
+// the filename of the file is returned. If a temp file is created, a
+// cleanup routine will be returned as the second return value,
+// otherwise that value will be nil
 func fetchFromGitlabIfNecessary(path string) (string, func(string), error) {
 	gitlabKey := model.GetGitlabToken()
 	if gitlabKey == "" {
@@ -283,18 +309,26 @@ func fetchFromGitlabIfNecessary(path string) (string, func(string), error) {
 	}
 
 	// silently allow other public non-Gitlab URLs
-	if !strings.HasPrefix(valPathURL.Host, "git") {
+	if !isConfiguredGitlabHost(valPathURL) {
 		return path, nil, nil
 	}
 
-	// if Gitlab, fetch the file using the API
-	path = fmt.Sprintf("%s&private_token=%s", path, gitlabKey)
+	if !strings.EqualFold(valPathURL.Scheme, "https") {
+		return "", nil, errors.New("Gitlab values file URL must use HTTPS")
+	}
+
+	req, err := http.NewRequest(http.MethodGet, path, nil)
+	if err != nil {
+		return "", nil, errors.Wrap(err, "failed to build Gitlab request")
+	}
+	req.Header.Set("PRIVATE-TOKEN", gitlabKey)
 
-	resp, err := http.Get(path)
+	resp, err := gitlabFetchClient.Do(req)
 	if err != nil {
 		return "", nil, errors.Wrap(err, "failed to request the values file from Gitlab")
 	}
-	if resp.StatusCode >= 400 {
+	defer resp.Body.Close()
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
 		return "", nil, errors.Errorf("request to Gitlab failed with status: %s", resp.Status)
 	}
 

internal/provisioner/utility/helm_utils_test.go

@@ -0,0 +1,197 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+//
+
+package utility
+
+import (
+	"crypto/tls"
+	"encoding/base64"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"os"
+	"strings"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/mattermost/mattermost-cloud/model"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// withGitlabConfig temporarily sets the package-level GitLab token and
+// gitops repo URL used by fetchFromGitlabIfNecessary, restoring previous
+// values on cleanup.
+func withGitlabConfig(t *testing.T, token, repoURL string) {
+	t.Helper()
+	prevToken := model.GetGitlabToken()
+	prevURL := model.GetGitopsRepoURL()
+	model.SetGitlabToken(token)
+	model.SetGitopsRepoURL(repoURL)
+	t.Cleanup(func() {
+		model.SetGitlabToken(prevToken)
+		model.SetGitopsRepoURL(prevURL)
+	})
+}
+
+// withFetchClient swaps the package-level gitlabFetchClient for a test
+// client that trusts httptest's TLS server and counts outbound calls.
+func withFetchClient(t *testing.T) *atomic.Int64 {
+	t.Helper()
+	prev := gitlabFetchClient
+	var hits atomic.Int64
+	gitlabFetchClient = &http.Client{
+		Timeout: 5 * time.Second,
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			return http.ErrUseLastResponse
+		},
+		Transport: &countingTransport{
+			counter: &hits,
+			next: &http.Transport{
+				TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+			},
+		},
+	}
+	t.Cleanup(func() { gitlabFetchClient = prev })
+	return &hits
+}
+
+type countingTransport struct {
+	counter *atomic.Int64
+	next    http.RoundTripper
+}
+
+func (c *countingTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+	c.counter.Add(1)
+	return c.next.RoundTrip(r)
+}
+
+func TestFetchFromGitlabIfNecessary_NoTokenReturnsPathUnchanged(t *testing.T) {
+	withGitlabConfig(t, "", "https://gitlab.example.com")
+	hits := withFetchClient(t)
+
+	in := "https://gitlab.example.com/api/v4/projects/1/repository/files/values.yaml?ref=main"
+	out, cleanup, err := fetchFromGitlabIfNecessary(in)
+	require.NoError(t, err)
+	assert.Equal(t, in, out)
+	assert.Nil(t, cleanup)
+	assert.Zero(t, hits.Load())
+}
+
+func TestFetchFromGitlabIfNecessary_NonConfiguredHostReturnsPathUnchanged(t *testing.T) {
+	withGitlabConfig(t, "secret-token", "https://gitlab.example.com")
+	hits := withFetchClient(t)
+
+	in := "https://example.org/values.yaml"
+	out, cleanup, err := fetchFromGitlabIfNecessary(in)
+	require.NoError(t, err)
+	assert.Equal(t, in, out)
+	assert.Nil(t, cleanup)
+	assert.Zero(t, hits.Load())
+}
+
+// TestFetchFromGitlabIfNecessary_HostMatchIsExact covers a range of URL
+// shapes whose host is not equal to the configured GitLab host. All of
+// them must be treated as non-GitLab URLs and returned unchanged.
+func TestFetchFromGitlabIfNecessary_HostMatchIsExact(t *testing.T) {
+	withGitlabConfig(t, "secret-token", "https://gitlab.example.com")
+	hits := withFetchClient(t)
+
+	nonConfiguredURLs := []string{
+		"https://git.example.com/values.yaml?ref=main",
+		"https://github.com/x?a=1",
+		"https://gitea.example.org/y?z=1",
+		"https://gitlab.example.com.other.org/values.yaml?ref=main",
+		"https://other.org/gitlab.example.com/values.yaml",
+		"https://gitlab.example.com@other.org/values.yaml",
+	}
+
+	for _, u := range nonConfiguredURLs {
+		out, cleanup, err := fetchFromGitlabIfNecessary(u)
+		require.NoError(t, err, "url=%s", u)
+		assert.Equal(t, u, out, "url=%s should be returned unchanged", u)
+		assert.Nil(t, cleanup, "url=%s should not produce a cleanup", u)
+	}
+	assert.Zero(t, hits.Load())
+}
+
+func TestFetchFromGitlabIfNecessary_RejectsHTTPForConfiguredHost(t *testing.T) {
+	withGitlabConfig(t, "secret-token", "https://gitlab.example.com")
+	hits := withFetchClient(t)
+
+	in := "http://gitlab.example.com/api/v4/projects/1/repository/files/values.yaml?ref=main"
+	_, _, err := fetchFromGitlabIfNecessary(in)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "HTTPS")
+	assert.Zero(t, hits.Load())
+}
+
+func TestFetchFromGitlabIfNecessary_SendsTokenViaPrivateTokenHeader(t *testing.T) {
+	var (
+		seenPrivateTokenHeader string
+		seenRawQuery           string
+	)
+
+	srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		seenPrivateTokenHeader = r.Header.Get("PRIVATE-TOKEN")
+		seenRawQuery = r.URL.RawQuery
+		body, _ := json.Marshal(gitlabValuesFileResponse{
+			Content: base64.StdEncoding.EncodeToString([]byte("key: value\n")),
+		})
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write(body)
+	}))
+	t.Cleanup(srv.Close)
+
+	srvURL, err := url.Parse(srv.URL)
+	require.NoError(t, err)
+
+	withGitlabConfig(t, "secret-token", "https://"+srvURL.Host)
+	_ = withFetchClient(t)
+
+	in := "https://" + srvURL.Host + "/api/v4/projects/1/repository/files/values.yaml?ref=main"
+	out, cleanup, err := fetchFromGitlabIfNecessary(in)
+	require.NoError(t, err)
+	require.NotNil(t, cleanup)
+	t.Cleanup(func() { cleanup(out) })
+
+	assert.Equal(t, "secret-token", seenPrivateTokenHeader)
+	assert.NotContains(t, seenRawQuery, "private_token")
+	assert.NotContains(t, seenRawQuery, "secret-token")
+
+	assert.True(t, strings.HasPrefix(out, os.TempDir()))
+	content, err := os.ReadFile(out)
+	require.NoError(t, err)
+	assert.Equal(t, "key: value\n", string(content))
+}
+
+// TestFetchFromGitlabIfNecessary_DoesNotFollowRedirects ensures the
+// fetch client surfaces a redirect response as an error rather than
+// silently following it.
+func TestFetchFromGitlabIfNecessary_DoesNotFollowRedirects(t *testing.T) {
+	var downstreamHit atomic.Bool
+	downstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		downstreamHit.Store(true)
+		w.WriteHeader(http.StatusOK)
+	}))
+	t.Cleanup(downstream.Close)
+
+	gitlab := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, downstream.URL+"/v", http.StatusFound)
+	}))
+	t.Cleanup(gitlab.Close)
+
+	gitlabURL, err := url.Parse(gitlab.URL)
+	require.NoError(t, err)
+
+	withGitlabConfig(t, "secret-token", "https://"+gitlabURL.Host)
+	_ = withFetchClient(t)
+
+	in := "https://" + gitlabURL.Host + "/api/v4/projects/1/repository/files/values.yaml?ref=main"
+	_, _, err = fetchFromGitlabIfNecessary(in)
+	require.Error(t, err)
+	assert.False(t, downstreamHit.Load())
+}