Skip to content

Commit 1297acf

Browse files
hmachmac
authored
Merge pull request #14216 from hmac/hmac-graphql-enum
Ruby: Restrict GraphQL remote flow sources
2 parents 5e92178 + 2214cae commit 1297acf

File tree

11 files changed

+315
-30
lines changed

11 files changed

+315
-30
lines changed

‎ruby/ql/lib/change-notes/2023-09-18-graphql-sources.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* GraphQL enums are no longer considered remote flow sources.

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

Lines changed: 189 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,46 @@ class GraphqlFieldDefinitionMethodCall extends GraphqlSchemaObjectClassMethodCal
253253

254254
/** Gets the name of this GraphQL field. */
255255
string getFieldName() { result = this.getArgument(0).getConstantValue().getStringlikeValue() }
256+
257+
/**
258+
* Gets the type of this field.
259+
*/
260+
GraphqlType getFieldType() { result = this.getArgument(1) }
261+
262+
/**
263+
* Gets an argument call inside this field definition.
264+
*/
265+
GraphqlFieldArgumentDefinitionMethodCall getAnArgumentCall() { result = this.getArgumentCall(_) }
266+
267+
/**
268+
* Gets the argument call for `name` inside this field definition.
269+
*/
270+
GraphqlFieldArgumentDefinitionMethodCall getArgumentCall(string name) {
271+
result.getEnclosingCallable() = this.getBlock() and result.getArgumentName() = name
272+
}
273+
}
274+
275+
/**
276+
* A call to `argument` in a GraphQL InputObject class.
277+
*/
278+
class GraphqlInputObjectArgumentDefinitionCall extends DataFlow::CallNode {
279+
GraphqlInputObjectArgumentDefinitionCall() {
280+
this =
281+
graphQlSchema()
282+
.getMember("InputObject")
283+
.getADescendentModule()
284+
.getAnOwnModuleSelf()
285+
.getAMethodCall("argument")
286+
}
287+
288+
/** Gets the name of the argument (i.e. the first argument to this `argument` method call) */
289+
string getArgumentName() { result = this.getArgument(0).getConstantValue().getStringlikeValue() }
290+
291+
/** Gets the type of this argument */
292+
GraphqlType getArgumentType() { result = this.getArgument(1).asExpr().getExpr() }
293+
294+
/** Gets the class representing the receiver of this method. */
295+
ClassDeclaration getReceiverClass() { result = this.asExpr().getExpr().getEnclosingModule() }
256296
}
257297

258298
/**
@@ -289,6 +329,64 @@ private class GraphqlFieldArgumentDefinitionMethodCall extends GraphqlSchemaObje
289329

290330
/** Gets the name of the argument (i.e. the first argument to this `argument` method call) */
291331
string getArgumentName() { result = this.getArgument(0).getConstantValue().getStringlikeValue() }
332+
333+
/** Gets the type of this argument */
334+
GraphqlType getArgumentType() { result = this.getArgument(1) }
335+
336+
/**
337+
* Gets the element type of this argument, if it is an array.
338+
* For example if the argument type is `[String]`, this predicate yields `String`.
339+
*/
340+
GraphqlType getArgumentElementType() {
341+
result =
342+
any(ArrayLiteral lit | lit = this.getArgument(1) and lit.getNumberOfElements() = 1)
343+
.getElement(0)
344+
}
345+
}
346+
347+
private class GraphqlType extends ConstantAccess {
348+
/**
349+
* Gets the module corresponding to this type, if it exists.
350+
*/
351+
Module getModule() { result.getAnImmediateReference() = this }
352+
353+
/**
354+
* Gets the type of a field/argument of this type, if it is an object type.
355+
*/
356+
GraphqlType getAFieldOrArgument() { result = this.getFieldOrArgument(_) }
357+
358+
/**
359+
* Gets the type of the `name` field/argument of this type, if it exists.
360+
*/
361+
GraphqlType getFieldOrArgument(string name) {
362+
result =
363+
any(GraphqlFieldDefinitionMethodCall field |
364+
field.getFieldName() = name and
365+
this.getModule().getADeclaration() = field.getReceiverClass()
366+
).getFieldType() or
367+
result =
368+
any(GraphqlInputObjectArgumentDefinitionCall arg |
369+
arg.getArgumentName() = name and this.getModule().getADeclaration() = arg.getReceiverClass()
370+
).getArgumentType()
371+
}
372+
373+
/**
374+
* Holds if this type is an enum.
375+
*/
376+
predicate isEnum() {
377+
API::getTopLevelMember("GraphQL")
378+
.getMember("Schema")
379+
.getMember("Enum")
380+
.getADescendentModule()
381+
.getAnImmediateReference()
382+
.asExpr()
383+
.getExpr() = this
384+
}
385+
386+
/**
387+
* Holds if this type is scalar - i.e. it is neither an object or an enum.
388+
*/
389+
predicate isScalar() { not exists(this.getAFieldOrArgument()) and not this.isEnum() }
292390
}
293391

294392
/**
@@ -350,29 +448,26 @@ class GraphqlFieldResolutionMethod extends Method, Http::Server::RequestHandler:
350448

351449
/** Gets the method call which is the definition of the field corresponding to this resolver method. */
352450
GraphqlFieldDefinitionMethodCall getDefinition() {
353-
result
354-
.getKeywordArgument("resolver_method")
355-
.getConstantValue()
356-
.isStringlikeValue(this.getName())
357-
or
358-
not exists(result.getKeywordArgument("resolver_method").(SymbolLiteral)) and
359-
result.getFieldName() = this.getName()
451+
result.getEnclosingModule() = this.getEnclosingModule() and
452+
(
453+
result
454+
.getKeywordArgument("resolver_method")
455+
.getConstantValue()
456+
.isStringlikeValue(this.getName())
457+
or
458+
not exists(result.getKeywordArgument("resolver_method").(SymbolLiteral)) and
459+
result.getFieldName() = this.getName()
460+
)
360461
}
361462

362463
// check for a named argument the same name as a defined argument for this field
363464
override Parameter getARoutedParameter() {
364465
result = this.getAParameter() and
365466
exists(GraphqlFieldArgumentDefinitionMethodCall argDefn |
366-
argDefn.getEnclosingCallable() = this.getDefinition().getBlock() and
367-
(
368-
result.(KeywordParameter).hasName(argDefn.getArgumentName())
369-
or
370-
// TODO this will cause false positives because now *anything* in the **args
371-
// param will be flagged as RoutedParameter/RemoteFlowSource, but really
372-
// only the hash keys corresponding to the defined arguments are user input
373-
// others could be things defined in the `:extras` keyword argument to the `argument`
374-
result instanceof HashSplatParameter // often you see `def field(**args)`
375-
)
467+
argDefn = this.getDefinition().getAnArgumentCall() and
468+
[argDefn.getArgumentType(), argDefn.getArgumentElementType()].isScalar()
469+
|
470+
result.(KeywordParameter).hasName(argDefn.getArgumentName())
376471
)
377472
}
378473

@@ -383,3 +478,80 @@ class GraphqlFieldResolutionMethod extends Method, Http::Server::RequestHandler:
383478
/** Gets the class containing this method. */
384479
GraphqlSchemaObjectClass getGraphqlClass() { result = schemaObjectClass }
385480
}
481+
482+
private DataFlow::CallNode hashParameterAccess(
483+
GraphqlFieldResolutionMethod method, HashSplatParameter param, GraphqlType type
484+
) {
485+
exists(
486+
DataFlow::LocalSourceNode paramNode, GraphqlFieldArgumentDefinitionMethodCall def, string key
487+
|
488+
param = method.getAParameter() and
489+
paramNode.(DataFlow::ParameterNode).getParameter() = param and
490+
def = method.getDefinition().getAnArgumentCall() and
491+
(
492+
// Direct access to the params hash
493+
[def.getArgumentType(), def.getArgumentElementType()] = type and
494+
def.getArgumentName() = key and
495+
paramNode.flowsTo(hashAccess(result, key))
496+
or
497+
// Nested access
498+
exists(GraphqlType type2 |
499+
hashParameterAccess(method, param, type2)
500+
.(DataFlow::LocalSourceNode)
501+
.flowsTo(hashAccess(result, key)) and
502+
type2.getFieldOrArgument(key) = type
503+
)
504+
)
505+
)
506+
}
507+
508+
private DataFlow::Node parameterAccess(
509+
GraphqlFieldResolutionMethod method, DataFlow::LocalSourceNode param, GraphqlType type
510+
) {
511+
param = getAGraphqlParameter(method, type) and
512+
result = param
513+
or
514+
exists(string key, GraphqlType type2 |
515+
param = parameterAccess(method, _, type2) and
516+
param.flowsTo(hashAccess(result, key)) and
517+
type2.getFieldOrArgument(key) = type
518+
)
519+
}
520+
521+
private DataFlow::ParameterNode getAGraphqlParameter(
522+
GraphqlFieldResolutionMethod method, GraphqlType type
523+
) {
524+
result.getCallable() = method and
525+
(
526+
result.getParameter() instanceof KeywordParameter and
527+
exists(GraphqlFieldArgumentDefinitionMethodCall c |
528+
c = method.getDefinition().getArgumentCall(result.getName())
529+
|
530+
type = [c.getArgumentType(), c.getArgumentElementType()]
531+
)
532+
or
533+
result.getParameter() instanceof SimpleParameter and
534+
type = method.getDefinition().getFieldType()
535+
)
536+
}
537+
538+
/**
539+
* Gets the receiver of the hash access `access` with key `key`.
540+
* For example, in `h["foo"]` the receiver is `h`, the key is "foo"
541+
* and `access` is the dataflow node for the whole expression.
542+
*/
543+
private DataFlow::Node hashAccess(DataFlow::CallNode access, string key) {
544+
access.asExpr() instanceof ExprNodes::ElementReferenceCfgNode and
545+
access.getArgument(0).getConstantValue().isStringlikeValue(key) and
546+
access.getReceiver() = result
547+
}
548+
549+
private class GraphqlParameterAccess extends RemoteFlowSource::Range {
550+
GraphqlParameterAccess() {
551+
exists(GraphqlType type | type.isScalar() |
552+
this = hashParameterAccess(_, _, type) or this = parameterAccess(_, _, type)
553+
)
554+
}
555+
556+
override string getSourceType() { result = "GraphQL" }
557+
}

‎ruby/ql/test/library-tests/frameworks/graphql/GraphQL.expected‎

Lines changed: 49 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,23 @@
11
graphqlSchemaObjectClass
22
| app/graphql/types/base_object.rb:2:3:4:5 | BaseObject |
33
| app/graphql/types/mutation_type.rb:2:3:4:5 | MutationType |
4-
| app/graphql/types/query_type.rb:2:3:45:5 | QueryType |
4+
| app/graphql/types/post.rb:1:1:6:5 | Post |
5+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType |
56
graphqlSchemaObjectFieldDefinition
67
| app/graphql/types/mutation_type.rb:2:3:4:5 | MutationType | app/graphql/types/mutation_type.rb:3:5:3:44 | call to field |
7-
| app/graphql/types/query_type.rb:2:3:45:5 | QueryType | app/graphql/types/query_type.rb:3:5:5:40 | call to field |
8-
| app/graphql/types/query_type.rb:2:3:45:5 | QueryType | app/graphql/types/query_type.rb:7:5:9:7 | call to field |
9-
| app/graphql/types/query_type.rb:2:3:45:5 | QueryType | app/graphql/types/query_type.rb:15:5:17:7 | call to field |
10-
| app/graphql/types/query_type.rb:2:3:45:5 | QueryType | app/graphql/types/query_type.rb:24:5:26:7 | call to field |
11-
| app/graphql/types/query_type.rb:2:3:45:5 | QueryType | app/graphql/types/query_type.rb:32:5:35:7 | call to field |
8+
| app/graphql/types/post.rb:1:1:6:5 | Post | app/graphql/types/post.rb:2:5:2:24 | call to field |
9+
| app/graphql/types/post.rb:1:1:6:5 | Post | app/graphql/types/post.rb:3:5:3:36 | call to field |
10+
| app/graphql/types/post.rb:1:1:6:5 | Post | app/graphql/types/post.rb:4:5:4:60 | call to field |
11+
| app/graphql/types/post.rb:1:1:6:5 | Post | app/graphql/types/post.rb:5:5:5:51 | call to field |
12+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:3:5:5:40 | call to field |
13+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:7:5:9:7 | call to field |
14+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:15:5:17:7 | call to field |
15+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:24:5:26:7 | call to field |
16+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:32:5:35:7 | call to field |
17+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:46:5:49:7 | call to field |
18+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:55:5:57:7 | call to field |
19+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:65:5:67:7 | call to field |
20+
| app/graphql/types/query_type.rb:2:3:85:5 | QueryType | app/graphql/types/query_type.rb:72:5:76:7 | call to field |
1221
graphqlResolveMethod
1322
| app/graphql/mutations/dummy.rb:9:5:12:7 | resolve |
1423
| app/graphql/resolvers/dummy_resolver.rb:10:5:13:7 | resolve |
@@ -23,24 +32,56 @@ graphqlLoadMethodRoutedParameter
2332
| app/graphql/resolvers/dummy_resolver.rb:6:5:8:7 | load_something | app/graphql/resolvers/dummy_resolver.rb:6:24:6:25 | id |
2433
graphqlFieldDefinitionMethodCall
2534
| app/graphql/types/mutation_type.rb:3:5:3:44 | call to field |
35+
| app/graphql/types/post.rb:2:5:2:24 | call to field |
36+
| app/graphql/types/post.rb:3:5:3:36 | call to field |
37+
| app/graphql/types/post.rb:4:5:4:60 | call to field |
38+
| app/graphql/types/post.rb:5:5:5:51 | call to field |
2639
| app/graphql/types/query_type.rb:3:5:5:40 | call to field |
2740
| app/graphql/types/query_type.rb:7:5:9:7 | call to field |
2841
| app/graphql/types/query_type.rb:15:5:17:7 | call to field |
2942
| app/graphql/types/query_type.rb:24:5:26:7 | call to field |
3043
| app/graphql/types/query_type.rb:32:5:35:7 | call to field |
44+
| app/graphql/types/query_type.rb:46:5:49:7 | call to field |
45+
| app/graphql/types/query_type.rb:55:5:57:7 | call to field |
46+
| app/graphql/types/query_type.rb:65:5:67:7 | call to field |
47+
| app/graphql/types/query_type.rb:72:5:76:7 | call to field |
3148
graphqlFieldResolutionMethod
3249
| app/graphql/types/query_type.rb:10:5:13:7 | with_arg |
3350
| app/graphql/types/query_type.rb:18:5:22:7 | custom_method |
3451
| app/graphql/types/query_type.rb:27:5:30:7 | with_splat |
3552
| app/graphql/types/query_type.rb:36:5:40:7 | with_splat_and_named_arg |
53+
| app/graphql/types/query_type.rb:50:5:53:7 | with_enum |
54+
| app/graphql/types/query_type.rb:58:5:63:7 | with_nested_enum |
55+
| app/graphql/types/query_type.rb:68:5:70:7 | with_array |
56+
| app/graphql/types/query_type.rb:77:5:84:7 | with_named_params |
3657
graphqlFieldResolutionRoutedParameter
3758
| app/graphql/types/query_type.rb:10:5:13:7 | with_arg | app/graphql/types/query_type.rb:10:18:10:23 | number |
3859
| app/graphql/types/query_type.rb:18:5:22:7 | custom_method | app/graphql/types/query_type.rb:18:23:18:33 | blah_number |
39-
| app/graphql/types/query_type.rb:27:5:30:7 | with_splat | app/graphql/types/query_type.rb:27:20:27:25 | **args |
4060
| app/graphql/types/query_type.rb:36:5:40:7 | with_splat_and_named_arg | app/graphql/types/query_type.rb:36:34:36:37 | arg1 |
41-
| app/graphql/types/query_type.rb:36:5:40:7 | with_splat_and_named_arg | app/graphql/types/query_type.rb:36:41:36:46 | **rest |
61+
| app/graphql/types/query_type.rb:68:5:70:7 | with_array | app/graphql/types/query_type.rb:68:20:68:23 | list |
62+
| app/graphql/types/query_type.rb:77:5:84:7 | with_named_params | app/graphql/types/query_type.rb:77:27:77:30 | arg1 |
4263
graphqlFieldResolutionDefinition
4364
| app/graphql/types/query_type.rb:10:5:13:7 | with_arg | app/graphql/types/query_type.rb:7:5:9:7 | call to field |
4465
| app/graphql/types/query_type.rb:18:5:22:7 | custom_method | app/graphql/types/query_type.rb:15:5:17:7 | call to field |
4566
| app/graphql/types/query_type.rb:27:5:30:7 | with_splat | app/graphql/types/query_type.rb:24:5:26:7 | call to field |
4667
| app/graphql/types/query_type.rb:36:5:40:7 | with_splat_and_named_arg | app/graphql/types/query_type.rb:32:5:35:7 | call to field |
68+
| app/graphql/types/query_type.rb:50:5:53:7 | with_enum | app/graphql/types/query_type.rb:46:5:49:7 | call to field |
69+
| app/graphql/types/query_type.rb:58:5:63:7 | with_nested_enum | app/graphql/types/query_type.rb:55:5:57:7 | call to field |
70+
| app/graphql/types/query_type.rb:68:5:70:7 | with_array | app/graphql/types/query_type.rb:65:5:67:7 | call to field |
71+
| app/graphql/types/query_type.rb:77:5:84:7 | with_named_params | app/graphql/types/query_type.rb:72:5:76:7 | call to field |
72+
graphqlRemoteFlowSources
73+
| app/graphql/mutations/dummy.rb:5:24:5:25 | id |
74+
| app/graphql/mutations/dummy.rb:9:17:9:25 | something |
75+
| app/graphql/resolvers/dummy_resolver.rb:6:24:6:25 | id |
76+
| app/graphql/resolvers/dummy_resolver.rb:10:17:10:25 | something |
77+
| app/graphql/types/query_type.rb:10:18:10:23 | number |
78+
| app/graphql/types/query_type.rb:18:23:18:33 | blah_number |
79+
| app/graphql/types/query_type.rb:28:22:28:37 | ...[...] |
80+
| app/graphql/types/query_type.rb:29:7:29:22 | ...[...] |
81+
| app/graphql/types/query_type.rb:36:34:36:37 | arg1 |
82+
| app/graphql/types/query_type.rb:38:22:38:32 | ...[...] |
83+
| app/graphql/types/query_type.rb:52:22:52:32 | ...[...] |
84+
| app/graphql/types/query_type.rb:60:22:60:41 | ...[...] |
85+
| app/graphql/types/query_type.rb:68:20:68:23 | list |
86+
| app/graphql/types/query_type.rb:77:27:77:30 | arg1 |
87+
| app/graphql/types/query_type.rb:80:22:80:33 | ...[...] |

‎ruby/ql/test/library-tests/frameworks/graphql/GraphQL.ql‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
private import codeql.ruby.frameworks.GraphQL
22
private import codeql.ruby.AST
3+
private import codeql.ruby.dataflow.RemoteFlowSources
34

45
query predicate graphqlSchemaObjectClass(GraphqlSchemaObjectClass cls) { any() }
56

@@ -34,3 +35,5 @@ query predicate graphqlFieldResolutionDefinition(
3435
) {
3536
meth.getDefinition() = def
3637
}
38+
39+
query predicate graphqlRemoteFlowSources(RemoteFlowSource src) { any() }

‎ruby/ql/test/library-tests/frameworks/graphql/app/graphql/types/base_enum.rb‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
class Types::BaseEnum < GraphQL::Schema::Enum
2+
end

‎ruby/ql/test/library-tests/frameworks/graphql/app/graphql/types/direction.rb‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
module Types
2+
class Direction < Types::BaseEnum
3+
value "asc", "Ascending order", value: "asc"
4+
value "desc", "Descending order", value: "desc"
5+
end
6+
end

‎ruby/ql/test/library-tests/frameworks/graphql/app/graphql/types/media_category.rb‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
module Types
2+
class MediaCategory < Types::BaseEnum
3+
value "AUDIO", "An audio file, such as music or spoken word"
4+
value "IMAGE", "A still image, such as a photo or graphic"
5+
value "TEXT", "Written words"
6+
value "VIDEO", "Motion picture, may have audio"
7+
end
8+
end

‎ruby/ql/test/library-tests/frameworks/graphql/app/graphql/types/post.rb‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
class Types::Post < GraphQL::Schema::Object
2+
field :title, String
3+
field :body, String, null: false
4+
field :media_category, Types::MediaCategory, null: false
5+
field :direction, Types::Direction, null: false
6+
end
7+
end
8+

‎ruby/ql/test/library-tests/frameworks/graphql/app/graphql/types/post_order.rb‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
module Types
2+
class PostOrder < Types::BaseInputObject
3+
argument :direction, Types::Direction, "The ordering direction", required: true
4+
end
5+
end

0 commit comments

Comments
 (0)