From: Tom Hughes <tom@compton.nu>
Date: Thu, 29 Jun 2017 19:52:57 +0000 (+0100)
Subject: Fix more parameter sanitisation issues and add tests
X-Git-Tag: live~3934
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/fe1e28b4f45f6aa4bb53dc5163c187c5206b66a7

Fix more parameter sanitisation issues and add tests
---

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 2be52f7d6..e89927c9d 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -64,7 +64,7 @@ Metrics/BlockNesting:
 # Offense count: 62
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 1783
+  Max: 1790
 
 # Offense count: 69
 Metrics/CyclomaticComplexity:
diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb
index f4667bef5..20894c4e8 100644
--- a/app/controllers/notes_controller.rb
+++ b/app/controllers/notes_controller.rb
@@ -279,6 +279,7 @@ class NotesController < ApplicationController
   def mine
     if params[:display_name]
       if @this_user = User.active.find_by(:display_name => params[:display_name])
+        @params = params.permit(:display_name)
         @title = t "note.mine.title", :user => @this_user.display_name
         @heading = t "note.mine.heading", :user => @this_user.display_name
         @description = t "note.mine.subheading", :user => render_to_string(:partial => "user", :object => @this_user)
diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb
index 467ca4c3c..ea5cdab10 100644
--- a/app/controllers/user_blocks_controller.rb
+++ b/app/controllers/user_blocks_controller.rb
@@ -12,6 +12,7 @@ class UserBlocksController < ApplicationController
   before_action :check_database_writable, :only => [:create, :update, :revoke]
 
   def index
+    @params = params.permit
     @user_blocks_pages, @user_blocks = paginate(:user_blocks,
                                                 :include => [:user, :creator, :revoker],
                                                 :order => "user_blocks.ends_at DESC",
@@ -88,6 +89,7 @@ class UserBlocksController < ApplicationController
   ##
   # shows a list of all the blocks on the given user
   def blocks_on
+    @params = params.permit(:display_name)
     @user_blocks_pages, @user_blocks = paginate(:user_blocks,
                                                 :include => [:user, :creator, :revoker],
                                                 :conditions => { :user_id => @this_user.id },
@@ -98,6 +100,7 @@ class UserBlocksController < ApplicationController
   ##
   # shows a list of all the blocks by the given user.
   def blocks_by
+    @params = params.permit(:display_name)
     @user_blocks_pages, @user_blocks = paginate(:user_blocks,
                                                 :include => [:user, :creator, :revoker],
                                                 :conditions => { :creator_id => @this_user.id },
diff --git a/app/views/notes/_notes_paging_nav.html.erb b/app/views/notes/_notes_paging_nav.html.erb
index 108cbb3d2..862eae17c 100644
--- a/app/views/notes/_notes_paging_nav.html.erb
+++ b/app/views/notes/_notes_paging_nav.html.erb
@@ -1,7 +1,7 @@
 <p>
 
 <% if @page > 1 %>
-<%= link_to t('changeset.changeset_paging_nav.previous'), params.merge({ :page => @page - 1 }) %>
+<%= link_to t('changeset.changeset_paging_nav.previous'), @params.merge({ :page => @page - 1 }) %>
 <% else %>
 <%= t('changeset.changeset_paging_nav.previous') %>
 <% end %>
@@ -11,7 +11,7 @@
 <% if @notes.size < @page_size %>
 <%= t('changeset.changeset_paging_nav.next') %>
 <% else %>
-<%= link_to t('changeset.changeset_paging_nav.next'), params.merge({ :page => @page + 1 }) %>
+<%= link_to t('changeset.changeset_paging_nav.next'), @params.merge({ :page => @page + 1 }) %>
 <% end %>
 
 </p>
diff --git a/app/views/user_blocks/_blocks.html.erb b/app/views/user_blocks/_blocks.html.erb
index d3908dab3..e9dfc7185 100644
--- a/app/views/user_blocks/_blocks.html.erb
+++ b/app/views/user_blocks/_blocks.html.erb
@@ -20,7 +20,7 @@
 
 <ul class='secondary-actions'>
   <% if @user_blocks_pages.current_page.number > 1 -%>
-    <li><%= link_to t('user_block.partial.previous'), params.merge({ :page => @user_blocks_pages.current_page.number - 1 }) %></li>
+    <li><%= link_to t('user_block.partial.previous'), @params.merge({ :page => @user_blocks_pages.current_page.number - 1 }) %></li>
   <% else -%>
     <li><%= t('user_block.partial.previous') %></li>
   <% end -%>
@@ -28,7 +28,7 @@
   <li><%= t('user_block.partial.showing_page', :page => @user_blocks_pages.current_page.number) %></li>
 
   <% if @user_blocks_pages.current_page.number < @user_blocks_pages.page_count -%>
-    <li><%= link_to t('user_block.partial.next'), params.merge({ :page => @user_blocks_pages.current_page.number + 1 }) %></li>
+    <li><%= link_to t('user_block.partial.next'), @params.merge({ :page => @user_blocks_pages.current_page.number + 1 }) %></li>
   <% else -%>
     <li><%= t('user_block.partial.next') %></li>
   <% end -%>
diff --git a/test/controllers/changeset_controller_test.rb b/test/controllers/changeset_controller_test.rb
index f1b029b39..654682ac0 100644
--- a/test/controllers/changeset_controller_test.rb
+++ b/test/controllers/changeset_controller_test.rb
@@ -2044,6 +2044,18 @@ EOF
     check_list_result(Changeset.where("id <= 4"))
   end
 
+  ##
+  # Check that a list with a next page link works
+  def test_list_more
+    create_list(:changeset, 50)
+
+    get :list, :params => { :format => "html" }
+    assert_response :success
+
+    get :list, :params => { :format => "html" }, :xhr => true
+    assert_response :success
+  end
+
   ##
   # This should display the last 20 non-empty changesets
   def test_feed
diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb
index 58fffb8d3..fcd93eb2a 100644
--- a/test/controllers/diary_entry_controller_test.rb
+++ b/test/controllers/diary_entry_controller_test.rb
@@ -570,6 +570,21 @@ class DiaryEntryControllerTest < ActionController::TestCase
     check_diary_list
   end
 
+  def test_list_paged
+    # Create several pages worth of diary entries
+    create_list(:diary_entry, 50)
+
+    # Try and get the list
+    get :list
+    assert_response :success
+    assert_select "div.diary_post", :count => 20
+
+    # Try and get the second page
+    get :list, :params => { :page => 2 }
+    assert_response :success
+    assert_select "div.diary_post", :count => 20
+  end
+
   def test_rss
     create(:language, :code => "de")
     create(:diary_entry, :language_code => "en")
diff --git a/test/controllers/notes_controller_test.rb b/test/controllers/notes_controller_test.rb
index c6d89f1ca..e19810851 100644
--- a/test/controllers/notes_controller_test.rb
+++ b/test/controllers/notes_controller_test.rb
@@ -999,4 +999,20 @@ class NotesControllerTest < ActionController::TestCase
     get :mine, :params => { :display_name => "non-existent" }
     assert_response :not_found
   end
+
+  def test_mine_paged
+    user = create(:user)
+
+    create_list(:note, 50) do |note|
+      create(:note_comment, :note => note, :author => user)
+    end
+
+    get :mine, :params => { :display_name => user.display_name }
+    assert_response :success
+    assert_select "table.note_list tr", :count => 11
+
+    get :mine, :params => { :display_name => user.display_name, :page => 2 }
+    assert_response :success
+    assert_select "table.note_list tr", :count => 11
+  end
 end
diff --git a/test/controllers/trace_controller_test.rb b/test/controllers/trace_controller_test.rb
index 1e7642e36..e53809840 100644
--- a/test/controllers/trace_controller_test.rb
+++ b/test/controllers/trace_controller_test.rb
@@ -257,6 +257,26 @@ class TraceControllerTest < ActionController::TestCase
     assert_template "user/no_such_user"
   end
 
+  # Check a multi-page list
+  def test_list_paged
+    # Create several pages worth of traces
+    create_list(:trace, 50)
+
+    # Try and get the list
+    get :list
+    assert_response :success
+    assert_select "table#trace_list tbody", :count => 1 do
+      assert_select "tr", :count => 20
+    end
+
+    # Try and get the second page
+    get :list, :params => { :page => 2 }
+    assert_response :success
+    assert_select "table#trace_list tbody", :count => 1 do
+      assert_select "tr", :count => 20
+    end
+  end
+
   # Check that the rss loads
   def test_rss
     user = create(:user)
diff --git a/test/controllers/user_blocks_controller_test.rb b/test/controllers/user_blocks_controller_test.rb
index 7be619c2a..e5b0bdb32 100644
--- a/test/controllers/user_blocks_controller_test.rb
+++ b/test/controllers/user_blocks_controller_test.rb
@@ -73,6 +73,24 @@ class UserBlocksControllerTest < ActionController::TestCase
     end
   end
 
+  ##
+  # test the index action with multiple pages
+  def test_index_paged
+    create_list(:user_block, 50)
+
+    get :index
+    assert_response :success
+    assert_select "table#block_list", :count => 1 do
+      assert_select "tr", :count => 21
+    end
+
+    get :index, :params => { :page => 2 }
+    assert_response :success
+    assert_select "table#block_list", :count => 1 do
+      assert_select "tr", :count => 21
+    end
+  end
+
   ##
   # test the show action
   def test_show
@@ -421,6 +439,25 @@ class UserBlocksControllerTest < ActionController::TestCase
     end
   end
 
+  ##
+  # test the blocks_on action with multiple pages
+  def test_blocks_on_paged
+    user = create(:user)
+    create_list(:user_block, 50, :user => user)
+
+    get :blocks_on, :params => { :display_name => user.display_name }
+    assert_response :success
+    assert_select "table#block_list", :count => 1 do
+      assert_select "tr", :count => 21
+    end
+
+    get :blocks_on, :params => { :display_name => user.display_name, :page => 2 }
+    assert_response :success
+    assert_select "table#block_list", :count => 1 do
+      assert_select "tr", :count => 21
+    end
+  end
+
   ##
   # test the blocks_by action
   def test_blocks_by
@@ -465,4 +502,23 @@ class UserBlocksControllerTest < ActionController::TestCase
     assert_select "table#block_list", false
     assert_select "p", "#{normal_user.display_name} has not made any blocks yet."
   end
+
+  ##
+  # test the blocks_by action with multiple pages
+  def test_blocks_by_paged
+    user = create(:moderator_user)
+    create_list(:user_block, 50, :creator => user)
+
+    get :blocks_by, :params => { :display_name => user.display_name }
+    assert_response :success
+    assert_select "table#block_list", :count => 1 do
+      assert_select "tr", :count => 21
+    end
+
+    get :blocks_by, :params => { :display_name => user.display_name, :page => 2 }
+    assert_response :success
+    assert_select "table#block_list", :count => 1 do
+      assert_select "tr", :count => 21
+    end
+  end
 end