Class: Scryer::Rules::IdorRule

Inherits:
Scryer::Rule show all
Defined in:
lib/scryer/rules/idor_rule.rb

Overview

Flags Model.find(params[...]) / Model.find_by(...params...) inside a controller where the receiver is a bare, unnamespaced constant (not scoped through e.g. current_user.things.find(...)) and no authorization call (authorize, authorize!, policy_scope, can?, cannot? — the common Pundit/CanCanCan method names) appears anywhere else in the same controller class.

This is the least precise rule in the gem, by nature of the problem: whether a given find is actually scoped to the current user is a question about the whole app's authorization model, not something visible from one file's AST. Expect real false positives — e.g. an admin-only controller already gated by a class-level before_action, or a genuinely global/unowned model (Country.find(params[:id])). Treat every finding as "worth a second look," not a confirmed bug — same spirit as CsrfProtectionRule's class-wide safeguard check, applied to a harder problem.

Constant Summary collapse

FINDER_METHODS = 
%w[find find_by find_by!].freeze
NON_MODEL_RECEIVERS =

Same reasoning as MassAssignmentRule::NON_MODEL_RECEIVERS: common stdlib/gem constants with their own .find-style methods that have nothing to do with an ActiveRecord model lookup.

%w[
  Struct OpenStruct Data Class Module BCrypt OpenSSL Net URI Digest
  JSON YAML Marshal String Array Hash Integer Float Symbol Comparable
  Enumerable File Dir
].freeze
AUTHORIZATION_METHODS =

Pundit's authorize/policy_scope/can?/cannot? plus two more well-established framework-provided safeguards, deliberately not an attempt at an exhaustive list of every app's custom guard method (e.g. a homegrown require_admin! before_action) — this rule's own documented limitation above already covers that case as expected noise, since there's no reliable way to know a custom method name actually performs record-level authorization rather than something unrelated:

- `load_and_authorize_resource` / `authorize_resource` — CanCanCan's
own controller macros; declaring either one authorizes every
action in the controller (the same effect as calling `authorize!`
in each action by hand), so a controller using it has the same
safeguard this rule already accepts for a manual `authorize!` call.
- `verify_authorized` / `verify_policy_scoped` — Pundit's own
safety-net `after_action` callbacks (`after_action
:verify_authorized`), which raise unless some `authorize`/
`policy_scope` call already happened during the action. A
controller using this callback is *more* rigorously guarded than
one with a bare `authorize` call, not less.
%w[
  authorize authorize! policy_scope can? cannot?
  load_and_authorize_resource authorize_resource
  verify_authorized verify_policy_scoped
].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 



64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
 
# File 'lib/scryer/rules/idor_rule.rb', line 64

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")

    body = node[3]
    next if each_call_names(body).any? { |name| AUTHORIZATION_METHODS.include?(name) }

    each_unscoped_find(body).each do |find_node, method_name|
      line = Ast.line_of(find_node)
      findings << finding(
        line: line,
        message: "`#{method_name}` looks up a record directly from `params`, and " \
                  "`#{class_name}` has no visible authorization check (no `authorize`, " \
                  "`policy_scope`, or `can?`/`cannot?` anywhere in it) — if this record " \
                  "belongs to a specific user/account, another user may be able to view or " \
                  "modify it just by changing the id in the request.",
        suggested_fix: "Scope the lookup to the current actor instead of the bare model, " \
                        "e.g. `current_user.things.#{method_name}(params[:id])`, or add an " \
                        "explicit authorization check (`authorize @thing` for Pundit, " \
                        "`authorize! :show, @thing` for CanCanCan) before using the record."
      )
    end
  end

  findings
end