Merge pull request #4680 from tomhughes/validate-page-numbers

Add parameter validation to pagination

Gemfile

@@ -63,6 +63,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"

Gemfile.lock

@@ -458,6 +458,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)
@@ -672,6 +675,7 @@ DEPENDENCIES
   rails (~> 7.1.0)
   rails-controller-testing
   rails-i18n (~> 7.0.0)
+  rails_param
   rinku (>= 2.0.6)
   rotp
   rtlcss

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? }
 
@@ -310,6 +312,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

app/controllers/changesets_controller.rb

@@ -18,6 +18,8 @@ class ChangesetsController < ApplicationController
   ##
   # list non-empty changesets in reverse chronological order
   def index
+    param! :max_id, Integer, :min => 1
+
     @params = params.permit(:display_name, :bbox, :friends, :nearby, :max_id, :list)
 
     if request.format == :atom && @params[:max_id]

app/controllers/concerns/pagination_methods.rb

@@ -6,6 +6,9 @@ module PaginationMethods
   ##
   # limit selected items to one page, get ids of first item before/after the page
   def get_page_items(items, includes: [], limit: 20)
+    param! :before, Integer, :min => 1
+    param! :after, Integer, :min => 1
+
     id_column = "#{items.table_name}.id"
     page_items = if params[:before]
                    items.where("#{id_column} < ?", params[:before]).order(:id => :desc)

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 }

app/controllers/notes_controller.rb

@@ -16,6 +16,8 @@ class NotesController < ApplicationController
   ##
   # Display a list of notes by a specified user
   def index
+    param! :page, Integer, :min => 1
+
     @params = params.permit(:display_name)
     @title = t ".title", :user => @user.display_name
     @page = (params[:page] || 1).to_i

app/views/errors/bad_request.html.erb

@@ -0,0 +1,3 @@
+<h1><%= t ".title" %></h1>
+<p><%= t ".description" %></p>
+<%= render :partial => "contact" %>

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"
+}

config/locales/en.yml

@@ -633,6 +633,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)

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

test/controllers/changesets_controller_test.rb

@@ -92,6 +92,15 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest
     check_index_result(changesets.last(20))
   end
 
+  ##
+  # This should report an error
+  def test_index_invalid_xhr
+    %w[-1 0 fred].each do |id|
+      get history_path(:format => "html", :list => "1", :max_id => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   ##
   # This should display the last 20 changesets closed in a specific area
   def test_index_bbox

test/controllers/diary_entries_controller_test.rb

@@ -590,6 +590,17 @@ class DiaryEntriesControllerTest < ActionDispatch::IntegrationTest
     assert_select "li.page-item.disabled span.page-link", :text => "Newer Entries", :count => 1
   end
 
+  def test_index_invalid_paged
+    # Try some invalid paged accesses
+    %w[-1 0 fred].each do |id|
+      get diary_entries_path(:before => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+
+      get diary_entries_path(:after => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   def test_rss
     create(:language, :code => "de")
     create(:diary_entry, :language_code => "en")
@@ -899,6 +910,18 @@ class DiaryEntriesControllerTest < ActionDispatch::IntegrationTest
     assert_response :not_found
   end
 
+  def test_comments_invalid_paged
+    user = create(:user)
+
+    %w[-1 0 fred].each do |id|
+      get diary_comments_path(:display_name => user.display_name, :before => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+
+      get diary_comments_path(:display_name => user.display_name, :after => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   def test_subscribe_page
     user = create(:user)
     other_user = create(:user)

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

test/controllers/notes_controller_test.rb

@@ -83,6 +83,15 @@ class NotesControllerTest < ActionDispatch::IntegrationTest
     assert_select "table.note_list tbody tr", :count => 10
   end
 
+  def test_index_invalid_paged
+    user = create(:user)
+
+    %w[-1 0 fred].each do |page|
+      get user_notes_path(user, :page => page)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   def test_empty_page
     user = create(:user)
     get user_notes_path(user)

test/controllers/traces_controller_test.rb

@@ -322,6 +322,17 @@ class TracesControllerTest < ActionDispatch::IntegrationTest
     assert_select "li.page-item a.page-link", :text => "Older Traces", :count => 2
   end
 
+  def test_index_invalid_paged
+    # Try some invalid paged accesses
+    %w[-1 0 fred].each do |id|
+      get traces_path(:before => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+
+      get traces_path(:after => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   # Check the RSS feed
   def test_rss
     user = create(:user)

test/controllers/user_blocks_controller_test.rb

@@ -115,6 +115,18 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest
     check_no_page_link "Older Blocks"
   end
 
+  ##
+  # test the index action with invalid pages
+  def test_index_invalid_paged
+    %w[-1 0 fred].each do |id|
+      get user_blocks_path(:before => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+
+      get user_blocks_path(:after => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   ##
   # test the show action
   def test_show
@@ -560,6 +572,20 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest
     check_no_page_link "Older Blocks"
   end
 
+  ##
+  # test the blocks_on action with invalid pages
+  def test_blocks_on_invalid_paged
+    user = create(:user)
+
+    %w[-1 0 fred].each do |id|
+      get user_blocks_on_path(user, :before => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+
+      get user_blocks_on_path(user, :after => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   ##
   # test the blocks_by action
   def test_blocks_by
@@ -628,6 +654,20 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest
     check_no_page_link "Older Blocks"
   end
 
+  ##
+  # test the blocks_by action with invalid pages
+  def test_blocks_by_invalid_paged
+    user = create(:moderator_user)
+
+    %w[-1 0 fred].each do |id|
+      get user_blocks_by_path(user, :before => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+
+      get user_blocks_by_path(user, :after => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   private
 
   def check_user_blocks_table(user_blocks)

test/controllers/users_controller_test.rb

@@ -558,6 +558,18 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     check_no_page_link "Older Users"
   end
 
+  def test_index_get_invalid_paginated
+    session_for(create(:administrator_user))
+
+    %w[-1 0 fred].each do |id|
+      get users_path(:before => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+
+      get users_path(:after => id)
+      assert_redirected_to :controller => :errors, :action => :bad_request
+    end
+  end
+
   private
 
   def check_no_page_link(name)