🐛 fix(proxy): GitHub GraphQL mutation gate no longer fails open on same-line prefixes

changelog.d/security-graphql-mutation-gate-prefixes.md

@@ -0,0 +1 @@
+- **proxy:** the GitHub GraphQL mutation gate no longer fails open when `mutation` follows a same-line prefix (a leading fragment definition, comma or BOM). It now uses the same string- and comment-aware depth-zero token scan as the Linear path, so read-only lanes cannot slip a mutation past the capability tier, write-surface and auto-merge checks (#17676).

src/crux/proxy/graphql_mutation.go

@@ -0,0 +1,11 @@
+package proxy
+
+// isGitHubGraphQLMutation reports whether the document holds a depth-zero
+// mutation operation. An anchored `^\s*mutation` regex misses same-line
+// prefixes (a leading fragment, comma or BOM), so every GitHub GraphQL gate
+// uses the string- and comment-aware token scan the Linear path already
+// relies on. A document the scan cannot read (an unterminated string) is
+// treated as a mutation: unclassifiable input fails closed.
+func isGitHubGraphQLMutation(query string) bool {
+	return countGraphQLKeyword(query, "mutation") != 0
+}

src/crux/proxy/graphql_mutation_prefix_test.go

@@ -0,0 +1,53 @@
+package proxy
+
+import (
+	"testing"
+
+	"github.com/hivecommons/hive/crux/agent"
+)
+
+func TestGraphQLMutationGate_SameLinePrefixes(t *testing.T) {
+	const mut = `mutation { addComment(input:{subjectId:"I_kw", body:"hi"}) { clientMutationId } }`
+	cases := []struct {
+		name, query string
+		write       bool
+	}{
+		{"plain mutation", mut, true},
+		{"fragment-first", `fragment F on Repository { id } ` + mut, true},
+		{"leading comma", `,` + mut, true},
+		{"leading BOM", "\uFEFF" + mut, true},
+		{"plain query", `query { viewer { login } }`, false},
+		{"fragment-first query", `fragment F on Repository { id } query { viewer { login } }`, false},
+		{"mutation word in a string", `query { search(query: "mutation { x }") { issueCount } }`, false},
+		{"escaped triple quote in block string", `fragment F on Repository { ref(qualifiedName: """\"""""") { id } }
+mutation { addComment(input:{subjectId:"x", body:"y"}) { subject { ... on Issue { repository { ...F } } } } }`, true},
+		{"unterminated string fails closed", `query { search(query: "abc) { issueCount } }`, true},
+		{"unterminated block string fails closed", `query { search(query: """abc) { issueCount } }`, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			body := graphQLBody(t, tc.query)
+			allowed, write := GraphQLAllowedCaps(agent.ModeAdvisory, agent.AgentCapabilities{}, body)
+			if allowed == tc.write || write != tc.write {
+				t.Errorf("allowed=%v write=%v, want allowed=%v write=%v", allowed, write, !tc.write, tc.write)
+			}
+		})
+	}
+}
+
+func TestGraphQLAutoMergeRefusal_SameLinePrefixes(t *testing.T) {
+	enabled := autoMergeOffFor("acme/frozen")
+	for name, q := range map[string]string{
+		"fragment-first": `fragment F on Repository { id } ` + mergeMutationQuery,
+		"leading comma":  `,` + mergeMutationQuery,
+		"leading BOM":    "\uFEFF" + mergeMutationQuery,
+	} {
+		t.Run(name, func(t *testing.T) {
+			body := string(graphQLBody(t, q))
+			body = body[:len(body)-1] + `,"variables":{"repo":"acme/frozen"}}`
+			if _, refused := GraphQLAutoMergeRefusal(enabled, []byte(body)); !refused {
+				t.Fatal("merge on an auto-merge-off repo was not refused")
+			}
+		})
+	}
+}

src/crux/proxy/linear_rules.go

@@ -371,14 +371,19 @@ func linearOperationStart(query string) (linearOpKind, int, bool) {
 // countGraphQLKeyword counts occurrences of an operation keyword appearing as a
 // standalone token at brace depth zero, ignoring occurrences inside strings and
 // comments. A substring count would miscount a field named `queryable` or the
-// word "mutation" inside a comment body.
+// word "mutation" inside a comment body. Returns -1 when the document cannot be
+// scanned (an unterminated string), so callers can fail closed.
 func countGraphQLKeyword(query, kw string) int {
 	n := 0
-	scanGraphQLTokens(query, func(name string, depth int) {
+	ok := scanGraphQLTokensAt(query, func(name string, depth, _ int) bool {
 		if depth == 0 && name == kw {
 			n++
 		}
+		return true
 	})
+	if !ok {
+		return -1
+	}
 	return n
 }
 
@@ -396,17 +401,11 @@ func indexGraphQLKeyword(query, kw string) int {
 	return idx
 }
 
-func scanGraphQLTokens(query string, fn func(name string, depth int)) {
-	scanGraphQLTokensAt(query, func(name string, depth, _ int) bool {
-		fn(name, depth)
-		return true
-	})
-}
-
 // scanGraphQLTokensAt walks the document emitting each bare name token with the
 // brace depth it was found at, skipping strings and comments so their contents
-// are never mistaken for tokens. fn returns false to stop the walk.
-func scanGraphQLTokensAt(query string, fn func(name string, depth, off int) bool) {
+// are never mistaken for tokens. fn returns false to stop the walk. Returns
+// false only when the walk aborted on an unreadable (unterminated) literal.
+func scanGraphQLTokensAt(query string, fn func(name string, depth, off int) bool) bool {
 	depth := 0
 	for i := 0; i < len(query); {
 		c := query[i]
@@ -418,7 +417,7 @@ func scanGraphQLTokensAt(query string, fn func(name string, depth, off int) bool
 		case c == '"':
 			var ok bool
 			if i, ok = skipGraphQLString(query, i); !ok {
-				return
+				return false
 			}
 		case c == '{':
 			depth++
@@ -429,25 +428,32 @@ func scanGraphQLTokensAt(query string, fn func(name string, depth, off int) bool
 		case isGraphQLNameStart(c):
 			name := graphQLIdentRe.FindString(query[i:])
 			if !fn(name, depth, i) {
-				return
+				return true
 			}
 			i += len(name)
 		default:
 			i++
 		}
 	}
+	return true
 }
 
 // skipGraphQLString advances past a string or block-string literal beginning at
 // i. Returns ok=false on an unterminated literal, which the callers treat as an
-// unreadable document.
+// unreadable document. Inside a block string `\"""` is an escaped triple quote,
+// not the terminator.
 func skipGraphQLString(query string, i int) (int, bool) {
 	if strings.HasPrefix(query[i:], `"""`) {
-		end := strings.Index(query[i+3:], `"""`)
-		if end < 0 {
-			return 0, false
+		for j := i + 3; j < len(query); j++ {
+			if strings.HasPrefix(query[j:], `\"""`) {
+				j += 3
+				continue
+			}
+			if strings.HasPrefix(query[j:], `"""`) {
+				return j + 3, true
+			}
 		}
-		return i + 3 + end + 3, true
+		return 0, false
 	}
 	i++ // opening quote
 	for i < len(query) {

src/crux/proxy/rules.go

@@ -577,8 +577,6 @@ type graphQLRequest struct {
 	Variables     any    `json:"variables"`
 }
 
-var graphQLMutationRe = regexp.MustCompile(`(?m)^\s*mutation\b`)
-
 // graphQLMergeMutationRe matches mutations that merge a pull request. Merging is
 // the highest-privilege write and must require the same mode as the REST
 // `PUT /pulls/{n}/merge` rule (ModeIssuesPRsMerge) — otherwise an ISSUES_ONLY
@@ -804,7 +802,7 @@ func GraphQLAllowedCaps(mode agent.AgentMode, caps agent.AgentCapabilities, body
 	}
 
 	query := strings.TrimSpace(req.Query)
-	if !graphQLMutationRe.MatchString(query) {
+	if !isGitHubGraphQLMutation(query) {
 		return true, false // read-only query
 	}
 
@@ -843,7 +841,7 @@ func GraphQLAutoMergeRefusal(enabled func(repo string) bool, body []byte) (strin
 	if err := json.Unmarshal(body, &req); err != nil {
 		return "", false
 	}
-	if !graphQLMutationRe.MatchString(strings.TrimSpace(req.Query)) || !graphQLMergeMutationRe.MatchString(req.Query) {
+	if !isGitHubGraphQLMutation(req.Query) || !graphQLMergeMutationRe.MatchString(req.Query) {
 		return "", false
 	}
 	if repo := repoFromGraphQLValue(req.Variables); repo != "" && !enabled(repo) {

src/crux/proxy/write_leak_gate_test.go

@@ -83,6 +83,9 @@ func TestSkipGraphQLString(t *testing.T) {
 		{"block string with quotes inside", `"""a"b""" rest`, 0, 9, true},
 		{"block string with newline", "\"\"\"a\nb\"\"\"", 0, 9, true},
 		{"unterminated block string", `"""abc`, 0, 0, false},
+		{"block string escaped triple quote", `"""a\"""b""" rest`, 0, 12, true},
+		{"block string only escaped triple quote", `"""\""""""`, 0, 10, true},
+		{"block string escaped terminator unterminated", `"""a\"""`, 0, 0, false},
 		{"offset start", `x "hi" y`, 2, 6, true},
 	}
 	for _, tt := range tests {

src/crux/proxy/write_surface_enforce.go

@@ -78,7 +78,7 @@ func WriteSurfaceGraphQLRefusal(enforced func(agent string) bool, agent string,
 		return "", false
 	}
 	var req graphQLRequest
-	if err := json.Unmarshal(body, &req); err == nil && !graphQLMutationRe.MatchString(strings.TrimSpace(req.Query)) {
+	if err := json.Unmarshal(body, &req); err == nil && !isGitHubGraphQLMutation(req.Query) {
 		return "", false
 	}
 	if !enforced(agent) {

src/crux/proxy/write_surface_enforce_test.go

@@ -89,6 +89,10 @@ func TestWriteSurfaceGraphQLRefusal(t *testing.T) {
 		{"mutation", "scanner", `{"query":"mutation { addComment(input:{}) { clientMutationId } }"}`, true},
 		{"query", "scanner", `{"query":"query { viewer { login } }"}`, false},
 		{"shorthand query", "scanner", `{"query":"{ viewer { login } }"}`, false},
+		{"fragment-first mutation", "scanner", `{"query":"fragment F on Repository { id } mutation { addComment(input:{}) { clientMutationId } }"}`, true},
+		{"leading comma mutation", "scanner", `{"query":",mutation { addComment(input:{}) { clientMutationId } }"}`, true},
+		{"leading BOM mutation", "scanner", "{\"query\":\"\uFEFFmutation { addComment(input:{}) { clientMutationId } }\"}", true},
+		{"fragment-first query", "scanner", `{"query":"fragment F on Repository { id } query { viewer { login } }"}`, false},
 		{"not json fails closed", "scanner", `not json`, true},
 		{"unlisted lane mutation", "reviewer", `{"query":"mutation { x }"}`, false},
 	}