diff --git a/changelog.d/security-graphql-mutation-gate-prefixes.md b/changelog.d/security-graphql-mutation-gate-prefixes.md new file mode 100644 index 0000000000..f4247f641e --- /dev/null +++ b/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). diff --git a/src/crux/proxy/graphql_mutation.go b/src/crux/proxy/graphql_mutation.go new file mode 100644 index 0000000000..84f0de2b29 --- /dev/null +++ b/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 +} diff --git a/src/crux/proxy/graphql_mutation_prefix_test.go b/src/crux/proxy/graphql_mutation_prefix_test.go new file mode 100644 index 0000000000..cfc94ba007 --- /dev/null +++ b/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") + } + }) + } +} diff --git a/src/crux/proxy/linear_rules.go b/src/crux/proxy/linear_rules.go index cb8dce203a..daf6394718 100644 --- a/src/crux/proxy/linear_rules.go +++ b/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) { diff --git a/src/crux/proxy/rules.go b/src/crux/proxy/rules.go index a40e7eb844..9f8db7751f 100644 --- a/src/crux/proxy/rules.go +++ b/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) { diff --git a/src/crux/proxy/write_leak_gate_test.go b/src/crux/proxy/write_leak_gate_test.go index 677955ec76..b535d3d677 100644 --- a/src/crux/proxy/write_leak_gate_test.go +++ b/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 { diff --git a/src/crux/proxy/write_surface_enforce.go b/src/crux/proxy/write_surface_enforce.go index 1d4dd4ff85..24466d6466 100644 --- a/src/crux/proxy/write_surface_enforce.go +++ b/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) { diff --git a/src/crux/proxy/write_surface_enforce_test.go b/src/crux/proxy/write_surface_enforce_test.go index a977478f68..cd82e2cbb0 100644 --- a/src/crux/proxy/write_surface_enforce_test.go +++ b/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}, }