fix(http): make server lockdown mode an upper bound over requests - #3112
Merged
SamMorrowDrums merged 4 commits intoAug 19, 2026
Conversation
In HTTP mode, RequestDeps.GetFlags previously combined the server operator's --lockdown-mode / GITHUB_LOCKDOWN_MODE configuration with the per-request X-MCP-Lockdown header using AND logic. This meant a request that omitted the header silently disabled lockdown mode even when the operator had explicitly enabled it server-wide. Change the combination to OR (server-enabled OR request-enabled), so server configuration is an upper bound: the operator's setting can never be relaxed by a request, while a request may still opt itself into lockdown mode when the operator has not already enabled it. This mirrors the existing upper-bound pattern already used for read-only mode in HTTP mode. Also align GetRepoAccessCache's cache-construction condition with the same effective-lockdown check so request-only lockdown mode has a working cache instead of failing closed with a "lockdown cache is not configured" error. Add focused tests covering server-only, request-only, both, and neither lockdown configurations, plus a regression test for the specific bug (server-enabled lockdown surviving a request that omits the header). Update README and the server-configuration/remote-server docs to describe the upper-bound semantics and reiterate that lockdown mode is a best-effort content filter, not a security boundary. Fixes #3104 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes HTTP lockdown precedence so requests cannot relax server-enforced lockdown.
Changes:
- Uses effective lockdown (
server || request) for flags and cache creation. - Adds coverage for all configuration combinations and the regression.
- Documents precedence and best-effort filtering semantics.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents lockdown precedence and limitations. |
pkg/github/dependencies.go |
Centralizes effective lockdown logic. |
pkg/github/dependencies_test.go |
Tests precedence and cache creation. |
docs/server-configuration.md |
Explains HTTP configuration semantics. |
docs/remote-server.md |
Clarifies header behavior. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
…ests httptest.Server.Close closes the idle connections of the process-global http.DefaultTransport, whichever server is being shut down. Every test in this package runs in parallel, issues a live request through http.DefaultTransport and shuts down its own server, so one test's cleanup could break another test's in-flight request with: net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called That flake failed build (ubuntu-latest) on this branch, and reproduces on unmodified main under `go test ./pkg/http/transport -race -count=800`, hitting both bearer_test.go and graphql_features_test.go. It is unrelated to the lockdown change in this PR. Give each test its own http.Transport so the global side effect cannot reach it. TestGraphQLFeaturesTransport_NilTransport asserts the documented nil to http.DefaultTransport fallback, so it keeps using that global and now runs serially instead. Assertions and coverage are unchanged. The same stress run that reproduced the failure now passes 2500 iterations under -race. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Brings in 769340d, which added a paragraph describing lockdown mode as a best-effort content filter rather than an authorization boundary. Git merged the docs cleanly but the result was semantically redundant: this branch had appended "Lockdown mode is a best-effort content filter, not a security boundary." to the paragraph immediately preceding the new one, so the same point was made twice in a row in both README.md and docs/server-configuration.md. Dropped the appended sentence in those two places; the incoming paragraph states it more fully. The composability note in docs/server-configuration.md keeps its mention, as it has no adjacent duplicate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Condense multi-line narration into short, focused comments. Keep only the non-obvious invariants (lockdown precedence, isolated transport rationale) and drop restated code/step-by-step prose. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
SamMorrowDrums
deleted the
sammorrowdrums-lockdown-mode-upper-bound-http
branch
August 19, 2026 13:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In HTTP mode, lockdown mode was only active when both the server operator's
--lockdown-mode/GITHUB_LOCKDOWN_MODEconfiguration and the per-requestX-MCP-Lockdownheader enabled it (ANDlogic). This let any request that simply omitted the header silently disable lockdown mode, even when the operator had explicitly enabled it server-wide.This PR changes the combination to
ORlogic:serverEnabled || requestEnabled. Server configuration is now an upper bound — the operator's setting can never be relaxed by a request — while a request may still opt itself into lockdown mode when the operator has not already enabled it. This mirrors the existing upper-bound pattern already used for read-only mode in HTTP mode (pkg/http/handler.go: "Static read-only is an upper bound — enforce before request filters").RequestDeps.GetRepoAccessCache's cache-construction condition was also aligned with the same effective-lockdown check, so request-only lockdown mode gets a working cache instead of failing closed with a"lockdown cache is not configured"error.Lockdown mode remains a best-effort content filter, not a security boundary — this change only fixes the activation/precedence semantics, not the filtering guarantees.
Changes
pkg/github/dependencies.go: addedRequestDeps.effectiveLockdownMode(ctx)helper (d.lockdownMode || ghcontext.IsLockdownMode(ctx)), used by bothGetFlagsandGetRepoAccessCache.pkg/github/dependencies_test.go: added focused tests for the four configuration combinations (neither / server-only / request-only / both) plus a dedicated regression test for the reported bug (server-enabled lockdown surviving a request that omits the header).README.md,docs/server-configuration.md,docs/remote-server.md): documented the new upper-bound semantics and reiterated that lockdown mode is a best-effort content filter, not a security boundary.Testing
script/lint— 0 issuesscript/test— all packages passTestRequestDepsLockdownModeIsUpperBound(table-driven over all 4 combos) andTestRequestDepsLockdownModeCannotBeDisabledByOmittingHeader(direct regression test)Fixes #3104