Merge pull request #7663 from github/hmac/api-graph-subclass

Ruby: Add basic subclassing support to API Graphs
This commit is contained in:
Harry Maclean
2022-02-04 10:19:07 +13:00
committed by GitHub
15 changed files with 256 additions and 135 deletions

View File

@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
*/
abstract predicate hasActualResult(Location location, string element, string tag, string value);
/**
* Like `hasActualResult`, but returns results that do not require a matching annotation.
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
* Override this predicate to specify optional results.
*/
predicate hasOptionalResult(Location location, string element, string tag, string value) {
none()
}
final predicate hasFailureMessage(FailureLocatable element, string message) {
exists(ActualResult actualResult |
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
)
or
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
message = "Unexpected result: " + actualResult.getExpectationText()
message = "Unexpected result: " + actualResult.getExpectationText() and
not actualResult.isOptional()
)
)
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
private newtype TFailureLocatable =
TActualResult(
InlineExpectationsTest test, Location location, string element, string tag, string value
InlineExpectationsTest test, Location location, string element, string tag, string value,
boolean optional
) {
test.hasActualResult(location, element, tag, value)
test.hasActualResult(location, element, tag, value) and
optional = false
or
test.hasOptionalResult(location, element, tag, value) and optional = true
} or
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
string element;
string tag;
string value;
boolean optional;
ActualResult() { this = TActualResult(test, location, element, tag, value) }
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
override string toString() { result = element }
@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
override string getTag() { result = tag }
override string getValue() { result = value }
predicate isOptional() { optional = true }
}
abstract private class Expectation extends FailureLocatable {

View File

@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
*/
abstract predicate hasActualResult(Location location, string element, string tag, string value);
/**
* Like `hasActualResult`, but returns results that do not require a matching annotation.
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
* Override this predicate to specify optional results.
*/
predicate hasOptionalResult(Location location, string element, string tag, string value) {
none()
}
final predicate hasFailureMessage(FailureLocatable element, string message) {
exists(ActualResult actualResult |
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
)
or
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
message = "Unexpected result: " + actualResult.getExpectationText()
message = "Unexpected result: " + actualResult.getExpectationText() and
not actualResult.isOptional()
)
)
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
private newtype TFailureLocatable =
TActualResult(
InlineExpectationsTest test, Location location, string element, string tag, string value
InlineExpectationsTest test, Location location, string element, string tag, string value,
boolean optional
) {
test.hasActualResult(location, element, tag, value)
test.hasActualResult(location, element, tag, value) and
optional = false
or
test.hasOptionalResult(location, element, tag, value) and optional = true
} or
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
string element;
string tag;
string value;
boolean optional;
ActualResult() { this = TActualResult(test, location, element, tag, value) }
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
override string toString() { result = element }
@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
override string getTag() { result = tag }
override string getValue() { result = value }
predicate isOptional() { optional = true }
}
abstract private class Expectation extends FailureLocatable {

View File

@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
*/
abstract predicate hasActualResult(Location location, string element, string tag, string value);
/**
* Like `hasActualResult`, but returns results that do not require a matching annotation.
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
* Override this predicate to specify optional results.
*/
predicate hasOptionalResult(Location location, string element, string tag, string value) {
none()
}
final predicate hasFailureMessage(FailureLocatable element, string message) {
exists(ActualResult actualResult |
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
)
or
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
message = "Unexpected result: " + actualResult.getExpectationText()
message = "Unexpected result: " + actualResult.getExpectationText() and
not actualResult.isOptional()
)
)
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
private newtype TFailureLocatable =
TActualResult(
InlineExpectationsTest test, Location location, string element, string tag, string value
InlineExpectationsTest test, Location location, string element, string tag, string value,
boolean optional
) {
test.hasActualResult(location, element, tag, value)
test.hasActualResult(location, element, tag, value) and
optional = false
or
test.hasOptionalResult(location, element, tag, value) and optional = true
} or
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
string element;
string tag;
string value;
boolean optional;
ActualResult() { this = TActualResult(test, location, element, tag, value) }
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
override string toString() { result = element }
@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
override string getTag() { result = tag }
override string getValue() { result = value }
predicate isOptional() { optional = true }
}
abstract private class Expectation extends FailureLocatable {

View File

@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
*/
abstract predicate hasActualResult(Location location, string element, string tag, string value);
/**
* Like `hasActualResult`, but returns results that do not require a matching annotation.
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
* Override this predicate to specify optional results.
*/
predicate hasOptionalResult(Location location, string element, string tag, string value) {
none()
}
final predicate hasFailureMessage(FailureLocatable element, string message) {
exists(ActualResult actualResult |
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
)
or
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
message = "Unexpected result: " + actualResult.getExpectationText()
message = "Unexpected result: " + actualResult.getExpectationText() and
not actualResult.isOptional()
)
)
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
private newtype TFailureLocatable =
TActualResult(
InlineExpectationsTest test, Location location, string element, string tag, string value
InlineExpectationsTest test, Location location, string element, string tag, string value,
boolean optional
) {
test.hasActualResult(location, element, tag, value)
test.hasActualResult(location, element, tag, value) and
optional = false
or
test.hasOptionalResult(location, element, tag, value) and optional = true
} or
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
string element;
string tag;
string value;
boolean optional;
ActualResult() { this = TActualResult(test, location, element, tag, value) }
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
override string toString() { result = element }
@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
override string getTag() { result = tag }
override string getValue() { result = value }
predicate isOptional() { optional = true }
}
abstract private class Expectation extends FailureLocatable {

View File

@@ -88,12 +88,14 @@ module API {
* This predicate may have multiple results when there are multiple constructor calls invoking this API component.
* Consider using `getAnInstantiation()` if there is a need to distinguish between individual constructor calls.
*/
Node getInstance() { result = this.getASuccessor(Label::instance()) }
Node getInstance() { result = this.getASubclass().getASuccessor(Label::instance()) }
/**
* Gets a node representing the result of calling a method on the receiver represented by this node.
*/
Node getReturn(string method) { result = this.getASuccessor(Label::return(method)) }
Node getReturn(string method) {
result = this.getASubclass().getASuccessor(Label::return(method))
}
/**
* Gets a `new` call to the function represented by this API component.
@@ -101,9 +103,26 @@ module API {
DataFlow::ExprNode getAnInstantiation() { result = this.getInstance().getAnImmediateUse() }
/**
* Gets a node representing a subclass of the class represented by this node.
* Gets a node representing a (direct or indirect) subclass of the class represented by this node.
* ```rb
* class A; end
* class B < A; end
* class C < B; end
* ```
* In the example above, `getMember("A").getASubclass()` will return uses of `A`, `B` and `C`.
*/
Node getASubclass() { result = this.getASuccessor(Label::subclass()) }
Node getASubclass() { result = this.getAnImmediateSubclass*() }
/**
* Gets a node representing a direct subclass of the class represented by this node.
* ```rb
* class A; end
* class B < A; end
* class C < B; end
* ```
* In the example above, `getMember("A").getAnImmediateSubclass()` will return uses of `B` only.
*/
Node getAnImmediateSubclass() { result = this.getASuccessor(Label::subclass()) }
/**
* Gets a string representation of the lexicographically least among all shortest access paths
@@ -254,10 +273,9 @@ module API {
*/
pragma[nomagic]
private predicate useRoot(string lbl, DataFlow::Node ref) {
exists(string name, ExprNodes::ConstantAccessCfgNode access, ConstantReadAccess read |
access = ref.asExpr() and
lbl = Label::member(read.getName()) and
read = access.getExpr()
exists(string name, ConstantReadAccess read |
read = ref.asExpr().getExpr() and
lbl = Label::member(read.getName())
|
name = resolveTopLevel(read)
or
@@ -389,6 +407,17 @@ module API {
useStep(lbl, node, ref)
)
)
or
// `pred` is a use of class A
// `succ` is a use of class B
// there exists a class declaration B < A
exists(ClassDeclaration c, DataFlow::Node a, DataFlow::Node b |
use(pred, a) and
use(succ, b) and
resolveConstant(b.asExpr().getExpr()) = resolveConstantWriteAccess(c) and
c.getSuperclassExpr() = a.asExpr().getExpr() and
lbl = Label::subclass()
)
}
/**

View File

@@ -233,12 +233,17 @@ private module ResolveImpl {
pragma[nomagic]
private string resolveConstantReadAccessNonRec(ConstantReadAccess c, int priority) {
// ::B
c.hasGlobalScope() and result = c.getName() and priority = 0
or
// A::B
exists(string name, string s | result = isDefinedConstantNonRec(s, name) |
s = resolveConstantReadAccessScopeNonRec(c, priority, name)
)
or
// module A
// B
// end
exists(string name |
exists(Namespace n, string qname |
n = constantReadAccessEnclosingNameSpace(c, priority, name) and

View File

@@ -4,22 +4,9 @@ private import codeql.ruby.controlflow.CfgNodes
private import codeql.ruby.DataFlow
private import codeql.ruby.dataflow.RemoteFlowSources
private import codeql.ruby.ast.internal.Module
private import codeql.ruby.ApiGraphs
private import ActionView
private class ActionControllerBaseAccess extends ConstantReadAccess {
ActionControllerBaseAccess() {
this.getName() = "Base" and
this.getScopeExpr().(ConstantAccess).getName() = "ActionController"
}
}
// ApplicationController extends ActionController::Base, but we
// treat it separately in case the ApplicationController definition
// is not in the database
private class ApplicationControllerAccess extends ConstantReadAccess {
ApplicationControllerAccess() { this.getName() = "ApplicationController" }
}
/**
* A `ClassDeclaration` for a class that extends `ActionController::Base`.
* For example,
@@ -35,16 +22,13 @@ private class ApplicationControllerAccess extends ConstantReadAccess {
*/
class ActionControllerControllerClass extends ClassDeclaration {
ActionControllerControllerClass() {
// class FooController < ActionController::Base
this.getSuperclassExpr() instanceof ActionControllerBaseAccess
or
// class FooController < ApplicationController
this.getSuperclassExpr() instanceof ApplicationControllerAccess
or
// class BarController < FooController
exists(ActionControllerControllerClass other |
other.getModule() = resolveConstantReadAccess(this.getSuperclassExpr())
)
this.getSuperclassExpr() =
[
API::getTopLevelMember("ActionController").getMember("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")
].getASubclass().getAUse().asExpr().getExpr()
}
/**

View File

@@ -7,20 +7,6 @@ private import codeql.ruby.ast.internal.Module
private import codeql.ruby.ApiGraphs
private import codeql.ruby.frameworks.StandardLibrary
private class ActiveRecordBaseAccess extends ConstantReadAccess {
ActiveRecordBaseAccess() {
this.getName() = "Base" and
this.getScopeExpr().(ConstantAccess).getName() = "ActiveRecord"
}
}
// ApplicationRecord extends ActiveRecord::Base, but we
// treat it separately in case the ApplicationRecord definition
// is not in the database
private class ApplicationRecordAccess extends ConstantReadAccess {
ApplicationRecordAccess() { this.getName() = "ApplicationRecord" }
}
/// See https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html
private string activeRecordPersistenceInstanceMethodName() {
result =
@@ -41,26 +27,28 @@ private predicate isBuiltInMethodForActiveRecordModelInstance(string methodName)
}
/**
* A `ClassDeclaration` for a class that extends `ActiveRecord::Base`. For example,
* A `ClassDeclaration` for a class that inherits from `ActiveRecord::Base`. For example,
*
* ```rb
* class UserGroup < ActiveRecord::Base
* has_many :users
* end
*
* class SpecialUserGroup < UserGroup
* end
* ```
*/
class ActiveRecordModelClass extends ClassDeclaration {
ActiveRecordModelClass() {
// class Foo < ActiveRecord::Base
this.getSuperclassExpr() instanceof ActiveRecordBaseAccess
or
// class Foo < ApplicationRecord
this.getSuperclassExpr() instanceof ApplicationRecordAccess
or
// class Bar < Foo
exists(ActiveRecordModelClass other |
other.getModule() = resolveConstantReadAccess(this.getSuperclassExpr())
)
this.getSuperclassExpr() =
[
API::getTopLevelMember("ActiveRecord").getMember("Base"),
// In Rails applications `ApplicationRecord` typically extends `ActiveRecord::Base`, but we
// treat it separately in case the `ApplicationRecord` definition is not in the database.
API::getTopLevelMember("ApplicationRecord")
].getASubclass().getAUse().asExpr().getExpr()
}
// Gets the class declaration for this class and all of its super classes

View File

@@ -10,44 +10,7 @@ private import codeql.ruby.dataflow.RemoteFlowSources
private import codeql.ruby.ast.internal.Module
private import codeql.ruby.ApiGraphs
private class GraphqlRelayClassicMutationAccess extends ConstantReadAccess {
//GraphQL::Schema::RelayClassicMutation
GraphqlRelayClassicMutationAccess() {
this =
API::getTopLevelMember("GraphQL")
.getMember("Schema")
.getMember("RelayClassicMutation")
.getAUse()
.asExpr()
.getExpr()
}
}
private class GraphqlSchemaResolverAccess extends ConstantReadAccess {
//GraphQL::Schema::Resolver
GraphqlSchemaResolverAccess() {
this =
API::getTopLevelMember("GraphQL")
.getMember("Schema")
.getMember("Resolver")
.getAUse()
.asExpr()
.getExpr()
}
}
private class GraphqlSchemaObjectAccess extends ConstantReadAccess {
//GraphQL::Schema::Object
GraphqlSchemaObjectAccess() {
this =
API::getTopLevelMember("GraphQL")
.getMember("Schema")
.getMember("Object")
.getAUse()
.asExpr()
.getExpr()
}
}
private API::Node graphQlSchema() { result = API::getTopLevelMember("GraphQL").getMember("Schema") }
/**
* A `ClassDeclaration` for a class that extends `GraphQL::Schema::RelayClassicMutation`.
@@ -77,13 +40,8 @@ private class GraphqlSchemaObjectAccess extends ConstantReadAccess {
*/
private class GraphqlRelayClassicMutationClass extends ClassDeclaration {
GraphqlRelayClassicMutationClass() {
// class BaseMutation < GraphQL::Schema::RelayClassicMutation
this.getSuperclassExpr() instanceof GraphqlRelayClassicMutationAccess
or
// class MyMutation < BaseMutation
exists(GraphqlRelayClassicMutationClass other |
other.getModule() = resolveConstantReadAccess(this.getSuperclassExpr())
)
this.getSuperclassExpr() =
graphQlSchema().getMember("RelayClassicMutation").getASubclass*().getAUse().asExpr().getExpr()
}
}
@@ -112,13 +70,8 @@ private class GraphqlRelayClassicMutationClass extends ClassDeclaration {
*/
private class GraphqlSchemaResolverClass extends ClassDeclaration {
GraphqlSchemaResolverClass() {
// class BaseResolver < GraphQL::Schema::Resolver
this.getSuperclassExpr() instanceof GraphqlSchemaResolverAccess
or
// class MyResolver < BaseResolver
exists(GraphqlSchemaResolverClass other |
other.getModule() = resolveConstantReadAccess(this.getSuperclassExpr())
)
this.getSuperclassExpr() =
graphQlSchema().getMember("Resolver").getASubclass().getAUse().asExpr().getExpr()
}
}
@@ -138,13 +91,8 @@ private class GraphqlSchemaResolverClass extends ClassDeclaration {
*/
class GraphqlSchemaObjectClass extends ClassDeclaration {
GraphqlSchemaObjectClass() {
// class BaseObject < GraphQL::Schema::Object
this.getSuperclassExpr() instanceof GraphqlSchemaObjectAccess
or
// class MyObject < BaseObject
exists(GraphqlSchemaObjectClass other |
other.getModule() = resolveConstantReadAccess(this.getSuperclassExpr())
)
this.getSuperclassExpr() =
graphQlSchema().getMember("Object").getASubclass().getAUse().asExpr().getExpr()
}
/** Gets a `GraphqlFieldDefinitionMethodCall` called in this class. */

View File

@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
*/
abstract predicate hasActualResult(Location location, string element, string tag, string value);
/**
* Like `hasActualResult`, but returns results that do not require a matching annotation.
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
* Override this predicate to specify optional results.
*/
predicate hasOptionalResult(Location location, string element, string tag, string value) {
none()
}
final predicate hasFailureMessage(FailureLocatable element, string message) {
exists(ActualResult actualResult |
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
)
or
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
message = "Unexpected result: " + actualResult.getExpectationText()
message = "Unexpected result: " + actualResult.getExpectationText() and
not actualResult.isOptional()
)
)
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
private newtype TFailureLocatable =
TActualResult(
InlineExpectationsTest test, Location location, string element, string tag, string value
InlineExpectationsTest test, Location location, string element, string tag, string value,
boolean optional
) {
test.hasActualResult(location, element, tag, value)
test.hasActualResult(location, element, tag, value) and
optional = false
or
test.hasOptionalResult(location, element, tag, value) and optional = true
} or
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
string element;
string tag;
string value;
boolean optional;
ActualResult() { this = TActualResult(test, location, element, tag, value) }
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
override string toString() { result = element }
@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
override string getTag() { result = tag }
override string getValue() { result = value }
predicate isOptional() { optional = true }
}
abstract private class Expectation extends FailureLocatable {

View File

@@ -0,0 +1,6 @@
classMethodCalls
| test1.rb:58:1:58:8 | Use getMember("M1").getMember("C1").getReturn("m") |
| test1.rb:59:1:59:8 | Use getMember("M2").getMember("C3").getReturn("m") |
instanceMethodCalls
| test1.rb:61:1:61:12 | Use getMember("M1").getMember("C1").instance.getReturn("m") |
| test1.rb:62:1:62:12 | Use getMember("M2").getMember("C3").instance.getReturn("m") |

View File

@@ -0,0 +1,13 @@
/**
* Tests of the public API of API Graphs
*/
import codeql.ruby.ApiGraphs
query predicate classMethodCalls(API::Node node) {
node = API::getTopLevelMember("M1").getMember("C1").getReturn("m")
}
query predicate instanceMethodCalls(API::Node node) {
node = API::getTopLevelMember("M1").getMember("C1").getInstance().getReturn("m")
}

View File

@@ -29,3 +29,34 @@ module Outer
end
Outer::Inner.foo #$ use=getMember("Outer").getMember("Inner").getReturn("foo")
module M1
class C1
def self.m
end
def m
end
end
end
class C2 < M1::C1 #$ use=getMember("M1").getMember("C1")
end
module M2
class C3 < M1::C1 #$ use=getMember("M1").getMember("C1")
end
class C4 < C2 #$ use=getMember("C2")
end
end
C2 #$ use=getMember("C2") use=getMember("M1").getMember("C1").getASubclass()
M2::C3 #$ use=getMember("M2").getMember("C3") use=getMember("M1").getMember("C1").getASubclass()
M2::C4 #$ use=getMember("M2").getMember("C4") use=getMember("C2").getASubclass() use=getMember("M1").getMember("C1").getASubclass().getASubclass()
M1::C1.m #$ use=getMember("M1").getMember("C1").getReturn("m")
M2::C3.m #$ use=getMember("M2").getMember("C3").getReturn("m") use=getMember("M1").getMember("C1").getASubclass().getReturn("m")
M1::C1.new.m #$ use=getMember("M1").getMember("C1").instance.getReturn("m")
M2::C3.new.m #$ use=getMember("M2").getMember("C3").instance.getReturn("m")

View File

@@ -30,6 +30,38 @@ class ApiUseTest extends InlineExpectationsTest {
element = n.toString()
)
}
// We also permit optional annotations for any other path on the line.
// This is used to test subclass paths, which typically have a shorter canonical path.
override predicate hasOptionalResult(Location location, string element, string tag, string value) {
exists(API::Node a, DataFlow::Node n | relevantNode(a, n, location) |
tag = "use" and
element = n.toString() and
value = getAPath(a, _)
)
}
}
private int size(AstNode n) { not n instanceof StmtSequence and result = count(n.getAChild*()) }
/**
* Gets a path of the given `length` from the root to the given node.
* This is a copy of `API::getAPath()` without the restriction on path length,
* which would otherwise rule out paths involving `getASubclass()`.
*/
string getAPath(API::Node node, int length) {
node instanceof API::Root and
length = 0 and
result = ""
or
exists(API::Node pred, string lbl, string predpath |
pred.getASuccessor(lbl) = node and
lbl != "" and
predpath = getAPath(pred, length - 1) and
exists(string dot | if length = 1 then dot = "" else dot = "." |
result = predpath + dot + lbl and
// avoid producing strings longer than 1MB
result.length() < 1000 * 1000
)
)
}

View File

@@ -93,4 +93,4 @@ class BazController < BarController
def yet_another_handler
Admin.delete_by(params[:admin_condition])
end
end
end