Authorization bypass / Privilege escalation

PoC (conceptual demonstration of pre-fix behavior vs post-fix behavior): # Scenario: A repository with a parent ruleset that grants bypass to the current actor (Always), and a branch rule that requires a PR # Old (pre-fix) behavior: # The provisioning check blocks because there is a pull_request rule on the branch, regardless of bypass in the parent ruleset. # Runtime push could still succeed due to bypass evaluation on the server side, creating a discrepancy and potential unauthorized write access. # New (post-fix) behavior: # The provisioning check consults the parent ruleset's current_user_can_bypass value. If it is Always or Exempt, the branch PR rule is considered non-blocking for direct pushes; otherwise it blocks. # Python-based PoC illustrating the difference branch_rules = [{"type": "pull_request", "ruleset_id": 1}] parent_rulesets = { 1: {"current_user_can_bypass": "always"} } def old_should_block(branch_rules, parent_rulesets): # old logic (incorrect): any PR rule blocks regardless of bypass if len(branch_rules) > 0: return True return False def fixed_should_block(branch_rules, parent_rulesets): # fixed logic: check parent ruleset bypass capability before blocking for rule in branch_rules: rid = rule.get("ruleset_id") if rid is None: return True pr_ruleset = parent_rulesets.get(rid, {}) can_by = pr_ruleset.get("current_user_can_bypass") if can_by in ("always", "exempt"): continue else: return True return False print("Old should block?", old_should_block(branch_rules, parent_rulesets)) print("Fixed should block?", fixed_should_block(branch_rules, parent_rulesets)) # Expected output: # Old should block? True (pre-fix would block) # Fixed should block? False (preflight may allow due to bypass in parent ruleset)

diff --git a/apps/provisioning/pkg/repository/github/impl.go b/apps/provisioning/pkg/repository/github/impl.go index 0db5778d0b221..684efea2e9955 100644 --- a/apps/provisioning/pkg/repository/github/impl.go +++ b/apps/provisioning/pkg/repository/github/impl.go @@ -155,17 +155,61 @@ func (r *githubClient) GetRulesets(ctx context.Context, owner, repository, branc return nil, nil } - // Check if pull request rule is active // Only pull_request rules actually block direct pushes. // Other rules like non_fast_forward (blocks force push only), // required_status_checks (checks run after push), etc. do not prevent regular git push operations. - if len(branchRules.PullRequest) > 0 { - logger.Debug("Branch requires pull request (blocks direct push)", - slog.Int("pr_rule_count", len(branchRules.PullRequest))) - return &Rulesets{RequiresPullRequest: true}, nil + if len(branchRules.PullRequest) == 0 { + logger.Debug("No blocking rules found for branch") + return nil, nil + } + + // bypass_actors live on the parent ruleset, not on the per-branch rule entries. + // We must fetch each unique parent to know whether the current actor (e.g. the + // GitHub App installation token in use) can bypass the PR requirement; GitHub + // evaluates that for us and returns it as `current_user_can_bypass`, so we don't + // need to know the App's installation ID to match against bypass_actors. + rulesetIDs := make(map[int64]struct{}, len(branchRules.PullRequest)) + for _, rule := range branchRules.PullRequest { + if rule == nil { + continue + } + if rule.RulesetID == 0 { + // Without a valid ID we can't fetch the parent to confirm bypass — keep blocking. + logger.Warn("Pull request rule has zero ruleset_id, treating as blocking") + return &Rulesets{RequiresPullRequest: true}, nil + } + rulesetIDs[rule.RulesetID] = struct{}{} + } + + for rulesetID := range rulesetIDs { + ruleset, _, err := r.gh.Repositories.GetRuleset(ctx, owner, repository, rulesetID, false) + if err != nil { + // Fail-closed: a silent false negative would let the Repository save and + // then fail every subsequent sync push with a 403. Surfacing a block at + // setup is the safer default. + logger.Warn("Failed to fetch parent ruleset, treating PR requirement as blocking", + slog.Int64("ruleset_id", rulesetID), + slog.Any("error", err)) + return &Rulesets{RequiresPullRequest: true}, nil + } + + // Only "always" and "exempt" allow unrestricted direct push. + // "pull_request" only bypasses during PR merge — direct push remains blocked. + canBypass := ruleset.CurrentUserCanBypass + if canBypass == nil || + (*canBypass != github.BypassModeAlways && *canBypass != github.BypassModeExempt) { + logger.Debug("Branch requires pull request (current actor cannot bypass)", + slog.Int64("ruleset_id", rulesetID), + slog.Any("current_user_can_bypass", canBypass)) + return &Rulesets{RequiresPullRequest: true}, nil + } + + logger.Debug("Ruleset PR requirement is bypassable by current actor", + slog.Int64("ruleset_id", rulesetID), + slog.String("bypass_mode", string(*canBypass))) } - logger.Debug("No blocking rules found for branch") + logger.Debug("All PR-requiring rulesets are bypassable by current actor") return nil, nil } diff --git a/apps/provisioning/pkg/repository/github/impl_test.go b/apps/provisioning/pkg/repository/github/impl_test.go index 82f80c1ca8fad..3ed5f999acd14 100644 --- a/apps/provisioning/pkg/repository/github/impl_test.go +++ b/apps/provisioning/pkg/repository/github/impl_test.go @@ -7,6 +7,8 @@ import ( "fmt" "io" "net/http" + "strings" + "sync/atomic" "testing" "time" @@ -1780,7 +1782,7 @@ func TestGithubClient_GetRulesets(t *testing.T) { wantErr: nil, }, { - name: "pull request rule is active", + name: "pull request rule is active and not bypassable", mockHandler: mockhub.NewMockedHTTPClient( mockhub.WithRequestMatchHandler( mockhub.GetReposRulesBranchesByOwnerByRepoByBranch, @@ -1799,6 +1801,373 @@ func TestGithubClient_GetRulesets(t *testing.T) { require.NoError(t, json.NewEncoder(w).Encode(rules)) }), ), + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesetsByOwnerByRepoByRulesetId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(map[string]interface{}{ + "id": 1, + "name": "test-ruleset", + "source": "test-owner/test-repo", + "enforcement": "active", + "current_user_can_bypass": "never", + })) + }), + ), + ), + owner: "test-owner", + repository: "test-repo", + branch: "main", + wantRulesets: &Rulesets{ + RequiresPullRequest: true, + }, + wantErr: nil, + }, + { + name: "pull request rule with bypass mode always returns no block", + mockHandler: mockhub.NewMockedHTTPClient( + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesBranchesByOwnerByRepoByBranch, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + rules := []map[string]interface{}{ + { + "type": "pull_request", + "ruleset_source_type": "Repository", + "ruleset_source": "test-owner/test-repo", + "ruleset_id": 1, + "parameters": map[string]interface{}{}, + }, + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(rules)) + }), + ), + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesetsByOwnerByRepoByRulesetId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(map[string]interface{}{ + "id": 1, + "name": "test-ruleset", + "source": "test-owner/test-repo", + "enforcement": "active", + "current_user_can_bypass": "always", + })) + }), + ), + ), + owner: "test-owner", + repository: "test-repo", + branch: "main", + wantRulesets: nil, + wantErr: nil, + }, + { + name: "pull request rule with bypass mode exempt returns no block", + mockHandler: mockhub.NewMockedHTTPClient( + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesBranchesByOwnerByRepoByBranch, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + rules := []map[string]interface{}{ + { + "type": "pull_request", + "ruleset_source_type": "Repository", + "ruleset_source": "test-owner/test-repo", + "ruleset_id": 1, + "parameters": map[string]interface{}{}, + }, + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(rules)) + }), + ), + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesetsByOwnerByRepoByRulesetId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(map[string]interface{}{ + "id": 1, + "name": "test-ruleset", + "source": "test-owner/test-repo", + "enforcement": "active", + "current_user_can_bypass": "exempt", + })) + }), + ), + ), + owner: "test-owner", + repository: "test-repo", + branch: "main", + wantRulesets: nil, + wantErr: nil, + }, + { + name: "pull request rule with bypass mode pull_request still blocks direct push", + mockHandler: mockhub.NewMockedHTTPClient( + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesBranchesByOwnerByRepoByBranch, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + rules := []map[string]interface{}{ + { + "type": "pull_request", + "ruleset_source_type": "Repository", + "ruleset_source": "test-owner/test-repo", + "ruleset_id": 1, + "parameters": map[string]interface{}{}, + }, + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(rules)) + }), + ), + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesetsByOwnerByRepoByRulesetId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(map[string]interface{}{ + "id": 1, + "name": "test-ruleset", + "source": "test-owner/test-repo", + "enforcement": "active", + "current_user_can_bypass": "pull_request", + })) + }), + ), + ), + owner: "test-owner", + repository: "test-repo", + branch: "main", + wantRulesets: &Rulesets{ + RequiresPullRequest: true, + }, + wantErr: nil, + }, + { + name: "pull request rule without current_user_can_bypass field still blocks", + mockHandler: mockhub.NewMockedHTTPClient( + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesBranchesByOwnerByRepoByBranch, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + rules := []map[string]interface{}{ + { + "type": "pull_request", + "ruleset_source_type": "Repository", + "ruleset_source": "test-owner/test-repo", + "ruleset_id": 1, + "parameters": map[string]interface{}{}, + }, + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(rules)) + }), + ), + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesetsByOwnerByRepoByRulesetId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(map[string]interface{}{ + "id": 1, + "name": "test-ruleset", + "source": "test-owner/test-repo", + "enforcement": "active", + })) + }), + ), + ), + owner: "test-owner", + repository: "test-repo", + branch: "main", + wantRulesets: &Rulesets{ + RequiresPullRequest: true, + }, + wantErr: nil, + }, + { + name: "multiple PR rules across rulesets where one is not bypassable still blocks", + mockHandler: mockhub.NewMockedHTTPClient( + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesBranchesByOwnerByRepoByBranch, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + rules := []map[string]interface{}{ + { + "type": "pull_request", + "ruleset_source_type": "Repository", + "ruleset_source": "test-owner/test-repo", + "ruleset_id": 1, + "parameters": map[string]interface{}{}, + }, + { + "type": "pull_request", + "ruleset_source_type": "Organization", + "ruleset_source": "test-owner", + "ruleset_id": 2, + "parameters": map[string]interface{}{}, + }, + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(rules)) + }), + ), + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesetsByOwnerByRepoByRulesetId, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Both rulesets are queried. Return "always" for one and + // "never" for the other so the overall result blocks. + bypass := "always" + if strings.HasSuffix(r.URL.Path, "/2") { + bypass = "never" + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(map[string]interface{}{ + "name": "ruleset", + "source": "test-owner/test-repo", + "enforcement": "active", + "current_user_can_bypass": bypass, + })) + }), + ), + ), + owner: "test-owner", + repository: "test-repo", + branch: "main", + wantRulesets: &Rulesets{ + RequiresPullRequest: true, + }, + wantErr: nil, + }, + { + name: "GetRuleset returns 403 treats as blocking", + mockHandler: mockhub.NewMockedHTTPClient( + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesBranchesByOwnerByRepoByBranch, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + rules := []map[string]interface{}{ + { + "type": "pull_request", + "ruleset_source_type": "Repository", + "ruleset_source": "test-owner/test-repo", + "ruleset_id": 1, + "parameters": map[string]interface{}{}, + }, + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(rules)) + }), + ), + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesetsByOwnerByRepoByRulesetId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + require.NoError(t, json.NewEncoder(w).Encode(&github.ErrorResponse{ + Response: &http.Response{StatusCode: http.StatusForbidden}, + Message: "Forbidden", + })) + }), + ), + ), + owner: "test-owner", + repository: "test-repo", + branch: "main", + wantRulesets: &Rulesets{ + RequiresPullRequest: true, + }, + wantErr: nil, + }, + { + name: "GetRuleset returns 404 treats as blocking", + mockHandler: mockhub.NewMockedHTTPClient( + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesBranchesByOwnerByRepoByBranch, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + rules := []map[string]interface{}{ + { + "type": "pull_request", + "ruleset_source_type": "Repository", + "ruleset_source": "test-owner/test-repo", + "ruleset_id": 1, + "parameters": map[string]interface{}{}, + }, + } + w.WriteHeader(http.StatusOK) + require.NoError(t, json.NewEncoder(w).Encode(rules)) + }), + ), + mockhub.WithRequestMatchHandler( + mockhub.GetReposRulesetsByOwnerByRepoByRulesetId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + require.NoError(t, json.NewEncoder(w).Encode(&github.ErrorResponse{ + Response: &http.Response{StatusCode: http.StatusNotFound}, + Message: "Not Found", + })) + }), + ), + ), + owner: "test-owner", + repository: "test-repo", + branch: "main", + wantRulesets: &Rulesets{ + RequiresPullRequest: true, + } ... [truncated]