Class: Scryer::Rules::AuthenticationBypassRule

Inherits:
Scryer::Rule
  • Object
show all
Defined in:
lib/scryer/rules/authentication_bypass_rule.rb

Overview

Flags a controller class that calls skip_before_action/ skip_action_callback naming a common authentication filter (authenticate_user!, authenticate!, ...) — same shape and reasoning as CsrfProtectionRule, just for auth filters instead of CSRF: skipping one is sometimes correct (a public endpoint, a webhook) but is also a common way to accidentally leave an action reachable without login, especially with a broad except:/no scoping at all.

Constant Summary collapse

SKIP_METHODS = 
%w[skip_before_action skip_action_callback skip_before_filter].freeze
AUTH_FILTER_NAMES = 
%w[
  authenticate_user! authenticate! authenticate_admin! authenticate_account!
  require_login require_authentication authorize_request
].freeze

Instance Attribute Summary

Attributes inherited from Scryer::Rule

#file, #sexp, #source

Instance Method Summary collapse

Methods inherited from Scryer::Rule

inherited, #initialize

Constructor Details

This class inherits a constructor from Scryer::Rule

Instance Method Details

#scan ⇒ Object 



25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
 
# File 'lib/scryer/rules/authentication_bypass_rule.rb', line 25

def scan
  findings = []

  Ast.each_node(sexp) do |node|
    next unless Ast.tagged?(node, :class)

    class_name = Ast.class_name(node[1])
    next unless class_name.to_s.end_with?("Controller")

    each_skip_call(node[3]).each do |skip_node, filter_name, args|
      line = Ast.line_of(skip_node)
      # A skip already scoped with `only: [...]` is the exact mitigation this
      # rule's own suggested_fix recommends (see below) — that's the common,
      # often entirely legitimate "public read-only actions on an otherwise
      # authenticated controller" pattern (an index/show page, a webhook
      # receiver), not evidence of a mistake. We still surface it (whether
      # each named action is *actually* meant to be public is app-specific
      # judgment this per-file rule can't verify), but the wording shouldn't
      # read as "this is wrong" the way the unscoped/`except:` case does.
      scoped = !Ast.keyword_arg(args, "only").nil?
      message =
        if scoped
          "`#{class_name}` skips the `#{filter_name}` authentication filter, scoped with " \
            "`only:` (`#{skip_call_method(skip_node)} :#{filter_name}`) — this is a common, " \
            "often legitimate pattern for public-facing read actions (an index/show page, a " \
            "webhook) on an otherwise authenticated controller. Worth a quick human check " \
            "that every named action is genuinely meant to be public, not a signal that " \
            "this is a bug on its own."
        else
          "`#{class_name}` skips the `#{filter_name}` authentication filter " \
            "(`#{skip_call_method(skip_node)} :#{filter_name}`) with no `only:` scoping — " \
            "every action this applies to is reachable without logging in unless something " \
            "else in this controller re-checks authentication."
        end
      findings << finding(
        line: line,
        message: message,
        suggested_fix: "If this is genuinely a public action (a webhook, a login/signup " \
                        "page), scope the skip tightly with `only: [:action_name]` rather " \
                        "than leaving it unscoped or using a broad `except:`. If it's not " \
                        "meant to be public, remove the skip."
      )
    end
  end

  findings
end