4

I am getting a warning message when I scan my code with Brakeman's Tool. It states that there is an Unscoped call to the following query:

@applicant = Applicant.find(params[:id])

Here is the actual error message:

+------------+----------------------+---------+---------------+-----------------------------------------------------------------------------------------------------------------------------------------+
| Confidence | Class                | Method  | Warning Type  | Message                                                                                                                                 |
+------------+----------------------+---------+---------------+-----------------------------------------------------------------------------------------------------------------------------------------+
| Weak       | ApplicantsController | show    | Unscoped Find | Unscoped call to Applicant#find near line 25: Applicant.find(+params[:id]+)                                                             |                                                       |
+------------+----------------------+---------+---------------+-----------------------------------------------------------------------------------------------------------------------------------------+

But when I replace the above query with the following one then it's fine:

@applicant = Applicant.where("id = ?", params[:id]).first

I don't understand what's wrong with the first query.

halfer
  • 19,824
  • 17
  • 99
  • 186
Amrinder Singh
  • 5,300
  • 12
  • 46
  • 88

1 Answers1

11

Brakeman is just warning you that you're querying the entire Applicant table, and not scoping it under another model, like current_tenant.applicants.find.... From Brakeman's docs:

Unscoped find (and related methods) are a form of Direct Object Reference. Models which belong to another model should typically be accessed via a scoped query.

For example, if an Account belongs to a User, then this may be an unsafe unscoped find:

Account.find(params[:id])

Depending on the action, this could allow an attacker to access any account they wish.

Instead, it should be scoped to the currently logged-in user:

current_user = User.find(session[:user_id])
current_user.accounts.find(params[:id])

If this is your desired behavior, you can configure Brakeman to ignore this warning as a false positive. To do that, run brakeman with the -I flag (or --interactive-ignore). Follow the instructions on Ignoring False Positives to step through all the warnings, and add this particular one to your ignore file.

In a nutshell: 

$ brakeman -I
Input file: |config/brakeman.ignore| 
# press Enter to accept the default ignore file
No such file. Continue with empty config? 
# press Enter to create the file
> 
1. Inspect all warnings
2. Hide previously ignored warnings
3. Skip - use current ignore configuration
# press 2 to step through all warnings, skipping previously ignored 
# Brakeman will now step through each warning, prompting you to for each one. 
# Press i to add this warning to the ignore list. 
# When finished, Brakeman will ask you what to do. 
# Press 1 to save changes to the ignore file.

The next time you run Brakeman, this warning should not appear.

Ryenski
  • 9,582
  • 3
  • 43
  • 47
  • thanks so much for this info, is the way which i am trying to use in my second query is correct? – Amrinder Singh Nov 24 '16 at 05:59
  • I would use the first query (`@applicant = Applicant.find(params[:id])`) - doing it the second way (`where..first`) will not raise a 404 error if the record is not found. Doing it the first way will raise an ActiveRecord::RecordNotFound exception, which is what you want. – Ryenski Nov 24 '16 at 06:01
  • Here's a similar answer: http://stackoverflow.com/questions/32102172/ruby-on-rails-what-do-these-brakeman-warnings-mean – Ryenski Nov 24 '16 at 06:06
  • I was just reading the Brakeman docs, and found out that you can configure it to ignore this warning as a false positive so it doesn't keep bugging you: http://brakemanscanner.org/docs/ignoring_false_positives/ – Ryenski Nov 24 '16 at 06:15