Skip to content

[Rails5] Use `safe_params` instead of `params` in `url_for` helpers

blackst0ne requested to merge blackst0ne-rails5-use-safe-params-instead-of-params-in-url-for-helpers into master

What does this MR do?

This MR replaces params with safe_params in url_for helpers to resolve security issues and failing specs with the

  19) Groups::MilestonesController#ensure_canonical_path for a GET request when requesting a redirected path when the old group path is substring of groups does not modify the /groups part of the path
      Failure/Error: url_for(params)
      
      ArgumentError:
        Attempting to generate a URL from non-sanitized request parameters! An attacker can inject malicious data into the generated URL, such as changing the host. Whitelist and sanitize passed parameters to be secure.
      # ./app/controllers/groups/application_controller.rb:36:in `build_canonical_path'
      # ./app/controllers/concerns/routable_actions.rb:40:in `ensure_canonical_path'
      # ./app/controllers/concerns/routable_actions.rb:7:in `find_routable!'
      # ./app/controllers/groups/application_controller.rb:14:in `group'
      # ./lib/gitlab/i18n.rb:50:in `with_locale'
      # ./lib/gitlab/i18n.rb:56:in `with_user_locale'
      # ./app/controllers/application_controller.rb:328:in `set_locale'
      # ./spec/controllers/groups/milestones_controller_spec.rb:216:in `block (6 levels) in <top (required)>'

errors.

This MR is based on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18241

Are there points in the code the reviewer needs to double check?

No.

Why was this MR needed?

Migration to Rails 5.0

Screenshots (if relevant)

No.

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

#14286 (closed) and !12841 (closed)

Merge request reports