fix: prevent GraphQL authorAssociation injection into User-type nodes and add 503 logging

Issue 1: The GraphQL rewriter was blindly injecting authorAssociation into
ALL nodes {} blocks using ReplaceAllString. This broke queries with
assignees, participants, labels, and other connections returning User/Team/
Label types that don't support authorAssociation.

Fix: Added findParentField() to identify the connection field name containing
each nodes block. Injection now only occurs when the parent is in a safe set
(pullRequests, issues, comments, reviews, search for issue/PR fields;
history for commit fields).

Issue 2: When --policy is missing, the proxy returned 503 "proxy enforcement
not configured" but did not log the error, making debugging difficult.

Fix: Added logger.LogError and logHandler.Printf calls so the 503 cause
appears in both proxy.log and debug output.

Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/483eafda-7d9e-4972-abe8-062c6a85813e

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Author: Copilot

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,106 @@ 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
+	braceFound := false
+	for i >= 0 {
+		switch query[i] {
+		case '{':
+			if depth == 0 {
+				braceFound = true
+			} else {
+				depth--
+			}
+		case '}':
+			depth++
+		}
+		if braceFound {
+			break
+		}
+		i--
+	}
+	if !braceFound {
+		return ""
+	}
+
+	// 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 after 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 {

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

internal/proxy/handler.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"log"
 	"net/http"
 
 	"go.opentelemetry.io/otel/attribute"
@@ -141,7 +140,8 @@ 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")
+		logHandler.Printf("returning 503: proxy enforcement not configured (no --policy flag provided)")
+		logger.LogError("proxy", "returning 503: proxy enforcement not configured (no --policy flag provided)")
 		http.Error(w, "proxy enforcement not configured", http.StatusServiceUnavailable)
 		return
 	}