Skip to content

Commit 56919ee

Browse files
hmachmac
committed
delete/destroy_all -> delete/destroy_by
The ActiveRecord `delete_all` and `destroy_all` methods do not take a condition argument - they act on the scope of their receiver. The `delete_by` and `destroy_by` methods do take an argument which can be raw SQL, and are therefore vulnerable to SQL injection. For more info: https://api.rubyonrails.org/v6.1.4/classes/ActiveRecord/Relation.html#method-i-delete_all https://api.rubyonrails.org/v6.1.4/classes/ActiveRecord/Relation.html#method-i-delete_by
1 parent 3a1b294 commit 56919ee

File tree

6 files changed

+49
-49
lines changed

6 files changed

+49
-49
lines changed

‎ql/lib/codeql/ruby/frameworks/ActionController.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ private class ApplicationControllerAccess extends ConstantReadAccess {
2828
* class FooController < ActionController::Base
2929
* def delete_handler
3030
* uid = params[:id]
31-
* User.delete_all("id = ?", uid)
31+
* User.delete_by("id = ?", uid)
3232
* end
3333
* end
3434
* ```

‎ql/lib/codeql/ruby/frameworks/ActiveRecord.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ private Expr sqlFragmentArgument(MethodCall call) {
6868
(
6969
methodName =
7070
[
71-
"delete_all", "destroy_all", "exists?", "find_by", "find_by_sql", "from", "group",
72-
"having", "joins", "lock", "not", "order", "pluck", "where"
71+
"delete_by", "destroy_by", "exists?", "find_by", "find_by_sql", "from", "group", "having",
72+
"joins", "lock", "not", "order", "pluck", "where"
7373
] and
7474
result = call.getArgument(0)
7575
or

‎ql/test/library-tests/frameworks/ActiveRecord.expected

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@ activeRecordModelClasses
44
| ActiveRecordInjection.rb:19:1:25:3 | Admin |
55
activeRecordSqlExecutionRanges
66
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
7-
| ActiveRecordInjection.rb:23:17:23:25 | condition |
7+
| ActiveRecordInjection.rb:23:16:23:24 | condition |
88
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] |
9-
| ActiveRecordInjection.rb:39:21:39:43 | "id = '#{...}'" |
10-
| ActiveRecordInjection.rb:43:23:43:45 | "id = '#{...}'" |
9+
| ActiveRecordInjection.rb:39:20:39:42 | "id = '#{...}'" |
10+
| ActiveRecordInjection.rb:43:22:43:44 | "id = '#{...}'" |
1111
| ActiveRecordInjection.rb:47:16:47:21 | <<-SQL |
1212
| ActiveRecordInjection.rb:54:20:54:47 | "user.id = '#{...}'" |
13-
| ActiveRecordInjection.rb:68:21:68:33 | ... + ... |
13+
| ActiveRecordInjection.rb:68:20:68:32 | ... + ... |
1414
| ActiveRecordInjection.rb:75:16:75:28 | "name #{...}" |
1515
| ActiveRecordInjection.rb:80:20:80:39 | "username = #{...}" |
1616
activeRecordModelClassMethodCalls
@@ -19,28 +19,28 @@ activeRecordModelClassMethodCalls
1919
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
2020
| ActiveRecordInjection.rb:15:5:15:40 | call to find_by |
2121
| ActiveRecordInjection.rb:15:5:15:46 | call to users |
22-
| ActiveRecordInjection.rb:23:5:23:26 | call to destroy_all |
22+
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
2323
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
24-
| ActiveRecordInjection.rb:39:5:39:44 | call to delete_all |
25-
| ActiveRecordInjection.rb:43:5:43:47 | call to destroy_all |
24+
| ActiveRecordInjection.rb:39:5:39:43 | call to delete_by |
25+
| ActiveRecordInjection.rb:43:5:43:46 | call to destroy_by |
2626
| ActiveRecordInjection.rb:47:5:47:35 | call to where |
2727
| ActiveRecordInjection.rb:54:5:54:14 | call to where |
2828
| ActiveRecordInjection.rb:54:5:54:48 | call to not |
2929
| ActiveRecordInjection.rb:56:5:56:51 | call to authenticate |
30-
| ActiveRecordInjection.rb:68:5:68:34 | call to delete_all |
30+
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
3131
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
3232
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |
3333
| ActiveRecordInjection.rb:85:5:85:33 | call to find_by |
3434
| ActiveRecordInjection.rb:88:5:88:34 | call to find |
35-
| ActiveRecordInjection.rb:94:5:94:46 | call to delete_all |
35+
| ActiveRecordInjection.rb:94:5:94:45 | call to delete_by |
3636
potentiallyUnsafeSqlExecutingMethodCall
3737
| ActiveRecordInjection.rb:10:5:10:68 | call to find |
38-
| ActiveRecordInjection.rb:23:5:23:26 | call to destroy_all |
38+
| ActiveRecordInjection.rb:23:5:23:25 | call to destroy_by |
3939
| ActiveRecordInjection.rb:35:5:35:45 | call to calculate |
40-
| ActiveRecordInjection.rb:39:5:39:44 | call to delete_all |
41-
| ActiveRecordInjection.rb:43:5:43:47 | call to destroy_all |
40+
| ActiveRecordInjection.rb:39:5:39:43 | call to delete_by |
41+
| ActiveRecordInjection.rb:43:5:43:46 | call to destroy_by |
4242
| ActiveRecordInjection.rb:47:5:47:35 | call to where |
4343
| ActiveRecordInjection.rb:54:5:54:48 | call to not |
44-
| ActiveRecordInjection.rb:68:5:68:34 | call to delete_all |
44+
| ActiveRecordInjection.rb:68:5:68:33 | call to delete_by |
4545
| ActiveRecordInjection.rb:75:5:75:29 | call to order |
4646
| ActiveRecordInjection.rb:80:7:80:40 | call to find_by |

‎ql/test/library-tests/frameworks/ActiveRecordInjection.rb

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ def self.from(user_group_id)
1717
end
1818

1919
class Admin < User
20-
def self.delete_all(condition = nil)
21-
# BAD: `delete_all` overrides an ActiveRecord method, but doesn't perform
20+
def self.delete_by(condition = nil)
21+
# BAD: `delete_by` overrides an ActiveRecord method, but doesn't perform
2222
# any validation before passing its arguments on to another ActiveRecord method
23-
destroy_all(condition)
23+
destroy_by(condition)
2424
end
2525
end
2626

@@ -36,11 +36,11 @@ def some_request_handler
3636

3737
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
3838
# where `params[:id]` is unsanitized
39-
User.delete_all("id = '#{params[:id]}'")
39+
User.delete_by("id = '#{params[:id]}'")
4040

4141
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
4242
# where `params[:id]` is unsanitized
43-
User.destroy_all(["id = '#{params[:id]}'"])
43+
User.destroy_by(["id = '#{params[:id]}'"])
4444

4545
# BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
4646
# where `params[:min_id]` is unsanitized
@@ -65,7 +65,7 @@ def some_other_request_handler
6565

6666
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
6767
# where `uid` is unsantized
68-
User.delete_all("id " + uidEq)
68+
User.delete_by("id " + uidEq)
6969
end
7070

7171
def safe_paths
@@ -91,6 +91,6 @@ def safe_paths
9191

9292
class BazController < BarController
9393
def yet_another_handler
94-
Admin.delete_all(params[:admin_condition])
94+
Admin.delete_by(params[:admin_condition])
9595
end
9696
end

‎ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ def self.from(user_group_id)
1717
end
1818

1919
class Admin < User
20-
def self.delete_all(condition = nil)
21-
# BAD: `delete_all` overrides an ActiveRecord method, but doesn't perform
20+
def self.delete_by(condition = nil)
21+
# BAD: `delete_by overrides an ActiveRecord method, but doesn't perform
2222
# any validation before passing its arguments on to another ActiveRecord method
23-
destroy_all(condition)
23+
destroy_by(condition)
2424
end
2525
end
2626

@@ -40,11 +40,11 @@ def some_request_handler
4040

4141
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
4242
# where `params[:id]` is unsanitized
43-
User.delete_all("id = '#{params[:id]}'")
43+
User.delete_by("id = '#{params[:id]}'")
4444

4545
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
4646
# where `params[:id]` is unsanitized
47-
User.destroy_all(["id = '#{params[:id]}'"])
47+
User.destroy_by(["id = '#{params[:id]}'"])
4848

4949
# BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
5050
# where `params[:min_id]` is unsanitized
@@ -69,7 +69,7 @@ def some_other_request_handler
6969

7070
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
7171
# where `uid` is unsantized
72-
User.delete_all("id " + uidEq)
72+
User.delete_by("id " + uidEq)
7373
end
7474

7575
def safe_paths
@@ -102,6 +102,6 @@ def safe_paths
102102

103103
class BazController < BarController
104104
def yet_another_handler
105-
Admin.delete_all(params[:admin_condition])
105+
Admin.delete_by(params[:admin_condition])
106106
end
107107
end

‎ql/test/query-tests/security/cwe-089/SqlInjection.expected

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,34 @@
11
edges
22
| ActiveRecordInjection.rb:8:25:8:28 | name : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
33
| ActiveRecordInjection.rb:8:31:8:34 | pass : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
4-
| ActiveRecordInjection.rb:20:23:20:31 | condition : | ActiveRecordInjection.rb:23:17:23:25 | condition |
4+
| ActiveRecordInjection.rb:20:22:20:30 | condition : | ActiveRecordInjection.rb:23:16:23:24 | condition |
55
| ActiveRecordInjection.rb:35:30:35:35 | call to params : | ActiveRecordInjection.rb:35:30:35:44 | ...[...] |
66
| ActiveRecordInjection.rb:39:18:39:23 | call to params : | ActiveRecordInjection.rb:39:18:39:32 | ...[...] |
7-
| ActiveRecordInjection.rb:43:30:43:35 | call to params : | ActiveRecordInjection.rb:43:21:43:43 | "id = '#{...}'" |
8-
| ActiveRecordInjection.rb:47:32:47:37 | call to params : | ActiveRecordInjection.rb:47:23:47:45 | "id = '#{...}'" |
7+
| ActiveRecordInjection.rb:43:29:43:34 | call to params : | ActiveRecordInjection.rb:43:20:43:42 | "id = '#{...}'" |
8+
| ActiveRecordInjection.rb:47:31:47:36 | call to params : | ActiveRecordInjection.rb:47:22:47:44 | "id = '#{...}'" |
99
| ActiveRecordInjection.rb:52:21:52:26 | call to params : | ActiveRecordInjection.rb:51:16:51:21 | <<-SQL |
1010
| ActiveRecordInjection.rb:58:34:58:39 | call to params : | ActiveRecordInjection.rb:58:20:58:47 | "user.id = '#{...}'" |
1111
| ActiveRecordInjection.rb:60:23:60:28 | call to params : | ActiveRecordInjection.rb:60:23:60:35 | ...[...] : |
1212
| ActiveRecordInjection.rb:60:23:60:35 | ...[...] : | ActiveRecordInjection.rb:8:25:8:28 | name : |
1313
| ActiveRecordInjection.rb:60:38:60:43 | call to params : | ActiveRecordInjection.rb:60:38:60:50 | ...[...] : |
1414
| ActiveRecordInjection.rb:60:38:60:50 | ...[...] : | ActiveRecordInjection.rb:8:31:8:34 | pass : |
15-
| ActiveRecordInjection.rb:66:10:66:15 | call to params : | ActiveRecordInjection.rb:72:21:72:33 | ... + ... |
16-
| ActiveRecordInjection.rb:105:22:105:27 | call to params : | ActiveRecordInjection.rb:105:22:105:45 | ...[...] : |
17-
| ActiveRecordInjection.rb:105:22:105:45 | ...[...] : | ActiveRecordInjection.rb:20:23:20:31 | condition : |
15+
| ActiveRecordInjection.rb:66:10:66:15 | call to params : | ActiveRecordInjection.rb:72:20:72:32 | ... + ... |
16+
| ActiveRecordInjection.rb:105:21:105:26 | call to params : | ActiveRecordInjection.rb:105:21:105:44 | ...[...] : |
17+
| ActiveRecordInjection.rb:105:21:105:44 | ...[...] : | ActiveRecordInjection.rb:20:22:20:30 | condition : |
1818
nodes
1919
| ActiveRecordInjection.rb:8:25:8:28 | name : | semmle.label | name : |
2020
| ActiveRecordInjection.rb:8:31:8:34 | pass : | semmle.label | pass : |
2121
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | semmle.label | "name='#{...}' and pass='#{...}'" |
22-
| ActiveRecordInjection.rb:20:23:20:31 | condition : | semmle.label | condition : |
23-
| ActiveRecordInjection.rb:23:17:23:25 | condition | semmle.label | condition |
22+
| ActiveRecordInjection.rb:20:22:20:30 | condition : | semmle.label | condition : |
23+
| ActiveRecordInjection.rb:23:16:23:24 | condition | semmle.label | condition |
2424
| ActiveRecordInjection.rb:35:30:35:35 | call to params : | semmle.label | call to params : |
2525
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] | semmle.label | ...[...] |
2626
| ActiveRecordInjection.rb:39:18:39:23 | call to params : | semmle.label | call to params : |
2727
| ActiveRecordInjection.rb:39:18:39:32 | ...[...] | semmle.label | ...[...] |
28-
| ActiveRecordInjection.rb:43:21:43:43 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
29-
| ActiveRecordInjection.rb:43:30:43:35 | call to params : | semmle.label | call to params : |
30-
| ActiveRecordInjection.rb:47:23:47:45 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
31-
| ActiveRecordInjection.rb:47:32:47:37 | call to params : | semmle.label | call to params : |
28+
| ActiveRecordInjection.rb:43:20:43:42 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
29+
| ActiveRecordInjection.rb:43:29:43:34 | call to params : | semmle.label | call to params : |
30+
| ActiveRecordInjection.rb:47:22:47:44 | "id = '#{...}'" | semmle.label | "id = '#{...}'" |
31+
| ActiveRecordInjection.rb:47:31:47:36 | call to params : | semmle.label | call to params : |
3232
| ActiveRecordInjection.rb:51:16:51:21 | <<-SQL | semmle.label | <<-SQL |
3333
| ActiveRecordInjection.rb:52:21:52:26 | call to params : | semmle.label | call to params : |
3434
| ActiveRecordInjection.rb:58:20:58:47 | "user.id = '#{...}'" | semmle.label | "user.id = '#{...}'" |
@@ -38,18 +38,18 @@ nodes
3838
| ActiveRecordInjection.rb:60:38:60:43 | call to params : | semmle.label | call to params : |
3939
| ActiveRecordInjection.rb:60:38:60:50 | ...[...] : | semmle.label | ...[...] : |
4040
| ActiveRecordInjection.rb:66:10:66:15 | call to params : | semmle.label | call to params : |
41-
| ActiveRecordInjection.rb:72:21:72:33 | ... + ... | semmle.label | ... + ... |
42-
| ActiveRecordInjection.rb:105:22:105:27 | call to params : | semmle.label | call to params : |
43-
| ActiveRecordInjection.rb:105:22:105:45 | ...[...] : | semmle.label | ...[...] : |
41+
| ActiveRecordInjection.rb:72:20:72:32 | ... + ... | semmle.label | ... + ... |
42+
| ActiveRecordInjection.rb:105:21:105:26 | call to params : | semmle.label | call to params : |
43+
| ActiveRecordInjection.rb:105:21:105:44 | ...[...] : | semmle.label | ...[...] : |
4444
subpaths
4545
#select
4646
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:60:23:60:28 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:60:23:60:28 | call to params | a user-provided value |
4747
| ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:60:38:60:43 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:60:38:60:43 | call to params | a user-provided value |
48-
| ActiveRecordInjection.rb:23:17:23:25 | condition | ActiveRecordInjection.rb:105:22:105:27 | call to params : | ActiveRecordInjection.rb:23:17:23:25 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:105:22:105:27 | call to params | a user-provided value |
48+
| ActiveRecordInjection.rb:23:16:23:24 | condition | ActiveRecordInjection.rb:105:21:105:26 | call to params : | ActiveRecordInjection.rb:23:16:23:24 | condition | This SQL query depends on $@. | ActiveRecordInjection.rb:105:21:105:26 | call to params | a user-provided value |
4949
| ActiveRecordInjection.rb:35:30:35:44 | ...[...] | ActiveRecordInjection.rb:35:30:35:35 | call to params : | ActiveRecordInjection.rb:35:30:35:44 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:35:30:35:35 | call to params | a user-provided value |
5050
| ActiveRecordInjection.rb:39:18:39:32 | ...[...] | ActiveRecordInjection.rb:39:18:39:23 | call to params : | ActiveRecordInjection.rb:39:18:39:32 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:39:18:39:23 | call to params | a user-provided value |
51-
| ActiveRecordInjection.rb:43:21:43:43 | "id = '#{...}'" | ActiveRecordInjection.rb:43:30:43:35 | call to params : | ActiveRecordInjection.rb:43:21:43:43 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:43:30:43:35 | call to params | a user-provided value |
52-
| ActiveRecordInjection.rb:47:23:47:45 | "id = '#{...}'" | ActiveRecordInjection.rb:47:32:47:37 | call to params : | ActiveRecordInjection.rb:47:23:47:45 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:47:32:47:37 | call to params | a user-provided value |
51+
| ActiveRecordInjection.rb:43:20:43:42 | "id = '#{...}'" | ActiveRecordInjection.rb:43:29:43:34 | call to params : | ActiveRecordInjection.rb:43:20:43:42 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:43:29:43:34 | call to params | a user-provided value |
52+
| ActiveRecordInjection.rb:47:22:47:44 | "id = '#{...}'" | ActiveRecordInjection.rb:47:31:47:36 | call to params : | ActiveRecordInjection.rb:47:22:47:44 | "id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:47:31:47:36 | call to params | a user-provided value |
5353
| ActiveRecordInjection.rb:51:16:51:21 | <<-SQL | ActiveRecordInjection.rb:52:21:52:26 | call to params : | ActiveRecordInjection.rb:51:16:51:21 | <<-SQL | This SQL query depends on $@. | ActiveRecordInjection.rb:52:21:52:26 | call to params | a user-provided value |
5454
| ActiveRecordInjection.rb:58:20:58:47 | "user.id = '#{...}'" | ActiveRecordInjection.rb:58:34:58:39 | call to params : | ActiveRecordInjection.rb:58:20:58:47 | "user.id = '#{...}'" | This SQL query depends on $@. | ActiveRecordInjection.rb:58:34:58:39 | call to params | a user-provided value |
55-
| ActiveRecordInjection.rb:72:21:72:33 | ... + ... | ActiveRecordInjection.rb:66:10:66:15 | call to params : | ActiveRecordInjection.rb:72:21:72:33 | ... + ... | This SQL query depends on $@. | ActiveRecordInjection.rb:66:10:66:15 | call to params | a user-provided value |
55+
| ActiveRecordInjection.rb:72:20:72:32 | ... + ... | ActiveRecordInjection.rb:66:10:66:15 | call to params : | ActiveRecordInjection.rb:72:20:72:32 | ... + ... | This SQL query depends on $@. | ActiveRecordInjection.rb:66:10:66:15 | call to params | a user-provided value |

0 commit comments

Comments
 (0)