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 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
28 changes: 23 additions & 5 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 @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to replace this block of looping with a call to findProjectInConfig so the tests now measure real code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@motatoes updated

for _, config := range diggerConfig.Projects {
if config.Name == project {
projectConfig = config
projectFound = true
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for _, config := range diggerConfig.Projects {
if config.Name == project {
projectConfig = config
projectFound = true
break
}
}
projectConfig, projectFound := findProjectInConfig(diggerConfig, project)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@motatoes updated


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)
Expand Down
192 changes: 192 additions & 0 deletions cli/pkg/github/github_manual_mode_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
Loading