Store any exceptions as an attachment

.github/workflows/ci.yml

@@ -34,15 +34,30 @@ jobs:
       - name: Lint code for consistent style
         run: bin/rubocop -f github
 
+  security:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v5
+
+      - name: Set up Ruby
+        uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: ruby-4.0.0
+          bundler-cache: true
+
+      - name: Run Brakeman security scan
+        run: bin/brakeman --quiet --no-pager --exit-on-warn --exit-on-error
+
   test:
     runs-on: ubuntu-latest
 
-    # services:
-    #  redis:
-    #    image: valkey/valkey:8
-    #    ports:
-    #      - 6379:6379
-    #    options: --health-cmd "redis-cli ping" --health-interval 10s --health-timeout 5s --health-retries 5
+    services:
+      upright-playwright:
+        image: jacoblincool/playwright:chromium-server-1.55.0
+        ports:
+          - 53333:53333
+
     steps:
       - name: Checkout code
         uses: actions/checkout@v5

.rubocop.yml

@@ -1,8 +1,8 @@
 # Omakase Ruby styling for Rails
 inherit_gem: { rubocop-rails-omakase: rubocop.yml }
 
-# Overwrite or add rules to create your own house style
-#
-# # Use `[a, [b, c]]` not `[ a, [ b, c ] ]`
-# Layout/SpaceInsideArrayLiteralBrackets:
-#   Enabled: false
+AllCops:
+  Exclude:
+    - test/dummy/db/schema.rb
+    - test/dummy/db/queue_schema.rb
+    - test/dummy/db/migrate/**/*

Gemfile

@@ -9,6 +9,7 @@ gem "puma"
 gem "sqlite3"
 gem "propshaft"
 gem "rubocop-rails-omakase", require: false
+gem "brakeman", require: false
 
 # Forked for Ruby 3.4 compatibility (required by engine, git source not supported in gemspec)
 gem "geohash_ruby", github: "lewispb/geohash_ruby", branch: "fix-ruby-34-warnings"

Gemfile.lock

@@ -119,6 +119,8 @@ GEM
     base64 (0.3.0)
     bigdecimal (4.0.1)
     bindata (2.5.1)
+    brakeman (7.1.2)
+      racc
     builder (3.3.0)
     concurrent-ruby (1.3.6)
     connection_pool (3.0.2)
@@ -607,6 +609,7 @@ PLATFORMS
   x86_64-linux
 
 DEPENDENCIES
+  brakeman
   geohash_ruby!
   mocha
   propshaft
@@ -637,6 +640,7 @@ CHECKSUMS
   base64 (0.3.0) sha256=27337aeabad6ffae05c265c450490628ef3ebd4b67be58257393227588f5a97b
   bigdecimal (4.0.1) sha256=8b07d3d065a9f921c80ceaea7c9d4ae596697295b584c296fe599dd0ad01c4a7
   bindata (2.5.1) sha256=53186a1ec2da943d4cb413583d680644eb810aacbf8902497aac8f191fad9e58
+  brakeman (7.1.2) sha256=6b04927710a2e7d13a72248b5d404c633188e02417f28f3d853e4b6370d26dce
   builder (3.3.0) sha256=497918d2f9dca528fdca4b88d84e4ef4387256d984b8154e9d5d3fe5a9c8835f
   concurrent-ruby (1.3.6) sha256=6b56837e1e7e5292f9864f34b69c5a2cbc75c0cf5338f1ce9903d10fa762d5ab
   connection_pool (3.0.2) sha256=33fff5ba71a12d2aa26cb72b1db8bba2a1a01823559fb01d29eb74c286e62e0a

app/models/concerns/upright/exception_recording.rb

@@ -0,0 +1,38 @@
+module Upright::ExceptionRecording
+  extend ActiveSupport::Concern
+
+  EXCEPTION_FILENAME = "exception.txt"
+
+  included do
+    attr_accessor :error
+
+    after_create :attach_exception
+  end
+
+  def attach_exception
+    if error
+      artifacts.attach(
+        io: StringIO.new(format_exception(error)),
+        filename: EXCEPTION_FILENAME,
+        content_type: "text/plain"
+      )
+    end
+  end
+
+  def exception_artifact
+    artifacts.find { |a| a.filename.to_s == EXCEPTION_FILENAME }
+  end
+
+  def exception_report
+    exception_artifact&.download
+  end
+
+  private
+    def format_exception(exception)
+      lines = [ "#{exception.class}: #{exception.message}" ]
+      if exception.backtrace
+        lines.concat(exception.backtrace.first(20).map { |line| "  #{line}" })
+      end
+      lines.join("\n")
+    end
+end

app/models/concerns/upright/probeable.rb

@@ -28,17 +28,17 @@ def check_and_record_later
   def check_and_record
     result = failsafe_check
 
-    probe_result = Upright::ProbeResult.create! \
+    Upright::ProbeResult.create!(
       probe_type: probe_type,
       probe_name: probe_name,
       probe_target: probe_target,
       probe_service: probe_service,
       status: result[:status],
-      duration: result[:duration]
-
-    on_check_recorded(probe_result)
-
-    probe_result
+      duration: result[:duration],
+      error: result[:error]
+    ).tap do |probe_result|
+      on_check_recorded(probe_result)
+    end
   end
 
   def probe_name
@@ -54,7 +54,7 @@ def probe_target
   end
 
   def on_check_recorded(probe_result)
-    # Optional hook for subclasses
+    raise NotImplementedError
   end
 
   def probe_service
@@ -72,7 +72,7 @@ def failsafe_check
     ensure
       log_probe_result(result:, error:, duration:)
 
-      return { status: result_description(result, error), duration: }
+      return { status: result_description(result, error), duration:, error: }
     end
 
     def result_description(result, error = nil)

app/models/upright/probe_result.rb

@@ -1,4 +1,6 @@
 class Upright::ProbeResult < Upright::ApplicationRecord
+  include Upright::ExceptionRecording
+
   self.table_name = "upright_probe_results"
 
   scope :by_type, ->(type) { where(probe_type: type) if type.present? }

bin/brakeman

@@ -0,0 +1,7 @@
+#!/usr/bin/env ruby
+require "rubygems"
+require "bundler/setup"
+
+ARGV.unshift("--ensure-latest")
+
+load Gem.bin_path("brakeman", "brakeman")

bin/ci

@@ -0,0 +1,6 @@
+#!/usr/bin/env ruby
+require_relative "../test/dummy/config/boot"
+require "active_support/continuous_integration"
+
+CI = ActiveSupport::ContinuousIntegration
+require_relative "../config/ci.rb"

config/brakeman.ignore

@@ -0,0 +1,39 @@
+{
+  "ignored_warnings": [
+    {
+      "warning_type": "Cross-Site Scripting",
+      "warning_code": 4,
+      "fingerprint": "842fc181b52f46f9a26f463b023f2567ff74c2b5b2b79c6004bab29f2e62203c",
+      "check_name": "LinkToHref",
+      "message": "Potentially unsafe model attribute in `link_to` href",
+      "file": "app/views/upright/nodes/index.html.erb",
+      "line": 16,
+      "link": "https://brakemanscanner.org/docs/warning_types/link_to_href",
+      "code": "link_to(Upright::Node.new.hostname, Upright::Node.new.url)",
+      "render_path": [
+        {
+          "type": "controller",
+          "class": "Upright::NodesController",
+          "method": "index",
+          "line": 5,
+          "file": "app/controllers/upright/nodes_controller.rb",
+          "rendered": {
+            "name": "upright/nodes/index",
+            "file": "app/views/upright/nodes/index.html.erb"
+          }
+        }
+      ],
+      "location": {
+        "type": "template",
+        "template": "upright/nodes/index"
+      },
+      "user_input": "Upright::Node.new.url",
+      "confidence": "Weak",
+      "cwe_id": [
+        79
+      ],
+      "note": "URL is generated from Rails route helpers, not user input"
+    }
+  ],
+  "brakeman_version": "7.1.2"
+}

config/ci.rb

@@ -0,0 +1,7 @@
+# Run using bin/ci
+
+CI.run do
+  step "Style: Ruby", "bin/rubocop"
+  step "Security: Brakeman code analysis", "bin/brakeman --quiet --no-pager --exit-on-warn --exit-on-error"
+  step "Tests: Rails", "bin/rails test"
+end

lib/upright.rb

@@ -100,5 +100,4 @@ def load_sites
         end
       end
   end
-
 end

test/controllers/nodes_controller_test.rb

@@ -18,5 +18,4 @@ class NodesControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_equal "nodes", @controller.controller_name
   end
-
 end

test/controllers/probe_results_controller_test.rb

@@ -93,5 +93,4 @@ class ProbeResultsControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_select "details.artifact-popover"
   end
-
 end

test/controllers/sessions_controller_test.rb

@@ -27,5 +27,4 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
 
     assert_response :not_found
   end
-
 end

test/fixtures/upright_probe_results.yml

@@ -40,3 +40,12 @@ http_probe_result_failed:
   status: "fail"
   created_at: <%= Time.current %>
   updated_at: <%= Time.current %>
+
+http_probe_result_error:
+  probe_type: http
+  probe_name: HTTP Probe
+  probe_target: https://example.com
+  duration: 1.0
+  status: "error"
+  created_at: <%= Time.current %>
+  updated_at: <%= Time.current %>

test/models/upright/probe_result_test.rb

@@ -18,4 +18,21 @@ class Upright::ProbeResultTest < ActiveSupport::TestCase
 
     assert_equal 0.0, chart_data[:duration]
   end
+
+  test "error attribute attaches exception report on create" do
+    exception = RuntimeError.new("Something went wrong")
+    exception.set_backtrace([ "app/models/foo.rb:10", "app/controllers/bar.rb:5" ])
+
+    result = Upright::ProbeResult.create!(
+      probe_name: "test", probe_type: "http", probe_target: "https://example.com",
+      status: :error, duration: 1.0, error: exception
+    )
+
+    expected = <<~REPORT.chomp
+      RuntimeError: Something went wrong
+        app/models/foo.rb:10
+        app/controllers/bar.rb:5
+    REPORT
+    assert_equal expected, result.exception_report
+  end
 end

test/models/upright/probes/http_probe_test.rb

@@ -124,20 +124,19 @@ class Upright::Probes::HTTPProbeTest < ActiveSupport::TestCase
     assert_equal "https://example.com/", result.probe_target
   end
 
-  test "check_and_record creates result with error status when check raises" do
+  test "check_and_record creates error result with exception artifact when check raises" do
     stub_request(:get, "https://example.com/").to_return(status: 200)
     set_test_site
     probe = Upright::Probes::HTTPProbe.new(name: "test", url: "https://example.com/")
     probe.logger = null_logger
-    probe.stubs(:check).raises(StandardError.new("unexpected error"))
+    probe.stubs(:check).raises(RuntimeError.new("connection refused"))
     Rails.error.stubs(:report)
 
-    assert_difference -> { Upright::ProbeResult.count } do
-      probe.check_and_record
-    end
+    probe.check_and_record
 
     result = Upright::ProbeResult.last
     assert_equal "error", result.status
+    assert_match(/RuntimeError: connection refused/, result.exception_report)
   end
 
   test "check_and_record attaches curl log and response as artifacts" do

test/models/upright/probes/playwright_probe_test.rb

@@ -90,8 +90,10 @@ def check
       assert_equal "error", probe_result.status
       assert_equal "playwright", probe_result.probe_type
       assert_equal "failing_probe", probe_result.probe_name
-      assert probe_result.artifacts.attached?, "Expected video artifact to be attached"
-      assert_equal "video/webm", probe_result.artifacts.first.content_type
+      assert probe_result.artifacts.attached?, "Expected artifacts to be attached"
+
+      video_artifact = probe_result.artifacts.find { |a| a.content_type == "video/webm" }
+      assert video_artifact, "Expected video artifact to be attached"
     end
   end