fix(http): make server lockdown mode an upper bound over requests (#3112)

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/http/transport/bearer_test.go

@@ -58,7 +58,7 @@ func TestBearerAuthTransport(t *testing.T) {
 			defer server.Close()
 
 			rt := &BearerAuthTransport{
-				Transport:     http.DefaultTransport,
+				Transport:     newIsolatedTransport(t),
 				Token:         tc.token,
 				TokenProvider: tc.tokenProvider,
 			}
@@ -91,7 +91,7 @@ func TestBearerAuthTransport_TokenProviderResolvedPerRequest(t *testing.T) {
 
 	current := ""
 	rt := &BearerAuthTransport{
-		Transport:     http.DefaultTransport,
+		Transport:     newIsolatedTransport(t),
 		TokenProvider: func() string { return current },
 	}
 
@@ -126,7 +126,7 @@ func TestBearerAuthTransport_PassesGraphQLFeaturesHeader(t *testing.T) {
 	defer server.Close()
 
 	rt := &BearerAuthTransport{
-		Transport: http.DefaultTransport,
+		Transport: newIsolatedTransport(t),
 		Token:     "token",
 	}
 
@@ -150,7 +150,7 @@ func TestBearerAuthTransport_DoesNotMutateOriginalRequest(t *testing.T) {
 	defer server.Close()
 
 	rt := &BearerAuthTransport{
-		Transport: http.DefaultTransport,
+		Transport: newIsolatedTransport(t),
 		Token:     "token",
 	}
 
@@ -356,7 +356,7 @@ func TestBearerAuthTransport_RedirectHostScoping(t *testing.T) {
 	require.NoError(t, err)
 
 	client := &http.Client{Transport: &BearerAuthTransport{
-		Transport:    http.DefaultTransport,
+		Transport:    newIsolatedTransport(t),
 		Token:        "secret-token",
 		AllowedHosts: []string{sourceURL.Host, allowedTargetURL.Host},
 	}}

pkg/http/transport/graphql_features_test.go

@@ -65,7 +65,7 @@ func TestGraphQLFeaturesTransport(t *testing.T) {
 
 			// Create the transport
 			transport := &GraphQLFeaturesTransport{
-				Transport: http.DefaultTransport,
+				Transport: newIsolatedTransport(t),
 			}
 
 			// Create a request
@@ -91,9 +91,10 @@ func TestGraphQLFeaturesTransport(t *testing.T) {
 	}
 }
 
+// TestGraphQLFeaturesTransport_NilTransport exercises the real
+// http.DefaultTransport fallback, so it can't run in parallel with tests that
+// close their own servers (that closes DefaultTransport's idle conns too).
 func TestGraphQLFeaturesTransport_NilTransport(t *testing.T) {
-	t.Parallel()
-
 	var capturedHeader string
 
 	// Create a test server
@@ -133,7 +134,7 @@ func TestGraphQLFeaturesTransport_DoesNotMutateOriginalRequest(t *testing.T) {
 
 	// Create the transport
 	transport := &GraphQLFeaturesTransport{
-		Transport: http.DefaultTransport,
+		Transport: newIsolatedTransport(t),
 	}
 
 	// Create a request with features

pkg/http/transport/helpers_test.go

@@ -0,0 +1,20 @@
+package transport
+
+import (
+	"net/http"
+	"testing"
+)
+
+// newIsolatedTransport returns an http.Transport owned by a single test.
+//
+// Sharing http.DefaultTransport across parallel tests is unsafe: closing one
+// test's httptest.Server also closes DefaultTransport's idle connections,
+// breaking other tests still using it. Tests asserting DefaultTransport
+// fallback behavior specifically must use it directly and not run in parallel.
+func newIsolatedTransport(t *testing.T) *http.Transport {
+	t.Helper()
+
+	transport := &http.Transport{}
+	t.Cleanup(transport.CloseIdleConnections)
+	return transport
+}