Add test cases and improve controller model

Author: joefarebrother

ruby/ql/lib/codeql/ruby/frameworks/Translation.qll

@@ -4,6 +4,7 @@ private import codeql.ruby.ApiGraphs
 private import codeql.ruby.dataflow.FlowSummary
 private import codeql.ruby.Concepts
 private import codeql.ruby.frameworks.ActionView
+private import codeql.ruby.frameworks.ActionController
 
 /** Gets a call to `I18n.translate`, which can use keyword arguments as inputs for string interpolation. */
 private MethodCall getI18nTranslateCall() {
@@ -25,9 +26,9 @@ private MethodCall getViewHelperTranslateCall() {
  */
 private MethodCall getControllerHelperTranslateCall() {
   result =
-    API::getTopLevelMember("ActionController")
-        .getMember("Base")
-        .getInstance()
+    any(ActionControllerClass c)
+        .getSelf()
+        .track()
         .getAMethodCall(["t", "translate"])
         .asExpr()
         .getExpr()

ruby/ql/test/library-tests/dataflow/local/TaintStep.expected

@@ -2798,6 +2798,7 @@
 | UseUseExplosion.rb:21:3691:21:3696 | call to use | UseUseExplosion.rb:21:3686:21:3696 | else ... |
 | UseUseExplosion.rb:24:5:25:7 | synthetic splat parameter | UseUseExplosion.rb:24:13:24:13 | i |
 | UseUseExplosion.rb:24:5:25:7 | use | UseUseExplosion.rb:1:1:26:3 | C |
+| file://:0:0:0:0 | [summary param] ** in I18n.translate | file://:0:0:0:0 | [summary] read: Argument[hash-splat].Element[any] in I18n.translate |
 | file://:0:0:0:0 | [summary param] position 0 in & | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in & |
 | file://:0:0:0:0 | [summary param] position 0 in + | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in + |
 | file://:0:0:0:0 | [summary param] position 0 in ActionController::Parameters#merge | file://:0:0:0:0 | [summary] to write: ReturnValue in ActionController::Parameters#merge |
@@ -2840,6 +2841,7 @@
 | file://:0:0:0:0 | [summary param] self in assoc-unknown-arg | file://:0:0:0:0 | [summary] read: Argument[self].Element[any] in assoc-unknown-arg |
 | file://:0:0:0:0 | [summary param] self in each(0) | file://:0:0:0:0 | [summary] read: Argument[self].Element[any] in each(0) |
 | file://:0:0:0:0 | [summary] read: Argument[0].Element[any] in Hash[] | file://:0:0:0:0 | [summary] read: Argument[0].Element[any].Element[1] in Hash[] |
+| file://:0:0:0:0 | [summary] read: Argument[hash-splat].Element[any] in I18n.translate | file://:0:0:0:0 | [summary] to write: ReturnValue in I18n.translate |
 | local_dataflow.rb:1:1:7:3 | self (foo) | local_dataflow.rb:3:8:3:10 | self |
 | local_dataflow.rb:1:1:7:3 | self in foo | local_dataflow.rb:1:1:7:3 | self (foo) |
 | local_dataflow.rb:1:1:7:3 | synthetic splat parameter | local_dataflow.rb:1:9:1:9 | a |

ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected

@@ -22,10 +22,20 @@ edges
 | app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:35:3:35:14 | call to display_text | provenance |  |
 | app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:43:76:43:87 | call to display_text | provenance |  |
 | app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | provenance |  |
+| app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:85:36:85:47 | call to display_text | provenance |  |
+| app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | app/views/foo/bars/show.html.erb:86:28:86:39 | call to display_text | provenance |  |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt | app/controllers/foo/bars_controller.rb:26:37:26:76 | call to [] [element :display_text] | provenance |  |
 | app/controllers/foo/bars_controller.rb:30:5:30:7 | str | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | provenance |  |
 | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] | provenance |  |
 | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] | app/controllers/foo/bars_controller.rb:30:5:30:7 | str | provenance |  |
+| app/controllers/foo/bars_controller.rb:33:32:33:37 | call to params | app/controllers/foo/bars_controller.rb:33:32:33:49 | ...[...] | provenance |  |
+| app/controllers/foo/bars_controller.rb:33:32:33:49 | ...[...] | app/controllers/foo/bars_controller.rb:33:5:33:50 | call to translate | provenance |  |
+| app/controllers/foo/bars_controller.rb:34:24:34:29 | call to params | app/controllers/foo/bars_controller.rb:34:24:34:41 | ...[...] | provenance |  |
+| app/controllers/foo/bars_controller.rb:34:24:34:41 | ...[...] | app/controllers/foo/bars_controller.rb:34:5:34:42 | call to t | provenance |  |
+| app/controllers/foo/bars_controller.rb:36:34:36:39 | call to params | app/controllers/foo/bars_controller.rb:36:34:36:51 | ...[...] | provenance |  |
+| app/controllers/foo/bars_controller.rb:36:34:36:51 | ...[...] | app/controllers/foo/bars_controller.rb:36:5:36:52 | call to t | provenance |  |
+| app/controllers/foo/bars_controller.rb:37:42:37:47 | call to params | app/controllers/foo/bars_controller.rb:37:42:37:59 | ...[...] | provenance |  |
+| app/controllers/foo/bars_controller.rb:37:42:37:59 | ...[...] | app/controllers/foo/bars_controller.rb:37:5:37:60 | call to translate | provenance |  |
 | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text [element] | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | provenance |  |
 | app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text, element] | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] [element] | provenance |  |
 | app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text] | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | provenance |  |
@@ -45,6 +55,8 @@ edges
 | app/views/foo/bars/show.html.erb:56:13:56:18 | call to params | app/views/foo/bars/show.html.erb:56:13:56:28 | ...[...] | provenance |  |
 | app/views/foo/bars/show.html.erb:73:19:73:24 | call to params | app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | provenance |  |
 | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | provenance |  |
+| app/views/foo/bars/show.html.erb:85:36:85:47 | call to display_text | app/views/foo/bars/show.html.erb:85:9:85:48 | call to translate | provenance |  |
+| app/views/foo/bars/show.html.erb:86:28:86:39 | call to display_text | app/views/foo/bars/show.html.erb:86:9:86:40 | call to t | provenance |  |
 nodes
 | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params | semmle.label | call to params |
 | app/controllers/foo/bars_controller.rb:9:12:9:29 | ...[...] | semmle.label | ...[...] |
@@ -66,6 +78,18 @@ nodes
 | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | semmle.label | call to params |
 | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] | semmle.label | ...[...] |
 | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | semmle.label | str |
+| app/controllers/foo/bars_controller.rb:33:5:33:50 | call to translate | semmle.label | call to translate |
+| app/controllers/foo/bars_controller.rb:33:32:33:37 | call to params | semmle.label | call to params |
+| app/controllers/foo/bars_controller.rb:33:32:33:49 | ...[...] | semmle.label | ...[...] |
+| app/controllers/foo/bars_controller.rb:34:5:34:42 | call to t | semmle.label | call to t |
+| app/controllers/foo/bars_controller.rb:34:24:34:29 | call to params | semmle.label | call to params |
+| app/controllers/foo/bars_controller.rb:34:24:34:41 | ...[...] | semmle.label | ...[...] |
+| app/controllers/foo/bars_controller.rb:36:5:36:52 | call to t | semmle.label | call to t |
+| app/controllers/foo/bars_controller.rb:36:34:36:39 | call to params | semmle.label | call to params |
+| app/controllers/foo/bars_controller.rb:36:34:36:51 | ...[...] | semmle.label | ...[...] |
+| app/controllers/foo/bars_controller.rb:37:5:37:60 | call to translate | semmle.label | call to translate |
+| app/controllers/foo/bars_controller.rb:37:42:37:47 | call to params | semmle.label | call to params |
+| app/controllers/foo/bars_controller.rb:37:42:37:59 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
 | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text [element] | semmle.label | call to display_text [element] |
 | app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text, element] | semmle.label | call to local_assigns [element :display_text, element] |
@@ -98,10 +122,18 @@ nodes
 | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | semmle.label | call to params |
 | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | semmle.label | call to display_text |
+| app/views/foo/bars/show.html.erb:85:9:85:48 | call to translate | semmle.label | call to translate |
+| app/views/foo/bars/show.html.erb:85:36:85:47 | call to display_text | semmle.label | call to display_text |
+| app/views/foo/bars/show.html.erb:86:9:86:40 | call to t | semmle.label | call to t |
+| app/views/foo/bars/show.html.erb:86:28:86:39 | call to display_text | semmle.label | call to display_text |
 subpaths
 #select
 | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params | user-provided value |
 | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params | user-provided value |
+| app/controllers/foo/bars_controller.rb:33:5:33:50 | call to translate | app/controllers/foo/bars_controller.rb:33:32:33:37 | call to params | app/controllers/foo/bars_controller.rb:33:5:33:50 | call to translate | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:33:32:33:37 | call to params | user-provided value |
+| app/controllers/foo/bars_controller.rb:34:5:34:42 | call to t | app/controllers/foo/bars_controller.rb:34:24:34:29 | call to params | app/controllers/foo/bars_controller.rb:34:5:34:42 | call to t | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:34:24:34:29 | call to params | user-provided value |
+| app/controllers/foo/bars_controller.rb:36:5:36:52 | call to t | app/controllers/foo/bars_controller.rb:36:34:36:39 | call to params | app/controllers/foo/bars_controller.rb:36:5:36:52 | call to t | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:36:34:36:39 | call to params | user-provided value |
+| app/controllers/foo/bars_controller.rb:37:5:37:60 | call to translate | app/controllers/foo/bars_controller.rb:37:42:37:47 | call to params | app/controllers/foo/bars_controller.rb:37:5:37:60 | call to translate | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:37:42:37:47 | call to params | user-provided value |
 | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:17:21:17:26 | call to params | user-provided value |
@@ -118,3 +150,5 @@ subpaths
 | app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | app/views/foo/bars/show.html.erb:73:19:73:24 | call to params | app/views/foo/bars/show.html.erb:73:19:73:34 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:73:19:73:24 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | app/views/foo/bars/show.html.erb:76:28:76:39 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/views/foo/bars/show.html.erb:76:28:76:33 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/show.html.erb:82:6:82:17 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
+| app/views/foo/bars/show.html.erb:85:9:85:48 | call to translate | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/show.html.erb:85:9:85:48 | call to translate | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
+| app/views/foo/bars/show.html.erb:86:9:86:40 | call to t | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | app/views/foo/bars/show.html.erb:86:9:86:40 | call to t | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |

ruby/ql/test/query-tests/security/cwe-079/app/controllers/foo/bars_controller.rb

@@ -29,5 +29,11 @@ def show
   def make_safe_html
     str = params[:user_name]
     str.html_safe
+
+    translate("welcome", name: params[:user_name]).html_safe # NOT OK - translate preserves taint
+    t("welcome", name: params[:user_name]).html_safe # NOT OK - t is an alias of translate
+    t("welcome_html", name: params[:user_name]).html_safe # OK - t escapes html when key ends in _html
+    I18n.t("welcome_html", name: params[:user_name]).html_safe # NOT OK - I18n.t does not escape html
+    I18n.translate("welcome_html", name: params[:user_name]).html_safe # NOT OK - alias
   end
 end

ruby/ql/test/query-tests/security/cwe-079/app/views/foo/bars/show.html.erb

@@ -80,3 +80,10 @@
 
 <%# BAD: A local rendered raw as a local variable %>
 <%== display_text %>
+
+<%# BAD: translate preserves taint %>
+<%= raw translate("welcome", name: display_text) %>
+<%= raw t("welcome", name: display_text) %>
+
+<%# GOOD: translate sanitizes for html keys %>
+<%= raw t("welcome1.html", name: display_text) %>