Merge branch 'blackst0ne-rails5-add-safe-params-helper' into 'master'

[Rails5] Add `safe_params` helper

See merge request gitlab-org/gitlab-ce!18241

18 files changed, 32 insertions, 20 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 24651dd392c..0fdd4d2cb47 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -5,6 +5,7 @@ class ApplicationController < ActionController::Base
   include Gitlab::GonHelper
   include GitlabRoutingHelper
   include PageLayoutHelper
+  include SafeParamsHelper
   include SentryHelper
   include WorkhorseHelper
   include EnforcesTwoFactorAuthentication
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index e89eaf7edda..f9e8fe624e8 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -86,7 +86,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     out_of_range = todos.current_page > total_pages
 
     if out_of_range
-      redirect_to url_for(params.merge(page: total_pages, only_path: true))
+      redirect_to url_for(safe_params.merge(page: total_pages, only_path: true))
     end
 
     out_of_range
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 5ac4b8710e2..79fa5818359 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -189,6 +189,6 @@ class GroupsController < Groups::ApplicationController
 
     params[:id] = group.to_param
 
-    url_for(params)
+    url_for(safe_params)
   end
 end
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 37f14230196..a93b116c6fe 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -404,7 +404,7 @@ class ProjectsController < Projects::ApplicationController
     params[:namespace_id] = project.namespace.to_param
     params[:id] = project.to_param
 
-    url_for(params)
+    url_for(safe_params)
   end
 
   def project_export_enabled
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 956df4a0a16..31f47a7aa7c 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -146,6 +146,6 @@ class UsersController < ApplicationController
   end
 
   def build_canonical_path(user)
-    url_for(params.merge(username: user.to_param))
+    url_for(safe_params.merge(username: user.to_param))
   end
 end
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index 866b8773db6..fef29789832 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -259,7 +259,7 @@ module BlobHelper
     options = []
 
     if error == :collapsed
-      options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, expanded: true, format: nil)))
+      options << link_to('load it anyway', url_for(safe_params.merge(viewer: viewer.type, expanded: true, format: nil)))
     end
 
     # If the error is `:server_side_but_stored_externally`, the simple viewer will show the same error,
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index b5ca39711bc..1bb82fd8150 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -180,7 +180,7 @@ module DiffHelper
   private
 
   def diff_btn(title, name, selected)
-    params_copy = params.dup
+    params_copy = safe_params.dup
     params_copy[:view] = name
 
     # Always use HTML to handle case where JSON diff rendered this button
diff --git a/app/helpers/safe_params_helper.rb b/app/helpers/safe_params_helper.rb
new file mode 100644
index 00000000000..b568e8810cc
--- /dev/null
+++ b/app/helpers/safe_params_helper.rb
@@ -0,0 +1,11 @@
+module SafeParamsHelper
+  # Rails 5.0 requires to permit `params` if they're used in url helpers.
+  # Use this helper when generating links with `params.merge(...)`
+  def safe_params
+    if params.respond_to?(:permit!)
+      params.except(:host, :port, :protocol).permit!
+    else
+      params
+    end
+  end
+end
diff --git a/app/views/dashboard/issues.atom.builder b/app/views/dashboard/issues.atom.builder
index 70ec6bc6257..d7b6fb9a4a1 100644
--- a/app/views/dashboard/issues.atom.builder
+++ b/app/views/dashboard/issues.atom.builder
@@ -1,5 +1,5 @@
 xml.title   "#{current_user.name} issues"
-xml.link    href: url_for(params), rel: "self", type: "application/atom+xml"
+xml.link    href: url_for(safe_params), rel: "self", type: "application/atom+xml"
 xml.link    href: issues_dashboard_url, rel: "alternate", type: "text/html"
 xml.id      issues_dashboard_url
 xml.updated @issues.first.updated_at.xmlschema if @issues.reorder(nil).any?
diff --git a/app/views/dashboard/issues.html.haml b/app/views/dashboard/issues.html.haml
index bb472b4c900..4bf04dadf01 100644
--- a/app/views/dashboard/issues.html.haml
+++ b/app/views/dashboard/issues.html.haml
@@ -2,12 +2,12 @@
 - page_title _("Issues")
 - @breadcrumb_link = issues_dashboard_path(assignee_id: current_user.id)
 = content_for :meta_tags do
-  = auto_discovery_link_tag(:atom, params.merge(rss_url_options), title: "#{current_user.name} issues")
+  = auto_discovery_link_tag(:atom, safe_params.merge(rss_url_options).to_h, title: "#{current_user.name} issues")
 
 .top-area
   = render 'shared/issuable/nav', type: :issues, display_count: !@no_filters_set
   .nav-controls
-    = link_to params.merge(rss_url_options), class: 'btn has-tooltip', data: { container: 'body' }, title: 'Subscribe' do
+    = link_to safe_params.merge(rss_url_options), class: 'btn has-tooltip', data: { container: 'body' }, title: 'Subscribe' do
       = icon('rss')
     = render 'shared/new_project_item_select', path: 'issues/new', label: "New issue", with_feature_enabled: 'issues', type: :issues
 
diff --git a/app/views/groups/issues.atom.builder b/app/views/groups/issues.atom.builder
index a239ea8caf0..2a385b661e5 100644
--- a/app/views/groups/issues.atom.builder
+++ b/app/views/groups/issues.atom.builder
@@ -1,5 +1,5 @@
 xml.title   "#{@group.name} issues"
-xml.link    href: url_for(params), rel: "self", type: "application/atom+xml"
+xml.link    href: url_for(safe_params), rel: "self", type: "application/atom+xml"
 xml.link    href: issues_group_url, rel: "alternate", type: "text/html"
 xml.id      issues_group_url
 xml.updated @issues.first.updated_at.xmlschema if @issues.reorder(nil).any?
diff --git a/app/views/groups/issues.html.haml b/app/views/groups/issues.html.haml
index 36df03302e8..bbfbea4ac7a 100644
--- a/app/views/groups/issues.html.haml
+++ b/app/views/groups/issues.html.haml
@@ -1,6 +1,6 @@
 - page_title "Issues"
 = content_for :meta_tags do
-  = auto_discovery_link_tag(:atom, params.merge(rss_url_options), title: "#{@group.name} issues")
+  = auto_discovery_link_tag(:atom, safe_params.merge(rss_url_options).to_h, title: "#{@group.name} issues")
 
 - if group_issues_count(state: 'all').zero?
   = render 'shared/empty_states/issues', project_select_button: true
diff --git a/app/views/projects/blob/_viewer.html.haml b/app/views/projects/blob/_viewer.html.haml
index 9c760c81527..b9663bbba15 100644
--- a/app/views/projects/blob/_viewer.html.haml
+++ b/app/views/projects/blob/_viewer.html.haml
@@ -4,7 +4,7 @@
 - load_async = local_assigns.fetch(:load_async, viewer.load_async? && render_error.nil?)
 - external_embed = local_assigns.fetch(:external_embed, false)
 
-- viewer_url = local_assigns.fetch(:viewer_url) { url_for(params.merge(viewer: viewer.type, format: :json)) } if load_async
+- viewer_url = local_assigns.fetch(:viewer_url) { url_for(safe_params.merge(viewer: viewer.type, format: :json)) } if load_async
 .blob-viewer{ data: { type: viewer.type, rich_type: rich_type, url: viewer_url }, class: ('hidden' if hidden) }
   - if render_error
     = render 'projects/blob/render_error', viewer: viewer
diff --git a/app/views/projects/diffs/_collapsed.html.haml b/app/views/projects/diffs/_collapsed.html.haml
index 8772bd4705f..5762f4d86d7 100644
--- a/app/views/projects/diffs/_collapsed.html.haml
+++ b/app/views/projects/diffs/_collapsed.html.haml
@@ -1,5 +1,5 @@
 - diff_file = viewer.diff_file
-- url = url_for(params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path, file_identifier: diff_file.file_identifier))
+- url = url_for(safe_params.merge(action: :diff_for_path, old_path: diff_file.old_path, new_path: diff_file.new_path, file_identifier: diff_file.file_identifier))
 .nothing-here-block.diff-collapsed{ data: { diff_for_path: url } }
   This diff is collapsed.
   %a.click-to-expand Click to expand it.
diff --git a/app/views/projects/issues/_nav_btns.html.haml b/app/views/projects/issues/_nav_btns.html.haml
index dd1a836fa20..297b928f020 100644
--- a/app/views/projects/issues/_nav_btns.html.haml
+++ b/app/views/projects/issues/_nav_btns.html.haml
@@ -1,4 +1,4 @@
-= link_to params.merge(rss_url_options), class: 'btn btn-default append-right-10 has-tooltip', title: 'Subscribe' do
+= link_to safe_params.merge(rss_url_options), class: 'btn btn-default append-right-10 has-tooltip', title: 'Subscribe' do
   = icon('rss')
 - if @can_bulk_update
   = button_tag "Edit issues", class: "btn btn-default append-right-10 js-bulk-update-toggle"
diff --git a/app/views/projects/issues/index.atom.builder b/app/views/projects/issues/index.atom.builder
index 4029926f373..6330245954e 100644
--- a/app/views/projects/issues/index.atom.builder
+++ b/app/views/projects/issues/index.atom.builder
@@ -1,5 +1,5 @@
 xml.title   "#{@project.name} issues"
-xml.link    href: url_for(params), rel: "self", type: "application/atom+xml"
+xml.link    href: url_for(safe_params), rel: "self", type: "application/atom+xml"
 xml.link    href: project_issues_url(@project), rel: "alternate", type: "text/html"
 xml.id      project_issues_url(@project)
 xml.updated @issues.first.updated_at.xmlschema if @issues.reorder(nil).any?
diff --git a/app/views/projects/issues/index.html.haml b/app/views/projects/issues/index.html.haml
index c427a9eedc2..1e7737aeb97 100644
--- a/app/views/projects/issues/index.html.haml
+++ b/app/views/projects/issues/index.html.haml
@@ -5,7 +5,7 @@
 - new_issue_email = @project.new_issuable_address(current_user, 'issue')
 
 = content_for :meta_tags do
-  = auto_discovery_link_tag(:atom, params.merge(rss_url_options), title: "#{@project.name} issues")
+  = auto_discovery_link_tag(:atom, safe_params.merge(rss_url_options).to_h, title: "#{@project.name} issues")
 
 - if project_issues(@project).exists?
   %div{ class: (container_class) }
diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml
index 376ac377562..68780cedeb1 100644
--- a/app/views/projects/merge_requests/creations/_new_submit.html.haml
+++ b/app/views/projects/merge_requests/creations/_new_submit.html.haml
@@ -26,16 +26,16 @@
   - else
     %ul.merge-request-tabs.nav-links.no-top.no-bottom
       %li.commits-tab.active
-        = link_to url_for(params), data: {target: 'div#commits', action: 'new', toggle: 'tab'} do
+        = link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tab'} do
           Commits
           %span.badge= @commits.size
       - if @pipelines.any?
         %li.builds-tab
-          = link_to url_for(params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tab'} do
+          = link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tab'} do
             Pipelines
             %span.badge= @pipelines.size
       %li.diffs-tab
-        = link_to url_for(params.merge(action: 'diffs')), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do
+        = link_to url_for(safe_params.merge(action: 'diffs')), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do
           Changes
           %span.badge= @merge_request.diff_size
 
@@ -46,7 +46,7 @@
         -# This tab is always loaded via AJAX
       - if @pipelines.any?
         #pipelines.pipelines.tab-pane
-          = render 'projects/merge_requests/pipelines', endpoint: url_for(params.merge(action: 'pipelines', format: :json)), disable_initialization: true
+          = render 'projects/merge_requests/pipelines', endpoint: url_for(safe_params.merge(action: 'pipelines', format: :json)), disable_initialization: true
 
   .mr-loading-status
     = spinner