Fix GraphQL authorAssociation injection into User-type nodes; log 503 on missing policy (#3413)

lpcox · web-flow · commit 46495a30d77b · 2026-04-08T22:03:28.000-07:00
The DIFC proxy's GraphQL rewriter blindly injected `authorAssociation` into **all** `nodes {}` blocks via `ReplaceAllString`. Queries with `assignees`, `participants`, `labels`, etc. failed with `Field 'authorAssociation' doesn't exist on type 'User'`. Separately, 503 responses from missing `--policy` were not logged, making debugging costly. ### GraphQL injection fix - Added safe-parent allowlists per field group — only inject into `nodes {}` blocks whose parent connection returns types that actually have the field: - Issue/PR fields (`author{login}`, `authorAssociation`): `pullRequests`, `issues`, `comments`, `reviews`, `search` - Commit fields (`author{user{login}}`): `history` - Added `findParentField()` — walks backward from each `nodes {` match to extract the enclosing connection field name, handling args and nested braces - `fieldsForTool()` now returns `([]guardFieldSet, map[string]bool)` to thread safe parents through to injection Before (breaks on `assignees.nodes → User`): ```graphql assignees(first:100) { nodes {authorAssociation, login } } ``` After (skips unsafe parents): ```graphql assignees(first:100) { nodes { login } } comments(first:10) { nodes {author{login},authorAssociation, body } } ``` ### 503 logging fix - Replaced `log.Printf` with `logHandler.Printf` + `logger.LogError` so missing-policy 503s appear in both debug output and `proxy.log` - Removed unused `log` import > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build2109691296/b514/launcher.test /tmp/go-build2109691296/b514/launcher.test -test.testlogfile=/tmp/go-build2109691296/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I olang.org/grpc@v1.80.0/internal/go1.25.8 .cfg x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -I 2335907/b437/_pkg_.a dU_c/JEVDElkKgVJXLgEedU_c x_amd64/vet --gdwarf-5 g/grpc/encoding/-atomic -o x_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build2109691296/b496/config.test /tmp/go-build2109691296/b496/config.test -test.testlogfile=/tmp/go-build2109691296/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s ortc�� H/bin/golangci-lint run --timeout=5m || echo &#34;��� Warning: golangci-lint failed (compatibility issue with Go 1.25.0). Continuing with other checks...&#34;; \ elif command -v golan 1.80.0/balancer_wrapper.go 64/pkg/tool/linux_amd64/vet . -imultiarch x86_64-linux-gnu/tmp/go-build547184664/b347/vet.cfg 64/pkg/tool/linux_amd64/vet 2335�� olang.org/grpc@v1.80.0/internal/-I olang.org/grpc@v1.80.0/internal//tmp/go-build3905974454/b444/ x_amd64/vet . --gdwarf2 --64 x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build2109691296/b514/launcher.test /tmp/go-build2109691296/b514/launcher.test -test.testlogfile=/tmp/go-build2109691296/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I olang.org/grpc@v1.80.0/internal/go1.25.8 .cfg x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -I 2335907/b437/_pkg_.a dU_c/JEVDElkKgVJXLgEedU_c x_amd64/vet --gdwarf-5 g/grpc/encoding/-atomic -o x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build2109691296/b514/launcher.test /tmp/go-build2109691296/b514/launcher.test -test.testlogfile=/tmp/go-build2109691296/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I olang.org/grpc@v1.80.0/internal/go1.25.8 .cfg x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -I 2335907/b437/_pkg_.a dU_c/JEVDElkKgVJXLgEedU_c x_amd64/vet --gdwarf-5 g/grpc/encoding/-atomic -o x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build2109691296/b523/mcp.test /tmp/go-build2109691296/b523/mcp.test -test.testlogfile=/tmp/go-build2109691296/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
diff --git a/internal/proxy/graphql_rewrite.go b/internal/proxy/graphql_rewrite.go
@@ -32,17 +32,36 @@ var commitFields = []guardFieldSet{
 	{"author{user{login}}", regexp.MustCompile(`\bauthor\s*\{[^}]*\buser\s*\{[^}]*\blogin\b`)},
 }
 
-// fieldsForTool returns the guard fields applicable to the given tool name,
-// or nil if no injection is needed.
-func fieldsForTool(toolName string) []guardFieldSet {
+// issueAndPRSafeParents are connection field names whose node types support
+// author{login} and authorAssociation (i.e., types implementing the Comment
+// interface: Issue, PullRequest, IssueComment, PullRequestReview, etc.).
+// Injection into other connection nodes (e.g. assignees→User, labels→Label)
+// would cause GraphQL validation errors.
+var issueAndPRSafeParents = map[string]bool{
+	"pullRequests": true,
+	"issues":       true,
+	"comments":     true,
+	"reviews":      true,
+	"search":       true,
+}
+
+// commitSafeParents are connection field names whose node types support
+// author{user{login}} (Commit type).
+var commitSafeParents = map[string]bool{
+	"history": true,
+}
+
+// fieldsForTool returns the guard fields and safe parent connection names
+// applicable to the given tool name, or nil if no injection is needed.
+func fieldsForTool(toolName string) ([]guardFieldSet, map[string]bool) {
 	switch toolName {
 	case "list_issues", "list_pull_requests", "issue_read", "pull_request_read",
 		"search_issues":
-		return issueAndPRFields
+		return issueAndPRFields, issueAndPRSafeParents
 	case "list_commits":
-		return commitFields
+		return commitFields, commitSafeParents
 	default:
-		return nil
+		return nil, nil
 	}
 }
 
@@ -73,7 +92,7 @@ func missingFields(query string, fields []guardFieldSet) []string {
 // Returns the (possibly modified) body. If injection is not needed or fails,
 // the original body is returned unchanged.
 func InjectGuardFields(body []byte, toolName string) []byte {
-	fields := fieldsForTool(toolName)
+	fields, safeParents := fieldsForTool(toolName)
 	if fields == nil {
 		logGraphQLRewrite.Printf("No guard field injection needed for tool=%s", toolName)
 		return body
@@ -90,7 +109,7 @@ func InjectGuardFields(body []byte, toolName string) []byte {
 	}
 
 	missing := missingFields(gql.Query, fields)
-	modified := injectFieldsIntoQuery(gql.Query, missing)
+	modified := injectFieldsIntoQuery(gql.Query, missing, safeParents)
 	if modified == gql.Query {
 		return body
 	}
@@ -108,7 +127,10 @@ func InjectGuardFields(body []byte, toolName string) []byte {
 // injectFieldsIntoQuery adds the given fields into the GraphQL query's node
 // selection or fragment. Each field string (e.g. "author{login}",
 // "authorAssociation") is comma-joined and injected as a single block.
-func injectFieldsIntoQuery(query string, fields []string) string {
+// safeParents limits direct nodes injection (Step 3) to nodes blocks whose
+// parent connection field is in the set, preventing injection into User/Label
+// type nodes that don't support the injected fields.
+func injectFieldsIntoQuery(query string, fields []string, safeParents map[string]bool) string {
 	injection := strings.Join(fields, ",")
 
 	// Step 1: Check if the query uses a fragment spread in the nodes.
@@ -134,16 +156,101 @@ func injectFieldsIntoQuery(query string, fields []string) string {
 	}
 
 	// Step 3: No fragment — inject directly into nodes { ... }
-	nodesPattern := regexp.MustCompile(`(nodes\s*\{)`)
-	if nodesPattern.MatchString(query) {
-		logGraphQLRewrite.Printf("Injecting into nodes selection: fields=%q", injection)
-		return nodesPattern.ReplaceAllString(query, "${1}"+injection+",")
+	// Only inject into nodes blocks whose parent connection field is in the
+	// safeParents set. This prevents injecting fields like authorAssociation
+	// into nodes of types that don't support them (e.g. User, Label, Team).
+	nodesPattern := regexp.MustCompile(`nodes\s*\{`)
+	matches := nodesPattern.FindAllStringIndex(query, -1)
+	if len(matches) > 0 {
+		var buf strings.Builder
+		pos := 0
+		injected := false
+		for _, m := range matches {
+			parent := findParentField(query, m[0])
+			buf.WriteString(query[pos:m[1]])
+			if safeParents[parent] {
+				buf.WriteString(injection + ",")
+				injected = true
+			} else {
+				logGraphQLRewrite.Printf("Skipping injection into nodes under %q (not a safe parent)", parent)
+			}
+			pos = m[1]
+		}
+		buf.WriteString(query[pos:])
+		if injected {
+			logGraphQLRewrite.Printf("Injecting into nodes selection: fields=%q", injection)
+			return buf.String()
+		}
 	}
 
 	logGraphQLRewrite.Printf("No injection point found in query for fields=%q", injection)
 	return query
 }
 
+// findParentField extracts the GraphQL connection field name that contains
+// the given nodes block. It walks backward from idx to find the enclosing
+// opening brace, then extracts the field name before it (skipping any
+// arguments in parentheses).
+func findParentField(query string, nodesIdx int) string {
+	// Walk backward from nodesIdx to find the enclosing `{`
+	depth := 0
+	i := nodesIdx - 1
+	for i >= 0 {
+		switch query[i] {
+		case '{':
+			if depth == 0 {
+				goto foundBrace
+			}
+			depth--
+		case '}':
+			depth++
+		}
+		i--
+	}
+	return "" // no enclosing brace found
+
+foundBrace:
+
+	// i now points to the `{` of the enclosing block.
+	// Walk backward past whitespace.
+	i--
+	for i >= 0 && (query[i] == ' ' || query[i] == '\n' || query[i] == '\t' || query[i] == '\r') {
+		i--
+	}
+	// If there are parenthesized args, skip them.
+	if i >= 0 && query[i] == ')' {
+		parenDepth := 1
+		i--
+		for i >= 0 && parenDepth > 0 {
+			switch query[i] {
+			case ')':
+				parenDepth++
+			case '(':
+				parenDepth--
+			}
+			i--
+		}
+		// Skip whitespace between the argument list and the field name.
+		for i >= 0 && (query[i] == ' ' || query[i] == '\n' || query[i] == '\t' || query[i] == '\r') {
+			i--
+		}
+	}
+	// Extract the field name (alphanumeric + underscore)
+	end := i + 1
+	for i >= 0 && isGraphQLFieldNameChar(query[i]) {
+		i--
+	}
+	if i+1 >= end {
+		return ""
+	}
+	return query[i+1 : end]
+}
+
+// isGraphQLFieldNameChar returns true for characters valid in a GraphQL field name.
+func isGraphQLFieldNameChar(c byte) bool {
+	return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_'
+}
+
 // injectIntoFragment adds a field to the end of a named fragment definition.
 // "fragment Name on Type { existing fields }" → "fragment Name on Type { existing fields field }"
 func injectIntoFragment(query, fragName, field string) string {
diff --git a/internal/proxy/graphql_rewrite_test.go b/internal/proxy/graphql_rewrite_test.go
@@ -2,6 +2,7 @@ package proxy
 
 import (
 	"encoding/json"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -247,6 +248,133 @@ func TestInjectGuardFields_PullRequestRead(t *testing.T) {
 	assert.Contains(t, gql.Query, "authorAssociation")
 }
 
+func TestInjectGuardFields_SkipsAssigneesNodes(t *testing.T) {
+	// Reproduces the bug from the issue: gh pr view with assignees causes
+	// "Field 'authorAssociation' doesn't exist on type 'User'" because
+	// injection was applied to ALL nodes blocks including assignees.nodes.
+	query := `query PullRequestByNumber {
+  repository(owner:"o", name:"r") {
+    pullRequest(number: 1820) {
+      number title author{login}
+      assignees(first: 100) { nodes { login } }
+    }
+  }
+}`
+	body, _ := json.Marshal(GraphQLRequest{Query: query})
+
+	result := InjectGuardFields(body, "pull_request_read")
+
+	var gql GraphQLRequest
+	require.NoError(t, json.Unmarshal(result, &gql))
+	// authorAssociation should NOT be injected because the only nodes block
+	// is assignees.nodes which returns User objects.
+	assert.NotContains(t, gql.Query, "authorAssociation",
+		"authorAssociation must not be injected into assignees.nodes (User type)")
+}
+
+func TestInjectGuardFields_MixedNodesBlocks(t *testing.T) {
+	// Query has both a safe connection (comments) and an unsafe one (assignees).
+	// Injection should only go into comments.nodes, not assignees.nodes.
+	query := `query {
+  repository(owner:"o", name:"r") {
+    pullRequest(number: 1) {
+      assignees(first: 10) { nodes { login } }
+      comments(first: 10) { nodes { body } }
+    }
+  }
+}`
+	body, _ := json.Marshal(GraphQLRequest{Query: query})
+
+	result := InjectGuardFields(body, "pull_request_read")
+
+	var gql GraphQLRequest
+	require.NoError(t, json.Unmarshal(result, &gql))
+	assert.Contains(t, gql.Query, "author{login}")
+	assert.Contains(t, gql.Query, "authorAssociation")
+	// Verify injection is in comments.nodes, not assignees.nodes
+	assert.Contains(t, gql.Query, `assignees(first: 10) { nodes { login } }`,
+		"assignees.nodes should remain unmodified")
+	assert.Contains(t, gql.Query, `comments(first: 10) { nodes {author{login},authorAssociation,`,
+		"comments.nodes should have injected fields")
+}
+
+func TestInjectGuardFields_SkipsLabelsNodes(t *testing.T) {
+	// labels.nodes returns Label objects — no authorAssociation field.
+	query := `query { repository(owner:"o", name:"r") { issues(first:10) { nodes { number labels(first:5) { nodes { name } } } } } }`
+	body, _ := json.Marshal(GraphQLRequest{Query: query})
+
+	result := InjectGuardFields(body, "list_issues")
+
+	var gql GraphQLRequest
+	require.NoError(t, json.Unmarshal(result, &gql))
+	assert.Contains(t, gql.Query, "author{login}")
+	assert.Contains(t, gql.Query, "authorAssociation")
+	// labels.nodes must not have injected fields
+	assert.Contains(t, gql.Query, `labels(first:5) { nodes { name } }`,
+		"labels.nodes should remain unmodified")
+}
+
+func TestInjectGuardFields_SkipsParticipantsNodes(t *testing.T) {
+	// participants.nodes returns User objects — no authorAssociation field.
+	query := `query {
+  repository(owner:"o", name:"r") {
+    pullRequest(number: 1) {
+      reviews(first: 5) { nodes { body } }
+      participants(first: 10) { nodes { login } }
+    }
+  }
+}`
+	body, _ := json.Marshal(GraphQLRequest{Query: query})
+
+	result := InjectGuardFields(body, "pull_request_read")
+
+	var gql GraphQLRequest
+	require.NoError(t, json.Unmarshal(result, &gql))
+	assert.Contains(t, gql.Query, "author{login}")
+	assert.Contains(t, gql.Query, "authorAssociation")
+	// participants.nodes should be unmodified
+	assert.Contains(t, gql.Query, `participants(first: 10) { nodes { login } }`)
+}
+
+func TestFindParentField(t *testing.T) {
+	tests := []struct {
+		name     string
+		query    string
+		nodesIdx int // index of "nodes" in the query
+		want     string
+	}{
+		{
+			name:  "simple connection",
+			query: `pullRequests(first:10) { nodes { number } }`,
+			want:  "pullRequests",
+		},
+		{
+			name:  "connection with totalCount before nodes",
+			query: `issues(first:5) { totalCount nodes { title } }`,
+			want:  "issues",
+		},
+		{
+			name:  "nested connection",
+			query: `pullRequest(number:1) { assignees(first:10) { nodes { login } } }`,
+			want:  "assignees",
+		},
+		{
+			name:  "connection without args",
+			query: `comments { nodes { body } }`,
+			want:  "comments",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			idx := strings.Index(tt.query, "nodes")
+			require.NotEqual(t, -1, idx, "query must contain 'nodes'")
+			got := findParentField(tt.query, idx)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
 func TestInjectGuardFields_NoNodesNoFragment(t *testing.T) {
 	// A query with required tool but no "nodes" block and no fragment spread —
 	// the injector cannot find a place to insert fields, so body is returned unchanged.
@@ -284,11 +412,13 @@ func TestFieldsForTool(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run("tool: "+tt.toolName, func(t *testing.T) {
-			fields := fieldsForTool(tt.toolName)
+			fields, safeParents := fieldsForTool(tt.toolName)
 			if tt.wantNil {
 				assert.Nil(t, fields, "expected nil fields for tool %q", tt.toolName)
+				assert.Nil(t, safeParents, "expected nil safeParents for tool %q", tt.toolName)
 			} else {
 				require.NotNil(t, fields, "expected non-nil fields for tool %q", tt.toolName)
+				require.NotNil(t, safeParents, "expected non-nil safeParents for tool %q", tt.toolName)
 				fieldStrings := make([]string, len(fields))
 				for i, f := range fields {
 					fieldStrings[i] = f.field
diff --git a/internal/proxy/handler.go b/internal/proxy/handler.go
@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"log"
 	"net/http"
 
 	"go.opentelemetry.io/otel/attribute"
@@ -141,7 +140,9 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 	defer difcSpan.End()
 
 	if !s.guardInitialized {
-		log.Printf("[proxy] WARNING: guard not initialized, blocking request")
+		errMsg := "returning 503: proxy enforcement not configured (no --policy flag provided)"
+		logHandler.Print(errMsg)
+		logger.LogError("proxy", "%s", errMsg)
 		http.Error(w, "proxy enforcement not configured", http.StatusServiceUnavailable)
 		return
 	}
