SpeciesFileGroup/taxonworks

View on GitHub
app/controllers/tasks/projects/activity_controller.rb

Summary

Maintainability
A
3 hrs
Test Coverage

Possible SQL injection
Open

      data = @klass.where("#{@target}_by_id": u.id).where("#{@klass.table_name}.#{@target}_at BETWEEN ? AND ?", @start_date, @end_date)

Injection is #1 on the 2013 OWASP Top Ten web security risks. SQL injection is when a user is able to manipulate a value which is used unsafely inside a SQL query. This can lead to data leaks, data loss, elevation of privilege, and other unpleasant outcomes.

Brakeman focuses on ActiveRecord methods dealing with building SQL statements.

A basic (Rails 2.x) example looks like this:

User.first(:conditions => "username = '#{params[:username]}'")

Brakeman would produce a warning like this:

Possible SQL injection near line 30: User.first(:conditions => ("username = '#{params[:username]}'"))

The safe way to do this query is to use a parameterized query:

User.first(:conditions => ["username = ?", params[:username]])

Brakeman also understands the new Rails 3.x way of doing things (and local variables and concatenation):

username = params[:user][:name].downcase
password = params[:user][:password]

User.first.where("username = '" + username + "' AND password = '" + password + "'")

This results in this kind of warning:

Possible SQL injection near line 37:
User.first.where((((("username = '" + params[:user][:name].downcase) + "' AND password = '") + params[:user][:password]) + "'"))

See the Ruby Security Guide for more information and Rails-SQLi.org for many examples of SQL injection in Rails.

Unsafe reflection method safe_constantize called with parameter value
Open

    @klass = params.require(:klass)&.safe_constantize

Brakeman reports on several cases of remote code execution, in which a user is able to control and execute code in ways unintended by application authors.

The obvious form of this is the use of eval with user input.

However, Brakeman also reports on dangerous uses of send, constantize, and other methods which allow creation of arbitrary objects or calling of arbitrary methods.

User controlled method execution
Open

      d[:data] = data.send("group_by_#{@time_span}", "#{@target}_at".to_sym ).count

Using unfiltered user data to select a Class or Method to be dynamically sent is dangerous.

It is much safer to whitelist the desired target or method.

Unsafe use of method:

method = params[:method]
@result = User.send(method.to_sym)

Safe:

method = params[:method] == 1 ? :method_a : :method_b
@result = User.send(method, *args)

Unsafe use of target:

table = params[:table]
model = table.classify.constantize
@result = model.send(:method)

Safe:

target = params[:target] == 1 ? Account : User
@result = target.send(:method, *args)

Including user data in the arguments passed to an Object#send is safe, as long as the method can properly handle potentially bad data.

Safe:

args = params["args"] || []
@result = User.send(:method, *args)

Method has too many lines. [37/25]
Open

  def type_report
    @klass = params.require(:klass)&.safe_constantize
    @time_span = params.require(:time_span)
    @target = params.require(:target)
    # @projects = params[:project_id].blank? ? Project.all : Project.where(id: params[:project_id])

This cop checks if the length of a method exceeds some maximum value. Comment lines can optionally be ignored. The maximum allowed length is configurable.

Method type_report has a Cognitive Complexity of 15 (exceeds 5 allowed). Consider refactoring.
Open

  def type_report
    @klass = params.require(:klass)&.safe_constantize
    @time_span = params.require(:time_span)
    @target = params.require(:target)
    # @projects = params[:project_id].blank? ? Project.all : Project.where(id: params[:project_id])
Severity: Minor
Found in app/controllers/tasks/projects/activity_controller.rb - About 1 hr to fix

Cognitive Complexity

Cognitive Complexity is a measure of how difficult a unit of code is to intuitively understand. Unlike Cyclomatic Complexity, which determines how difficult your code will be to test, Cognitive Complexity tells you how difficult your code will be to read and comprehend.

A method's cognitive complexity is based on a few simple rules:

  • Code is not considered more complex when it uses shorthand that the language provides for collapsing multiple statements into one
  • Code is considered more complex for each "break in the linear flow of the code"
  • Code is considered more complex when "flow breaking structures are nested"

Further reading

Method type_report has 37 lines of code (exceeds 25 allowed). Consider refactoring.
Open

  def type_report
    @klass = params.require(:klass)&.safe_constantize
    @time_span = params.require(:time_span)
    @target = params.require(:target)
    # @projects = params[:project_id].blank? ? Project.all : Project.where(id: params[:project_id])
Severity: Minor
Found in app/controllers/tasks/projects/activity_controller.rb - About 1 hr to fix

    Use params[:user_id].present? instead of !params[:user_id].blank?.
    Open

        if !params[:user_id].blank?

    This cop checks for code that can be written with simpler conditionals using Object#present? defined by Active Support.

    Interaction with Style/UnlessElse: The configuration of NotBlank will not produce an offense in the context of unless else if Style/UnlessElse is inabled. This is to prevent interference between the auto-correction of the two cops.

    Example: NotNilAndNotEmpty: true (default)

    # Converts usages of `!nil? && !empty?` to `present?`
    
    # bad
    !foo.nil? && !foo.empty?
    
    # bad
    foo != nil && !foo.empty?
    
    # good
    foo.present?

    Example: NotBlank: true (default)

    # Converts usages of `!blank?` to `present?`
    
    # bad
    !foo.blank?
    
    # bad
    not foo.blank?
    
    # good
    foo.present?

    Example: UnlessBlank: true (default)

    # Converts usages of `unless blank?` to `if present?`
    
    # bad
    something unless foo.blank?
    
    # good
    something if foo.present?

    Prefer 1000.years.
    Open

        @start_date = params[:start_date].blank? ? 1000.year.ago.to_date : params[:start_date].to_date

    This cop checks for correct grammar when using ActiveSupport's core extensions to the numeric classes.

    Example:

    # bad
    3.day.ago
    1.months.ago
    
    # good
    3.days.ago
    1.month.ago

    Identical blocks of code found in 2 locations. Consider refactoring.
    Open

        if a = @data.first
          data_labels.each do |k|
            if a[:data][k].nil?
              a[:data][k] = 0
            end
    Severity: Minor
    Found in app/controllers/tasks/projects/activity_controller.rb and 1 other location - About 25 mins to fix
    app/controllers/administration_controller.rb on lines 59..65

    Duplicated Code

    Duplicated code can lead to software that is hard to understand and difficult to change. The Don't Repeat Yourself (DRY) principle states:

    Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

    When you violate DRY, bugs and maintenance problems are sure to follow. Duplicated code has a tendency to both continue to replicate and also to diverge (leaving bugs as two similar implementations differ in subtle ways).

    Tuning

    This issue has a mass of 30.

    We set useful threshold defaults for the languages we support but you may want to adjust these settings based on your project guidelines.

    The threshold configuration represents the minimum mass a code block must have to be analyzed for duplication. The lower the threshold, the more fine-grained the comparison.

    If the engine is too easily reporting duplication, try raising the threshold. If you suspect that the engine isn't catching enough duplication, try lowering the threshold. The best setting tends to differ from language to language.

    See codeclimate-duplication's documentation for more information about tuning the mass threshold in your .codeclimate.yml.

    Refactorings

    Further Reading

    There are no issues that match your filters.

    Category
    Status