Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed detection of UnscopedFind if optional: true is defined in the model and is not self-reference association #1764

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

NerdyBoyCool
Copy link

ref #1139
ref #1153

The change in #1153 avoids checking UnscopedFind for model classes with optional: true settings, but I think UnscopedFind warning should be issued for the following cases.

The current code only checks if optional: true is defined in the model class, so the check for the Group model is skipped.

class Group < ApplicationRecord
  belongs_to :user, optional: true
end
class GroupsController < ApplicationController
  def show
    @group = Group.find(params[:id])
  end
end

This PR solves this problem by also checking self-reference associations.
We have also added implementation and testing for the issues mentioned in #1139 so that they are not affected.

@NerdyBoyCool NerdyBoyCool changed the title Fixed detection of UnscopedFind if optional: true is defined in the model and is not self-reference related Fixed detection of UnscopedFind if optional: true is defined in the model and is not self-reference association Mar 3, 2023
@NerdyBoyCool
Copy link
Author

@presidentbeef
Can you give me your opinion on this change?
I would appreciate any advice and opinions.

@presidentbeef
Copy link
Owner

I'm not sure I understand the intent here. If the relationship is optional, wouldn't you have legitimate cases where the application can't look up a model via the relationship..? In other words, is this change just as likely to increase false positives as it is to decrease false negatives?

@NerdyBoyCool
Copy link
Author

The first example I gave was a little confusing, so I will give an example that is similar to the problem we encountered.

class User < ApplicationRecord
end

class Organization < ApplicationRecord
  has_many :members, dependent: :destroy
  has_many :users, dependent: :destroy
end

class Member < ApplicationRecord
  belongs_to :user
  belongs_to :organization

  has_many :articles, dependent: :destroy
end

class Article < ApplicationRecord
  belongs_to :author, class_name: "User", foreign_key: "author_id", optional: true
  belongs_to :organization
end

# Bad
artciles = Article.find(params[:id])

# Good
organization = Organization.first
articles = organization.articles.find(params[:id])

I expected brakeman to detect that the Article was not being retrieved via an Organization instance, but it did not because the Article model already defines optional: true for the User.
As you say, this type of problem could increase the number of false positives, but I thought the problem would be greater if the code that calls find directly against Organization was skipped.

@@ -52,12 +52,16 @@ def process_result result
:cwe_id => [285]
end

def optional_belongs_to? exp
def self_referential_association? exp, name
return false unless exp.is_a? Array

exp.each do |e|
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what we actually want is to check if there is any belongs_to relationship that is not optional?

Is the self-referential piece actually that important?

return false unless exp.is_a? Array

exp.each do |e|
if hash? e and true? hash_access(e, :optional)
return true
hash_iterate e do |key, value|
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is class_name going to show up more than once as a key? Couldn't this also use hash_access(:class_name) instead of iterating?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants