diff --git a/fixtures/scenarios.json b/fixtures/scenarios.json index f2f3038..13c0887 100644 --- a/fixtures/scenarios.json +++ b/fixtures/scenarios.json @@ -153,6 +153,11 @@ "method": "GET", "path": "/repos/acme-ghorg/fake-app-repo/branches/main/protection" }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/fake-app-repo/rules/branches/main" + }, { "service": "github", "method": "GET", @@ -194,6 +199,14 @@ "body": "{\"required_pull_request_reviews\":{\"required_approving_review_count\":2,\"dismiss_stale_reviews\":true},\"enforce_admins\":{\"enabled\":true}}", "description": "fetch branch protection rules (requires 2 approvals, enforce for admins)" }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/fake-app-repo/rules/branches/main", + "status_code": 200, + "body": "[]", + "description": "fetch repository rulesets (none configured)" + }, { "service": "github", "method": "GET", @@ -271,6 +284,11 @@ "method": "GET", "path": "/repos/acme-ghorg/fake-app-repo/branches/main/protection" }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/fake-app-repo/rules/branches/main" + }, { "service": "github", "method": "GET", @@ -302,6 +320,14 @@ "body": "{\"required_pull_request_reviews\":{\"required_approving_review_count\":2}}", "description": "fetch branch protection rules (requires 2 approvals)" }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/fake-app-repo/rules/branches/main", + "status_code": 200, + "body": "[]", + "description": "fetch repository rulesets (none configured)" + }, { "service": "github", "method": "GET", @@ -312,6 +338,137 @@ } ] }, + { + "name": "pr_webhook_ruleset_bypass", + "description": "Detect bypass when repo uses rulesets instead of legacy branch protection", + "event_type": "webhook", + "webhook_type": "pull_request", + "config_overrides": { + "APP_GITHUB_WEBHOOK_SECRET": "" + }, + "webhook_payload": { + "action": "closed", + "number": 101, + "pull_request": { + "id": 101, + "number": 101, + "state": "closed", + "merged": true, + "merged_at": "2025-11-22T12:00:00Z", + "merged_by": { + "login": "admin-user", + "id": 2, + "type": "User" + }, + "head": { + "ref": "feature-branch", + "sha": "abc123def456" + }, + "base": { + "ref": "main", + "sha": "def456abc123" + }, + "html_url": "https://github.com/acme-ghorg/ruleset-repo/pull/101" + }, + "repository": { + "name": "ruleset-repo", + "full_name": "acme-ghorg/ruleset-repo", + "owner": { + "login": "acme-ghorg" + } + } + }, + "expected_calls": [ + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/pulls/101" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/branches/main/protection" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/rules/branches/main" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/pulls/101/reviews" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/collaborators/admin-user/permission" + }, + { + "service": "slack", + "method": "POST", + "path": "/chat.postMessage" + } + ], + "mock_responses": [ + { + "service": "github", + "method": "POST", + "path": "/app/installations/987654/access_tokens", + "status_code": 201, + "body": "{\"token\":\"ghs_mock_installation_token\",\"expires_at\":\"2099-12-31T23:59:59Z\"}", + "description": "github app installation token authentication" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/pulls/101", + "status_code": 200, + "body": "{\"number\":101,\"state\":\"closed\",\"merged\":true,\"merged_at\":\"2025-11-22T12:00:00Z\",\"merged_by\":{\"login\":\"admin-user\"},\"base\":{\"ref\":\"main\"}}", + "description": "fetch pr #101 details (merged by admin-user)" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/branches/main/protection", + "status_code": 404, + "body": "{\"message\":\"Branch not protected\"}", + "description": "no legacy branch protection configured" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/rules/branches/main", + "status_code": 200, + "body": "[{\"type\":\"pull_request\",\"ruleset_source_type\":\"Repository\",\"ruleset_source\":\"acme-ghorg/ruleset-repo\",\"ruleset_id\":123,\"parameters\":{\"required_approving_review_count\":1,\"dismiss_stale_reviews_on_push\":false,\"require_code_owner_review\":false,\"require_last_push_approval\":false,\"required_review_thread_resolution\":false}}]", + "description": "fetch repository rulesets (requires 1 approval via ruleset)" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/pulls/101/reviews", + "status_code": 200, + "body": "[]", + "description": "fetch pr reviews (no approvals)" + }, + { + "service": "github", + "method": "GET", + "path": "/repos/acme-ghorg/ruleset-repo/collaborators/admin-user/permission", + "status_code": 200, + "body": "{\"permission\":\"admin\",\"user\":{\"login\":\"admin-user\"}}", + "description": "check merger permissions (admin-user has admin permission)" + }, + { + "service": "slack", + "method": "POST", + "path": "/chat.postMessage", + "status_code": 200, + "body": "{\"ok\":true,\"channel\":\"C01234TEST\",\"ts\":\"1234567890.123456\"}", + "description": "send bypass alert notification to slack" + } + ] + }, { "name": "okta_sync_inactive_users", "description": "Test that inactive Okta users are excluded from sync", diff --git a/internal/github/pr.go b/internal/github/pr.go index caaa060..495036d 100644 --- a/internal/github/pr.go +++ b/internal/github/pr.go @@ -23,6 +23,7 @@ type PRComplianceResult struct { PR *github.PullRequest BaseBranch string Protection *github.Protection + BranchRules *github.BranchRules Violations []ComplianceViolation UserHasBypass bool UserBypassReason string @@ -51,36 +52,49 @@ func (c *Client) CheckPRCompliance(ctx context.Context, owner, repo string, prNu baseBranch := *pr.Base.Ref - protection, _, err := c.client.Repositories.GetBranchProtection(ctx, owner, repo, baseBranch) - if err != nil { - return &PRComplianceResult{ - PR: pr, - BaseBranch: baseBranch, - Violations: []ComplianceViolation{}, - }, nil - } - result := &PRComplianceResult{ PR: pr, BaseBranch: baseBranch, - Protection: protection, Violations: []ComplianceViolation{}, } - c.checkReviewRequirements(ctx, owner, repo, pr, protection, result) - c.checkStatusRequirements(ctx, owner, repo, pr, protection, result) + // fetch legacy branch protection rules + protection, _, err := c.client.Repositories.GetBranchProtection(ctx, owner, repo, baseBranch) + if err == nil { + result.Protection = protection + } + + // fetch repository rulesets for the branch + branchRules, _, err := c.client.Repositories.GetRulesForBranch(ctx, owner, repo, baseBranch, nil) + if err == nil { + result.BranchRules = branchRules + } + + c.checkReviewRequirements(ctx, owner, repo, pr, result) + c.checkStatusRequirements(ctx, owner, repo, pr, result) c.checkUserBypassPermission(ctx, owner, repo, pr, result) return result, nil } // checkReviewRequirements validates that PR had required approving reviews. -func (c *Client) checkReviewRequirements(ctx context.Context, owner, repo string, pr *github.PullRequest, protection *github.Protection, result *PRComplianceResult) { - if protection.RequiredPullRequestReviews == nil { - return +// checks both legacy branch protection and repository rulesets. +func (c *Client) checkReviewRequirements(ctx context.Context, owner, repo string, pr *github.PullRequest, result *PRComplianceResult) { + requiredApprovals := 0 + + // check legacy branch protection + if result.Protection != nil && result.Protection.RequiredPullRequestReviews != nil { + requiredApprovals = result.Protection.RequiredPullRequestReviews.RequiredApprovingReviewCount } - requiredApprovals := protection.RequiredPullRequestReviews.RequiredApprovingReviewCount + // check repository rulesets (use the highest requirement) + if result.BranchRules != nil { + for _, rule := range result.BranchRules.PullRequest { + if rule.Parameters.RequiredApprovingReviewCount > requiredApprovals { + requiredApprovals = rule.Parameters.RequiredApprovingReviewCount + } + } + } if requiredApprovals == 0 { return @@ -107,16 +121,36 @@ func (c *Client) checkReviewRequirements(ctx context.Context, owner, repo string } // checkStatusRequirements validates that required status checks passed. -func (c *Client) checkStatusRequirements(ctx context.Context, owner, repo string, pr *github.PullRequest, protection *github.Protection, result *PRComplianceResult) { - if protection.RequiredStatusChecks == nil || protection.RequiredStatusChecks.Contexts == nil || len(*protection.RequiredStatusChecks.Contexts) == 0 { +// checks both legacy branch protection and repository rulesets. +func (c *Client) checkStatusRequirements(ctx context.Context, owner, repo string, pr *github.PullRequest, result *PRComplianceResult) { + if pr.Head == nil || pr.Head.SHA == nil { return } - if pr.Head == nil || pr.Head.SHA == nil { - return + // collect required checks from both sources + requiredChecks := make(map[string]bool) + + // check legacy branch protection + if result.Protection != nil && + result.Protection.RequiredStatusChecks != nil && + result.Protection.RequiredStatusChecks.Contexts != nil { + for _, check := range *result.Protection.RequiredStatusChecks.Contexts { + requiredChecks[check] = true + } } - requiredChecks := *protection.RequiredStatusChecks.Contexts + // check repository rulesets + if result.BranchRules != nil { + for _, rule := range result.BranchRules.RequiredStatusChecks { + for _, check := range rule.Parameters.RequiredStatusChecks { + requiredChecks[check.Context] = true + } + } + } + + if len(requiredChecks) == 0 { + return + } combinedStatus, _, err := c.client.Repositories.GetCombinedStatus(ctx, owner, repo, *pr.Head.SHA, nil) if err != nil { @@ -130,7 +164,7 @@ func (c *Client) checkStatusRequirements(ctx context.Context, owner, repo string } } - for _, required := range requiredChecks { + for required := range requiredChecks { if !passedChecks[required] { result.Violations = append(result.Violations, ComplianceViolation{ Type: "missing_status_check",