betterment / betterlint Goto Github PK

Here I upgraded from 1.0 to 1.1 and things broke with this error - we probably want to use fetch and default that one :)

An error occurred while Betterment/NonStandardActions cop was inspecting /Users/jamie/src/equals/test/controllers/application_controller_test.rb:123:4.
undefined method `+' for nil:NilClass

          @allowed_actions ||= cop_config['StandardActions'] + cop_config['AdditionalAllowedActions'] # rubocop:disable InternalAffairs/UndefinedConfig
                                                             ^
/Users/jamie/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/betterlint-1.1.0/lib/rubocop/cop/betterment/non_standard_actions.rb:65:in `allowed_actions'
/Users/jamie/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/betterlint-1.1.0/lib/rubocop/cop/betterment/non_standard_actions.rb:69:in `allowed_action?'
/Users/jamie/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/betterlint-1.1.0/lib/rubocop/cop/betterment/non_standard_actions.rb:58:in `check_raw_route'
/Users/jamie/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/betterlint-1.1.0/lib/rubocop/cop/betterment/non_standard_actions.rb:34:in `block in on_block'

We disabled the suggestion here #18, but it might be worth adding the extension instead.

https://github.com/rubocop/rubocop-capybara

Model.new({something: params.fetch(:something_id)}) isn't flagged by the authz cop in some contexts due to fetch not being tracked in the same way permit is. Adding fetch catches it but introduces false positives due to the way method return values are propagated.

Diff:

diff --git a/lib/rubocop/cop/betterment/utils/parser.rb b/lib/rubocop/cop/betterment/utils/parser.rb
index 1777d46..1d7b8a9 100644
--- a/lib/rubocop/cop/betterment/utils/parser.rb
+++ b/lib/rubocop/cop/betterment/utils/parser.rb
@@ -99,7 +99,7 @@ module RuboCop
 
           children.each do |child|
             ancestors = child.ancestors.select do |ancestor|
-              ancestor.send_type? && ancestor.method?(:permit)
+              ancestor.send_type? && (ancestor.method?(:permit) || ancestor.method?(:fetch))
             end
 
             ancestors.each do |ancestor|

Applying this ends up treating these two return values the same, but they are definitely not:

• current_user.objects.unwrap_or_raise!.find(params.permit(:object_id))
• params.permit(:object_id)

  def test
    some_user.other_model.active.find_by_token(token)
  end

  def token
    params[:token]
  end

This raises an offense, even though we're operating in a trusted context (off of some_user). Interestingly, using find_by(token:) and find(token) do not raise offenses, despite fundamentally being the same level of risk. The way we look for dynamic method names may be to blame.

        METHOD_PATTERN = /^find_by_(.+?)(!)?$/
...
        # yoinked from Rails/DynamicFindBy
        def static_method_name(method_name)
          match = METHOD_PATTERN.match(method_name)
          return nil unless match

          match[2] ? 'find_by!' : 'find_by'
        end

UnscopedFind has made the assumption that if you're doing a find off of an object (e.g. current_user), your query will be scoped to whatever belongs to that object. However, it turns out there's an unscoped method which when called at any point will remove any scopes, effectively letting you find any object regardless of who owns it.

For example, current_user.cats.find(params[:id]) will not raise an offense because the find call on cats is scoped to the current user. This same logic is erroneously extended to current_user.cats.unscoped.find(params[:id]), which does the same query but without the where clause that scopes it to the current user.

This cop should be modified to flag any uses of unscoped.

I was able to put together this diff that is the bare minimum needed to start detecting unscoped calls, but I haven't taken the time to sit down and list out test cases or write any specs.

diff --git a/lib/rubocop/cop/betterment/unscoped_find.rb b/lib/rubocop/cop/betterment/unscoped_find.rb
index fe3aa8e..bc6a71d 100644
--- a/lib/rubocop/cop/betterment/unscoped_find.rb
+++ b/lib/rubocop/cop/betterment/unscoped_find.rb
@@ -31,6 +31,10 @@ module RuboCop
           (send (const ... _) {#{FINDS.map(&:inspect).join(' ')}} ...)
         PATTERN
 
+        def_node_matcher :unscoped?, <<-PATTERN
+          (send (send ... :unscoped) {#{FINDS.map(&:inspect).join(' ')}} ...)
+        PATTERN
+
         def_node_search :find_graphql_namespace_nodes, <<~PATTERN, name: GRAPHQL_PATTERN
           (const _ %name)
         PATTERN
@@ -49,6 +53,7 @@ module RuboCop
           _, _, *arg_nodes = *node # rubocop:disable InternalAffairs/NodeDestructuring
           return unless
             (
+              unscoped?(node) ||
               find?(node) ||
               custom_scope_find?(node) ||
               static_method_name(node.method_name)

tl;dr todo

• flag all unscoped
• list out test cases
• write tests

To make site prism pages more discoverable, I propose we have a linter rule which enforces that the module namespaces for a site prism page matches the value passed to set_url.

The general rules would be something like

• Each static segment of the url that's not the final segment should have a module declaration, in the same order they appear in the url
• Dynamic segments of the url are ignored
• If the last segment of a url is dynamic, then the class name can be anything as long as it ends in Page
• If the last segment of the url is not dynamic, then it needs to be <CamelCasedSegment>Page

Here are some thoughts on what should pass and fail

Good

class UsersPage
  set_url "users"
end

module Users
  class ShowPage
    set_url "users/{id}"
  end
end

module Users
  class EditPage
    set_url "users/{id}/edit"
  end
end

module Users
  class NewPage
    set_url "users/{id}/new"
  end
end

module Users
  class PostsPage
    set_url "users/{id}/posts"
  end
end

Bad

class UsersPostsPage
  set_url "users/{id}/posts"
end

class UserPage
    set_url "users/{id}"
end