diff --git a/internal/auth/acl/scope_test.go b/internal/auth/acl/scope_test.go index 3f54aa242..1b1087065 100644 --- a/internal/auth/acl/scope_test.go +++ b/internal/auth/acl/scope_test.go @@ -81,6 +81,14 @@ func TestScopePermits(t *testing.T) { assert.True(t, ScopePermits("read mcp", ResourceMCP, Permissions{ActionView})) assert.False(t, ScopePermits("read mcp", ResourceMCP, Permissions{ActionUpdate})) assert.False(t, ScopePermits("metrics", ResourceMCP, Permissions{ActionView})) + // "mcp:read" is NOT an alias for "mcp read" - KeyValue.Parse treats + // it as {key=mcp, value=read}, which Attr.Find("mcp") never matches + // because the resource lookup expects a {mcp,true} entry. This + // regression assertion locks in the documented behavior in + // specs/platform/acl-scopes.md so the parser does not silently + // start accepting the colon form. + assert.False(t, ScopePermits("mcp:read", ResourceMCP, Permissions{ActionView})) + assert.False(t, ScopePermits("mcp:read", ResourceMCP, Permissions{ActionUpdate})) }) } diff --git a/internal/commands/mcp.go b/internal/commands/mcp.go index c1ebfdb94..cceafbc81 100644 --- a/internal/commands/mcp.go +++ b/internal/commands/mcp.go @@ -2,12 +2,14 @@ package commands import ( "context" + "errors" "log/slog" "os" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/urfave/cli/v2" + "github.com/photoprism/photoprism/internal/config" "github.com/photoprism/photoprism/internal/mcp" ) @@ -29,8 +31,22 @@ var MCPServeCommand = &cli.Command{ // mcpServeAction starts the MCP server over the stdio transport, // writing a startup line to stderr so the JSON-RPC stream on stdout -// stays clean for the MCP client. +// stays clean for the MCP client. The action loads the active config +// and refuses to start when DisableMCP is set, mirroring how the +// HTTP transport at /api/v1/mcp is gated; pass --disable-mcp=false on +// the command line to override the operator setting for ad-hoc +// development runs. func mcpServeAction(ctx *cli.Context) error { + conf, err := InitConfig(ctx) + + if err != nil { + return err + } + + if err = mcpDisabledExit(conf); err != nil { + return err + } + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelInfo})) logger.Info("starting mcp server", "transport", "stdio", @@ -41,6 +57,17 @@ func mcpServeAction(ctx *cli.Context) error { return runMCPServer(context.Background(), ctx, &sdkmcp.StdioTransport{}) } +// mcpDisabledExit returns a cli.ExitCoder error when MCP is disabled +// by config, pointing the caller at --disable-mcp=false for ad-hoc +// development runs. It returns nil when MCP is enabled. +func mcpDisabledExit(conf *config.Config) error { + if conf == nil || !conf.DisableMCP() { + return nil + } + + return cli.Exit(errors.New("mcp serve disabled by config; pass --disable-mcp=false to override"), 1) +} + // runMCPServer builds an MCP server from the CLI app metadata (Version, // Edition) and runs it over the given transport until ctx is canceled or // the transport closes. The transport is a parameter so tests can diff --git a/internal/commands/mcp_test.go b/internal/commands/mcp_test.go index fc537c80e..96e2119be 100644 --- a/internal/commands/mcp_test.go +++ b/internal/commands/mcp_test.go @@ -10,6 +10,8 @@ import ( sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/require" "github.com/urfave/cli/v2" + + "github.com/photoprism/photoprism/internal/photoprism/get" ) // TestMCPCommandRegistered ensures the MCP command is present in the CLI catalog. @@ -85,6 +87,65 @@ func TestRunMCPServerOverInMemoryTransport(t *testing.T) { } } +// TestMCPDisabledExit covers the gate helper that mcpServeAction calls +// before starting the stdio transport: DisableMCP=true must return a +// cli.ExitCoder error whose message points at --disable-mcp=false as +// the documented override; DisableMCP=false must return nil so the +// server can start; nil config must be tolerated. +func TestMCPDisabledExit(t *testing.T) { + conf := get.Config() + require.NotNil(t, conf, "test harness must provide a shared config") + + original := conf.Options().DisableMCP + t.Cleanup(func() { conf.Options().DisableMCP = original }) + + t.Run("DisableMCPTrue_Refuses", func(t *testing.T) { + conf.Options().DisableMCP = true + + err := mcpDisabledExit(conf) + require.Error(t, err) + + coder, ok := err.(cli.ExitCoder) + require.True(t, ok, "expected cli.ExitCoder, got %T", err) + require.Equal(t, 1, coder.ExitCode()) + require.Contains(t, err.Error(), "--disable-mcp=false") + }) + + t.Run("DisableMCPFalse_Allows", func(t *testing.T) { + conf.Options().DisableMCP = false + + require.NoError(t, mcpDisabledExit(conf)) + }) + + t.Run("NilConfig_Allows", func(t *testing.T) { + require.NoError(t, mcpDisabledExit(nil)) + }) +} + +// TestMCPServeRefusesWhenDisabled drives the full mcp serve action through +// RunWithTestContext to confirm the gate trips end-to-end before the stdio +// transport is started. The DisableMCP=false branch is not exercised here +// because mcpServeAction would block on stdin once it reaches the transport; +// the gate-level coverage in TestMCPDisabledExit is sufficient to assert the +// pre-transport branch. +func TestMCPServeRefusesWhenDisabled(t *testing.T) { + conf := get.Config() + require.NotNil(t, conf, "test harness must provide a shared config") + + original := conf.Options().DisableMCP + t.Cleanup(func() { conf.Options().DisableMCP = original }) + + conf.Options().DisableMCP = true + + _, err := RunWithTestContext(MCPServeCommand, []string{"serve"}) + require.Error(t, err) + + coder, ok := err.(cli.ExitCoder) + require.True(t, ok, "expected cli.ExitCoder, got %T", err) + require.Equal(t, 1, coder.ExitCode()) + require.Contains(t, err.Error(), "--disable-mcp=false") +} + // TestMCPAppMetadata covers the happy path and every fallback branch of // mcpAppMetadata: missing key, non-string value, empty-string value, and // nil metadata map. diff --git a/internal/mcp/README.md b/internal/mcp/README.md index 91787edee..19ef62dc3 100644 --- a/internal/mcp/README.md +++ b/internal/mcp/README.md @@ -6,7 +6,7 @@ - **Transports:** - CLI: `photoprism mcp serve` (stdio, no auth; development and testing). The static-data contract documented under *Authorization* and *Extending the Tool Surface* below applies — write-capable tools MUST require a session token even on stdio. - - HTTP: `POST/GET/DELETE /api/v1/mcp` (Streamable HTTP, authenticated). Can be disabled via `--disable-mcp` / `PHOTOPRISM_DISABLE_MCP` / `DisableMCP` so the route responds with the standard 404 when an operator does not want the endpoint exposed. The flag is also surfaced to the frontend through `ClientConfig.Disable.MCP` (`disable.mcp`), letting the UI hide MCP-related controls while the endpoint is off. + - HTTP: `POST/GET/DELETE /api/v1/mcp` (Streamable HTTP, authenticated). Disabled together with the stdio transport via `--disable-mcp` / `PHOTOPRISM_DISABLE_MCP` / `DisableMCP`: the HTTP route responds with the standard 404 and `photoprism mcp serve` refuses to start (pass `--disable-mcp=false` on the command line to override for ad-hoc development runs). The flag is also surfaced to the frontend through `ClientConfig.Disable.MCP` (`disable.mcp`), letting the UI hide MCP-related controls while the endpoint is off. - **Authorization:** HTTP endpoint enforces the `ResourceMCP` ACL (admin plus the API client roles in every edition, manager in Pro/Portal); anonymous access is permitted in public mode for the currently registered read-only tools. - **Request Body Cap:** HTTP POST bodies are bounded at `MaxMCPRequestBytes` (currently `MaxMutationRequestBytes`, 256 KiB). Oversized requests receive the standard `413 Request Entity Too Large` response before the upstream SDK reads the body. Early rejection via `Content-Length` protects against large known-size payloads; `http.MaxBytesReader` plus a response-writer wrapper handle chunked bodies. The wrapper translates the SDK's internal `400 "failed to read body"` into a consistent `413` and suppresses the SDK's error phrasing so it does not leak to clients. - **Session Timeout:** Streamable HTTP sessions idle out after `McpSessionTimeout` (5 minutes by default). Active clients renew the idle timer on every JSON-RPC request, so interactive IDE use is unaffected; sessions abandoned without the `DELETE` tear-down free up promptly instead of lingering. @@ -62,6 +62,12 @@ Start the MCP server over stdio: The process waits for an MCP client on stdin/stdout. Logs are written to stderr so the MCP message stream stays valid. +When the running config has `DisableMCP=true` (via `options.yml`, `PHOTOPRISM_DISABLE_MCP`, or `--disable-mcp`), the command refuses to start with `mcp serve disabled by config; pass --disable-mcp=false to override`. Pass the override on the command line for ad-hoc development runs without flipping the operator-facing setting: + +```bash +./photoprism --disable-mcp=false mcp serve +``` + ### Run via HTTP Start PhotoPrism: