Skip to content

Use Tempfile for auto-attestation bundles to avoid file conflicts and cleanup issues #9327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hsbt merged 3 commits into from
Feb 10, 2026
Merged

Use Tempfile for auto-attestation bundles to avoid file conflicts and cleanup issues #9327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78257c3
Initial plan
Copilot Feb 10, 2026
9916ed3
Use Tempfile for auto-attestation bundles and clean up after use
Copilot Feb 10, 2026
ffcce01
Fix Tempfile to not auto-delete on close
Copilot Feb 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/rubygems/commands/push_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,12 @@ def send_push_request_with_attestation(name, args)
Gem.read_binary(attestation)
end
else
[Gem.read_binary(attest!(name))]
bundle_path = attest!(name)
begin
[Gem.read_binary(bundle_path)]
ensure
File.unlink(bundle_path) if bundle_path && File.exist?(bundle_path)
end
Comment on lines +125 to +129
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ensure cleanup uses File.unlink(...) without rescuing errors. If unlink fails (Windows file locking, permissions, race between exist? and unlink), the push could error out even though the request was constructed successfully. Prefer a non-raising cleanup (FileUtils.rm_f) or rescue SystemCallError around the unlink so cleanup failure doesn’t break the command.

Copilot uses AI. Check for mistakes.
end
bundles = "[" + attestations.join(",") + "]"

Expand All @@ -136,8 +141,14 @@ def send_push_request_with_attestation(name, args)

def attest!(name)
require "open3"
require "tempfile"

# Create a temporary file for the bundle
basename = File.basename(name, ".*")
tempfile = Tempfile.new([basename, ".sigstore.json"])
bundle = tempfile.path
tempfile.close(false) # Close but don't unlink - we need the file for sigstore-cli
Comment on lines +146 to +150
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attest! creates a Tempfile but only returns its path. Since Tempfile registers a finalizer to unlink the file on GC, the bundle can be deleted between returning from attest! and Gem.read_binary(bundle_path), causing intermittent ENOENT and attestation fallback. Consider managing the tempfile lifetime across signing+reading (e.g., create/hold the Tempfile in send_push_request_with_attestation and only unlink after reading, or use an approach that disables Tempfile auto-unlink and ensures manual cleanup on both success and failure).

Copilot uses AI. Check for mistakes.

bundle = "#{name}.sigstore.json"
env = defined?(Bundler.unbundled_env) ? Bundler.unbundled_env : ENV.to_h
out, st = Open3.capture2e(
env,
Expand Down
5 changes: 3 additions & 2 deletions test/rubygems/test_gem_commands_push_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@ def test_execute_attestation_auto
@fetcher.data["#{Gem.host}/api/v1/gems"] = HTTPResponseFactory.create(body: @response, code: 200, msg: "OK")

attestation_path = "#{@path}.sigstore.json"
File.write(attestation_path, "auto-attestation")
attestation_content = "auto-attestation"
File.write(attestation_path, attestation_content)
@cmd.options[:args] = [@path]

@cmd.stub(:attest!, attestation_path) do
Expand All @@ -133,7 +134,7 @@ def test_execute_attestation_auto
assert_equal Gem::Net::HTTP::Post, @fetcher.last_request.class
content_length = @fetcher.last_request["Content-Length"].to_i
assert_equal content_length, @fetcher.last_request.body.length
assert_attestation_multipart Gem.read_binary(attestation_path)
assert_attestation_multipart attestation_content
Comment on lines 130 to +137
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test exercises the auto-attestation path but no longer asserts the key new behavior: that the generated bundle file is cleaned up after request construction. Add an assertion after @cmd.execute that the file at attestation_path no longer exists to prevent regressions in the cleanup logic.

Copilot uses AI. Check for mistakes.
end

def test_execute_attestation_fallback
Expand Down
Loading