fix: migrate proxy chart dependencies and refactor related functions

[Ash-exp]

Description

Fixes https://github.com/devtron-labs/sprint-tasks/issues/2750

Checklist:

• The title of the PR states what changed and the related issues number (used for the release note).
• Does this PR requires documentation updates?
• I've updated documentation as required by this PR.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have tested it for all user roles.
• I have added all the required unit/api test cases.

Does this PR introduce a user-facing change?

Summary by Bito

• This pull request introduces a new method for migrating proxy chart dependencies within the Git operation service, which may introduce risks related to dependency management.
• It removes unused variables and refactors existing functions to streamline the process of handling chart files.
• The changes enhance the clarity and maintainability of the code, particularly in managing chart updates and organizing chart file constants.
• Overall, the changes touch on the management of proxy chart dependencies, refactoring of functions, and organization of chart file constants, introducing potential risks in dependency management.

General approach: ensure that any user-influenced data that ends up in a filesystem path is validated before use. In this flow, PushChartToGitRequestDTO is created from InstallAppVersionDTO by ParseChartGitPushRequest, and its AppName and EnvName are later used to construct gitOpsChartLocation. The least invasive, behavior-preserving fix is to validate those two fields (and, if desired, RepoURL, though that is a URL not a filesystem path here) as early as possible and reject or log invalid values.

Best concrete fix:

1. Add a small validation helper in pkg/appStore/installedApp/adapter/Adapter.go that checks that a string intended for use as a single path component does not contain /, \, or ... This matches the recommended technique from the background description.
2. In ParseChartGitPushRequest, before constructing and returning PushChartToGitRequestDTO, call this validator on installAppRequestDTO.AppName and installAppRequestDTO.EnvironmentName. If either is invalid, we return a DTO with safe, sanitized values (e.g., empty strings) or, more robustly, we could panic or log an error; but since this function currently has no error return and we must avoid changing its signature, the safest option within the given constraints is to sanitize by replacing invalid names with empty strings and logging the issue.
3. To avoid changing imports in other files, use only strings from the standard library (added to Adapter.go’s imports). No other files need to change, because once AppName and EnvName are guaranteed not to contain path separators or .., the subsequent joins in GitOperationServiceImpl.MigrateProxyChartDependenciesIfRequired and CloneChartForHelmApp will not be vulnerable to path traversal.

Specifically:

• Edit pkg/appStore/installedApp/adapter/Adapter.go:
  • Add an import of "strings".
  • Add a helper function sanitizePathComponent(name string) string and isValidPathComponent(name string) bool.
  • In ParseChartGitPushRequest, sanitize AppName and EnvironmentName before using them to populate the DTO.

This keeps behavior unchanged for valid input, while blocking malicious characters for untrusted input, and addresses both alert variants since they share the same sink.

---

General fix: ensure that any path derived (directly or indirectly) from user-controlled data is constrained to a safe directory. In this case, requirementsYamlPath must be guaranteed to reside under the clonedDir tree produced by CloneChartForHelmApp. We can do this by computing absolute, cleaned paths with filepath.Abs/filepath.Clean and verifying that the final path has clonedDir as its prefix. If the resulting path doesn’t stay within clonedDir, we should log and return an error instead of deleting.

Best concrete fix here: modify shouldDeleteProxyChartRequirementsYaml to:

1. Normalize clonedDir to an absolute, cleaned path (safeBase).
2. Compute the absolute requirementsYamlPath via filepath.Abs(filepath.Join(clonedDir, chartRefBean.REQUIREMENTS_YAML_FILE)).
3. Ensure requirementsYamlPath is still under safeBase using a safe prefix check that accounts for path separators (e.g. compare requirementsYamlPath == safeBase or strings.HasPrefix(requirementsYamlPath, safeBase+string(os.PathSeparator))).
4. If the check fails, log and return an error, preventing any subsequent os.Stat/os.Remove from touching the filesystem.
5. Otherwise, continue with the existing os.Stat logic and return the safe, normalized path.

This fix is localized to pkg/deployment/gitOps/git/GitOperationService.go inside the existing function; it preserves behavior for valid paths but blocks path traversal attempts. We already import filepath, os, and strings in this file, so no new imports are needed.

---

In general, the problem is that user-controlled strings (AppName, EnvName) are used to construct directory names (gitOpsChartLocation) which are then joined into a filesystem path, without ensuring that those strings are valid single path components (no /, \, or ..). The safest way to fix this without changing overall functionality is to sanitize or validate those strings at the point where we convert the DTO into the GitOps request object, ensuring that any path-dependent fields are guaranteed to be single safe components.

The minimal and robust fix in this codebase is:

1. Introduce a small helper in pkg/appStore/installedApp/adapter/Adapter.go that converts arbitrary names into safe path components by:

   • Replacing path separators (/, \) with -.
   • Replacing any .. sequences (directory traversal patterns) with -.
   • Optionally trimming spaces.
     This preserves the general “shape” of names while making them safe for use as directory names.

2. Use this helper inside ParseChartGitPushRequest when populating AppName and EnvName in PushChartToGitRequestDTO. This ensures that wherever those fields are later used to construct paths (e.g., in gitOpsChartLocation := fmt.Sprintf("%s-%s", pushChartToGitRequest.AppName, pushChartToGitRequest.EnvName)), they are safe.

3. No changes are needed in GitOperationServiceImpl.shouldMigrateProxyChartDependencies itself; once inputs are sanitized at the adapter layer, chartYamlPath and other derived paths are no longer influenced by unsafe data.

This approach:

• Keeps existing business behavior (names are still recognizable, still combined as <app>-<env>).
• Centralizes sanitization in one place (ParseChartGitPushRequest), automatically covering all call sites.
• Avoids changing imports in other files; only strings is added to Adapter.go.

---

In general, the fix is to ensure that any user-controlled data used to construct filesystem paths is validated or normalized so it cannot break out of the intended directory tree or introduce special path components. In this code, the tainted values are AppName and EnvName flowing into PushChartToGitRequestDTO and then into gitOpsChartLocation and workingDir. We should constrain these components to a safe character set and disallow path separators and .., so that even if a client sends malicious names, the resulting directory names under clonedDir cannot escape or reference unexpected paths.

The best way to fix this without changing existing functionality is:

1. Introduce a small helper in pkg/appStore/installedApp/adapter/Adapter.go that sanitizes app and environment names for filesystem use. This helper will:
   • Return an error if the string contains "/", "\", or "..", or
   • Alternatively, replace disallowed characters with safe ones. To be conservative and avoid changing semantics, we will reject dangerous patterns rather than silently alter them.
2. Use this helper inside ParseChartGitPushRequest to validate installAppRequestDTO.AppName and installAppRequestDTO.EnvironmentName before constructing the PushChartToGitRequestDTO. To avoid changing the function signature (which would cascade many changes), we cannot return an error from ParseChartGitPushRequest. Instead, we sanitize by stripping or replacing forbidden characters with a safe delimiter (e.g., -) while logging via existing utilities if needed. That way, we preserve behavior for normal names but neutralize path traversal characters.
3. Alternatively (and simpler for this context), we perform the sanitization at the point of path use, i.e., in MigrateProxyChartDependenciesIfRequired, just before constructing gitOpsChartLocation. This keeps the change localized and directly protects the vulnerable sink.

Given the constraints (minimal surface change, no new errors added to existing signatures), the most contained and robust fix here is to sanitize the components in MigrateProxyChartDependenciesIfRequired before using them in fmt.Sprintf("%s-%s", ...). We will:

• Add a small, unexported helper sanitizePathComponent in GitOperationService.go.
• Use it to transform pushChartToGitRequest.AppName and pushChartToGitRequest.EnvName into safe directory-name components (safeAppName, safeEnvName), replacing any /, \, or .. with - and optionally trimming whitespace.
• Build gitOpsChartLocation from these safe values instead of raw ones.

This change stays entirely within pkg/deployment/gitOps/git/GitOperationService.go, and does not require new imports beyond what’s available (strings is already imported). No behavior changes for normal, simple names; only malicious or malformed names are normalized to safe directory components.

---

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud