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

feat: Expand UnscopedFind offenses in "graphql" namespaces #30

Merged
merged 5 commits into from
Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/rubocop/cop/betterment/unscoped_find.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,18 @@ def on_send(node)
static_method_name(node.method_name)
) && !@unauthenticated_models.include?(Utils::Parser.get_root_token(node))

add_offense(node) if find_param_arg(arg_nodes)
add_offense(node) if find_param_arg(arg_nodes) || graphql_namespace?(node)
end

private

def graphql_namespace?(node)
node
.ancestors
.select { |n| n.class_type? || n.module_type? }
.any? { |n| n.identifier.to_s.downcase.include?("graphql") }
Copy link
Contributor

@dkubb dkubb Dec 8, 2023

Choose a reason for hiding this comment

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

question: Just curious if we should match the string if it contains the "graphql" string anywhere even if it's a subset of some larger string?

What if instead we used:

n.identifier.to_s.match?(/\bGraphQL\b/)

If we want to match GraphQL and Graphql or similar, then we could add the /i regexp modifier to make it case insensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm interesting, so I had intentionally written it to be at least flexible enough to match GraphQL and Graphql, thinking that I could easily imagine someone making the choice to downcase the QL, and thinking that that decision didn't feel super relevant to me, as to whether this cop should still apply.

Down to hear any other opinions on the question! Either way, I think the regex is a nicer touch (particularly with the word boundary) so I can switch to the case insensitive version and then see what others think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I should note that I think the odds of another string containing the subset graphql, but it being something else, is pretty low. It's more as a matter of practice that I try to match strings in a way that would be correct in more instances.

I have seen production bugs caused by using String#include? where a stricter match wouldn't have.

end

def find_param_arg(arg_nodes)
return unless arg_nodes

Expand Down
37 changes: 35 additions & 2 deletions spec/rubocop/cop/betterment/unscoped_find_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,42 @@ def create
expect(cop.messages.uniq).to eq([offense_unscoped_find])
end

it 'does not register an offense when trust chaining even with user input' do
it 'registers an offense when in a GraphQL namespace with no params' do
inspect_source(<<~RUBY)
class Foo::GraphQL::Application
def create
user_id = 1
secret_id = 2
# find all Secret records matching a particular secret_id
Secret.find(secret_id)
Secret.find_by(user: user_id)
end
end

module Foo
module GraphQL
class Application
def create
user_id = 1
secret_id = 2
# find all Secret records matching a particular secret_id
Secret.find(secret_id)
Secret.find_by(user: user_id)
end
end
end
end
RUBY

expect(cop.offenses.size).to be(4)
expect(cop.offenses.map(&:line)).to eq([6, 7, 18, 19])
expect(cop.highlights.uniq).to eq(['Secret.find(secret_id)', 'Secret.find_by(user: user_id)'])
expect(cop.messages.uniq).to eq([offense_unscoped_find])
end

it 'does not register an offense when trust chaining even with user input in graphql namespace' do
expect_no_offenses(<<~RUBY)
class Application
class GraphQL::Application
def create
current_user.secrets.find(params[:secret_id])
current_user.secrets.active.find(params[:secret_id])
Expand Down
Loading