Skip to content

Commit 56bf977

Browse files
alexrfordalexrford
committed
Ruby: trim some SQLi related comments from ActiveRecord.rb
1 parent de486ba commit 56bf977

File tree

3 files changed

+84
-112
lines changed

3 files changed

+84
-112
lines changed

‎ruby/ql/test/library-tests/frameworks/ActionController.expected

Lines changed: 33 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
11
actionControllerControllerClasses
2-
| ActiveRecord.rb:27:1:58:3 | FooController |
3-
| ActiveRecord.rb:60:1:90:3 | BarController |
4-
| ActiveRecord.rb:92:1:96:3 | BazController |
2+
| ActiveRecord.rb:23:1:37:3 | FooController |
3+
| ActiveRecord.rb:39:1:62:3 | BarController |
4+
| ActiveRecord.rb:64:1:68:3 | BazController |
55
| app/controllers/comments_controller.rb:1:1:7:3 | CommentsController |
66
| app/controllers/foo/bars_controller.rb:3:1:31:3 | BarsController |
77
| app/controllers/photos_controller.rb:1:1:4:3 | PhotosController |
88
| app/controllers/posts_controller.rb:1:1:10:3 | PostsController |
99
| app/controllers/users/notifications_controller.rb:2:3:5:5 | NotificationsController |
1010
actionControllerActionMethods
11-
| ActiveRecord.rb:32:3:57:5 | some_request_handler |
12-
| ActiveRecord.rb:61:3:69:5 | some_other_request_handler |
13-
| ActiveRecord.rb:71:3:89:5 | safe_paths |
14-
| ActiveRecord.rb:93:3:95:5 | yet_another_handler |
11+
| ActiveRecord.rb:27:3:36:5 | some_request_handler |
12+
| ActiveRecord.rb:40:3:45:5 | some_other_request_handler |
13+
| ActiveRecord.rb:47:3:61:5 | safe_paths |
14+
| ActiveRecord.rb:65:3:67:5 | yet_another_handler |
1515
| app/controllers/comments_controller.rb:2:3:3:5 | index |
1616
| app/controllers/comments_controller.rb:5:3:6:5 | show |
1717
| app/controllers/foo/bars_controller.rb:5:3:7:5 | index |
@@ -23,38 +23,38 @@ actionControllerActionMethods
2323
| app/controllers/posts_controller.rb:8:3:9:5 | upvote |
2424
| app/controllers/users/notifications_controller.rb:3:5:4:7 | mark_as_read |
2525
paramsCalls
26-
| ActiveRecord.rb:35:30:35:35 | call to params |
27-
| ActiveRecord.rb:39:29:39:34 | call to params |
28-
| ActiveRecord.rb:43:31:43:36 | call to params |
29-
| ActiveRecord.rb:48:21:48:26 | call to params |
30-
| ActiveRecord.rb:54:34:54:39 | call to params |
31-
| ActiveRecord.rb:56:23:56:28 | call to params |
32-
| ActiveRecord.rb:56:38:56:43 | call to params |
33-
| ActiveRecord.rb:62:10:62:15 | call to params |
34-
| ActiveRecord.rb:72:11:72:16 | call to params |
35-
| ActiveRecord.rb:77:12:77:17 | call to params |
36-
| ActiveRecord.rb:83:12:83:17 | call to params |
37-
| ActiveRecord.rb:88:15:88:20 | call to params |
38-
| ActiveRecord.rb:94:21:94:26 | call to params |
26+
| ActiveRecord.rb:28:30:28:35 | call to params |
27+
| ActiveRecord.rb:29:29:29:34 | call to params |
28+
| ActiveRecord.rb:30:31:30:36 | call to params |
29+
| ActiveRecord.rb:32:21:32:26 | call to params |
30+
| ActiveRecord.rb:34:34:34:39 | call to params |
31+
| ActiveRecord.rb:35:23:35:28 | call to params |
32+
| ActiveRecord.rb:35:38:35:43 | call to params |
33+
| ActiveRecord.rb:41:10:41:15 | call to params |
34+
| ActiveRecord.rb:48:11:48:16 | call to params |
35+
| ActiveRecord.rb:52:12:52:17 | call to params |
36+
| ActiveRecord.rb:57:12:57:17 | call to params |
37+
| ActiveRecord.rb:60:15:60:20 | call to params |
38+
| ActiveRecord.rb:66:21:66:26 | call to params |
3939
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
4040
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
4141
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |
4242
| app/controllers/foo/bars_controller.rb:22:10:22:15 | call to params |
4343
| app/views/foo/bars/show.html.erb:5:9:5:14 | call to params |
4444
paramsSources
45-
| ActiveRecord.rb:35:30:35:35 | call to params |
46-
| ActiveRecord.rb:39:29:39:34 | call to params |
47-
| ActiveRecord.rb:43:31:43:36 | call to params |
48-
| ActiveRecord.rb:48:21:48:26 | call to params |
49-
| ActiveRecord.rb:54:34:54:39 | call to params |
50-
| ActiveRecord.rb:56:23:56:28 | call to params |
51-
| ActiveRecord.rb:56:38:56:43 | call to params |
52-
| ActiveRecord.rb:62:10:62:15 | call to params |
53-
| ActiveRecord.rb:72:11:72:16 | call to params |
54-
| ActiveRecord.rb:77:12:77:17 | call to params |
55-
| ActiveRecord.rb:83:12:83:17 | call to params |
56-
| ActiveRecord.rb:88:15:88:20 | call to params |
57-
| ActiveRecord.rb:94:21:94:26 | call to params |
45+
| ActiveRecord.rb:28:30:28:35 | call to params |
46+
| ActiveRecord.rb:29:29:29:34 | call to params |
47+
| ActiveRecord.rb:30:31:30:36 | call to params |
48+
| ActiveRecord.rb:32:21:32:26 | call to params |
49+
| ActiveRecord.rb:34:34:34:39 | call to params |
50+
| ActiveRecord.rb:35:23:35:28 | call to params |
51+
| ActiveRecord.rb:35:38:35:43 | call to params |
52+
| ActiveRecord.rb:41:10:41:15 | call to params |
53+
| ActiveRecord.rb:48:11:48:16 | call to params |
54+
| ActiveRecord.rb:52:12:52:17 | call to params |
55+
| ActiveRecord.rb:57:12:57:17 | call to params |
56+
| ActiveRecord.rb:60:15:60:20 | call to params |
57+
| ActiveRecord.rb:66:21:66:26 | call to params |
5858
| app/controllers/foo/bars_controller.rb:13:21:13:26 | call to params |
5959
| app/controllers/foo/bars_controller.rb:14:10:14:15 | call to params |
6060
| app/controllers/foo/bars_controller.rb:21:21:21:26 | call to params |

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

Lines changed: 51 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,60 +1,60 @@
11
activeRecordModelClasses
22
| ActiveRecord.rb:1:1:3:3 | UserGroup |
3-
| ActiveRecord.rb:5:1:17:3 | User |
4-
| ActiveRecord.rb:19:1:25:3 | Admin |
3+
| ActiveRecord.rb:5:1:15:3 | User |
4+
| ActiveRecord.rb:17:1:21:3 | Admin |
55
activeRecordInstances
6-
| ActiveRecord.rb:10:5:10:68 | call to find |
7-
| ActiveRecord.rb:15:5:15:40 | call to find_by |
8-
| ActiveRecord.rb:79:5:81:7 | if ... |
9-
| ActiveRecord.rb:79:43:80:40 | then ... |
10-
| ActiveRecord.rb:80:7:80:40 | call to find_by |
11-
| ActiveRecord.rb:85:5:85:33 | call to find_by |
12-
| ActiveRecord.rb:88:5:88:34 | call to find |
6+
| ActiveRecord.rb:9:5:9:68 | call to find |
7+
| ActiveRecord.rb:13:5:13:40 | call to find_by |
8+
| ActiveRecord.rb:53:5:55:7 | if ... |
9+
| ActiveRecord.rb:53:43:54:40 | then ... |
10+
| ActiveRecord.rb:54:7:54:40 | call to find_by |
11+
| ActiveRecord.rb:58:5:58:33 | call to find_by |
12+
| ActiveRecord.rb:60:5:60:34 | call to find |
1313
activeRecordSqlExecutionRanges
14-
| ActiveRecord.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" |
15-
| ActiveRecord.rb:23:16:23:24 | condition |
16-
| ActiveRecord.rb:35:30:35:44 | ...[...] |
17-
| ActiveRecord.rb:39:20:39:42 | "id = '#{...}'" |
18-
| ActiveRecord.rb:43:22:43:44 | "id = '#{...}'" |
19-
| ActiveRecord.rb:47:16:47:21 | <<-SQL |
20-
| ActiveRecord.rb:54:20:54:47 | "user.id = '#{...}'" |
21-
| ActiveRecord.rb:68:20:68:32 | ... + ... |
22-
| ActiveRecord.rb:75:16:75:28 | "name #{...}" |
23-
| ActiveRecord.rb:80:20:80:39 | "username = #{...}" |
14+
| ActiveRecord.rb:9:33:9:67 | "name='#{...}' and pass='#{...}'" |
15+
| ActiveRecord.rb:19:16:19:24 | condition |
16+
| ActiveRecord.rb:28:30:28:44 | ...[...] |
17+
| ActiveRecord.rb:29:20:29:42 | "id = '#{...}'" |
18+
| ActiveRecord.rb:30:22:30:44 | "id = '#{...}'" |
19+
| ActiveRecord.rb:31:16:31:21 | <<-SQL |
20+
| ActiveRecord.rb:34:20:34:47 | "user.id = '#{...}'" |
21+
| ActiveRecord.rb:44:20:44:32 | ... + ... |
22+
| ActiveRecord.rb:50:16:50:28 | "name #{...}" |
23+
| ActiveRecord.rb:54:20:54:39 | "username = #{...}" |
2424
activeRecordModelClassMethodCalls
2525
| ActiveRecord.rb:2:3:2:17 | call to has_many |
2626
| ActiveRecord.rb:6:3:6:24 | call to belongs_to |
27-
| ActiveRecord.rb:10:5:10:68 | call to find |
28-
| ActiveRecord.rb:15:5:15:40 | call to find_by |
29-
| ActiveRecord.rb:15:5:15:46 | call to users |
30-
| ActiveRecord.rb:23:5:23:25 | call to destroy_by |
31-
| ActiveRecord.rb:35:5:35:45 | call to calculate |
32-
| ActiveRecord.rb:39:5:39:43 | call to delete_by |
33-
| ActiveRecord.rb:43:5:43:46 | call to destroy_by |
34-
| ActiveRecord.rb:47:5:47:35 | call to where |
35-
| ActiveRecord.rb:54:5:54:14 | call to where |
36-
| ActiveRecord.rb:54:5:54:48 | call to not |
37-
| ActiveRecord.rb:56:5:56:51 | call to authenticate |
38-
| ActiveRecord.rb:68:5:68:33 | call to delete_by |
39-
| ActiveRecord.rb:75:5:75:29 | call to order |
40-
| ActiveRecord.rb:80:7:80:40 | call to find_by |
41-
| ActiveRecord.rb:85:5:85:33 | call to find_by |
42-
| ActiveRecord.rb:88:5:88:34 | call to find |
43-
| ActiveRecord.rb:94:5:94:45 | call to delete_by |
27+
| ActiveRecord.rb:9:5:9:68 | call to find |
28+
| ActiveRecord.rb:13:5:13:40 | call to find_by |
29+
| ActiveRecord.rb:13:5:13:46 | call to users |
30+
| ActiveRecord.rb:19:5:19:25 | call to destroy_by |
31+
| ActiveRecord.rb:28:5:28:45 | call to calculate |
32+
| ActiveRecord.rb:29:5:29:43 | call to delete_by |
33+
| ActiveRecord.rb:30:5:30:46 | call to destroy_by |
34+
| ActiveRecord.rb:31:5:31:35 | call to where |
35+
| ActiveRecord.rb:34:5:34:14 | call to where |
36+
| ActiveRecord.rb:34:5:34:48 | call to not |
37+
| ActiveRecord.rb:35:5:35:51 | call to authenticate |
38+
| ActiveRecord.rb:44:5:44:33 | call to delete_by |
39+
| ActiveRecord.rb:50:5:50:29 | call to order |
40+
| ActiveRecord.rb:54:7:54:40 | call to find_by |
41+
| ActiveRecord.rb:58:5:58:33 | call to find_by |
42+
| ActiveRecord.rb:60:5:60:34 | call to find |
43+
| ActiveRecord.rb:66:5:66:45 | call to delete_by |
4444
potentiallyUnsafeSqlExecutingMethodCall
45-
| ActiveRecord.rb:10:5:10:68 | call to find |
46-
| ActiveRecord.rb:23:5:23:25 | call to destroy_by |
47-
| ActiveRecord.rb:35:5:35:45 | call to calculate |
48-
| ActiveRecord.rb:39:5:39:43 | call to delete_by |
49-
| ActiveRecord.rb:43:5:43:46 | call to destroy_by |
50-
| ActiveRecord.rb:47:5:47:35 | call to where |
51-
| ActiveRecord.rb:54:5:54:48 | call to not |
52-
| ActiveRecord.rb:68:5:68:33 | call to delete_by |
53-
| ActiveRecord.rb:75:5:75:29 | call to order |
54-
| ActiveRecord.rb:80:7:80:40 | call to find_by |
45+
| ActiveRecord.rb:9:5:9:68 | call to find |
46+
| ActiveRecord.rb:19:5:19:25 | call to destroy_by |
47+
| ActiveRecord.rb:28:5:28:45 | call to calculate |
48+
| ActiveRecord.rb:29:5:29:43 | call to delete_by |
49+
| ActiveRecord.rb:30:5:30:46 | call to destroy_by |
50+
| ActiveRecord.rb:31:5:31:35 | call to where |
51+
| ActiveRecord.rb:34:5:34:48 | call to not |
52+
| ActiveRecord.rb:44:5:44:33 | call to delete_by |
53+
| ActiveRecord.rb:50:5:50:29 | call to order |
54+
| ActiveRecord.rb:54:7:54:40 | call to find_by |
5555
activeRecordModelInstantiations
56-
| ActiveRecord.rb:10:5:10:68 | call to find | ActiveRecord.rb:5:1:17:3 | User |
57-
| ActiveRecord.rb:15:5:15:40 | call to find_by | ActiveRecord.rb:1:1:3:3 | UserGroup |
58-
| ActiveRecord.rb:80:7:80:40 | call to find_by | ActiveRecord.rb:5:1:17:3 | User |
59-
| ActiveRecord.rb:85:5:85:33 | call to find_by | ActiveRecord.rb:5:1:17:3 | User |
60-
| ActiveRecord.rb:88:5:88:34 | call to find | ActiveRecord.rb:5:1:17:3 | User |
56+
| ActiveRecord.rb:9:5:9:68 | call to find | ActiveRecord.rb:5:1:15:3 | User |
57+
| ActiveRecord.rb:13:5:13:40 | call to find_by | ActiveRecord.rb:1:1:3:3 | UserGroup |
58+
| ActiveRecord.rb:54:7:54:40 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
59+
| ActiveRecord.rb:58:5:58:33 | call to find_by | ActiveRecord.rb:5:1:15:3 | User |
60+
| ActiveRecord.rb:60:5:60:34 | call to find | ActiveRecord.rb:5:1:15:3 | User |

‎ruby/ql/test/library-tests/frameworks/ActiveRecord.rb

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,16 @@ class User < ApplicationRecord
66
belongs_to :user_group
77

88
def self.authenticate(name, pass)
9-
# BAD: possible untrusted input interpolated into SQL fragment
109
find(:first, :conditions => "name='#{name}' and pass='#{pass}'")
1110
end
1211

1312
def self.from(user_group_id)
14-
# GOOD: `find_by` with hash argument
1513
UserGroup.find_by(id: user_group_id).users
1614
end
1715
end
1816

1917
class Admin < User
2018
def self.delete_by(condition = nil)
21-
# BAD: `delete_by` overrides an ActiveRecord method, but doesn't perform
22-
# any validation before passing its arguments on to another ActiveRecord method
2319
destroy_by(condition)
2420
end
2521
end
@@ -28,31 +24,14 @@ class FooController < ActionController::Base
2824

2925
MAX_USER_ID = 100_000
3026

31-
# A string tainted by user input is inserted into an SQL query
3227
def some_request_handler
33-
# BAD: executes `SELECT AVG(#{params[:column]}) FROM "users"`
34-
# where `params[:column]` is unsanitized
3528
User.calculate(:average, params[:column])
36-
37-
# BAD: executes `DELETE FROM "users" WHERE (id = '#{params[:id]}')`
38-
# where `params[:id]` is unsanitized
3929
User.delete_by("id = '#{params[:id]}'")
40-
41-
# BAD: executes `SELECT "users".* FROM "users" WHERE (id = '#{params[:id]}')`
42-
# where `params[:id]` is unsanitized
4330
User.destroy_by(["id = '#{params[:id]}'"])
44-
45-
# BAD: executes `SELECT "users".* FROM "users" WHERE id BETWEEN '#{params[:min_id]}' AND 100000`
46-
# where `params[:min_id]` is unsanitized
4731
User.where(<<-SQL, MAX_USER_ID)
4832
id BETWEEN '#{params[:min_id]}' AND ?
4933
SQL
50-
51-
# BAD: chained method case
52-
# executes `SELECT "users".* FROM "users" WHERE (NOT (user_id = 'params[:id]'))`
53-
# where `params[:id]` is unsanitized
5434
User.where.not("user.id = '#{params[:id]}'")
55-
5635
User.authenticate(params[:name], params[:pass])
5736
end
5837
end
@@ -62,29 +41,22 @@ def some_other_request_handler
6241
ps = params
6342
uid = ps[:id]
6443
uidEq = "= '#{uid}'"
65-
66-
# BAD: executes `DELETE FROM "users" WHERE (id = #{uid})`
67-
# where `uid` is unsantized
6844
User.delete_by("id " + uidEq)
6945
end
7046

7147
def safe_paths
7248
dir = params[:order]
73-
# GOOD: barrier guard prevents taint flow
7449
dir = "DESC" unless dir == "ASC"
7550
User.order("name #{dir}")
7651

7752
name = params[:user_name]
78-
# GOOD: barrier guard prevents taint flow
7953
if %w(alice bob charlie).include? name
8054
User.find_by("username = #{name}")
8155
end
8256

8357
name = params[:user_name]
84-
# GOOD: hash arguments are sanitized by ActiveRecord
8558
User.find_by(user_name: name)
8659

87-
# OK: `find` method is overridden in `User`
8860
User.find(params[:user_group])
8961
end
9062
end

0 commit comments

Comments
 (0)