From 2199c0420e362ae760e3b6975088ce0d02ffb578 Mon Sep 17 00:00:00 2001 From: Ivan Greene Date: Thu, 21 Mar 2024 13:43:46 -0400 Subject: [PATCH] Boolean query param cop (#35) This is a mistake I've made at least twice now. Basically, query params like `?should_do_something=false` will result in the value of string 'false' when fetched from a params object. ```ruby # Sending '?should_do_something=false' results in string 'false' here! should_do_something = params.fetch(:should_do_something, false) ``` The only way that they can be falsey is if they are omitted from the request and default to false. This is also an issue where we are expecting exactly true and not just any truthy value. The solution is to cast them explicitly ```ruby # Always an actual boolean value should_do_something = ActiveModel::Type::Boolean.new.cast(params.fetch(:should_do_something, false)) ``` --- Gemfile.lock | 5 +- betterlint.gemspec | 2 +- lib/rubocop/cop/betterment.rb | 1 + lib/rubocop/cop/betterment/fetch_boolean.rb | 61 ++++++++++++++++ .../cop/betterment/fetch_boolean_spec.rb | 73 +++++++++++++++++++ 5 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 lib/rubocop/cop/betterment/fetch_boolean.rb create mode 100644 spec/rubocop/cop/betterment/fetch_boolean_spec.rb diff --git a/Gemfile.lock b/Gemfile.lock index fef9ec3..07bcded 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - betterlint (1.7.0) + betterlint (1.8.0) rubocop (> 1.0) rubocop-performance rubocop-rails @@ -63,6 +63,8 @@ GEM method_source (1.0.0) minitest (5.20.0) mutex_m (0.2.0) + nokogiri (1.16.2-arm64-darwin) + racc (~> 1.4) nokogiri (1.16.2-x86_64-darwin) racc (~> 1.4) nokogiri (1.16.2-x86_64-linux) @@ -167,6 +169,7 @@ GEM zeitwerk (2.6.12) PLATFORMS + arm64-darwin-22 x86_64-darwin-22 x86_64-linux diff --git a/betterlint.gemspec b/betterlint.gemspec index 26d07f6..8e4f90b 100644 --- a/betterlint.gemspec +++ b/betterlint.gemspec @@ -5,7 +5,7 @@ $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) Gem::Specification.new do |s| s.name = "betterlint" - s.version = "1.7.0" + s.version = "1.8.0" s.authors = ["Development"] s.email = ["development@betterment.com"] s.summary = "Betterment rubocop configuration" diff --git a/lib/rubocop/cop/betterment.rb b/lib/rubocop/cop/betterment.rb index f22e156..6d44622 100644 --- a/lib/rubocop/cop/betterment.rb +++ b/lib/rubocop/cop/betterment.rb @@ -19,3 +19,4 @@ require 'rubocop/cop/betterment/server_error_assertion' require 'rubocop/cop/betterment/hardcoded_id' require 'rubocop/cop/betterment/vague_serialize' +require 'rubocop/cop/betterment/fetch_boolean' diff --git a/lib/rubocop/cop/betterment/fetch_boolean.rb b/lib/rubocop/cop/betterment/fetch_boolean.rb new file mode 100644 index 0000000..3a933bc --- /dev/null +++ b/lib/rubocop/cop/betterment/fetch_boolean.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Betterment + class FetchBoolean < Base + MSG = <<~MSG + A boolean fetched from query params or ENV will never be false when + explicitly specified on the request or env var. Please use a model + with a boolean attribute, or cast the value. + MSG + + # @!method fetch_boolean?(node) + def_node_matcher :fetch_boolean?, <<-PATTERN + (send _ :fetch _ (boolean)) + PATTERN + + # @!method fetch_env_boolean?(node) + def_node_matcher :fetch_env_boolean?, <<-PATTERN + (send (const nil? :ENV) :fetch _ (boolean)) + PATTERN + + # @!method boolean_cast?(node) + def_node_search :boolean_cast?, <<-PATTERN + (send + (send + (const + (const + (const nil? :ActiveModel) :Type) :Boolean) :new) :cast + ...) + PATTERN + + # @!method action_controller?(node) + def_node_search :action_controller?, <<~PATTERN + { + (const {nil? cbase} :ApplicationController) + (const (const {nil? cbase} :ActionController) :Base) + } + PATTERN + + def on_send(node) + return unless fetch_env_boolean?(node) || + (fetch_boolean?(node) && inherit_action_controller_base?(node)) + + return if node.each_ancestor(:send).any? { |ancestor| boolean_cast?(ancestor) } + + add_offense(node) + end + + private + + def inherit_action_controller_base?(node) + class_node = node.each_ancestor(:class).first + return false unless class_node + + action_controller?(class_node) + end + end + end + end +end diff --git a/spec/rubocop/cop/betterment/fetch_boolean_spec.rb b/spec/rubocop/cop/betterment/fetch_boolean_spec.rb new file mode 100644 index 0000000..7011e1f --- /dev/null +++ b/spec/rubocop/cop/betterment/fetch_boolean_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe RuboCop::Cop::Betterment::FetchBoolean, :config do + context 'when using parameters' do + it 'registers an offense when defaulting to a boolean value' do + expect_offense(<<~RUBY) + class UserController < ApplicationController + def taxable + params.fetch(:taxable, false) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ A boolean fetched [...] + end + end + RUBY + end + + it 'registers an offense when defaulting to a boolean value in assignment from ENV' do + expect_offense(<<~RUBY) + class App + def do_thing + in_progress = ENV.fetch('IN_PROGRESS', true) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ A boolean fetched [...] + check_for(in_progress) + end + end + RUBY + end + + it 'registers an offense when defaulting to a boolean value in method call' do + expect_offense(<<~RUBY) + class UserController < ApplicationController + def create + do_thing( + in_progress: create_params.permit(:in_progress, :taxable).fetch(:in_progress, false), + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ A boolean fetched [...] + ) + end + end + RUBY + end + + it 'does not register an offense when wrapped with cast' do + expect_no_offenses(<<~RUBY) + class UserController < ApplicationController + def create + ActiveModel::Type::Boolean.new.cast(params.fetch(:taxable, false)) + in_progress = ActiveModel::Type::Boolean.new.cast user_params.fetch(:in_progress, true) + do_thing( + in_progress: ActiveModel::Type::Boolean.new.cast( + create_params.permit(:in_progress, :taxable).fetch(:in_progress, false) + ) + ) + end + end + RUBY + end + + it 'does not register an offense for params when not in a controller' do + expect_no_offenses(<<~RUBY) + class Application + def create + params.fetch(:taxable, false) + in_progress = other.fetch('thing', true) + do_thing( + in_progress: create_params.permit(:in_progress, :taxable).fetch(:in_progress, false), + ) + end + end + RUBY + end + end +end