Read config and .codeowners from base ref instead of head

README.md

@@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better
 
 [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1)
 [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml)
-![Coverage](https://img.shields.io/badge/Coverage-81.7%25-brightgreen)
+![Coverage](https://img.shields.io/badge/Coverage-81.8%25-brightgreen)
 [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause)
 [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md)
 

internal/app/app.go

@@ -106,8 +106,13 @@ func (a *App) Run() (*OutputData, error) {
 	}
 	a.printDebug("PR: %d\n", a.client.PR().GetNumber())
 
-	// Read config
-	conf, err := owners.ReadConfig(a.config.RepoDir)
+	// Create file reader for base ref to prevent PR authors from modifying config or .codeowners
+	// This ensures the security policy comes from the protected branch, not the PR branch
+	fileReader := git.NewGitRefFileReader(a.client.PR().Base.GetSHA(), a.config.RepoDir)
+	a.printDebug("Using base ref %s for codeowners.toml and .codeowners files\n", a.client.PR().Base.GetSHA())
+
+	// Read config from base ref
+	conf, err := owners.ReadConfig(a.config.RepoDir, fileReader)
 	if err != nil {
 		a.printWarn("Error reading codeowners.toml - using default config\n")
 	}
@@ -129,8 +134,8 @@ func (a *App) Run() (*OutputData, error) {
 	}
 	a.gitDiff = gitDiff
 
-	// Initialize codeowners
-	codeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), a.config.WarningBuffer)
+	// Initialize codeowners using base ref file reader (same reader as config)
+	codeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), fileReader, a.config.WarningBuffer)
 	if err != nil {
 		return &OutputData{}, fmt.Errorf("NewCodeOwners Error: %v", err)
 	}
@@ -402,10 +407,10 @@ func (a *App) processTokenOwnerApproval() (*gh.CurrentApproval, error) {
 
 func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) {
 	fileReviewers := f.MapMap(a.codeowners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string { return reviewers.Flatten() })
-	
+
 	var approvers []string
 	var approvalsToDismiss []*gh.CurrentApproval
-	
+
 	if a.Conf.DisableSmartDismissal {
 		// Smart dismissal is disabled - treat all approvals as valid
 		a.printDebug("Smart dismissal disabled - keeping all approvals\n")
@@ -417,7 +422,7 @@ func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) {
 		// Normal smart dismissal logic
 		approvers, approvalsToDismiss = a.client.CheckApprovals(fileReviewers, ghApprovals, a.gitDiff)
 	}
-	
+
 	a.codeowners.ApplyApprovals(approvers)
 
 	if len(approvalsToDismiss) > 0 {

internal/config/config.go

@@ -1,23 +1,22 @@
 package owners
 
 import (
-	"errors"
-	"os"
 	"strings"
 
+	"github.com/multimediallc/codeowners-plus/pkg/codeowners"
 	"github.com/pelletier/go-toml/v2"
 )
 
 type Config struct {
-	MaxReviews           *int         `toml:"max_reviews"`
-	MinReviews           *int         `toml:"min_reviews"`
-	UnskippableReviewers []string     `toml:"unskippable_reviewers"`
-	Ignore               []string     `toml:"ignore"`
-	Enforcement          *Enforcement `toml:"enforcement"`
-	HighPriorityLabels   []string     `toml:"high_priority_labels"`
-	AdminBypass          *AdminBypass `toml:"admin_bypass"`
-	DetailedReviewers    bool         `toml:"detailed_reviewers"`
-	DisableSmartDismissal bool        `toml:"disable_smart_dismissal"`
+	MaxReviews            *int         `toml:"max_reviews"`
+	MinReviews            *int         `toml:"min_reviews"`
+	UnskippableReviewers  []string     `toml:"unskippable_reviewers"`
+	Ignore                []string     `toml:"ignore"`
+	Enforcement           *Enforcement `toml:"enforcement"`
+	HighPriorityLabels    []string     `toml:"high_priority_labels"`
+	AdminBypass           *AdminBypass `toml:"admin_bypass"`
+	DetailedReviewers     bool         `toml:"detailed_reviewers"`
+	DisableSmartDismissal bool         `toml:"disable_smart_dismissal"`
 }
 
 type Enforcement struct {
@@ -26,11 +25,11 @@ type Enforcement struct {
 }
 
 type AdminBypass struct {
-	Enabled       bool     `toml:"enabled"`
-	AllowedUsers  []string `toml:"allowed_users"`
+	Enabled      bool     `toml:"enabled"`
+	AllowedUsers []string `toml:"allowed_users"`
 }
 
-func ReadConfig(path string) (*Config, error) {
+func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) {
 	if !strings.HasSuffix(path, "/") {
 		path += "/"
 	}
@@ -46,11 +45,17 @@ func ReadConfig(path string) (*Config, error) {
 		DetailedReviewers:    false,
 	}
 
+	// Use filesystem reader if none provided
+	if fileReader == nil {
+		fileReader = &codeowners.FilesystemReader{}
+	}
+
 	fileName := path + "codeowners.toml"
-	if _, err := os.Stat(fileName); errors.Is(err, os.ErrNotExist) {
+
+	if !fileReader.PathExists(fileName) {
 		return defaultConfig, nil
 	}
-	file, err := os.ReadFile(fileName)
+	file, err := fileReader.ReadFile(fileName)
 	if err != nil {
 		return defaultConfig, err
 	}

internal/config/config_git_ref_test.go

@@ -0,0 +1,155 @@
+package owners
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+)
+
+type mockConfigFileReader struct {
+	files map[string]string
+}
+
+func (m *mockConfigFileReader) ReadFile(path string) ([]byte, error) {
+	content, ok := m.files[path]
+	if !ok {
+		return nil, fmt.Errorf("file not found: %s", path)
+	}
+	return []byte(content), nil
+}
+
+func (m *mockConfigFileReader) PathExists(path string) bool {
+	_, ok := m.files[path]
+	return ok
+}
+
+func TestReadConfigFromGitRef(t *testing.T) {
+	// Simulate reading config from a git ref
+	mockReader := &mockConfigFileReader{
+		files: map[string]string{
+			"test/repo/codeowners.toml": `
+max_reviews = 3
+min_reviews = 2
+unskippable_reviewers = ["@admin"]
+
+[enforcement]
+approval = true
+fail_check = false
+`,
+		},
+	}
+
+	config, err := ReadConfig("test/repo", mockReader)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if config.MaxReviews == nil || *config.MaxReviews != 3 {
+		t.Errorf("expected max_reviews = 3, got %v", config.MaxReviews)
+	}
+
+	if config.MinReviews == nil || *config.MinReviews != 2 {
+		t.Errorf("expected min_reviews = 2, got %v", config.MinReviews)
+	}
+
+	if len(config.UnskippableReviewers) != 1 || config.UnskippableReviewers[0] != "@admin" {
+		t.Errorf("expected unskippable_reviewers = [@admin], got %v", config.UnskippableReviewers)
+	}
+
+	if !config.Enforcement.Approval {
+		t.Error("expected enforcement.approval = true")
+	}
+
+	if config.Enforcement.FailCheck {
+		t.Error("expected enforcement.fail_check = false")
+	}
+}
+
+func TestReadConfigFromGitRefNotFound(t *testing.T) {
+	// Simulate config file not existing in git ref
+	mockReader := &mockConfigFileReader{
+		files: map[string]string{},
+	}
+
+	config, err := ReadConfig("test/repo", mockReader)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// Should return default config
+	if config.MaxReviews != nil {
+		t.Error("expected nil max_reviews for default config")
+	}
+
+	if config.MinReviews != nil {
+		t.Error("expected nil min_reviews for default config")
+	}
+
+	if config.Enforcement.Approval {
+		t.Error("expected enforcement.approval = false for default config")
+	}
+
+	if !config.Enforcement.FailCheck {
+		t.Error("expected enforcement.fail_check = true for default config")
+	}
+}
+
+func TestReadConfigFromGitRefInvalidToml(t *testing.T) {
+	// Simulate invalid TOML content
+	mockReader := &mockConfigFileReader{
+		files: map[string]string{
+			"test/repo/codeowners.toml": "invalid toml [[[",
+		},
+	}
+
+	_, err := ReadConfig("test/repo", mockReader)
+	if err == nil {
+		t.Error("expected error for invalid TOML")
+	}
+
+	if !strings.Contains(err.Error(), "toml") && !strings.Contains(err.Error(), "parse") {
+		t.Errorf("expected TOML parse error, got: %v", err)
+	}
+}
+
+func TestReadConfigUsesFilesystemWhenNilReader(t *testing.T) {
+	// When nil reader is passed, it should use filesystem reader
+	// This test just ensures the fallback logic works
+	config, err := ReadConfig("../../test_project", nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	// The test_project directory might not have a codeowners.toml
+	// In that case, we should get default config
+	if config.Enforcement == nil {
+		t.Error("expected enforcement config to be initialized")
+	}
+}
+
+// TestConfigSecurityIsolation verifies that even if filesystem has different config,
+// the git ref reader returns the correct config from the ref
+func TestConfigSecurityIsolation(t *testing.T) {
+	// This simulates the security scenario where:
+	// - Base ref has max_reviews = 5
+	// - PR branch (filesystem) has max_reviews = 1 (trying to bypass)
+	// - We should only see the base ref config (max_reviews = 5)
+
+	mockReader := &mockConfigFileReader{
+		files: map[string]string{
+			"test/repo/codeowners.toml": "max_reviews = 5",
+		},
+	}
+
+	config, err := ReadConfig("test/repo", mockReader)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if config.MaxReviews == nil || *config.MaxReviews != 5 {
+		t.Errorf("expected max_reviews = 5 from base ref, got %v", config.MaxReviews)
+	}
+
+	// Even if filesystem has different value, we're reading from git ref
+	// so we should only see the base ref value
+}

internal/config/config_test.go

@@ -100,7 +100,7 @@ max_reviews = invalid
 			// Test with and without trailing slash
 			paths := []string{configPath, configPath + "/"}
 			for _, path := range paths {
-				got, err := ReadConfig(path)
+				got, err := ReadConfig(path, nil)
 				if tc.expectedErr {
 					if err == nil {
 						t.Error("expected error but got none")
@@ -176,7 +176,7 @@ func TestReadConfigFileError(t *testing.T) {
 	}
 
 	// Try to read config from directory with no permissions
-	_, err = ReadConfig(configPath)
+	_, err = ReadConfig(configPath, nil)
 	if err == nil {
 		t.Error("expected error when reading from directory with no permissions")
 	}

internal/git/file_reader.go

@@ -0,0 +1,59 @@
+package git
+
+import (
+	"fmt"
+	"strings"
+)
+
+// GitRefFileReader reads files from a specific git ref
+type GitRefFileReader struct {
+	ref      string
+	dir      string
+	executor gitCommandExecutor
+}
+
+// NewGitRefFileReader creates a new GitRefFileReader for reading files from a git ref
+func NewGitRefFileReader(ref string, dir string) *GitRefFileReader {
+	return &GitRefFileReader{
+		ref:      ref,
+		dir:      dir,
+		executor: newRealGitExecutor(dir),
+	}
+}
+
+// ReadFile reads a file from the git ref
+func (r *GitRefFileReader) ReadFile(path string) ([]byte, error) {
+	// Normalize path - make it relative to the repository root
+	path = r.normalizePathForGit(path)
+
+	// Use git show to read the file from the ref
+	output, err := r.executor.execute("git", "show", fmt.Sprintf("%s:%s", r.ref, path))
+	if err != nil {
+		return nil, fmt.Errorf("failed to read file %s from ref %s: %w", path, r.ref, err)
+	}
+	return output, nil
+}
+
+// PathExists checks if a file exists in the git ref
+func (r *GitRefFileReader) PathExists(path string) bool {
+	// Normalize path - make it relative to the repository root
+	path = r.normalizePathForGit(path)
+
+	// Use git cat-file to check if the file exists
+	_, err := r.executor.execute("git", "cat-file", "-e", fmt.Sprintf("%s:%s", r.ref, path))
+	return err == nil
+}
+
+// normalizePathForGit converts an absolute filesystem path to a path relative to the repository root
+func (r *GitRefFileReader) normalizePathForGit(path string) string {
+	// We want to remove the path to the dir Root
+	dir_prefix := r.dir
+	if !strings.HasSuffix(dir_prefix, "/") {
+		dir_prefix = dir_prefix + "/"
+	}
+	path = strings.TrimPrefix(path, dir_prefix)
+	if strings.HasPrefix(path, "/") {
+		return path[1:]
+	}
+	return path
+}