Skip to content

Commit 2b3e4b6

Browse files
committed
Read from the base ref using git instead of filesystem
1 parent ec7642b commit 2b3e4b6

File tree

13 files changed

+447
-61
lines changed

13 files changed

+447
-61
lines changed

.github/workflows/codeowners.yml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ jobs:
2323
- name: 'Checkout Code Repository'
2424
uses: actions/checkout@v5
2525
with:
26-
ref: ${{ github.base_ref }}
2726
fetch-depth: 0
2827

2928
- name: 'Update action.yml to build locally'

README.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ jobs:
8989
- name: 'Checkout Code Repository'
9090
uses: actions/checkout@v4
9191
with:
92-
ref: ${{ github.base_ref }} # use the base_ref to prevent bypass by PR author
9392
fetch-depth: 0
9493

9594
- name: 'Codeowners Plus'

internal/app/app.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,13 @@ func (a *App) Run() (*OutputData, error) {
106106
}
107107
a.printDebug("PR: %d\n", a.client.PR().GetNumber())
108108

109-
// Read config
110-
conf, err := owners.ReadConfig(a.config.RepoDir)
109+
// Create file reader for base ref to prevent PR authors from modifying config or .codeowners
110+
// This ensures the security policy comes from the protected branch, not the PR branch
111+
fileReader := git.NewGitRefFileReader(a.client.PR().Base.GetSHA(), a.config.RepoDir)
112+
a.printDebug("Using base ref %s for codeowners.toml and .codeowners files\n", a.client.PR().Base.GetSHA())
113+
114+
// Read config from base ref
115+
conf, err := owners.ReadConfig(a.config.RepoDir, fileReader)
111116
if err != nil {
112117
a.printWarn("Error reading codeowners.toml - using default config\n")
113118
}
@@ -129,8 +134,8 @@ func (a *App) Run() (*OutputData, error) {
129134
}
130135
a.gitDiff = gitDiff
131136

132-
// Initialize codeowners
133-
codeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), a.config.WarningBuffer)
137+
// Initialize codeowners using base ref file reader (same reader as config)
138+
codeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), fileReader, a.config.WarningBuffer)
134139
if err != nil {
135140
return &OutputData{}, fmt.Errorf("NewCodeOwners Error: %v", err)
136141
}
@@ -402,10 +407,10 @@ func (a *App) processTokenOwnerApproval() (*gh.CurrentApproval, error) {
402407

403408
func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) {
404409
fileReviewers := f.MapMap(a.codeowners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string { return reviewers.Flatten() })
405-
410+
406411
var approvers []string
407412
var approvalsToDismiss []*gh.CurrentApproval
408-
413+
409414
if a.Conf.DisableSmartDismissal {
410415
// Smart dismissal is disabled - treat all approvals as valid
411416
a.printDebug("Smart dismissal disabled - keeping all approvals\n")
@@ -417,7 +422,7 @@ func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) {
417422
// Normal smart dismissal logic
418423
approvers, approvalsToDismiss = a.client.CheckApprovals(fileReviewers, ghApprovals, a.gitDiff)
419424
}
420-
425+
421426
a.codeowners.ApplyApprovals(approvers)
422427

423428
if len(approvalsToDismiss) > 0 {

internal/config/config.go

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,22 @@
11
package owners
22

33
import (
4-
"errors"
5-
"os"
64
"strings"
75

6+
"github.com/multimediallc/codeowners-plus/pkg/codeowners"
87
"github.com/pelletier/go-toml/v2"
98
)
109

1110
type Config struct {
12-
MaxReviews *int `toml:"max_reviews"`
13-
MinReviews *int `toml:"min_reviews"`
14-
UnskippableReviewers []string `toml:"unskippable_reviewers"`
15-
Ignore []string `toml:"ignore"`
16-
Enforcement *Enforcement `toml:"enforcement"`
17-
HighPriorityLabels []string `toml:"high_priority_labels"`
18-
AdminBypass *AdminBypass `toml:"admin_bypass"`
19-
DetailedReviewers bool `toml:"detailed_reviewers"`
20-
DisableSmartDismissal bool `toml:"disable_smart_dismissal"`
11+
MaxReviews *int `toml:"max_reviews"`
12+
MinReviews *int `toml:"min_reviews"`
13+
UnskippableReviewers []string `toml:"unskippable_reviewers"`
14+
Ignore []string `toml:"ignore"`
15+
Enforcement *Enforcement `toml:"enforcement"`
16+
HighPriorityLabels []string `toml:"high_priority_labels"`
17+
AdminBypass *AdminBypass `toml:"admin_bypass"`
18+
DetailedReviewers bool `toml:"detailed_reviewers"`
19+
DisableSmartDismissal bool `toml:"disable_smart_dismissal"`
2120
}
2221

2322
type Enforcement struct {
@@ -26,11 +25,11 @@ type Enforcement struct {
2625
}
2726

2827
type AdminBypass struct {
29-
Enabled bool `toml:"enabled"`
30-
AllowedUsers []string `toml:"allowed_users"`
28+
Enabled bool `toml:"enabled"`
29+
AllowedUsers []string `toml:"allowed_users"`
3130
}
3231

33-
func ReadConfig(path string) (*Config, error) {
32+
func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) {
3433
if !strings.HasSuffix(path, "/") {
3534
path += "/"
3635
}
@@ -46,11 +45,17 @@ func ReadConfig(path string) (*Config, error) {
4645
DetailedReviewers: false,
4746
}
4847

48+
// Use filesystem reader if none provided
49+
if fileReader == nil {
50+
fileReader = &codeowners.FilesystemReader{}
51+
}
52+
4953
fileName := path + "codeowners.toml"
50-
if _, err := os.Stat(fileName); errors.Is(err, os.ErrNotExist) {
54+
55+
if !fileReader.PathExists(fileName) {
5156
return defaultConfig, nil
5257
}
53-
file, err := os.ReadFile(fileName)
58+
file, err := fileReader.ReadFile(fileName)
5459
if err != nil {
5560
return defaultConfig, err
5661
}
Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
package owners
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
"testing"
7+
)
8+
9+
type mockConfigFileReader struct {
10+
files map[string]string
11+
}
12+
13+
func (m *mockConfigFileReader) ReadFile(path string) ([]byte, error) {
14+
content, ok := m.files[path]
15+
if !ok {
16+
return nil, fmt.Errorf("file not found: %s", path)
17+
}
18+
return []byte(content), nil
19+
}
20+
21+
func (m *mockConfigFileReader) PathExists(path string) bool {
22+
_, ok := m.files[path]
23+
return ok
24+
}
25+
26+
func TestReadConfigFromGitRef(t *testing.T) {
27+
// Simulate reading config from a git ref
28+
mockReader := &mockConfigFileReader{
29+
files: map[string]string{
30+
"test/repo/codeowners.toml": `
31+
max_reviews = 3
32+
min_reviews = 2
33+
unskippable_reviewers = ["@admin"]
34+
35+
[enforcement]
36+
approval = true
37+
fail_check = false
38+
`,
39+
},
40+
}
41+
42+
config, err := ReadConfig("test/repo", mockReader)
43+
if err != nil {
44+
t.Fatalf("unexpected error: %v", err)
45+
}
46+
47+
if config.MaxReviews == nil || *config.MaxReviews != 3 {
48+
t.Errorf("expected max_reviews = 3, got %v", config.MaxReviews)
49+
}
50+
51+
if config.MinReviews == nil || *config.MinReviews != 2 {
52+
t.Errorf("expected min_reviews = 2, got %v", config.MinReviews)
53+
}
54+
55+
if len(config.UnskippableReviewers) != 1 || config.UnskippableReviewers[0] != "@admin" {
56+
t.Errorf("expected unskippable_reviewers = [@admin], got %v", config.UnskippableReviewers)
57+
}
58+
59+
if !config.Enforcement.Approval {
60+
t.Error("expected enforcement.approval = true")
61+
}
62+
63+
if config.Enforcement.FailCheck {
64+
t.Error("expected enforcement.fail_check = false")
65+
}
66+
}
67+
68+
func TestReadConfigFromGitRefNotFound(t *testing.T) {
69+
// Simulate config file not existing in git ref
70+
mockReader := &mockConfigFileReader{
71+
files: map[string]string{},
72+
}
73+
74+
config, err := ReadConfig("test/repo", mockReader)
75+
if err != nil {
76+
t.Fatalf("unexpected error: %v", err)
77+
}
78+
79+
// Should return default config
80+
if config.MaxReviews != nil {
81+
t.Error("expected nil max_reviews for default config")
82+
}
83+
84+
if config.MinReviews != nil {
85+
t.Error("expected nil min_reviews for default config")
86+
}
87+
88+
if config.Enforcement.Approval {
89+
t.Error("expected enforcement.approval = false for default config")
90+
}
91+
92+
if !config.Enforcement.FailCheck {
93+
t.Error("expected enforcement.fail_check = true for default config")
94+
}
95+
}
96+
97+
func TestReadConfigFromGitRefInvalidToml(t *testing.T) {
98+
// Simulate invalid TOML content
99+
mockReader := &mockConfigFileReader{
100+
files: map[string]string{
101+
"test/repo/codeowners.toml": "invalid toml [[[",
102+
},
103+
}
104+
105+
_, err := ReadConfig("test/repo", mockReader)
106+
if err == nil {
107+
t.Error("expected error for invalid TOML")
108+
}
109+
110+
if !strings.Contains(err.Error(), "toml") && !strings.Contains(err.Error(), "parse") {
111+
t.Errorf("expected TOML parse error, got: %v", err)
112+
}
113+
}
114+
115+
func TestReadConfigUsesFilesystemWhenNilReader(t *testing.T) {
116+
// When nil reader is passed, it should use filesystem reader
117+
// This test just ensures the fallback logic works
118+
config, err := ReadConfig("../../test_project", nil)
119+
if err != nil {
120+
t.Fatalf("unexpected error: %v", err)
121+
}
122+
123+
// The test_project directory might not have a codeowners.toml
124+
// In that case, we should get default config
125+
if config.Enforcement == nil {
126+
t.Error("expected enforcement config to be initialized")
127+
}
128+
}
129+
130+
// TestConfigSecurityIsolation verifies that even if filesystem has different config,
131+
// the git ref reader returns the correct config from the ref
132+
func TestConfigSecurityIsolation(t *testing.T) {
133+
// This simulates the security scenario where:
134+
// - Base ref has max_reviews = 5
135+
// - PR branch (filesystem) has max_reviews = 1 (trying to bypass)
136+
// - We should only see the base ref config (max_reviews = 5)
137+
138+
mockReader := &mockConfigFileReader{
139+
files: map[string]string{
140+
"test/repo/codeowners.toml": "max_reviews = 5",
141+
},
142+
}
143+
144+
config, err := ReadConfig("test/repo", mockReader)
145+
if err != nil {
146+
t.Fatalf("unexpected error: %v", err)
147+
}
148+
149+
if config.MaxReviews == nil || *config.MaxReviews != 5 {
150+
t.Errorf("expected max_reviews = 5 from base ref, got %v", config.MaxReviews)
151+
}
152+
153+
// Even if filesystem has different value, we're reading from git ref
154+
// so we should only see the base ref value
155+
}

internal/config/config_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ max_reviews = invalid
100100
// Test with and without trailing slash
101101
paths := []string{configPath, configPath + "/"}
102102
for _, path := range paths {
103-
got, err := ReadConfig(path)
103+
got, err := ReadConfig(path, nil)
104104
if tc.expectedErr {
105105
if err == nil {
106106
t.Error("expected error but got none")
@@ -176,7 +176,7 @@ func TestReadConfigFileError(t *testing.T) {
176176
}
177177

178178
// Try to read config from directory with no permissions
179-
_, err = ReadConfig(configPath)
179+
_, err = ReadConfig(configPath, nil)
180180
if err == nil {
181181
t.Error("expected error when reading from directory with no permissions")
182182
}

internal/git/file_reader.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package git
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
)
7+
8+
// GitRefFileReader reads files from a specific git ref
9+
type GitRefFileReader struct {
10+
ref string
11+
dir string
12+
executor gitCommandExecutor
13+
}
14+
15+
// NewGitRefFileReader creates a new GitRefFileReader for reading files from a git ref
16+
func NewGitRefFileReader(ref string, dir string) *GitRefFileReader {
17+
return &GitRefFileReader{
18+
ref: ref,
19+
dir: dir,
20+
executor: newRealGitExecutor(dir),
21+
}
22+
}
23+
24+
// ReadFile reads a file from the git ref
25+
func (r *GitRefFileReader) ReadFile(path string) ([]byte, error) {
26+
// Normalize path - remove leading slash if present
27+
path = strings.TrimPrefix(path, "/")
28+
29+
// Use git show to read the file from the ref
30+
output, err := r.executor.execute("git", "show", fmt.Sprintf("%s:%s", r.ref, path))
31+
if err != nil {
32+
return nil, fmt.Errorf("failed to read file %s from ref %s: %w", path, r.ref, err)
33+
}
34+
return output, nil
35+
}
36+
37+
// PathExists checks if a file exists in the git ref
38+
func (r *GitRefFileReader) PathExists(path string) bool {
39+
// Normalize path - remove leading slash if present
40+
path = strings.TrimPrefix(path, "/")
41+
42+
// Use git cat-file to check if the file exists
43+
_, err := r.executor.execute("git", "cat-file", "-e", fmt.Sprintf("%s:%s", r.ref, path))
44+
return err == nil
45+
}

0 commit comments

Comments
 (0)