Skip to content

Commit 0282dee

Browse files
kartik-579kartik-579
authored
fix: Code security fixes (sql injection, sensitive data logs) (#5787)
* updated ExternalLinkIdentifierMappingRepository * updated ExternalLinkIdentifierMappingRepository * updated ExternalLinkIdentifierMappingRepository * updated UserRepository, UserRepositoryQueryBuilder * updated DefaultAuthRoleRepository * updated DefaultAuthRoleRepository * updated InstalledAppRepository * updated AppStoreApplicationVersionRepository * added like clause util * updated CveStoreRepository * updated BulkUpdateRepository * updated BulkUpdateRepository * updated BulkUpdateRepository * updated BulkUpdateRepository * updated AppRepository * updated AppRepository * updated AppRepository * updated AppRepository * updated VariableTemplateParser * updated CdHandler * updated CIPipelineEventPublishService * updated CIPipelineEventPublishService * updated TriggerService * updated FullModeDeploymentService * updated FullModeDeploymentService * udpated AppStoreDeploymentService * udpated AppStoreDeploymentService * updated chartGroupService * updated RestClient * updated WriteJsonResp * updated GitProviderRestHandler * updated GitProviderRestHandler * updated GitProviderRestHandler * updated GitProviderRestHandler * updated DockerRegRestHandler * updated DockerRegRestHandler * updated DockerRegRestHandler * updated DockerRegRestHandler * updated DockerRegRestHandler * updated DockerRegRestHandler * updated DockerRegRestHandler * updated DockerRegRestHandler * updated CIPipelineEventPublishService * updated CdHandler * updated AppStoreApplicationVersionRepository * "updated ImageScanDeployInfoRepository * updated wire * updated AppListingRepositoryQueryBuilder & AppListingRepository * updated AppListingRepositoryQueryBuilder & AppListingRepository * updated ValidationHelper * wip * wip * self review change * self review change * self review change * self review change * self review change * self review change * added comment * added comment * updated enum * order change * wip * conversion fix * queryParams order change * wip * update * removed conversion method for in
1 parent c54fc03 commit 0282dee

32 files changed

Lines changed: 458 additions & 484 deletions

‎api/restHandler/DockerRegRestHandler.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -307,10 +307,10 @@ func (impl DockerRegRestHandlerImpl) ValidateDockerRegistryConfig(w http.Respons
307307
}
308308
bean.User = userId
309309

310-
impl.logger.Infow("request payload, ValidateDockerRegistryConfig", "payload", bean)
310+
impl.logger.Infow("request payload, ValidateDockerRegistryConfig", "dockerRegistryId", bean.Id)
311311
err = impl.validator.Struct(bean)
312312
if err != nil {
313-
impl.logger.Errorw("validation err, ValidateDockerRegistryConfig", "err", err, "payload", bean)
313+
impl.logger.Errorw("validation err, ValidateDockerRegistryConfig", "err", err, "dockerRegistryId", bean.Id)
314314
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
315315
return
316316
}
@@ -494,23 +494,23 @@ func (impl DockerRegRestHandlerImpl) UpdateDockerRegistryConfig(w http.ResponseW
494494
var bean types.DockerArtifactStoreBean
495495
err = decoder.Decode(&bean)
496496
if err != nil {
497-
impl.logger.Errorw("request err, UpdateDockerRegistryConfig", "err", err, "payload", bean)
497+
impl.logger.Errorw("request err, UpdateDockerRegistryConfig", "err", err, "dockerRegistryId", bean.Id)
498498
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
499499
return
500500
}
501501
bean.User = userId
502502
requestErr := ValidateDockerArtifactStoreRequestBean(bean)
503503
if requestErr != nil {
504504
err = fmt.Errorf("invalid payload, missing or incorrect values for required fields")
505-
impl.logger.Errorw("validation err, SaveDockerRegistryConfig", "err", err, "payload", bean)
505+
impl.logger.Errorw("validation err, SaveDockerRegistryConfig", "err", err, "dockerRegistryId", bean.Id)
506506
common.WriteJsonResp(w, requestErr, nil, http.StatusBadRequest)
507507
return
508508
}
509509

510-
impl.logger.Infow("request payload, UpdateDockerRegistryConfig", "err", err, "payload", bean)
510+
impl.logger.Infow("request payload, UpdateDockerRegistryConfig", "err", err, "dockerRegistryId", bean.Id)
511511
err = impl.validator.Struct(bean)
512512
if err != nil {
513-
impl.logger.Errorw("validation err, UpdateDockerRegistryConfig", "err", err, "payload", bean)
513+
impl.logger.Errorw("validation err, UpdateDockerRegistryConfig", "err", err, "dockerRegistryId", bean.Id)
514514
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
515515
return
516516
}
@@ -525,7 +525,7 @@ func (impl DockerRegRestHandlerImpl) UpdateDockerRegistryConfig(w http.ResponseW
525525

526526
res, err := impl.dockerRegistryConfig.Update(&bean)
527527
if err != nil {
528-
impl.logger.Errorw("service err, UpdateDockerRegistryConfig", "err", err, "payload", bean)
528+
impl.logger.Errorw("service err, UpdateDockerRegistryConfig", "err", err, "dockerRegistryId", bean.Id)
529529
common.WriteJsonResp(w, err, nil, http.StatusInternalServerError)
530530
return
531531
}

‎api/restHandler/GitProviderRestHandler.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,15 +175,15 @@ func (impl GitProviderRestHandlerImpl) UpdateGitRepoConfig(w http.ResponseWriter
175175
var bean types.GitRegistry
176176
err = decoder.Decode(&bean)
177177
if err != nil {
178-
impl.logger.Errorw("request err, UpdateGitRepoConfig", "err", err, "payload", bean)
178+
impl.logger.Errorw("request err, UpdateGitRepoConfig", "err", err, "gitRegistryId", bean.Id)
179179
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
180180
return
181181
}
182182
bean.UserId = userId
183-
impl.logger.Infow("request payload, UpdateGitRepoConfig", "payload", bean)
183+
impl.logger.Infow("request payload, UpdateGitRepoConfig", "gitRegistryId", bean.Id)
184184
err = impl.validator.Struct(bean)
185185
if err != nil {
186-
impl.logger.Errorw("validation err, UpdateGitRepoConfig", "err", err, "payload", bean)
186+
impl.logger.Errorw("validation err, UpdateGitRepoConfig", "err", err, "gitRegistryId", bean.Id)
187187
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
188188
return
189189
}
@@ -197,7 +197,7 @@ func (impl GitProviderRestHandlerImpl) UpdateGitRepoConfig(w http.ResponseWriter
197197

198198
res, err := impl.gitRegistryConfig.Update(&bean)
199199
if err != nil {
200-
impl.logger.Errorw("service err, UpdateGitRepoConfig", "err", err, "payload", bean)
200+
impl.logger.Errorw("service err, UpdateGitRepoConfig", "err", err, "gitRegistryId", bean.Id)
201201
common.WriteJsonResp(w, err, nil, http.StatusInternalServerError)
202202
return
203203
}

‎api/restHandler/common/apiError.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ func WriteJsonResp(w http.ResponseWriter, err error, respBody interface{}, statu
114114
}
115115
}
116116
if status > 299 || err != nil {
117-
util.GetLogger().Infow("ERROR RES", "TYPE", "API-ERROR", "RES", response.Code, "ERROR-MSG", response.Errors, "err", err)
117+
util.GetLogger().Infow("ERROR RES", "TYPE", "API-ERROR", "RES", response.Code, "err", err)
118118
}
119119
w.Header().Set(CONTENT_TYPE, APPLICATION_JSON)
120120
w.WriteHeader(status)

‎client/gitSensor/GitSensorRestClient.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,6 @@ func (session *RestClientImpl) doRequest(clientRequest *ClientRequest) (resBody
273273
if req, err := json.Marshal(clientRequest.RequestBody); err != nil {
274274
return nil, nil, err
275275
} else {
276-
session.logger.Debugw("argo req with body", "body", string(req))
277276
body = bytes.NewBuffer(req)
278277
}
279278
}

‎cmd/external-app/wire_gen.go‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/sql/repository/AppListingRepository.go‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -116,10 +116,10 @@ func (impl AppListingRepositoryImpl) FetchJobs(appIds []int, statuses []string,
116116
if len(appIds) == 0 {
117117
return jobContainers, nil
118118
}
119-
jobsQuery := impl.appListingRepositoryQueryBuilder.BuildJobListingQuery(appIds, statuses, environmentIds, sortOrder)
119+
jobsQuery, jobsQueryParams := impl.appListingRepositoryQueryBuilder.BuildJobListingQuery(appIds, statuses, environmentIds, sortOrder)
120120

121121
impl.Logger.Debugw("basic app detail query: ", jobsQuery)
122-
_, appsErr := impl.dbConnection.Query(&jobContainers, jobsQuery)
122+
_, appsErr := impl.dbConnection.Query(&jobContainers, jobsQuery, jobsQueryParams...)
123123
if appsErr != nil {
124124
impl.Logger.Error(appsErr)
125125
return jobContainers, appsErr
@@ -220,10 +220,10 @@ func (impl AppListingRepositoryImpl) FetchAppsByEnvironmentV2(appListingFilter h
220220

221221
if string(appListingFilter.SortBy) == helper.LastDeployedSortBy {
222222

223-
query := impl.appListingRepositoryQueryBuilder.GetAppIdsQueryWithPaginationForLastDeployedSearch(appListingFilter)
223+
query, queryParams := impl.appListingRepositoryQueryBuilder.GetAppIdsQueryWithPaginationForLastDeployedSearch(appListingFilter)
224224
impl.Logger.Debug("GetAppIdsQueryWithPaginationForLastDeployedSearch query ", query)
225225
start := time.Now()
226-
_, err := impl.dbConnection.Query(&lastDeployedTimeDTO, query)
226+
_, err := impl.dbConnection.Query(&lastDeployedTimeDTO, query, queryParams...)
227227
middleware.AppListingDuration.WithLabelValues("getAppIdsQueryWithPaginationForLastDeployedSearch", "devtron").Observe(time.Since(start).Seconds())
228228
if err != nil || len(lastDeployedTimeDTO) == 0 {
229229
if err != nil {
@@ -238,9 +238,9 @@ func (impl AppListingRepositoryImpl) FetchAppsByEnvironmentV2(appListingFilter h
238238
appIdsFound[i] = obj.AppId
239239
}
240240
appListingFilter.AppIds = appIdsFound
241-
appContainerQuery := impl.appListingRepositoryQueryBuilder.GetQueryForAppEnvContainerss(appListingFilter)
242-
impl.Logger.Debug("GetQueryForAppEnvContainerss query ", query)
243-
_, err = impl.dbConnection.Query(&appEnvContainer, appContainerQuery)
241+
appContainerQuery, appContainerQueryParams := impl.appListingRepositoryQueryBuilder.GetQueryForAppEnvContainers(appListingFilter)
242+
impl.Logger.Debug("GetQueryForAppEnvContainers query ", query)
243+
_, err = impl.dbConnection.Query(&appEnvContainer, appContainerQuery, appContainerQueryParams...)
244244
if err != nil {
245245
impl.Logger.Errorw("error in getting appEnvContainers with appList filter from db", "err", err, "filter", appListingFilter, "query", appContainerQuery)
246246
return appEnvArr, appsSize, err
@@ -250,10 +250,10 @@ func (impl AppListingRepositoryImpl) FetchAppsByEnvironmentV2(appListingFilter h
250250

251251
// to get all the appIds in appEnvs allowed for user and filtered by the appListing filter and sorted by name
252252
appIdCountDtos := make([]*bean.AppEnvironmentContainer, 0)
253-
appIdCountQuery := impl.appListingRepositoryQueryBuilder.GetAppIdsQueryWithPaginationForAppNameSearch(appListingFilter)
253+
appIdCountQuery, appIdCountQueryParams := impl.appListingRepositoryQueryBuilder.GetAppIdsQueryWithPaginationForAppNameSearch(appListingFilter)
254254
impl.Logger.Debug("GetAppIdsQueryWithPaginationForAppNameSearch query ", appIdCountQuery)
255255
start := time.Now()
256-
_, appsErr := impl.dbConnection.Query(&appIdCountDtos, appIdCountQuery)
256+
_, appsErr := impl.dbConnection.Query(&appIdCountDtos, appIdCountQuery, appIdCountQueryParams...)
257257
middleware.AppListingDuration.WithLabelValues("getAppIdsQueryWithPaginationForAppNameSearch", "devtron").Observe(time.Since(start).Seconds())
258258
if appsErr != nil || len(appIdCountDtos) == 0 {
259259
if appsErr != nil {
@@ -271,10 +271,10 @@ func (impl AppListingRepositoryImpl) FetchAppsByEnvironmentV2(appListingFilter h
271271
appListingFilter.AppIds = uniqueAppIds
272272
// set appids required for this page in the filter and get the appEnv containers of these apps
273273
appListingFilter.AppIds = uniqueAppIds
274-
appsEnvquery := impl.appListingRepositoryQueryBuilder.GetQueryForAppEnvContainerss(appListingFilter)
275-
impl.Logger.Debug("GetQueryForAppEnvContainerss query: ", appsEnvquery)
274+
appsEnvquery, appsEnvQueryParams := impl.appListingRepositoryQueryBuilder.GetQueryForAppEnvContainers(appListingFilter)
275+
impl.Logger.Debug("GetQueryForAppEnvContainers query: ", appsEnvquery)
276276
start = time.Now()
277-
_, appsErr = impl.dbConnection.Query(&appEnvContainer, appsEnvquery)
277+
_, appsErr = impl.dbConnection.Query(&appEnvContainer, appsEnvquery, appsEnvQueryParams...)
278278
middleware.AppListingDuration.WithLabelValues("buildAppListingQuery", "devtron").Observe(time.Since(start).Seconds())
279279
if appsErr != nil {
280280
impl.Logger.Errorw("error in getting appEnvContainers with appList filter from db", "err", appsErr, "filter", appListingFilter, "query", appsEnvquery)

‎internal/sql/repository/app/AppRepository.go‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/devtron-labs/devtron/internal/sql/repository/helper"
2222
"github.com/devtron-labs/devtron/pkg/sql"
2323
"github.com/devtron-labs/devtron/pkg/team"
24+
"github.com/devtron-labs/devtron/util"
2425
"github.com/go-pg/pg"
2526
"go.uber.org/zap"
2627
"time"
@@ -290,9 +291,10 @@ func (repo AppRepositoryImpl) FindAllActiveAppsWithTeamWithTeamId(teamID int, ap
290291

291292
func (repo AppRepositoryImpl) FindAllActiveAppsWithTeamByAppNameMatch(appNameMatch string, appType helper.AppType) ([]*App, error) {
292293
var apps []*App
293-
appNameLikeQuery := "app.app_name like '%" + appNameMatch + "%'"
294294
err := repo.dbConnection.Model(&apps).Column("Team").
295-
Where("app.active = ?", true).Where("app.app_type = ?", appType).Where(appNameLikeQuery).
295+
Where("app.active = ?", true).
296+
Where("app.app_type = ?", appType).
297+
Where("app.app_name like ?", util.GetLIKEClauseQueryParam(appNameMatch)).
296298
Select()
297299
return apps, err
298300
}
@@ -499,24 +501,25 @@ func (repo AppRepositoryImpl) FetchAppIdsWithFilter(jobListingFilter helper.AppL
499501
Id int `json:"id"`
500502
}
501503
var jobIds []AppId
502-
whereCondition := " where active = true and app_type = 2 "
504+
var queryParams []interface{}
505+
query := "select id from app where active = true and app_type = 2 "
503506
if len(jobListingFilter.Teams) > 0 {
504-
whereCondition += " and team_id in (" + helper.GetCommaSepratedString(jobListingFilter.Teams) + ")"
507+
query += " and team_id in (?) "
508+
queryParams = append(queryParams, pg.In(jobListingFilter.Teams))
505509
}
506510
if len(jobListingFilter.AppIds) > 0 {
507-
whereCondition += " and id in (" + helper.GetCommaSepratedString(jobListingFilter.AppIds) + ")"
511+
query += " and id in (?) "
512+
queryParams = append(queryParams, pg.In(jobListingFilter.AppIds))
508513
}
509-
510514
if len(jobListingFilter.AppNameSearch) > 0 {
511-
whereCondition += " and display_name like '%" + jobListingFilter.AppNameSearch + "%' "
515+
query += " and display_name like ? "
516+
queryParams = append(queryParams, util.GetLIKEClauseQueryParam(jobListingFilter.AppNameSearch))
512517
}
513-
orderByCondition := " order by display_name "
518+
query += " order by display_name "
514519
if jobListingFilter.SortOrder == "DESC" {
515-
orderByCondition += string(jobListingFilter.SortOrder)
520+
query += " DESC "
516521
}
517-
query := "select id " + "from app " + whereCondition + orderByCondition
518-
519-
_, err := repo.dbConnection.Query(&jobIds, query)
522+
_, err := repo.dbConnection.Query(&jobIds, query, queryParams...)
520523
appCounts := make([]int, 0)
521524
for _, id := range jobIds {
522525
appCounts = append(appCounts, id.Id)
@@ -535,12 +538,10 @@ func (repo AppRepositoryImpl) FetchAppIdsByDisplayNamesForJobs(names []string) (
535538
DisplayName string `json:"display_name"`
536539
}
537540
var jobIdName []App
538-
whereCondition := fmt.Sprintf(" where active = true and app_type = %v ", helper.Job)
539-
whereCondition += " and display_name in (" + helper.GetCommaSepratedStringWithComma(names) + ");"
540-
query := "select id, display_name from app " + whereCondition
541-
_, err := repo.dbConnection.Query(&jobIdName, query)
542-
appResp := make(map[int]string)
543-
jobIds := make([]int, 0)
541+
query := "select id, display_name from app where active = ? and app_type = ? and display_name in (?);"
542+
_, err := repo.dbConnection.Query(&jobIdName, query, true, helper.Job, pg.In(names))
543+
appResp := make(map[int]string, len(jobIdName))
544+
jobIds := make([]int, 0, len(jobIdName))
544545
for _, id := range jobIdName {
545546
appResp[id.Id] = id.DisplayName
546547
jobIds = append(jobIds, id.Id)

0 commit comments

Comments
 (0)