Skip to content

JS: Handle spread/rest in API graphs #19108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 1, 2025
Merged

JS: Handle spread/rest in API graphs #19108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff99d5c
JS: Add test for API graph through spread args
asgerf Mar 24, 2025
1ad471c
JS: Track through spread/rest params in API graphs
asgerf Mar 24, 2025
b834ffe
JS: Fix a bad join order
asgerf Mar 27, 2025
f64bdcc
Update javascript/ql/lib/semmle/javascript/ApiGraphs.qll
asgerf Mar 31, 2025
149ec20
JS: Add comment about internal edge
asgerf Mar 31, 2025
e1784bb
JS: Fix handling of spread args on a bound function
asgerf Mar 31, 2025
4746cfd
JS: Add clarifying comment
asgerf Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions javascript/ql/lib/semmle/javascript/ApiGraphs.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,11 @@ module API {
Node getParameter(int i) {
Stages::ApiStage::ref() and
result = this.getASuccessor(Label::parameter(i))
or
exists(int spreadIndex, string arrayProp |
result = this.getASuccessor(Label::spreadArgument(spreadIndex)).getMember(arrayProp) and
i = spreadIndex + arrayProp.toInt()
)
}

/**
Expand Down Expand Up @@ -860,6 +865,23 @@ module API {
.getStaticMember(name, DataFlow::MemberKind::getter())
.getAReturn()
)
or
// Handle rest parameters escaping into external code. For example:
//
// function foo(...rest) {
// externalFunc(rest);
// }
//
// Here, 'rest' reaches a def-node at the call to externalFunc, so we need to ensure
// the arguments passed to 'foo' are stored in the 'rest' array.
exists(Function fun, DataFlow::InvokeNode invoke, int argIndex, Parameter rest |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is modelling the parameters at (or after) the rest parameter, and modelling each entry as an array store into the spread argument.
And this is then only used for the getParameter method to be able to retrieve the correct parameter when there are spread arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Added a comment to clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is then only used for the getParameter method to be able to retrieve the correct parameter when there are spread arguments?

Ah, actually no, it's more than that. If a library model expects an argument to contain array elements, this will allow the model to find those array elements when the array was created by passing arguments into a rest parameter.

fun.getRestParameter() = rest and
rest.flow() = pred and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.getALocalUse() on pred?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed. It would be equivalent to getALocalSource() on the rest parameter, but parameters are already a source.

invoke.getACallee() = fun and
invoke.getArgument(argIndex) = rhs and
argIndex >= rest.getIndex() and
lbl = Label::member((argIndex - rest.getIndex()).toString())
)
)
or
exists(DataFlow::ClassNode cls, string name |
Expand Down Expand Up @@ -888,6 +910,11 @@ module API {
i = -1 and lbl = Label::receiver()
)
or
exists(int i |
spreadArgumentPassing(base, i, rhs) and
lbl = Label::spreadArgument(i)
)
or
exists(DataFlow::SourceNode src, DataFlow::PropWrite pw |
use(base, src) and pw = trackUseNode(src).getAPropertyWrite() and rhs = pw.getRhs()
|
Expand Down Expand Up @@ -931,6 +958,29 @@ module API {
)
}

pragma[nomagic]
private int firstSpreadIndex(InvokeExpr expr) {
result = min(int i | expr.getArgument(i) instanceof SpreadElement)
}

pragma[nomagic]
private InvokeExpr getAnInvocationWithSpread(DataFlow::SourceNode node, int i) {
result = node.getAnInvocation().asExpr() and
i = firstSpreadIndex(result)
}

private predicate spreadArgumentPassing(TApiNode base, int i, DataFlow::Node spreadArray) {
exists(
DataFlow::Node use, DataFlow::SourceNode pred, int bound, InvokeExpr invoke, int spreadPos
|
use(base, use) and
pred = trackUseNode(use, _, bound, "") and
invoke = getAnInvocationWithSpread(pred, spreadPos) and
spreadArray = invoke.getArgument(spreadPos).(SpreadElement).getOperand().flow() and
i = bound + spreadPos
)
}

/**
* Holds if `rhs` is the right-hand side of a definition of node `nd`.
*/
Expand Down Expand Up @@ -1579,6 +1629,9 @@ module API {
/** Gets the edge label for the receiver. */
LabelReceiver receiver() { any() }

/** Gets the edge label for a spread argument passed at index `i`. */
LabelSpreadArgument spreadArgument(int i) { result.getIndex() = i }

/** Gets the `return` edge label. */
LabelReturn return() { any() }

Expand Down Expand Up @@ -1628,6 +1681,7 @@ module API {
} or
MkLabelReceiver() or
MkLabelReturn() or
MkLabelSpreadArgument(int index) { index = [0 .. 10] } or
MkLabelDecoratedClass() or
MkLabelDecoratedMember() or
MkLabelDecoratedParameter() or
Expand Down Expand Up @@ -1743,6 +1797,21 @@ module API {
override string toString() { result = "getReceiver()" }
}

/** A label representing an array passed as a spread argument at a given index. */
class LabelSpreadArgument extends ApiLabel, MkLabelSpreadArgument {
private int index;

LabelSpreadArgument() { this = MkLabelSpreadArgument(index) }

/** Gets the argument index at which the spread argument appears. */
int getIndex() { result = index }

override string toString() {
// Note: This refers to the internal edge that has no corresponding method on API::Node
result = "getSpreadArgument(" + index + ")"
}
}

/** A label for a function that is a wrapper around another function. */
class LabelForwardingFunction extends ApiLabel, MkLabelForwardingFunction {
override string toString() { result = "getForwardingFunction()" }
Expand Down
6 changes: 6 additions & 0 deletions javascript/ql/lib/semmle/javascript/dataflow/Sources.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,12 @@ private module Cached {
cached
predicate invocation(DataFlow::SourceNode func, DataFlow::InvokeNode invoke) {
hasLocalSource(invoke.getCalleeNode(), func)
or
exists(ClassDefinition cls, SuperCall call |
hasLocalSource(cls.getSuperClass().flow(), func) and
call.getBinder() = cls.getConstructor().getBody() and
invoke = call.flow()
)
}

/**
Expand Down
18 changes: 18 additions & 0 deletions javascript/ql/test/ApiGraphs/spread/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,21 @@ function f() {
lib.m1({
...f()
})

function getArgs() {
return [
'x', /* def=moduleImport("something").getMember("exports").getMember("m2").getSpreadArgument(0).getArrayElement() */
'y', /* def=moduleImport("something").getMember("exports").getMember("m2").getSpreadArgument(0).getArrayElement() */
]
}

lib.m2(...getArgs());

function f3() {
return [
'x', /* def=moduleImport("something").getMember("exports").getMember("m3").getSpreadArgument(1).getArrayElement() */
'y', /* def=moduleImport("something").getMember("exports").getMember("m3").getSpreadArgument(1).getArrayElement() */
]
}

lib.m3.bind(undefined, 1)(...f3());
12 changes: 12 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#select
| apollo.serverSide.ts:8:39:8:64 | get(fil ... => {}) | apollo.serverSide.ts:7:36:7:44 | { files } | apollo.serverSide.ts:8:43:8:50 | file.url | The $@ of this request depends on a $@. | apollo.serverSide.ts:8:43:8:50 | file.url | URL | apollo.serverSide.ts:7:36:7:44 | { files } | user-provided value |
| apollo.serverSide.ts:18:37:18:62 | get(fil ... => {}) | apollo.serverSide.ts:17:34:17:42 | { files } | apollo.serverSide.ts:18:41:18:48 | file.url | The $@ of this request depends on a $@. | apollo.serverSide.ts:18:41:18:48 | file.url | URL | apollo.serverSide.ts:17:34:17:42 | { files } | user-provided value |
| axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | axiosInterceptors.serverSide.js:19:21:19:28 | req.body | axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | The $@ of this request depends on a $@. | axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | endpoint | axiosInterceptors.serverSide.js:19:21:19:28 | req.body | user-provided value |
| serverSide.js:18:5:18:20 | request(tainted) | serverSide.js:14:29:14:35 | req.url | serverSide.js:18:13:18:19 | tainted | The $@ of this request depends on a $@. | serverSide.js:18:13:18:19 | tainted | URL | serverSide.js:14:29:14:35 | req.url | user-provided value |
| serverSide.js:20:5:20:24 | request.get(tainted) | serverSide.js:14:29:14:35 | req.url | serverSide.js:20:17:20:23 | tainted | The $@ of this request depends on a $@. | serverSide.js:20:17:20:23 | tainted | URL | serverSide.js:14:29:14:35 | req.url | user-provided value |
Expand Down Expand Up @@ -31,6 +32,11 @@ edges
| apollo.serverSide.ts:8:13:8:17 | files | apollo.serverSide.ts:8:28:8:31 | file | provenance | |
| apollo.serverSide.ts:8:28:8:31 | file | apollo.serverSide.ts:8:43:8:46 | file | provenance | |
| apollo.serverSide.ts:8:43:8:46 | file | apollo.serverSide.ts:8:43:8:50 | file.url | provenance | |
| apollo.serverSide.ts:17:34:17:42 | files | apollo.serverSide.ts:18:11:18:15 | files | provenance | |
| apollo.serverSide.ts:17:34:17:42 | { files } | apollo.serverSide.ts:17:34:17:42 | files | provenance | |
| apollo.serverSide.ts:18:11:18:15 | files | apollo.serverSide.ts:18:26:18:29 | file | provenance | |
| apollo.serverSide.ts:18:26:18:29 | file | apollo.serverSide.ts:18:41:18:44 | file | provenance | |
| apollo.serverSide.ts:18:41:18:44 | file | apollo.serverSide.ts:18:41:18:48 | file.url | provenance | |
| axiosInterceptors.serverSide.js:19:11:19:17 | { url } | axiosInterceptors.serverSide.js:19:11:19:28 | url | provenance | |
| axiosInterceptors.serverSide.js:19:11:19:28 | url | axiosInterceptors.serverSide.js:20:23:20:25 | url | provenance | |
| axiosInterceptors.serverSide.js:19:21:19:28 | req.body | axiosInterceptors.serverSide.js:19:11:19:17 | { url } | provenance | |
Expand Down Expand Up @@ -91,6 +97,12 @@ nodes
| apollo.serverSide.ts:8:28:8:31 | file | semmle.label | file |
| apollo.serverSide.ts:8:43:8:46 | file | semmle.label | file |
| apollo.serverSide.ts:8:43:8:50 | file.url | semmle.label | file.url |
| apollo.serverSide.ts:17:34:17:42 | files | semmle.label | files |
| apollo.serverSide.ts:17:34:17:42 | { files } | semmle.label | { files } |
| apollo.serverSide.ts:18:11:18:15 | files | semmle.label | files |
| apollo.serverSide.ts:18:26:18:29 | file | semmle.label | file |
| apollo.serverSide.ts:18:41:18:44 | file | semmle.label | file |
| apollo.serverSide.ts:18:41:18:48 | file.url | semmle.label | file.url |
| axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | semmle.label | userProvidedUrl |
| axiosInterceptors.serverSide.js:19:11:19:17 | { url } | semmle.label | { url } |
| axiosInterceptors.serverSide.js:19:11:19:28 | url | semmle.label | url |
Expand Down
4 changes: 2 additions & 2 deletions javascript/ql/test/query-tests/Security/CWE-918/apollo.serverSide.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ function createApolloServer(typeDefs) {

const resolvers2 = {
Mutation: {
downloadFiles: async (_, { files }) => { // $ MISSING: Source[js/request-forgery]
files.forEach((file) => { get(file.url, (res) => {}); }); // $ MISSING: Alert[js/request-forgery] Sink[js/request-forgery]
downloadFiles: async (_, { files }) => { // $ Source[js/request-forgery]
files.forEach((file) => { get(file.url, (res) => {}); }); // $ Alert[js/request-forgery] Sink[js/request-forgery]
return true;
},
},
Expand Down