Add framework for parameter validation using rails_param gem

diff --git a/Gemfile b/Gemfile
index 13daf82676bb95bb735ba4daee79cf5f1a7f6cc1..253b685a7bd8964a1122bc40578e13e9f7522bec 100644 (file)
--- a/Gemfile
+++ b/Gemfile
@@ -62,6 +62,7 @@ gem "oauth-plugin", ">= 0.5.1"
 gem "openstreetmap-deadlock_retry", ">= 1.3.1", :require => "deadlock_retry"
 gem "rack-cors"
 gem "rails-i18n", "~> 7.0.0"
 gem "openstreetmap-deadlock_retry", ">= 1.3.1", :require => "deadlock_retry"
 gem "rack-cors"
 gem "rails-i18n", "~> 7.0.0"

+gem "rails_param"

 gem "rinku", ">= 2.0.6", :require => "rails_rinku"
 gem "strong_migrations"
 gem "validates_email_format_of", ">= 1.5.1"
 gem "rinku", ">= 2.0.6", :require => "rails_rinku"
 gem "strong_migrations"
 gem "validates_email_format_of", ">= 1.5.1"

diff --git a/Gemfile.lock b/Gemfile.lock
index f3e39e829b05a57e1749b4075761c3ee43f0e535..5406ba2ff3939b7c687794e946b3368ab4cd6464 100644 (file)
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -459,6 +459,9 @@ GEM
     rails-i18n (7.0.9)
       i18n (>= 0.7, < 2)
       railties (>= 6.0.0, < 8)
     rails-i18n (7.0.9)
       i18n (>= 0.7, < 2)
       railties (>= 6.0.0, < 8)

+    rails_param (1.3.1)
+      actionpack (>= 3.2.0)
+      activesupport (>= 3.2.0)

     railties (7.1.3.2)
       actionpack (= 7.1.3.2)
       activesupport (= 7.1.3.2)
     railties (7.1.3.2)
       actionpack (= 7.1.3.2)
       activesupport (= 7.1.3.2)


@@ -665,6 +668,7 @@ DEPENDENCIES
   rails (~> 7.1.0)
   rails-controller-testing
   rails-i18n (~> 7.0.0)
   rails (~> 7.1.0)
   rails-controller-testing
   rails-i18n (~> 7.0.0)

+  rails_param

   rinku (>= 2.0.6)
   rotp
   rtlcss
   rinku (>= 2.0.6)
   rotp
   rtlcss

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 488e6a8189cbb41c5b389f34cb9c8b48c8777783..d80b286d60dda69842ccd0d0e902bc6e800ec425 100644 (file)
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -10,6 +10,8 @@ class ApplicationController < ActionController::Base
   rescue_from CanCan::AccessDenied, :with => :deny_access
   check_authorization
 
   rescue_from CanCan::AccessDenied, :with => :deny_access
   check_authorization
 

+  rescue_from RailsParam::InvalidParameterError, :with => :invalid_parameter
+

   before_action :fetch_body
   around_action :better_errors_allow_inline, :if => proc { Rails.env.development? }
 
   before_action :fetch_body
   around_action :better_errors_allow_inline, :if => proc { Rails.env.development? }
 


@@ -306,6 +308,17 @@ class ApplicationController < ActionController::Base
     end
   end
 
     end
   end
 

+  def invalid_parameter(_exception)
+    if request.get?
+      respond_to do |format|
+        format.html { redirect_to :controller => "/errors", :action => "bad_request" }
+        format.any { head :bad_request }
+      end
+    else
+      head :bad_request
+    end
+  end
+

   # extract authorisation credentials from headers, returns user = nil if none
   def auth_data
     if request.env.key? "X-HTTP_AUTHORIZATION" # where mod_rewrite might have put it
   # extract authorisation credentials from headers, returns user = nil if none
   def auth_data
     if request.env.key? "X-HTTP_AUTHORIZATION" # where mod_rewrite might have put it

diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb
index ee1fcca6f8892ea346c1cacd3161eebc9c56983c..605403348a07ecacc543ae4c9ecfdc2eabe759ab 100644 (file)
--- a/app/controllers/errors_controller.rb
+++ b/app/controllers/errors_controller.rb
@@ -5,6 +5,13 @@ class ErrorsController < ApplicationController
 
   before_action :set_locale
 
 
   before_action :set_locale
 

+  def bad_request
+    respond_to do |format|
+      format.html { render :status => :bad_request }
+      format.any { render :status => :bad_request, :plain => "" }
+    end
+  end
+

   def forbidden
     respond_to do |format|
       format.html { render :status => :forbidden }
   def forbidden
     respond_to do |format|
       format.html { render :status => :forbidden }

diff --git a/app/views/errors/bad_request.html.erb b/app/views/errors/bad_request.html.erb
new file mode 100644 (file)
index 0000000..036517b
--- /dev/null
+++ b/app/views/errors/bad_request.html.erb
@@ -0,0 +1,3 @@
+<h1><%= t ".title" %></h1>
+<p><%= t ".description" %></p>
+<%= render :partial => "contact" %>

diff --git a/config/brakeman.ignore b/config/brakeman.ignore
new file mode 100644 (file)
index 0000000..b8fed1d
--- /dev/null
+++ b/config/brakeman.ignore
@@ -0,0 +1,29 @@
+{
+  "ignored_warnings": [
+    {
+      "warning_type": "HTTP Verb Confusion",
+      "warning_code": 118,
+      "fingerprint": "9567bbac855c6ec5552572700ec809d7c1d77f59953e6725aeca54fee5091674",
+      "check_name": "VerbConfusion",
+      "message": "Potential HTTP verb confusion. `HEAD` is routed like `GET` but `request.get?` will return `false`",
+      "file": "app/controllers/application_controller.rb",
+      "line": 312,
+      "link": "https://brakemanscanner.org/docs/warning_types/http_verb_confusion/",
+      "code": "if request.get? then\n  respond_to do\n   format.html do\n   redirect_to(:controller => \"/errors\", :action => \"bad_request\")\n   end\n  format.any do\n   head(:bad_request)\n   end\n   end\nelse\n  head(:bad_request)\nend",
+      "render_path": null,
+      "location": {
+        "type": "method",
+        "class": "ApplicationController",
+        "method": "invalid_parameter"
+      },
+      "user_input": "request.get?",
+      "confidence": "Weak",
+      "cwe_id": [
+        352
+      ],
+      "note": ""
+    }
+  ],
+  "updated": "2024-04-11 10:07:03 +0100",
+  "brakeman_version": "6.1.2"
+}

diff --git a/config/locales/en.yml b/config/locales/en.yml
index f9117ca1cc89add7669440805fbb59138644c7b0..b035cccc3368d0b9dd0a4f4414679dd22e7bac3b 100644 (file)
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -634,6 +634,9 @@ en:
       contact_url_title: Various contact channels explained
       contact: contact
       contact_the_community_html: Feel free to %{contact_link} the OpenStreetMap community if you have found a broken link / bug. Make a note of the exact URL of your request.
       contact_url_title: Various contact channels explained
       contact: contact
       contact_the_community_html: Feel free to %{contact_link} the OpenStreetMap community if you have found a broken link / bug. Make a note of the exact URL of your request.

+    bad_request:
+      title: Bad request
+      description: The operation you requested on the OpenStreetMap server is not valid (HTTP 400)

     forbidden:
       title: Forbidden
       description: The operation you requested on the OpenStreetMap server is only available to administrators (HTTP 403)
     forbidden:
       title: Forbidden
       description: The operation you requested on the OpenStreetMap server is only available to administrators (HTTP 403)

diff --git a/config/routes.rb b/config/routes.rb
index d1e4c74ae5e06c0d00736051520e1031bdd96411..1a229b365dcdf5ea1d85596dd3f06b31b8de60a1 100644 (file)
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -347,6 +347,7 @@ OpenStreetMap::Application.routes.draw do
   resources :redactions
 
   # errors
   resources :redactions
 
   # errors

+  match "/400", :to => "errors#bad_request", :via => :all

   match "/403", :to => "errors#forbidden", :via => :all
   match "/404", :to => "errors#not_found", :via => :all
   match "/500", :to => "errors#internal_server_error", :via => :all
   match "/403", :to => "errors#forbidden", :via => :all
   match "/404", :to => "errors#not_found", :via => :all
   match "/500", :to => "errors#internal_server_error", :via => :all

diff --git a/test/controllers/errors_controller_test.rb b/test/controllers/errors_controller_test.rb
index 9f8ccee56d66acc34b147625b643af81698df178..253cbbdc0bf795463afd2061ee610c789b11b663 100644 (file)
--- a/test/controllers/errors_controller_test.rb
+++ b/test/controllers/errors_controller_test.rb
@@ -2,6 +2,10 @@ require "test_helper"
 
 class ErrorsControllerTest < ActionDispatch::IntegrationTest
   def test_routes
 
 class ErrorsControllerTest < ActionDispatch::IntegrationTest
   def test_routes

+    assert_routing(
+      { :path => "/400", :method => :get },
+      { :controller => "errors", :action => "bad_request" }
+    )

     assert_routing(
       { :path => "/403", :method => :get },
       { :controller => "errors", :action => "forbidden" }
     assert_routing(
       { :path => "/403", :method => :get },
       { :controller => "errors", :action => "forbidden" }


@@ -16,6 +20,11 @@ class ErrorsControllerTest < ActionDispatch::IntegrationTest
     )
   end
 
     )
   end
 

+  def test_bad_request
+    get "/400"
+    assert_response :bad_request
+  end
+

   def test_forbidden
     get "/403"
     assert_response :forbidden
   def test_forbidden
     get "/403"
     assert_response :forbidden