gitshowcase/gitshowcase

View on GitHub
app/controllers/admin/users_controller.rb

Summary

Maintainability
A
2 hrs
Test Coverage

Possible SQL injection
Open

        query = query.where("(#{where.join(' OR ')})", *values)

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.

Possible SQL injection
Open

      query = query.where("(#{fields.join(' OR ')})", *(["%#{@filters[:search]}%"] * fields.count))

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.

Assignment Branch Condition size for index is too high. [49.42/15]
Open

  def index
    @levels = levels = User::CompletenessService::LEVELS.keys
    @search_fields = SEARCH_FIELDS

    query = User.order('id DESC')

This cop checks that the ABC size of methods is not higher than the configured maximum. The ABC size is based on assignments, branches (method calls), and conditions. See http://c2.com/cgi/wiki?AbcMetric

Method has too many lines. [28/10]
Open

  def index
    @levels = levels = User::CompletenessService::LEVELS.keys
    @search_fields = SEARCH_FIELDS

    query = User.order('id DESC')

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.

Cyclomatic complexity for index is too high. [9/6]
Open

  def index
    @levels = levels = User::CompletenessService::LEVELS.keys
    @search_fields = SEARCH_FIELDS

    query = User.order('id DESC')

This cop checks that the cyclomatic complexity of methods is not higher than the configured maximum. The cyclomatic complexity is the number of linearly independent paths through a method. The algorithm counts decision points and adds one.

An if statement (or unless or ?:) increases the complexity by one. An else branch does not, since it doesn't add a decision point. The && operator (or keyword and) can be converted to a nested if statement, and ||/or is shorthand for a sequence of ifs, so they also add one. Loops can be said to have an exit condition, so they add one.

Perceived complexity for index is too high. [9/7]
Open

  def index
    @levels = levels = User::CompletenessService::LEVELS.keys
    @search_fields = SEARCH_FIELDS

    query = User.order('id DESC')

This cop tries to produce a complexity score that's a measure of the complexity the reader experiences when looking at a method. For that reason it considers when nodes as something that doesn't add as much complexity as an if or a &&. Except if it's one of those special case/when constructs where there's no expression after case. Then the cop treats it as an if/elsif/elsif... and lets all the when nodes count. In contrast to the CyclomaticComplexity cop, this cop considers else nodes as adding complexity.

Example:

def my_method                   # 1
  if cond                       # 1
    case var                    # 2 (0.8 + 4 * 0.2, rounded)
    when 1 then func_one
    when 2 then func_two
    when 3 then func_three
    when 4..10 then func_other
    end
  else                          # 1
    do_something until a && b   # 2
  end                           # ===
end                             # 7 complexity points

Method index has a Cognitive Complexity of 12 (exceeds 5 allowed). Consider refactoring.
Open

  def index
    @levels = levels = User::CompletenessService::LEVELS.keys
    @search_fields = SEARCH_FIELDS

    query = User.order('id DESC')
Severity: Minor
Found in app/controllers/admin/users_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 index has 28 lines of code (exceeds 25 allowed). Consider refactoring.
Open

  def index
    @levels = levels = User::CompletenessService::LEVELS.keys
    @search_fields = SEARCH_FIELDS

    query = User.order('id DESC')
Severity: Minor
Found in app/controllers/admin/users_controller.rb - About 1 hr to fix

    Line is too long. [92/80]
    Open

            completeness: params[:completeness] || Hash[@levels.map { |key| [key.to_s, true] }],

    Missing top-level class documentation comment.
    Open

    class Admin::UsersController < AdminController

    This cop checks for missing top-level documentation of classes and modules. Classes with no body are exempt from the check and so are namespace modules - modules that have nothing in their bodies except classes, other modules, or constant definitions.

    The documentation requirement is annulled if the class or module has a "#:nodoc:" comment next to it. Likewise, "#:nodoc: all" does the same for all its children.

    Example:

    # bad
    class Person
      # ...
    end
    
    # good
    # Description/Explanation of Person class
    class Person
      # ...
    end

    Use nested module/class definitions instead of compact style.
    Open

    class Admin::UsersController < AdminController

    This cop checks the style of children definitions at classes and modules. Basically there are two different styles:

    Example: EnforcedStyle: nested (default)

    # good
    # have each child on its own line
    class Foo
      class Bar
      end
    end

    Example: EnforcedStyle: compact

    # good
    # combine definitions as much as possible
    class Foo::Bar
    end

    The compact style is only forced for classes/modules with one child.

    Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
    Open

            search: params[:search],

    This cops checks the indentation of the first key in a hash literal where the opening brace and the first key are on separate lines. The other keys' indentations are handled by the AlignHash cop.

    By default, Hash literals that are arguments in a method call with parentheses, and where the opening curly brace of the hash is on the same line as the opening parenthesis of the method call, shall have their first key indented one step (two spaces) more than the position inside the opening parenthesis.

    Other hash literals shall have their first key indented one step more than the start of the line where the opening curly brace is.

    This default style is called 'specialinsideparentheses'. Alternative styles are 'consistent' and 'align_braces'. Here are examples:

    Example: EnforcedStyle: specialinsideparentheses (default)

    # The `special_inside_parentheses` style enforces that the first key
    # in a hash literal where the opening brace and the first key are on
    # separate lines is indented one step (two spaces) more than the
    # position inside the opening parentheses.
    
    # bad
    hash = {
      key: :value
    }
    and_in_a_method_call({
      no: :difference
                         })
    
    # good
    special_inside_parentheses
    hash = {
      key: :value
    }
    but_in_a_method_call({
                           its_like: :this
                         })

    Example: EnforcedStyle: consistent

    # The `consistent` style enforces that the first key in a hash
    # literal where the opening brace and the first key are on
    # seprate lines is indented the same as a hash literal which is not
    # defined inside a method call.
    
    # bad
    hash = {
      key: :value
    }
    but_in_a_method_call({
                           its_like: :this
                          })
    
    # good
    hash = {
      key: :value
    }
    and_in_a_method_call({
      no: :difference
    })

    Example: EnforcedStyle: align_braces

    # The `align_brackets` style enforces that the opening and closing
    # braces are indented to the same position.
    
    # bad
    and_now_for_something = {
                              completely: :different
    }
    
    # good
    and_now_for_something = {
                              completely: :different
                            }

    Line is too long. [99/80]
    Open

          query = query.where("(#{fields.join(' OR ')})", *(["%#{@filters[:search]}%"] * fields.count))

    Use next to skip iteration.
    Open

            if @filters[:completeness].key? level

    Use next to skip iteration instead of a condition at the end.

    Example: EnforcedStyle: skipmodifierifs (default)

    # bad
    [1, 2].each do |a|
      if a == 1
        puts a
      end
    end
    
    # good
    [1, 2].each do |a|
      next unless a == 1
      puts a
    end
    
    # good
    [1, 2].each do |o|
      puts o unless o == 1
    end

    Example: EnforcedStyle: always

    # With `always` all conditions at the end of an iteration needs to be
    # replaced by next - with `skip_modifier_ifs` the modifier if like
    # this one are ignored: `[1, 2].each { |a| return 'yes' if a == 1 }`
    
    # bad
    [1, 2].each do |o|
      puts o unless o == 1
    end
    
    # bad
    [1, 2].each do |a|
      if a == 1
        puts a
      end
    end
    
    # good
    [1, 2].each do |a|
      next unless a == 1
      puts a
    end

    Put empty method definitions on a single line.
    Open

      def show
      end

    This cop checks for the formatting of empty method definitions. By default it enforces empty method definitions to go on a single line (compact style), but it can be configured to enforce the end to go on its own line (expanded style).

    Note: A method definition is not considered empty if it contains comments.

    Example: EnforcedStyle: compact (default)

    # bad
    def foo(bar)
    end
    
    def self.foo(bar)
    end
    
    # good
    def foo(bar); end
    
    def foo(bar)
      # baz
    end
    
    def self.foo(bar); end

    Example: EnforcedStyle: expanded

    # bad
    def foo(bar); end
    
    def self.foo(bar); end
    
    # good
    def foo(bar)
    end
    
    def self.foo(bar)
    end

    Avoid using rescue in its modifier form.
    Open

        is_number = true if Float(param) rescue false

    This cop checks for uses of rescue in its modifier form.

    Example:

    # bad
    some_method rescue handle_error
    
    # good
    begin
      some_method
    rescue
      handle_error
    end

    Line is too long. [95/80]
    Open

            funnel: params[:funnel] || Hash[%w[no_domain 0 1 2 3 4 5 6+].map { |key| [key, true] }]

    Line is too long. [83/80]
    Open

            field: params[:field] || Hash[SEARCH_FIELDS.map { |field| [field, true] }],

    Line is too long. [102/80]
    Open

          fields = SEARCH_FIELDS.map { |field| "#{field} ILIKE ?" if @filters[:field].key? field }.compact

    Freeze mutable objects assigned to constants.
    Open

      SEARCH_FIELDS = %w[name username email location role website domain company company_website]

    This cop checks whether some constant value isn't a mutable literal (e.g. array or hash).

    Example:

    # bad
    CONST = [1, 2, 3]
    
    # good
    CONST = [1, 2, 3].freeze

    Pass &:to_sym as an argument to map instead of a block.
    Open

        if levels != @filters[:completeness].keys.map { |key| key.to_sym }

    Use symbols as procs when possible.

    Example:

    # bad
    something.map { |s| s.upcase }
    
    # good
    something.map(&:upcase)

    Line is too long. [94/80]
    Open

      SEARCH_FIELDS = %w[name username email location role website domain company company_website]

    Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
    Open

          if where.count > 0

    Checks for if and unless statements that would fit on one line if written as a modifier if/unless. The maximum line length is configured in the Metrics/LineLength cop.

    Example:

    # bad
    if condition
      do_stuff(bar)
    end
    
    unless qux.empty?
      Foo.do_something
    end
    
    # good
    do_stuff(bar) if condition
    Foo.do_something unless qux.empty?

    There are no issues that match your filters.

    Category
    Status