Merge commit from fork

Co-authored-by: Matthew McNeely <matthew.mcneely@gmail.com>

Author: SnailSploit

graphql/admin/current_user.go

@@ -39,11 +39,11 @@ func extractName(ctx context.Context) (string, error) {
 }
 
 func (gsr *currentUserResolver) Rewrite(ctx context.Context,
-	gqlQuery schema.Query) ([]*dql.GraphQuery, error) {
+	gqlQuery schema.Query) ([]*dql.GraphQuery, map[string]string, error) {
 
 	name, err := extractName(ctx)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	gqlQuery.Rename("getUser")

graphql/resolve/auth_query_test.yaml

@@ -1760,15 +1760,17 @@
       }
     }
   dgquery: |-
-    query {
+    query checkPwd($pwd0: string) {
       checkUserPassword(func: eq(User.username, "user")) @filter((eq(val(pwd), 1) AND type(User))) {
         User.username : User.username
         dgraph.uid : uid
       }
       checkPwd(func: eq(User.username, "user")) @filter(type(User)) {
-        pwd as checkpwd(User.password, "Password")
+        pwd as checkpwd(User.password, $pwd0)
       }
     }
+  dgvars:
+    $pwd0: Password
 
 - name: Password Query with RBAC rule true
   gqlquery: |
@@ -1782,7 +1784,7 @@
   jwtvar:
     ROLE: Admin
   dgquery: |-
-    query {
+    query checkPwd($pwd0: string) {
       checkLogPassword(func: uid(LogRoot)) @filter((eq(val(pwd), 1) AND type(Log))) {
         Log.id : uid
         Log.logs : Log.logs
@@ -1791,9 +1793,11 @@
       LogRoot as var(func: uid(Log_1))
       Log_1 as var(func: uid(0x123))
       checkPwd(func: uid(LogRoot)) @filter(type(Log)) {
-        pwd as checkpwd(Log.pwd, "something")
+        pwd as checkpwd(Log.pwd, $pwd0)
       }
     }
+  dgvars:
+    $pwd0: something
 
 - name: Password Query with RBAC rule false
   gqlquery: |
@@ -1825,7 +1829,7 @@
   jwtvar:
     USER: user1
   dgquery: |-
-    query {
+    query checkPwd($pwd0: string) {
       checkProjectPassword(func: uid(ProjectRoot)) @filter((eq(val(pwd), 1) AND type(Project))) {
         Project.name : Project.name
         Project.projID : uid
@@ -1853,9 +1857,11 @@
         }
       }
       checkPwd(func: uid(ProjectRoot)) @filter(type(Project)) {
-        pwd as checkpwd(Project.pwd, "something")
+        pwd as checkpwd(Project.pwd, $pwd0)
       }
     }
+  dgvars:
+    $pwd0: something
 
 - name:
     Type with password query should apply Interface's password rules and along with its own auth
@@ -1872,7 +1878,7 @@
     ANS: "true"
     USER: ADMIN
   dgquery: |-
-    query {
+    query checkPwd($pwd0: string) {
       checkQuestionPassword(func: uid(QuestionRoot)) @filter((eq(val(pwd), 1) AND type(Question))) {
         Question.id : uid
         Question.text : Post.text
@@ -1884,9 +1890,11 @@
         Question.text : Post.text
       }
       checkPwd(func: uid(QuestionRoot)) @filter(type(Question)) {
-        pwd as checkpwd(Question.pwd, "something")
+        pwd as checkpwd(Question.pwd, $pwd0)
       }
     }
+  dgvars:
+    $pwd0: something
 
 - name:
     Type which inherits password auth rules from interfaces returns no results when auth rules fail
@@ -1918,7 +1926,7 @@
     ANS: "true"
     USER: ADMIN
   dgquery: |-
-    query {
+    query checkPwd($pwd0: string) {
       checkPostPassword(func: uid(PostRoot)) @filter((eq(val(pwd), 1) AND type(Post))) {
         dgraph.type
         Post.text : Post.text
@@ -1939,9 +1947,11 @@
       }
       Answer_6 as var(func: type(Answer))
       checkPwd(func: uid(PostRoot)) @filter(type(Post)) {
-        pwd as checkpwd(Post.pwd, "something")
+        pwd as checkpwd(Post.pwd, $pwd0)
       }
     }
+  dgvars:
+    $pwd0: something
 
 - name: Entities query with query auth rules
   gqlquery: |

graphql/resolve/auth_test.go

@@ -54,6 +54,7 @@ type AuthQueryRewritingCase struct {
 	// Dgraph upsert query and mutations built from the GQL
 	DGQuery        string
 	DGQuerySec     string
+	DGVars         map[string]string
 	DGMutations    []*dgraphMutation
 	DGMutationsSec []*dgraphMutation
 
@@ -444,7 +445,7 @@ func queryRewriting(t *testing.T, sch string, authMeta *testutil.AuthMeta, b []b
 				require.NoError(t, err)
 			}
 
-			dgQuery, err := testRewriter.Rewrite(ctx, gqlQuery)
+			dgQuery, dgVars, err := testRewriter.Rewrite(ctx, gqlQuery)
 
 			if tcase.Error != nil {
 				require.NotNil(t, err)
@@ -453,9 +454,14 @@ func queryRewriting(t *testing.T, sch string, authMeta *testutil.AuthMeta, b []b
 			} else {
 				require.NoError(t, err)
 				require.Equal(t, tcase.DGQuery, dgraph.AsString(dgQuery))
+				if len(tcase.DGVars) == 0 {
+					require.Empty(t, dgVars)
+				} else {
+					require.Equal(t, tcase.DGVars, dgVars)
+				}
 			}
 			// Check for unused variables.
-			_, err = dql.Parse(dql.Request{Str: dgraph.AsString(dgQuery)})
+			_, err = dql.Parse(dql.Request{Str: dgraph.AsString(dgQuery), Variables: dgVars})
 			require.NoError(t, err)
 		})
 	}

graphql/resolve/query.go

@@ -40,8 +40,10 @@ type QueryResolver interface {
 }
 
 // A QueryRewriter can build a Dgraph dql.GraphQuery from a GraphQL query,
+// along with any DQL variable bindings (e.g. for parameterized password
+// queries) that must be supplied to the executor.
 type QueryRewriter interface {
-	Rewrite(ctx context.Context, q schema.Query) ([]*dql.GraphQuery, error)
+	Rewrite(ctx context.Context, q schema.Query) ([]*dql.GraphQuery, map[string]string, error)
 }
 
 // QueryResolverFunc is an adapter that allows to build a QueryResolver from
@@ -121,7 +123,7 @@ func (qr *queryResolver) rewriteAndExecute(ctx context.Context, query schema.Que
 		}
 	}
 
-	dgQuery, err := qr.queryRewriter.Rewrite(ctx, query)
+	dgQuery, vars, err := qr.queryRewriter.Rewrite(ctx, query)
 	if err != nil {
 		return emptyResult(schema.GQLWrapf(err, "couldn't rewrite query %s",
 			query.ResponseName()))
@@ -130,7 +132,8 @@ func (qr *queryResolver) rewriteAndExecute(ctx context.Context, query schema.Que
 
 	queryTimer := newtimer(ctx, &dgraphQueryDuration.OffsetDuration)
 	queryTimer.Start()
-	resp, err := qr.executor.Execute(ctx, &dgoapi.Request{Query: qry, ReadOnly: true}, query)
+	resp, err := qr.executor.Execute(ctx,
+		&dgoapi.Request{Query: qry, Vars: vars, ReadOnly: true}, query)
 	queryTimer.Stop()
 
 	if err != nil && !x.IsGqlErrorList(err) {

graphql/resolve/query_rewriter.go

@@ -110,14 +110,16 @@ func getAuthSelector(queryType schema.QueryType) func(t schema.Type) *schema.Rul
 	return queryAuthSelector
 }
 
-// Rewrite rewrites a GraphQL query into DQL.
+// Rewrite rewrites a GraphQL query into DQL. It also returns any DQL variable
+// bindings (e.g. parameterized password values) that must be passed to the
+// executor alongside the query string.
 func (qr *queryRewriter) Rewrite(
 	ctx context.Context,
-	gqlQuery schema.Query) ([]*dql.GraphQuery, error) {
+	gqlQuery schema.Query) ([]*dql.GraphQuery, map[string]string, error) {
 
 	customClaims, err := gqlQuery.GetAuthMeta().ExtractCustomClaims(ctx)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	authRw := &authRewriter{
@@ -143,29 +145,30 @@ func (qr *queryRewriter) Rewrite(
 		// ATM, I'm not sure how to hook into the GraphQL validator to get that to happen
 		xid, uid, err := gqlQuery.IDArgValue()
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 
 		dgQuery := rewriteAsGet(gqlQuery, uid, xid, authRw)
-		return dgQuery, nil
+		return dgQuery, nil, nil
 	case schema.SimilarByIdQuery:
 		xid, uid, err := gqlQuery.IDArgValue()
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
-		return rewriteAsSimilarByIdQuery(gqlQuery, uid, xid, authRw), nil
+		return rewriteAsSimilarByIdQuery(gqlQuery, uid, xid, authRw), nil, nil
 	case schema.SimilarByEmbeddingQuery:
-		return rewriteAsSimilarByEmbeddingQuery(gqlQuery, authRw), nil
+		return rewriteAsSimilarByEmbeddingQuery(gqlQuery, authRw), nil, nil
 	case schema.FilterQuery:
-		return rewriteAsQuery(gqlQuery, authRw), nil
+		return rewriteAsQuery(gqlQuery, authRw), nil, nil
 	case schema.PasswordQuery:
 		return passwordQuery(gqlQuery, authRw)
 	case schema.AggregateQuery:
-		return aggregateQuery(gqlQuery, authRw), nil
+		return aggregateQuery(gqlQuery, authRw), nil, nil
 	case schema.EntitiesQuery:
-		return entitiesQuery(gqlQuery, authRw)
+		queries, err := entitiesQuery(gqlQuery, authRw)
+		return queries, nil, err
 	default:
-		return nil, errors.Errorf("unimplemented query type %s", gqlQuery.QueryType())
+		return nil, nil, errors.Errorf("unimplemented query type %s", gqlQuery.QueryType())
 	}
 }
 
@@ -340,17 +343,18 @@ func aggregateQuery(query schema.Query, authRw *authRewriter) []*dql.GraphQuery
 	return append([]*dql.GraphQuery{finalMainQuery}, dgQuery...)
 }
 
-func passwordQuery(m schema.Query, authRw *authRewriter) ([]*dql.GraphQuery, error) {
+func passwordQuery(m schema.Query, authRw *authRewriter) (
+	[]*dql.GraphQuery, map[string]string, error) {
 	xid, uid, err := m.IDArgValue()
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	dgQuery := rewriteAsGet(m, uid, xid, authRw)
 
 	// Handle empty dgQuery
 	if strings.HasSuffix(dgQuery[0].Attr, "()") {
-		return dgQuery, nil
+		return dgQuery, nil, nil
 	}
 
 	// mainQuery is the query with check<Type>Password as Attr.
@@ -362,15 +366,24 @@ func passwordQuery(m schema.Query, authRw *authRewriter) ([]*dql.GraphQuery, err
 	predicate := queriedType.DgraphPredicate(name)
 	password := m.ArgValue(name).(string)
 
+	// Parameterize the password as a DQL variable so the value is bound by
+	// the executor rather than interpolated into the query string. This
+	// prevents DQL injection via passwords containing characters such as
+	// a double-quote.
+	const pwdVar = "$pwd0"
+	if mainQuery.Args == nil {
+		mainQuery.Args = make(map[string]string)
+	}
+	mainQuery.Args[pwdVar] = "string"
+
 	// This adds the checkPwd function
 	op := &dql.GraphQuery{
 		Attr:   "checkPwd",
 		Func:   mainQuery.Func,
 		Filter: mainQuery.Filter,
 		Children: []*dql.GraphQuery{{
-			Var: "pwd",
-			Attr: fmt.Sprintf(`checkpwd(%s, "%s")`, predicate,
-				password),
+			Var:  "pwd",
+			Attr: fmt.Sprintf(`checkpwd(%s, %s)`, predicate, pwdVar),
 		}},
 	}
 
@@ -397,7 +410,7 @@ func passwordQuery(m schema.Query, authRw *authRewriter) ([]*dql.GraphQuery, err
 
 	mainQuery.Filter = ft
 
-	return append(dgQuery, op), nil
+	return append(dgQuery, op), map[string]string{pwdVar: password}, nil
 }
 
 func intersection(a, b []uint64) []uint64 {

graphql/resolve/query_test.go

@@ -42,6 +42,7 @@ type QueryRewritingCase struct {
 	GQLQuery     string
 	GQLVariables string
 	DGQuery      string
+	DGVars       map[string]string
 }
 
 func TestQueryRewriting(t *testing.T) {
@@ -70,9 +71,14 @@ func TestQueryRewriting(t *testing.T) {
 			require.NoError(t, err)
 			gqlQuery := test.GetQuery(t, op)
 
-			dgQuery, err := testRewriter.Rewrite(context.Background(), gqlQuery)
+			dgQuery, dgVars, err := testRewriter.Rewrite(context.Background(), gqlQuery)
 			require.NoError(t, err)
 			require.Equal(t, tcase.DGQuery, dgraph.AsString(dgQuery))
+			if len(tcase.DGVars) == 0 {
+				require.Empty(t, dgVars)
+			} else {
+				require.Equal(t, tcase.DGVars, dgVars)
+			}
 		})
 	}
 }

graphql/resolve/query_test.yaml

@@ -2446,15 +2446,17 @@
       }
     }
   dgquery: |-
-    query {
+    query checkPwd($pwd0: string) {
       checkUserPassword(func: eq(User.name, "user1")) @filter((eq(val(pwd), 1) AND type(User))) {
         User.name : User.name
         dgraph.uid : uid
       }
       checkPwd(func: eq(User.name, "user1")) @filter(type(User)) {
-        pwd as checkpwd(User.pwd, "Password")
+        pwd as checkpwd(User.pwd, $pwd0)
       }
     }
+  dgvars:
+    $pwd0: Password
 
 - name: Password query with alias
   gqlquery: |
@@ -2464,15 +2466,37 @@
       }
     }
   dgquery: |-
+    query checkPwd($pwd0: string) {
+      checkUserPassword(func: eq(User.name, "user1")) @filter((eq(val(pwd), 1) AND type(User))) {
+        User.name : User.name
+        dgraph.uid : uid
+      }
+      checkPwd(func: eq(User.name, "user1")) @filter(type(User)) {
+        pwd as checkpwd(User.pwd, $pwd0)
+      }
+    }
+  dgvars:
+    $pwd0: Password
+
+- name: Password query parameterizes pwd to prevent DQL injection
+  gqlquery: |
     query {
+      checkUserPassword(name: "user1", pwd: "evil\") { msg } injected(func: has(User.name)) #") {
+        name
+      }
+    }
+  dgquery: |-
+    query checkPwd($pwd0: string) {
       checkUserPassword(func: eq(User.name, "user1")) @filter((eq(val(pwd), 1) AND type(User))) {
         User.name : User.name
         dgraph.uid : uid
       }
       checkPwd(func: eq(User.name, "user1")) @filter(type(User)) {
-        pwd as checkpwd(User.pwd, "Password")
+        pwd as checkpwd(User.pwd, $pwd0)
       }
     }
+  dgvars:
+    $pwd0: 'evil") { msg } injected(func: has(User.name)) #'
 
 - name: Rewrite without custom fields
   gqlquery: |