Merge pull request #17302 from Homebrew/ww/redact-env

Homebrew · May 14, 2024 · 1f603d3 · 1f603d3
2 parents f4a9869 + f78a888
commit 1f603d3
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 66 deletions.
diff --git a/Library/Homebrew/attestation.rb b/Library/Homebrew/attestation.rb
@@ -5,9 +5,12 @@
 require "json"
 require "utils/popen"
 require "exceptions"
+require "system_command"
 
 module Homebrew
  module Attestation
+ extend SystemCommand::Mixin
+
  # @api private
  HOMEBREW_CORE_REPO = "Homebrew/homebrew-core"
  # @api private
@@ -71,7 +74,7 @@ def self.gh_executable
  signing_workflow: T.nilable(String), subject: T.nilable(String)).returns(T::Hash[T.untyped, T.untyped])
  }
  def self.check_attestation(bottle, signing_repo, signing_workflow = nil, subject = nil)
- cmd = [gh_executable, "attestation", "verify", bottle.cached_download, "--repo", signing_repo, "--format",
+ cmd = ["attestation", "verify", bottle.cached_download, "--repo", signing_repo, "--format",
  "json"]
 
  cmd += ["--cert-identity", signing_workflow] if signing_workflow.present?
@@ -83,7 +86,8 @@ def self.check_attestation(bottle, signing_repo, signing_workflow = nil, subject
  raise GhAuthNeeded, "missing credentials" if credentials.blank?
 
  begin
- output = Utils.safe_popen_read({ "GH_TOKEN" => credentials }, *cmd)
+ result = system_command!(gh_executable, args: cmd, env: { "GH_TOKEN" => credentials },
+ secrets: [credentials])
  rescue ErrorDuringExecution => e
  # Even if we have credentials, they may be invalid or malformed.
  raise GhAuthNeeded, "invalid credentials" if e.status.exitstatus == 4
@@ -92,7 +96,7 @@ def self.check_attestation(bottle, signing_repo, signing_workflow = nil, subject
  end
 
  begin
- attestations = JSON.parse(output)
+ attestations = JSON.parse(result.stdout)
  rescue JSON::ParserError
  raise InvalidAttestationError, "attestation verification returned malformed JSON"
  end

diff --git a/Library/Homebrew/test/attestation_spec.rb b/Library/Homebrew/test/attestation_spec.rb
@@ -13,39 +13,45 @@
  let(:fake_bottle) do
  instance_double(Bottle, cached_download:, filename: fake_bottle_filename, url: fake_bottle_url)
  end
- let(:fake_json_resp) do
- JSON.dump([
- { verificationResult: {
- verifiedTimestamps: [{ timestamp: "2024-03-13T00:00:00Z" }],
- statement: { subject: [{ name: fake_bottle_filename.to_s }] },
- } },
- ])
+ let(:fake_result_invalid_json) { instance_double(SystemCommand::Result, stdout: "\"invalid JSON") }
+ let(:fake_result_json_resp) do
+ instance_double(SystemCommand::Result,
+ stdout: JSON.dump([
+ { verificationResult: {
+ verifiedTimestamps: [{ timestamp: "2024-03-13T00:00:00Z" }],
+ statement: { subject: [{ name: fake_bottle_filename.to_s }] },
+ } },
+ ]))
  end
- let(:fake_json_resp_backfill) do
- JSON.dump([
- { verificationResult: {
- verifiedTimestamps: [{ timestamp: "2024-03-13T00:00:00Z" }],
- statement: {
- subject: [{ name: "#{Digest::SHA256.hexdigest(fake_bottle_url)}--#{fake_bottle_filename}" }],
- },
- } },
- ])
+ let(:fake_result_json_resp_backfill) do
+ digest = Digest::SHA256.hexdigest(fake_bottle_url)
+ instance_double(SystemCommand::Result,
+ stdout: JSON.dump([
+ { verificationResult: {
+ verifiedTimestamps: [{ timestamp: "2024-03-13T00:00:00Z" }],
+ statement: {
+ subject: [{ name: "#{digest}--#{fake_bottle_filename}" }],
+ },
+ } },
+ ]))
  end
- let(:fake_json_resp_too_new) do
- JSON.dump([
- { verificationResult: {
- verifiedTimestamps: [{ timestamp: "2024-03-15T00:00:00Z" }],
- statement: { subject: [{ name: fake_bottle_filename.to_s }] },
- } },
- ])
+ let(:fake_result_json_resp_too_new) do
+ instance_double(SystemCommand::Result,
+ stdout: JSON.dump([
+ { verificationResult: {
+ verifiedTimestamps: [{ timestamp: "2024-03-15T00:00:00Z" }],
+ statement: { subject: [{ name: fake_bottle_filename.to_s }] },
+ } },
+ ]))
  end
  let(:fake_json_resp_wrong_sub) do
- JSON.dump([
- { verificationResult: {
- verifiedTimestamps: [{ timestamp: "2024-03-13T00:00:00Z" }],
- statement: { subject: [{ name: "wrong-subject.tar.gz" }] },
- } },
- ])
+ instance_double(SystemCommand::Result,
+ stdout: JSON.dump([
+ { verificationResult: {
+ verifiedTimestamps: [{ timestamp: "2024-03-13T00:00:00Z" }],
+ statement: { subject: [{ name: "wrong-subject.tar.gz" }] },
+ } },
+ ]))
  end
 
  describe "::gh_executable" do
@@ -78,9 +84,10 @@
  expect(GitHub::API).to receive(:credentials)
  .and_return(fake_gh_creds)
 
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::HOMEBREW_CORE_REPO, "--format", "json")
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::HOMEBREW_CORE_REPO, "--format", "json"],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
  .and_raise(ErrorDuringExecution.new(["foo"], status: fake_error_status))
 
  expect do
@@ -93,9 +100,10 @@
  expect(GitHub::API).to receive(:credentials)
  .and_return(fake_gh_creds)
 
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::HOMEBREW_CORE_REPO, "--format", "json")
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::HOMEBREW_CORE_REPO, "--format", "json"],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
  .and_raise(ErrorDuringExecution.new(["foo"], status: fake_auth_status))
 
  expect do
@@ -108,10 +116,11 @@
  expect(GitHub::API).to receive(:credentials)
  .and_return(fake_gh_creds)
 
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::HOMEBREW_CORE_REPO, "--format", "json")
- .and_return("\"invalid json")
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::HOMEBREW_CORE_REPO, "--format", "json"],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
+ .and_return(fake_result_invalid_json)
 
  expect do
  described_class.check_attestation fake_bottle,
@@ -123,9 +132,10 @@
  expect(GitHub::API).to receive(:credentials)
  .and_return(fake_gh_creds)
 
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::HOMEBREW_CORE_REPO, "--format", "json")
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::HOMEBREW_CORE_REPO, "--format", "json"],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
  .and_return(fake_json_resp_wrong_sub)
 
  expect do
@@ -145,43 +155,48 @@
  end
 
  it "calls gh with args for homebrew-core" do
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::HOMEBREW_CORE_REPO, "--format", "json", "--cert-identity",
- described_class::HOMEBREW_CORE_CI_URI)
- .and_return(fake_json_resp)
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::HOMEBREW_CORE_REPO, "--format", "json", "--cert-identity",
+ described_class::HOMEBREW_CORE_CI_URI],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
+ .and_return(fake_result_json_resp)
 
  described_class.check_core_attestation fake_bottle
  end
 
  it "calls gh with args for backfill when homebrew-core fails" do
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::HOMEBREW_CORE_REPO, "--format", "json", "--cert-identity",
- described_class::HOMEBREW_CORE_CI_URI)
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::HOMEBREW_CORE_REPO, "--format", "json", "--cert-identity",
+ described_class::HOMEBREW_CORE_CI_URI],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
  .once
  .and_raise(described_class::InvalidAttestationError)
 
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::BACKFILL_REPO, "--format", "json")
- .and_return(fake_json_resp_backfill)
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::BACKFILL_REPO, "--format", "json"],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
+ .and_return(fake_result_json_resp_backfill)
 
  described_class.check_core_attestation fake_bottle
  end
 
  it "raises when the backfilled attestation is too new" do
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::HOMEBREW_CORE_REPO, "--format", "json", "--cert-identity",
- described_class::HOMEBREW_CORE_CI_URI)
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::HOMEBREW_CORE_REPO, "--format", "json", "--cert-identity",
+ described_class::HOMEBREW_CORE_CI_URI],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
  .once
  .and_raise(described_class::InvalidAttestationError)
 
- expect(Utils).to receive(:safe_popen_read)
- .with({ "GH_TOKEN" => fake_gh_creds }, fake_gh, "attestation", "verify", cached_download, "--repo",
- described_class::BACKFILL_REPO, "--format", "json")
- .and_return(fake_json_resp_too_new)
+ expect(described_class).to receive(:system_command!)
+ .with(fake_gh, args: ["attestation", "verify", cached_download, "--repo",
+ described_class::BACKFILL_REPO, "--format", "json"],
+ env: { "GH_TOKEN" => fake_gh_creds }, secrets: [fake_gh_creds])
+ .and_return(fake_result_json_resp_too_new)
 
  expect do
  described_class.check_core_attestation fake_bottle