From c7719942802de882e94bb0cb102f2858cb0fdb44 Mon Sep 17 00:00:00 2001 From: Yashvardhan Thanvi Date: Thu, 22 Oct 2026 08:49:09 +0530 Subject: [PATCH] fix(blueprint): keep the scheduled plan unmasked after PATCH /blueprints PatchBlueprint sanitized the blueprint it had just handed to the cron job, and the gitextractor sanitizer rewrote the options map in place. The next scheduled pipeline then cloned with a masked URL until the server restarted. Normal-mode blueprints with credentials in beforePlan or afterPlan were hit too, since those tasks are shared with the generated plan. Sanitizing now works on copies: SanitizePluginOption masks a clone of the options, SanitizeTask returns a new task, and SanitizeBlueprint and SanitizePipeline build a new plan. PatchBlueprint sanitizes a copy of the saved blueprint for its response, so the plan it returns is still masked. Closes #9243 Signed-off-by: Yashvardhan Thanvi --- backend/server/services/blueprint.go | 6 ++- backend/server/services/blueprint_test.go | 63 +++++++++++++++++++++++ backend/server/services/pipeline.go | 52 +++++++++++++------ 3 files changed, 103 insertions(+), 18 deletions(-) diff --git a/backend/server/services/blueprint.go b/backend/server/services/blueprint.go index 6acc95849d3..39d93189e19 100644 --- a/backend/server/services/blueprint.go +++ b/backend/server/services/blueprint.go @@ -230,10 +230,12 @@ func PatchBlueprint(id uint64, body map[string]interface{}) (*models.Blueprint, if err != nil { return nil, err } - if err := SanitizeBlueprint(blueprint); err != nil { + // the cron job registered by saveBlueprint holds this blueprint, so sanitize a copy for the response + sanitized := *blueprint + if err := SanitizeBlueprint(&sanitized); err != nil { return nil, errors.Convert(err) } - return blueprint, nil + return &sanitized, nil } // DeleteBlueprint FIXME ... diff --git a/backend/server/services/blueprint_test.go b/backend/server/services/blueprint_test.go index d5f80ae0f25..218ad336ca0 100644 --- a/backend/server/services/blueprint_test.go +++ b/backend/server/services/blueprint_test.go @@ -21,9 +21,72 @@ import ( "testing" coreModels "github.com/apache/devlake/core/models" + "github.com/apache/devlake/impls/logruslog" + mockdal "github.com/apache/devlake/mocks/core/dal" + "github.com/robfig/cron/v3" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) +const ( + testGitUrl = "https://git:secret-token@example.com/group/repo.git" + testMaskedGitUrl = "https://git:************@example.com/group/repo.git" +) + +func TestSanitizeBlueprintLeavesTheSourcePlanUnchanged(t *testing.T) { + bp := &coreModels.Blueprint{ + Plan: coreModels.PipelinePlan{{ + {Plugin: "gitextractor", Options: map[string]interface{}{"url": testGitUrl, "name": "group/repo"}}, + }}, + } + + sanitized := *bp + require.NoError(t, SanitizeBlueprint(&sanitized)) + + assert.Equal(t, map[string]interface{}{"url": testMaskedGitUrl, "name": "group/repo"}, sanitized.Plan[0][0].Options) + assert.Equal(t, map[string]interface{}{"url": testGitUrl, "name": "group/repo"}, bp.Plan[0][0].Options) +} + +func TestPatchBlueprintKeepsTheScheduledPlanUnsanitized(t *testing.T) { + withProjectTestValidator(t) + testDB := mockdal.NewDal(t) + withProjectTestDatabase(t, testDB) + previousLogger, previousCron, previousCronIds := logger, cronManager, bpCronIdMap + logger, cronManager, bpCronIdMap = logruslog.Global, cron.New(), map[uint64]cron.EntryID{} + t.Cleanup(func() { + logger, cronManager, bpCronIdMap = previousLogger, previousCron, previousCronIds + }) + + testDB.On("First", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + bp := args.Get(0).(*coreModels.Blueprint) + bp.ID = 9 + bp.Name = "advanced" + bp.Mode = coreModels.BLUEPRINT_MODE_ADVANCED + bp.Enable = true + bp.CronConfig = "0 0 * * *" + bp.Plan = coreModels.PipelinePlan{{ + {Plugin: "gitextractor", Options: map[string]interface{}{"url": testGitUrl}}, + }} + }).Return(nil).Once() + testDB.On("Pluck", "name", mock.Anything, mock.Anything).Return(nil).Once() + testDB.On("All", mock.Anything, mock.Anything).Return(nil).Once() + testDB.On("Update", mock.Anything).Return(nil).Once() + testDB.On("Delete", mock.Anything, mock.Anything).Return(nil).Times(3) + + patched, err := PatchBlueprint(9, map[string]interface{}{"cronConfig": "*/5 * * * *"}) + require.NoError(t, err) + assert.Equal(t, testMaskedGitUrl, patched.Plan[0][0].Options["url"]) + + // the cron job must still run the stored plan, not the one sanitized for the response + entries := cronManager.Entries() + require.Len(t, entries, 1) + job, ok := entries[0].Job.(*BlueprintJob) + require.True(t, ok) + assert.Equal(t, "*/5 * * * *", job.Blueprint.CronConfig) + assert.Equal(t, testGitUrl, job.Blueprint.Plan[0][0].Options["url"]) +} + func TestParallelizePipelineTasks(t *testing.T) { plan1 := coreModels.PipelinePlan{ { diff --git a/backend/server/services/pipeline.go b/backend/server/services/pipeline.go index bc7b6f9ceb0..059d72c2d0e 100644 --- a/backend/server/services/pipeline.go +++ b/backend/server/services/pipeline.go @@ -20,9 +20,11 @@ package services import ( "context" "fmt" + "maps" "net/url" "os" "path/filepath" + "slices" "strings" "sync" "time" @@ -44,6 +46,10 @@ import ( var defaultNotificationService *DefaultPipelineNotificationService var globalPipelineLog = logruslog.Global.Nested("pipeline service") + +// pluginOptionSanitizers mask secrets in plugin options. Each one receives a +// shallow copy of the options, so it may replace top-level values but must not +// modify nested maps or slices in place. var pluginOptionSanitizers = map[string]func(map[string]interface{}){ "gitextractor": func(options map[string]interface{}) { if v, ok := options["url"]; ok { @@ -144,45 +150,59 @@ func CreatePipeline(newPipeline *models.NewPipeline, shouldSanitize bool) (*mode return pipeline, nil } +// SanitizeBlueprint replaces blueprint.Plan with a sanitized copy. The original +// plan is left untouched because it may be shared, e.g. with a scheduled job. func SanitizeBlueprint(blueprint *models.Blueprint) error { - for planStageIdx, pipelineStage := range blueprint.Plan { - for planTaskIdx := range pipelineStage { - pipelineTask, err := SanitizeTask(blueprint.Plan[planStageIdx][planTaskIdx]) - if err != nil { - return err - } - blueprint.Plan[planStageIdx][planTaskIdx] = pipelineTask - } + plan, err := sanitizePlan(blueprint.Plan) + if err != nil { + return err } + blueprint.Plan = plan return nil } +// SanitizePipeline replaces pipeline.Plan with a sanitized copy. func SanitizePipeline(pipeline *models.Pipeline) error { - for planStageIdx, pipelineStage := range pipeline.Plan { - for planTaskIdx := range pipelineStage { - task := pipeline.Plan[planStageIdx][planTaskIdx] + plan, err := sanitizePlan(pipeline.Plan) + if err != nil { + return err + } + pipeline.Plan = plan + return nil +} + +func sanitizePlan(plan models.PipelinePlan) (models.PipelinePlan, error) { + sanitized := slices.Clone(plan) + for planStageIdx, pipelineStage := range plan { + sanitized[planStageIdx] = slices.Clone(pipelineStage) + for planTaskIdx, task := range pipelineStage { pipelineTask, err := SanitizeTask(task) if err != nil { - return err + return nil, err } - pipeline.Plan[planStageIdx][planTaskIdx] = pipelineTask + sanitized[planStageIdx][planTaskIdx] = pipelineTask } } - return nil + return sanitized, nil } +// SanitizeTask returns a copy of the task with sanitized options. func SanitizeTask(pipelineTask *models.PipelineTask) (*models.PipelineTask, error) { pluginName := pipelineTask.Plugin options, err := SanitizePluginOption(pluginName, pipelineTask.Options) if err != nil { return pipelineTask, err } - pipelineTask.Options = options - return pipelineTask, nil + sanitized := *pipelineTask + sanitized.Options = options + return &sanitized, nil } +// SanitizePluginOption returns the sanitized options. The sanitizer works on a +// shallow copy, so the given map is never modified. func SanitizePluginOption(pluginName string, option map[string]interface{}) (map[string]interface{}, error) { if sanitizer, ok := pluginOptionSanitizers[pluginName]; ok { + option = maps.Clone(option) sanitizer(option) } return option, nil