Skip to content

Commit ab7fd89

Browse files
hmachmac
authored
Merge pull request github#7663 from github/hmac/api-graph-subclass
Ruby: Add basic subclassing support to API Graphs
2 parents e328c62 + 704b585 commit ab7fd89

File tree

15 files changed

+256
-135
lines changed

15 files changed

+256
-135
lines changed

‎cpp/ql/test/TestUtilities/InlineExpectationsTest.qll

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
123123
*/
124124
abstract predicate hasActualResult(Location location, string element, string tag, string value);
125125

126+
/**
127+
* Like `hasActualResult`, but returns results that do not require a matching annotation.
128+
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
129+
* Override this predicate to specify optional results.
130+
*/
131+
predicate hasOptionalResult(Location location, string element, string tag, string value) {
132+
none()
133+
}
134+
126135
final predicate hasFailureMessage(FailureLocatable element, string message) {
127136
exists(ActualResult actualResult |
128137
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
134143
)
135144
or
136145
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
137-
message = "Unexpected result: " + actualResult.getExpectationText()
146+
message = "Unexpected result: " + actualResult.getExpectationText() and
147+
not actualResult.isOptional()
138148
)
139149
)
140150
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
243253

244254
private newtype TFailureLocatable =
245255
TActualResult(
246-
InlineExpectationsTest test, Location location, string element, string tag, string value
256+
InlineExpectationsTest test, Location location, string element, string tag, string value,
257+
boolean optional
247258
) {
248-
test.hasActualResult(location, element, tag, value)
259+
test.hasActualResult(location, element, tag, value) and
260+
optional = false
261+
or
262+
test.hasOptionalResult(location, element, tag, value) and optional = true
249263
} or
250264
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
251265
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
277291
string element;
278292
string tag;
279293
string value;
294+
boolean optional;
280295

281-
ActualResult() { this = TActualResult(test, location, element, tag, value) }
296+
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
282297

283298
override string toString() { result = element }
284299

@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
289304
override string getTag() { result = tag }
290305

291306
override string getValue() { result = value }
307+
308+
predicate isOptional() { optional = true }
292309
}
293310

294311
abstract private class Expectation extends FailureLocatable {

‎csharp/ql/test/TestUtilities/InlineExpectationsTest.qll

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
123123
*/
124124
abstract predicate hasActualResult(Location location, string element, string tag, string value);
125125

126+
/**
127+
* Like `hasActualResult`, but returns results that do not require a matching annotation.
128+
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
129+
* Override this predicate to specify optional results.
130+
*/
131+
predicate hasOptionalResult(Location location, string element, string tag, string value) {
132+
none()
133+
}
134+
126135
final predicate hasFailureMessage(FailureLocatable element, string message) {
127136
exists(ActualResult actualResult |
128137
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
134143
)
135144
or
136145
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
137-
message = "Unexpected result: " + actualResult.getExpectationText()
146+
message = "Unexpected result: " + actualResult.getExpectationText() and
147+
not actualResult.isOptional()
138148
)
139149
)
140150
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
243253

244254
private newtype TFailureLocatable =
245255
TActualResult(
246-
InlineExpectationsTest test, Location location, string element, string tag, string value
256+
InlineExpectationsTest test, Location location, string element, string tag, string value,
257+
boolean optional
247258
) {
248-
test.hasActualResult(location, element, tag, value)
259+
test.hasActualResult(location, element, tag, value) and
260+
optional = false
261+
or
262+
test.hasOptionalResult(location, element, tag, value) and optional = true
249263
} or
250264
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
251265
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
277291
string element;
278292
string tag;
279293
string value;
294+
boolean optional;
280295

281-
ActualResult() { this = TActualResult(test, location, element, tag, value) }
296+
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
282297

283298
override string toString() { result = element }
284299

@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
289304
override string getTag() { result = tag }
290305

291306
override string getValue() { result = value }
307+
308+
predicate isOptional() { optional = true }
292309
}
293310

294311
abstract private class Expectation extends FailureLocatable {

‎java/ql/test/TestUtilities/InlineExpectationsTest.qll

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
123123
*/
124124
abstract predicate hasActualResult(Location location, string element, string tag, string value);
125125

126+
/**
127+
* Like `hasActualResult`, but returns results that do not require a matching annotation.
128+
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
129+
* Override this predicate to specify optional results.
130+
*/
131+
predicate hasOptionalResult(Location location, string element, string tag, string value) {
132+
none()
133+
}
134+
126135
final predicate hasFailureMessage(FailureLocatable element, string message) {
127136
exists(ActualResult actualResult |
128137
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
134143
)
135144
or
136145
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
137-
message = "Unexpected result: " + actualResult.getExpectationText()
146+
message = "Unexpected result: " + actualResult.getExpectationText() and
147+
not actualResult.isOptional()
138148
)
139149
)
140150
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
243253

244254
private newtype TFailureLocatable =
245255
TActualResult(
246-
InlineExpectationsTest test, Location location, string element, string tag, string value
256+
InlineExpectationsTest test, Location location, string element, string tag, string value,
257+
boolean optional
247258
) {
248-
test.hasActualResult(location, element, tag, value)
259+
test.hasActualResult(location, element, tag, value) and
260+
optional = false
261+
or
262+
test.hasOptionalResult(location, element, tag, value) and optional = true
249263
} or
250264
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
251265
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
277291
string element;
278292
string tag;
279293
string value;
294+
boolean optional;
280295

281-
ActualResult() { this = TActualResult(test, location, element, tag, value) }
296+
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
282297

283298
override string toString() { result = element }
284299

@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
289304
override string getTag() { result = tag }
290305

291306
override string getValue() { result = value }
307+
308+
predicate isOptional() { optional = true }
292309
}
293310

294311
abstract private class Expectation extends FailureLocatable {

‎python/ql/test/TestUtilities/InlineExpectationsTest.qll

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,15 @@ abstract class InlineExpectationsTest extends string {
123123
*/
124124
abstract predicate hasActualResult(Location location, string element, string tag, string value);
125125

126+
/**
127+
* Like `hasActualResult`, but returns results that do not require a matching annotation.
128+
* A failure will still arise if there is an annotation that does not match any results, but not vice versa.
129+
* Override this predicate to specify optional results.
130+
*/
131+
predicate hasOptionalResult(Location location, string element, string tag, string value) {
132+
none()
133+
}
134+
126135
final predicate hasFailureMessage(FailureLocatable element, string message) {
127136
exists(ActualResult actualResult |
128137
actualResult.getTest() = this and
@@ -134,7 +143,8 @@ abstract class InlineExpectationsTest extends string {
134143
)
135144
or
136145
not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
137-
message = "Unexpected result: " + actualResult.getExpectationText()
146+
message = "Unexpected result: " + actualResult.getExpectationText() and
147+
not actualResult.isOptional()
138148
)
139149
)
140150
or
@@ -243,9 +253,13 @@ private string expectationPattern() {
243253

244254
private newtype TFailureLocatable =
245255
TActualResult(
246-
InlineExpectationsTest test, Location location, string element, string tag, string value
256+
InlineExpectationsTest test, Location location, string element, string tag, string value,
257+
boolean optional
247258
) {
248-
test.hasActualResult(location, element, tag, value)
259+
test.hasActualResult(location, element, tag, value) and
260+
optional = false
261+
or
262+
test.hasOptionalResult(location, element, tag, value) and optional = true
249263
} or
250264
TValidExpectation(ExpectationComment comment, string tag, string value, string knownFailure) {
251265
exists(TColumn column, string tags |
@@ -277,8 +291,9 @@ class ActualResult extends FailureLocatable, TActualResult {
277291
string element;
278292
string tag;
279293
string value;
294+
boolean optional;
280295

281-
ActualResult() { this = TActualResult(test, location, element, tag, value) }
296+
ActualResult() { this = TActualResult(test, location, element, tag, value, optional) }
282297

283298
override string toString() { result = element }
284299

@@ -289,6 +304,8 @@ class ActualResult extends FailureLocatable, TActualResult {
289304
override string getTag() { result = tag }
290305

291306
override string getValue() { result = value }
307+
308+
predicate isOptional() { optional = true }
292309
}
293310

294311
abstract private class Expectation extends FailureLocatable {

‎ruby/ql/lib/codeql/ruby/ApiGraphs.qll

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -88,22 +88,41 @@ module API {
8888
* This predicate may have multiple results when there are multiple constructor calls invoking this API component.
8989
* Consider using `getAnInstantiation()` if there is a need to distinguish between individual constructor calls.
9090
*/
91-
Node getInstance() { result = this.getASuccessor(Label::instance()) }
91+
Node getInstance() { result = this.getASubclass().getASuccessor(Label::instance()) }
9292

9393
/**
9494
* Gets a node representing the result of calling a method on the receiver represented by this node.
9595
*/
96-
Node getReturn(string method) { result = this.getASuccessor(Label::return(method)) }
96+
Node getReturn(string method) {
97+
result = this.getASubclass().getASuccessor(Label::return(method))
98+
}
9799

98100
/**
99101
* Gets a `new` call to the function represented by this API component.
100102
*/
101103
DataFlow::ExprNode getAnInstantiation() { result = this.getInstance().getAnImmediateUse() }
102104

103105
/**
104-
* Gets a node representing a subclass of the class represented by this node.
106+
* Gets a node representing a (direct or indirect) subclass of the class represented by this node.
107+
* ```rb
108+
* class A; end
109+
* class B < A; end
110+
* class C < B; end
111+
* ```
112+
* In the example above, `getMember("A").getASubclass()` will return uses of `A`, `B` and `C`.
113+
*/
114+
Node getASubclass() { result = this.getAnImmediateSubclass*() }
115+
116+
/**
117+
* Gets a node representing a direct subclass of the class represented by this node.
118+
* ```rb
119+
* class A; end
120+
* class B < A; end
121+
* class C < B; end
122+
* ```
123+
* In the example above, `getMember("A").getAnImmediateSubclass()` will return uses of `B` only.
105124
*/
106-
Node getASubclass() { result = this.getASuccessor(Label::subclass()) }
125+
Node getAnImmediateSubclass() { result = this.getASuccessor(Label::subclass()) }
107126

108127
/**
109128
* Gets a string representation of the lexicographically least among all shortest access paths
@@ -254,10 +273,9 @@ module API {
254273
*/
255274
pragma[nomagic]
256275
private predicate useRoot(string lbl, DataFlow::Node ref) {
257-
exists(string name, ExprNodes::ConstantAccessCfgNode access, ConstantReadAccess read |
258-
access = ref.asExpr() and
259-
lbl = Label::member(read.getName()) and
260-
read = access.getExpr()
276+
exists(string name, ConstantReadAccess read |
277+
read = ref.asExpr().getExpr() and
278+
lbl = Label::member(read.getName())
261279
|
262280
name = resolveTopLevel(read)
263281
or
@@ -389,6 +407,17 @@ module API {
389407
useStep(lbl, node, ref)
390408
)
391409
)
410+
or
411+
// `pred` is a use of class A
412+
// `succ` is a use of class B
413+
// there exists a class declaration B < A
414+
exists(ClassDeclaration c, DataFlow::Node a, DataFlow::Node b |
415+
use(pred, a) and
416+
use(succ, b) and
417+
resolveConstant(b.asExpr().getExpr()) = resolveConstantWriteAccess(c) and
418+
c.getSuperclassExpr() = a.asExpr().getExpr() and
419+
lbl = Label::subclass()
420+
)
392421
}
393422

394423
/**

‎ruby/ql/lib/codeql/ruby/ast/internal/Module.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,12 +233,17 @@ private module ResolveImpl {
233233

234234
pragma[nomagic]
235235
private string resolveConstantReadAccessNonRec(ConstantReadAccess c, int priority) {
236+
// ::B
236237
c.hasGlobalScope() and result = c.getName() and priority = 0
237238
or
239+
// A::B
238240
exists(string name, string s | result = isDefinedConstantNonRec(s, name) |
239241
s = resolveConstantReadAccessScopeNonRec(c, priority, name)
240242
)
241243
or
244+
// module A
245+
// B
246+
// end
242247
exists(string name |
243248
exists(Namespace n, string qname |
244249
n = constantReadAccessEnclosingNameSpace(c, priority, name) and

‎ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll

Lines changed: 8 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -4,22 +4,9 @@ private import codeql.ruby.controlflow.CfgNodes
44
private import codeql.ruby.DataFlow
55
private import codeql.ruby.dataflow.RemoteFlowSources
66
private import codeql.ruby.ast.internal.Module
7+
private import codeql.ruby.ApiGraphs
78
private import ActionView
89

9-
private class ActionControllerBaseAccess extends ConstantReadAccess {
10-
ActionControllerBaseAccess() {
11-
this.getName() = "Base" and
12-
this.getScopeExpr().(ConstantAccess).getName() = "ActionController"
13-
}
14-
}
15-
16-
// ApplicationController extends ActionController::Base, but we
17-
// treat it separately in case the ApplicationController definition
18-
// is not in the database
19-
private class ApplicationControllerAccess extends ConstantReadAccess {
20-
ApplicationControllerAccess() { this.getName() = "ApplicationController" }
21-
}
22-
2310
/**
2411
* A `ClassDeclaration` for a class that extends `ActionController::Base`.
2512
* For example,
@@ -35,16 +22,13 @@ private class ApplicationControllerAccess extends ConstantReadAccess {
3522
*/
3623
class ActionControllerControllerClass extends ClassDeclaration {
3724
ActionControllerControllerClass() {
38-
// class FooController < ActionController::Base
39-
this.getSuperclassExpr() instanceof ActionControllerBaseAccess
40-
or
41-
// class FooController < ApplicationController
42-
this.getSuperclassExpr() instanceof ApplicationControllerAccess
43-
or
44-
// class BarController < FooController
45-
exists(ActionControllerControllerClass other |
46-
other.getModule() = resolveConstantReadAccess(this.getSuperclassExpr())
47-
)
25+
this.getSuperclassExpr() =
26+
[
27+
API::getTopLevelMember("ActionController").getMember("Base"),
28+
// In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we
29+
// treat it separately in case the `ApplicationController` definition is not in the database.
30+
API::getTopLevelMember("ApplicationController")
31+
].getASubclass().getAUse().asExpr().getExpr()
4832
}
4933

5034
/**

0 commit comments

Comments
 (0)