Ruby: Model actioncontroller filter overrides

If a filter is registered twice with the same name, the last
registration wins.

Author: hmac

ruby/ql/lib/codeql/ruby/frameworks/actioncontroller/Filters.qll

@@ -271,8 +271,34 @@ module Filters {
       not exists(Filter mid | this.runsBefore(mid, action) | mid.runsBefore(result, action))
     }
 
+    /**
+     * Holds if this callback does not run for `action`. This is either because
+     * it has been explicitly skipped by a `SkipFilter` or because a callback
+     * with the same name is registered later one, overriding this one.
+     */
     predicate skipped(ActionControllerActionMethod action) {
-      this = any(SkipFilter f | f.getKind() = this.getKind()).getSkippedFilter(action)
+      this = any(SkipFilter f | f.getKind() = this.getKind()).getSkippedFilter(action) or
+      this.overridden()
+    }
+
+    /**
+     * Holds if this callback is overridden by a callback with the same name. For example:
+     * ```rb
+     * class UsersController
+     *   before_action :foo # this filter is override by the subsequent `before_action :foo` call below.
+     *   before_action :bar
+     *   before_action :foo
+     * end
+     * ```
+     */
+    private predicate overridden() {
+      exists(Filter f |
+        f != this and
+        f.getFilterCallable() = this.getFilterCallable() and
+        f.getFilterName() = this.getFilterName() and
+        f.getKind() = this.getKind() and
+        this.registeredBefore(f)
+      )
     }
 
     private string getFilterName() { result = this.getConstantValue().getStringlikeValue() }

ruby/ql/test/library-tests/frameworks/action_controller/Filters.expected

@@ -1,48 +1,53 @@
-| controllers/comments_controller.rb:12:3:46:5 | index | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:90:3:91:5 | foo |
-| controllers/comments_controller.rb:12:3:46:5 | index | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
-| controllers/comments_controller.rb:12:3:46:5 | index | controllers/comments_controller.rb:12:3:46:5 | index | controllers/comments_controller.rb:86:3:88:5 | this_must_run_last |
-| controllers/comments_controller.rb:12:3:46:5 | index | controllers/comments_controller.rb:82:3:84:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
-| controllers/comments_controller.rb:12:3:46:5 | index | controllers/comments_controller.rb:90:3:91:5 | foo | controllers/comments_controller.rb:93:3:94:5 | bar |
-| controllers/comments_controller.rb:12:3:46:5 | index | controllers/comments_controller.rb:93:3:94:5 | bar | controllers/comments_controller.rb:12:3:46:5 | index |
-| controllers/comments_controller.rb:48:3:49:5 | create | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:69:3:72:5 | ensure_user_can_edit_comments |
-| controllers/comments_controller.rb:48:3:49:5 | create | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
-| controllers/comments_controller.rb:48:3:49:5 | create | controllers/comments_controller.rb:48:3:49:5 | create | controllers/comments_controller.rb:78:3:80:5 | log_comment_change |
-| controllers/comments_controller.rb:48:3:49:5 | create | controllers/comments_controller.rb:69:3:72:5 | ensure_user_can_edit_comments | controllers/comments_controller.rb:90:3:91:5 | foo |
-| controllers/comments_controller.rb:48:3:49:5 | create | controllers/comments_controller.rb:78:3:80:5 | log_comment_change | controllers/comments_controller.rb:86:3:88:5 | this_must_run_last |
-| controllers/comments_controller.rb:48:3:49:5 | create | controllers/comments_controller.rb:82:3:84:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
-| controllers/comments_controller.rb:48:3:49:5 | create | controllers/comments_controller.rb:90:3:91:5 | foo | controllers/comments_controller.rb:93:3:94:5 | bar |
-| controllers/comments_controller.rb:48:3:49:5 | create | controllers/comments_controller.rb:93:3:94:5 | bar | controllers/comments_controller.rb:48:3:49:5 | create |
-| controllers/comments_controller.rb:51:3:57:5 | show | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:74:3:76:5 | set_comment |
-| controllers/comments_controller.rb:51:3:57:5 | show | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
-| controllers/comments_controller.rb:51:3:57:5 | show | controllers/comments_controller.rb:51:3:57:5 | show | controllers/comments_controller.rb:86:3:88:5 | this_must_run_last |
-| controllers/comments_controller.rb:51:3:57:5 | show | controllers/comments_controller.rb:74:3:76:5 | set_comment | controllers/comments_controller.rb:90:3:91:5 | foo |
-| controllers/comments_controller.rb:51:3:57:5 | show | controllers/comments_controller.rb:82:3:84:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
-| controllers/comments_controller.rb:51:3:57:5 | show | controllers/comments_controller.rb:90:3:91:5 | foo | controllers/comments_controller.rb:93:3:94:5 | bar |
-| controllers/comments_controller.rb:51:3:57:5 | show | controllers/comments_controller.rb:93:3:94:5 | bar | controllers/comments_controller.rb:51:3:57:5 | show |
-| controllers/comments_controller.rb:59:3:61:5 | photo | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:90:3:91:5 | foo |
-| controllers/comments_controller.rb:59:3:61:5 | photo | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
-| controllers/comments_controller.rb:59:3:61:5 | photo | controllers/comments_controller.rb:59:3:61:5 | photo | controllers/comments_controller.rb:78:3:80:5 | log_comment_change |
-| controllers/comments_controller.rb:59:3:61:5 | photo | controllers/comments_controller.rb:78:3:80:5 | log_comment_change | controllers/comments_controller.rb:86:3:88:5 | this_must_run_last |
-| controllers/comments_controller.rb:59:3:61:5 | photo | controllers/comments_controller.rb:82:3:84:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
-| controllers/comments_controller.rb:59:3:61:5 | photo | controllers/comments_controller.rb:90:3:91:5 | foo | controllers/comments_controller.rb:93:3:94:5 | bar |
-| controllers/comments_controller.rb:59:3:61:5 | photo | controllers/comments_controller.rb:93:3:94:5 | bar | controllers/comments_controller.rb:59:3:61:5 | photo |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:69:3:72:5 | ensure_user_can_edit_comments |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/comments_controller.rb:78:3:80:5 | log_comment_change |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/comments_controller.rb:69:3:72:5 | ensure_user_can_edit_comments | controllers/comments_controller.rb:74:3:76:5 | set_comment |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/comments_controller.rb:74:3:76:5 | set_comment | controllers/comments_controller.rb:90:3:91:5 | foo |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/comments_controller.rb:78:3:80:5 | log_comment_change | controllers/comments_controller.rb:86:3:88:5 | this_must_run_last |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/comments_controller.rb:82:3:84:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/comments_controller.rb:90:3:91:5 | foo | controllers/comments_controller.rb:93:3:94:5 | bar |
-| controllers/comments_controller.rb:63:3:65:5 | destroy | controllers/comments_controller.rb:93:3:94:5 | bar | controllers/comments_controller.rb:63:3:65:5 | destroy |
+| controllers/comments_controller.rb:16:3:50:5 | index | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:98:3:99:5 | foo |
+| controllers/comments_controller.rb:16:3:50:5 | index | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
+| controllers/comments_controller.rb:16:3:50:5 | index | controllers/comments_controller.rb:16:3:50:5 | index | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags |
+| controllers/comments_controller.rb:16:3:50:5 | index | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags | controllers/comments_controller.rb:94:3:96:5 | this_must_run_last |
+| controllers/comments_controller.rb:16:3:50:5 | index | controllers/comments_controller.rb:90:3:92:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
+| controllers/comments_controller.rb:16:3:50:5 | index | controllers/comments_controller.rb:98:3:99:5 | foo | controllers/comments_controller.rb:101:3:102:5 | bar |
+| controllers/comments_controller.rb:16:3:50:5 | index | controllers/comments_controller.rb:101:3:102:5 | bar | controllers/comments_controller.rb:16:3:50:5 | index |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:73:3:76:5 | ensure_user_can_edit_comments |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/comments_controller.rb:52:3:53:5 | create | controllers/comments_controller.rb:82:3:84:5 | log_comment_change |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/comments_controller.rb:73:3:76:5 | ensure_user_can_edit_comments | controllers/comments_controller.rb:98:3:99:5 | foo |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/comments_controller.rb:82:3:84:5 | log_comment_change | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags | controllers/comments_controller.rb:94:3:96:5 | this_must_run_last |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/comments_controller.rb:90:3:92:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/comments_controller.rb:98:3:99:5 | foo | controllers/comments_controller.rb:101:3:102:5 | bar |
+| controllers/comments_controller.rb:52:3:53:5 | create | controllers/comments_controller.rb:101:3:102:5 | bar | controllers/comments_controller.rb:52:3:53:5 | create |
+| controllers/comments_controller.rb:55:3:61:5 | show | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:78:3:80:5 | set_comment |
+| controllers/comments_controller.rb:55:3:61:5 | show | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
+| controllers/comments_controller.rb:55:3:61:5 | show | controllers/comments_controller.rb:55:3:61:5 | show | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags |
+| controllers/comments_controller.rb:55:3:61:5 | show | controllers/comments_controller.rb:78:3:80:5 | set_comment | controllers/comments_controller.rb:98:3:99:5 | foo |
+| controllers/comments_controller.rb:55:3:61:5 | show | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags | controllers/comments_controller.rb:94:3:96:5 | this_must_run_last |
+| controllers/comments_controller.rb:55:3:61:5 | show | controllers/comments_controller.rb:90:3:92:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
+| controllers/comments_controller.rb:55:3:61:5 | show | controllers/comments_controller.rb:98:3:99:5 | foo | controllers/comments_controller.rb:101:3:102:5 | bar |
+| controllers/comments_controller.rb:55:3:61:5 | show | controllers/comments_controller.rb:101:3:102:5 | bar | controllers/comments_controller.rb:55:3:61:5 | show |
+| controllers/comments_controller.rb:63:3:65:5 | photo | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:98:3:99:5 | foo |
+| controllers/comments_controller.rb:63:3:65:5 | photo | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
+| controllers/comments_controller.rb:63:3:65:5 | photo | controllers/comments_controller.rb:63:3:65:5 | photo | controllers/comments_controller.rb:82:3:84:5 | log_comment_change |
+| controllers/comments_controller.rb:63:3:65:5 | photo | controllers/comments_controller.rb:82:3:84:5 | log_comment_change | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags |
+| controllers/comments_controller.rb:63:3:65:5 | photo | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags | controllers/comments_controller.rb:94:3:96:5 | this_must_run_last |
+| controllers/comments_controller.rb:63:3:65:5 | photo | controllers/comments_controller.rb:90:3:92:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
+| controllers/comments_controller.rb:63:3:65:5 | photo | controllers/comments_controller.rb:98:3:99:5 | foo | controllers/comments_controller.rb:101:3:102:5 | bar |
+| controllers/comments_controller.rb:63:3:65:5 | photo | controllers/comments_controller.rb:101:3:102:5 | bar | controllers/comments_controller.rb:63:3:65:5 | photo |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:73:3:76:5 | ensure_user_can_edit_comments |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:82:3:84:5 | log_comment_change |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:73:3:76:5 | ensure_user_can_edit_comments | controllers/comments_controller.rb:78:3:80:5 | set_comment |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:78:3:80:5 | set_comment | controllers/comments_controller.rb:98:3:99:5 | foo |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:82:3:84:5 | log_comment_change | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:86:3:88:5 | check_feature_flags | controllers/comments_controller.rb:94:3:96:5 | this_must_run_last |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:90:3:92:5 | this_must_run_first | controllers/application_controller.rb:10:3:12:5 | log_request |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:98:3:99:5 | foo | controllers/comments_controller.rb:101:3:102:5 | bar |
+| controllers/comments_controller.rb:67:3:69:5 | destroy | controllers/comments_controller.rb:101:3:102:5 | bar | controllers/comments_controller.rb:67:3:69:5 | destroy |
 | controllers/photos_controller.rb:3:3:6:5 | show | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/photos_controller.rb:3:3:6:5 | show |
 | controllers/photos_controller.rb:3:3:6:5 | show | controllers/photos_controller.rb:3:3:6:5 | show | controllers/photos_controller.rb:8:3:9:5 | foo |
-| controllers/posts_controller.rb:6:3:7:5 | index | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:6:3:7:5 | index |
-| controllers/posts_controller.rb:6:3:7:5 | index | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
-| controllers/posts_controller.rb:9:3:10:5 | show | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:17:3:19:5 | set_post |
-| controllers/posts_controller.rb:9:3:10:5 | show | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
-| controllers/posts_controller.rb:9:3:10:5 | show | controllers/posts_controller.rb:17:3:19:5 | set_post | controllers/posts_controller.rb:9:3:10:5 | show |
-| controllers/posts_controller.rb:12:3:13:5 | upvote | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:17:3:19:5 | set_post |
-| controllers/posts_controller.rb:12:3:13:5 | upvote | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
-| controllers/posts_controller.rb:12:3:13:5 | upvote | controllers/posts_controller.rb:12:3:13:5 | upvote | controllers/posts_controller.rb:21:3:23:5 | log_upvote |
-| controllers/posts_controller.rb:12:3:13:5 | upvote | controllers/posts_controller.rb:17:3:19:5 | set_post | controllers/posts_controller.rb:12:3:13:5 | upvote |
+| controllers/posts_controller.rb:10:3:11:5 | index | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:10:3:11:5 | index |
+| controllers/posts_controller.rb:10:3:11:5 | index | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
+| controllers/posts_controller.rb:13:3:14:5 | show | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:13:3:14:5 | show |
+| controllers/posts_controller.rb:13:3:14:5 | show | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/posts_controller.rb:21:3:23:5 | set_post |
+| controllers/posts_controller.rb:13:3:14:5 | show | controllers/posts_controller.rb:21:3:23:5 | set_post | controllers/application_controller.rb:6:3:8:5 | set_user |
+| controllers/posts_controller.rb:16:3:17:5 | upvote | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:16:3:17:5 | upvote |
+| controllers/posts_controller.rb:16:3:17:5 | upvote | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/posts_controller.rb:21:3:23:5 | set_post |
+| controllers/posts_controller.rb:16:3:17:5 | upvote | controllers/posts_controller.rb:16:3:17:5 | upvote | controllers/posts_controller.rb:25:3:27:5 | log_upvote |
+| controllers/posts_controller.rb:16:3:17:5 | upvote | controllers/posts_controller.rb:21:3:23:5 | set_post | controllers/application_controller.rb:6:3:8:5 | set_user |

ruby/ql/test/library-tests/frameworks/action_controller/controllers/comments_controller.rb

@@ -1,9 +1,13 @@
 class CommentsController < ApplicationController
+  after_action :check_feature_flags
+  after_action :log_comment_change
   prepend_after_action :this_must_run_last
   before_action :set_user
   before_action :ensure_user_can_edit_comments, only: WRITE_ACTIONS
   before_action :set_comment, only: [:show, :edit, :update, :destroy]
   before_action :foo, :bar
+
+  # this overrides the earlier callback on L2
   after_action :log_comment_change, except: [:index, :show, :new]
   prepend_before_action :this_must_run_first
 
@@ -78,6 +82,10 @@ def set_comment
   def log_comment_change
     AuditLog.create!(:comment_change, user: @user, comment: @comment)
   end
+  
+  def check_feature_flags
+    raise CommentsNotEnabled unless FeatureFlag.enabled?(:comments)
+  end
 
   def this_must_run_first
     # for whatever reason

ruby/ql/test/library-tests/frameworks/action_controller/controllers/posts_controller.rb

@@ -1,7 +1,11 @@
 class PostsController < ApplicationController
   before_action :set_user
   append_before_action :set_post, only: [:show, :upvote]
+  after_action :log_upvote
+
+  # these calls override the earlier ones
   after_action :log_upvote, only: :upvote
+  before_action :set_user
 
   def index
   end