From d87f8c11f3483917e5c6f8ce3d2e35a827770f6f Mon Sep 17 00:00:00 2001 From: Hugo Samayoa Date: Fri, 4 Jul 2025 11:39:20 -0700 Subject: [PATCH 1/5] fix: critical security bug in manual mode project selection --- cli/pkg/github/github.go | 28 +++- cli/pkg/github/github_manual_mode_test.go | 192 ++++++++++++++++++++++ 2 files changed, 215 insertions(+), 5 deletions(-) create mode 100644 cli/pkg/github/github_manual_mode_test.go diff --git a/cli/pkg/github/github.go b/cli/pkg/github/github.go index 0ce2ad8e8..3723fe2e0 100644 --- a/cli/pkg/github/github.go +++ b/cli/pkg/github/github.go @@ -3,6 +3,10 @@ package github import ( "errors" "fmt" + "log/slog" + "os" + "strings" + "github.com/diggerhq/digger/cli/pkg/digger" "github.com/diggerhq/digger/cli/pkg/drift" github_models "github.com/diggerhq/digger/cli/pkg/github/models" @@ -21,9 +25,6 @@ import ( "github.com/google/go-github/v61/github" "github.com/samber/lo" "gopkg.in/yaml.v3" - "log/slog" - "os" - "strings" ) func initLogger() { @@ -146,11 +147,28 @@ func GitHubCI(lock core_locking.Lock, policyCheckerProvider core_policy.PolicyCh } var projectConfig digger_config.Project - for _, projectConfig = range diggerConfig.Projects { - if projectConfig.Name == project { + var projectFound bool + for _, config := range diggerConfig.Projects { + if config.Name == project { + projectConfig = config + projectFound = true break } } + + if !projectFound { + // Log available projects to help with debugging + var availableProjects []string + for _, p := range diggerConfig.Projects { + availableProjects = append(availableProjects, p.Name) + } + slog.Error("Project not found in digger configuration", + "requestedProject", project, + "availableProjects", availableProjects) + usage.ReportErrorAndExit(githubActor, fmt.Sprintf("Project '%s' not found in digger configuration. Available projects: %v", project, availableProjects), 1) + } + + slog.Info("Found project configuration", "projectName", projectConfig.Name, "projectDir", projectConfig.Dir) workflow := diggerConfig.Workflows[projectConfig.Workflow] stateEnvVars, commandEnvVars := digger_config.CollectTerraformEnvConfig(workflow.EnvVars, true) diff --git a/cli/pkg/github/github_manual_mode_test.go b/cli/pkg/github/github_manual_mode_test.go new file mode 100644 index 000000000..e09f8bdb0 --- /dev/null +++ b/cli/pkg/github/github_manual_mode_test.go @@ -0,0 +1,192 @@ +package github + +import ( + "testing" + + "github.com/diggerhq/digger/libs/digger_config" + "github.com/stretchr/testify/assert" +) + +func TestManualModeProjectValidation(t *testing.T) { + // Create a test digger config with some projects + diggerConfig := &digger_config.DiggerConfig{ + Projects: []digger_config.Project{ + { + Name: "project-a", + Dir: "./project-a", + }, + { + Name: "project-b", + Dir: "./project-b", + }, + { + Name: "project-c", + Dir: "./project-c", + }, + }, + } + + tests := []struct { + name string + requestedProject string + shouldFind bool + expectedProject string + }{ + { + name: "Valid project should be found", + requestedProject: "project-b", + shouldFind: true, + expectedProject: "project-b", + }, + { + name: "Non-existent project should not be found", + requestedProject: "non-existent-project", + shouldFind: false, + expectedProject: "", + }, + { + name: "Empty project name should not be found", + requestedProject: "", + shouldFind: false, + expectedProject: "", + }, + { + name: "Case sensitive project name should not be found", + requestedProject: "Project-A", + shouldFind: false, + expectedProject: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Simulate the fixed project selection logic + var projectConfig digger_config.Project + var projectFound bool + + for _, config := range diggerConfig.Projects { + if config.Name == tt.requestedProject { + projectConfig = config + projectFound = true + break + } + } + + if tt.shouldFind { + assert.True(t, projectFound, "Expected to find project %s", tt.requestedProject) + assert.Equal(t, tt.expectedProject, projectConfig.Name, "Expected project name to match") + } else { + assert.False(t, projectFound, "Expected NOT to find project %s", tt.requestedProject) + // In the old buggy code, projectConfig would contain the last project from the loop + // With our fix, projectFound will be false and we should exit with an error + } + }) + } +} + +func TestManualModeProjectValidationWithOldBuggyLogic(t *testing.T) { + // This test demonstrates the old buggy behavior + diggerConfig := &digger_config.DiggerConfig{ + Projects: []digger_config.Project{ + { + Name: "project-a", + Dir: "./project-a", + }, + { + Name: "project-b", + Dir: "./project-b", + }, + { + Name: "dangerous-project", + Dir: "./dangerous-project", + }, + }, + } + + // Simulate the OLD buggy logic + requestedProject := "non-existent-project" + var projectConfig digger_config.Project + + // This is the OLD BUGGY CODE that would cause the issue + for _, projectConfig = range diggerConfig.Projects { + if projectConfig.Name == requestedProject { + break + } + } + + // In the old code, projectConfig would now contain "dangerous-project" + // (the last project in the loop) even though we requested "non-existent-project" + assert.Equal(t, "dangerous-project", projectConfig.Name, + "This demonstrates the old bug: projectConfig contains the last project from the loop") + assert.NotEqual(t, requestedProject, projectConfig.Name, + "This shows the dangerous behavior: we're using a different project than requested") +} + +func TestAvailableProjectsLogging(t *testing.T) { + // Test that we properly log available projects when a project is not found + diggerConfig := &digger_config.DiggerConfig{ + Projects: []digger_config.Project{ + {Name: "web-app"}, + {Name: "api-service"}, + {Name: "database"}, + }, + } + + requestedProject := "missing-project" + var projectFound bool + var availableProjects []string + + // Simulate the project search + for _, config := range diggerConfig.Projects { + if config.Name == requestedProject { + projectFound = true + break + } + } + + if !projectFound { + // Collect available projects for error message + for _, p := range diggerConfig.Projects { + availableProjects = append(availableProjects, p.Name) + } + } + + assert.False(t, projectFound) + assert.Equal(t, []string{"web-app", "api-service", "database"}, availableProjects) +} + +// This test would actually call usage.ReportErrorAndExit in a real scenario +// but we can't easily test that without mocking the exit behavior +func TestProjectNotFoundErrorMessage(t *testing.T) { + diggerConfig := &digger_config.DiggerConfig{ + Projects: []digger_config.Project{ + {Name: "project-1"}, + {Name: "project-2"}, + }, + } + + requestedProject := "invalid-project" + var projectFound bool + var availableProjects []string + + for _, config := range diggerConfig.Projects { + if config.Name == requestedProject { + projectFound = true + break + } + } + + if !projectFound { + for _, p := range diggerConfig.Projects { + availableProjects = append(availableProjects, p.Name) + } + + // This would normally call usage.ReportErrorAndExit + expectedErrorMsg := "Project 'invalid-project' not found in digger configuration. Available projects: [project-1 project-2]" + + // Verify the error message format + actualErrorMsg := "Project '" + requestedProject + "' not found in digger configuration. Available projects: " + + "[project-1 project-2]" + assert.Equal(t, expectedErrorMsg, actualErrorMsg) + } +} From 449a2ca3fc5d15033379ed6762363917aefcb921 Mon Sep 17 00:00:00 2001 From: Hugo Samayoa Date: Fri, 4 Jul 2025 11:53:33 -0700 Subject: [PATCH 2/5] Add greptile suggestions --- cli/pkg/github/github.go | 5 +- cli/pkg/github/github_manual_mode_test.go | 104 +++++++++++++--------- 2 files changed, 66 insertions(+), 43 deletions(-) diff --git a/cli/pkg/github/github.go b/cli/pkg/github/github.go index 3723fe2e0..d5eed781e 100644 --- a/cli/pkg/github/github.go +++ b/cli/pkg/github/github.go @@ -169,7 +169,10 @@ func GitHubCI(lock core_locking.Lock, policyCheckerProvider core_policy.PolicyCh } slog.Info("Found project configuration", "projectName", projectConfig.Name, "projectDir", projectConfig.Dir) - workflow := diggerConfig.Workflows[projectConfig.Workflow] + workflow, ok := diggerConfig.Workflows[projectConfig.Workflow] + if !ok { + usage.ReportErrorAndExit(githubActor, fmt.Sprintf("Workflow '%s' not found for project '%s'", projectConfig.Workflow, projectConfig.Name), 1) + } stateEnvVars, commandEnvVars := digger_config.CollectTerraformEnvConfig(workflow.EnvVars, true) diff --git a/cli/pkg/github/github_manual_mode_test.go b/cli/pkg/github/github_manual_mode_test.go index e09f8bdb0..5de224234 100644 --- a/cli/pkg/github/github_manual_mode_test.go +++ b/cli/pkg/github/github_manual_mode_test.go @@ -1,12 +1,32 @@ package github import ( + "fmt" "testing" "github.com/diggerhq/digger/libs/digger_config" "github.com/stretchr/testify/assert" ) +// Helper function to search for a project in the configuration +func findProjectInConfig(projects []digger_config.Project, projectName string) (digger_config.Project, bool) { + for _, config := range projects { + if config.Name == projectName { + return config, true + } + } + return digger_config.Project{}, false +} + +// Helper function to get available project names +func getAvailableProjectNames(projects []digger_config.Project) []string { + var availableProjects []string + for _, p := range projects { + availableProjects = append(availableProjects, p.Name) + } + return availableProjects +} + func TestManualModeProjectValidation(t *testing.T) { // Create a test digger config with some projects diggerConfig := &digger_config.DiggerConfig{ @@ -60,21 +80,13 @@ func TestManualModeProjectValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Simulate the fixed project selection logic - var projectConfig digger_config.Project - var projectFound bool - - for _, config := range diggerConfig.Projects { - if config.Name == tt.requestedProject { - projectConfig = config - projectFound = true - break - } - } + // Use helper function for project search + projectConfig, projectFound := findProjectInConfig(diggerConfig.Projects, tt.requestedProject) if tt.shouldFind { assert.True(t, projectFound, "Expected to find project %s", tt.requestedProject) assert.Equal(t, tt.expectedProject, projectConfig.Name, "Expected project name to match") + assert.Equal(t, "./"+tt.expectedProject, projectConfig.Dir, "Expected project directory to match") } else { assert.False(t, projectFound, "Expected NOT to find project %s", tt.requestedProject) // In the old buggy code, projectConfig would contain the last project from the loop @@ -133,23 +145,10 @@ func TestAvailableProjectsLogging(t *testing.T) { } requestedProject := "missing-project" - var projectFound bool - var availableProjects []string - // Simulate the project search - for _, config := range diggerConfig.Projects { - if config.Name == requestedProject { - projectFound = true - break - } - } - - if !projectFound { - // Collect available projects for error message - for _, p := range diggerConfig.Projects { - availableProjects = append(availableProjects, p.Name) - } - } + // Use helper functions + _, projectFound := findProjectInConfig(diggerConfig.Projects, requestedProject) + availableProjects := getAvailableProjectNames(diggerConfig.Projects) assert.False(t, projectFound) assert.Equal(t, []string{"web-app", "api-service", "database"}, availableProjects) @@ -166,27 +165,48 @@ func TestProjectNotFoundErrorMessage(t *testing.T) { } requestedProject := "invalid-project" - var projectFound bool - var availableProjects []string - for _, config := range diggerConfig.Projects { - if config.Name == requestedProject { - projectFound = true - break - } - } + // Use helper functions + _, projectFound := findProjectInConfig(diggerConfig.Projects, requestedProject) + availableProjects := getAvailableProjectNames(diggerConfig.Projects) - if !projectFound { - for _, p := range diggerConfig.Projects { - availableProjects = append(availableProjects, p.Name) - } + assert.False(t, projectFound) + if !projectFound { // This would normally call usage.ReportErrorAndExit expectedErrorMsg := "Project 'invalid-project' not found in digger configuration. Available projects: [project-1 project-2]" - // Verify the error message format - actualErrorMsg := "Project '" + requestedProject + "' not found in digger configuration. Available projects: " + - "[project-1 project-2]" + // Verify the error message format using fmt.Sprintf for better maintainability + actualErrorMsg := fmt.Sprintf("Project '%s' not found in digger configuration. Available projects: %v", requestedProject, availableProjects) assert.Equal(t, expectedErrorMsg, actualErrorMsg) + assert.Equal(t, []string{"project-1", "project-2"}, availableProjects) + } +} + +func TestWorkflowValidation(t *testing.T) { + // Test that we properly validate workflow existence + diggerConfig := &digger_config.DiggerConfig{ + Projects: []digger_config.Project{ + { + Name: "test-project", + Workflow: "custom-workflow", + }, + }, + Workflows: map[string]digger_config.Workflow{ + "default": { + Plan: &digger_config.Stage{Steps: []digger_config.Step{{Action: "init"}}}, + Apply: &digger_config.Stage{Steps: []digger_config.Step{{Action: "apply"}}}, + }, + }, } + + // Test valid workflow + project := diggerConfig.Projects[0] + _, workflowExists := diggerConfig.Workflows[project.Workflow] + assert.False(t, workflowExists, "custom-workflow should not exist") + + // Test workflow that exists + project.Workflow = "default" + _, workflowExists = diggerConfig.Workflows[project.Workflow] + assert.True(t, workflowExists, "default workflow should exist") } From 93c70cd4edada4f91f6c948b13f98c110f8a6a77 Mon Sep 17 00:00:00 2001 From: Hugo Samayoa Date: Wed, 16 Jul 2025 18:49:29 -0700 Subject: [PATCH 3/5] move findProjectInConfig to main source --- cli/pkg/github/github.go | 20 +++++++++++--------- cli/pkg/github/github_manual_mode_test.go | 10 ---------- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/cli/pkg/github/github.go b/cli/pkg/github/github.go index d5eed781e..ad2ae09ff 100644 --- a/cli/pkg/github/github.go +++ b/cli/pkg/github/github.go @@ -146,15 +146,7 @@ func GitHubCI(lock core_locking.Lock, policyCheckerProvider core_policy.PolicyCh usage.ReportErrorAndExit(githubActor, "provide 'project' to run in 'manual' mode", 2) } - var projectConfig digger_config.Project - var projectFound bool - for _, config := range diggerConfig.Projects { - if config.Name == project { - projectConfig = config - projectFound = true - break - } - } + projectConfig, projectFound := findProjectInConfig(diggerConfig.Projects, project) if !projectFound { // Log available projects to help with debugging @@ -371,6 +363,16 @@ func GitHubCI(lock core_locking.Lock, policyCheckerProvider core_policy.PolicyCh usage.ReportErrorAndExit(githubActor, "Digger finished successfully", 0) } +// Helper function to search for a project in the configuration +func findProjectInConfig(projects []digger_config.Project, projectName string) (digger_config.Project, bool) { + for _, config := range projects { + if config.Name == projectName { + return config, true + } + } + return digger_config.Project{}, false +} + func logCommands(projectCommands []scheduler.Job) { logMessage := fmt.Sprintf("Following commands are going to be executed:\n") for _, pc := range projectCommands { diff --git a/cli/pkg/github/github_manual_mode_test.go b/cli/pkg/github/github_manual_mode_test.go index 5de224234..dd2cc2fc4 100644 --- a/cli/pkg/github/github_manual_mode_test.go +++ b/cli/pkg/github/github_manual_mode_test.go @@ -8,16 +8,6 @@ import ( "github.com/stretchr/testify/assert" ) -// Helper function to search for a project in the configuration -func findProjectInConfig(projects []digger_config.Project, projectName string) (digger_config.Project, bool) { - for _, config := range projects { - if config.Name == projectName { - return config, true - } - } - return digger_config.Project{}, false -} - // Helper function to get available project names func getAvailableProjectNames(projects []digger_config.Project) []string { var availableProjects []string From fde6294759dc632fad740c3bc2392fd0473e9d21 Mon Sep 17 00:00:00 2001 From: Hugo Samayoa Date: Thu, 24 Jul 2025 08:06:56 -0700 Subject: [PATCH 4/5] Update cli/pkg/github/github_manual_mode_test.go Co-authored-by: bismuthdev[bot] <177057995+bismuthdev[bot]@users.noreply.github.com> --- cli/pkg/github/github_manual_mode_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cli/pkg/github/github_manual_mode_test.go b/cli/pkg/github/github_manual_mode_test.go index dd2cc2fc4..1685e8338 100644 --- a/cli/pkg/github/github_manual_mode_test.go +++ b/cli/pkg/github/github_manual_mode_test.go @@ -190,10 +190,15 @@ func TestWorkflowValidation(t *testing.T) { }, } - // Test valid workflow + // Test invalid workflow project := diggerConfig.Projects[0] _, workflowExists := diggerConfig.Workflows[project.Workflow] assert.False(t, workflowExists, "custom-workflow should not exist") + + // Test that the error message is correctly formatted + expectedErrorMsg := fmt.Sprintf("Workflow '%s' not found for project '%s'", project.Workflow, project.Name) + actualErrorMsg := fmt.Sprintf("Workflow '%s' not found for project '%s'", project.Workflow, project.Name) + assert.Equal(t, expectedErrorMsg, actualErrorMsg) // Test workflow that exists project.Workflow = "default" From 506b6008ad5d7f6e8d1091766a011bafc7599449 Mon Sep 17 00:00:00 2001 From: Hugo Samayoa Date: Fri, 25 Jul 2025 10:44:23 -0700 Subject: [PATCH 5/5] Update cli/pkg/github/github.go Co-authored-by: bismuthdev[bot] <177057995+bismuthdev[bot]@users.noreply.github.com> --- cli/pkg/github/github.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/pkg/github/github.go b/cli/pkg/github/github.go index ad2ae09ff..55abb4489 100644 --- a/cli/pkg/github/github.go +++ b/cli/pkg/github/github.go @@ -146,7 +146,7 @@ func GitHubCI(lock core_locking.Lock, policyCheckerProvider core_policy.PolicyCh usage.ReportErrorAndExit(githubActor, "provide 'project' to run in 'manual' mode", 2) } - projectConfig, projectFound := findProjectInConfig(diggerConfig.Projects, project) + projectConfig, projectFound := findProjectInConfig(diggerConfig.Projects, project) if !projectFound { // Log available projects to help with debugging