From e92eef04ccaf4f6a3944dcac93ad0510756c71d0 Mon Sep 17 00:00:00 2001
From: fiveNinePlusR <fiveNinePlusR@gmail.com>
Date: Sun, 29 Sep 2024 19:58:37 -0700
Subject: [PATCH 1/2] add some security fixes for brakeman

---
 .../v1/location_machine_xrefs_controller.rb   |  6 +-
 .../v1/location_picture_xrefs_controller.rb   |  4 +-
 app/models/location.rb                        |  4 +-
 app/models/machine_score_xref.rb              |  8 +-
 app/models/suggested_location.rb              | 12 ++-
 app/models/user.rb                            |  2 +-
 app/views/locations/_form.html.haml           |  2 +-
 app/views/locations/_locations.html.haml      |  2 +-
 bin/bundle-audit                              | 27 ++++++
 bin/bundler-audit                             | 27 ++++++
 config/brakeman.ignore                        | 94 +++++++++++++++++++
 config/initializers/cookies_serializer.rb     |  2 +-
 12 files changed, 170 insertions(+), 20 deletions(-)
 create mode 100755 bin/bundle-audit
 create mode 100755 bin/bundler-audit
 create mode 100644 config/brakeman.ignore

diff --git a/app/controllers/api/v1/location_machine_xrefs_controller.rb b/app/controllers/api/v1/location_machine_xrefs_controller.rb
index f55a90568..1334c37f6 100644
--- a/app/controllers/api/v1/location_machine_xrefs_controller.rb
+++ b/app/controllers/api/v1/location_machine_xrefs_controller.rb
@@ -38,12 +38,12 @@ def create
 
         return return_response(AUTH_REQUIRED_MSG, 'errors') if user.nil?
 
-        location_id = params[:location_id]
-        machine_id = params[:machine_id]
+        location_id = params[:location_id].to_i
+        machine_id = params[:machine_id].to_i
         condition = params[:condition]
         status_code = 200
 
-        return return_response('Failed to find machine', 'errors') if machine_id.nil? || location_id.nil? || !Machine.exists?(machine_id) || !Location.exists?(location_id)
+        return return_response('Failed to find machine', 'errors') if machine_id.zero? || location_id.zero? || !Machine.exists?(machine_id) || !Location.exists?(location_id)
 
         lmx = LocationMachineXref.find_by_location_id_and_machine_id(location_id, machine_id)
 
diff --git a/app/controllers/api/v1/location_picture_xrefs_controller.rb b/app/controllers/api/v1/location_picture_xrefs_controller.rb
index 867a831f3..a9579071e 100644
--- a/app/controllers/api/v1/location_picture_xrefs_controller.rb
+++ b/app/controllers/api/v1/location_picture_xrefs_controller.rb
@@ -21,8 +21,8 @@ def show
       def create
         return return_response(AUTH_REQUIRED_MSG, 'errors') if current_user.nil?
 
-        location_id = params[:location_id]
-        return return_response('Failed to find location', 'errors') if location_id.nil? || !Location.exists?(location_id)
+        location_id = params[:location_id].to_i
+        return return_response('Failed to find location', 'errors') if location_id.zero? || !Location.exists?(location_id)
 
         photo = params[:photo]
         return return_response('Missing photo to add', 'errors') if photo.nil?
diff --git a/app/models/location.rb b/app/models/location.rb
index e14302fd3..e3352c82d 100644
--- a/app/models/location.rb
+++ b/app/models/location.rb
@@ -3,8 +3,8 @@ class Location < ApplicationRecord
 
   validates_presence_of :name, :street, :city, :country
   validates :phone, phone: { possible: true, allow_blank: true, message: 'Phone format not valid.' }
-  validates :website, format: { with: %r{http(s?)://}, message: 'must begin with http:// or https://' }, if: :website?
-  validates :name, :street, :city, format: { with: /^\S.*/, message: "Can't start with a blank", multiline: true }
+  validates :website, format: { with: %r{\Ahttp(s?)://}, message: 'must begin with http:// or https://' }, if: :website?
+  validates :name, :street, :city, format: { with: /\A\S.*/, message: "Can't start with a blank", multiline: true }
   validates :lat, :lon, presence: { message: 'Latitude/Longitude failed to generate. Please double check address and try again, or manually enter the lat/lon' }
 
   belongs_to :location_type, optional: true
diff --git a/app/models/machine_score_xref.rb b/app/models/machine_score_xref.rb
index c55b44955..6f17986a0 100644
--- a/app/models/machine_score_xref.rb
+++ b/app/models/machine_score_xref.rb
@@ -8,8 +8,8 @@ class MachineScoreXref < ApplicationRecord
 
   scope :zone_id, lambda { |id|
     joins(:location_machine_xref).joins(:location).where("
-      locations.zone_id = #{id}
-    ")
+      locations.zone_id = ?
+    ", id)
   }
 
   scope :region, lambda { |name|
@@ -17,8 +17,8 @@ class MachineScoreXref < ApplicationRecord
     joins(:location_machine_xref).joins(:location).where("
       location_machine_xrefs.id = machine_score_xrefs.location_machine_xref_id
       and locations.id = location_machine_xrefs.location_id
-      and locations.region_id = #{r.id}
-    ")
+      and locations.region_id = ?
+    ", r.id)
   }
 
   def username
diff --git a/app/models/suggested_location.rb b/app/models/suggested_location.rb
index 98e4fced1..d62fb5850 100644
--- a/app/models/suggested_location.rb
+++ b/app/models/suggested_location.rb
@@ -5,8 +5,8 @@ class SuggestedLocation < ApplicationRecord
   validates_presence_of :name, :machines, on: :create
   validates_presence_of :street, :city, :zip, on: :update
 
-  validates :website, format: { with: %r{http(s?)://}, message: 'must begin with http:// or https://' }, if: :website?, on: :update
-  validates :name, :street, :city, format: { with: /^\S.*/, message: "Can't start with a blank", multiline: true }, on: :update
+  validates :website, format: { with: %r{\Ahttp(s?)://}, message: 'must begin with http:// or https://' }, if: :website?, on: :update
+  validates :name, :street, :city, format: { with: /\A\S.*/, message: "Can't start with a blank", multiline: true }, on: :update
   validates :lat, :lon, presence: { message: 'Latitude/Longitude failed to generate. Please double check address and try again, or manually enter the lat/lon' }, on: :update
 
   belongs_to :region, optional: true
@@ -97,19 +97,21 @@ def convert_to_location(user_email)
 
       delete
 
-      ActiveRecord::Base.connection.execute(<<HERE)
+      sql = <<HERE
 insert into versions values (
   nextval('versions_id_seq'),
   'Location',
-  #{location.id},
+  :location_id,
   'converted from suggested location',
-  '#{user_email}',
+  :user_email,
   NULL,
   now(),
   NULL,
   NULL
 )
 HERE
+      sanitized_sql = ActiveRecord::Base.sanitize_sql_array([sql, { location_id: location.id, user_email: user_email }])
+      ActiveRecord::Base.connection.execute(sanitized_sql)
     end
   end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 3cfa1537d..aa3b40da7 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -11,7 +11,7 @@ class User < ApplicationRecord
 
   validates :username, presence: true, uniqueness: { case_sensitive: false }
 
-  validates_format_of :username, with: /^[a-zA-Z0-9_\.]*$/, multiline: true
+  validates_format_of :username, with: /\A[a-zA-Z0-9_\.]*\z/, multiline: true
   validates :username, length: { maximum: 20 }
   strip_attributes only: %i[username password]
 
diff --git a/app/views/locations/_form.html.haml b/app/views/locations/_form.html.haml
index 59f2c18a7..a8b42f0b2 100644
--- a/app/views/locations/_form.html.haml
+++ b/app/views/locations/_form.html.haml
@@ -32,7 +32,7 @@
         :javascript
           $(function () {
             $('#by_#{key}_name').autocomplete({
-              source: '/#{key}s/autocomplete?region_level_search=1;region=#{params[:region]}',
+              source: '/#{key}s/autocomplete?region_level_search=1;region=#{h(params[:region])}',
               minLength: 2,
               delay: 500
               });
diff --git a/app/views/locations/_locations.html.haml b/app/views/locations/_locations.html.haml
index 2bc1bdfa6..d42cfc60d 100644
--- a/app/views/locations/_locations.html.haml
+++ b/app/views/locations/_locations.html.haml
@@ -15,7 +15,7 @@
       [#{[@lat, @lon].join(', ')}]
     );
 
-    var hrefOrig = '#{request.scheme}://#{request.host_with_port}/#{@region ? @region.name.downcase : @operators_map ? @operators_map : "map"}?';
+    var hrefOrig = '#{request.scheme}://#{request.host_with_port}/#{@region ? h(@region.name.downcase) : @operators_map ? @operators_map : "map"}?';
     var def_value = window.location.href;
 
     var url = '';
diff --git a/bin/bundle-audit b/bin/bundle-audit
new file mode 100755
index 000000000..a0e7ba0e8
--- /dev/null
+++ b/bin/bundle-audit
@@ -0,0 +1,27 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+#
+# This file was generated by Bundler.
+#
+# The application 'bundle-audit' is installed as part of a gem, and
+# this file is here to facilitate running it.
+#
+
+ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../Gemfile", __dir__)
+
+bundle_binstub = File.expand_path("bundle", __dir__)
+
+if File.file?(bundle_binstub)
+  if File.read(bundle_binstub, 300).include?("This file was generated by Bundler")
+    load(bundle_binstub)
+  else
+    abort("Your `bin/bundle` was not generated by Bundler, so this binstub cannot run.
+Replace `bin/bundle` by running `bundle binstubs bundler --force`, then run this command again.")
+  end
+end
+
+require "rubygems"
+require "bundler/setup"
+
+load Gem.bin_path("bundler-audit", "bundle-audit")
diff --git a/bin/bundler-audit b/bin/bundler-audit
new file mode 100755
index 000000000..334a73784
--- /dev/null
+++ b/bin/bundler-audit
@@ -0,0 +1,27 @@
+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+#
+# This file was generated by Bundler.
+#
+# The application 'bundler-audit' is installed as part of a gem, and
+# this file is here to facilitate running it.
+#
+
+ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../Gemfile", __dir__)
+
+bundle_binstub = File.expand_path("bundle", __dir__)
+
+if File.file?(bundle_binstub)
+  if File.read(bundle_binstub, 300).include?("This file was generated by Bundler")
+    load(bundle_binstub)
+  else
+    abort("Your `bin/bundle` was not generated by Bundler, so this binstub cannot run.
+Replace `bin/bundle` by running `bundle binstubs bundler --force`, then run this command again.")
+  end
+end
+
+require "rubygems"
+require "bundler/setup"
+
+load Gem.bin_path("bundler-audit", "bundler-audit")
diff --git a/config/brakeman.ignore b/config/brakeman.ignore
new file mode 100644
index 000000000..b5e503e7c
--- /dev/null
+++ b/config/brakeman.ignore
@@ -0,0 +1,94 @@
+{
+  "ignored_warnings": [
+    {
+      "warning_type": "Format Validation",
+      "warning_code": 30,
+      "fingerprint": "06f9be88a8af832b07095516dcafa96909e3f7770c63c803906d29d94ed9f5fc",
+      "check_name": "ValidationRegex",
+      "message": "Insufficient validation for `website` using `/\\Ahttp(s?):\\/\\//`. Use `\\A` and `\\z` as anchors",
+      "file": "app/models/location.rb",
+      "line": 6,
+      "link": "https://brakemanscanner.org/docs/warning_types/format_validation/",
+      "code": null,
+      "render_path": null,
+      "location": {
+        "type": "model",
+        "model": "Location"
+      },
+      "user_input": null,
+      "confidence": "High",
+      "cwe_id": [
+        777
+      ],
+      "note": ""
+    },
+    {
+      "warning_type": "Format Validation",
+      "warning_code": 30,
+      "fingerprint": "06f9be88a8af832b07095516dcafa96909e3f7770c63c803906d29d94ed9f5fc",
+      "check_name": "ValidationRegex",
+      "message": "Insufficient validation for `name` using `/\\A\\S.*/`. Use `\\A` and `\\z` as anchors",
+      "file": "app/models/location.rb",
+      "line": 7,
+      "link": "https://brakemanscanner.org/docs/warning_types/format_validation/",
+      "code": null,
+      "render_path": null,
+      "location": {
+        "type": "model",
+        "model": "Location"
+      },
+      "user_input": null,
+      "confidence": "High",
+      "cwe_id": [
+        777
+      ],
+      "note": ""
+    },
+    {
+      "warning_type": "Format Validation",
+      "warning_code": 30,
+      "fingerprint": "8b632b14ee9e862130a926be342087e1f1ec9d174f244c2d61fc2a4514afd2f7",
+      "check_name": "ValidationRegex",
+      "message": "Insufficient validation for `website` using `/\\Ahttp(s?):\\/\\//`. Use `\\A` and `\\z` as anchors",
+      "file": "app/models/suggested_location.rb",
+      "line": 8,
+      "link": "https://brakemanscanner.org/docs/warning_types/format_validation/",
+      "code": null,
+      "render_path": null,
+      "location": {
+        "type": "model",
+        "model": "SuggestedLocation"
+      },
+      "user_input": null,
+      "confidence": "High",
+      "cwe_id": [
+        777
+      ],
+      "note": ""
+    },
+    {
+      "warning_type": "Format Validation",
+      "warning_code": 30,
+      "fingerprint": "8b632b14ee9e862130a926be342087e1f1ec9d174f244c2d61fc2a4514afd2f7",
+      "check_name": "ValidationRegex",
+      "message": "Insufficient validation for `name` using `/\\A\\S.*/`. Use `\\A` and `\\z` as anchors",
+      "file": "app/models/suggested_location.rb",
+      "line": 9,
+      "link": "https://brakemanscanner.org/docs/warning_types/format_validation/",
+      "code": null,
+      "render_path": null,
+      "location": {
+        "type": "model",
+        "model": "SuggestedLocation"
+      },
+      "user_input": null,
+      "confidence": "High",
+      "cwe_id": [
+        777
+      ],
+      "note": ""
+    }
+  ],
+  "updated": "2024-09-29 19:42:12 -0700",
+  "brakeman_version": "6.2.1"
+}
diff --git a/config/initializers/cookies_serializer.rb b/config/initializers/cookies_serializer.rb
index f51a497e1..5a6a32d37 100644
--- a/config/initializers/cookies_serializer.rb
+++ b/config/initializers/cookies_serializer.rb
@@ -2,4 +2,4 @@
 
 # Specify a serializer for the signed and encrypted cookie jars.
 # Valid options are :json, :marshal, and :hybrid.
-Rails.application.config.action_dispatch.cookies_serializer = :hybrid
+Rails.application.config.action_dispatch.cookies_serializer = :json

From 5d171682efb7a27c3b82f81851c0a98053000624 Mon Sep 17 00:00:00 2001
From: fiveNinePlusR <fiveNinePlusR@gmail.com>
Date: Sun, 29 Sep 2024 20:08:06 -0700
Subject: [PATCH 2/2] make rubocop happy

---
 app/controllers/machines_controller.rb                  | 8 ++++----
 app/controllers/pages_controller.rb                     | 4 ++--
 spec/features/location_machine_xrefs_controller_spec.rb | 1 -
 spec/features/locations_controller_spec.rb              | 2 --
 spec/features/pages_controller_spec.rb                  | 2 +-
 spec/requests/api/v1/locations_controller_spec.rb       | 2 +-
 spec/spec_helper.rb                                     | 1 -
 7 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/app/controllers/machines_controller.rb b/app/controllers/machines_controller.rb
index ace761c55..f5fa9dc47 100644
--- a/app/controllers/machines_controller.rb
+++ b/app/controllers/machines_controller.rb
@@ -28,11 +28,11 @@ def autocomplete
       sanitized_sql = ActiveRecord::Base.sanitize_sql_array([sql, { term: params[:term] }])
 
       results = ActiveRecord::Base.connection.select_all(sanitized_sql)
-        .map do |m|
-          name_year = "#{m['name']} (#{m['manufacturer']}, #{m['year']})"
+                                  .map do |m|
+        name_year = "#{m['name']} (#{m['manufacturer']}, #{m['year']})"
 
-          { label: name_year, value: name_year, id: m['id'], group_id: m['machine_group_id'] }
-        end
+        { label: name_year, value: name_year, id: m['id'], group_id: m['machine_group_id'] }
+      end
 
     end
 
diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb
index cca1c3dae..59fa33ccb 100644
--- a/app/controllers/pages_controller.rb
+++ b/app/controllers/pages_controller.rb
@@ -52,7 +52,7 @@ def map
 
     params[:user_faved] = user.id if user && !params[:user_faved].blank?
 
-    if !params[:by_location_id].blank? && loc = Location.where(id: params[:by_location_id]).first
+    if !params[:by_location_id].blank? && (loc = Location.where(id: params[:by_location_id]).first)
       @title_params[:title] = loc.name
       location_type = loc.location_type.name + ' - ' unless loc.location_type.nil?
       machine_list = ' - ' + loc.machine_names_first_no_year.join(', ') unless loc.machine_names_first_no_year.empty?
@@ -74,7 +74,7 @@ def region
     @location_count = @locations.count
     @lmx_count = @region.machines_count
 
-    if !params[:by_location_id].blank? && loc = Location.where(id: params[:by_location_id]).first
+    if !params[:by_location_id].blank? && (loc = Location.where(id: params[:by_location_id]).first)
       @title_params[:title] = loc.name
       location_type = loc.location_type.name + ' - ' unless loc.location_type.nil?
       machine_list = ' - ' + loc.machine_names_first_no_year.join(', ') unless loc.machine_names_first_no_year.empty?
diff --git a/spec/features/location_machine_xrefs_controller_spec.rb b/spec/features/location_machine_xrefs_controller_spec.rb
index ca55153ac..f3637f68f 100644
--- a/spec/features/location_machine_xrefs_controller_spec.rb
+++ b/spec/features/location_machine_xrefs_controller_spec.rb
@@ -658,7 +658,6 @@
     end
 
     it 'allows case insensitive searches of a region' do
-
       chicago_region = FactoryBot.create(:region, name: 'chicago', full_name: 'Chicago')
       FactoryBot.create(:location, id: 23, region: chicago_region, name: 'Chicago Location')
 
diff --git a/spec/features/locations_controller_spec.rb b/spec/features/locations_controller_spec.rb
index 99ab2e266..e08199799 100644
--- a/spec/features/locations_controller_spec.rb
+++ b/spec/features/locations_controller_spec.rb
@@ -111,7 +111,6 @@
     before(:each) do
       @location = FactoryBot.create(:location, region_id: @region.id, name: 'Cleo')
       @machine = FactoryBot.create(:machine, name: 'Bawb')
-
     end
 
     it 'removes a machine from a location' do
@@ -531,7 +530,6 @@
       @user = FactoryBot.create(:user)
       login(@user)
 
-
       @location = FactoryBot.create(:location, region: @region, name: 'Cleo')
     end
 
diff --git a/spec/features/pages_controller_spec.rb b/spec/features/pages_controller_spec.rb
index 2737ca6db..62f1d2974 100644
--- a/spec/features/pages_controller_spec.rb
+++ b/spec/features/pages_controller_spec.rb
@@ -527,7 +527,7 @@
       visit "/#{@region.name}/high_rollers"
       expect(page).to have_title('High Scores')
 
-      visit "/map/?by_location_id=1234"
+      visit '/map/?by_location_id=1234'
       expect(page).to have_title('Pinball Map')
 
       visit "/map/?by_location_id=#{@location.id}"
diff --git a/spec/requests/api/v1/locations_controller_spec.rb b/spec/requests/api/v1/locations_controller_spec.rb
index 1da8fc3e4..08553bb99 100644
--- a/spec/requests/api/v1/locations_controller_spec.rb
+++ b/spec/requests/api/v1/locations_controller_spec.rb
@@ -831,7 +831,7 @@
     it 'respects no_details and shows fewer location fields' do
       lmx = FactoryBot.create(:location_machine_xref, location: @location, machine: FactoryBot.create(:machine, id: 7777, name: 'Cleo'))
       FactoryBot.create(:machine_condition, location_machine_xref_id: lmx.id, comment: 'foo bar')
-      FactoryBot.create(:machine_score_xref, location_machine_xref: lmx, score: 567890)
+      FactoryBot.create(:machine_score_xref, location_machine_xref: lmx, score: 567_890)
       get "/api/v1/locations/#{@location.id}.json", params: { no_details: 1 }
 
       expect(response.body).to include('Satchmo')
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 7eab97aab..0398cfa81 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -14,7 +14,6 @@
 include Sprockets::Rails::Helper
 include ActiveSupport::Testing::TimeHelpers
 
-
 SimpleCov.start
 Coveralls.wear!