Skip to content

Commit ad127f7

Browse files
tgrunnagleclaude
tgrunnagle
and
claude
authored
Cover Serve-path authz and annotation-enrichment omission (#5482)
* Cover Serve-path authz/annotation omission P2.3 of the vMCP core refactor (epic #5419). The Serve-built *Server must produce the MCP middleware chain WITHOUT the HTTP authz and annotation-enrichment layers — authorization moves to the core admission seam (#5438) — while the still-live server.New path keeps enforcing authz with no regression. The nil-guard mechanism already exists: Serve leaves cfg.AuthzMiddleware nil (buildServeConfig does not map it), so the shared (*Server).Handler guards at server.go:614/:622 skip both blocks. This change locks that contract in and corrects forward-reference comments that the revised plan made stale. - Add TestServeOmitsAuthzAndAnnotation: a Serve-built server has config.AuthzMiddleware == nil and still builds its Handler. - Add TestHandlerAppliesAuthzAndAnnotationOnlyWhenConfigured: the shared Handler applies both layers iff AuthzMiddleware != nil, proving it serves both modes and the blocks are not deleted. - Sync stale #5441 comments: physical removal of the inert blocks moves to Phase 3 (#5445); the (Authz + optimizer) fail-fast moves to #5442 with the core-enforcement switch; discovery-relocation attribution is #5442 only. Physical deletion of the blocks and the optimizer+authz fail-fast are out of scope here (deferred to #5445 and #5442 respectively), per the issue's guard-don't-delete strategy. Closes #5441 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review: drop brittle line numbers in test comment Fixed issues from code review: - MEDIUM: TestServeOmitsAuthzAndAnnotation's doc comment cited server.go:614/:622 for the authz/annotation blocks, but this PR's own chain-comment addition shifted those lines (:614 now points at the backend-enrichment guard). Refer to the blocks by their s.config.AuthzMiddleware != nil guard instead, matching the production comments and the "Keep Comments Synchronized" rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review: comment accuracy and test clarity Addresses #5482 review comments: - LOW core/admission.go (3390209836): align AllowToolCall fail-fast note to future tense — it lands with the core-enforcement switch in #5442 - LOW server/server_test.go (3390209870): set Content-Type so the nil-authz case reaches the chain representatively instead of a content-negotiation 400 - LOW server/server_test.go (3390209877): name discovery's session-scoped sessionless pass-through as the load-bearing dependency (by mechanism) - LOW server/server_test.go (3390209882): drop the goroutine/timeout scaffolding that never fires — both paths return synchronously - INFO server/annotation_enrichment.go (3390209888): note the core admission seam (#5438) reuses the conversion logic, so this copy must stay in lockstep - INFO server/server_test.go (3390209903): note List/Discover mocks stay permissive but do not fire on the session-scoped pass-through path Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 359e998 commit ad127f7

7 files changed

Lines changed: 188 additions & 26 deletions

File tree

‎pkg/vmcp/core/admission.go‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,11 @@ import (
3030
// The optimizer-admission integration the HTTP path performs today — call_tool
3131
// inner-target authorization, find_tool response filtering, and inner-target
3232
// annotation sourcing — is deliberately NOT in this seam; it is deferred to its own
33-
// focused PR. Until then the optimizer keeps the HTTP middleware, and #5441's
34-
// composition root fails fast (vmcp.ErrInvalidConfig) when Authz is set together
35-
// with the optimizer, so the combination can never silently route through this seam.
33+
// focused PR. Until then the optimizer keeps the HTTP middleware, and the composition
34+
// root that wires core.New must fail fast (vmcp.ErrInvalidConfig) when Authz is set
35+
// together with the optimizer, so the combination can never silently route through this
36+
// seam. That fail-fast lands with the core-enforcement switch in #5442 (#5441 keeps the
37+
// legacy server.New path, which retains the HTTP authz middleware).
3638
type Admission interface {
3739
// FilterTools returns the subset of tools the identity may call. Mirrors
3840
// pkg/authz filterToolsByPolicy: a per-tool AuthorizeWithJWTClaims(call) using
@@ -60,8 +62,8 @@ type Admission interface {
6062
// A non-nil authzCfg with zero policies is NOT treated as allow-all here: it falls
6163
// through to CreateAuthorizer, which returns ErrNoPolicies, so the core fails
6264
// CLOSED. This is a deliberate divergence from the live HTTP path, which allows-all
63-
// for the same input. Fail-closed is the safer default; #5441 reconciles the two
64-
// when it collapses the construction paths into one shared constructor.
65+
// for the same input. Fail-closed is the safer default; Phase 3 (#5445) reconciles the
66+
// two when it collapses the construction paths into one shared constructor.
6567
//
6668
// When authzCfg is set, the authorizer is built via the same registry path the
6769
// HTTP middleware uses (authorizers.GetFactory + CreateAuthorizer, the construction
@@ -73,7 +75,7 @@ type Admission interface {
7375
// defaulting an empty EntitiesJSON to "[]") belongs to whoever builds authzCfg, not
7476
// to this generic factory path, which consumes RawConfig as-is. newCedarAuthzMiddleware
7577
// is the sibling construction path; the two must stay in lockstep (notably the
76-
// empty-serverName check below) until #5441 collapses them.
78+
// empty-serverName check below) until Phase 3 (#5445) collapses them.
7779
func newAdmission(authzCfg *authorizers.Config, serverName string) (Admission, error) {
7880
if authzCfg == nil {
7981
return allowAllAdmission{}, nil
@@ -172,8 +174,8 @@ func (a *cedarAdmission) FilterTools(
172174
// the tool's annotations for when-clause evaluation. It authorizes the tool named in
173175
// `tool.Name` — there is no optimizer meta-tool special-casing here (see the
174176
// [Admission] doc): the optimizer's call_tool inner-target authorization is deferred
175-
// to a dedicated optimizer-admission PR, and #5441 fails fast on (Authz + optimizer)
176-
// so that combination never reaches this seam in the meantime.
177+
// to a dedicated optimizer-admission PR; the (Authz + optimizer) fail-fast that keeps that
178+
// combination from reaching this seam lands with the core-enforcement switch in #5442.
177179
func (a *cedarAdmission) AllowToolCall(
178180
ctx context.Context, identity *auth.Identity, tool *vmcp.Tool, args map[string]any,
179181
) (bool, error) {
@@ -295,8 +297,8 @@ func (allowAllAdmission) AllowPromptGet(_ context.Context, _ *auth.Identity, _ *
295297
// than reusing it: that copy lives in package server, which imports this core
296298
// package, so importing it here would create a cycle (server -> core). Like the
297299
// filterHealthyBackends C2 duplication in this package, it is intentional and
298-
// temporary — #5441 retires the server-side middleware (and its copy) on the domain
299-
// path. Keep the two in sync until then.
300+
// temporary — Phase 3 (#5445) retires the server-side middleware (and its copy) on the
301+
// domain path. Keep the two in sync until then.
300302
func convertAnnotations(ann *vmcp.ToolAnnotations) *authorizers.ToolAnnotations {
301303
if ann == nil {
302304
return nil

‎pkg/vmcp/server/annotation_enrichment.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,11 @@ func findToolAnnotations(toolName string, caps *aggregator.AggregatedCapabilitie
9292
//
9393
// Kept identical to core.convertAnnotations (pkg/vmcp/core/admission.go), including
9494
// the nil guard below — the two are intentional, temporary duplicates (the core
95-
// cannot import this package: server imports core) and #5441 deletes this copy.
95+
// cannot import this package: server imports core). #5441 makes this middleware inert
96+
// on the Serve path via the AuthzMiddleware nil-guard; physical removal of this copy
97+
// is deferred to Phase 3 (#5445). Do NOT delete it early on the import-cycle reasoning
98+
// alone: the core admission seam (#5438) reuses the conversion logic, so this copy must
99+
// stay in lockstep with core.convertAnnotations until the middleware itself is retired.
96100
func convertAnnotations(ann *vmcp.ToolAnnotations) *authorizers.ToolAnnotations {
97101
if ann == nil {
98102
return nil

‎pkg/vmcp/server/serve.go‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -144,12 +144,14 @@ type ServerConfig struct {
144144
// .well-known endpoints, and any embedded auth-server routes) when Serve's *Server
145145
// is served.
146146
//
147-
// The remaining transport concerns — the authenticated middleware chain (#5441), the
148-
// direct VMCP request path (#5442), and the AS runner / status reporter / optimizer /
149-
// health monitor lifecycle (#5443) — are relocated under Serve by the subsequent
150-
// Phase 2 tasks. server.New is not yet routed through Serve and keeps its own copy of
151-
// the session wiring until Phase 3, so this is purely additive and observable behavior
152-
// is unchanged.
147+
// The authenticated middleware chain is produced by the shared (*Server).Handler that
148+
// the Serve-built *Server already uses, with the authz and annotation-enrichment layers
149+
// guarded off via a nil AuthzMiddleware (#5441); authorization moves to the core
150+
// admission seam (#5438). The remaining transport concerns — the direct VMCP request
151+
// path (#5442) and the AS runner / status reporter / optimizer / health monitor
152+
// lifecycle (#5443) — are relocated under Serve by the subsequent Phase 2 tasks.
153+
// server.New is not yet routed through Serve and keeps its own copy of the session
154+
// wiring until Phase 3, so this is purely additive and observable behavior is unchanged.
153155
//
154156
// Serve returns a vmcp.ErrInvalidConfig-wrapped error for a nil cfg, a nil core, or a
155157
// nil required collaborator (SessionManagerConfig or BackendRegistry). The session
@@ -158,10 +160,9 @@ type ServerConfig struct {
158160
//
159161
// Contract: the returned *Server now has a live session manager and backend registry,
160162
// but a nil discovery manager, router, and backend client at this phase. It must NOT
161-
// be Start()ed or served on the "/" MCP route until #5441/#5442 wire those fields —
162-
// the carried-forward Handler passes them to discovery.Middleware, which would
163-
// nil-deref. The unauthenticated routes (registered as direct mux entries) are safe
164-
// to serve.
163+
// be Start()ed or served on the "/" MCP route until #5442 wires those fields — the
164+
// carried-forward Handler passes them to discovery.Middleware, which would nil-deref.
165+
// The unauthenticated routes (registered as direct mux entries) are safe to serve.
165166
func Serve(ctx context.Context, v core.VMCP, cfg *ServerConfig) (*Server, error) {
166167
if cfg == nil {
167168
return nil, fmt.Errorf("%w: nil server config", vmcp.ErrInvalidConfig)
@@ -277,8 +278,10 @@ func Serve(ctx context.Context, v core.VMCP, cfg *ServerConfig) (*Server, error)
277278
//
278279
// Several Config fields are deliberately NOT mapped at this phase (see
279280
// TestBuildServeConfigMapsSharedFields, which guards this list against drift):
280-
// - AuthzMiddleware: the authenticated/authz middleware chain is relocated under
281-
// Serve by #5441, after which authorization moves to the core admission seam.
281+
// - AuthzMiddleware: intentionally left nil on the Serve path. The shared
282+
// (*Server).Handler omits both the authz and annotation-enrichment blocks when
283+
// AuthzMiddleware is nil; authorization moves to the core admission seam (#5438).
284+
// The inert blocks stay in the shared Handler until Phase 3 (#5445) removes them.
282285
// - HealthMonitorConfig: Serve receives the already-built *health.Monitor via
283286
// ServerConfig.HealthMonitor (A2) and assigns it to the Server directly, so it
284287
// never needs the monitor's construction config.

‎pkg/vmcp/server/serve_session_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ import (
3333
// These tests exercise the session-creation wiring and SDK hooks relocated into
3434
// Serve (#5440). They drive the SDK session lifecycle through the relocated
3535
// vmcpSessionMgr + mcpServer directly, mounting the Streamable HTTP server WITHOUT
36-
// the authenticated discovery middleware (relocated by #5441/#5442). That keeps the
36+
// the authenticated discovery middleware (relocated by #5442). That keeps the
3737
// test within this task's scope while proving the hooks fire and two-phase session
3838
// creation runs identically when Serve is exercised directly. The full HTTP suite
3939
// stays on server.New (its parity gate) in session_management_integration_test.go.
@@ -110,7 +110,7 @@ func TestServeRegistersSessionHooks(t *testing.T) {
110110
t.Cleanup(func() { _ = srv.Stop(context.Background()) })
111111

112112
// Mount the Streamable HTTP server on the relocated mcpServer + vmcpSessionMgr,
113-
// bypassing the not-yet-relocated discovery middleware (#5441/#5442).
113+
// bypassing the not-yet-relocated discovery middleware (#5442).
114114
streamable := server.NewStreamableHTTPServer(
115115
srv.mcpServer,
116116
server.WithEndpointPath("/mcp"),

‎pkg/vmcp/server/serve_test.go‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,32 @@ func TestServeHandlerRegistersUnauthenticatedRoutes(t *testing.T) {
194194
assert.NotEqual(t, http.StatusOK, rec.Code)
195195
}
196196

197+
// TestServeOmitsAuthzAndAnnotation proves the Serve path produces the MCP middleware
198+
// chain WITHOUT the authz and annotation-enrichment layers. The mechanism is the shared
199+
// (*Server).Handler guard `s.config.AuthzMiddleware != nil`: Serve leaves AuthzMiddleware
200+
// nil (buildServeConfig does not map it — see TestBuildServeConfigMapsSharedFields), so
201+
// both the authz block and the AnnotationEnrichmentMiddleware block — each gated on that
202+
// guard in Handler — are skipped on the Serve path. Authorization instead runs through the core
203+
// admission seam (#5438). The blocks are NOT deleted here — they stay in the shared
204+
// Handler so the still-live server.New path keeps enforcing authz; physical removal is
205+
// Phase 3 (#5445). The companion TestHandlerAppliesAuthzAndAnnotationOnlyWhenConfigured
206+
// proves the same shared Handler DOES apply both layers when AuthzMiddleware is non-nil.
207+
func TestServeOmitsAuthzAndAnnotation(t *testing.T) {
208+
t.Parallel()
209+
210+
srv, err := Serve(context.Background(), &stubVMCP{}, testMinimalServeConfig())
211+
require.NoError(t, err)
212+
t.Cleanup(func() { _ = srv.Stop(context.Background()) })
213+
214+
assert.Nil(t, srv.config.AuthzMiddleware,
215+
"Serve must leave AuthzMiddleware nil so the shared Handler omits authz + annotation-enrichment")
216+
217+
// The Serve path still produces the rest of the chain: Handler builds without error.
218+
handler, err := srv.Handler(context.Background())
219+
require.NoError(t, err)
220+
require.NotNil(t, handler)
221+
}
222+
197223
func TestServeHandlerRegistersMetricsWhenTelemetryEnabled(t *testing.T) {
198224
t.Parallel()
199225

@@ -300,7 +326,7 @@ func TestBuildServeConfigMapsSharedFields(t *testing.T) {
300326
t.Parallel()
301327

302328
intentionallyUnmapped := map[string]struct{}{
303-
"AuthzMiddleware": {}, // authenticated/authz chain relocated by #5441
329+
"AuthzMiddleware": {}, // intentionally nil on Serve path; authz moves to core admission seam (#5438), shared Handler skips it
304330
"HealthMonitorConfig": {}, // monitor injected pre-built via ServerConfig.HealthMonitor (A2)
305331
"StatusReporter": {}, // set directly on Server; Config.StatusReporter only read by New
306332
"SessionFactory": {}, // session manager built in Serve from ServerConfig.SessionManagerConfig

‎pkg/vmcp/server/server.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,13 @@ func (s *Server) Handler(_ context.Context) (http.Handler, error) {
585585
// Execution order: recovery → header-val → auth+parser → audit →
586586
// discovery → annotation-enrichment → authz → backend-enrichment →
587587
// MCP-parsing → telemetry → handler
588+
//
589+
// The authz and annotation-enrichment layers are both guarded by
590+
// s.config.AuthzMiddleware != nil: applied on the server.New path (authz on) and
591+
// omitted on the Serve path, which leaves AuthzMiddleware nil so authorization
592+
// moves to the core admission seam (#5438). Both blocks remain in this shared
593+
// Handler — physical removal is deferred to Phase 3 (#5445), after server.New is
594+
// routed through Serve and the legacy authz path is gone.
588595

589596
var mcpHandler http.Handler = streamableServer
590597

‎pkg/vmcp/server/server_test.go‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,11 @@ import (
1717
"go.uber.org/mock/gomock"
1818

1919
"github.com/stacklok/toolhive/pkg/audit"
20+
"github.com/stacklok/toolhive/pkg/authz/authorizers"
21+
mcpparser "github.com/stacklok/toolhive/pkg/mcp"
2022
"github.com/stacklok/toolhive/pkg/vmcp"
23+
"github.com/stacklok/toolhive/pkg/vmcp/aggregator"
24+
"github.com/stacklok/toolhive/pkg/vmcp/discovery"
2125
discoveryMocks "github.com/stacklok/toolhive/pkg/vmcp/discovery/mocks"
2226
"github.com/stacklok/toolhive/pkg/vmcp/mocks"
2327
"github.com/stacklok/toolhive/pkg/vmcp/optimizer"
@@ -785,3 +789,119 @@ func TestAcceptHeaderValidation(t *testing.T) {
785789
})
786790
}
787791
}
792+
793+
// TestHandlerAppliesAuthzAndAnnotationOnlyWhenConfigured proves the shared
794+
// (*Server).Handler applies BOTH the authz layer and the annotation-enrichment layer
795+
// exactly when config.AuthzMiddleware != nil. This is the single guard that lets the
796+
// same Handler serve both modes: the server.New path (AuthzMiddleware set) keeps
797+
// enforcing authz with no regression, while the Serve path (AuthzMiddleware nil; see
798+
// TestServeOmitsAuthzAndAnnotation) omits both layers and defers authorization to the
799+
// core admission seam (#5438). The blocks are not deleted here — physical removal is
800+
// Phase 3 (#5445).
801+
func TestHandlerAppliesAuthzAndAnnotationOnlyWhenConfigured(t *testing.T) {
802+
t.Parallel()
803+
804+
// Distinctive status only the observable authz layer writes, so its presence in the
805+
// chain is unambiguous.
806+
const sentinelStatus = http.StatusTeapot
807+
808+
readOnly := true
809+
caps := &aggregator.AggregatedCapabilities{
810+
Tools: []vmcp.Tool{{
811+
Name: "my_tool",
812+
Annotations: &vmcp.ToolAnnotations{ReadOnlyHint: &readOnly},
813+
}},
814+
}
815+
816+
tests := []struct {
817+
name string
818+
withAuthz bool
819+
wantAuthzApplied bool
820+
}{
821+
{name: "applied when AuthzMiddleware set (server.New path)", withAuthz: true, wantAuthzApplied: true},
822+
{name: "omitted when AuthzMiddleware nil (Serve path)", withAuthz: false, wantAuthzApplied: false},
823+
}
824+
825+
for _, tt := range tests {
826+
t.Run(tt.name, func(t *testing.T) {
827+
t.Parallel()
828+
829+
ctrl := gomock.NewController(t)
830+
t.Cleanup(ctrl.Finish)
831+
mockRouter := routerMocks.NewMockRouter(ctrl)
832+
mockBackendClient := mocks.NewMockBackendClient(ctrl)
833+
mockDiscoveryMgr := discoveryMocks.NewMockManager(ctrl)
834+
mockBackendRegistry := mocks.NewMockBackendRegistry(ctrl)
835+
// List/Discover stay permissive (AnyTimes) but do not fire on this path: with
836+
// WithSessionScopedRouting() and no Mcp-Session-Id, discovery.Middleware returns
837+
// before aggregating. Stop fires via srv.Stop in the cleanup below.
838+
mockBackendRegistry.EXPECT().List(gomock.Any()).Return(nil).AnyTimes()
839+
mockDiscoveryMgr.EXPECT().Discover(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
840+
mockDiscoveryMgr.EXPECT().Stop().AnyTimes()
841+
842+
var (
843+
authzApplied bool
844+
annotationsSeen bool
845+
)
846+
// Observable authz layer: records that it ran AND whether the
847+
// annotation-enrichment layer (which executes immediately before it) injected
848+
// the tool's annotations into ctx, then short-circuits with the sentinel.
849+
authz := func(_ http.Handler) http.Handler {
850+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
851+
authzApplied = true
852+
annotationsSeen = authorizers.ToolAnnotationsFromContext(r.Context()) != nil
853+
w.WriteHeader(sentinelStatus)
854+
})
855+
}
856+
857+
cfg := &server.Config{Host: "127.0.0.1", Port: 0, SessionFactory: newNoopMockFactory(t)}
858+
if tt.withAuthz {
859+
cfg.AuthzMiddleware = authz
860+
}
861+
862+
srv, err := server.New(t.Context(), cfg, mockRouter, mockBackendClient, mockDiscoveryMgr, mockBackendRegistry, nil)
863+
require.NoError(t, err)
864+
t.Cleanup(func() { _ = srv.Stop(context.Background()) })
865+
866+
handler, err := srv.Handler(t.Context())
867+
require.NoError(t, err)
868+
869+
// Craft a tools/call request. This chain has no auth-parser, so inject the
870+
// parsed request and discovered capabilities directly into ctx (as
871+
// ParsingMiddleware and the discovery middleware would).
872+
//
873+
// Load-bearing dependency: discovery.Middleware is built with
874+
// WithSessionScopedRouting(), so a request with no Mcp-Session-Id passes straight
875+
// through without touching the (mock) manager and WITHOUT rewriting ctx —
876+
// preserving the injected capabilities for the annotation-enrichment layer. If
877+
// that session-scoped pass-through ever changes to overwrite ctx, annotationsSeen
878+
// silently goes false; the positive-case assert.True below is the guard.
879+
ctx := context.WithValue(t.Context(), mcpparser.MCPRequestContextKey,
880+
&mcpparser.ParsedMCPRequest{Method: "tools/call", ResourceID: "my_tool"})
881+
ctx = discovery.WithDiscoveredCapabilities(ctx, caps)
882+
883+
req := httptest.NewRequest(http.MethodPost, "/mcp", nil).WithContext(ctx)
884+
// Set Content-Type so the nil-authz request reaches a representative chain point
885+
// (the inner SDK handler) rather than being rejected during content negotiation;
886+
// both cases then exercise the chain identically.
887+
req.Header.Set("Content-Type", "application/json")
888+
rec := httptest.NewRecorder()
889+
890+
// Both paths return synchronously: the configured case short-circuits at the
891+
// observable authz layer (sentinel), and the nil case falls through to the inner
892+
// SDK handler, which answers this POST without blocking. A direct call suffices —
893+
// no goroutine/timeout scaffolding needed.
894+
handler.ServeHTTP(rec, req)
895+
896+
assert.Equal(t, tt.wantAuthzApplied, authzApplied)
897+
if tt.wantAuthzApplied {
898+
assert.Equal(t, sentinelStatus, rec.Code, "observable authz layer should have written the sentinel status")
899+
assert.True(t, annotationsSeen,
900+
"annotation-enrichment must run before authz and inject the tool annotations")
901+
} else {
902+
assert.NotEqual(t, sentinelStatus, rec.Code,
903+
"no authz layer should run on the Serve-style (nil AuthzMiddleware) path")
904+
}
905+
})
906+
}
907+
}

0 commit comments

Comments
 (0)