Merge remote-tracking branch 'origin/main' into sammorrowdrums-align-pr-3111-body-limit

Author: SamMorrowDrums

README.md

@@ -1619,6 +1619,8 @@ docker run -i --rm \
   ghcr.io/github/github-mcp-server
 ```
 
+In HTTP mode, this flag (or `GITHUB_LOCKDOWN_MODE`) is an upper bound: the `X-MCP-Lockdown` request header can enable lockdown mode when the operator has not, but it cannot disable lockdown mode the operator has already enabled. See the [Server Configuration Guide](docs/server-configuration.md#lockdown-mode) for details.
+
 The behavior of lockdown mode depends on the tool invoked.
 
 Following tools will return an error when the author lacks the push access:

docs/remote-server.md

@@ -67,9 +67,10 @@ The Remote GitHub MCP server has optional headers equivalent to the Local server
 - `X-MCP-Readonly`: Enables only "read" tools.
     - Equivalent to `GITHUB_READ_ONLY` env var for Local server.
     - If this header is empty, "false", "f", "no", "n", "0", or "off" (ignoring whitespace and case), it will be interpreted as false. All other values are interpreted as true.
-- `X-MCP-Lockdown`: Enables lockdown mode, hiding public issue details created by users without push access.
+- `X-MCP-Lockdown`: Enables lockdown mode, hiding public issue details created by users without push access. Lockdown mode is a best-effort content filter, not a security boundary.
     - Equivalent to `GITHUB_LOCKDOWN_MODE` env var for Local server.
     - If this header is empty, "false", "f", "no", "n", "0", or "off" (ignoring whitespace and case), it will be interpreted as false. All other values are interpreted as true.
+    - Server-side lockdown configuration is an upper bound: if the operator has already enabled lockdown mode, this header cannot disable it for a request. The header can only enable (or redundantly re-enable) lockdown mode; it cannot relax lockdown mode below the operator's configuration.
 - `X-MCP-Insiders`: Enables insiders mode for early access to new features.
     - Equivalent to `GITHUB_INSIDERS` env var or `--insiders` flag for Local server.
     - If this header is empty, "false", "f", "no", "n", "0", or "off" (ignoring whitespace and case), it will be interpreted as false. All other values are interpreted as true.

docs/server-configuration.md

@@ -29,6 +29,8 @@ Note: **read-only** mode acts as a strict security filter that takes precedence
 
 Note: **excluded tools** takes precedence over toolsets and individual tools — listed tools are always excluded, even if their toolset is enabled or they are explicitly added via `--tools` / `X-MCP-Tools`.
 
+Note: server-side **lockdown mode** (`--lockdown-mode` / `GITHUB_LOCKDOWN_MODE`) is an upper bound in HTTP mode — once an operator enables it, the `X-MCP-Lockdown` header can no longer disable it for a given request. A request may still use the header to enable lockdown mode for itself when the operator has not already enabled it server-wide, but it can never relax lockdown mode below what the operator configured. Lockdown mode remains a best-effort content filter, not a security boundary.
+
 ---
 
 ## Configuration Examples
@@ -292,6 +294,8 @@ When active, this mode will disable all tools that are not read-only even if the
 
 Lockdown mode ensures the server only surfaces content in public repositories from users with push access to that repository. Private repositories are unaffected, and collaborators retain full access to their own content.
 
+> In HTTP mode, server-side lockdown mode (`--lockdown-mode` / `GITHUB_LOCKDOWN_MODE`) is an upper bound: the `X-MCP-Lockdown` header can enable lockdown mode for a request when the operator has not enabled it server-wide, but it cannot disable lockdown mode the operator has already enabled.
+
 Lockdown mode is a best-effort content filter meant to reduce prompt-injection risk from untrusted repository content; it is not an authorization boundary. It does not restrict what the underlying credential can otherwise read or write, and content withheld from a filtered tool response may still be reachable through other tools or direct GitHub API access with the same credential.
 
 As an intentional exception, content authored by trusted bot accounts (currently `github-actions[bot]` and `copilot`) is always treated as safe, regardless of push access, so routine automation output isn't filtered.

pkg/github/dependencies.go

@@ -439,9 +439,17 @@ func (d *RequestDeps) GetRawClient(ctx context.Context) (*raw.Client, error) {
 	return rawClient, nil
 }
 
+// effectiveLockdownMode reports whether lockdown mode is active for the
+// request. d.lockdownMode is an operator-set upper bound: the per-request
+// X-MCP-Lockdown header (ghcontext.IsLockdownMode) can only enable lockdown,
+// never disable one the operator already turned on.
+func (d *RequestDeps) effectiveLockdownMode(ctx context.Context) bool {
+	return d.lockdownMode || ghcontext.IsLockdownMode(ctx)
+}
+
 // GetRepoAccessCache implements ToolDependencies.
 func (d *RequestDeps) GetRepoAccessCache(ctx context.Context) (*lockdown.RepoAccessCache, error) {
-	if !d.lockdownMode {
+	if !d.effectiveLockdownMode(ctx) {
 		return nil, nil
 	}
 
@@ -466,7 +474,7 @@ func (d *RequestDeps) GetT() translations.TranslationHelperFunc { return d.T }
 // GetFlags implements ToolDependencies.
 func (d *RequestDeps) GetFlags(ctx context.Context) FeatureFlags {
 	return FeatureFlags{
-		LockdownMode: d.lockdownMode && ghcontext.IsLockdownMode(ctx),
+		LockdownMode: d.effectiveLockdownMode(ctx),
 	}
 }
 

pkg/github/dependencies_test.go

@@ -198,6 +198,116 @@ func TestIsFeatureEnabled_EmptyFlagName(t *testing.T) {
 	assert.False(t, result, "Expected false for empty flag name")
 }
 
+// TestRequestDepsLockdownModeIsUpperBound verifies the X-MCP-Lockdown header
+// can only enable lockdown, never disable the operator's server-side setting.
+func TestRequestDepsLockdownModeIsUpperBound(t *testing.T) {
+	t.Parallel()
+
+	resolver := newRequestDepsAPIHostResolver(t, "https://example.com")
+
+	newDeps := func(serverLockdown bool) *github.RequestDeps {
+		return github.NewRequestDeps(
+			resolver,
+			"test",
+			serverLockdown,
+			nil,
+			translations.NullTranslationHelper,
+			0,
+			nil,
+			testExporters(),
+		)
+	}
+
+	tokenCtx := func(requestLockdown bool) context.Context {
+		ctx := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "request-token"})
+		if requestLockdown {
+			ctx = ghcontext.WithLockdownMode(ctx, true)
+		}
+		return ctx
+	}
+
+	tests := []struct {
+		name             string
+		serverLockdown   bool
+		requestLockdown  bool
+		wantLockdownMode bool
+	}{
+		{
+			name:             "neither server nor request enable lockdown",
+			serverLockdown:   false,
+			requestLockdown:  false,
+			wantLockdownMode: false,
+		},
+		{
+			name:             "server-only lockdown is enforced without a request header",
+			serverLockdown:   true,
+			requestLockdown:  false,
+			wantLockdownMode: true,
+		},
+		{
+			name:             "request-only lockdown can enable it when the server has not",
+			serverLockdown:   false,
+			requestLockdown:  true,
+			wantLockdownMode: true,
+		},
+		{
+			name:             "server and request both enabling lockdown stays enabled",
+			serverLockdown:   true,
+			requestLockdown:  true,
+			wantLockdownMode: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			deps := newDeps(tt.serverLockdown)
+			ctx := tokenCtx(tt.requestLockdown)
+
+			flags := deps.GetFlags(ctx)
+			assert.Equal(t, tt.wantLockdownMode, flags.LockdownMode, "GetFlags().LockdownMode")
+
+			cache, err := deps.GetRepoAccessCache(ctx)
+			require.NoError(t, err)
+			if tt.wantLockdownMode {
+				assert.NotNil(t, cache, "expected a repo access cache to be built when lockdown mode is effectively enabled")
+			} else {
+				assert.Nil(t, cache, "expected no repo access cache when lockdown mode is effectively disabled")
+			}
+		})
+	}
+}
+
+// TestRequestDepsLockdownModeCannotBeDisabledByOmittingHeader is a regression
+// test for #3104: omitting the X-MCP-Lockdown header must not disable
+// server-enabled lockdown mode.
+func TestRequestDepsLockdownModeCannotBeDisabledByOmittingHeader(t *testing.T) {
+	t.Parallel()
+
+	resolver := newRequestDepsAPIHostResolver(t, "https://example.com")
+	deps := github.NewRequestDeps(
+		resolver,
+		"test",
+		true, // server-enabled lockdown
+		nil,
+		translations.NullTranslationHelper,
+		0,
+		nil,
+		testExporters(),
+	)
+
+	// No X-MCP-Lockdown header sent.
+	ctx := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "request-token"})
+
+	flags := deps.GetFlags(ctx)
+	assert.True(t, flags.LockdownMode, "server-enabled lockdown mode must remain enabled when a request omits the lockdown header")
+
+	cache, err := deps.GetRepoAccessCache(ctx)
+	require.NoError(t, err)
+	assert.NotNil(t, cache, "repo access cache must still be built so server-enabled lockdown mode can be enforced")
+}
+
 func TestIsFeatureEnabled_CheckerError(t *testing.T) {
 	t.Parallel()
 

pkg/github/discussions.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/github/github-mcp-server/pkg/ifc"
 	"github.com/github/github-mcp-server/pkg/inventory"
+	"github.com/github/github-mcp-server/pkg/sanitize"
 	"github.com/github/github-mcp-server/pkg/scopes"
 	"github.com/github/github-mcp-server/pkg/translations"
 	"github.com/github/github-mcp-server/pkg/utils"
@@ -99,7 +100,7 @@ type WithCategoryNoOrder struct {
 func fragmentToDiscussion(fragment NodeFragment) *github.Discussion {
 	return &github.Discussion{
 		Number:    github.Ptr(int(fragment.Number)),
-		Title:     github.Ptr(string(fragment.Title)),
+		Title:     github.Ptr(sanitize.Sanitize(string(fragment.Title))),
 		HTMLURL:   github.Ptr(string(fragment.URL)),
 		CreatedAt: &github.Timestamp{Time: fragment.CreatedAt.Time},
 		UpdatedAt: &github.Timestamp{Time: fragment.UpdatedAt.Time},
@@ -360,8 +361,8 @@ func GetDiscussion(t translations.TranslationHelperFunc) inventory.ServerTool {
 			// like ListDiscussions and GetDiscussionComments).
 			response := map[string]any{
 				"number":     int(d.Number),
-				"title":      string(d.Title),
-				"body":       string(d.Body),
+				"title":      sanitize.Sanitize(string(d.Title)),
+				"body":       sanitize.Sanitize(string(d.Body)),
 				"url":        string(d.URL),
 				"closed":     bool(d.Closed),
 				"isAnswered": bool(d.IsAnswered),
@@ -520,18 +521,10 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
 					return utils.NewToolResultError(err.Error()), nil, nil
 				}
 				for _, c := range q.Repository.Discussion.Comments.Nodes {
-					comment := MinimalDiscussionComment{
-						ID:              fmt.Sprintf("%v", c.ID),
-						Body:            string(c.Body),
-						IsAnswer:        bool(c.IsAnswer),
-						ReplyTotalCount: c.Replies.TotalCount,
-					}
+					comment := newMinimalDiscussionComment(fmt.Sprintf("%v", c.ID), string(c.Body), bool(c.IsAnswer))
+					comment.ReplyTotalCount = c.Replies.TotalCount
 					for _, r := range c.Replies.Nodes {
-						comment.Replies = append(comment.Replies, MinimalDiscussionComment{
-							ID:       fmt.Sprintf("%v", r.ID),
-							Body:     string(r.Body),
-							IsAnswer: bool(r.IsAnswer),
-						})
+						comment.Replies = append(comment.Replies, newMinimalDiscussionComment(fmt.Sprintf("%v", r.ID), string(r.Body), bool(r.IsAnswer)))
 					}
 					comments = append(comments, comment)
 				}
@@ -562,11 +555,7 @@ func GetDiscussionComments(t translations.TranslationHelperFunc) inventory.Serve
 					return utils.NewToolResultError(err.Error()), nil, nil
 				}
 				for _, c := range q.Repository.Discussion.Comments.Nodes {
-					comments = append(comments, MinimalDiscussionComment{
-						ID:       fmt.Sprintf("%v", c.ID),
-						Body:     string(c.Body),
-						IsAnswer: bool(c.IsAnswer),
-					})
+					comments = append(comments, newMinimalDiscussionComment(fmt.Sprintf("%v", c.ID), string(c.Body), bool(c.IsAnswer)))
 				}
 				pageInfo = q.Repository.Discussion.Comments.PageInfo
 				totalCount = q.Repository.Discussion.Comments.TotalCount

pkg/github/discussions_test.go

@@ -553,6 +553,30 @@ func Test_GetDiscussion(t *testing.T) {
 			expectError: true,
 			errContains: "discussion not found",
 		},
+		{
+			name: "sanitizes malicious title and body",
+			response: githubv4mock.DataResponse(map[string]any{
+				"repository": map[string]any{"discussion": map[string]any{
+					"number":     1,
+					"title":      maliciousText,
+					"body":       maliciousText,
+					"url":        "https://github.com/owner/repo/discussions/1",
+					"createdAt":  "2025-04-25T12:00:00Z",
+					"closed":     false,
+					"isAnswered": false,
+					"category":   map[string]any{"name": "General"},
+				}},
+			}),
+			expectError: false,
+			expected: map[string]any{
+				"number":     float64(1),
+				"title":      sanitizedText,
+				"body":       sanitizedText,
+				"url":        "https://github.com/owner/repo/discussions/1",
+				"closed":     false,
+				"isAnswered": false,
+			},
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {

pkg/github/find_duplicate.go

@@ -152,12 +152,15 @@ func FindDuplicate(t translations.TranslationHelperFunc) inventory.ServerTool {
 					return utils.NewToolResultError("ranked duplicate detection is unavailable: the semantic-similarity endpoint returned issues without ranking metadata (the server-side duplicate-ranking feature is not enabled for this caller or repository)"), nil, nil
 				}
 				candidates = append(candidates, duplicateCandidate{
-					Issue: MinimalIssueRef{
-						Number: res.Issue.Number,
-						Title:  res.Issue.Title,
-						State:  res.Issue.State,
-						URL:    res.Issue.HTMLURL,
-					},
+					// Candidates are always scoped to the requested repository, so the
+					// ref's repository field is left empty as it was before.
+					Issue: newMinimalIssueRef(
+						res.Issue.Number,
+						res.Issue.Title,
+						res.Issue.State,
+						res.Issue.HTMLURL,
+						"",
+					),
 					Score:           res.Score,
 					Confidence:      res.Confidence,
 					LikelyDuplicate: res.LikelyDuplicate,

pkg/github/find_duplicate_test.go

@@ -120,6 +120,52 @@ func Test_FindDuplicate_RankedResults(t *testing.T) {
 	assert.False(t, candidates[1].LikelyDuplicate)
 }
 
+// Test_FindDuplicate_SanitizesIssueTitle asserts that candidate issue titles, which are
+// user-authored content from an arbitrary repository, are sanitized before being returned.
+// Without this the tool would forward hidden-instruction payloads straight to the model.
+func Test_FindDuplicate_SanitizesIssueTitle(t *testing.T) {
+	serverTool := FindDuplicate(translations.NullTranslationHelper)
+
+	rankedResults := []map[string]any{
+		{
+			"issue": map[string]any{
+				"number":   456,
+				"title":    maliciousText,
+				"state":    "open",
+				"html_url": "https://github.com/owner/repo/issues/456",
+			},
+			"score":            0.95,
+			"confidence":       "high",
+			"likely_duplicate": true,
+		},
+	}
+
+	handler := func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write(MustMarshal(rankedResults))
+	}
+
+	client := mustNewGHClient(t, NewMockedHTTPClient(WithRequestMatchHandler(endpointSemanticallySimilar, http.HandlerFunc(handler))))
+	deps := BaseDeps{Client: client}
+	toolHandler := serverTool.Handler(deps)
+
+	request := createMCPRequest(map[string]any{
+		"owner":        "owner",
+		"repo":         "repo",
+		"issue_number": float64(123),
+	})
+	result, err := toolHandler(ContextWithDeps(context.Background(), deps), &request)
+	require.NoError(t, err)
+	require.False(t, result.IsError, "expected result to not be an error")
+
+	text := getTextResult(t, result)
+	var candidates []duplicateCandidate
+	require.NoError(t, json.Unmarshal([]byte(text.Text), &candidates))
+	require.Len(t, candidates, 1)
+	assert.Equal(t, sanitizedText, candidates[0].Issue.Title)
+	assert.NotContains(t, text.Text, "<script>")
+}
+
 func Test_FindDuplicate_OmitsUnsetParams(t *testing.T) {
 	serverTool := FindDuplicate(translations.NullTranslationHelper)
 

pkg/github/issue_dependencies.go

@@ -171,16 +171,17 @@ func issueToDependencyRef(issue *github.Issue) MinimalIssueRef {
 	if issue == nil {
 		return MinimalIssueRef{}
 	}
-	ref := MinimalIssueRef{
-		Number: issue.GetNumber(),
-		Title:  issue.GetTitle(),
-		State:  strings.ToUpper(issue.GetState()),
-		URL:    issue.GetHTMLURL(),
-	}
+	var repository string
 	if owner, repo, ok := parseRepositoryURL(issue.GetRepositoryURL()); ok {
-		ref.Repository = owner + "/" + repo
+		repository = owner + "/" + repo
 	}
-	return ref
+	return newMinimalIssueRef(
+		issue.GetNumber(),
+		issue.GetTitle(),
+		strings.ToUpper(issue.GetState()),
+		issue.GetHTMLURL(),
+		repository,
+	)
 }
 
 // IssueDependencyWrite creates a tool to add or remove an issue dependency