Add a RuboCop to enforce the use of StrongParameters

What does this MR do and why?

See Add CI-enforced Secure Code Guideline for use o... (#466113 - closed) for the "Why". This is a "paved road" from Product Security Engineering: the goal is to make it easier for GitLab contributors to "do the right thing" and prevent classes of vulnerabilities from occurring in the future. There are no known vulnerabilities in our code-base related to this (i.e. this is not a bugvulnerability MR).

This MR:

  • Adds a RuboCop in grace mode
  • The RuboCop detects use of params without #permit or #require(d)
  • Adds existing violations to the ignore list
  • Adds documentation to our Secure Code Guidelines about the vulnerability we're trying to prevent with this cop

Example

If we ignore the Exclude, we can see a sample of violations:

% cd app/controllers/groups
% rubocop --ignore-parent-exclusion --only Rails/StrongParams
Inspecting 41 files
.CC.CCC....C.CCCC......CC.CCC.C....CCC...

Offenses:

application_controller.rb:23:38: C: [Correctable] Rails/StrongParams: Pass ActionController::StrongParameters in arguments instead of unsanitized ActionController::Parameters.
    @group ||= find_routable!(Group, params[:group_id] || params[:id], request.fullpath)
                                     ^^^^^^
application_controller.rb:23:59: C: [Correctable] Rails/StrongParams: Pass ActionController::StrongParameters in arguments instead of unsanitized ActionController::Parameters.
    @group ||= find_routable!(Group, params[:group_id] || params[:id], request.fullpath)
                                                          ^^^^^^
...
application_controller.rb:63:5: C: Rails/StrongParams: Pass ActionController::StrongParameters in arguments instead of unsanitized ActionController::Parameters.
    params[:group_id] = group.to_param
    ^^^^^^
...
registry/repositories_controller.rb:22:99: C: Rails/StrongParams: Pass ActionController::StrongParameters in arguments instead of unsanitized ActionController::Parameters.
            @images = ContainerRepositoriesFinder.new(user: current_user, subject: group, params: params.slice(:name))
                                                                                                  ^^^^^^
...
settings/slacks_controller.rb:29:102: C: Rails/StrongParams: Pass ActionController::StrongParameters in arguments instead of unsanitized ActionController::Parameters.
        Integrations::SlackInstallation::GroupService.new(group, current_user: current_user, params: params)
                                                                                                     ^^^^^^
...
milestones_controller.rb:124:13: C: [Correctable] Rails/StrongParams: Pass ActionController::StrongParameters in arguments instead of unsanitized ActionController::Parameters.
    @sort = params[:sort] || 'due_date_asc'
            ^^^^^^
...
41 files inspected, 53 offenses detected, 42 offenses autocorrectable

We use params a lot. In my mind all of the above should be using params.permit[:thing], even though the current way of doing things doesn't currently lead to any vulnerabilities.

TODO

  • Get help detecting params when it is not the sole / final argument
  • Confirm this won't break JiHu
  • Post-merge:
    • Open an issue to find & replace pagination params (e.g. params[:page])
    • Open an issue to resolve auto-correctable params
    • Open an issue to coordinate the resolution of un-auto-correctable params

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

Resolves Add CI-enforced Secure Code Guideline for use o... (#466113 - closed)

Edited by Nick Malcolm

Merge request reports

Loading