Skip to content

Commit 246ad46

Browse files
hmachmac
committed
Ruby: Account for filter skip ordering
A `skip_*_filter :foo` call only has an effect if there was an earlier call that registered `:foo` as a filter.
1 parent a164e76 commit 246ad46

File tree

3 files changed

+61
-33
lines changed

3 files changed

+61
-33
lines changed

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

Lines changed: 50 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -198,33 +198,12 @@ module Filters {
198198

199199
FilterKind getKind() { result = kind }
200200

201-
private ModuleBase getEnclosingModule() { result = call.getExpr().getEnclosingModule() }
202-
203201
/**
204202
* Holds if this callback is registered before `other`. This does not
205203
* guarantee that the callback will be executed before `other`. For example,
206204
* `after_action` callbacks are executed in reverse order.
207205
*/
208-
predicate registeredBefore(Filter other) {
209-
// before_action :this, :other
210-
//
211-
// before_action :this
212-
// before_action :other
213-
this.getBasicBlock() = other.getBasicBlock() and this.getASuccessor+() = other
214-
or
215-
this.getEnclosingModule() = other.getEnclosingModule() and
216-
this.getBasicBlock().strictlyDominates(other.getBasicBlock()) and
217-
this != other
218-
or
219-
// This callback is in a superclass of `other`'s class.
220-
//
221-
// class A
222-
// before_action :this
223-
//
224-
// class B < A
225-
// before_action :other
226-
other.getEnclosingModule().getModule() = this.getEnclosingModule().getModule().getASubClass+()
227-
}
206+
predicate registeredBefore(Filter other) { callbackRegisteredBefore(call, _, this, other) }
228207

229208
/**
230209
* Holds if this callback runs before `other`.
@@ -277,7 +256,9 @@ module Filters {
277256
* with the same name is registered later one, overriding this one.
278257
*/
279258
predicate skipped(ActionControllerActionMethod action) {
280-
this = any(SkipFilter f | f.getKind() = this.getKind()).getSkippedFilter(action) or
259+
this =
260+
any(SkipFilter f | f.getKind() = this.getKind() and this.registeredBefore(f))
261+
.getSkippedFilter(action) or
281262
this.overridden()
282263
}
283264

@@ -339,6 +320,11 @@ module Filters {
339320

340321
ActionControllerActionMethod getAction() { result = call.getAction() }
341322

323+
predicate registeredBefore(StringlikeLiteralCfgNode other) {
324+
(other instanceof SkipFilter or other instanceof Filter) and
325+
callbackRegisteredBefore(call, _, this, other)
326+
}
327+
342328
Filter getSkippedFilter(ActionControllerActionMethod action) {
343329
not result instanceof SkipFilter and
344330
action = this.getAction() and
@@ -347,6 +333,47 @@ module Filters {
347333
}
348334
}
349335

336+
/**
337+
* Holds if `predCall` (resp. `succCall`) registers or skips the callback referred to by `pred` (`succ`) and `predCall` is evaluated called before `succCall`.
338+
* Typically this means that `pred` is registered before `succ`, or `pred` is skipped before `succ`, depending on the nature of the call.
339+
* `pred` and `succ` are expected to be arguments. In the examples below, `pred` is `:foo` and `succ` is `:bar`.
340+
* ```rb
341+
* before_action :foo, :bar
342+
* skip_before_action :foo, :bar
343+
* ```
344+
* This does not guarantee that the callback referred to by `pred` will be executed before
345+
* `succ`. For example, `after_action` callbacks are executed in reverse order.
346+
*/
347+
private predicate callbackRegisteredBefore(
348+
FilterCall predCall, FilterCall succCall, StringlikeLiteralCfgNode pred,
349+
StringlikeLiteralCfgNode succ
350+
) {
351+
pred = predCall.getFilterArgument() and
352+
succ = succCall.getFilterArgument() and
353+
(
354+
// before_action :this, :other
355+
//
356+
// before_action :this
357+
// before_action :other
358+
pred.getBasicBlock() = succ.getBasicBlock() and
359+
pred.getASuccessor+() = succ
360+
or
361+
predCall.getExpr().getEnclosingModule() = succCall.getExpr().getEnclosingModule() and
362+
pred.getBasicBlock().strictlyDominates(succ.getBasicBlock()) and
363+
pred != succ
364+
or
365+
// This callback is in a superclass of `other`'s class.
366+
//
367+
// class A
368+
// before_action :this
369+
//
370+
// class B < A
371+
// before_action :other
372+
succCall.getExpr().getEnclosingModule().getModule() =
373+
predCall.getExpr().getEnclosingModule().getModule().getASubClass+()
374+
)
375+
}
376+
350377
/**
351378
* Holds if `pred` is called before `succ` in the callback chain for action `action`.
352379
* `pred` and `succ` may be methods bound to callbacks or controller actions.
@@ -381,6 +408,5 @@ module Filters {
381408
* Holds if `pred` is called before `succ` in the callback chain for some action.
382409
* `pred` and `succ` may be methods bound to callbacks or controller actions.
383410
*/
384-
cached
385411
predicate next(Method pred, Method succ) { next(_, pred, succ) }
386412
}

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,12 +42,12 @@
4242
| 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 |
4343
| 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 |
4444
| 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 |
45-
| 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 |
46-
| 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 |
47-
| 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 |
48-
| 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 |
49-
| 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 |
50-
| 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 |
51-
| 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 |
52-
| 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 |
53-
| 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 |
45+
| controllers/posts_controller.rb:12:3:13:5 | index | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:12:3:13:5 | index |
46+
| controllers/posts_controller.rb:12:3:13:5 | index | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
47+
| controllers/posts_controller.rb:15:3:16:5 | show | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:15:3:16:5 | show |
48+
| controllers/posts_controller.rb:15:3:16:5 | show | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/posts_controller.rb:23:3:25:5 | set_post |
49+
| controllers/posts_controller.rb:15:3:16:5 | show | controllers/posts_controller.rb:23:3:25:5 | set_post | controllers/application_controller.rb:6:3:8:5 | set_user |
50+
| controllers/posts_controller.rb:18:3:19:5 | upvote | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/posts_controller.rb:18:3:19:5 | upvote |
51+
| controllers/posts_controller.rb:18:3:19:5 | upvote | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/posts_controller.rb:23:3:25:5 | set_post |
52+
| controllers/posts_controller.rb:18:3:19:5 | upvote | controllers/posts_controller.rb:18:3:19:5 | upvote | controllers/posts_controller.rb:27:3:29:5 | log_upvote |
53+
| controllers/posts_controller.rb:18:3:19:5 | upvote | controllers/posts_controller.rb:23:3:25:5 | set_post | controllers/application_controller.rb:6:3:8:5 | set_user |

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ class PostsController < ApplicationController
66
# these calls override the earlier ones
77
after_action :log_upvote, only: :upvote
88
before_action :set_user
9+
skip_before_action :set_user
10+
before_action :set_user
911

1012
def index
1113
end

0 commit comments

Comments
 (0)