Skip to content

fix: critical security bug in manual mode project selection #2009

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 31 additions & 8 deletions cli/pkg/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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() {
Expand Down Expand Up @@ -145,13 +146,25 @@ 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
for _, projectConfig = range diggerConfig.Projects {
if projectConfig.Name == project {
break
projectConfig, projectFound := findProjectInConfig(diggerConfig.Projects, project)

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, ok := diggerConfig.Workflows[projectConfig.Workflow]
if !ok {
usage.ReportErrorAndExit(githubActor, fmt.Sprintf("Workflow '%s' not found for project '%s'", projectConfig.Workflow, projectConfig.Name), 1)
}
workflow := diggerConfig.Workflows[projectConfig.Workflow]

stateEnvVars, commandEnvVars := digger_config.CollectTerraformEnvConfig(workflow.EnvVars, true)

Expand Down Expand Up @@ -350,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 {
Expand Down
202 changes: 202 additions & 0 deletions cli/pkg/github/github_manual_mode_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
package github

import (
"fmt"
"testing"

"github.com/diggerhq/digger/libs/digger_config"
"github.com/stretchr/testify/assert"
)

// 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{
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) {
// 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
// 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"

// 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)
}

// 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"

// Use helper functions
_, projectFound := findProjectInConfig(diggerConfig.Projects, requestedProject)
availableProjects := getAvailableProjectNames(diggerConfig.Projects)

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 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")
Comment on lines +193 to +196
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TestWorkflowValidation test in github_manual_mode_test.go only checks that the workflow doesn't exist, but doesn't verify that the error message is correctly formatted or that the error handling works as expected.

In the main code at line 166 of github.go, when a workflow is not found, it calls usage.ReportErrorAndExit with a formatted error message. The test should verify that this error message is correctly formatted to ensure that the error handling is working properly.

The fix adds a test that verifies the error message format matches what's expected in the main code, similar to how TestProjectNotFoundErrorMessage tests the project not found error message.

Suggested change
// Test valid workflow
project := diggerConfig.Projects[0]
_, workflowExists := diggerConfig.Workflows[project.Workflow]
assert.False(t, workflowExists, "custom-workflow should not exist")
// 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"
_, workflowExists = diggerConfig.Workflows[project.Workflow]
assert.True(t, workflowExists, "default workflow should exist")
}
Loading