fix: stop the remaining production 500s reported by Sentry (#2435)

* fix: stop the remaining production 500s reported by Sentry

Four unresolved Sentry issues were still live on develop:

- MIRU-WEB-5Q: a multipart POST with an empty body crashed with
  Rack::Multipart::EmptyContentError because the reports/pdf throttle in
  rack_attack.rb read req.params for every request before checking the
  method and path. The throttle now returns early and reads the query string
  only, and EncodingSanitizer answers 400 for the whole Rack::Multipart error
  family instead of just BoundaryTooLongError.
- MIRU-WEB-58: ActionController::Base.forgery_protection_strategy was nil in
  this app despite load_defaults 8.0, so every unverified non-GET request to
  an ApplicationController descendant raised NoMethodError on
  `forgery_protection_strategy.new` (579 events, mostly scanners posting to
  the SPA catch-all). ApplicationController now declares
  protect_from_forgery with: :exception, which Rails maps to 422 and Sentry
  ignores.
- MIRU-WEB-5N: an unknown bill_status from the CLI API raised ArgumentError.
  Both TimesheetEntry enums validate instead, so the API returns 422.
- MIRU-WEB-5R: DatabaseBackupJob failed outright on a transient R2
  InternalError. It now retries S3 service and networking errors with
  polynomial backoff, and the upload closes its file handles.

Each fix carries a spec that failed before the change.

* Retry only transient S3 errors and test the throttle block directly

Review follow-ups: Aws::S3::Errors::ServiceError also covers permanent
errors such as AccessDenied and NoSuchBucket, which are not worth five
attempts, so the backup job now retries InternalError, ServiceUnavailable,
SlowDown, RequestTimeout and networking errors only. The multipart spec also
calls the reports/pdf throttle block with a malformed multipart request, so
a future change that parses the body inside the block fails the spec
regardless of routing.

Author: vipulnsward

app/controllers/application_controller.rb

@@ -8,6 +8,8 @@ class ApplicationController < ActionController::Base
   include Pagy::Backend
   include SetCurrentDetails
 
+  protect_from_forgery with: :exception
+
   # Vite handles asset compilation
 
   around_action :switch_locale

app/jobs/database_backup_job.rb

@@ -2,6 +2,9 @@
 
 class DatabaseBackupJob < ApplicationJob
   queue_as :default
+  retry_on "Aws::S3::Errors::InternalError", "Aws::S3::Errors::ServiceUnavailable", "Aws::S3::Errors::SlowDown",
+    "Aws::S3::Errors::RequestTimeout", "Seahorse::Client::NetworkingError",
+    wait: :polynomially_longer, attempts: 5
 
   def perform
     return unless ActiveModel::Type::Boolean.new.cast(ENV.fetch("DATABASE_BACKUP_ENABLED", Rails.env.production?))

app/models/timesheet_entry.rb

@@ -11,8 +11,8 @@ class TimesheetEntry < ApplicationRecord
   attribute :source_metadata, :json, default: {}
   attribute :proof_metadata, :json, default: {}
   attribute :review_status, :integer
-  enum :bill_status, [:non_billable, :unbilled, :billed]
-  enum :review_status, [:not_required, :pending_review, :approved, :rejected]
+  enum :bill_status, [:non_billable, :unbilled, :billed], validate: true
+  enum :review_status, [:not_required, :pending_review, :approved, :rejected], validate: true
 
   SOURCES = %w[manual cli mcp automation import].freeze
   SOURCE_METADATA_KEYS = %w[tool skill mcp_server].freeze

app/services/database_backup_service.rb

@@ -73,8 +73,8 @@ def run_pg_dump!(target_path)
     end
 
     def upload_backup!(path)
-      client.put_object(bucket: bucket_name, key: archive_key, body: File.open(path, "rb"))
-      client.put_object(bucket: bucket_name, key: latest_key, body: File.open(path, "rb"))
+      File.open(path, "rb") { |file| client.put_object(bucket: bucket_name, key: archive_key, body: file) }
+      File.open(path, "rb") { |file| client.put_object(bucket: bucket_name, key: latest_key, body: file) }
       archive_key
     end
 

config/initializers/encoding_sanitizer.rb

@@ -11,6 +11,15 @@ class EncodingSanitizer
     "application/x-www-form-urlencoded",
     "text/"
   ].freeze
+  MULTIPART_ERRORS = %i[
+    EmptyContentError
+    BoundaryTooLongError
+    MultipartPartLimitError
+    MultipartTotalPartLimitError
+    MissingInputError
+  ].filter_map do |name|
+    Rack::Multipart.const_get(name, false) if Rack::Multipart.const_defined?(name, false)
+  end.freeze
 
   def initialize(app)
     @app = app
@@ -33,7 +42,11 @@ def call(env)
     end
 
     @app.call(env)
-  rescue Rack::Multipart::BoundaryTooLongError
+  rescue *MULTIPART_ERRORS
+    [400, { "Content-Type" => "text/plain", "Content-Length" => "11" }, ["Bad Request"]]
+  rescue EOFError
+    raise unless multipart_form_data?(env)
+
     [400, { "Content-Type" => "text/plain", "Content-Length" => "11" }, ["Bad Request"]]
   end
 
@@ -63,6 +76,10 @@ def sanitize_request_body?(env)
       TEXTUAL_CONTENT_TYPES.any? { |type| content_type.start_with?(type) }
     end
 
+    def multipart_form_data?(env)
+      env["CONTENT_TYPE"].to_s.downcase.start_with?("multipart/form-data")
+    end
+
     # Wrapper for rack.input that sanitizes encoding on read
     class SanitizedInput < SimpleDelegator
       def initialize(input)

config/initializers/rack_attack.rb

@@ -23,8 +23,9 @@ class Rack::Attack
   end
 
   throttle("reports/pdf/ip", limit: 5, period: 1.minute) do |req|
-    pdf_download = req.path.end_with?(".pdf") || req.params["format"] == "pdf"
-    req.ip if req.get? && pdf_download && req.path.match?(%r{\A/api/v1/reports/[^/]+/download(?:\.pdf)?\z})
+    next unless req.get? && req.path.match?(%r{\A/api/v1/reports/[^/]+/download(?:\.pdf)?\z})
+
+    req.ip if req.path.end_with?(".pdf") || req.GET["format"] == "pdf"
   end
 
   throttle("invitations/resend/ip", limit: 5, period: 1.minute) do |req|

spec/jobs/database_backup_job_spec.rb

@@ -29,4 +29,15 @@
 
     expect(DatabaseBackupService).not_to have_received(:new)
   end
+
+  it "retries S3 service errors" do
+    ENV["DATABASE_BACKUP_ENABLED"] = "true"
+    service = instance_double(DatabaseBackupService)
+    allow(DatabaseBackupService).to receive(:new).and_return(service)
+    allow(service).to receive(:process).and_raise(Aws::S3::Errors::InternalError.new(nil, "boom"))
+
+    described_class.perform_now
+
+    expect(described_class).to have_been_enqueued
+  end
 end

spec/middleware/encoding_sanitizer_spec.rb

@@ -104,6 +104,15 @@
         expect(status).to eq(400)
         expect(body).to eq(["Bad Request"])
       end
+
+      it "returns bad request when multipart content is empty" do
+        app = ->(_env) { raise Rack::Multipart::EmptyContentError }
+
+        status, _headers, body = described_class.new(app).call({})
+
+        expect(status).to eq(400)
+        expect(body).to eq(["Bad Request"])
+      end
     end
   end
 

spec/models/timesheet_entry_spec.rb

@@ -61,6 +61,14 @@
         .is_less_than_or_equal_to(6000000)
         .is_greater_than_or_equal_to(0.0)
     end
+
+    it "validates an invalid bill status" do
+      timesheet_entry = build(:timesheet_entry)
+
+      expect { timesheet_entry.bill_status = "zzz" }.not_to raise_error
+      expect(timesheet_entry).not_to be_valid
+      expect(timesheet_entry.errors[:bill_status]).to be_present
+    end
   end
 
   describe "Callbacks" do

spec/requests/api/v1/cli/timesheet_entries/create_spec.rb

@@ -76,4 +76,20 @@
 
     expect(response).to have_http_status(:not_found)
   end
+
+  it "returns an error for an invalid bill status" do
+    create(:project_member, project:, user:)
+
+    send_request :post, api_v1_cli_timesheet_entries_path, params: {
+      timesheet_entry: {
+        project_id: project.id,
+        duration_minutes: 90,
+        work_date: Date.current.iso8601,
+        bill_status: "zzz"
+      }
+    }, headers: cli_auth_headers(cli_token)
+
+    expect(response).to have_http_status(:unprocessable_content)
+    expect(json_response["errors"]).to be_present
+  end
 end