-
-
Notifications
You must be signed in to change notification settings - Fork 981
Add Attestation Repair action In Avo #6186
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| class Avo::Actions::RepairAttestation < Avo::Actions::ApplicationAction | ||
| self.name = "Repair attestation" | ||
| self.visible = lambda { | ||
| current_user.team_member?("rubygems-org") && view == :show | ||
| } | ||
| self.message = lambda { | ||
| if resource.record.repairable? | ||
| "This attestation has invalid data that needs repair. Proceed with repair?" | ||
| else | ||
| "This attestation appears valid. No repairs are expected." | ||
| end | ||
| } | ||
| self.confirm_button_label = "Repair attestation" | ||
|
|
||
| class ActionHandler < Avo::Actions::ActionHandler | ||
| def handle_record(attestation) | ||
| changes = attestation.repair! | ||
|
|
||
| if changes | ||
| succeed "Attestation repaired: #{changes.join(', ')}" | ||
| else | ||
| succeed "No repair was needed" | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,9 +2,17 @@ class Avo::Resources::Attestation < Avo::BaseResource | |||||||||
| self.title = :id | ||||||||||
| self.includes = [:version] | ||||||||||
|
|
||||||||||
| def actions | ||||||||||
| action Avo::Actions::RepairAttestation | ||||||||||
| end | ||||||||||
|
|
||||||||||
| def fields | ||||||||||
| field :id, as: :id | ||||||||||
|
|
||||||||||
| field :valid, as: :boolean, only_on: %i[index show] do | ||||||||||
| record.valid_bundle? | ||||||||||
|
||||||||||
| record.valid_bundle? | |
| Rails.cache.fetch(["attestation-valid-bundle", record.id, record.updated_at]) do | |
| record.valid_bundle? | |
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the purpose of this to see if the attestation needs repairing? It still shows as valid when it does need to be repaired.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,6 @@ | ||||||||
| class Attestation < ApplicationRecord | ||||||||
| include AttestationBundleRepair | ||||||||
|
|
||||||||
| belongs_to :version | ||||||||
|
|
||||||||
| validates :body, :media_type, presence: true | ||||||||
|
|
@@ -10,6 +12,12 @@ def sigstore_bundle | |||||||
| ) | ||||||||
| end | ||||||||
|
|
||||||||
| def valid_bundle? | ||||||||
| sigstore_bundle.present? | ||||||||
| rescue Sigstore::Error | ||||||||
|
||||||||
| rescue Sigstore::Error | |
| rescue StandardError => e | |
| Rails.logger.warn("Failed to validate sigstore bundle for Attestation #{id}: #{e.class}: #{e.message}") if defined?(Rails) && Rails.respond_to?(:logger) && Rails.logger |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new valid_bundle? method in the Attestation model lacks test coverage. Consider adding tests to verify that it correctly returns true for valid bundles and false for invalid bundles (both for Sigstore::Error and other potential exceptions).
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,64 @@ | ||||||||
| module AttestationBundleRepair | ||||||||
| extend ActiveSupport::Concern | ||||||||
|
|
||||||||
| def repairable? | ||||||||
| verification = body["verificationMaterial"] | ||||||||
| return false if verification.blank? | ||||||||
|
|
||||||||
| missing_kind_version?(verification) || double_encoded_certificate?(verification) | ||||||||
| end | ||||||||
|
|
||||||||
| def repair! | ||||||||
| verification = body["verificationMaterial"] | ||||||||
| return false if verification.blank? | ||||||||
|
|
||||||||
| new_body = body.deep_dup | ||||||||
| new_verification = new_body["verificationMaterial"] | ||||||||
| changes = [] | ||||||||
|
|
||||||||
| repair_kind_version!(new_verification, changes) if missing_kind_version?(verification) | ||||||||
| repair_certificate!(new_verification, changes) if double_encoded_certificate?(verification) | ||||||||
|
|
||||||||
| return false if changes.empty? | ||||||||
|
|
||||||||
| update!(body: new_body) | ||||||||
| changes | ||||||||
| end | ||||||||
|
Comment on lines
+1
to
+26
|
||||||||
|
|
||||||||
| private | ||||||||
|
|
||||||||
| def missing_kind_version?(verification) | ||||||||
| verification["tlogEntries"]&.any? { |entry| !entry.key?("kindVersion") } | ||||||||
| end | ||||||||
|
|
||||||||
| def double_encoded_certificate?(verification) | ||||||||
| return false unless (raw_bytes = verification.dig("certificate", "rawBytes")) | ||||||||
|
|
||||||||
| decoded = Base64.strict_decode64(raw_bytes) | ||||||||
| decoded.start_with?("-----BEGIN CERTIFICATE-----") | ||||||||
| rescue ArgumentError | ||||||||
| false | ||||||||
| end | ||||||||
|
|
||||||||
| def repair_kind_version!(verification, changes) | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at our attestations, I believe the kind should be |
||||||||
| verification["tlogEntries"]&.each_with_index do |entry, idx| | ||||||||
| next if entry.key?("kindVersion") | ||||||||
| entry["kindVersion"] = { "kind" => "dsse", "version" => "0.0.1" } | ||||||||
|
||||||||
| changes << "Added missing kindVersion to tlogEntry #{idx}" | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| def repair_certificate!(verification, changes) | ||||||||
| raw_bytes = verification.dig("certificate", "rawBytes") | ||||||||
| return unless raw_bytes | ||||||||
|
|
||||||||
| decoded = Base64.strict_decode64(raw_bytes) | ||||||||
| return unless decoded.start_with?("-----BEGIN CERTIFICATE-----") | ||||||||
|
|
||||||||
| cert = OpenSSL::X509::Certificate.new(decoded) | ||||||||
| verification["certificate"]["rawBytes"] = Base64.strict_encode64(cert.to_der) | ||||||||
| changes << "Converted double-encoded PEM certificate to DER" | ||||||||
| rescue ArgumentError, OpenSSL::X509::CertificateError => e | ||||||||
| Rails.logger.warn("Failed to repair certificate: #{e.message}") | ||||||||
|
||||||||
| Rails.logger.warn("Failed to repair certificate: #{e.message}") | |
| Rails.logger.warn("Failed to repair certificate: #{e.message}") | |
| changes << "Failed to repair certificate: #{e.message}" |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,4 +4,68 @@ class AttestationTest < ActiveSupport::TestCase | |||||
| should belong_to(:version) | ||||||
| should validate_presence_of(:media_type) | ||||||
| should validate_presence_of(:body) | ||||||
|
|
||||||
| context "#repairable?" do | ||||||
| setup do | ||||||
| @version = create(:version) | ||||||
| end | ||||||
|
|
||||||
| should "be repairable when verificationMaterial is missing" do | ||||||
| attestation = Attestation.new( | ||||||
| version: @version, | ||||||
| media_type: "application/vnd.dev.sigstore.bundle.v0.3+json", | ||||||
| body: {} | ||||||
| ) | ||||||
|
|
||||||
| refute_predicate attestation, :repairable? | ||||||
| end | ||||||
|
Comment on lines
+13
to
+21
|
||||||
|
|
||||||
| should "be repairable when kindVersion is missing from tlog entry" do | ||||||
| attestation = Attestation.new( | ||||||
| version: @version, | ||||||
| media_type: "application/vnd.dev.sigstore.bundle.v0.3+json", | ||||||
| body: { | ||||||
| "verificationMaterial" => { | ||||||
| "tlogEntries" => [{ "logIndex" => 123 }], | ||||||
| "certificate" => { "rawBytes" => Base64.strict_encode64("DER data") } | ||||||
| } | ||||||
| } | ||||||
| ) | ||||||
|
|
||||||
| assert_predicate attestation, :repairable? | ||||||
| end | ||||||
|
|
||||||
| should "be repairable when certificate is double-encoded PEM" do | ||||||
| pem_cert = "-----BEGIN CERTIFICATE-----\nMIIBkTCB+wIJAKHBfpN...\n-----END CERTIFICATE-----\n" | ||||||
| double_encoded = Base64.strict_encode64(pem_cert) | ||||||
|
|
||||||
| attestation = Attestation.new( | ||||||
| version: @version, | ||||||
| media_type: "application/vnd.dev.sigstore.bundle.v0.3+json", | ||||||
| body: { | ||||||
| "verificationMaterial" => { | ||||||
| "tlogEntries" => [{ "kindVersion" => { "kind" => "dsse", "version" => "0.0.1" } }], | ||||||
| "certificate" => { "rawBytes" => double_encoded } | ||||||
| } | ||||||
| } | ||||||
| ) | ||||||
|
|
||||||
| assert_predicate attestation, :repairable? | ||||||
| end | ||||||
|
|
||||||
| should "be not repairable when bundle has no known issues" do | ||||||
|
||||||
| should "be not repairable when bundle has no known issues" do | |
| should "not be repairable when bundle has no known issues" do |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repair! method from the AttestationBundleRepair concern is only tested indirectly through the Avo action tests. Consider adding direct unit tests for the repair! method in test/models/attestation_test.rb to ensure it properly modifies the attestation body and returns the expected changes array.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| require "test_helper" | ||
|
|
||
| class RepairAttestationTest < ActiveSupport::TestCase | ||
| make_my_diffs_pretty! | ||
|
|
||
| setup do | ||
| @view_context = mock | ||
| @avo = mock | ||
| @view_context.stubs(:avo).returns(@avo) | ||
| @avo.stubs(:resources_audit_path).returns("resources_audit_path") | ||
| Avo::Current.stubs(:view_context).returns(@view_context) | ||
| @admin = create(:admin_github_user, :is_admin) | ||
| @version = create(:version) | ||
| end | ||
|
|
||
| test "repairs attestation with missing kindVersion" do | ||
| attestation = Attestation.create!( | ||
| version: @version, | ||
| media_type: "application/vnd.dev.sigstore.bundle.v0.3+json", | ||
| body: { | ||
| "verificationMaterial" => { | ||
| "tlogEntries" => [{ "logIndex" => 123 }], | ||
| "certificate" => { "rawBytes" => Base64.strict_encode64("DER data") } | ||
| } | ||
| } | ||
|
Comment on lines
+20
to
+25
Check failureCode scanning / CodeQL Clear-text storage of sensitive information High test
This stores sensitive data returned by
a write to certificate Error loading related location Loading Copilot AutofixAI 7 days ago Copilot could not generate an autofix suggestion Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support. |
||
| ) | ||
|
|
||
| assert_predicate attestation, :repairable? | ||
|
|
||
| action = Avo::Actions::RepairAttestation.new | ||
| action.handle( | ||
| fields: { comment: "Repairing invalid attestation" }, | ||
| current_user: @admin, | ||
| resource: nil, | ||
| records: [attestation], | ||
| query: nil | ||
| ) | ||
|
|
||
| attestation.reload | ||
|
|
||
| refute_predicate attestation, :repairable? | ||
| assert_equal( | ||
| { "kind" => "dsse", "version" => "0.0.1" }, | ||
| attestation.body.dig("verificationMaterial", "tlogEntries", 0, "kindVersion") | ||
| ) | ||
| end | ||
|
Comment on lines
+16
to
+46
|
||
|
|
||
| test "repairs attestation with double-encoded PEM certificate" do | ||
| key = OpenSSL::PKey::RSA.new(2048) | ||
| cert = OpenSSL::X509::Certificate.new | ||
| cert.version = 2 | ||
| cert.serial = 1 | ||
| cert.subject = OpenSSL::X509::Name.parse("/CN=Test") | ||
| cert.issuer = cert.subject | ||
| cert.public_key = key.public_key | ||
| cert.not_before = Time.current | ||
| cert.not_after = Time.current + 3600 | ||
| cert.sign(key, OpenSSL::Digest.new("SHA256")) | ||
|
|
||
| pem_cert = cert.to_pem | ||
| double_encoded = Base64.strict_encode64(pem_cert) | ||
|
|
||
| attestation = Attestation.create!( | ||
| version: @version, | ||
| media_type: "application/vnd.dev.sigstore.bundle.v0.3+json", | ||
| body: { | ||
| "verificationMaterial" => { | ||
| "tlogEntries" => [{ "kindVersion" => { "kind" => "dsse", "version" => "0.0.1" } }], | ||
| "certificate" => { "rawBytes" => double_encoded } | ||
| } | ||
| } | ||
|
Comment on lines
+66
to
+71
Check failureCode scanning / CodeQL Clear-text storage of sensitive information High test
This stores sensitive data returned by
a write to certificate Error loading related location Loading Copilot AutofixAI 7 days ago Copilot could not generate an autofix suggestion Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support. |
||
| ) | ||
|
|
||
| assert_predicate attestation, :repairable? | ||
|
|
||
| action = Avo::Actions::RepairAttestation.new | ||
| action.handle( | ||
| fields: { comment: "Repairing invalid attestation" }, | ||
| current_user: @admin, | ||
| resource: nil, | ||
| records: [attestation], | ||
| query: nil | ||
| ) | ||
|
|
||
| attestation.reload | ||
|
|
||
| refute_predicate attestation, :repairable? | ||
|
|
||
| raw_bytes = attestation.body.dig("verificationMaterial", "certificate", "rawBytes") | ||
| decoded = Base64.strict_decode64(raw_bytes) | ||
|
|
||
| refute decoded.start_with?("-----BEGIN CERTIFICATE-----") | ||
| end | ||
|
Comment on lines
+48
to
+93
|
||
|
|
||
| test "does nothing for attestation without known issues" do | ||
| attestation = Attestation.create!( | ||
| version: @version, | ||
| media_type: "application/vnd.dev.sigstore.bundle.v0.3+json", | ||
| body: { | ||
| "verificationMaterial" => { | ||
| "tlogEntries" => [{ "kindVersion" => { "kind" => "dsse", "version" => "0.0.1" } }], | ||
| "certificate" => { "rawBytes" => Base64.strict_encode64("DER data") } | ||
| } | ||
| } | ||
|
Comment on lines
+99
to
+104
Check failureCode scanning / CodeQL Clear-text storage of sensitive information High test
This stores sensitive data returned by
a write to certificate Error loading related location Loading Copilot AutofixAI 7 days ago Copilot could not generate an autofix suggestion Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support. |
||
| ) | ||
|
|
||
| refute_predicate attestation, :repairable? | ||
| original_body = attestation.body.deep_dup | ||
|
|
||
| action = Avo::Actions::RepairAttestation.new | ||
| action.handle( | ||
| fields: { comment: "Attempting repair on valid attestation" }, | ||
| current_user: @admin, | ||
| resource: nil, | ||
| records: [attestation], | ||
| query: nil | ||
| ) | ||
|
|
||
| attestation.reload | ||
|
|
||
| assert_equal original_body, attestation.body | ||
| end | ||
|
|
||
| test "repairs both missing kindVersion and double-encoded certificate simultaneously" do | ||
| key = OpenSSL::PKey::RSA.new(2048) | ||
| cert = OpenSSL::X509::Certificate.new | ||
| cert.version = 2 | ||
| cert.serial = 1 | ||
| cert.subject = OpenSSL::X509::Name.parse("/CN=Test") | ||
| cert.issuer = cert.subject | ||
| cert.public_key = key.public_key | ||
| cert.not_before = Time.current | ||
| cert.not_after = Time.current + 3600 | ||
| cert.sign(key, OpenSSL::Digest.new("SHA256")) | ||
|
|
||
| pem_cert = cert.to_pem | ||
| double_encoded = Base64.strict_encode64(pem_cert) | ||
|
|
||
| attestation = Attestation.create!( | ||
| version: @version, | ||
| media_type: "application/vnd.dev.sigstore.bundle.v0.3+json", | ||
| body: { | ||
| "verificationMaterial" => { | ||
| "tlogEntries" => [{ "logIndex" => 123 }], | ||
| "certificate" => { "rawBytes" => double_encoded } | ||
| } | ||
| } | ||
|
Comment on lines
+142
to
+147
Check failureCode scanning / CodeQL Clear-text storage of sensitive information High test
This stores sensitive data returned by
a write to certificate Error loading related location Loading Copilot AutofixAI 7 days ago Copilot could not generate an autofix suggestion Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support. |
||
| ) | ||
|
|
||
| assert_predicate attestation, :repairable? | ||
|
|
||
| action = Avo::Actions::RepairAttestation.new | ||
| action.handle( | ||
| fields: { comment: "Repairing attestation with both issues" }, | ||
| current_user: @admin, | ||
| resource: nil, | ||
| records: [attestation], | ||
| query: nil | ||
| ) | ||
|
|
||
| attestation.reload | ||
|
|
||
| refute_predicate attestation, :repairable? | ||
|
|
||
| # Verify kindVersion was added | ||
| assert_equal( | ||
| { "kind" => "dsse", "version" => "0.0.1" }, | ||
| attestation.body.dig("verificationMaterial", "tlogEntries", 0, "kindVersion") | ||
| ) | ||
|
|
||
| # Verify certificate was converted to DER | ||
| raw_bytes = attestation.body.dig("verificationMaterial", "certificate", "rawBytes") | ||
| decoded = Base64.strict_decode64(raw_bytes) | ||
|
|
||
| refute decoded.start_with?("-----BEGIN CERTIFICATE-----") | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's potential to say what is exactly wrong aka kind version, double encode or both, but this is fine for now.