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

[edgarbellot]

Summary

Small cleanup around fetchFromGitlabIfNecessary so it lines up with the way --utilities-git-url is meant to be used:

• Match the configured GitLab host by exact hostname (case-insensitive) instead of a "git" prefix.
• Require https:// when reaching out to the configured host.
• Send the token via the standard PRIVATE-TOKEN header instead of appending it to the query string.
• Use a dedicated http.Client with redirects disabled and a request timeout, so a misbehaving upstream cannot stall the supervisor goroutine.
• Reject any non-2xx response explicitly.

Adds a unit test file for the function (none existed before), covering the host match, scheme requirement, header propagation, redirect behavior, and the no-token-configured path.

JIRA ticket: https://mattermost.atlassian.net/browse/MM-69024

Thank you to Samy Ghannad for letting us know about this issue.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR restricts GitLab Helm values fetching to configured hosts over HTTPS only. It introduces a shared HTTP client with timeout and redirect suppression, validates hostnames against the configured GitOps repository, constructs authenticated requests using the PRIVATE-TOKEN header, and treats non-2xx responses as errors. Tests verify token handling, hostname matching, scheme validation, and redirect rejection.

Changes

GitLab Helm Values Fetch Security

Layer / File(s)	Summary
HTTP Client Setup with Timeout and Redirect Suppression internal/provisioner/utility/helm_utils.go	Adds time import and shared gitlabFetchClient with 30-second timeout and CheckRedirect callback that prevents following redirects.
Hostname Validation and Authenticated Fetch Logic internal/provisioner/utility/helm_utils.go	Introduces isConfiguredGitlabHost helper for case-insensitive exact hostname matching; updates fetchFromGitlabIfNecessary to require HTTPS, validate against configured host, and build authenticated GET requests with PRIVATE-TOKEN header using the shared client.
Test Infrastructure and Helpers internal/provisioner/utility/helm_utils_test.go	Adds test file with withGitlabConfig helper to temporarily override GitLab token and repository URL, and withFetchClient helper with countingTransport to track HTTP calls and skip TLS verification.
Test Cases for Validation, Authentication, and Redirect Handling internal/provisioner/utility/helm_utils_test.go	Tests verify URLs remain unchanged when token or correct host are missing; HTTP scheme is rejected with HTTPS error for configured hosts; PRIVATE-TOKEN header is sent without leaking tokens; fetched content is decoded and persisted to temp files; and redirects are rejected without following targets.

---

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: tightening GitLab host matching logic and improving request handling for helm values fetching.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The pull request description clearly explains the security and reliability improvements to GitLab host matching and request handling, including specific changes like exact hostname matching, HTTPS requirement, token header handling, timeout configuration, and the addition of comprehensive unit tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/gitlab-values-fetch-host-matching

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/provisioner/utility/helm_utils.go (1)

346-358: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Close the file handle from os.CreateTemp before writing.

os.CreateTemp returns an open file handle that is never closed. While os.WriteFile works by path, the original descriptor from CreateTemp remains open, causing a file descriptor leak.

🐛 Proposed fix to close the temp file handle

 	temporaryValuesFile, err := os.CreateTemp(os.TempDir(), "helm-values-file-")
 	if err != nil {
 		return "", nil, errors.Wrap(err, "failed to create temporary file for Helm values file")
 	}
+	temporaryValuesFile.Close()

 	content, err := base64.StdEncoding.DecodeString(valuesFileBytes.Content)

Alternatively, write directly to the open file handle instead of using os.WriteFile:

 	temporaryValuesFile, err := os.CreateTemp(os.TempDir(), "helm-values-file-")
 	if err != nil {
 		return "", nil, errors.Wrap(err, "failed to create temporary file for Helm values file")
 	}
+	defer temporaryValuesFile.Close()

 	content, err := base64.StdEncoding.DecodeString(valuesFileBytes.Content)
 	if err != nil {
 		return "", nil, errors.Wrap(err, "failed to decode base64-encoded YAML file")
 	}

-	err = os.WriteFile(temporaryValuesFile.Name(), content, 0600)
+	_, err = temporaryValuesFile.Write(content)
 	if err != nil {
 		return "", nil, errors.Wrap(err, "failed to write values file to disk for Helm to read")
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/provisioner/utility/helm_utils.go` around lines 346 - 358, The
temporary file handle returned by os.CreateTemp (variable temporaryValuesFile)
is never closed, leaking a file descriptor; either close temporaryValuesFile
before calling os.WriteFile(temporaryValuesFile.Name(), ...) or, better, write
to the open file handle (temporaryValuesFile.Write or io.WriteString) and then
close it (temporaryValuesFile.Close())—apply this change in the function that
creates the temp file and writes the Helm values so the descriptor is always
closed even on error (use defer temporaryValuesFile.Close() immediately after
successful CreateTemp).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@internal/provisioner/utility/helm_utils.go`:
- Around line 346-358: The temporary file handle returned by os.CreateTemp
(variable temporaryValuesFile) is never closed, leaking a file descriptor;
either close temporaryValuesFile before calling
os.WriteFile(temporaryValuesFile.Name(), ...) or, better, write to the open file
handle (temporaryValuesFile.Write or io.WriteString) and then close it
(temporaryValuesFile.Close())—apply this change in the function that creates the
temp file and writes the Helm values so the descriptor is always closed even on
error (use defer temporaryValuesFile.Close() immediately after successful
CreateTemp).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a62c190b-e25d-4274-941f-51f8eaabdb56

📥 Commits

Reviewing files that changed from the base of the PR and between 06adb2a and bb0d0f3.

📒 Files selected for processing (2)

• internal/provisioner/utility/helm_utils.go
• internal/provisioner/utility/helm_utils_test.go