From f5b5e75b26180c63338a15471ebb21d1bc258b2b Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Tue, 22 Sep 2026 15:23:22 +1200 Subject: [PATCH] Verify publishing environment reviewers after setup. --- context/getting-started.md | 4 + fixtures/bake/gem/github/project_client.rb | 4 +- guides/getting-started/readme.md | 4 + lib/bake/gem/github/project.rb | 27 ++++-- releases.md | 4 + test/bake/gem/github/project/environment.rb | 98 ++++++++++++++++++++- 6 files changed, 130 insertions(+), 11 deletions(-) diff --git a/context/getting-started.md b/context/getting-started.md index 92f6862..536f05e 100644 --- a/context/getting-started.md +++ b/context/getting-started.md @@ -69,10 +69,14 @@ reviewers: Replace `your-org/managers` with your organization and team slug, for example `socketry/managers`. Individual user logins are also supported. Reviewers need at least read access to the repository. Setup resolves their GitHub IDs without changing repository access or team membership. +Reviewer teams must have **Visible** visibility in GitHub team settings. Plan and apply reject **Secret** teams before changing any settings, because GitHub can silently discard them from the environment reviewer list. Setup does not change team visibility. + GitHub accepts one to six users or teams, and **one approval from any listed reviewer or team member is sufficient**. It does not support a minimum environment approval count. The default two PR approvals are independent of this publishing approval. Create the environment and restrict its deployment branch as described above before running plan or apply with reviewers configured. The plan previews the current and desired environment settings. Apply replaces its reviewer list while preserving its wait timer, self-review prevention, administrator bypass setting, and deployment branch restrictions. Custom deployment protection rules are managed separately and are not modified. Reapplying an identical reviewer list leaves the environment unchanged. +After updating the environment, apply reads its settings back and fails if the reviewer identities or preserved protections differ from the requested configuration. Reviewer order does not matter. Earlier updates may already have completed; inspect the environment in GitHub and rerun `gem:github:setup:plan` before retrying apply. + Omitting `reviewers` leaves environment settings unmanaged, including any existing reviewer requirement. An empty list is rejected. To remove an existing requirement, change the environment settings explicitly in GitHub. The RubyGems Trusted Publisher must explicitly require the `rubygems` environment; leaving that field blank would allow this trusted publisher to authenticate jobs without the environment approval. Keep administrator bypass enabled if administrators should be able to explicitly authorize publication without a reviewer. Environment approvals are available for public repositories on GitHub Free. diff --git a/fixtures/bake/gem/github/project_client.rb b/fixtures/bake/gem/github/project_client.rb index 78940f9..b1fdf01 100644 --- a/fixtures/bake/gem/github/project_client.rb +++ b/fixtures/bake/gem/github/project_client.rb @@ -32,7 +32,9 @@ def readlines(*arguments, **options) @writes << File.read(arguments[arguments.index("--body-file") + 1]) ["https://github.com/socketry/example/pull/42\n"] else - [JSON.generate(@responses.fetch(arguments.fetch(2)))] + response = @responses.fetch(arguments.fetch(2)) + response = response.call if response.respond_to?(:call) + [JSON.generate(response)] end end diff --git a/guides/getting-started/readme.md b/guides/getting-started/readme.md index 92f6862..536f05e 100644 --- a/guides/getting-started/readme.md +++ b/guides/getting-started/readme.md @@ -69,10 +69,14 @@ reviewers: Replace `your-org/managers` with your organization and team slug, for example `socketry/managers`. Individual user logins are also supported. Reviewers need at least read access to the repository. Setup resolves their GitHub IDs without changing repository access or team membership. +Reviewer teams must have **Visible** visibility in GitHub team settings. Plan and apply reject **Secret** teams before changing any settings, because GitHub can silently discard them from the environment reviewer list. Setup does not change team visibility. + GitHub accepts one to six users or teams, and **one approval from any listed reviewer or team member is sufficient**. It does not support a minimum environment approval count. The default two PR approvals are independent of this publishing approval. Create the environment and restrict its deployment branch as described above before running plan or apply with reviewers configured. The plan previews the current and desired environment settings. Apply replaces its reviewer list while preserving its wait timer, self-review prevention, administrator bypass setting, and deployment branch restrictions. Custom deployment protection rules are managed separately and are not modified. Reapplying an identical reviewer list leaves the environment unchanged. +After updating the environment, apply reads its settings back and fails if the reviewer identities or preserved protections differ from the requested configuration. Reviewer order does not matter. Earlier updates may already have completed; inspect the environment in GitHub and rerun `gem:github:setup:plan` before retrying apply. + Omitting `reviewers` leaves environment settings unmanaged, including any existing reviewer requirement. An empty list is rejected. To remove an existing requirement, change the environment settings explicitly in GitHub. The RubyGems Trusted Publisher must explicitly require the `rubygems` environment; leaving that field blank would allow this trusted publisher to authenticate jobs without the environment approval. Keep administrator bypass enabled if administrators should be able to explicitly authorize publication without a reviewer. Environment approvals are available for public repositories on GitHub Free. diff --git a/lib/bake/gem/github/project.rb b/lib/bake/gem/github/project.rb index 1e683c9..0012038 100644 --- a/lib/bake/gem/github/project.rb +++ b/lib/bake/gem/github/project.rb @@ -157,6 +157,7 @@ def inspect_commit(commit) # Return a read-only comparison of managed settings and current repository settings. # @returns [Hash] Desired rules, existing rules, environments, optional environment changes, and expected Trusted Publisher settings. This does not verify RubyGems ownership or publisher configuration. + # @raises [RuntimeError] If a reviewer team is secret or belongs to another organization. def doctor { desired_rules: Setup.rules(@config), @@ -174,8 +175,9 @@ def doctor # Apply the named rulesets and configured reviewers for an existing environment. Invoke after reviewing doctor output. # Preserves the environment's wait timer, self-review prevention, administrator bypass, and branch restrictions. + # Reads back updated environment settings to verify GitHub retained them. # @returns [Hash] The desired ruleset payloads after successful application. - # @raises [RuntimeError] If more than one existing ruleset has a managed name. + # @raises [RuntimeError] If a reviewer team is unsuitable, managed rulesets are ambiguous, or environment verification fails. Earlier updates may already have completed. # @raises [Bake::Gem::CommandExecutionError] If an API operation fails; earlier updates may already have completed. def apply changes = environment_changes @@ -193,6 +195,9 @@ def apply if changes && changes.fetch(:current) != changes.fetch(:desired) write_api("repos/#{@repository}/#{environment_path}", changes.fetch(:desired), method: "PUT") + unless environment_settings == changes.fetch(:desired) + raise "GitHub did not retain the requested settings for environment #{changes.fetch(:name)}. Check its reviewers and protection rules in GitHub, then rerun gem:github:setup:plan." + end end return rules @@ -209,19 +214,26 @@ def environment_changes return nil unless @config.key?("reviewers") Setup.validate_reviewers(@config["reviewers"]) + current = environment_settings + reviewers = @config.fetch("reviewers").map{|name| resolve_reviewer(name)}.uniq.sort_by{|reviewer| reviewer.values_at(:type, :id)} + + return {name: @config.fetch("environment"), current: current, desired: current.merge(reviewers: reviewers)} + end + + # Read the environment settings, comparing reviewer identities independently of their order. + def environment_settings environment = api(environment_path) protections = environment.fetch("protection_rules").to_h{|rule| [rule.fetch("type"), rule]} reviews = protections.fetch("required_reviewers", {}) - current = { + reviewers = reviews.fetch("reviewers", []).map{|entry| {type: entry.fetch("type"), id: entry.fetch("reviewer").fetch("id")}} + + return { wait_timer: protections.fetch("wait_timer", {}).fetch("wait_timer", 0), prevent_self_review: reviews.fetch("prevent_self_review", false), can_admins_bypass: environment.fetch("can_admins_bypass"), deployment_branch_policy: environment.fetch("deployment_branch_policy"), - reviewers: reviews.fetch("reviewers", []).map{|entry| {type: entry.fetch("type"), id: entry.fetch("reviewer").fetch("id")}}, + reviewers: reviewers.uniq.sort_by{|reviewer| reviewer.values_at(:type, :id)}, } - reviewers = @config.fetch("reviewers").map{|name| resolve_reviewer(name)} - - return {name: @config.fetch("environment"), current: current, desired: current.merge(reviewers: reviewers)} end def resolve_reviewer(name) @@ -235,6 +247,9 @@ def resolve_reviewer(name) type = "User" end response = JSON.parse(readlines("gh", "api", path, chdir: @root).join) + if type == "Team" && response.fetch("privacy") == "secret" + raise "Reviewer team #{name} is Secret. Change its visibility to Visible in GitHub team settings before running setup." + end return {type: type, id: response.fetch("id")} end diff --git a/releases.md b/releases.md index fb20101..ddb54ff 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,9 @@ # Releases +## Unreleased + + - Reject Secret environment reviewer teams before applying release settings, and verify that GitHub retained the requested reviewers and existing protections after updating the environment. + ## v0.3.1 - Publish validated release PRs from pushes to the default branch, retaining the exact merged commit and environment approval without requiring `pull_request_target`. diff --git a/test/bake/gem/github/project/environment.rb b/test/bake/gem/github/project/environment.rb index 923b041..5153a64 100644 --- a/test/bake/gem/github/project/environment.rb +++ b/test/bake/gem/github/project/environment.rb @@ -18,12 +18,26 @@ } end + let(:saved_reviewers) {[{"type" => "Team", "reviewer" => {"id" => 123, "slug" => "managers"}}]} + let(:saved_environment) do + { + "can_admins_bypass" => true, + "deployment_branch_policy" => {"protected_branches" => false, "custom_branch_policies" => true}, + "protection_rules" => [ + {"type" => "branch_policy"}, + {"type" => "required_reviewers", "prevent_self_review" => false, "reviewers" => saved_reviewers}, + ], + } + end + before do Bake::Gem::GitHub::Setup.new(root).generate(repository: "socketry/example", checks: ["Tests"], reviewers: ["socketry/managers"]) project.responses["repos/socketry/example/rulesets?per_page=100"] = [] project.responses["repos/socketry/example/environments"] = {"environments" => [environment]} - project.responses["repos/socketry/example/environments/rubygems"] = environment - project.responses["orgs/socketry/teams/managers"] = {"id" => 123} + project.responses["repos/socketry/example/environments/rubygems"] = -> do + project.writes.any?{|payload| payload.key?("reviewers")} ? saved_environment : environment + end + project.responses["orgs/socketry/teams/managers"] = {"id" => 123, "privacy" => "closed"} end with "#doctor" do @@ -53,6 +67,9 @@ with "#apply" do it "updates reviewers without weakening existing environment protections" do environment["can_admins_bypass"] = false + saved_environment["can_admins_bypass"] = false + saved_environment["protection_rules"] << {"type" => "wait_timer", "wait_timer" => 30} + saved_environment["protection_rules"].find{|rule| rule["type"] == "required_reviewers"}["prevent_self_review"] = true environment["protection_rules"] += [ {"type" => "wait_timer", "wait_timer" => 30}, {"type" => "required_reviewers", "prevent_self_review" => true, "reviewers" => [{"type" => "User", "reviewer" => {"id" => 456}}]}, @@ -66,12 +83,13 @@ "deployment_branch_policy" => environment.fetch("deployment_branch_policy"), "reviewers" => [{"type" => "Team", "id" => 123}], } - expect(project.requests.last[0, 5]).to be == ["gh", "api", "repos/socketry/example/environments/rubygems", "--method", "PUT"] + expect(project.requests.last).to be == ["gh", "api", "repos/socketry/example/environments/rubygems"] expect(project.writes.size).to be == 5 end it "preserves an unrestricted branch policy and enabled administrator bypass" do environment["deployment_branch_policy"] = nil + saved_environment["deployment_branch_policy"] = nil project.apply expect(project.writes.last).to have_keys("deployment_branch_policy" => be_nil, "can_admins_bypass" => be == true) @@ -85,6 +103,67 @@ expect(project.writes.all?{|payload| payload.key?("rules")}).to be == true end + it "accepts reordered reviewers and ignores duplicate configured identities" do + project.config["reviewers"] = ["ioquatix", "socketry/managers", "ioquatix"] + project.responses["users/ioquatix"] = {"id" => 456} + saved_reviewers.unshift({"type" => "User", "reviewer" => {"id" => 456}}) + + project.apply + expect(project.writes.last.fetch("reviewers")).to be == [{"type" => "Team", "id" => 123}, {"type" => "User", "id" => 456}] + + project.apply + expect(project.writes.count{|payload| payload.key?("reviewers")}).to be == 1 + end + + it "rejects a successful write which silently drops every reviewer" do + saved_reviewers.clear + + expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain.*rubygems.*gem:github:setup:plan/) + expect(project.writes.last.fetch("reviewers")).to be == [{"type" => "Team", "id" => 123}] + end + + it "rejects a successful write which drops one of the requested reviewers" do + project.config["reviewers"] << "ioquatix" + project.responses["users/ioquatix"] = {"id" => 456} + + expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/) + end + + it "rejects an unexpected reviewer who could approve publication" do + saved_reviewers << {"type" => "User", "reviewer" => {"id" => 456}} + + expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/) + end + + it "distinguishes user and team reviewers with the same numeric ID" do + saved_reviewers.first["type"] = "User" + + expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/) + end + + it "rejects a successful write which removes the reviewer protection rule" do + saved_environment["protection_rules"].reject!{|rule| rule["type"] == "required_reviewers"} + + expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/) + end + + it "rejects a successful write which changes an existing protection" do + environment["can_admins_bypass"] = false + + expect{project.apply}.to raise_exception(RuntimeError, message: be =~ /did not retain/) + expect(project.writes.last.fetch("can_admins_bypass")).to be == false + end + + it "propagates failure to read back the applied environment" do + project.responses["repos/socketry/example/environments/rubygems"] = -> do + raise "GitHub readback failed" unless project.writes.empty? + environment + end + + expect{project.apply}.to raise_exception(RuntimeError, message: be == "GitHub readback failed") + expect(project.writes.size).to be == 5 + end + it "leaves environment settings unmanaged when reviewers are omitted" do project.config.delete("reviewers") project.apply @@ -125,10 +204,21 @@ it "escapes the configured environment name in API requests" do project.config["environment"] = "release / gems" - project.responses["repos/socketry/example/environments/release%20%2F%20gems"] = environment + project.responses["repos/socketry/example/environments/release%20%2F%20gems"] = project.responses.fetch("repos/socketry/example/environments/rubygems") project.apply expect(project.requests.last.fetch(2)).to be == "repos/socketry/example/environments/release%20%2F%20gems" end end + + [:doctor, :apply].each do |operation| + with "##{operation}" do + it "rejects secret reviewer teams before changing any settings" do + project.responses["orgs/socketry/teams/managers"]["privacy"] = "secret" + + expect{project.public_send(operation)}.to raise_exception(RuntimeError, message: be =~ /socketry\/managers is Secret.*Visible/) + expect(project.writes).to be == [] + end + end + end end