New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse error on FactoryBot attribute named "included" #1209

Closed
tobypinder opened this Issue May 15, 2018 · 1 comment

Comments

Projects
None yet
2 participants
@tobypinder

tobypinder commented May 15, 2018

Background

Brakeman version: brakeman 4.3.0
Rails version: Rails 5.1.4
Ruby version: ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]

Issue

brakeman appears not to parse FactoryBot factory definitions in the case that a factory field is named included and the attribute is dynamic.

Example minimal test case

  • CREATE gemfile with factorybot in it.
  • CREATE lib/foo_factory.rb with contents
FactoryBot.define do
  factory :foo do
    included { "an attribute value" }
  end
end

Backtrace

47: from [...]/bin/ruby_executable_hooks:15:in `<main>'
46: from [...]/bin/ruby_executable_hooks:15:in `eval'
45: from [...]/bin/brakeman:23:in `<main>'
44: from [...]/bin/brakeman:23:in `load'
43: from [...]/gems/brakeman-4.3.0/bin/brakeman:8:in `<top (required)>'
42: from [...]/gems/brakeman-4.3.0/lib/brakeman/commandline.rb:20:in `start'
41: from [...]/gems/brakeman-4.3.0/lib/brakeman/commandline.rb:35:in `run'
40: from [...]/gems/brakeman-4.3.0/lib/brakeman/commandline.rb:142:in `run_report'
39: from [...]/gems/brakeman-4.3.0/lib/brakeman/commandline.rb:118:in `regular_report'
38: from [...]/gems/brakeman-4.3.0/lib/brakeman/commandline.rb:133:in `run_brakeman'
37: from [...]/gems/brakeman-4.3.0/lib/brakeman.rb:77:in `run'
36: from [...]/gems/brakeman-4.3.0/lib/brakeman.rb:354:in `scan'
35: from [...]/gems/brakeman-4.3.0/lib/brakeman/scanner.rb:49:in `process'
34: from [...]/gems/brakeman-4.3.0/lib/brakeman/scanner.rb:197:in `process_libs'
33: from [...]/gems/brakeman-4.3.0/lib/brakeman/scanner.rb:299:in `track_progress'
32: from [...]/gems/brakeman-4.3.0/lib/brakeman/scanner.rb:299:in `each'
31: from [...]/gems/brakeman-4.3.0/lib/brakeman/scanner.rb:302:in `block in track_progress'
30: from [...]/gems/brakeman-4.3.0/lib/brakeman/scanner.rb:199:in `block in process_libs'
29: from [...]/gems/brakeman-4.3.0/lib/brakeman/scanner.rb:205:in `process_lib'
28: from [...]/gems/brakeman-4.3.0/lib/brakeman/processor.rb:99:in `process_lib'
27: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/library_processor.rb:21:in `process_library'
26: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:71:in `process'
25: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:112:in `in_context'
24: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:75:in `block in process'
23: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/library_processor.rb:64:in `process_iter'
22: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:39:in `process_default'
21: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:39:in `each_with_index'
20: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:39:in `each'
19: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:40:in `block in process_default'
18: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:71:in `process'
17: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:112:in `in_context'
16: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:75:in `block in process'
15: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/library_processor.rb:64:in `process_iter'
14: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:39:in `process_default'
13: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:39:in `each_with_index'
12: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:39:in `each'
11: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:40:in `block in process_default'
10: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:71:in `process'
 9: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:112:in `in_context'
 8: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:75:in `block in process'
 7: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:109:in `process_block'
 6: from (eval):3:in `map!'
 5: from (eval):3:in `map!'
 4: from [...]/gems/brakeman-4.3.0/lib/brakeman/processors/base_processor.rb:110:in `block in process_block'
 3: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:71:in `process'
 2: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:112:in `in_context'
 1: from [...]/gems/brakeman-4.3.0/lib/ruby_parser/bm_sexp_processor.rb:75:in `block in process'
[...]/gems/brakeman-4.3.0/lib/brakeman/processors/library_processor.rb:68:in `process_iter': undefined method `options' for nil:NilClass (NoMethodError)

I've been unable to reproduce this issue with

  • Differently named attributes
  • included "an attribute value" instead of included { "an attribute value" } (ie not lazy-evaluated)

As per the parse error guide I tried

ruby syntax check

$ ruby -c lib/factories/foo_factory.rb     
Syntax OK

ruby_parse

$ ruby_parse lib/factories/foo_factory.rb

# file = lib/factories/foo_factory.rb loc = 5
s(:iter,
 s(:call, s(:const, :FactoryBot), :define),
 0,
 s(:iter,
  s(:call, nil, :factory, s(:lit, :foo)),
  0,
  s(:iter, s(:call, nil, :included), 0, s(:str, "another_value"))))
done


 0.01s:   778.14 l/s:   12.46 Kb/s:    0 Kb:    5 loc:lib/factories/foo_factory.rb

 0.01s:   778.14 l/s:   12.46 Kb/s:    0 Kb:    5 loc:TOTAL

As ruby_parse and ruby -c suggests that this syntax is valid (and other static analysis tools like rubocop parse these factories effectively) this suggests to me that brakeman is handling included in a different way and being surprised by the block argument

I'm aware that Module#included exists and it's likely being treated as a keyword somewhere. We can look at changing this parameter name internally but I'm certain that the factory has no side effects in it's current form and that it should be parseable. However I appreciate that the FactoryBot DSL does a bunch of metaprogramming which may make identifying the root cause of this more difficult.

@tobypinder tobypinder changed the title from Brakeman parse error on FactoryBot attribute named "included" to Parse error on FactoryBot attribute named "included" May 15, 2018

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 16, 2018

Hi Toby,

Thank you for all the investigation!

Yes, this is because Brakeman is treating it like Module#included, but that is easy to fix.

Repository owner locked and limited conversation to collaborators Jul 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.