diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveResource.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveResource.qll index 96219915770..9f0e0f4b859 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/ActiveResource.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/ActiveResource.qll @@ -18,8 +18,12 @@ module ActiveResource { * An ActiveResource model class. This is any (transitive) subclass of ActiveResource. */ pragma[nomagic] - private API::Node modelApiNode() { - result = API::getTopLevelMember("ActiveResource").getMember("Base").getASubclass() + private API::Node activeResourceBaseClass() { + result = API::getTopLevelMember("ActiveResource").getMember("Base") + } + + private DataFlow::ClassNode activeResourceClass() { + result = activeResourceBaseClass().getADescendentModule() } /** @@ -30,16 +34,8 @@ module ActiveResource { * end * ``` */ - class ModelClass extends ClassDeclaration { - API::Node model; - - ModelClass() { - model = modelApiNode() and - this.getSuperclassExpr() = model.getAValueReachableFromSource().asExpr().getExpr() - } - - /** Gets the API node for this model */ - API::Node getModelApiNode() { result = model } + class ModelClassNode extends DataFlow::ClassNode { + ModelClassNode() { this = activeResourceClass() } /** Gets a call to `site=`, which sets the base URL for this model. */ SiteAssignCall getASiteAssignment() { result.getModelClass() = this } @@ -49,6 +45,46 @@ module ActiveResource { c = this.getASiteAssignment() and c.disablesCertificateValidation() } + + /** Gets a method call on this class that returns an instance of the class. */ + private DataFlow::CallNode getAChainedCall() { + result.(FindCall).getModelClass() = this + or + result.(CreateCall).getModelClass() = this + or + result.(CustomHttpCall).getModelClass() = this + or + result.(CollectionCall).getCollection().getModelClass() = this and + result.getMethodName() = ["first", "last"] + } + + /** Gets an API node referring to an instance of this class. */ + API::Node getAnInstanceReference() { + result = this.trackInstance() + or + result = this.getAChainedCall().track() + } + } + + /** DEPRECATED. Use `ModelClassNode` instead. */ + deprecated class ModelClass extends ClassDeclaration { + private ModelClassNode cls; + + ModelClass() { this = cls.getADeclaration() } + + /** Gets the class for which this is a declaration. */ + ModelClassNode getClassNode() { result = cls } + + /** Gets the API node for this class object. */ + deprecated API::Node getModelApiNode() { result = cls.trackModule() } + + /** Gets a call to `site=`, which sets the base URL for this model. */ + SiteAssignCall getASiteAssignment() { result = cls.getASiteAssignment() } + + /** Holds if `c` sets a base URL which does not use HTTPS. */ + predicate disablesCertificateValidation(SiteAssignCall c) { + cls.disablesCertificateValidation(c) + } } /** @@ -62,25 +98,20 @@ module ActiveResource { * ``` */ class ModelClassMethodCall extends DataFlow::CallNode { - API::Node model; + private ModelClassNode cls; - ModelClassMethodCall() { - model = modelApiNode() and - this = classMethodCall(model, _) - } + ModelClassMethodCall() { this = cls.trackModule().getAMethodCall(_) } /** Gets the model class for this call. */ - ModelClass getModelClass() { result.getModelApiNode() = model } + ModelClassNode getModelClass() { result = cls } } /** * A call to `site=` on an ActiveResource model class. * This sets the base URL for all HTTP requests made by this class. */ - private class SiteAssignCall extends DataFlow::CallNode { - API::Node model; - - SiteAssignCall() { model = modelApiNode() and this = classMethodCall(model, "site=") } + private class SiteAssignCall extends ModelClassMethodCall { + SiteAssignCall() { this.getMethodName() = "site=" } /** * Gets a node that contributes to the URLs used for HTTP requests by the parent @@ -88,12 +119,10 @@ module ActiveResource { */ DataFlow::Node getAUrlPart() { result = this.getArgument(0) } - /** Gets the model class for this call. */ - ModelClass getModelClass() { result.getModelApiNode() = model } - /** Holds if this site value specifies HTTP rather than HTTPS. */ predicate disablesCertificateValidation() { this.getAUrlPart() + // TODO: We should not need all this just to get the string value .asExpr() .(ExprNodes::AssignExprCfgNode) .getRhs() @@ -141,87 +170,70 @@ module ActiveResource { } /** + * DEPRECATED. Use `ModelClassNode.getAnInstanceReference()` instead. + * * An ActiveResource model object. */ - class ModelInstance extends DataFlow::Node { - ModelClass cls; + deprecated class ModelInstance extends DataFlow::Node { + private ModelClassNode cls; - ModelInstance() { - exists(API::Node model | model = modelApiNode() | - this = model.getInstance().getAValueReachableFromSource() and - cls.getModelApiNode() = model - ) - or - exists(FindCall call | call.flowsTo(this) | cls = call.getModelClass()) - or - exists(CreateCall call | call.flowsTo(this) | cls = call.getModelClass()) - or - exists(CustomHttpCall call | call.flowsTo(this) | cls = call.getModelClass()) - or - exists(CollectionCall call | - call.getMethodName() = ["first", "last"] and - call.flowsTo(this) - | - cls = call.getCollection().getModelClass() - ) - } + ModelInstance() { this = cls.getAnInstanceReference().getAValueReachableFromSource() } /** Gets the model class for this instance. */ - ModelClass getModelClass() { result = cls } + ModelClassNode getModelClass() { result = cls } } /** * A call to a method on an ActiveResource model object. */ class ModelInstanceMethodCall extends DataFlow::CallNode { - ModelInstance i; + private ModelClassNode cls; - ModelInstanceMethodCall() { this.getReceiver() = i } + ModelInstanceMethodCall() { this = cls.getAnInstanceReference().getAMethodCall(_) } /** Gets the model instance for this call. */ - ModelInstance getInstance() { result = i } + deprecated ModelInstance getInstance() { result = this.getReceiver() } /** Gets the model class for this call. */ - ModelClass getModelClass() { result = i.getModelClass() } + ModelClassNode getModelClass() { result = cls } } /** - * A collection of ActiveResource model objects. + * DEPRECATED. Use `CollectionSource` instead. + * + * A data flow node that may refer to a collection of ActiveResource model objects. */ - class Collection extends DataFlow::Node { - ModelClassMethodCall classMethodCall; + deprecated class Collection extends DataFlow::Node { + Collection() { this = any(CollectionSource src).track().getAValueReachableFromSource() } + } - Collection() { - classMethodCall.flowsTo(this) and - ( - classMethodCall.getMethodName() = "all" - or - classMethodCall.getMethodName() = "find" and - classMethodCall.getArgument(0).asExpr().getConstantValue().isStringlikeValue("all") - ) + /** + * A call that returns a collection of ActiveResource model objects. + */ + class CollectionSource extends ModelClassMethodCall { + CollectionSource() { + this.getMethodName() = "all" + or + this.getArgument(0).asExpr().getConstantValue().isStringlikeValue("all") } - - /** Gets the model class for this collection. */ - ModelClass getModelClass() { result = classMethodCall.getModelClass() } } /** * A method call on a collection. */ class CollectionCall extends DataFlow::CallNode { - CollectionCall() { this.getReceiver() instanceof Collection } + private CollectionSource collection; + + CollectionCall() { this = collection.track().getAMethodCall(_) } /** Gets the collection for this call. */ - Collection getCollection() { result = this.getReceiver() } + CollectionSource getCollection() { result = collection } } private class ModelClassMethodCallAsHttpRequest extends Http::Client::Request::Range, ModelClassMethodCall { - ModelClass cls; - ModelClassMethodCallAsHttpRequest() { - this.getModelClass() = cls and this.getMethodName() = ["all", "build", "create", "create!", "find", "first", "last"] } @@ -230,12 +242,14 @@ module ActiveResource { override predicate disablesCertificateValidation( DataFlow::Node disablingNode, DataFlow::Node argumentOrigin ) { - cls.disablesCertificateValidation(disablingNode) and + this.getModelClass().disablesCertificateValidation(disablingNode) and // TODO: highlight real argument origin argumentOrigin = disablingNode } - override DataFlow::Node getAUrlPart() { result = cls.getASiteAssignment().getAUrlPart() } + override DataFlow::Node getAUrlPart() { + result = this.getModelClass().getASiteAssignment().getAUrlPart() + } override DataFlow::Node getResponseBody() { result = this } } @@ -243,10 +257,7 @@ module ActiveResource { private class ModelInstanceMethodCallAsHttpRequest extends Http::Client::Request::Range, ModelInstanceMethodCall { - ModelClass cls; - ModelInstanceMethodCallAsHttpRequest() { - this.getModelClass() = cls and this.getMethodName() = [ "exists?", "reload", "save", "save!", "destroy", "delete", "get", "patch", "post", "put", @@ -259,42 +270,15 @@ module ActiveResource { override predicate disablesCertificateValidation( DataFlow::Node disablingNode, DataFlow::Node argumentOrigin ) { - cls.disablesCertificateValidation(disablingNode) and + this.getModelClass().disablesCertificateValidation(disablingNode) and // TODO: highlight real argument origin argumentOrigin = disablingNode } - override DataFlow::Node getAUrlPart() { result = cls.getASiteAssignment().getAUrlPart() } + override DataFlow::Node getAUrlPart() { + result = this.getModelClass().getASiteAssignment().getAUrlPart() + } override DataFlow::Node getResponseBody() { result = this } } - - /** - * A call to a class method. - * - * TODO: is this general enough to be useful elsewhere? - * - * Examples: - * ```rb - * class A - * def self.m; end - * - * m # call - * end - * - * A.m # call - * ``` - */ - private DataFlow::CallNode classMethodCall(API::Node classNode, string methodName) { - // A.m - result = classNode.getAMethodCall(methodName) - or - // class A - // A.m - // end - result.getReceiver().asExpr() instanceof ExprNodes::SelfVariableAccessCfgNode and - result.asExpr().getExpr().getEnclosingModule().(ClassDeclaration).getSuperclassExpr() = - classNode.getAValueReachableFromSource().asExpr().getExpr() and - result.getMethodName() = methodName - } } diff --git a/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.expected b/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.expected index a55946c1852..e6d3b056971 100644 --- a/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.expected +++ b/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.expected @@ -33,6 +33,13 @@ modelInstances | active_resource.rb:26:9:26:14 | people | | active_resource.rb:26:9:26:20 | call to first | | active_resource.rb:27:1:27:5 | alice | +modelInstancesAsSource +| active_resource.rb:1:1:3:3 | Person | active_resource.rb:5:9:5:33 | call to new | +| active_resource.rb:1:1:3:3 | Person | active_resource.rb:8:9:8:22 | call to find | +| active_resource.rb:1:1:3:3 | Person | active_resource.rb:16:1:16:23 | call to new | +| active_resource.rb:1:1:3:3 | Person | active_resource.rb:18:1:18:22 | call to get | +| active_resource.rb:1:1:3:3 | Person | active_resource.rb:24:10:24:26 | call to find | +| active_resource.rb:1:1:3:3 | Person | active_resource.rb:26:9:26:20 | call to first | modelInstanceMethodCalls | active_resource.rb:6:1:6:10 | call to save | | active_resource.rb:9:1:9:13 | call to address= | @@ -50,3 +57,6 @@ collections | active_resource.rb:24:1:24:26 | ... = ... | | active_resource.rb:24:10:24:26 | call to find | | active_resource.rb:26:9:26:14 | people | +collectionSources +| active_resource.rb:23:10:23:19 | call to all | +| active_resource.rb:24:10:24:26 | call to find | diff --git a/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.ql b/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.ql index 1f2fd1efcf1..f1898ddbc98 100644 --- a/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.ql +++ b/ruby/ql/test/library-tests/frameworks/active_resource/ActiveResource.ql @@ -3,7 +3,8 @@ import codeql.ruby.DataFlow import codeql.ruby.frameworks.ActiveResource query predicate modelClasses( - ActiveResource::ModelClass c, DataFlow::Node siteAssignCall, boolean disablesCertificateValidation + ActiveResource::ModelClassNode c, DataFlow::Node siteAssignCall, + boolean disablesCertificateValidation ) { c.getASiteAssignment() = siteAssignCall and if c.disablesCertificateValidation(siteAssignCall) @@ -13,8 +14,16 @@ query predicate modelClasses( query predicate modelClassMethodCalls(ActiveResource::ModelClassMethodCall c) { any() } -query predicate modelInstances(ActiveResource::ModelInstance c) { any() } +deprecated query predicate modelInstances(ActiveResource::ModelInstance c) { any() } + +query predicate modelInstancesAsSource( + ActiveResource::ModelClassNode cls, DataFlow::LocalSourceNode node +) { + node = cls.getAnInstanceReference().asSource() +} query predicate modelInstanceMethodCalls(ActiveResource::ModelInstanceMethodCall c) { any() } -query predicate collections(ActiveResource::Collection c) { any() } +deprecated query predicate collections(ActiveResource::Collection c) { any() } + +query predicate collectionSources(ActiveResource::CollectionSource c) { any() }