- 
                Notifications
    You must be signed in to change notification settings 
- Fork 554
sync: Release candidate v0.42.0 #6805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* refactor: update chart handling and dependency management in GitOps operations * refactor: improve chart metadata handling and error logging in GitOps operations * refactor: add support for setting APIVersion in chart metadata * refactor: convert requirements.yaml to JSON before unmarshalling in GitOps operations * refactor: convert chart.yaml to JSON before unmarshalling in GitOps operations * refactor: enhance dependency validation by using unique keys for comparison in GitOps operations * refactor: convert chart metadata from JSON to YAML format for GitOps operations * refactor: update shouldMigrateProxyChartDependencies to accept expected chart metadata and remove requirements.yaml checks * docs: add developer guide for creating API specs and generating Go beans * wip --------- Co-authored-by: Ash-exp <[email protected]> Co-authored-by: ayushmaheshwari <[email protected]> Co-authored-by: iamayushm <[email protected]>
) * corrected api specs * specs enhance * Merge pull request #6771 from devtron-labs/fix-cron-job-suspend fix: cron job suspend * fix: update default sequence for bulk edit config table and correct README link * corrected specs * corrected specs * Improve CI status fetching logic in `CiHandlerImpl` to handle linked pipelines, add duplicate removal utility, and enhance logging and fallback mechanisms. * Refactor `CiHandlerImpl` to modularize and streamline CI pipeline status fetching, extracting common logic into reusable functions and improving linked pipeline handling. * Remove redundant comments in `CiHandler.go` to enhance code clarity and maintain consistency with refactored CI status handling logic. * Clean up redundant comments in `preparePipelineStatusLookup` to enhance readability and align with refactored CI pipeline handling logic. * Remove unused `mapWorkflowsToLinkedPipelines` function from `CiHandler.go` to enhance code clarity and eliminate dead code. * Extract `RemoveDuplicateInts` utility to `CiCdUtil.go` for reusability and replace the inlined method in `CiHandler.go`. * vendor for spec-validator * spec-fixes as per cursor when ran on results file * spec-fixes manually for /orchestrator path * script fix to run against server * naming changed from devtron-lab duplicated removed ap spec correction * removed extra code * wip: adding cluster status * fix cluster status * Enhance CI status fetching in `CiHandlerImpl` to handle linked pipelines. Add linked pipeline mapping and workflow statuses, refactor status population logic, and introduce reusable adapter methods for linked CI workflows. * framework.go update * api-spec fix api-spec fix api-spec fix api-spec fix API Error wrapper API Error wrapper specs fixes * spec fix * script fix * Update workflow status table scripts to latest version. * Update `authenticator` and `common-lib` dependencies to latest versions. * added sql ent files (#6780) * devops specs included * script fix * script fix * migration number chnage (#6783) * feat: add resource recommendation APIs and update openapi.yaml * feat: add Resource Recommendation to openapi.yaml * specs fix * redocly script to generate html * refact * refact * refact * doc: api spec merger * chore: test commit * misc: script name rectified in action file * misc: testing change in /spec * misc: test commit * misc: test commit * refact * refact * refact * refact * feat: action fix and resource recommender specs * fix: action fixes * chore: test trigger commit * chore: test run commit * chore: test commit * chore: test commit * feat: git push job in action file * feat: git push job in action file * chore: test commit * chore: test commit * chore: test commit * chore: test commit * fix panic * chore: test commit * chore: test commit * chore: test commit * fix: script misc chore fixes and trigger commit * fix: script fixes * fix: script fixes * fix: script fixes * fix: script fixes * fix: script fixes * fix: script fixes * fix: script fixes * create API token validation * misc: added support for cronjob annotations and probes (#6787) * added support for cronjob annotations and probes * support for older charts * reconfiguring semverCompare * removed restartPolicy from cronjob-config * added support for external-secrets.io/v1 * added default values for eso * older compatibility * rectify the semverCompare * added support for jobLabels and jobAnnotations * refact * refact * security policy specs fixed * refact * refact * refact * refact * refact * refact * ent separation * ent separation * feat: automate API specs workflow and documentation (#6786) * feat: api spec hosting mechanism * fix: trigger condition update on wf file * fix: remove old wf * refact in specs * generate-api-docs.sh fix --------- Co-authored-by: prakhar katiyar <[email protected]> Co-authored-by: prakhar katiyar <[email protected]> * driftSpec.yaml added * fix: API token generation api responses refactoring (#6788) * fix: API token generation api responses refactoring * fix: register custom validation against tag for api token name validations * fix: register custom validation against tag for api token name validations * Revert "fix: register custom validation against tag for api token name validations" This reverts commit 7593c27. * fix: remove `required` validation from Description and expiryAtInMs * fix: adding resource conflict api response in WriteJsonResp utility * fix: path params int validation updated to whole numbers only * fix: handled resource not found response for update and delete api, token * path corrected * cmscs added * corrected specs * fix: api responses (#6789) * fix: API token generation api responses refactoring * fix: register custom validation against tag for api token name validations * fix: register custom validation against tag for api token name validations * Revert "fix: register custom validation against tag for api token name validations" This reverts commit 7593c27. * fix: remove `required` validation from Description and expiryAtInMs * fix: adding resource conflict api response in WriteJsonResp utility * fix: path params int validation updated to whole numbers only * fix: handled resource not found response for update and delete api, token * fix: validation for SSO config name field * fix: enhanced query param validation for commit metadata for pipeline material * fix: disable updating clsutername and api name in update clsuter api * fix: enhanced api response in query param validation failure reeors in ge default deployment template * fix: disable modifying cluster nae and env name in update env api * fix: resolving review comments * fix: resolving review comments --------- Co-authored-by: iamayushm <[email protected]> Co-authored-by: Ash-exp <[email protected]> Co-authored-by: Prakash Kumar <[email protected]> Co-authored-by: ayushmaheshwari <[email protected]> Co-authored-by: Vikram <[email protected]> Co-authored-by: Shivam Nagar <[email protected]> Co-authored-by: SATYAsasini <[email protected]> Co-authored-by: Badal Kumar <[email protected]> Co-authored-by: satya_prakash <[email protected]>
# Conflicts: # specs/application/labels.yaml # specs/authentication/authentication.yaml # specs/bulk/bulk-actions.yaml # specs/bulk/bulk-operations.yaml # specs/cluster/capacity.yaml # specs/ent-only/ClusterResourceRecommendation.yaml # specs/ent-only/infrastructure_spec.yaml # specs/ent-only/panels_api-spec.yaml # specs/environment/config-diff.yaml # specs/environment/team-management.yaml # specs/helm/charts.yaml # specs/notifications/webhooks.yaml # specs/template/config-maps.yaml # specs/template/deployment-chart-type.yaml # specs/workflow/workflow.yaml
* Merge pull request #6771 from devtron-labs/fix-cron-job-suspend fix: cron job suspend * fix: update default sequence for bulk edit config table and correct README link * Improve CI status fetching logic in `CiHandlerImpl` to handle linked pipelines, add duplicate removal utility, and enhance logging and fallback mechanisms. * Refactor `CiHandlerImpl` to modularize and streamline CI pipeline status fetching, extracting common logic into reusable functions and improving linked pipeline handling. * Remove redundant comments in `CiHandler.go` to enhance code clarity and maintain consistency with refactored CI status handling logic. * Clean up redundant comments in `preparePipelineStatusLookup` to enhance readability and align with refactored CI pipeline handling logic. * Remove unused `mapWorkflowsToLinkedPipelines` function from `CiHandler.go` to enhance code clarity and eliminate dead code. * Extract `RemoveDuplicateInts` utility to `CiCdUtil.go` for reusability and replace the inlined method in `CiHandler.go`. * wip: adding cluster status * fix cluster status * Enhance CI status fetching in `CiHandlerImpl` to handle linked pipelines. Add linked pipeline mapping and workflow statuses, refactor status population logic, and introduce reusable adapter methods for linked CI workflows. * Update workflow status table scripts to latest version. * Update `authenticator` and `common-lib` dependencies to latest versions. * added sql ent files (#6780) * migration number chnage (#6783) * feat: add resource recommendation APIs and update openapi.yaml * feat: add Resource Recommendation to openapi.yaml * fix panic * misc: added support for cronjob annotations and probes (#6787) * added support for cronjob annotations and probes * support for older charts * reconfiguring semverCompare * removed restartPolicy from cronjob-config * added support for external-secrets.io/v1 * added default values for eso * older compatibility * rectify the semverCompare * added support for jobLabels and jobAnnotations * feat: automate API specs workflow and documentation (#6786) * feat: api spec hosting mechanism * fix: trigger condition update on wf file * fix: remove old wf * refact in specs * generate-api-docs.sh fix --------- Co-authored-by: prakhar katiyar <[email protected]> Co-authored-by: prakhar katiyar <[email protected]> --------- Co-authored-by: iamayushm <[email protected]> Co-authored-by: Ash-exp <[email protected]> Co-authored-by: Prakash Kumar <[email protected]> Co-authored-by: ayushmaheshwari <[email protected]> Co-authored-by: Vikram <[email protected]> Co-authored-by: Shivam Nagar <[email protected]> Co-authored-by: Badal Kumar <[email protected]> Co-authored-by: satya_prakash <[email protected]>
misc: Dev sync
sync: Main sync rc42
| Bito Automatic Review Skipped - Large PR  | 
| 
 | 
| handler.logger.Errorw("Failed to fetch role group by ID", "roleGroupId", id, "err", err) | ||
| common.WriteJsonRespWithResourceContextFromId(w, err, nil, 0, "role group", id) | ||
| return | ||
| } | 
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
strconv.Atoi
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 2 months ago
To prevent incorrect values from being passed via this conversion, add explicit upper and lower bounds checks on the parsed integer value before converting from int (from strconv.Atoi) to int32. This should be done in the handler (in FetchRoleGroupById) before the call to int32(id). If the value does not fit in the acceptable range (0 <= id <= math.MaxInt32), return a validation error to the user. You'll need to import the math package to get the value of math.MaxInt32. This minimal check ensures conversion is always sound and prevents both negative and oversized values.
- 
    
    
    Copy modified line R29 
- 
    
    
    Copy modified lines R436-R442 
| @@ -26,7 +26,7 @@ | ||
| "net/http" | ||
| "strconv" | ||
| "strings" | ||
|  | ||
| "math" | ||
| "github.com/devtron-labs/devtron/api/restHandler/common" | ||
| "github.com/devtron-labs/devtron/internal/util" | ||
| "github.com/devtron-labs/devtron/pkg/auth/authorisation/casbin" | ||
| @@ -433,6 +433,13 @@ | ||
| return | ||
| } | ||
|  | ||
| // Upper bound check for int32 compatibility | ||
| if id < 0 || id > math.MaxInt32 { | ||
| err := util.NewValidationErrorForField("id", "must be a positive integer within int32 range") | ||
| common.WriteJsonResp(w, err, nil, http.StatusBadRequest) | ||
| return | ||
| } | ||
|  | ||
| res, err := handler.roleGroupService.FetchRoleGroupsById(int32(id)) | ||
| if err != nil { | ||
| handler.logger.Errorw("Failed to fetch role group by ID", "roleGroupId", id, "err", err) | 
| func NewResourceNotFoundError(resourceType, resourceId string) *ApiError { | ||
| return NewApiError( | ||
| http.StatusNotFound, | ||
| fmt.Sprintf("%s with ID '%s' not found", resourceType, resourceId), | 
Check failure
Code scanning / CodeQL
Potentially unsafe quoting Critical
JSON value
If this
JSON value
If this
JSON value
If this
JSON value
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 2 months ago
To fix the problem, any untrusted strings interpolated within quoted delimiters (e.g., '...') should be properly escaped so that embedded quote or backslash characters do not break out of the quotes. For Go code, the best practice is to escape single quotes as \' and backslashes as \\ before interpolation. This can be achieved using strings.ReplaceAll.
Specifically, in internal/util/ResourceErrorFactory.go, update all error message constructions that interpolate untrusted user input inside single quotes (e.g., lines 37, 47, 57, 67, 77). Add a helper function escapeSingleQuotes(s string), which will first escape \ and then escape '. Use this escaping only on those interpolated fields (such as resourceId, resourceName, fieldName, etc.) that appear inside quoted contexts.
You will need to import "strings" at the top of the file.
- 
    
    
    Copy modified line R23 
- 
    
    
    Copy modified lines R33-R39 
- 
    
    
    Copy modified line R43 
- 
    
    
    Copy modified line R46 
- 
    
    
    Copy modified line R49 
- 
    
    
    Copy modified line R52 
- 
    
    
    Copy modified line R58 
- 
    
    
    Copy modified line R61 
- 
    
    
    Copy modified line R67 
- 
    
    
    Copy modified line R70 
- 
    
    
    Copy modified line R76 
- 
    
    
    Copy modified line R79 
| @@ -20,6 +20,7 @@ | ||
| "fmt" | ||
| "github.com/devtron-labs/devtron/internal/constants" | ||
| "net/http" | ||
| "strings" | ||
| ) | ||
|  | ||
| // ResourceContext holds information about the resource for better error messages | ||
| @@ -29,12 +30,20 @@ | ||
| Operation string | ||
| } | ||
|  | ||
| // escapeSingleQuotes escapes backslash and single quote characters for use inside single-quoted strings | ||
| func escapeSingleQuotes(s string) string { | ||
| s = strings.ReplaceAll(s, `\`, `\\`) | ||
| s = strings.ReplaceAll(s, `'`, `\\'`) | ||
| return s | ||
| } | ||
|  | ||
| // NewResourceNotFoundError creates a user-friendly error for resource not found scenarios | ||
| // Leverages existing util.NewApiError() function | ||
| func NewResourceNotFoundError(resourceType, resourceId string) *ApiError { | ||
| escapedResourceId := escapeSingleQuotes(resourceId) | ||
| return NewApiError( | ||
| http.StatusNotFound, | ||
| fmt.Sprintf("%s with ID '%s' not found", resourceType, resourceId), | ||
| fmt.Sprintf("%s with ID '%s' not found", resourceType, escapedResourceId), | ||
| fmt.Sprintf("%s not found: %s", resourceType, resourceId), | ||
| ).WithCode(constants.ResourceNotFound). | ||
| WithUserDetailMessage(fmt.Sprintf("The requested %s does not exist or has been deleted.", resourceType)) | ||
| @@ -42,9 +46,10 @@ | ||
|  | ||
| // NewDuplicateResourceError creates a user-friendly error for duplicate resource scenarios | ||
| func NewDuplicateResourceError(resourceType, resourceName string) *ApiError { | ||
| escapedResourceName := escapeSingleQuotes(resourceName) | ||
| return NewApiError( | ||
| http.StatusConflict, | ||
| fmt.Sprintf("%s with name '%s' already exists", resourceType, resourceName), | ||
| fmt.Sprintf("%s with name '%s' already exists", resourceType, escapedResourceName), | ||
| fmt.Sprintf("duplicate %s: %s", resourceType, resourceName), | ||
| ).WithCode(constants.DuplicateResource). | ||
| WithUserDetailMessage(fmt.Sprintf("A %s with this name already exists. Please choose a different name.", resourceType)) | ||
| @@ -52,9 +55,10 @@ | ||
|  | ||
| // NewValidationErrorForField creates a user-friendly error for field validation failures | ||
| func NewValidationErrorForField(fieldName, reason string) *ApiError { | ||
| escapedFieldName := escapeSingleQuotes(fieldName) | ||
| return NewApiError( | ||
| http.StatusBadRequest, | ||
| fmt.Sprintf("Validation failed for field '%s': %s", fieldName, reason), | ||
| fmt.Sprintf("Validation failed for field '%s': %s", escapedFieldName, reason), | ||
| fmt.Sprintf("validation failed for %s: %s", fieldName, reason), | ||
| ).WithCode(constants.ValidationFailed). | ||
| WithUserDetailMessage("Please check the field value and try again.") | ||
| @@ -62,9 +64,10 @@ | ||
|  | ||
| // NewInvalidPathParameterError creates a user-friendly error for invalid path parameters | ||
| func NewInvalidPathParameterError(paramName, paramValue string) *ApiError { | ||
| escapedParamName := escapeSingleQuotes(paramName) | ||
| return NewApiError( | ||
| http.StatusBadRequest, | ||
| fmt.Sprintf("Invalid path parameter '%s'", paramName), | ||
| fmt.Sprintf("Invalid path parameter '%s'", escapedParamName), | ||
| fmt.Sprintf("invalid path parameter %s: %s", paramName, paramValue), | ||
| ).WithCode(constants.InvalidPathParameter). | ||
| WithUserDetailMessage("Please check the parameter format and try again.") | ||
| @@ -72,9 +73,10 @@ | ||
|  | ||
| // NewMissingRequiredFieldError creates a user-friendly error for missing required fields | ||
| func NewMissingRequiredFieldError(fieldName string) *ApiError { | ||
| escapedFieldName := escapeSingleQuotes(fieldName) | ||
| return NewApiError( | ||
| http.StatusBadRequest, | ||
| fmt.Sprintf("Required field '%s' is missing", fieldName), | ||
| fmt.Sprintf("Required field '%s' is missing", escapedFieldName), | ||
| fmt.Sprintf("missing required field: %s", fieldName), | ||
| ).WithCode(constants.MissingRequiredField). | ||
| WithUserDetailMessage("Please provide all required fields and try again.") | 
| func NewDuplicateResourceError(resourceType, resourceName string) *ApiError { | ||
| return NewApiError( | ||
| http.StatusConflict, | ||
| fmt.Sprintf("%s with name '%s' already exists", resourceType, resourceName), | 
Check failure
Code scanning / CodeQL
Potentially unsafe quoting Critical
JSON value
If this
JSON value
If this
JSON value
If this
JSON value
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 2 months ago
General approach:
Ensure that values interpolated inside quoted substrings are safely escaped so that embedded single quotes or escape characters cannot break out of the quote context.
Detailed solution:
- On all fmt.Sprintfcalls wrapping possibly-untrusted data in'...'(i.e., where a string is interpolated between single quotes in a format string), first escape all backslashes (\→\\) and single quotes ('→\') in the tainted variable before string interpolation.
- Use strings.ReplaceAllto escape backslashes before single quotes, to ensure escape characters aren't themselves broken.
- Ensure the proper import of the stringspackage in any file where the escaping is added.
- Only modify the variables which can actually receive untrusted user data (in this case, resourceName,resourceId,fieldName, and any other identifier wrapped inside'...'in afmt.Sprintf).
- Do not over-escape where not needed (e.g. do not escape variables not wrapped in quotes).
- Only change the affected code region in the snippets provided; do NOT modify any other files or logic.
- 
    
    
    Copy modified line R21 
- 
    
    
    Copy modified line R36 
- 
    
    
    Copy modified line R39 
- 
    
    
    Copy modified line R45 
- 
    
    
    Copy modified line R48 
- 
    
    
    Copy modified line R54 
- 
    
    
    Copy modified line R57 
- 
    
    
    Copy modified line R63 
- 
    
    
    Copy modified line R66 
- 
    
    
    Copy modified line R72 
- 
    
    
    Copy modified line R75 
| @@ -18,6 +18,7 @@ | ||
|  | ||
| import ( | ||
| "fmt" | ||
| "strings" | ||
| "github.com/devtron-labs/devtron/internal/constants" | ||
| "net/http" | ||
| ) | ||
| @@ -32,9 +33,10 @@ | ||
| // NewResourceNotFoundError creates a user-friendly error for resource not found scenarios | ||
| // Leverages existing util.NewApiError() function | ||
| func NewResourceNotFoundError(resourceType, resourceId string) *ApiError { | ||
| safeResourceId := strings.ReplaceAll(strings.ReplaceAll(resourceId, `\`, `\\`), "'", "\\'") | ||
| return NewApiError( | ||
| http.StatusNotFound, | ||
| fmt.Sprintf("%s with ID '%s' not found", resourceType, resourceId), | ||
| fmt.Sprintf("%s with ID '%s' not found", resourceType, safeResourceId), | ||
| fmt.Sprintf("%s not found: %s", resourceType, resourceId), | ||
| ).WithCode(constants.ResourceNotFound). | ||
| WithUserDetailMessage(fmt.Sprintf("The requested %s does not exist or has been deleted.", resourceType)) | ||
| @@ -42,9 +42,10 @@ | ||
|  | ||
| // NewDuplicateResourceError creates a user-friendly error for duplicate resource scenarios | ||
| func NewDuplicateResourceError(resourceType, resourceName string) *ApiError { | ||
| safeResourceName := strings.ReplaceAll(strings.ReplaceAll(resourceName, `\`, `\\`), "'", "\\'") | ||
| return NewApiError( | ||
| http.StatusConflict, | ||
| fmt.Sprintf("%s with name '%s' already exists", resourceType, resourceName), | ||
| fmt.Sprintf("%s with name '%s' already exists", resourceType, safeResourceName), | ||
| fmt.Sprintf("duplicate %s: %s", resourceType, resourceName), | ||
| ).WithCode(constants.DuplicateResource). | ||
| WithUserDetailMessage(fmt.Sprintf("A %s with this name already exists. Please choose a different name.", resourceType)) | ||
| @@ -52,9 +51,10 @@ | ||
|  | ||
| // NewValidationErrorForField creates a user-friendly error for field validation failures | ||
| func NewValidationErrorForField(fieldName, reason string) *ApiError { | ||
| safeFieldName := strings.ReplaceAll(strings.ReplaceAll(fieldName, `\`, `\\`), "'", "\\'") | ||
| return NewApiError( | ||
| http.StatusBadRequest, | ||
| fmt.Sprintf("Validation failed for field '%s': %s", fieldName, reason), | ||
| fmt.Sprintf("Validation failed for field '%s': %s", safeFieldName, reason), | ||
| fmt.Sprintf("validation failed for %s: %s", fieldName, reason), | ||
| ).WithCode(constants.ValidationFailed). | ||
| WithUserDetailMessage("Please check the field value and try again.") | ||
| @@ -62,9 +60,10 @@ | ||
|  | ||
| // NewInvalidPathParameterError creates a user-friendly error for invalid path parameters | ||
| func NewInvalidPathParameterError(paramName, paramValue string) *ApiError { | ||
| safeParamName := strings.ReplaceAll(strings.ReplaceAll(paramName, `\`, `\\`), "'", "\\'") | ||
| return NewApiError( | ||
| http.StatusBadRequest, | ||
| fmt.Sprintf("Invalid path parameter '%s'", paramName), | ||
| fmt.Sprintf("Invalid path parameter '%s'", safeParamName), | ||
| fmt.Sprintf("invalid path parameter %s: %s", paramName, paramValue), | ||
| ).WithCode(constants.InvalidPathParameter). | ||
| WithUserDetailMessage("Please check the parameter format and try again.") | ||
| @@ -72,9 +69,10 @@ | ||
|  | ||
| // NewMissingRequiredFieldError creates a user-friendly error for missing required fields | ||
| func NewMissingRequiredFieldError(fieldName string) *ApiError { | ||
| safeFieldName := strings.ReplaceAll(strings.ReplaceAll(fieldName, `\`, `\\`), "'", "\\'") | ||
| return NewApiError( | ||
| http.StatusBadRequest, | ||
| fmt.Sprintf("Required field '%s' is missing", fieldName), | ||
| fmt.Sprintf("Required field '%s' is missing", safeFieldName), | ||
| fmt.Sprintf("missing required field: %s", fieldName), | ||
| ).WithCode(constants.MissingRequiredField). | ||
| WithUserDetailMessage("Please provide all required fields and try again.") | 
| gitOpsChartLocation := fmt.Sprintf("%s-%s", pushChartToGitRequest.AppName, pushChartToGitRequest.EnvName) | ||
| dir := filepath.Join(clonedDir, gitOpsChartLocation) | ||
| chartYamlPath := filepath.Join(dir, chartRefBean.CHART_YAML_FILE) | ||
| if _, err := os.Stat(chartYamlPath); os.IsNotExist(err) { | 
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
This path depends on a
user-provided value
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 2 months ago
To fix the problem, we need to ensure that all user-supplied input used in constructing file paths (here: AppName and EnvName, which flow into gitOpsChartLocation → dir → chartYamlPath) is properly validated to prevent path traversal and other filesystem attacks.
Best approach:
- Explicitly validate that both AppNameandEnvName(frompushChartToGitRequest) do not contain/,\, or..sequences before using them in the path construction.
- If the inputs are only supposed to be a single path component (a typical assumption for names), reject any containing /,\, or...
- Insert this validation at the top of the shouldMigrateProxyChartDependenciesfunction inInstalledAppGitOpsService.go, before constructing any paths.
- On invalid input, log the possible attack and return an error. No further processing should occur in that case.
Implementation steps:
- Add validation checks (using e.g. strings.Contains) at the start of the function forpushChartToGitRequest.AppNameandpushChartToGitRequest.EnvName.
- If invalid, log and return.
- No new dependencies are required, as stringsis already imported.
- 
    
    
    Copy modified lines R431-R437 
| @@ -428,6 +428,13 @@ | ||
| } | ||
|  | ||
| func (impl *FullModeDeploymentServiceImpl) shouldMigrateProxyChartDependencies(pushChartToGitRequest *gitBean.PushChartToGitRequestDTO, expectedChartYamlContent string) (bool, error) { | ||
| // Validate that AppName and EnvName do not contain dangerous characters (single path component policy) | ||
| if strings.Contains(pushChartToGitRequest.AppName, "/") || strings.Contains(pushChartToGitRequest.AppName, "\\") || strings.Contains(pushChartToGitRequest.AppName, "..") || | ||
| strings.Contains(pushChartToGitRequest.EnvName, "/") || strings.Contains(pushChartToGitRequest.EnvName, "\\") || strings.Contains(pushChartToGitRequest.EnvName, "..") { | ||
| err := fmt.Errorf("invalid appName or envName in request: possible path traversal attempt (AppName: %q, EnvName: %q)", pushChartToGitRequest.AppName, pushChartToGitRequest.EnvName) | ||
| impl.Logger.Errorw("invalid appName or envName in shouldMigrateProxyChartDependencies", "err", err) | ||
| return false, err | ||
| } | ||
| clonedDir, err := impl.gitOperationService.CloneChartForHelmApp(pushChartToGitRequest.AppName, pushChartToGitRequest.RepoURL, pushChartToGitRequest.TargetRevision) | ||
| if err != nil { | ||
| impl.Logger.Errorw("error in cloning chart for helm app", "appName", pushChartToGitRequest.AppName, "repoUrl", pushChartToGitRequest.RepoURL, "err", err) | 
| } | ||
| impl.Logger.Debugw("dependencies found in requirements.yaml file", "appName", pushChartToGitRequest.AppName, "envName", pushChartToGitRequest.EnvName, "dependencies", expectedChartMetaData.Dependencies) | ||
| // check if chart.yaml file has dependencies | ||
| chartYamlContent, err := os.ReadFile(chartYamlPath) | 
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
This path depends on a
user-provided value
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 2 months ago
General fix:
The problem is uncontrolled user input (AppName and EnvName from the API) being used to construct paths, potentially allowing path traversal or access outside the intended subdirectory. The best practice is to "sanitize" or validate these values before use:
- If these are supposed to be simple names, reject any value containing path separators (/,\) or parent directory references (..).
- If you need to support sub-paths, after constructing the full file path, ensure the result is still under the intended root directory using normalization and prefix checking.
Best fix for this case:
- Validate both AppNameandEnvNamefields inshouldMigrateProxyChartDependencies(and any code paths writing them as path components) to ensure they do not contain/,\, or...
- If validation fails, log an error and fail the operation.
- This localizes the fix and does not change the interface (API), but immediately prevents the exploitation at the site of path construction.
What is needed:
- Add a validation snippet before constructing gitOpsChartLocationinshouldMigrateProxyChartDependencies.
- If pushChartToGitRequest.AppNameorpushChartToGitRequest.EnvNameare "unsafe", return an error before continuing.
- (If additional context is needed, a helper validation function can be defined in the same file for clarity and reuse.)
- 
    
    
    Copy modified lines R431-R436 
- 
    
    
    Copy modified lines R515-R522 
| @@ -428,6 +428,12 @@ | ||
| } | ||
|  | ||
| func (impl *FullModeDeploymentServiceImpl) shouldMigrateProxyChartDependencies(pushChartToGitRequest *gitBean.PushChartToGitRequestDTO, expectedChartYamlContent string) (bool, error) { | ||
| // Validate AppName and EnvName to prevent path traversal | ||
| if !isSafePathComponent(pushChartToGitRequest.AppName) || !isSafePathComponent(pushChartToGitRequest.EnvName) { | ||
| impl.Logger.Errorw("invalid appName or envName in shouldMigrateProxyChartDependencies, possible path traversal attempt", | ||
| "appName", pushChartToGitRequest.AppName, "envName", pushChartToGitRequest.EnvName) | ||
| return false, fmt.Errorf("invalid application name or environment name") | ||
| } | ||
| clonedDir, err := impl.gitOperationService.CloneChartForHelmApp(pushChartToGitRequest.AppName, pushChartToGitRequest.RepoURL, pushChartToGitRequest.TargetRevision) | ||
| if err != nil { | ||
| impl.Logger.Errorw("error in cloning chart for helm app", "appName", pushChartToGitRequest.AppName, "repoUrl", pushChartToGitRequest.RepoURL, "err", err) | ||
| @@ -506,3 +512,11 @@ | ||
| strings.ToLower(strings.TrimSpace(dependency.Version)), | ||
| strings.ToLower(strings.TrimSpace(dependency.Repository))) | ||
| } | ||
|  | ||
| // isSafePathComponent ensures a string is safe to use as a single path segment (no slash, backslash, or '..') | ||
| func isSafePathComponent(s string) bool { | ||
| if strings.Contains(s, "/") || strings.Contains(s, "\\") || strings.Contains(s, "..") { | ||
| return false | ||
| } | ||
| return true | ||
| } | 
| // copying content from reference template dir to cloned dir (if Chart.yaml file is not found) | ||
| // if Chart.yaml file is not found, we are assuming here that reference chart contents are not pushed in git-ops repo | ||
| if _, err := os.Stat(filepath.Join(dir, "Chart.yaml")); os.IsNotExist(err) { | ||
| if _, err := os.Stat(filepath.Join(dir, chartRefBean.CHART_YAML_FILE)); os.IsNotExist(err) { | 
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 2 months ago
To fix this issue, we should ensure that any path components that originate from user input (directly or transitively) used to create directories or files are validated for safety. This includes repo names, app names, and chart locations. The single best way is to enforce, before using them in path construction:
- No slashes (/,\), no..sequences, and no leading/trailing whitespace.
- Optionally, enforce an allow-list or pattern for acceptable values (e.g., only alphanumerics, hyphens, underscores).
- The normalization of the final target directory should guarantee containment within the application's working directory.
For the shown snippets, edit the place where repo directory names and chart directory names are constructed (e.g., in PushChartToGitRepo, wherever gitOpsRepoName, chartLocation, chartDir, etc. are received, concatenate, and used for paths). Add validation checks and short-circuit/error out if they fail. It's best to create a helper validation function and call it at each relevant point.
Required edits:
- Implement a helper function isSafePathComponent(string) boolsomewhere appropriate (e.g., top ofpkg/deployment/gitOps/git/GitOperationService.go).
- In PushChartToGitRepo, before usinggitOpsRepoName,chartLocation, and any other user-derived path in file/directory creation (especially before constructingchartDiranddir), call the helper and abort (return error/log) on failure.
- 
    
    
    Copy modified lines R44-R57 
- 
    
    
    Copy modified lines R136-R144 
| @@ -41,6 +41,20 @@ | ||
| "time" | ||
| ) | ||
|  | ||
| // isSafePathComponent checks supplied path component for allowed characters and rejects traversal or separator. | ||
| func isSafePathComponent(component string) bool { | ||
| if len(component) == 0 || | ||
| strings.Contains(component, "/") || | ||
| strings.Contains(component, "\\") || | ||
| strings.Contains(component, "..") || | ||
| component != strings.TrimSpace(component) { | ||
| return false | ||
| } | ||
| // Add stricter pattern if needed, e.g., only allow [a-zA-Z0-9-_] | ||
| re := regexp.MustCompile(`^[a-zA-Z0-9\-_\.]+$`) | ||
| return re.MatchString(component) | ||
| } | ||
|  | ||
| type GitOperationService interface { | ||
| CreateGitRepositoryForDevtronApp(ctx context.Context, gitOpsRepoName string, targetRevision string, userId int32) (chartGitAttribute *commonBean.ChartGitAttribute, err error) | ||
| // CreateFirstCommitOnHead - creates the first commit on the head of the git repository (mostly empty). | ||
| @@ -119,6 +133,15 @@ | ||
| } | ||
|  | ||
| func (impl *GitOperationServiceImpl) PushChartToGitRepo(ctx context.Context, gitOpsRepoName, chartLocation, tempReferenceTemplateDir, repoUrl, targetRevision string, userId int32) (err error) { | ||
| // Validate path components | ||
| if !isSafePathComponent(gitOpsRepoName) { | ||
| impl.logger.Errorw("Unsafe repo name detected for gitOpsRepoName, refusing path operation", "repoName", gitOpsRepoName) | ||
| return fmt.Errorf("invalid or unsafe repository name") | ||
| } | ||
| if !isSafePathComponent(chartLocation) { | ||
| impl.logger.Errorw("Unsafe chart location detected for chartLocation, refusing path operation", "chartLocation", chartLocation) | ||
| return fmt.Errorf("invalid or unsafe chart location") | ||
| } | ||
| newCtx, span := otel.Tracer("orchestrator").Start(ctx, "GitOperationServiceImpl.PushChartToGitRepo") | ||
| defer span.End() | ||
| chartDir := fmt.Sprintf("%s-%s", gitOpsRepoName, impl.chartTemplateService.GetDir()) | 
| gitOpsChartLocation := fmt.Sprintf("%s-%s", pushChartToGitRequest.AppName, pushChartToGitRequest.EnvName) | ||
| dir := filepath.Join(clonedDir, gitOpsChartLocation) | ||
| err := os.MkdirAll(dir, os.ModePerm) | ||
| err = os.MkdirAll(dir, os.ModePerm) | 
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
This path depends on a
user-provided value
This path depends on a
user-provided value
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 2 months ago
To address the vulnerability, we must ensure that user-controlled input (AppName and EnvironmentName, used to construct gitOpsChartLocation) cannot contain directory traversal sequences, absolute paths, or path separators. This can be done by validating these fields in the service layer where the chart location is constructed, specifically before using them in filesystem operations.
The simplest and safest approach is to check each component for any path separator (/, \) or "..", and reject the request if such values are present. Since the inputs are expected to be identifiers (names), this restriction is reasonable and will prevent traversal and arbitrary directory creation.
The fix should be implemented at the start of PushChartToGitOpsRepoForHelmApp in pkg/deployment/gitOps/git/GitOperationService.go, before constructing gitOpsChartLocation. If an invalid input is detected, we should return an error and log the incident.
Required changes:
- Add validation in PushChartToGitOpsRepoForHelmAppforpushChartToGitRequest.AppNameandpushChartToGitRequest.EnvName.
- Reject any value containing /,\, or"..".
- Log and return an error if invalid input is detected.
No new external dependency is required -- standard library strings suffices.
- 
    
    
    Copy modified lines R329-R335 
| @@ -326,6 +326,13 @@ | ||
| // PushChartToGitOpsRepoForHelmApp pushes built chart to GitOps repo (Specific implementation for Helm Apps) | ||
| // TODO refactoring: Make a common method for both PushChartToGitRepo and PushChartToGitOpsRepoForHelmApp | ||
| func (impl *GitOperationServiceImpl) PushChartToGitOpsRepoForHelmApp(ctx context.Context, pushChartToGitRequest *bean.PushChartToGitRequestDTO, valuesConfig *ChartConfig) (*commonBean.ChartGitAttribute, string, error) { | ||
| // Validate that AppName and EnvName are not malicious | ||
| for _, component := range []string{pushChartToGitRequest.AppName, pushChartToGitRequest.EnvName} { | ||
| if strings.Contains(component, "/") || strings.Contains(component, "\\") || strings.Contains(component, "..") { | ||
| impl.logger.Errorw("invalid chart location component", "value", component) | ||
| return nil, "", fmt.Errorf("invalid chart location component: %s", component) | ||
| } | ||
| } | ||
| clonedDir, err := impl.CloneChartForHelmApp(pushChartToGitRequest.AppName, pushChartToGitRequest.RepoURL, pushChartToGitRequest.TargetRevision) | ||
| if err != nil { | ||
| impl.logger.Errorw("error in cloning chart for helm app", "appName", pushChartToGitRequest.AppName, "repoUrl", pushChartToGitRequest.RepoURL, "err", err) | 
| } | ||
|  | ||
| // print the curl command for debugging | ||
| v.logger.Debugw("Curl command", "command", v.buildCurlCommand(req)) | 
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High test
Sensitive data returned by HTTP request headers
          
            
              
                
              
            
            Show autofix suggestion
            Hide autofix suggestion
          
      Copilot Autofix
AI 2 months ago
The best way to fix this problem is to exclude sensitive headers from being logged as part of the constructed curl command. Instead of logging all headers verbatim, the buildCurlCommand function should redact (<REDACTED>) or omit values for known sensitive headers, such as Authorization, Cookie, and any custom headers that commonly contain secrets. This can be achieved by maintaining a set of sensitive header names and replacing their values before including them in the logged curl command.
Specifically, within buildCurlCommand:
- Define a map or list of sensitive header names: e.g., "Authorization", "Cookie", "Set-Cookie", "X-Api-Key", etc.
- When constructing each header line, check if the header is sensitive.
- If sensitive, replace the value with <REDACTED>; otherwise, include the real value.
- The fix should only touch the buildCurlCommandfunction in the current file.
No new dependencies are needed: this logic can be expressed in a few lines of Go within the existing code.
- 
    
    
    Copy modified lines R72-R80 
- 
    
    
    Copy modified lines R84-R89 
- 
    
    
    Copy modified line R96 
| @@ -69,10 +69,24 @@ | ||
| cmd.WriteString(req.URL.String()) | ||
| cmd.WriteString("' \\\n") | ||
|  | ||
| // Redact sensitive headers | ||
| sensitiveHeaders := map[string]struct{}{ | ||
| "Authorization": {}, | ||
| "Cookie": {}, | ||
| "Set-Cookie": {}, | ||
| "X-Api-Key": {}, | ||
| "X-Auth-Token": {}, | ||
| } | ||
|  | ||
| // Add headers | ||
| for key, values := range req.Header { | ||
| for _, value := range values { | ||
| cmd.WriteString(fmt.Sprintf(" --header '%s: %s' \\\n", key, value)) | ||
| _, isSensitive := sensitiveHeaders[strings.Title(strings.ToLower(key))] | ||
| headerValue := value | ||
| if isSensitive { | ||
| headerValue = "<REDACTED>" | ||
| } | ||
| cmd.WriteString(fmt.Sprintf(" --header '%s: %s' \\\n", key, headerValue)) | ||
| } | ||
| } | ||
|  | ||
| @@ -82,7 +93,7 @@ | ||
| for name, value := range v.additionalCookies { | ||
| cmd.WriteString(name) | ||
| cmd.WriteString("=") | ||
| cmd.WriteString(value) | ||
| cmd.WriteString("<REDACTED>") | ||
| cmd.WriteString("; ") | ||
| } | ||
| cmd.WriteString("' \\\n") | 


Description
Fixes #
Checklist:
Does this PR introduce a user-facing change?