Dig can cause weird exceptions

[jankoegel]

The problem

• I have a form that doesn't use a model and whose single relevant value has the same name as the controller. The controller is called OneTimePasswordsController and the form input field is called one_time_password.
• the code of the invisible_captcha gem breaks in unexpected ways
• I think this PR removes the surprising behaviour

Where does it happen

• lines 92 and 102 here call Ruby's dig method (permalink for current master's code):
  

  invisible_captcha/lib/invisible_captcha/controller_ext.rb

  Lines 92 to 102 in 1b70b6e

  	if params[honeypot].present? || params.dig(scope, honeypot).present?
  	warn_spam("Honeypot param '#{honeypot}' was present.")
  	return true
  	else
  	# No honeypot spam detected, remove honeypot from params to avoid UnpermittedParameters exceptions
  	params.delete(honeypot) if params.key?(honeypot)
  	params[scope].try(:delete, honeypot) if params.key?(scope)
  	end
  	else
  	InvisibleCaptcha.honeypots.each do |default_honeypot|
  	if params[default_honeypot].present? || params.dig(scope, default_honeypot).present?

• JFYI one dig was added in this commit:
  954312b#diff-0e2f2afd505e226663c121b957ac3ea24ff5cc092ad4fa6a4129351c03407441R91

Why it happens

• dig throws a TypeError exception when params[scope] is set (e.g. to a string).

• simpler Ruby example
  this works:

  params = { actor: { name: "Nicolas Cage" } }
  
  # digging with keys that don't exist
  params.dig(:foo, :bar)
  => nil

  but this breaks with an exception:

  # digging with keys that partially exist
  params.dig(:actor, :name, :whatever)
  => TypeError: String does not have #dig method

• Full article on dig and its problems: http://anamaria.martinezgomez.name/2018/01/07/ruby-dig.html

• related discussion from the Ruby mailing list: https://bugs.ruby-lang.org/issues/11762

Suggested Fix

• don't use dig, it's not a 1:1 replacement for params[scope] && params[scope][honeypot]

[codecov]

Codecov Report

Merging #128 (c9e3c18) into master (1b70b6e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #128   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files          22       22           
  Lines         435      435           
=======================================
  Hits          428      428           
  Misses          7        7           

Files	Coverage Δ
lib/invisible_captcha/controller_ext.rb	96.77% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[markets]

Thanks 👍

[jankoegel]

Thanks for the super quick merge :)
Just discovered this note in the README. I think we can remove it now, can't we?

NOTE: If you define a parameter with the same name as your controller and do not define a custom scope, you will see a TypeError when processing your action. To fix this error, specify a scope or rename the parameter to a different name than your controller.

(in this section: https://github.com/markets/invisible_captcha/blob/master/README.md#controller-method-options)

[markets]

Ah right! Good catch 🧐

We can remove this note now (basically revert #124). Would you like to send a PR?

[jankoegel]

Sure:
-> #129

[jankoegel]

cc @maxveldink

[markets]

Thanks @jankoegel!

I'd like to release a new version soon, this is the current diff: v2.1.0...master

Maybe it's worth a patch release v2.1.1 now?

[jankoegel]

@markets i'm wondering whether the specs from here could be useful:
https://github.com/markets/invisible_captcha/pull/123/files?w=1#diff-db98e7444786adef8a0b22647a7f361c171b1ab2b5f885bd38618969a3f4ceab

Looking for a way to prevent the next person from re-adding dig again.

I have 6 failing specs locally, though. Using Ruby 3.2.2:

Rspec output

  1) InvisibleCaptcha::ControllerExt honeypot attribute with random honeypot with no scope passes with no spam
     Failure/Error: expect(response.body).to be_present
       expected `"".present?` to be truthy, got false
     # ./spec/controllers_spec.rb:151:in `block (5 levels) in <top (required)>'

  2) InvisibleCaptcha::ControllerExt honeypot attribute with random honeypot with no scope with parameter the same name as the controller passes with no spam
     Failure/Error: if params[default_honeypot].present? || (params[scope] && params[scope][default_honeypot].present?)

     TypeError:
       no implicit conversion of Symbol into Integer
     # ./lib/invisible_captcha/controller_ext.rb:102:in `[]'
     # ./lib/invisible_captcha/controller_ext.rb:102:in `block in honeypot_spam?'
     # ./lib/invisible_captcha/controller_ext.rb:101:in `each'
     # ./lib/invisible_captcha/controller_ext.rb:101:in `honeypot_spam?'
     # ./lib/invisible_captcha/controller_ext.rb:24:in `detect_spam'
     # ./lib/invisible_captcha/controller_ext.rb:13:in `block in invisible_captcha'
     # ./spec/controllers_spec.rb:162:in `block (6 levels) in <top (required)>'

  3) InvisibleCaptcha::ControllerExt honeypot attribute with random honeypot auto-scoped passes with no spam
     Failure/Error: expect(response.body).to be_present
       expected `"".present?` to be truthy, got false
     # ./spec/controllers_spec.rb:129:in `block (5 levels) in <top (required)>'

  4) InvisibleCaptcha::ControllerExt honeypot attribute with random honeypot auto-scoped with parameter the same name as the controller passes with no spam
     Failure/Error: if params[default_honeypot].present? || (params[scope] && params[scope][default_honeypot].present?)

     TypeError:
       no implicit conversion of Symbol into Integer
     # ./lib/invisible_captcha/controller_ext.rb:102:in `[]'
     # ./lib/invisible_captcha/controller_ext.rb:102:in `block in honeypot_spam?'
     # ./lib/invisible_captcha/controller_ext.rb:101:in `each'
     # ./lib/invisible_captcha/controller_ext.rb:101:in `honeypot_spam?'
     # ./lib/invisible_captcha/controller_ext.rb:24:in `detect_spam'
     # ./lib/invisible_captcha/controller_ext.rb:13:in `block in invisible_captcha'
     # ./spec/controllers_spec.rb:140:in `block (6 levels) in <top (required)>'

  5) InvisibleCaptcha::ControllerExt submission timestamp_threshold successful submissions allow to set a custom timestamp_threshold per action
     Failure/Error: expect(response.body).to be_present
       expected `"".present?` to be truthy, got false
     # ./spec/controllers_spec.rb:99:in `block (4 levels) in <top (required)>'

  6) InvisibleCaptcha::ControllerExt submission timestamp_threshold successful submissions passes if submission on or after timestamp_threshold
     Failure/Error: expect(response.body).to be_present
       expected `"".present?` to be truthy, got false
     # ./spec/controllers_spec.rb:87:in `block (4 levels) in <top (required)>'

[markets]

🤔 Weird... The CI already runs on Ruby 3.2.2 with success: https://github.com/markets/invisible_captcha/actions/runs/6629004356/job/18007346398#step:3:13