From: Tom Hughes Date: Thu, 11 Apr 2024 07:45:12 +0000 (+0100) Subject: Add framework for parameter validation using rails_param gem X-Git-Tag: live~613^2~3 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/feff501b25ef5116fb9bc847dfb679919b44dce3 Add framework for parameter validation using rails_param gem --- diff --git a/Gemfile b/Gemfile index 13daf8267..253b685a7 100644 --- 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 "rails_param" 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 f3e39e829..5406ba2ff 100644 --- 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_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) @@ -665,6 +668,7 @@ DEPENDENCIES rails (~> 7.1.0) rails-controller-testing rails-i18n (~> 7.0.0) + rails_param rinku (>= 2.0.6) rotp rtlcss diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 488e6a818..d80b286d6 100644 --- 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 RailsParam::InvalidParameterError, :with => :invalid_parameter + 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 + 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 diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index ee1fcca6f..605403348 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -5,6 +5,13 @@ class ErrorsController < ApplicationController 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 } diff --git a/app/views/errors/bad_request.html.erb b/app/views/errors/bad_request.html.erb new file mode 100644 index 000000000..036517bdd --- /dev/null +++ b/app/views/errors/bad_request.html.erb @@ -0,0 +1,3 @@ +

<%= t ".title" %>

+

<%= t ".description" %>

+<%= render :partial => "contact" %> diff --git a/config/brakeman.ignore b/config/brakeman.ignore new file mode 100644 index 000000000..b8fed1d9b --- /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 f9117ca1c..b035cccc3 100644 --- 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. + 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) diff --git a/config/routes.rb b/config/routes.rb index d1e4c74ae..1a229b365 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -347,6 +347,7 @@ OpenStreetMap::Application.routes.draw do 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 diff --git a/test/controllers/errors_controller_test.rb b/test/controllers/errors_controller_test.rb index 9f8ccee56..253cbbdc0 100644 --- 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 + assert_routing( + { :path => "/400", :method => :get }, + { :controller => "errors", :action => "bad_request" } + ) assert_routing( { :path => "/403", :method => :get }, { :controller => "errors", :action => "forbidden" } @@ -16,6 +20,11 @@ class ErrorsControllerTest < ActionDispatch::IntegrationTest ) end + def test_bad_request + get "/400" + assert_response :bad_request + end + def test_forbidden get "/403" assert_response :forbidden