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
gracemode - The RuboCop detects use of
paramswithout#permitor#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 paramswhen 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)