Skip to content

Commit aaff09a

Browse files
Merge commit from fork
Regexp filter values are written into the generated DQL verbatim, unlike other operators which are quoted. Pass them through only when they are well-formed /pattern/flags literals; otherwise quote them as ordinary string arguments. Adds a rewriter regression test.
1 parent 2744eff commit aaff09a

2 files changed

Lines changed: 89 additions & 1 deletion

File tree

graphql/resolve/query_rewriter.go

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2346,7 +2346,13 @@ func buildUnionFilter(typ schema.Type, filter map[string]interface{}) (*dql.Filt
23462346
func maybeQuoteArg(fn string, arg interface{}) string {
23472347
switch arg := arg.(type) {
23482348
case string: // dateTime also parsed as string
2349-
if fn == "regexp" {
2349+
// A regexp argument is a /.../ literal, so it can't be %q-quoted like
2350+
// other operators — it's written into the DQL query verbatim. Only pass
2351+
// it through raw when it is a single, self-contained regexp literal;
2352+
// otherwise fall through and quote it, which contains an injection
2353+
// payload as an ordinary string arg. See isValidRegexArg
2354+
// (GHSA-33p8-wc97-5qcj, CWE-943).
2355+
if fn == "regexp" && isValidRegexArg(arg) {
23502356
return arg
23512357
}
23522358
return fmt.Sprintf("%q", arg)
@@ -2357,6 +2363,53 @@ func maybeQuoteArg(fn string, arg interface{}) string {
23572363
}
23582364
}
23592365

2366+
// isValidRegexArg reports whether s is a single, self-contained DQL regexp
2367+
// literal — a leading '/', a pattern terminated by the first following
2368+
// unescaped '/', and then only regexp flag characters ([a-zA-Z]). This mirrors
2369+
// dql.lexRegex, which decides how the value is tokenized once it reaches
2370+
// Dgraph: anything after the closing '/' that is not a flag would lex into
2371+
// additional DQL tokens.
2372+
//
2373+
// The GraphQL rewriter writes regexp arguments into the DQL query string
2374+
// verbatim. Without this check a crafted value such as `/x/) OR has(pred`
2375+
// closes the regexp() call and appends attacker-controlled DQL, letting an
2376+
// unauthenticated caller bypass the intended filter and read every node of the
2377+
// type (GHSA-33p8-wc97-5qcj, CWE-943). Callers quote the value when this
2378+
// returns false, containing it as an ordinary string argument.
2379+
func isValidRegexArg(s string) bool {
2380+
if len(s) < 2 || s[0] != '/' {
2381+
return false
2382+
}
2383+
// Find the closing '/', honoring backslash escapes, exactly as dql.lexRegex.
2384+
closed := false
2385+
i := 1
2386+
for i < len(s) {
2387+
switch s[i] {
2388+
case '\\':
2389+
i += 2 // skip the escaped character
2390+
continue
2391+
case '/':
2392+
i++
2393+
closed = true
2394+
}
2395+
if closed {
2396+
break
2397+
}
2398+
i++
2399+
}
2400+
if !closed {
2401+
return false // unclosed regexp
2402+
}
2403+
// Everything after the closing '/' must be a regexp flag; anything else
2404+
// would tokenize into additional DQL.
2405+
for ; i < len(s); i++ {
2406+
if c := s[i]; !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')) {
2407+
return false
2408+
}
2409+
}
2410+
return true
2411+
}
2412+
23602413
// first returns the first element it finds in a map - we bump into lots of one-element
23612414
// maps like { "anyofterms": "GraphQL" }. fst helps extract that single mapping.
23622415
func first(aMap map[string]interface{}) (string, interface{}) {

graphql/resolve/query_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,41 @@ func TestQueryRewriting(t *testing.T) {
7272
}
7373
}
7474

75+
// TestRegexpFilterInjectionIsContained is a regression guard for
76+
// GHSA-33p8-wc97-5qcj (CWE-943). A `regexp` filter argument is written into the
77+
// DQL query verbatim (it is a /.../ literal, not a %q-quotable string). A value
78+
// crafted to close the regexp() call and append boolean DQL must NOT reach the
79+
// query as raw text, or an unauthenticated caller can bypass the intended
80+
// filter and read every node of the type. Country.name is
81+
// @search(by: ["trigram"]), so it exposes a regexp filter.
82+
func TestRegexpFilterInjectionIsContained(t *testing.T) {
83+
gqlSchema := test.LoadSchemaFromFile(t, "schema.graphql")
84+
testRewriter := NewQueryRewriter()
85+
86+
// The payload closes the regexp literal and the regexp() call, then ORs in
87+
// has(Country.name) so the filter matches every Country.
88+
const gqlQuery = `query {
89+
queryCountry(filter: { name: { regexp: "/x/) OR has(Country.name" }}) {
90+
name
91+
}
92+
}`
93+
94+
op, err := gqlSchema.Operation(&schema.Request{Query: gqlQuery})
95+
require.NoError(t, err)
96+
q := test.GetQuery(t, op)
97+
98+
dgQuery, _, err := testRewriter.Rewrite(context.Background(), q)
99+
require.NoError(t, err)
100+
got := dgraph.AsString(dgQuery)
101+
102+
// Vulnerable rewriting emits the payload raw, turning it into executable DQL.
103+
require.NotContains(t, got, "regexp(Country.name, /x/) OR has(Country.name)",
104+
"regexp argument emitted as raw DQL — injection breakout is possible:\n%s", got)
105+
// Fixed rewriting contains the payload as a single quoted argument.
106+
require.Contains(t, got, `regexp(Country.name, "/x/) OR has(Country.name")`,
107+
"regexp argument should be quoted and contained:\n%s", got)
108+
}
109+
75110
type HTTPRewritingCase struct {
76111
Name string
77112
GQLQuery string

0 commit comments

Comments
 (0)