feat: detect permissions from actions (#137)

* feat: detect permissions from actions * refactor(test): fix 25 SonarCloud issues by extracting test constants Resolved all SonarCloud code quality issues for PR #137: - Fixed 12 string duplication issues (S1192) - Fixed 13 naming convention issues (S100) Changes: - Centralized test constants in appconstants/test_constants.go * Added 9 parser test constants for YAML templates * Added 3 template test constants for paths and versions - Updated parser_test.go to use shared constants - Updated template_test.go to use shared constants - Renamed 13 test functions to camelCase (removed underscores) * chore: reduce code duplication * fix: implement cr fixes * chore: deduplication
2026-02-16 00:50:00 +00:00 · 2026-01-04 02:48:29 +02:00
parent 9534bf9e45
commit ce23f93b74
12 changed files with 1797 additions and 170 deletions
--- a/internal/parser_test.go
+++ b/internal/parser_test.go
@@ -3,23 +3,45 @@ package internal
 import (
 	"os"
 	"path/filepath"
+	"reflect"
+	"strings"
 	"testing"

 	"github.com/ivuorinen/gh-action-readme/appconstants"
 	"github.com/ivuorinen/gh-action-readme/testutil"
 )

+const testPermissionWrite = "write"
+
+// parseActionFromContent creates a temporary action.yml file with the given content and parses it.
+func parseActionFromContent(t *testing.T, content string) (*ActionYML, error) {
+	t.Helper()
+
+	tmpFile, err := os.CreateTemp(t.TempDir(), appconstants.TestActionFilePattern)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = os.Remove(tmpFile.Name()) }()
+
+	if _, err := tmpFile.WriteString(content); err != nil {
+		t.Fatal(err)
+	}
+	_ = tmpFile.Close()
+
+	return ParseActionYML(tmpFile.Name())
+}
+
 // createTestDirWithAction creates a directory with an action.yml file and returns both paths.
 func createTestDirWithAction(t *testing.T, baseDir, dirName, yamlContent string) (string, string) {
 	t.Helper()
 	dirPath := filepath.Join(baseDir, dirName)
-	if err := os.Mkdir(dirPath, appconstants.FilePermDir); err != nil { // nolint:gosec
+	if err := os.Mkdir(dirPath, appconstants.FilePermDir); err != nil {
 		t.Fatalf(testutil.ErrCreateDir(dirName), err)
 	}
 	actionPath := filepath.Join(dirPath, appconstants.ActionFileNameYML)
 	if err := os.WriteFile(
 		actionPath, []byte(yamlContent), appconstants.FilePermDefault,
-	); err != nil { // nolint:gosec
+	); err != nil {
 		t.Fatalf(testutil.ErrCreateFile(dirName+"/action.yml"), err)
 	}

@@ -32,7 +54,7 @@ func createTestFile(t *testing.T, baseDir, fileName, content string) string {
 	filePath := filepath.Join(baseDir, fileName)
 	if err := os.WriteFile(
 		filePath, []byte(content), appconstants.FilePermDefault,
-	); err != nil { // nolint:gosec
+	); err != nil {
 		t.Fatalf(testutil.ErrCreateFile(fileName), err)
 	}

@@ -224,14 +246,14 @@ func TestDiscoverActionFilesNestedIgnoredDirs(t *testing.T) {
 	//         action.yml (should be ignored)

 	nodeModulesDir := filepath.Join(tmpDir, appconstants.DirNodeModules, "deep", "nested")
-	if err := os.MkdirAll(nodeModulesDir, appconstants.FilePermDir); err != nil { // nolint:gosec
+	if err := os.MkdirAll(nodeModulesDir, appconstants.FilePermDir); err != nil {
 		t.Fatalf(testutil.ErrCreateDir("nested"), err)
 	}

 	nestedAction := filepath.Join(nodeModulesDir, appconstants.ActionFileNameYML)
 	if err := os.WriteFile(
 		nestedAction, []byte(appconstants.TestYAMLNested), appconstants.FilePermDefault,
-	); err != nil { // nolint:gosec
+	); err != nil {
 		t.Fatalf(testutil.ErrCreateFile("nested action.yml"), err)
 	}

@@ -254,19 +276,19 @@ func TestDiscoverActionFilesNonRecursive(t *testing.T) {
 	rootAction := filepath.Join(tmpDir, appconstants.ActionFileNameYML)
 	if err := os.WriteFile(
 		rootAction, []byte(appconstants.TestYAMLRoot), appconstants.FilePermDefault,
-	); err != nil { // nolint:gosec
+	); err != nil {
 		t.Fatalf(testutil.ErrCreateFile("action.yml"), err)
 	}

 	// Create subdirectory (should not be searched in non-recursive mode)
 	subDir := filepath.Join(tmpDir, "sub")
-	if err := os.Mkdir(subDir, appconstants.FilePermDir); err != nil { // nolint:gosec
+	if err := os.Mkdir(subDir, appconstants.FilePermDir); err != nil {
 		t.Fatalf(testutil.ErrCreateDir("sub"), err)
 	}
 	subAction := filepath.Join(subDir, appconstants.ActionFileNameYML)
 	if err := os.WriteFile(
 		subAction, []byte(appconstants.TestYAMLSub), appconstants.FilePermDefault,
-	); err != nil { // nolint:gosec
+	); err != nil {
 		t.Fatalf(testutil.ErrCreateFile("sub/action.yml"), err)
 	}

@@ -283,3 +305,586 @@ func TestDiscoverActionFilesNonRecursive(t *testing.T) {
 		t.Errorf("DiscoverActionFiles() = %v, want %v", files[0], rootAction)
 	}
 }
+
+// TestParsePermissionsFromComments tests parsing permissions from header comments.
+func TestParsePermissionsFromComments(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		content string
+		want    map[string]string
+		wantErr bool
+	}{
+		{
+			name: "single permission with dash format",
+			content: `# yaml-language-server: $schema=https://json.schemastore.org/github-action.json
+# permissions:
+#   - contents: read  # Required for checking out repository
+name: Test Action`,
+			want: map[string]string{
+				"contents": "read",
+			},
+			wantErr: false,
+		},
+		{
+			name: "multiple permissions",
+			content: `# permissions:
+#   - contents: read
+#   - issues: write
+#   - pull-requests: write
+name: Test Action`,
+			want: map[string]string{
+				"contents":      "read",
+				"issues":        "write",
+				"pull-requests": "write",
+			},
+			wantErr: false,
+		},
+		{
+			name: "permissions without dash",
+			content: `# permissions:
+#   contents: read
+#   issues: write
+name: Test Action`,
+			want: map[string]string{
+				"contents": "read",
+				"issues":   "write",
+			},
+			wantErr: false,
+		},
+		{
+			name: "no permissions block",
+			content: `# Just a comment
+name: Test Action`,
+			want:    map[string]string{},
+			wantErr: false,
+		},
+		{
+			name: "permissions with inline comments",
+			content: `# permissions:
+#   - contents: read  # Needed for checkout
+#   - issues: write   # To create issues
+name: Test Action`,
+			want: map[string]string{
+				"contents": "read",
+				"issues":   "write",
+			},
+			wantErr: false,
+		},
+		{
+			name: "empty permissions block",
+			content: `# permissions:
+name: Test Action`,
+			want:    map[string]string{},
+			wantErr: false,
+		},
+		{
+			name: "permissions with mixed formats",
+			content: `# permissions:
+#   - contents: read
+#   issues: write
+name: Test Action`,
+			want: map[string]string{
+				"contents": "read",
+				"issues":   "write",
+			},
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			// Create temp file
+			tmpFile, err := os.CreateTemp(t.TempDir(), appconstants.TestActionFilePattern)
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer func() { _ = os.Remove(tmpFile.Name()) }()
+
+			if _, err := tmpFile.WriteString(tt.content); err != nil {
+				t.Fatal(err)
+			}
+			_ = tmpFile.Close()
+
+			got, err := parsePermissionsFromComments(tmpFile.Name())
+
+			if (err != nil) != tt.wantErr {
+				t.Errorf("parsePermissionsFromComments() error = %v, wantErr %v", err, tt.wantErr)
+
+				return
+			}
+
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("parsePermissionsFromComments() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+// TestParseActionYMLWithCommentPermissions tests that ParseActionYML includes comment permissions.
+func TestParseActionYMLWithCommentPermissions(t *testing.T) {
+	t.Parallel()
+
+	content := appconstants.TestPermissionsHeader +
+		"#   - contents: read\n" +
+		appconstants.TestActionNameLine +
+		appconstants.TestDescriptionLine +
+		appconstants.TestRunsLine +
+		appconstants.TestCompositeUsing +
+		appconstants.TestStepsEmpty
+
+	action, err := parseActionFromContent(t, content)
+	if err != nil {
+		t.Fatalf(appconstants.TestErrorFormat, err)
+	}
+
+	if action.Permissions == nil {
+		t.Fatal("Expected permissions to be parsed from comments")
+	}
+
+	if action.Permissions["contents"] != "read" {
+		t.Errorf("Expected contents: read, got %v", action.Permissions)
+	}
+}
+
+// TestParseActionYMLYAMLPermissionsOverrideComments tests that YAML permissions override comments.
+func TestParseActionYMLYAMLPermissionsOverrideComments(t *testing.T) {
+	t.Parallel()
+
+	content := appconstants.TestPermissionsHeader +
+		"#   - contents: read\n" +
+		"#   - issues: write\n" +
+		appconstants.TestActionNameLine +
+		appconstants.TestDescriptionLine +
+		"permissions:\n" +
+		"  contents: write  # YAML override\n" +
+		appconstants.TestRunsLine +
+		appconstants.TestCompositeUsing +
+		appconstants.TestStepsEmpty
+
+	action, err := parseActionFromContent(t, content)
+	if err != nil {
+		t.Fatalf(appconstants.TestErrorFormat, err)
+	}
+
+	// YAML should override comment
+	if action.Permissions["contents"] != testPermissionWrite {
+		t.Errorf(
+			"Expected YAML permissions to override comment permissions, got contents: %v",
+			action.Permissions["contents"],
+		)
+	}
+
+	// Comment permission should be merged in
+	if action.Permissions["issues"] != testPermissionWrite {
+		t.Errorf(
+			"Expected comment permissions to be merged with YAML permissions, got issues: %v",
+			action.Permissions["issues"],
+		)
+	}
+}
+
+// TestParseActionYMLOnlyYAMLPermissions tests parsing when only YAML permissions exist.
+func TestParseActionYMLOnlyYAMLPermissions(t *testing.T) {
+	t.Parallel()
+
+	content := appconstants.TestActionNameLine +
+		appconstants.TestDescriptionLine +
+		"permissions:\n" +
+		"  contents: read\n" +
+		"  issues: write\n" +
+		appconstants.TestRunsLine +
+		appconstants.TestCompositeUsing +
+		appconstants.TestStepsEmpty
+
+	action, err := parseActionFromContent(t, content)
+	if err != nil {
+		t.Fatalf(appconstants.TestErrorFormat, err)
+	}
+
+	if action.Permissions == nil {
+		t.Fatal("Expected permissions to be parsed from YAML")
+	}
+
+	if action.Permissions["contents"] != "read" {
+		t.Errorf("Expected contents: read, got %v", action.Permissions)
+	}
+
+	if action.Permissions["issues"] != testPermissionWrite {
+		t.Errorf("Expected issues: write, got %v", action.Permissions)
+	}
+}
+
+// TestParseActionYMLNoPermissions tests parsing when no permissions exist.
+func TestParseActionYMLNoPermissions(t *testing.T) {
+	t.Parallel()
+
+	content := appconstants.TestActionNameLine +
+		appconstants.TestDescriptionLine +
+		appconstants.TestRunsLine +
+		appconstants.TestCompositeUsing +
+		appconstants.TestStepsEmpty
+
+	action, err := parseActionFromContent(t, content)
+	if err != nil {
+		t.Fatalf(appconstants.TestErrorFormat, err)
+	}
+
+	if action.Permissions != nil {
+		t.Errorf("Expected no permissions, got %v", action.Permissions)
+	}
+}
+
+// TestParseActionYMLMalformedYAML tests parsing with malformed YAML.
+func TestParseActionYMLMalformedYAML(t *testing.T) {
+	t.Parallel()
+
+	content := appconstants.TestActionNameLine +
+		appconstants.TestDescriptionLine +
+		"invalid-yaml: [\n" + // Unclosed bracket
+		"  - item"
+
+	_, err := parseActionFromContent(t, content)
+	if err == nil {
+		t.Error("Expected error for malformed YAML, got nil")
+	}
+}
+
+// TestParseActionYMLEmptyFile tests parsing an empty file.
+func TestParseActionYMLEmptyFile(t *testing.T) {
+	t.Parallel()
+
+	tmpFile, err := os.CreateTemp(t.TempDir(), appconstants.TestActionFilePattern)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = os.Remove(tmpFile.Name()) }()
+
+	_ = tmpFile.Close()
+
+	_, err = ParseActionYML(tmpFile.Name())
+	// Empty file should return EOF error from YAML parser
+	if err == nil {
+		t.Error("Expected EOF error for empty file, got nil")
+	}
+}
+
+// TestParsePermissionLineEdgeCases tests edge cases in permission line parsing.
+func TestParsePermissionLineEdgeCases(t *testing.T) {
+	tests := []struct {
+		name      string
+		input     string
+		wantKey   string
+		wantValue string
+		wantOK    bool
+	}{
+		{
+			name:      "comment at start is parsed",
+			input:     "#contents: read",
+			wantKey:   "#contents",
+			wantValue: "read",
+			wantOK:    true,
+		},
+		{
+			name:      "empty value after colon",
+			input:     "contents:",
+			wantKey:   "",
+			wantValue: "",
+			wantOK:    false,
+		},
+		{
+			name:      "only spaces after colon",
+			input:     "contents:   ",
+			wantKey:   "",
+			wantValue: "",
+			wantOK:    false,
+		},
+		{
+			name:      "valid with inline comment",
+			input:     "contents: read # required",
+			wantKey:   "contents",
+			wantValue: "read",
+			wantOK:    true,
+		},
+		{
+			name:      "valid with leading dash",
+			input:     "- issues: write",
+			wantKey:   "issues",
+			wantValue: "write",
+			wantOK:    true,
+		},
+		{
+			name:      "no colon",
+			input:     "invalid permission line",
+			wantKey:   "",
+			wantValue: "",
+			wantOK:    false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			key, value, ok := parsePermissionLine(tt.input)
+
+			if ok != tt.wantOK {
+				t.Errorf("parsePermissionLine() ok = %v, want %v", ok, tt.wantOK)
+			}
+
+			if key != tt.wantKey {
+				t.Errorf("parsePermissionLine() key = %q, want %q", key, tt.wantKey)
+			}
+
+			if value != tt.wantValue {
+				t.Errorf("parsePermissionLine() value = %q, want %q", value, tt.wantValue)
+			}
+		})
+	}
+}
+
+// TestProcessPermissionEntryIndentationEdgeCases tests indentation scenarios.
+func TestProcessPermissionEntryIndentationEdgeCases(t *testing.T) {
+	tests := []struct {
+		name               string
+		line               string
+		content            string
+		initialIndent      int
+		wantBreak          bool
+		wantPermissionsLen int
+	}{
+		{
+			name:               "first item sets indent",
+			line:               appconstants.TestContentsRead,
+			content:            "contents: read",
+			initialIndent:      -1,
+			wantBreak:          false,
+			wantPermissionsLen: 1,
+		},
+		{
+			name:               "dedented breaks",
+			line:               "# contents: read",
+			content:            "contents: read",
+			initialIndent:      2,
+			wantBreak:          true,
+			wantPermissionsLen: 0,
+		},
+		{
+			name:               "same indent continues",
+			line:               "#   issues: write",
+			content:            "issues: write",
+			initialIndent:      3,
+			wantBreak:          false,
+			wantPermissionsLen: 1,
+		},
+		{
+			name:               "invalid format skipped",
+			line:               "#   invalid-line-no-colon",
+			content:            "invalid-line-no-colon",
+			initialIndent:      3,
+			wantBreak:          false,
+			wantPermissionsLen: 0,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			permissions := make(map[string]string)
+			indent := tt.initialIndent
+
+			shouldBreak := processPermissionEntry(tt.line, tt.content, &indent, permissions)
+
+			if shouldBreak != tt.wantBreak {
+				t.Errorf("processPermissionEntry() shouldBreak = %v, want %v", shouldBreak, tt.wantBreak)
+			}
+
+			if len(permissions) != tt.wantPermissionsLen {
+				t.Errorf(
+					"processPermissionEntry() permissions length = %d, want %d",
+					len(permissions),
+					tt.wantPermissionsLen,
+				)
+			}
+		})
+	}
+}
+
+// TestParsePermissionsFromCommentsEdgeCases tests edge cases in comment parsing.
+func TestParsePermissionsFromCommentsEdgeCases(t *testing.T) {
+	tests := []struct {
+		name        string
+		content     string
+		wantPerms   map[string]string
+		wantErr     bool
+		description string
+	}{
+		{
+			name: "duplicate permissions",
+			content: appconstants.TestPermissionsHeader +
+				appconstants.TestContentsRead +
+				"#   contents: write\n",
+			wantPerms:   map[string]string{"contents": "write"},
+			wantErr:     false,
+			description: "last value wins",
+		},
+		{
+			name: "mixed valid and invalid lines",
+			content: appconstants.TestPermissionsHeader +
+				appconstants.TestContentsRead +
+				"#   invalid-line-no-value\n" +
+				"#   issues: write\n",
+			wantPerms:   map[string]string{"contents": "read", "issues": "write"},
+			wantErr:     false,
+			description: "invalid lines skipped",
+		},
+		{
+			name: "permissions block ends at non-comment",
+			content: appconstants.TestPermissionsHeader +
+				appconstants.TestContentsRead +
+				appconstants.TestActionNameLine +
+				"#   issues: write\n",
+			wantPerms:   map[string]string{"contents": "read"},
+			wantErr:     false,
+			description: "stops at first non-comment",
+		},
+		{
+			name: "only permissions header",
+			content: appconstants.TestPermissionsHeader +
+				appconstants.TestActionNameLine,
+			wantPerms:   map[string]string{},
+			wantErr:     false,
+			description: "empty permissions block",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tmpFile, err := os.CreateTemp(t.TempDir(), "test-*.yml")
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer func() { _ = os.Remove(tmpFile.Name()) }()
+
+			if _, err := tmpFile.WriteString(tt.content); err != nil {
+				t.Fatal(err)
+			}
+			_ = tmpFile.Close()
+
+			perms, err := parsePermissionsFromComments(tmpFile.Name())
+
+			if (err != nil) != tt.wantErr {
+				t.Errorf("parsePermissionsFromComments() error = %v, wantErr %v", err, tt.wantErr)
+			}
+
+			if !reflect.DeepEqual(perms, tt.wantPerms) {
+				t.Errorf("parsePermissionsFromComments() = %v, want %v (%s)", perms, tt.wantPerms, tt.description)
+			}
+		})
+	}
+}
+
+// TestMergePermissionsEdgeCases tests permission merging edge cases.
+func TestMergePermissionsEdgeCases(t *testing.T) {
+	tests := []struct {
+		name         string
+		yamlPerms    map[string]string
+		commentPerms map[string]string
+		wantPerms    map[string]string
+	}{
+		{
+			name:         "both nil",
+			yamlPerms:    nil,
+			commentPerms: nil,
+			wantPerms:    nil,
+		},
+		{
+			name:         "yaml nil, comments empty",
+			yamlPerms:    nil,
+			commentPerms: map[string]string{},
+			wantPerms:    nil,
+		},
+		{
+			name:         "yaml empty, comments nil",
+			yamlPerms:    map[string]string{},
+			commentPerms: nil,
+			wantPerms:    map[string]string{},
+		},
+		{
+			name:         "yaml has value, comments override",
+			yamlPerms:    map[string]string{"contents": "read"},
+			commentPerms: map[string]string{"issues": "write"},
+			wantPerms:    map[string]string{"contents": "read", "issues": "write"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			action := &ActionYML{Permissions: tt.yamlPerms}
+			mergePermissions(action, tt.commentPerms)
+
+			if !reflect.DeepEqual(action.Permissions, tt.wantPerms) {
+				t.Errorf("mergePermissions() = %v, want %v", action.Permissions, tt.wantPerms)
+			}
+		})
+	}
+}
+
+// TestDiscoverActionFilesWalkErrors tests error handling during directory walk.
+func TestDiscoverActionFilesWalkErrors(t *testing.T) {
+	// Test with a path that doesn't exist
+	_, err := DiscoverActionFiles("/nonexistent/path/that/does/not/exist", true, []string{})
+	if err == nil {
+		t.Error("Expected error for nonexistent directory, got nil")
+	}
+
+	// Test that error message mentions the path
+	if err != nil && !strings.Contains(err.Error(), "/nonexistent/path/that/does/not/exist") {
+		t.Errorf("Expected error to mention path, got: %v", err)
+	}
+}
+
+// TestWalkFuncErrorHandling tests walkFunc error propagation.
+func TestWalkFuncErrorHandling(t *testing.T) {
+	walker := &actionFileWalker{
+		ignoredDirs: []string{},
+		actionFiles: []string{},
+	}
+
+	// Create a valid FileInfo for testing
+	tmpDir := t.TempDir()
+	info, err := os.Stat(tmpDir)
+	if err != nil {
+		t.Fatalf("Failed to stat temp dir: %v", err)
+	}
+
+	// Test with valid directory - should return nil
+	err = walker.walkFunc(tmpDir, info, nil)
+	if err != nil {
+		t.Errorf("walkFunc() with valid directory should return nil, got: %v", err)
+	}
+
+	// Test with pre-existing error - should propagate
+	testErr := filepath.SkipDir
+	err = walker.walkFunc(tmpDir, info, testErr)
+	if err != testErr {
+		t.Errorf("walkFunc() should propagate error, got %v, want %v", err, testErr)
+	}
+}
+
+// TestParseActionYMLOnlyComments tests file with only comments.
+func TestParseActionYMLOnlyComments(t *testing.T) {
+	t.Parallel()
+
+	content := "# This is a comment\n" +
+		"# Another comment\n" +
+		appconstants.TestPermissionsHeader +
+		appconstants.TestContentsRead
+
+	_, err := parseActionFromContent(t, content)
+	// File with only comments should return EOF error from YAML parser
+	// (comments are parsed separately, but YAML decoder still needs valid YAML)
+	if err == nil {
+		t.Error("Expected EOF error for comment-only file, got nil")
+	}
+}