diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll index 1093d406ab8..de678e0ec08 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll @@ -24,7 +24,7 @@ deprecated class ParamsCall = Rails::ParamsCall; deprecated class CookiesCall = Rails::CookiesCall; /** - * A `ClassDeclaration` for a class that extends `ActionController::Base`. + * A class that extends `ActionController::Base`. * For example, * * ```rb @@ -36,26 +36,59 @@ deprecated class CookiesCall = Rails::CookiesCall; * end * ``` */ -class ActionControllerControllerClass extends ClassDeclaration { - ActionControllerControllerClass() { - this.getSuperclassExpr() = +class ActionControllerClass extends DataFlow::ClassNode { + ActionControllerClass() { + this = [ - API::getTopLevelMember("ActionController").getMember("Base"), + DataFlow::getConst("ActionController").getConst("Base"), // In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we // treat it separately in case the `ApplicationController` definition is not in the database. - API::getTopLevelMember("ApplicationController"), + DataFlow::getConst("ApplicationController"), // ActionController::Metal technically doesn't contain all of the // methods available in Base, such as those for rendering views. // However we prefer to be over-sensitive in this case in order to find // more results. - API::getTopLevelMember("ActionController").getMember("Metal") - ].getASubclass().getAValueReachableFromSource().asExpr().getExpr() + DataFlow::getConst("ActionController").getConst("Metal") + ].getADescendentModule() } /** * Gets a `ActionControllerActionMethod` defined in this class. */ - ActionControllerActionMethod getAnAction() { result = this.getAMethod() } + ActionControllerActionMethod getAnAction() { result = this.getAnInstanceMethod().asMethod() } + + /** + * Gets a `self` that possibly refers to an instance of this class. + */ + DataFlow::LocalSourceNode getSelf() { + result = this.getAnInstanceSelf() + or + // Include the module-level `self` to recover some cases where a block at the module level + // is invoked with an instance as the `self`, which we currently can't model directly. + // Concretely this happens in the block passed to `rescue_from`. + // TODO: revisit when we have better infrastructure for handling self in a block + result = this.getModuleLevelSelf() + } +} + +private DataFlow::LocalSourceNode actionControllerInstance() { + result = any(ActionControllerClass cls).getSelf() +} + +/** + * DEPRECATED. Use `ActionControllerClass` instead. + * + * A `ClassDeclaration` corresponding to an `ActionControllerClass`. + */ +deprecated class ActionControllerControllerClass extends ClassDeclaration { + ActionControllerControllerClass() { this = any(ActionControllerClass cls).getADeclaration() } + + /** + * Gets a `ActionControllerActionMethod` defined in this class. + */ + ActionControllerActionMethod getAnAction() { + result = this.getAMethod().(Method) and result.isPrivate() + } } /** @@ -63,9 +96,11 @@ class ActionControllerControllerClass extends ClassDeclaration { * This may be the target of a route handler, if such a route is defined. */ class ActionControllerActionMethod extends Method, Http::Server::RequestHandler::Range { - private ActionControllerControllerClass controllerClass; + private ActionControllerClass controllerClass; - ActionControllerActionMethod() { this = controllerClass.getAMethod() and not this.isPrivate() } + ActionControllerActionMethod() { + this = controllerClass.getAnInstanceMethod().asMethod() and not this.isPrivate() + } /** * Establishes a mapping between a method within the file @@ -91,7 +126,7 @@ class ActionControllerActionMethod extends Method, Http::Server::RequestHandler: /** * Gets the controller class containing this method. */ - ActionControllerControllerClass getControllerClass() { + ActionControllerClass getControllerClass() { // TODO: model the implicit render call when a path through the method does // not end at an explicit render or redirect result = controllerClass @@ -102,37 +137,23 @@ class ActionControllerActionMethod extends Method, Http::Server::RequestHandler: * May return multiple results. */ ActionDispatch::Routing::Route getARoute() { - exists(string name | + exists(string name, DataFlow::MethodNode m | isRoute(result, name, controllerClass) and - isActionControllerMethod(this, name, controllerClass) + m = controllerClass.getInstanceMethod(name) and + this = m.asMethod() ) } } pragma[nomagic] private predicate isRoute( - ActionDispatch::Routing::Route route, string name, ActionControllerControllerClass controllerClass + ActionDispatch::Routing::Route route, string name, ActionControllerClass controllerClass ) { route.getController() + "_controller" = - ActionDispatch::Routing::underscore(controllerClass.getAQualifiedName()) and + ActionDispatch::Routing::underscore(controllerClass.getQualifiedName()) and name = route.getAction() } -// A method call with a `self` receiver from within a controller class -private class ActionControllerContextCall extends MethodCall { - private ActionControllerControllerClass controllerClass; - - ActionControllerContextCall() { - this.getReceiver() instanceof SelfVariableAccess and - this.getEnclosingModule() = controllerClass - } - - /** - * Gets the controller class containing this method. - */ - ActionControllerControllerClass getControllerClass() { result = controllerClass } -} - /** * A `RemoteFlowSource::Range` to represent accessing the * ActionController parameters available via the `params` method. @@ -158,13 +179,17 @@ class CookiesSource extends Http::Server::RequestInputAccess::Range { } /** A call to `cookies` from within a controller. */ -private class ActionControllerCookiesCall extends ActionControllerContextCall, CookiesCallImpl { - ActionControllerCookiesCall() { this.getMethodName() = "cookies" } +private class ActionControllerCookiesCall extends CookiesCallImpl { + ActionControllerCookiesCall() { + this = actionControllerInstance().getAMethodCall("cookies").asExpr().getExpr() + } } /** A call to `params` from within a controller. */ -private class ActionControllerParamsCall extends ActionControllerContextCall, ParamsCallImpl { - ActionControllerParamsCall() { this.getMethodName() = "params" } +private class ActionControllerParamsCall extends ParamsCallImpl { + ActionControllerParamsCall() { + this = actionControllerInstance().getAMethodCall("params").asExpr().getExpr() + } } /** Modeling for `ActionDispatch::Request`. */ @@ -174,10 +199,7 @@ private module Request { * `ActionDispatch::Request`. */ private class RequestNode extends DataFlow::CallNode { - RequestNode() { - this.asExpr().getExpr() instanceof ActionControllerContextCall and - this.getMethodName() = "request" - } + RequestNode() { this = actionControllerInstance().getAMethodCall("request") } } /** @@ -290,8 +312,7 @@ private module Request { */ private class EnvHttpAccess extends DataFlow::CallNode, Http::Server::RequestInputAccess::Range { EnvHttpAccess() { - any(EnvCall c).(DataFlow::LocalSourceNode).flowsTo(this.getReceiver()) and - this.getMethodName() = "[]" and + this = any(EnvCall c).getAMethodCall("[]") and this.getArgument(0).getConstantValue().getString().regexpMatch("^HTTP_.+") } @@ -302,21 +323,32 @@ private module Request { } /** A call to `render` from within a controller. */ -private class ActionControllerRenderCall extends ActionControllerContextCall, RenderCallImpl { - ActionControllerRenderCall() { this.getMethodName() = "render" } +private class ActionControllerRenderCall extends RenderCallImpl { + ActionControllerRenderCall() { + this = actionControllerInstance().getAMethodCall("render").asExpr().getExpr() + } } /** A call to `render_to` from within a controller. */ -private class ActionControllerRenderToCall extends ActionControllerContextCall, RenderToCallImpl { - ActionControllerRenderToCall() { this.getMethodName() = ["render_to_body", "render_to_string"] } +private class ActionControllerRenderToCall extends RenderToCallImpl { + ActionControllerRenderToCall() { + this = + actionControllerInstance() + .getAMethodCall(["render_to_body", "render_to_string"]) + .asExpr() + .getExpr() + } } /** A call to `html_escape` from within a controller. */ private class ActionControllerHtmlEscapeCall extends HtmlEscapeCallImpl { ActionControllerHtmlEscapeCall() { // "h" is aliased to "html_escape" in ActiveSupport - this.getMethodName() = ["html_escape", "html_escape_once", "h", "sanitize"] and - this.getEnclosingModule() instanceof ActionControllerControllerClass + this = + actionControllerInstance() + .getAMethodCall(["html_escape", "html_escape_once", "h", "sanitize"]) + .asExpr() + .getExpr() } } @@ -324,9 +356,16 @@ private class ActionControllerHtmlEscapeCall extends HtmlEscapeCallImpl { * A call to the `redirect_to` method, used in an action to redirect to a * specific URL/path or to a different action in this controller. */ -class RedirectToCall extends ActionControllerContextCall { +class RedirectToCall extends MethodCall { + private ActionControllerClass controller; + RedirectToCall() { - this.getMethodName() = ["redirect_to", "redirect_back", "redirect_back_or_to"] + this = + controller + .getSelf() + .getAMethodCall(["redirect_to", "redirect_back", "redirect_back_or_to"]) + .asExpr() + .getExpr() } /** Gets the `Expr` representing the URL to redirect to, if any */ @@ -338,8 +377,10 @@ class RedirectToCall extends ActionControllerContextCall { /** Gets the `ActionControllerActionMethod` to redirect to, if any */ ActionControllerActionMethod getRedirectActionMethod() { - this.getKeywordArgument("action").getConstantValue().isStringlikeValue(result.getName()) and - result.getEnclosingModule() = this.getControllerClass() + exists(string name | + this.getKeywordArgument("action").getConstantValue().isStringlikeValue(name) and + result = controller.getInstanceMethod(name).asMethod() + ) } /** @@ -373,18 +414,8 @@ class ActionControllerRedirectResponse extends Http::Server::HttpRedirectRespons } pragma[nomagic] -private predicate isActionControllerMethod(Method m, string name, ActionControllerControllerClass c) { - m.getName() = name and - m.getEnclosingModule() = c -} - -pragma[nomagic] -private predicate actionControllerHasHelperMethodCall(ActionControllerControllerClass c, string name) { - exists(MethodCall mc | - mc.getMethodName() = "helper_method" and - mc.getAnArgument().getConstantValue().isStringlikeValue(name) and - mc.getEnclosingModule() = c - ) +private predicate actionControllerHasHelperMethodCall(ActionControllerClass c, string name) { + c.getAModuleLevelCall("helper_method").getArgument(_).getConstantValue().isStringlikeValue(name) } /** @@ -404,27 +435,28 @@ private predicate actionControllerHasHelperMethodCall(ActionControllerController * See also https://api.rubyonrails.org/classes/AbstractController/Helpers/ClassMethods.html#method-i-helper_method */ class ActionControllerHelperMethod extends Method { - private ActionControllerControllerClass controllerClass; + private ActionControllerClass controllerClass; ActionControllerHelperMethod() { - exists(string name | - isActionControllerMethod(this, name, controllerClass) and - actionControllerHasHelperMethodCall(controllerClass, name) + exists(DataFlow::MethodNode m, string name | + m = controllerClass.getInstanceMethod(name) and + actionControllerHasHelperMethodCall(controllerClass, name) and + this = m.asMethod() ) } /** Gets the class containing this helper method. */ - ActionControllerControllerClass getControllerClass() { result = controllerClass } + ActionControllerClass getControllerClass() { result = controllerClass } } /** - * Gets an `ActionControllerControllerClass` associated with the given `ErbFile` + * Gets an `ActionControllerClass` associated with the given `ErbFile` * according to Rails path conventions. * For instance, a template file at `app/views/foo/bar/baz.html.erb` will be * mapped to a controller class in `app/controllers/foo/bar/baz_controller.rb`, * if such a controller class exists. */ -ActionControllerControllerClass getAssociatedControllerClass(ErbFile f) { +ActionControllerClass getAssociatedControllerClass(ErbFile f) { // There is a direct mapping from template file to controller class controllerTemplateFile(result, f) or @@ -448,7 +480,7 @@ ActionControllerControllerClass getAssociatedControllerClass(ErbFile f) { * This handles mappings between controllers in `app/controllers/`, and * templates in `app/views/` and `app/views/layouts/`. */ -predicate controllerTemplateFile(ActionControllerControllerClass cls, ErbFile templateFile) { +predicate controllerTemplateFile(ActionControllerClass cls, ErbFile templateFile) { exists(string templatesPath, string sourcePrefix, string subPath, string controllerPath | controllerPath = cls.getLocation().getFile().getRelativePath() and templatesPath = templateFile.getParentContainer().getRelativePath() and @@ -484,16 +516,14 @@ class ActionControllerSkipForgeryProtectionCall extends CsrfProtectionSetting::R /** * A call to `protect_from_forgery`. */ -private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetting::Range { - private ActionControllerContextCall callExpr; - +private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetting::Range, + DataFlow::CallNode { ActionControllerProtectFromForgeryCall() { - callExpr = this.asExpr().getExpr() and - callExpr.getMethodName() = "protect_from_forgery" + this = actionControllerInstance().getAMethodCall("protect_from_forgery") } private string getWithValueText() { - result = callExpr.getKeywordArgument("with").getConstantValue().getSymbol() + result = getKeywordArgument("with").getConstantValue().getSymbol() } // Calls without `with: :exception` can allow for bypassing CSRF protection @@ -508,11 +538,7 @@ private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetti */ private class SendFile extends FileSystemAccess::Range, DataFlow::CallNode { SendFile() { - this.getMethodName() = "send_file" and - ( - this.asExpr().getExpr() instanceof ActionControllerContextCall or - this.getReceiver().asExpr().getExpr() instanceof Response::ResponseCall - ) + this = [actionControllerInstance(), Response::response()].getAMethodCall("send_file") } override DataFlow::Node getAPathArgument() { result = this.getArgument(0) } @@ -522,21 +548,13 @@ private module ParamsSummaries { private import codeql.ruby.dataflow.FlowSummary /** - * An instance of `ActionController::Parameters`, including those returned + * Gets an instance of `ActionController::Parameters`, including those returned * from method calls on other instances. */ - private class ParamsInstance extends DataFlow::Node { - ParamsInstance() { - this.asExpr().getExpr() instanceof Rails::ParamsCall - or - this = - any(DataFlow::CallNode call | - call.getReceiver() instanceof ParamsInstance and - call.getMethodName() = paramsMethodReturningParamsInstance() - ) - or - exists(ParamsInstance prev | prev.(DataFlow::LocalSourceNode).flowsTo(this)) - } + private DataFlow::LocalSourceNode paramsInstance() { + result.asExpr().getExpr() instanceof Rails::ParamsCall + or + result = paramsInstance().getAMethodCall(paramsMethodReturningParamsInstance()) } /** @@ -578,8 +596,7 @@ private module ParamsSummaries { MethodsReturningParamsInstanceSummary() { this = "ActionController::Parameters#" } override MethodCall getACall() { - any(ParamsInstance i).asExpr().getExpr() = result.getReceiver() and - result.getMethodName() = methodReturnsTaintFromSelf() + result = paramsInstance().getAMethodCall(methodReturnsTaintFromSelf()).asExpr().getExpr() } override predicate propagatesFlowExt(string input, string output, boolean preservesValue) { @@ -601,9 +618,8 @@ private module ParamsSummaries { override MethodCall getACall() { result.getMethodName() = ["merge", "reverse_merge", "with_defaults"] and - exists(ParamsInstance i | - i.asExpr().getExpr() = [result.getReceiver(), result.getArgument(0)] - ) + paramsInstance().getALocalUse().asExpr().getExpr() = + [result.getReceiver(), result.getArgument(0)] } override predicate propagatesFlowExt(string input, string output, boolean preservesValue) { @@ -625,9 +641,8 @@ private module ParamsSummaries { override MethodCall getACall() { result.getMethodName() = ["merge!", "reverse_merge!", "with_defaults!"] and - exists(ParamsInstance i | - i.asExpr().getExpr() = [result.getReceiver(), result.getArgument(0)] - ) + paramsInstance().getALocalUse().asExpr().getExpr() = + [result.getReceiver(), result.getArgument(0)] } override predicate propagatesFlowExt(string input, string output, boolean preservesValue) { @@ -643,15 +658,12 @@ private module ParamsSummaries { * response. */ private module Response { - class ResponseCall extends ActionControllerContextCall { - ResponseCall() { this.getMethodName() = "response" } + DataFlow::LocalSourceNode response() { + result = actionControllerInstance().getAMethodCall("response") } class BodyWrite extends DataFlow::CallNode, Http::Server::HttpResponse::Range { - BodyWrite() { - this.getReceiver().asExpr().getExpr() instanceof ResponseCall and - this.getMethodName() = "body=" - } + BodyWrite() { this = response().getAMethodCall("body=") } override DataFlow::Node getBody() { result = this.getArgument(0) } @@ -661,10 +673,7 @@ private module Response { } class SendFileCall extends DataFlow::CallNode, Http::Server::HttpResponse::Range { - SendFileCall() { - this.getReceiver().asExpr().getExpr() instanceof ResponseCall and - this.getMethodName() = "send_file" - } + SendFileCall() { this = response().getAMethodCall("send_file") } override DataFlow::Node getBody() { result = this.getArgument(0) } @@ -677,19 +686,12 @@ private module Response { HeaderWrite() { // response.header[key] = val // response.headers[key] = val - exists(MethodCall headerCall | - headerCall.getMethodName() = ["header", "headers"] and - headerCall.getReceiver() instanceof ResponseCall - | - this.getReceiver().asExpr().getExpr() = headerCall and - this.getMethodName() = "[]=" - ) + this = response().getAMethodCall(["header", "headers"]).getAMethodCall("[]=") or // response.set_header(key) = val // response[header] = val // response.add_header(key, val) - this.getReceiver().asExpr().getExpr() instanceof ResponseCall and - this.getMethodName() = ["set_header", "[]=", "add_header"] + this = response().getAMethodCall(["set_header", "[]=", "add_header"]) } override string getName() { @@ -702,12 +704,12 @@ private module Response { class SpecificHeaderWrite extends DataFlow::CallNode, Http::Server::HeaderWriteAccess::Range { SpecificHeaderWrite() { // response. = val - this.getReceiver().asExpr().getExpr() instanceof ResponseCall and - this.getMethodName() = - [ - "location=", "cache_control=", "_cache_control=", "etag=", "charset=", "content_type=", - "date=", "last_modified=", "weak_etag=", "strong_etag=" - ] + this = + response() + .getAMethodCall([ + "location=", "cache_control=", "_cache_control=", "etag=", "charset=", + "content_type=", "date=", "last_modified=", "weak_etag=", "strong_etag=" + ]) } override string getName() { diff --git a/ruby/ql/src/experimental/weak-params/WeakParams.ql b/ruby/ql/src/experimental/weak-params/WeakParams.ql index 21f9e7bc6ce..cf18a37283f 100644 --- a/ruby/ql/src/experimental/weak-params/WeakParams.ql +++ b/ruby/ql/src/experimental/weak-params/WeakParams.ql @@ -17,18 +17,11 @@ import codeql.ruby.frameworks.ActionController import DataFlow::PathGraph /** - * A call to `request` in an ActionController controller class. + * Gets a call to `request` in an ActionController controller class. * This probably refers to the incoming HTTP request object. */ -class ActionControllerRequest extends DataFlow::Node { - ActionControllerRequest() { - exists(DataFlow::CallNode c | - c.asExpr().getExpr().getEnclosingModule() instanceof ActionControllerControllerClass and - c.getMethodName() = "request" - | - c.flowsTo(this) - ) - } +DataFlow::LocalSourceNode request() { + result = any(ActionControllerClass cls).getSelf().getAMethodCall("request") } /** @@ -36,9 +29,11 @@ class ActionControllerRequest extends DataFlow::Node { */ class WeakParams extends DataFlow::CallNode { WeakParams() { - this.getReceiver() instanceof ActionControllerRequest and - this.getMethodName() = - ["path_parameters", "query_parameters", "request_parameters", "GET", "POST"] + this = + request() + .getAMethodCall([ + "path_parameters", "query_parameters", "request_parameters", "GET", "POST" + ]) } } diff --git a/ruby/ql/test/library-tests/frameworks/ActionController.expected b/ruby/ql/test/library-tests/frameworks/ActionController.expected index 89f9f8b797e..3b67a085656 100644 --- a/ruby/ql/test/library-tests/frameworks/ActionController.expected +++ b/ruby/ql/test/library-tests/frameworks/ActionController.expected @@ -1,17 +1,17 @@ actionControllerControllerClasses | action_controller/input_access.rb:1:1:50:3 | UsersController | -| action_controller/params_flow.rb:1:1:151:3 | MyController | +| action_controller/params_flow.rb:1:1:153:3 | MyController | +| action_controller/params_flow.rb:161:1:169:3 | Subclass | | active_record/ActiveRecord.rb:23:1:39:3 | FooController | | active_record/ActiveRecord.rb:41:1:64:3 | BarController | | active_record/ActiveRecord.rb:66:1:98:3 | BazController | | active_record/ActiveRecord.rb:100:1:108:3 | AnnotatedController | -| active_storage/active_storage.rb:39:1:45:3 | PostsController | | app/controllers/comments_controller.rb:1:1:40:3 | CommentsController | | app/controllers/foo/bars_controller.rb:3:1:46:3 | BarsController | | app/controllers/photos_controller.rb:1:1:4:3 | PhotosController | | app/controllers/posts_controller.rb:1:1:10:3 | PostsController | | app/controllers/tags_controller.rb:1:1:2:3 | TagsController | -| app/controllers/users/notifications_controller.rb:2:3:5:5 | NotificationsController | +| app/controllers/users/notifications_controller.rb:2:3:5:5 | Users::NotificationsController | actionControllerActionMethods | action_controller/input_access.rb:2:3:49:5 | index | | action_controller/params_flow.rb:2:3:4:5 | m1 | @@ -47,6 +47,8 @@ actionControllerActionMethods | action_controller/params_flow.rb:125:3:132:5 | m30 | | action_controller/params_flow.rb:134:3:141:5 | m31 | | action_controller/params_flow.rb:143:3:150:5 | m32 | +| action_controller/params_flow.rb:156:3:158:5 | m33 | +| action_controller/params_flow.rb:162:3:164:5 | m34 | | active_record/ActiveRecord.rb:27:3:38:5 | some_request_handler | | active_record/ActiveRecord.rb:42:3:47:5 | some_other_request_handler | | active_record/ActiveRecord.rb:49:3:63:5 | safe_paths | @@ -117,6 +119,9 @@ paramsCalls | action_controller/params_flow.rb:144:10:144:15 | call to params | | action_controller/params_flow.rb:145:32:145:37 | call to params | | action_controller/params_flow.rb:148:22:148:27 | call to params | +| action_controller/params_flow.rb:157:10:157:15 | call to params | +| action_controller/params_flow.rb:163:10:163:15 | call to params | +| action_controller/params_flow.rb:167:10:167:15 | call to params | | action_mailer/mailer.rb:3:10:3:15 | call to params | | active_record/ActiveRecord.rb:28:30:28:35 | call to params | | active_record/ActiveRecord.rb:29:29:29:34 | call to params | @@ -192,6 +197,9 @@ paramsSources | action_controller/params_flow.rb:144:10:144:15 | call to params | | action_controller/params_flow.rb:145:32:145:37 | call to params | | action_controller/params_flow.rb:148:22:148:27 | call to params | +| action_controller/params_flow.rb:157:10:157:15 | call to params | +| action_controller/params_flow.rb:163:10:163:15 | call to params | +| action_controller/params_flow.rb:167:10:167:15 | call to params | | action_mailer/mailer.rb:3:10:3:15 | call to params | | active_record/ActiveRecord.rb:28:30:28:35 | call to params | | active_record/ActiveRecord.rb:29:29:29:34 | call to params | @@ -306,6 +314,9 @@ httpInputAccesses | action_controller/params_flow.rb:144:10:144:15 | call to params | ActionController::Metal#params | | action_controller/params_flow.rb:145:32:145:37 | call to params | ActionController::Metal#params | | action_controller/params_flow.rb:148:22:148:27 | call to params | ActionController::Metal#params | +| action_controller/params_flow.rb:157:10:157:15 | call to params | ActionController::Metal#params | +| action_controller/params_flow.rb:163:10:163:15 | call to params | ActionController::Metal#params | +| action_controller/params_flow.rb:167:10:167:15 | call to params | ActionController::Metal#params | | action_mailer/mailer.rb:3:10:3:15 | call to params | ActionController::Metal#params | | active_record/ActiveRecord.rb:28:30:28:35 | call to params | ActionController::Metal#params | | active_record/ActiveRecord.rb:29:29:29:34 | call to params | ActionController::Metal#params | diff --git a/ruby/ql/test/library-tests/frameworks/ActionController.ql b/ruby/ql/test/library-tests/frameworks/ActionController.ql index dca683dc6ab..406c95d3194 100644 --- a/ruby/ql/test/library-tests/frameworks/ActionController.ql +++ b/ruby/ql/test/library-tests/frameworks/ActionController.ql @@ -5,7 +5,7 @@ private import codeql.ruby.frameworks.ActionView private import codeql.ruby.Concepts private import codeql.ruby.DataFlow -query predicate actionControllerControllerClasses(ActionControllerControllerClass cls) { any() } +query predicate actionControllerControllerClasses(ActionControllerClass cls) { any() } query predicate actionControllerActionMethods(ActionControllerActionMethod m) { any() } @@ -25,11 +25,11 @@ query predicate redirectToCalls(RedirectToCall c) { any() } query predicate actionControllerHelperMethods(ActionControllerHelperMethod m) { any() } -query predicate getAssociatedControllerClasses(ActionControllerControllerClass cls, ErbFile f) { +query predicate getAssociatedControllerClasses(ActionControllerClass cls, ErbFile f) { cls = getAssociatedControllerClass(f) } -query predicate controllerTemplateFiles(ActionControllerControllerClass cls, ErbFile templateFile) { +query predicate controllerTemplateFiles(ActionControllerClass cls, ErbFile templateFile) { controllerTemplateFile(cls, templateFile) } diff --git a/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected b/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected index d06e3862b30..1970434bb2a 100644 --- a/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected +++ b/ruby/ql/test/library-tests/frameworks/action_controller/params-flow.expected @@ -44,6 +44,9 @@ edges | params_flow.rb:145:32:145:37 | call to params : | params_flow.rb:145:10:145:38 | call to with_defaults! | | params_flow.rb:148:5:148:5 | [post] p : | params_flow.rb:149:10:149:10 | p | | params_flow.rb:148:22:148:27 | call to params : | params_flow.rb:148:5:148:5 | [post] p : | +| params_flow.rb:157:10:157:15 | call to params : | params_flow.rb:157:10:157:19 | ...[...] | +| params_flow.rb:163:10:163:15 | call to params : | params_flow.rb:163:10:163:19 | ...[...] | +| params_flow.rb:167:10:167:15 | call to params : | params_flow.rb:167:10:167:19 | ...[...] | nodes | params_flow.rb:3:10:3:15 | call to params : | semmle.label | call to params : | | params_flow.rb:3:10:3:19 | ...[...] | semmle.label | ...[...] | @@ -130,6 +133,12 @@ nodes | params_flow.rb:148:5:148:5 | [post] p : | semmle.label | [post] p : | | params_flow.rb:148:22:148:27 | call to params : | semmle.label | call to params : | | params_flow.rb:149:10:149:10 | p | semmle.label | p | +| params_flow.rb:157:10:157:15 | call to params : | semmle.label | call to params : | +| params_flow.rb:157:10:157:19 | ...[...] | semmle.label | ...[...] | +| params_flow.rb:163:10:163:15 | call to params : | semmle.label | call to params : | +| params_flow.rb:163:10:163:19 | ...[...] | semmle.label | ...[...] | +| params_flow.rb:167:10:167:15 | call to params : | semmle.label | call to params : | +| params_flow.rb:167:10:167:19 | ...[...] | semmle.label | ...[...] | subpaths #select | params_flow.rb:3:10:3:19 | ...[...] | params_flow.rb:3:10:3:15 | call to params : | params_flow.rb:3:10:3:19 | ...[...] | $@ | params_flow.rb:3:10:3:15 | call to params : | call to params : | @@ -173,3 +182,6 @@ subpaths | params_flow.rb:144:10:144:44 | call to with_defaults! | params_flow.rb:144:10:144:15 | call to params : | params_flow.rb:144:10:144:44 | call to with_defaults! | $@ | params_flow.rb:144:10:144:15 | call to params : | call to params : | | params_flow.rb:145:10:145:38 | call to with_defaults! | params_flow.rb:145:32:145:37 | call to params : | params_flow.rb:145:10:145:38 | call to with_defaults! | $@ | params_flow.rb:145:32:145:37 | call to params : | call to params : | | params_flow.rb:149:10:149:10 | p | params_flow.rb:148:22:148:27 | call to params : | params_flow.rb:149:10:149:10 | p | $@ | params_flow.rb:148:22:148:27 | call to params : | call to params : | +| params_flow.rb:157:10:157:19 | ...[...] | params_flow.rb:157:10:157:15 | call to params : | params_flow.rb:157:10:157:19 | ...[...] | $@ | params_flow.rb:157:10:157:15 | call to params : | call to params : | +| params_flow.rb:163:10:163:19 | ...[...] | params_flow.rb:163:10:163:15 | call to params : | params_flow.rb:163:10:163:19 | ...[...] | $@ | params_flow.rb:163:10:163:15 | call to params : | call to params : | +| params_flow.rb:167:10:167:19 | ...[...] | params_flow.rb:167:10:167:15 | call to params : | params_flow.rb:167:10:167:19 | ...[...] | $@ | params_flow.rb:167:10:167:15 | call to params : | call to params : | diff --git a/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb b/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb index 7b4dc634720..20098a556c3 100644 --- a/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb +++ b/ruby/ql/test/library-tests/frameworks/action_controller/params_flow.rb @@ -148,4 +148,22 @@ class MyController < ActionController::Base p.with_defaults!(params) sink p # $hasTaintFlow end + + include Mixin +end + +module Mixin + def m33 + sink params[:x] # $hasTaintFlow + end +end + +class Subclass < MyController + def m34 + sink params[:x] # $hasTaintFlow + end + + rescue_from 'Foo::Bar' do |err| + sink params[:x] # $hasTaintFlow + end end