diff --git a/.gitignore b/.gitignore index 3ac07964c4..f636cdc0ca 100644 --- a/.gitignore +++ b/.gitignore @@ -176,3 +176,5 @@ test-script-mode.lock.yml # Test metrics patterns workflow (temporary validation file) .github/workflows/test-metrics-patterns.md .github/workflows/test-metrics-patterns.lock.yml +.github/workflows/test-*.md +.github/workflows/test-*.lock.yml diff --git a/pkg/cli/github.go b/pkg/cli/github.go index 2ca44ca86b..b90fe77a66 100644 --- a/pkg/cli/github.go +++ b/pkg/cli/github.go @@ -10,21 +10,38 @@ import ( var githubLog = logger.New("cli:github") // getGitHubHost returns the GitHub host URL from environment variables. -// It checks GITHUB_SERVER_URL first (GitHub Actions standard), -// then falls back to GH_HOST (gh CLI standard), -// and finally defaults to https://github.com +// Environment variables are checked in priority order for GitHub Enterprise support: +// 1. GITHUB_SERVER_URL - GitHub Actions standard (e.g., https://MYORG.ghe.com) +// 2. GITHUB_ENTERPRISE_HOST - GitHub Enterprise standard (e.g., MYORG.ghe.com) +// 3. GITHUB_HOST - GitHub Enterprise standard (e.g., MYORG.ghe.com) +// 4. GH_HOST - GitHub CLI standard (e.g., MYORG.ghe.com) +// 5. Defaults to https://github.com if none are set +// +// The function normalizes the URL by adding https:// if missing and removing trailing slashes. func getGitHubHost() string { - host := os.Getenv("GITHUB_SERVER_URL") - if host == "" { - host = os.Getenv("GH_HOST") + envVars := []string{"GITHUB_SERVER_URL", "GITHUB_ENTERPRISE_HOST", "GITHUB_HOST", "GH_HOST"} + + for _, envVar := range envVars { + if value := os.Getenv(envVar); value != "" { + githubLog.Printf("Resolved GitHub host from %s: %s", envVar, value) + return normalizeGitHubHostURL(value) + } } - if host == "" { - host = "https://github.com" - githubLog.Print("Using default GitHub host: https://github.com") - } else { - githubLog.Printf("Resolved GitHub host: %s", host) + + defaultHost := "https://github.com" + githubLog.Printf("No GitHub host environment variable set, using default: %s", defaultHost) + return defaultHost +} + +// normalizeGitHubHostURL ensures the host URL has https:// scheme and no trailing slashes +func normalizeGitHubHostURL(rawHostURL string) string { + // Remove all trailing slashes + normalized := strings.TrimRight(rawHostURL, "/") + + // Add https:// scheme if no scheme is present + if !strings.HasPrefix(normalized, "https://") && !strings.HasPrefix(normalized, "http://") { + normalized = "https://" + normalized } - // Remove trailing slash for consistency - return strings.TrimSuffix(host, "/") + return normalized } diff --git a/pkg/cli/github_test.go b/pkg/cli/github_test.go index f3857ff564..dce8ab1b02 100644 --- a/pkg/cli/github_test.go +++ b/pkg/cli/github_test.go @@ -8,46 +8,124 @@ import ( func TestGetGitHubHost(t *testing.T) { tests := []struct { - name string - serverURL string - ghHost string - expectedHost string + name string + serverURL string + enterpriseHost string + githubHost string + ghHost string + expectedHost string }{ { - name: "defaults to github.com", - serverURL: "", - ghHost: "", - expectedHost: "https://github.com", + name: "defaults to github.com", + serverURL: "", + enterpriseHost: "", + githubHost: "", + ghHost: "", + expectedHost: "https://github.com", }, { - name: "uses GITHUB_SERVER_URL when set", - serverURL: "https://github.enterprise.com", - ghHost: "", - expectedHost: "https://github.enterprise.com", + name: "uses GITHUB_SERVER_URL when set", + serverURL: "https://github.enterprise.com", + enterpriseHost: "", + githubHost: "", + ghHost: "", + expectedHost: "https://github.enterprise.com", }, { - name: "uses GH_HOST when GITHUB_SERVER_URL not set", - serverURL: "", - ghHost: "https://github.company.com", - expectedHost: "https://github.company.com", + name: "uses GITHUB_ENTERPRISE_HOST when GITHUB_SERVER_URL not set", + serverURL: "", + enterpriseHost: "github.enterprise.com", + githubHost: "", + ghHost: "", + expectedHost: "https://github.enterprise.com", }, { - name: "GITHUB_SERVER_URL takes precedence over GH_HOST", - serverURL: "https://github.enterprise.com", - ghHost: "https://github.company.com", - expectedHost: "https://github.enterprise.com", + name: "uses GITHUB_HOST when GITHUB_SERVER_URL and GITHUB_ENTERPRISE_HOST not set", + serverURL: "", + enterpriseHost: "", + githubHost: "github.company.com", + ghHost: "", + expectedHost: "https://github.company.com", }, { - name: "removes trailing slash from GITHUB_SERVER_URL", - serverURL: "https://github.enterprise.com/", - ghHost: "", - expectedHost: "https://github.enterprise.com", + name: "uses GH_HOST when other vars not set", + serverURL: "", + enterpriseHost: "", + githubHost: "", + ghHost: "https://github.company.com", + expectedHost: "https://github.company.com", }, { - name: "removes trailing slash from GH_HOST", - serverURL: "", - ghHost: "https://github.company.com/", - expectedHost: "https://github.company.com", + name: "GITHUB_SERVER_URL takes precedence over all others", + serverURL: "https://github.enterprise.com", + enterpriseHost: "github.other.com", + githubHost: "github.another.com", + ghHost: "https://github.company.com", + expectedHost: "https://github.enterprise.com", + }, + { + name: "GITHUB_ENTERPRISE_HOST takes precedence over GITHUB_HOST and GH_HOST", + serverURL: "", + enterpriseHost: "github.enterprise.com", + githubHost: "github.company.com", + ghHost: "github.other.com", + expectedHost: "https://github.enterprise.com", + }, + { + name: "GITHUB_HOST takes precedence over GH_HOST", + serverURL: "", + enterpriseHost: "", + githubHost: "github.company.com", + ghHost: "github.other.com", + expectedHost: "https://github.company.com", + }, + { + name: "removes trailing slash from GITHUB_SERVER_URL", + serverURL: "https://github.enterprise.com/", + enterpriseHost: "", + githubHost: "", + ghHost: "", + expectedHost: "https://github.enterprise.com", + }, + { + name: "removes trailing slash from GITHUB_ENTERPRISE_HOST", + serverURL: "", + enterpriseHost: "github.enterprise.com/", + githubHost: "", + ghHost: "", + expectedHost: "https://github.enterprise.com", + }, + { + name: "removes trailing slash from GH_HOST", + serverURL: "", + enterpriseHost: "", + githubHost: "", + ghHost: "https://github.company.com/", + expectedHost: "https://github.company.com", + }, + { + name: "adds https:// prefix to GITHUB_ENTERPRISE_HOST", + serverURL: "", + enterpriseHost: "MYORG.ghe.com", + githubHost: "", + ghHost: "", + expectedHost: "https://MYORG.ghe.com", + }, + { + name: "adds https:// prefix to GITHUB_HOST", + serverURL: "", + enterpriseHost: "", + githubHost: "MYORG.ghe.com", + ghHost: "", + expectedHost: "https://MYORG.ghe.com", + }, + { + name: "adds https:// prefix to GH_HOST", + serverURL: "", + enterpriseHost: "", + githubHost: "", + ghHost: "MYORG.ghe.com", + expectedHost: "https://MYORG.ghe.com", }, } @@ -55,6 +133,8 @@ func TestGetGitHubHost(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // Set test env vars (always set to ensure clean state) t.Setenv("GITHUB_SERVER_URL", tt.serverURL) + t.Setenv("GITHUB_ENTERPRISE_HOST", tt.enterpriseHost) + t.Setenv("GITHUB_HOST", tt.githubHost) t.Setenv("GH_HOST", tt.ghHost) // Test diff --git a/pkg/gitutil/gitutil_test.go b/pkg/gitutil/gitutil_test.go deleted file mode 100644 index c581b9f26c..0000000000 --- a/pkg/gitutil/gitutil_test.go +++ /dev/null @@ -1,285 +0,0 @@ -//go:build !integration - -package gitutil - -import "testing" - -func TestIsAuthError(t *testing.T) { - tests := []struct { - name string - errMsg string - want bool - }{ - // Test gh_token variations - {"gh_token error", "error: GH_TOKEN is required", true}, - {"gh_token lowercase", "error: gh_token is required", true}, - {"gh_token mixed case", "Error: Gh_Token missing", true}, - - // Test GITHUB_TOKEN variations - {"GITHUB_TOKEN error", "GITHUB_TOKEN not set", true}, - {"github_token lowercase", "github_token not set", true}, - {"github_token mixed", "GitHub_Token is missing", true}, - - // Test authentication variations - {"authentication failed", "authentication failed: invalid credentials", true}, - {"Authentication uppercase", "AUTHENTICATION ERROR: Please log in", true}, - {"authentication in sentence", "The authentication process has failed", true}, - - // Test not logged into variations - {"not logged into GitHub", "You are not logged into any GitHub hosts", true}, - {"Not Logged Into mixed", "Error: Not Logged Into GitHub", true}, - {"not logged into lowercase", "error: not logged into github", true}, - - // Test unauthorized variations - {"unauthorized access", "401 Unauthorized: access denied", true}, - {"UNAUTHORIZED uppercase", "UNAUTHORIZED: Access token is invalid", true}, - {"unauthorized lowercase", "401 unauthorized", true}, - - // Test forbidden variations - {"forbidden access", "403 Forbidden: insufficient permissions", true}, - {"FORBIDDEN uppercase", "FORBIDDEN: You don't have access", true}, - {"forbidden lowercase", "403 forbidden", true}, - - // Test permission denied variations - {"permission denied", "Permission denied to repository", true}, - {"PERMISSION DENIED uppercase", "PERMISSION DENIED: Insufficient privileges", true}, - {"permission denied lowercase", "permission denied", true}, - - // Test case insensitivity - {"case insensitive", "AUTHENTICATION ERROR", true}, - {"mixed case 1", "AuThEnTiCaTiOn FaIlEd", true}, - {"mixed case 2", "PeRmIsSiOn DeNiEd", true}, - - // Test errors that should NOT match - {"not auth error - file not found", "file not found: example.txt", false}, - {"not auth error - network", "network timeout while connecting", false}, - {"not auth error - syntax", "syntax error: unexpected token", false}, - {"not auth error - partial match author", "author not found in repository", false}, - {"not auth error - partial match bidden", "the bidden treasure was found", false}, - {"not auth error - generic", "something went wrong", false}, - {"empty error message", "", false}, - {"not auth - rate limit", "API rate limit exceeded", false}, - {"not auth - not found", "404 Not Found: repository does not exist", false}, - - // Test edge cases - {"only keyword gh_token", "gh_token", true}, - {"only keyword github_token", "github_token", true}, - {"only keyword authentication", "authentication", true}, - {"only keyword unauthorized", "unauthorized", true}, - {"only keyword forbidden", "forbidden", true}, - {"only keyword permission denied", "permission denied", true}, - - // Test with additional context - {"with newlines", "error:\nauthentication failed\nplease login", true}, - {"with tabs", "error:\tauthentication\tfailed", true}, - {"at start", "forbidden: access denied", true}, - {"at end", "access denied: forbidden", true}, - {"in middle", "the authentication has failed completely", true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := IsAuthError(tt.errMsg) - if got != tt.want { - t.Errorf("IsAuthError(%q) = %v, want %v", tt.errMsg, got, tt.want) - } - }) - } -} - -func TestIsHexString(t *testing.T) { - tests := []struct { - name string - s string - want bool - }{ - // Valid hex strings - lowercase - {"valid lowercase", "abcdef0123456789", true}, - {"valid all a-f", "abcdef", true}, - {"valid all 0-9", "0123456789", true}, - - // Valid hex strings - uppercase - {"valid uppercase", "ABCDEF0123456789", true}, - {"valid all A-F", "ABCDEF", true}, - - // Valid hex strings - mixed case - {"valid mixed", "AbCdEf0123456789", true}, - {"valid alternating", "AaBbCcDdEeFf", true}, - - // Valid real-world SHAs - {"valid full SHA", "d3422bf940923ef1d43db5559652b8e1e71869f3", true}, - {"valid short SHA 1", "abc123", true}, - {"valid short SHA 2", "deadbeef", true}, - {"valid short SHA 3", "cafebabe", true}, - - // Valid single characters - {"valid single digit 0", "0", true}, - {"valid single digit 9", "9", true}, - {"valid single letter a", "a", true}, - {"valid single letter f", "f", true}, - {"valid single letter A", "A", true}, - {"valid single letter F", "F", true}, - - // Valid special cases - {"valid all zeros", "0000000000", true}, - {"valid all ones", "1111111111", true}, - {"valid all f lowercase", "ffffffff", true}, - {"valid all F uppercase", "FFFFFFFF", true}, - {"valid long SHA", "0123456789abcdef0123456789abcdef01234567", true}, - - // Invalid - letters beyond f - {"invalid - contains g", "abcdefg", false}, - {"invalid - contains h", "abc123h", false}, - {"invalid - contains z", "xyz123", false}, - {"invalid - all invalid letters", "ghijklmnopqrstuvwxyz", false}, - {"invalid - mixed valid and invalid", "abc123xyz", false}, - - // Invalid - special characters - {"invalid - contains space", "abc 123", false}, - {"invalid - contains dash", "abc-123", false}, - {"invalid - contains underscore", "abc_123", false}, - {"invalid - contains dot", "abc.123", false}, - {"invalid - contains slash", "abc/123", false}, - {"invalid - contains backslash", "abc\\123", false}, - {"invalid - contains colon", "abc:123", false}, - {"invalid - contains at", "abc@123", false}, - {"invalid - contains hash", "abc#123", false}, - {"invalid - contains dollar", "abc$123", false}, - {"invalid - contains percent", "abc%123", false}, - {"invalid - contains ampersand", "abc&123", false}, - {"invalid - contains star", "abc*123", false}, - {"invalid - contains plus", "abc+123", false}, - {"invalid - contains equals", "abc=123", false}, - - // Invalid - whitespace - {"invalid - leading space", " abc123", false}, - {"invalid - trailing space", "abc123 ", false}, - {"invalid - tab", "abc\t123", false}, - {"invalid - newline", "abc\n123", false}, - {"invalid - carriage return", "abc\r123", false}, - - // Invalid - empty and edge cases - {"empty string", "", false}, - {"single space", " ", false}, - {"only spaces", " ", false}, - - // Invalid - unicode and non-ASCII - {"invalid - unicode", "abc123中文", false}, - {"invalid - emoji", "abc123😀", false}, - {"invalid - accented", "ábć123", false}, - - // Invalid - parentheses and brackets - {"invalid - parentheses", "abc(123)", false}, - {"invalid - square brackets", "abc[123]", false}, - {"invalid - curly braces", "abc{123}", false}, - {"invalid - angle brackets", "abc<123>", false}, - - // Invalid - quotes - {"invalid - single quote", "abc'123", false}, - {"invalid - double quote", "abc\"123", false}, - {"invalid - backtick", "abc`123", false}, - - // Invalid - punctuation - {"invalid - comma", "abc,123", false}, - {"invalid - semicolon", "abc;123", false}, - {"invalid - exclamation", "abc!123", false}, - {"invalid - question", "abc?123", false}, - - // Valid - boundary testing - {"valid - exactly 40 chars", "1234567890abcdef1234567890abcdef12345678", true}, - {"valid - 41 chars", "1234567890abcdef1234567890abcdef123456789", true}, - {"valid - 7 chars (short SHA)", "abc1234", true}, - {"valid - 6 chars", "abc123", true}, - {"valid - 2 chars", "ab", true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := IsHexString(tt.s) - if got != tt.want { - t.Errorf("IsHexString(%q) = %v, want %v", tt.s, got, tt.want) - } - }) - } -} - -// Benchmark tests to measure performance -func BenchmarkIsAuthError(b *testing.B) { - testCases := []string{ - "authentication failed: invalid credentials", - "file not found: example.txt", - "403 Forbidden: insufficient permissions", - "network timeout while connecting to server", - "AUTHENTICATION ERROR: Please log in", - "something went wrong", - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - for _, tc := range testCases { - IsAuthError(tc) - } - } -} - -func BenchmarkIsHexString(b *testing.B) { - testCases := []string{ - "d3422bf940923ef1d43db5559652b8e1e71869f3", - "abc123", - "ABCDEF0123456789", - "invalid-string-with-special-chars", - "0123456789", - "ghijklmnop", - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - for _, tc := range testCases { - IsHexString(tc) - } - } -} - -// Test edge cases and behavior consistency -func TestIsAuthErrorConsistency(t *testing.T) { - // Test that function is case-insensitive - variants := []string{ - "authentication", - "AUTHENTICATION", - "Authentication", - "AuThEnTiCaTiOn", - } - - for _, v := range variants { - if !IsAuthError(v) { - t.Errorf("IsAuthError should be case-insensitive, failed for: %q", v) - } - } -} - -func TestIsHexStringConsistency(t *testing.T) { - // Test that function handles both upper and lower case - testPairs := []struct { - lower string - upper string - }{ - {"abc", "ABC"}, - {"def", "DEF"}, - {"0123456789abcdef", "0123456789ABCDEF"}, - {"deadbeef", "DEADBEEF"}, - } - - for _, pair := range testPairs { - lowerResult := IsHexString(pair.lower) - upperResult := IsHexString(pair.upper) - - if lowerResult != upperResult { - t.Errorf("IsHexString should handle case consistently: %q=%v, %q=%v", - pair.lower, lowerResult, pair.upper, upperResult) - } - - if !lowerResult || !upperResult { - t.Errorf("Both %q and %q should be valid hex strings", pair.lower, pair.upper) - } - } -} diff --git a/pkg/workflow/permissions_operations.go b/pkg/workflow/permissions_operations.go index f15cf7ab18..33492f5f17 100644 --- a/pkg/workflow/permissions_operations.go +++ b/pkg/workflow/permissions_operations.go @@ -212,6 +212,15 @@ func (p *Permissions) RenderToYAML() string { if scope == PermissionIdToken && p.allLevel == PermissionRead { continue } + // Skip discussions when expanding all: read unless explicitly set + // This prevents issues in GitHub Enterprise where discussions might not be available + // Discussions permission should be added explicitly or via safe-outputs that need it + if scope == PermissionDiscussions && p.allLevel == PermissionRead { + // Only include if explicitly set in permissions map + if _, explicitlySet := p.permissions[PermissionDiscussions]; !explicitlySet { + continue + } + } allPerms[scope] = p.allLevel } } diff --git a/pkg/workflow/permissions_rendering_test.go b/pkg/workflow/permissions_rendering_test.go index 3f37027c5c..b3d4fab296 100644 --- a/pkg/workflow/permissions_rendering_test.go +++ b/pkg/workflow/permissions_rendering_test.go @@ -75,9 +75,11 @@ func TestPermissions_AllReadWithIdTokenWrite(t *testing.T) { } // Test that all normal scopes have read access + // Note: discussions is NOT included in all: read expansion by default + // (to support GitHub Enterprise where discussions may not be available) normalScopes := []PermissionScope{ PermissionActions, PermissionAttestations, PermissionChecks, PermissionContents, - PermissionDeployments, PermissionDiscussions, PermissionIssues, PermissionPackages, + PermissionDeployments, PermissionIssues, PermissionPackages, PermissionPages, PermissionPullRequests, PermissionRepositoryProj, PermissionSecurityEvents, PermissionStatuses, PermissionModels, } @@ -105,13 +107,13 @@ func TestPermissions_AllReadWithIdTokenWrite(t *testing.T) { yaml := perms.RenderToYAML() // Should contain all normal scopes with read access + // Note: discussions is NOT included in all: read expansion by default expectedLines := []string{ " actions: read", " attestations: read", " checks: read", " contents: read", " deployments: read", - " discussions: read", " issues: read", " packages: read", " pages: read", @@ -133,6 +135,11 @@ func TestPermissions_AllReadWithIdTokenWrite(t *testing.T) { if strings.Contains(yaml, "id-token: read") { t.Errorf("YAML should NOT contain 'id-token: read', but got:\n%s", yaml) } + + // Should NOT contain discussions: read (not included in all: read expansion) + if strings.Contains(yaml, "discussions: read") { + t.Errorf("YAML should NOT contain 'discussions: read' from all:read expansion, but got:\n%s", yaml) + } } func TestPermissions_AllReadRenderToYAML(t *testing.T) { @@ -152,7 +159,6 @@ func TestPermissions_AllReadRenderToYAML(t *testing.T) { " checks: read", " contents: read", " deployments: read", - " discussions: read", " issues: read", " models: read", " packages: read", @@ -162,6 +168,9 @@ func TestPermissions_AllReadRenderToYAML(t *testing.T) { " security-events: read", " statuses: read", }, + notContains: []string{ + " discussions: read", // Should NOT be included in all: read expansion (GitHub Enterprise compatibility) + }, }, { name: "all: read with explicit override - write overrides read", @@ -228,7 +237,6 @@ func TestPermissions_AllReadRenderToYAML(t *testing.T) { " checks: read", " contents: read", " deployments: read", - " discussions: read", " issues: read", " models: read", " packages: read", @@ -239,7 +247,8 @@ func TestPermissions_AllReadRenderToYAML(t *testing.T) { " statuses: read", }, notContains: []string{ - " id-token: read", // Should NOT be included since id-token doesn't support read + " id-token: read", // Should NOT be included since id-token doesn't support read + " discussions: read", // Should NOT be included (GitHub Enterprise compatibility) }, }, }