diff --git a/CHANGELOG.md b/CHANGELOG.md index 6547917..bc9652b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +This release makes the server enforce the MCP specification by default. Several behaviors +that were previously absent or lenient are now always on; see Changed for the breaking +details and how to adapt. + ### Added - Session lifecycle limits: `:max_sessions` (reject new sessions with `503` past a cap — @@ -19,19 +23,71 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Declarative tool scopes: `tool "name", scopes: ["files:write"], ...` enforces the scopes against `ctx.auth` before the handler runs, failing closed when the request carries no authorization. -- `:validate_arguments` transport option (default `false`) validates `tools/call` arguments - against each DSL tool's `input_schema` and rejects a mismatch with `invalid_params` before - the handler runs. `Urchin.Schema` implements the supported (minimal) JSON Schema subset. - `:expose_internal_errors` transport option (default `false`). Unexpected exceptions and malformed handler returns are now logged in full but return a generic message to the client; enable the option to surface the detail in development. Deliberate `Urchin.Error` - values and `{:error, message}` returns still pass through unchanged. + values and `{:error, message}` returns are never redacted — their `message`/`data` reach the + client unchanged. - Capability guards: `Urchin.Context.create_message/3`, `elicit/3` and `list_roots/2` return an error without contacting the client when it did not advertise the matching `sampling`/`elicitation`/`roots` capability. - `415 Unsupported Media Type` for POST requests whose `Content-Type` is not `application/json`. - `SECURITY.md` with a threat model, deployment checklist and vulnerability reporting. +- `:sse_buffer_limit` transport option (default `nil`, preserving the session's internal + default of `100`) forwarding the per-session GET-stream replay buffer size to the session; + previously only configurable on `Urchin.Session` directly. + +### Changed + +The following are now enforced by default, with no opt-out, for MCP spec compliance. They are +breaking relative to `0.2.0`. + +- A DSL tool's `tools/call` arguments are validated against its `input_schema` before the + handler runs; a mismatch is returned as a `CallToolResult` with `isError: true` so the model + can self-correct. A tool that declares no `input_schema` now defaults to an object that + accepts no properties (`additionalProperties: false`), so unexpected arguments are rejected — + declare an explicit `input_schema` to accept arbitrary fields. A non-object `arguments` value + is a malformed request and remains a JSON-RPC `invalid_params` error. Servers that implement + `call_tool/3` by hand validate their own arguments. `Urchin.Schema` implements the supported + (minimal) JSON Schema subset. +- Operation requests received before the client sends `notifications/initialized` are rejected + with `invalid_request`; only `ping` is allowed before initialization (the lifecycle's + pings-and-logging exception is for the server's own requests, not the client's + `logging/setLevel`). Clients must complete the lifecycle handshake before issuing other + requests. +- A `tools/call` handler's `{:error, message}` (string) is returned as a `CallToolResult` with + `isError: true` so the model can self-correct. A protocol error returned as + `{:error, %Urchin.Error{}}` is always a JSON-RPC error. (Previously a string handler error + became a JSON-RPC internal error.) +- Duplicate literal tool names within a server are rejected at compile time (a silently shadowed + duplicate was previously accepted, with the last declaration winning); non-literal names (a + variable or expression) cannot be compared statically and are not checked. Urchin enforces no + tool-name pattern (the MCP schema imposes none); servers should still follow the MCP naming + recommendations. +- `initialize` requires `protocolVersion` (string), `capabilities` (object) and `clientInfo` + (with a string `name` and `version`); a missing or mistyped field is an `invalid_params` + error rather than a silently-defaulted value. The server's `serverInfo` must likewise carry a + string `name` and `version`. +- The `MCP-Protocol-Version` header is validated on `DELETE`, matching `POST` and `GET`. +- `completion/complete` request params are validated (`ref` as a `ref/prompt`/`ref/resource` + union, `argument.name`/`value` as strings, `context.arguments` values as strings) and return + `invalid_params` when malformed. Results are capped at 100 values — a handler returning more is + truncated to the top 100 (already ranked by relevance) with `hasMore` set — and a + non-conforming result shape (non-string `values`, etc.) is an internal error. +- A tool's `input_schema` and `output_schema` must be JSON Schema objects whose root `type` is + `"object"` (per the MCP tools spec); the DSL rejects a non-conforming schema at compile time. +- `logging/setLevel` is now a library builtin: when the server advertises the `logging` + capability (via `use Urchin.Server, logging: true`) it succeeds and applies the level to the + session even without a `set_log_level/2` callback. The level is validated against the MCP log + levels (`invalid_params` otherwise), an exported `set_log_level/2` still runs as a hook, and + the session level is updated only after the hook succeeds. Servers that do not advertise + `logging` return `method_not_found`. + +### Fixed + +- README no longer claims unqualified "resumable SSE streams"; resumption is scoped to the + GET stream, matching the implementation. ## [0.2.0] - 2026-06-05 diff --git a/README.md b/README.md index 8812934..a7297d7 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ specification over the **Streamable HTTP** transport. - Mount as a `Plug` into Phoenix/Plug pipelines, or run standalone with Bandit. - Tools, resources, resource templates, prompts, completion and logging. - Server-initiated requests over SSE: sampling, elicitation and roots. -- Progress notifications, cancellation, pagination and resumable SSE streams. +- Progress notifications, cancellation, pagination and a resumable GET SSE stream. - Optional OAuth 2.1 authorization: RFC 9728 discovery and pluggable token validation. > This library implements the server side only. The stdio transport is intentionally @@ -287,7 +287,10 @@ tool "delete", description: "Delete a file" do if Urchin.Auth.Claims.has_scope?(Urchin.Context.auth(ctx), "files:write") do {:ok, [Urchin.Content.text("deleted")]} else - {:error, "files:write scope required"} + # Return an Urchin.Error so the denial is a JSON-RPC `invalid_request`, matching the + # declarative `scopes:` path. A bare string `{:error, "..."}` would instead surface as a + # `CallToolResult` with `isError: true`. + {:error, Urchin.Error.invalid_request("files:write scope required")} end end ``` @@ -344,7 +347,7 @@ Passed to `Urchin.Transport.StreamableHTTP`, `Urchin.Endpoint` or `Urchin.start_ | `:request_timeout` | `60_000` | per-request handler timeout (ms) | | `:validate_protocol_version` | `true` | validate the `MCP-Protocol-Version` header | | `:expose_internal_errors` | `false` | return raised-exception messages to the client (dev only); exceptions are always logged | -| `:validate_arguments` | `false` | validate `tools/call` arguments against each tool's `input_schema` (see `Urchin.Schema`) | +| `:sse_buffer_limit` | `nil` | max recent GET-stream (general SSE) events kept per session for resumption replay (`nil` keeps the session default of `100`) | | `:max_sessions` | `nil` | reject new sessions with `503` past this many, atomically and before the server's `init/1` runs (`nil` = unlimited) | | `:session_idle_timeout` | `nil` | terminate a session after this many ms without client activity; a session serving a request is not reaped (`nil` = never) | | `:session_max_lifetime` | `nil` | terminate a session this many ms after creation regardless of activity; set above your longest tool run (`nil` = never) | @@ -352,6 +355,14 @@ Passed to `Urchin.Transport.StreamableHTTP`, `Urchin.Endpoint` or `Urchin.start_ `Urchin.Endpoint`/`Urchin.start_link/2` additionally accept `:port`, `:ip`, `:scheme` and `:path`. +Some MCP behaviors are enforced unconditionally and have no option: a DSL tool's `tools/call` +arguments are validated against its `input_schema` (a mismatch is an `isError` `CallToolResult`; a +tool with no schema accepts no properties — servers that implement `call_tool/3` by hand validate +their own arguments); operation requests before `notifications/initialized` are rejected (`ping` +excepted); a `tools/call` handler's `{:error, binary}` is returned as an `isError` +`CallToolResult`; duplicate literal tool names are rejected at compile time; and +`completion/complete` results are capped at 100 values. + ## Specification coverage | Area | Methods | @@ -370,7 +381,8 @@ The transport implements: a single endpoint serving POST/GET/DELETE, the JSON-vs-SSE response decision, `202 Accepted` for notifications and responses, `Origin` validation, `MCP-Session-Id` management, the `MCP-Protocol-Version` header, SSE priming events, per-stream event ids, and `Last-Event-ID` resumption of the GET -stream. +stream. Urchin currently replays the GET general stream only; POST request streams are +not replayed (the spec permits, but does not require, replaying either). ### Not included diff --git a/SECURITY.md b/SECURITY.md index 853b7df..a5af851 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -17,8 +17,10 @@ and does not protect against, and what you must add before exposing a server pub - **Error redaction.** Unexpected exceptions and malformed handler returns are logged in full and replaced with a generic message before reaching clients (`:expose_internal_errors`, default `false`, opts into the detail for development). Deliberate errors — `Urchin.Error` - values and `{:error, message}` returns — pass through unchanged, so keep secrets and - internals out of their `message`/`data`. + values and `{:error, message}` returns — are not redacted; their `message`/`data` reach the + client unchanged, so keep secrets and internals out of them. For `tools/call`, a string + `{:error, message}` is surfaced as a `CallToolResult` with `isError: true` rather than a + JSON-RPC error. - **Capability-gated server-initiated requests.** `sampling/createMessage`, `elicitation/create` and `roots/list` are only sent when the client advertised the capability. @@ -26,10 +28,10 @@ and does not protect against, and what you must add before exposing a server pub `ctx.auth` before the handler runs, failing closed when the request carries no authorization (only meaningful when `ctx.auth` is populated, typically by `:auth` or an upstream `Urchin.Auth.Plug`). -- **Opt-in argument validation.** `:validate_arguments` checks `tools/call` arguments - against each tool's `input_schema`. It is a minimal subset of JSON Schema (see - `Urchin.Schema`), so unsupported keywords and `output_schema` are still your handler's - responsibility. +- **Argument validation.** A DSL tool's `tools/call` arguments are validated against its + `input_schema` before the handler runs (a hand-written `call_tool/3` validates its own + arguments). It is a minimal subset of JSON Schema (see `Urchin.Schema`), so unsupported + keywords and `output_schema` are still your handler's responsibility. - **Bounded request bodies** (`@max_body`, ~8 MB) and a per-request handler timeout. - **Session lifecycle limits** (opt-in): `:max_sessions`, `:session_idle_timeout` and `:session_max_lifetime`. Without them a session persists until the client sends `DELETE`, @@ -49,7 +51,7 @@ Urchin does **not** yet provide these; supply them in your deployment: 4. **Per-tool authorization beyond scopes.** Declarative `scopes:` covers scope checks; add app-specific authorization (ownership, tenancy, row-level access) in handlers via `ctx.auth`. -5. **Full input validation.** Enable `:validate_arguments` for structural checks, but +5. **Full input validation.** Structural checks against `input_schema` run automatically, but validate unsupported JSON Schema keywords, business rules and `output_schema` in your handler — `Urchin.Schema` is a minimal subset. diff --git a/lib/urchin/context.ex b/lib/urchin/context.ex index a6cb0db..d10c051 100644 --- a/lib/urchin/context.ex +++ b/lib/urchin/context.ex @@ -34,7 +34,7 @@ defmodule Urchin.Context do assigns: %{}, min_log_level: "debug", expose_internal_errors: false, - validate_arguments: false, + initialized: false, cancelled_ref: nil ] @@ -53,12 +53,16 @@ defmodule Urchin.Context do assigns: map(), min_log_level: String.t(), expose_internal_errors: boolean(), - validate_arguments: boolean(), + initialized: boolean(), cancelled_ref: reference() | nil } @default_request_timeout 30_000 + @doc "Returns the valid MCP log levels, in increasing severity order." + @spec log_levels() :: [String.t()] + def log_levels, do: @log_levels + @doc "Returns the user state established by `c:Urchin.Server.init/1`." @spec state(t()) :: term() def state(%__MODULE__{state: state}), do: state diff --git a/lib/urchin/dispatcher.ex b/lib/urchin/dispatcher.ex index 338c90a..65cb8d2 100644 --- a/lib/urchin/dispatcher.ex +++ b/lib/urchin/dispatcher.ex @@ -10,7 +10,7 @@ defmodule Urchin.Dispatcher do require Logger - alias Urchin.{Context, Error, Protocol, Result} + alias Urchin.{Context, Error, Protocol, Result, Session} @doc """ Handles an `initialize` request. @@ -22,24 +22,25 @@ defmodule Urchin.Dispatcher do @spec initialize(module(), map(), Context.t()) :: {:ok, map(), map()} | {:error, Error.t()} def initialize(server, params, ctx) when is_map(params) do - requested = Map.get(params, "protocolVersion", Protocol.latest_version()) - negotiated = Protocol.negotiate(requested) - - result = - %{ - protocolVersion: negotiated, - capabilities: capabilities(server), - serverInfo: server.server_info() + with :ok <- validate_initialize_params(params) do + negotiated = Protocol.negotiate(Map.fetch!(params, "protocolVersion")) + + result = + %{ + protocolVersion: negotiated, + capabilities: capabilities(server), + serverInfo: validated_server_info(server) + } + |> maybe_put(:instructions, instructions(server)) + + meta = %{ + protocol_version: negotiated, + client_info: Map.fetch!(params, "clientInfo"), + client_capabilities: Map.fetch!(params, "capabilities") } - |> maybe_put(:instructions, instructions(server)) - meta = %{ - protocol_version: negotiated, - client_info: Map.get(params, "clientInfo"), - client_capabilities: Map.get(params, "capabilities", %{}) - } - - {:ok, result, meta} + {:ok, result, meta} + end rescue error in Urchin.Error -> {:error, error} @@ -52,6 +53,53 @@ defmodule Urchin.Dispatcher do {:error, Error.invalid_params("initialize params must be an object")} end + # The client MUST send protocolVersion (string), capabilities (object) and clientInfo (with a + # string name and version) in the initialize request; a missing or mistyped field is a protocol + # error rather than a silently-defaulted value. + defp validate_initialize_params(params) do + with :ok <- ensure_string(params, "protocolVersion"), + :ok <- ensure_object(params, "capabilities"), + :ok <- ensure_object(params, "clientInfo") do + validate_client_info(Map.fetch!(params, "clientInfo")) + end + end + + defp validate_client_info(info) do + if is_binary(info[:name] || info["name"]) and is_binary(info[:version] || info["version"]) do + :ok + else + {:error, Error.invalid_params("initialize: clientInfo requires a string name and version")} + end + end + + defp ensure_string(params, key) do + case Map.get(params, key) do + value when is_binary(value) -> :ok + _ -> {:error, Error.invalid_params(~s(initialize: missing or invalid string "#{key}"))} + end + end + + defp ensure_object(params, key) do + case Map.get(params, key) do + value when is_map(value) -> :ok + _ -> {:error, Error.invalid_params(~s(initialize: missing or invalid object "#{key}"))} + end + end + + # serverInfo MUST carry a string name and version. The DSL enforces this at the + # __server_info__ boundary, but a hand-written server_info/0 could omit them and produce a + # malformed InitializeResult; surface that as an internal error instead of shipping it. + defp validated_server_info(server) do + info = server.server_info() + + if is_map(info) and is_binary(info[:name] || info["name"]) and + is_binary(info[:version] || info["version"]) do + info + else + raise Error.internal_error("serverInfo must include a string name and version") + end + end + @doc """ Handles an operational (post-initialization) request, returning `{:ok, result_map}` or `{:error, Urchin.Error.t()}`. @@ -64,7 +112,11 @@ defmodule Urchin.Dispatcher do end def handle_request(server, method, params, ctx) do - do_handle(server, method, params, ctx) + # Lifecycle gate: until notifications/initialized has been received, reject operation requests + # other than ping with invalid_request. + with :ok <- check_initialized(method, ctx) do + do_handle(server, method, params, ctx) + end rescue error in Urchin.Error -> {:error, error} @@ -77,6 +129,29 @@ defmodule Urchin.Dispatcher do {:error, Error.internal_error(generic_or(ctx, "Handler threw: " <> inspect(value)))} end + # Until the client sends notifications/initialized the session is not initialized; only the + # pre-init methods are honoured. `notifications/initialized` is a notification routed straight + # into the session, so it never reaches this request-only path. + defp check_initialized(_method, %Context{initialized: true}), do: :ok + + defp check_initialized(method, %Context{}) do + if pre_init_allowed?(method) do + :ok + else + {:error, + Error.invalid_request( + "Server not initialized: send notifications/initialized before #{method}" + )} + end + end + + # Only ping is allowed before the client sends notifications/initialized; per the MCP lifecycle + # the client should not send requests other than pings until initialization completes. The + # pings-and-logging exception in the spec is for the server's own requests/notifications, not + # the client's logging/setLevel. + defp pre_init_allowed?("ping"), do: true + defp pre_init_allowed?(_method), do: false + # ping is always available regardless of declared capabilities. defp do_handle(_server, "ping", _params, _ctx), do: {:ok, %{}} @@ -90,7 +165,10 @@ defmodule Urchin.Dispatcher do with_callback(server, :call_tool, 3, fn -> name = require_string(params, "name") args = Map.get(params, "arguments", %{}) - call_tool_result(server, name, args, %{ctx | progress_token: progress_token(params)}) + + with :ok <- require_arguments_object(args) do + call_tool_result(server, name, args, %{ctx | progress_token: progress_token(params)}) + end end) end @@ -157,9 +235,9 @@ defmodule Urchin.Dispatcher do defp do_handle(server, "completion/complete", params, ctx) do with_callback(server, :complete, 4, fn -> - ref = require_map(params, "ref") - argument = require_map(params, "argument") - completion_context = Map.get(params, "context", %{}) + ref = require_completion_ref(params) + argument = require_completion_argument(params) + completion_context = require_completion_context(params) case server.complete(ref, argument, completion_context, ctx) do {:ok, completion} -> {:ok, %{completion: completion(completion)}} @@ -169,10 +247,20 @@ defmodule Urchin.Dispatcher do end defp do_handle(server, "logging/setLevel", params, ctx) do - with_callback(server, :set_log_level, 2, fn -> + # logging/setLevel is a library builtin, available only when the server advertises the + # logging capability. The level is validated, the optional set_log_level/2 hook runs, and + # the session level is updated only after both succeed, so a failed call leaves no change. + if logging_advertised?(server) do level = require_string(params, "level") - empty_result(server.set_log_level(level, ctx), ctx) - end) + + with :ok <- validate_log_level(level), + :ok <- run_log_level_hook(server, level, ctx), + :ok <- set_session_log_level(ctx, level) do + {:ok, %{}} + end + else + {:error, Error.method_not_found("Server does not support logging/setLevel")} + end end defp do_handle(_server, method, _params, _ctx) do @@ -187,11 +275,18 @@ defmodule Urchin.Dispatcher do %Result.CallTool{} = result -> {:ok, Result.CallTool.to_map(result)} {:ok, content} when is_list(content) -> {:ok, %{content: content, isError: false}} {:ok, content, opts} when is_list(content) -> {:ok, call_tool_map(content, opts)} - other -> normalize_error(other, ctx) + {:error, {:invalid_tool_input, reason}} -> invalid_tool_input(reason, ctx) + other -> tool_error_result(other, ctx) end rescue + error in Urchin.Error -> + # A deliberately raised Urchin.Error is a protocol-level error, matching a returned + # {:error, %Urchin.Error{}}: it stays a JSON-RPC error rather than an isError result. + {:error, error} + exception -> - # A tool that raises reports a tool-execution error so the model can self-correct. + # A tool that raises any other exception reports a tool-execution error so the model + # can self-correct. Logger.error( "Urchin tool #{name} crashed: " <> Exception.format(:error, exception, __STACKTRACE__) @@ -201,6 +296,21 @@ defmodule Urchin.Dispatcher do {:ok, %{content: [Urchin.Content.text(text)], isError: true}} end + # A handler's {:error, binary} is a tool-execution error: it becomes an isError CallToolResult + # (so the model can self-correct) per the MCP tool error semantics, not a JSON-RPC error. A + # protocol-level {:error, %Error{}} and every other shape still go through normalize_error. + defp tool_error_result({:error, message}, _ctx) when is_binary(message) do + {:ok, %{content: [Urchin.Content.text(message)], isError: true}} + end + + defp tool_error_result(other, ctx), do: normalize_error(other, ctx) + + # An input-schema validation failure is likewise a tool-execution error: an isError + # CallToolResult so the model can self-correct, not a JSON-RPC protocol error. + defp invalid_tool_input(reason, _ctx) do + {:ok, %{content: [Urchin.Content.text(reason)], isError: true}} + end + defp call_tool_map(content, opts) do %{content: content, isError: opts[:is_error] || false} |> maybe_put(:structuredContent, opts[:structured_content]) @@ -218,12 +328,55 @@ defmodule Urchin.Dispatcher do defp empty_result({:ok, _}, _ctx), do: {:ok, %{}} defp empty_result(other, ctx), do: normalize_error(other, ctx) - defp completion(values) when is_list(values), do: %{values: values} + @max_completion_values 100 + + defp completion(values) when is_list(values), do: build_completion(values, nil, nil) defp completion(%{} = completion) do - %{values: get_either(completion, :values, "values", [])} - |> maybe_put(:total, get_either(completion, :total, "total", nil)) - |> maybe_put(:hasMore, get_either(completion, :has_more, "hasMore", nil)) + build_completion( + get_either(completion, :values, "values", []), + get_either(completion, :total, "total", nil), + get_either(completion, :has_more, "hasMore", nil) + ) + end + + defp completion(_other) do + raise Error.internal_error("completion result must be a map or a list of values") + end + + # CompleteResult.completion is `{ values: string[], total?: number, hasMore?: boolean }`. A + # malformed result is a server bug, so it surfaces as an internal error rather than shipping a + # non-conforming response. The spec also caps values at 100 per response; when a handler returns + # more, the list is truncated to the top 100 (already ranked) and hasMore is necessarily true. + defp build_completion(values, total, has_more) do + validate_completion_result!(values, total, has_more) + + {capped, has_more} = + if length(values) > @max_completion_values do + {Enum.take(values, @max_completion_values), true} + else + {values, has_more} + end + + %{values: capped} + |> maybe_put(:total, total) + |> maybe_put(:hasMore, has_more) + end + + defp validate_completion_result!(values, total, has_more) do + cond do + not (is_list(values) and Enum.all?(values, &is_binary/1)) -> + raise Error.internal_error("completion values must be a list of strings") + + not (is_nil(total) or is_number(total)) -> + raise Error.internal_error("completion total must be a number") + + not (is_nil(has_more) or is_boolean(has_more)) -> + raise Error.internal_error("completion hasMore must be a boolean") + + true -> + :ok + end end # Reads a value that may be keyed by atom or string, preserving false/0 values. @@ -264,6 +417,46 @@ defmodule Urchin.Dispatcher do end end + # Apply the client-requested log level to the session when one exists; a nil session + # (e.g. a handler invoked in a unit test) is a no-op. If the session died mid-request, + # surface a clean error rather than a generic crash from the GenServer.call exit. + defp set_session_log_level(%Context{session: session}, level) when is_pid(session) do + Session.set_log_level(session, level) + :ok + catch + :exit, _ -> {:error, Error.invalid_request("Session not found")} + end + + defp set_session_log_level(_ctx, _level), do: :ok + + # logging/setLevel is offered only when the server advertises the logging capability. + # Accept both atom (DSL-derived) and string (hand-written, JSON-shaped) capability keys. + defp logging_advertised?(server) do + caps = capabilities(server) + Map.has_key?(caps, :logging) or Map.has_key?(caps, "logging") + end + + defp validate_log_level(level) do + if level in Context.log_levels() do + :ok + else + {:error, Error.invalid_params("Invalid log level: " <> level)} + end + end + + # Runs the optional set_log_level/2 hook; a missing hook is a no-op success. + defp run_log_level_hook(server, level, ctx) do + if exported?(server, :set_log_level, 2) do + case server.set_log_level(level, ctx) do + :ok -> :ok + {:ok, _} -> :ok + other -> normalize_error(other, ctx) + end + else + :ok + end + end + defp capabilities(server) do if exported?(server, :capabilities, 0), do: server.capabilities(), else: %{} end @@ -294,6 +487,14 @@ defmodule Urchin.Dispatcher do end end + # CallToolRequestParams.arguments is, when present, an object. A non-object value is a malformed + # request (CallToolRequest shape violation), not a tool input-schema error, so it stays a + # protocol-level JSON-RPC error rather than an isError tool result. + defp require_arguments_object(args) when is_map(args), do: :ok + + defp require_arguments_object(_args), + do: {:error, Error.invalid_params("tools/call arguments must be an object")} + defp require_map(params, key) do case Map.get(params, key) do value when is_map(value) -> value @@ -301,6 +502,58 @@ defmodule Urchin.Dispatcher do end end + # CompleteRequestParams.ref is a discriminated union: ref/prompt carries a string name, + # ref/resource carries a string uri. Anything else is a malformed request. + defp require_completion_ref(params) do + ref = require_map(params, "ref") + + case Map.get(ref, "type") do + "ref/prompt" -> _ = require_string(ref, "name") + "ref/resource" -> _ = require_string(ref, "uri") + other -> raise Error.invalid_params("completion ref.type is invalid: #{inspect(other)}") + end + + ref + end + + # CompleteRequestParams.argument is `{ name: string, value: string }`, both required. + defp require_completion_argument(params) do + argument = require_map(params, "argument") + _ = require_string(argument, "name") + _ = require_string(argument, "value") + argument + end + + # CompleteRequestParams.context is optional; its `arguments` map, when present, maps argument + # names to string values. + defp require_completion_context(params) do + case Map.get(params, "context") do + nil -> + %{} + + %{} = context -> + validate_context_arguments!(Map.get(context, "arguments")) + context + + _ -> + raise Error.invalid_params("completion context must be an object") + end + end + + defp validate_context_arguments!(nil), do: :ok + + defp validate_context_arguments!(arguments) when is_map(arguments) do + if Enum.all?(arguments, fn {_k, v} -> is_binary(v) end) do + :ok + else + raise Error.invalid_params("completion context.arguments values must be strings") + end + end + + defp validate_context_arguments!(_other) do + raise Error.invalid_params("completion context.arguments must be an object") + end + # Rescued exceptions are always logged in full but, by default, are not surfaced to the # client. Set the transport's :expose_internal_errors to return the message instead. defp handler_error(exception, stacktrace, ctx, label) do diff --git a/lib/urchin/server.ex b/lib/urchin/server.ex index 9184374..9edc968 100644 --- a/lib/urchin/server.ex +++ b/lib/urchin/server.ex @@ -34,6 +34,11 @@ defmodule Urchin.Server do Capabilities are derived automatically from the declared features. + Duplicate literal tool names declared via the DSL are rejected at compile time (non-literal + names cannot be compared statically and are not checked). Urchin enforces no tool-name pattern + (the MCP schema imposes none); servers should still follow the MCP naming recommendations (a + conservative charset, a length bound, no whitespace). + ## Behaviour Implement the callbacks directly for full control or stateful servers. All @@ -48,8 +53,11 @@ defmodule Urchin.Server do * `read_resource/2`: `{:ok, contents}` or `{:error, reason}` * `get_prompt/3`: `{:ok, messages}` or `{:ok, messages, description}` - Any `{:error, reason}` where `reason` is a string or `Urchin.Error` becomes a JSON-RPC - error; raised exceptions become internal errors. + For every callback, a returned or raised `Urchin.Error` becomes that JSON-RPC error. A + `call_tool/3` handler's `{:error, binary}` is surfaced as a `CallToolResult` with `isError: true` + so the model can self-correct, as is a tool that raises any other exception. For the other + callbacks an `{:error, binary}` becomes a JSON-RPC internal error and any other raised exception + becomes an internal error. """ alias Urchin.{Context, Error} @@ -247,6 +255,33 @@ defmodule Urchin.Server do raise ArgumentError, "tool :scopes must be a list of strings, got: #{inspect(other)}" end + # Duplicate tool names within a server are rejected at compile time (a silently shadowed + # duplicate is a bug). The MCP schema imposes no tool-name pattern, so none is enforced. + # Non-literal names (a variable or call) cannot be compared statically and are skipped, + # mirroring handler_name/2. + defp validate_tool_names!(tool_dispatch) do + tool_dispatch + |> Enum.map(fn {name, _fname, _scopes} -> name end) + |> Enum.filter(&is_binary/1) + |> validate_unique_tool_names!() + end + + defp validate_unique_tool_names!(names) do + duplicates = + names + |> Enum.frequencies() + |> Enum.filter(fn {_name, count} -> count > 1 end) + |> Enum.map(fn {name, _count} -> name end) + + case duplicates do + [] -> + :ok + + dups -> + raise ArgumentError, "duplicate tool name(s): #{Enum.map_join(dups, ", ", &inspect/1)}" + end + end + # Generates a deterministic private handler name. Determinism matters because Mix's # incremental compiler assumes stable output for unchanged sources. defp handler_name(prefix, name) when is_binary(name) do @@ -263,7 +298,10 @@ defmodule Urchin.Server do mod = env.module opts = Module.get_attribute(mod, :mcp_opts) || [] - has_tools? = Module.get_attribute(mod, :mcp_tool_dispatch) != [] + tool_dispatch = Module.get_attribute(mod, :mcp_tool_dispatch) || [] + validate_tool_names!(tool_dispatch) + + has_tools? = tool_dispatch != [] has_resources? = Module.get_attribute(mod, :mcp_resources) != [] has_templates? = Module.get_attribute(mod, :mcp_resource_templates) != [] has_prompts? = Module.get_attribute(mod, :mcp_prompt_dispatch) != [] @@ -430,20 +468,22 @@ defmodule Urchin.Server do @doc false @spec __validate_tool_args__(String.t(), map(), Context.t(), [Urchin.Tool.t()]) :: - :ok | {:error, Urchin.Error.t()} - def __validate_tool_args__(_name, _args, %Context{validate_arguments: false}, _tools), do: :ok - + :ok | {:error, {:invalid_tool_input, String.t()}} def __validate_tool_args__(name, args, _ctx, tools) do - # Use the same effective schema the wire advertises: an omitted input_schema means an - # object, so validation is not silently skipped for a tool that declared no schema. + # Use the same effective schema the wire advertises (Tool.default_input_schema/0 when the tool + # declared none), so validation is not silently skipped for a tool that declared no schema. schema = Enum.find_value(tools, fn tool -> - if tool.name == name, do: tool.input_schema || %{"type" => "object"} + if tool.name == name, do: tool.input_schema || Urchin.Tool.default_input_schema() end) + # By the time args reaches here it is already an object (the dispatcher rejects a non-object + # CallToolRequestParams.arguments as a protocol error). What remains is input-schema validation + # (missing required field, wrong property type, ...), which is a tool-input error: the dispatcher + # shapes it as an isError CallToolResult, not a JSON-RPC error. case Urchin.Schema.validate(schema, args) do :ok -> :ok - {:error, reason} -> {:error, Urchin.Error.invalid_params(reason)} + {:error, reason} -> {:error, {:invalid_tool_input, reason}} end end diff --git a/lib/urchin/session.ex b/lib/urchin/session.ex index 973fd80..5cdfe74 100644 --- a/lib/urchin/session.ex +++ b/lib/urchin/session.ex @@ -108,15 +108,20 @@ defmodule Urchin.Session do @doc """ Handles a client-originated notification or response delivered over POST. - Notifications (`notifications/cancelled`, `notifications/initialized`, ...) update - session state; responses are correlated to a pending outbound request. + Notifications (`notifications/cancelled`, ...) update session state; responses are correlated + to a pending outbound request. `notifications/initialized` is committed synchronously via + `mark_initialized/1`, not through this path. """ @spec handle_client_message(pid(), Urchin.JSONRPC.decoded()) :: :ok def handle_client_message(pid, message), do: GenServer.cast(pid, {:client_message, message}) @doc "Sets the minimum log level the client wishes to receive." @spec set_log_level(pid(), String.t()) :: :ok - def set_log_level(pid, level), do: GenServer.cast(pid, {:set_log_level, level}) + def set_log_level(pid, level), do: GenServer.call(pid, {:set_log_level, level}) + + @doc "Marks the session initialized synchronously (after notifications/initialized)." + @spec mark_initialized(pid()) :: :ok + def mark_initialized(pid), do: GenServer.call(pid, :mark_initialized) @doc "Registers (or replaces) the GET general stream, returning events to replay." @spec register_general_stream(pid(), pid(), {String.t(), non_neg_integer()} | nil) :: @@ -162,7 +167,7 @@ defmodule Urchin.Session do general_stream_id: "g0", general_seq: 0, general_buffer: [], - buffer_limit: Keyword.get(opts, :buffer_limit, @default_buffer_limit), + buffer_limit: Keyword.get(opts, :buffer_limit) || @default_buffer_limit, inflight: %{}, cancelled: MapSet.new(), outbound: %{}, @@ -194,12 +199,21 @@ defmodule Urchin.Session do protocol_version: state.protocol_version, client_info: state.client_info, client_capabilities: state.client_capabilities, - min_log_level: state.min_log_level + min_log_level: state.min_log_level, + initialized: state.initialized } {:reply, snapshot, touch(state)} end + def handle_call(:mark_initialized, _from, state) do + {:reply, :ok, touch(%{state | initialized: true})} + end + + def handle_call({:set_log_level, level}, _from, state) do + {:reply, :ok, %{state | min_log_level: level}} + end + def handle_call({:start_request, request_id, task_pid, owner_pid}, _from, state) do seq = state.post_seq + 1 stream_id = "p" <> Integer.to_string(seq) @@ -268,10 +282,6 @@ defmodule Urchin.Session do {:noreply, %{state | outbound: drop_outbound(state.outbound, id)}} end - def handle_cast({:set_log_level, level}, state) do - {:noreply, %{state | min_log_level: level}} - end - def handle_cast({:subscribe, uri}, state) do {:noreply, %{state | subscriptions: MapSet.put(state.subscriptions, uri)}} end @@ -334,9 +344,8 @@ defmodule Urchin.Session do ## Internal: client message handling - defp handle_client({:notification, "notifications/initialized", _params}, state) do - %{state | initialized: true} - end + # notifications/initialized is committed synchronously via mark_initialized/1 (the transport + # routes it there so the next request observes initialized: true), so it is not handled here. defp handle_client({:notification, "notifications/cancelled", params}, state) do request_id = Map.get(params, "requestId") diff --git a/lib/urchin/tool.ex b/lib/urchin/tool.ex index eb332ee..ea47149 100644 --- a/lib/urchin/tool.ex +++ b/lib/urchin/tool.ex @@ -3,7 +3,8 @@ defmodule Urchin.Tool do A tool definition advertised via `tools/list`. Mirrors the `Tool` type from the MCP schema. `input_schema` is a JSON Schema object - describing the tool arguments; when omitted it defaults to an empty object schema. + describing the tool arguments; when omitted it defaults to `default_input_schema/0`, an + object that accepts no properties. """ alias Urchin.WireFormat @@ -42,8 +43,8 @@ defmodule Urchin.Tool do name: fetch_name!(attrs), title: attrs[:title], description: attrs[:description], - input_schema: attrs[:input_schema], - output_schema: attrs[:output_schema], + input_schema: validate_object_schema!(attrs[:input_schema], :input_schema), + output_schema: validate_object_schema!(attrs[:output_schema], :output_schema), annotations: attrs[:annotations], execution: attrs[:execution], icons: attrs[:icons], @@ -54,10 +55,38 @@ defmodule Urchin.Tool do defp fetch_name!(%{name: name}) when is_binary(name), do: name defp fetch_name!(_), do: raise(ArgumentError, "tool requires a string :name") + # inputSchema and outputSchema are JSON Schema objects whose root `type` is "object" (MCP tools + # spec). nil is allowed: input_schema falls back to default_input_schema/0 and output_schema is + # optional. A non-object schema would advertise a non-conforming tools/list entry. + defp validate_object_schema!(nil, _field), do: nil + + defp validate_object_schema!(schema, field) when is_map(schema) do + case schema["type"] || schema[:type] do + "object" -> + schema + + other -> + raise ArgumentError, + ~s(tool #{field} must be a JSON Schema object with "type": "object", got type: #{inspect(other)}) + end + end + + defp validate_object_schema!(other, field) do + raise ArgumentError, + "tool #{field} must be a map (a JSON Schema object), got: #{inspect(other)}" + end + + @doc """ + The input schema advertised for a tool that declares none: an object accepting no + properties, per the MCP recommendation for parameterless tools. + """ + @spec default_input_schema() :: map() + def default_input_schema, do: %{"type" => "object", "additionalProperties" => false} + @doc "Serializes the tool to its JSON-RPC wire shape." @spec to_map(t()) :: map() def to_map(%__MODULE__{} = tool) do - %{name: tool.name, inputSchema: tool.input_schema || %{"type" => "object"}} + %{name: tool.name, inputSchema: tool.input_schema || default_input_schema()} |> WireFormat.maybe_put(:title, tool.title) |> WireFormat.maybe_put(:description, tool.description) |> WireFormat.maybe_put(:outputSchema, tool.output_schema) diff --git a/lib/urchin/transport/streamable_http.ex b/lib/urchin/transport/streamable_http.ex index e7ed5f1..4d7b4eb 100644 --- a/lib/urchin/transport/streamable_http.ex +++ b/lib/urchin/transport/streamable_http.ex @@ -30,13 +30,19 @@ defmodule Urchin.Transport.StreamableHTTP do tool run, since it can expire a session mid-request. * `:expose_internal_errors` - return raised-exception messages to the client instead of a generic error (default `false`). Exceptions are always logged; enable only in development. - * `:validate_arguments` - validate `tools/call` arguments against each tool's - `input_schema` (DSL tools) before the handler runs, rejecting a mismatch with - `invalid_params` (default `false`). See `Urchin.Schema` for the supported subset. + * `:sse_buffer_limit` - the maximum number of recent general-stream (GET SSE) events each + session keeps for resumption replay. Defaults to `nil`, which preserves the session's + internal default of `100`. A positive integer or `nil`. * `:auth` - an `Urchin.Auth` (or keyword options) to require OAuth 2.1 bearer tokens on every request; `nil` (default) serves MCP unauthenticated. The metadata discovery endpoint is served by `Urchin.Endpoint`/`Urchin.Auth.Metadata`, not this plug. + The transport enforces the spec by default and these behaviors are not configurable: it + validates a DSL tool's `tools/call` arguments against its input schema (a hand-written + `call_tool/3` validates its own arguments), rejects operation requests received before + `notifications/initialized` (only `ping` is allowed pre-init), and surfaces a tool handler's + `{:error, binary}` as an `isError` `CallToolResult`. + The plug reads the raw request body itself, so mount it before any JSON body parser. """ @@ -68,10 +74,10 @@ defmodule Urchin.Transport.StreamableHTTP do request_timeout: Keyword.get(opts, :request_timeout, 60_000), validate_protocol_version: Keyword.get(opts, :validate_protocol_version, true), expose_internal_errors: Keyword.get(opts, :expose_internal_errors, false), - validate_arguments: Keyword.get(opts, :validate_arguments, false), max_sessions: positive_integer_opt!(opts, :max_sessions), session_idle_timeout: positive_integer_opt!(opts, :session_idle_timeout), session_max_lifetime: positive_integer_opt!(opts, :session_max_lifetime), + sse_buffer_limit: positive_integer_opt!(opts, :sse_buffer_limit), auth: Auth.coerce!(Keyword.get(opts, :auth)) } end @@ -163,16 +169,20 @@ defmodule Urchin.Transport.StreamableHTTP do end defp start_session(conn, config, id, result, meta, server_state, reservation) do - case Session.start( - server: config.server, - server_state: server_state, - protocol_version: meta.protocol_version, - client_info: meta.client_info, - client_capabilities: meta.client_capabilities, - min_log_level: config.min_log_level, - idle_timeout: config.session_idle_timeout, - max_lifetime: config.session_max_lifetime - ) do + session_opts = + [ + server: config.server, + server_state: server_state, + protocol_version: meta.protocol_version, + client_info: meta.client_info, + client_capabilities: meta.client_capabilities, + min_log_level: config.min_log_level, + idle_timeout: config.session_idle_timeout, + max_lifetime: config.session_max_lifetime + ] + |> maybe_put_buffer_limit(config.sse_buffer_limit) + + case Session.start(session_opts) do {:ok, session_id, pid} -> # Hand the reserved slot to the session. If the limiter no longer knows the # reservation (e.g. it restarted since reserve/1), the session would be uncounted, @@ -207,6 +217,20 @@ defmodule Urchin.Transport.StreamableHTTP do end # Notifications and responses are acknowledged with 202 and routed into the session. + # notifications/initialized is committed synchronously so that, once the client has the + # 202, a subsequent request always observes initialized: true in the session snapshot. + defp route_session_message( + conn, + _config, + session_pid, + {:notification, "notifications/initialized", _params} + ) do + case mark_initialized_safe(session_pid) do + :ok -> send_resp(conn, 202, "") + {:error, status, error} -> send_error(conn, status, nil, error) + end + end + defp route_session_message(conn, _config, session_pid, {:notification, _m, _p} = msg) do Session.handle_client_message(session_pid, msg) send_resp(conn, 202, "") @@ -244,7 +268,7 @@ defmodule Urchin.Transport.StreamableHTTP do auth: conn_auth(conn), min_log_level: snapshot.min_log_level, expose_internal_errors: config.expose_internal_errors, - validate_arguments: config.validate_arguments + initialized: snapshot.initialized } {task_pid, task_ref} = @@ -419,13 +443,14 @@ defmodule Urchin.Transport.StreamableHTTP do end defp handle_delete(conn, config) do - case lookup_session(conn, config) do - {:ok, session_pid} -> - Session.terminate(session_pid) - send_resp(conn, 204, "") - - {:error, status, error} -> - send_error(conn, status, nil, error) + # DELETE is a post-initialize request, so it carries the MCP-Protocol-Version header like POST + # and GET; validate it before terminating the session. + with {:ok, session_pid} <- lookup_session(conn, config), + :ok <- check_protocol_version(conn, config) do + Session.terminate(session_pid) + send_resp(conn, 204, "") + else + {:error, status, error} -> send_error(conn, status, nil, error) end end @@ -453,6 +478,17 @@ defmodule Urchin.Transport.StreamableHTTP do end end + # notifications/initialized commits synchronously via a GenServer.call so the next request + # observes initialized: true. lookup_session/2 only proves the session was alive a moment ago, so + # a session that terminates in between would make the call exit and crash the Plug process; catch + # that and report a clean "Session not found", mirroring lookup_session/2 (and set_log_level). + defp mark_initialized_safe(session_pid) do + Session.mark_initialized(session_pid) + :ok + catch + :exit, _ -> {:error, 404, Error.invalid_request("Session not found")} + end + defp check_protocol_version(_conn, %{validate_protocol_version: false}), do: :ok defp check_protocol_version(conn, _config) do @@ -518,6 +554,11 @@ defmodule Urchin.Transport.StreamableHTTP do end end + # When nil, omit the key so the Session keeps its own default (@default_buffer_limit). + # Passing buffer_limit: nil would defeat Keyword.get's default and crash push_general. + defp maybe_put_buffer_limit(opts, nil), do: opts + defp maybe_put_buffer_limit(opts, limit), do: Keyword.put(opts, :buffer_limit, limit) + ## Origin / Accept defp validate_origin(conn, config) do diff --git a/mix.exs b/mix.exs index d2c93e4..7c4df7b 100644 --- a/mix.exs +++ b/mix.exs @@ -1,7 +1,7 @@ defmodule Urchin.MixProject do use Mix.Project - @version "0.2.0" + @version "0.3.0" @source_url "https://github.com/urth-inc/urchin" def project do diff --git a/test/support/echo_server.ex b/test/support/echo_server.ex index 596b6d2..7c868d5 100644 --- a/test/support/echo_server.ex +++ b/test/support/echo_server.ex @@ -47,6 +47,21 @@ defmodule Urchin.Test.EchoServer do {:error, {:db, "postgres://secret@host"}} end + tool "failing", description: "Returns a string error reason" do + _ = {args, ctx} + {:error, "tool said no"} + end + + tool "protocol_error", description: "Returns an Urchin.Error protocol error" do + _ = {args, ctx} + {:error, Urchin.Error.invalid_params("bad")} + end + + tool "raise_protocol", description: "Raises an Urchin.Error protocol error" do + _ = {args, ctx} + raise Urchin.Error.invalid_params("raised bad") + end + tool "secret", description: "Requires the secret:read scope", scopes: ["secret:read"] do diff --git a/test/urchin/core_test.exs b/test/urchin/core_test.exs index 99140db..54fd540 100644 --- a/test/urchin/core_test.exs +++ b/test/urchin/core_test.exs @@ -119,11 +119,43 @@ defmodule Urchin.CoreTest do assert decoded == %{ "name" => "t", "description" => "d", - "inputSchema" => %{"type" => "object"} + "inputSchema" => %{"type" => "object", "additionalProperties" => false} } end end + describe "Urchin.Tool schema validation" do + test "accepts an object input and output schema" do + tool = + Tool.new( + name: "t", + input_schema: %{"type" => "object", "properties" => %{}}, + output_schema: %{"type" => "object"} + ) + + assert tool.input_schema["type"] == "object" + assert tool.output_schema["type"] == "object" + end + + test "rejects an input schema whose root type is not object" do + assert_raise ArgumentError, ~r/input_schema.*type.*object/, fn -> + Tool.new(name: "t", input_schema: %{"type" => "array"}) + end + end + + test "rejects a non-map input schema" do + assert_raise ArgumentError, ~r/input_schema must be a map/, fn -> + Tool.new(name: "t", input_schema: "nope") + end + end + + test "rejects an output schema whose root type is not object" do + assert_raise ArgumentError, ~r/output_schema.*type.*object/, fn -> + Tool.new(name: "t", output_schema: %{"type" => "string"}) + end + end + end + describe "Urchin.URITemplate" do test "matches a single-segment variable" do assert {:ok, %{"name" => "world"}} = diff --git a/test/urchin/dispatcher_test.exs b/test/urchin/dispatcher_test.exs index dceb407..132eb03 100644 --- a/test/urchin/dispatcher_test.exs +++ b/test/urchin/dispatcher_test.exs @@ -1,10 +1,79 @@ +defmodule Urchin.DispatcherTest.LoggingServer do + @moduledoc false + # Exports set_log_level/2 so the builtin-plus-hook path can be exercised. + use Urchin.Server, name: "logging", version: "1.0.0", logging: true + + @impl true + def set_log_level(level, ctx) do + case ctx.assigns do + %{test_pid: pid} -> send(pid, {:set_log_level_called, level}) + _ -> :ok + end + + :ok + end +end + +defmodule Urchin.DispatcherTest.NoLoggingServer do + @moduledoc false + # Advertises no logging capability. + use Urchin.Server, name: "no-logging", version: "1.0.0" +end + +defmodule Urchin.DispatcherTest.FailingLoggingServer do + @moduledoc false + # Advertises logging (via the callback) but the set_log_level/2 hook always fails. + use Urchin.Server, name: "failing-logging", version: "1.0.0" + + @impl true + def set_log_level(_level, _ctx), do: {:error, "nope"} +end + +defmodule Urchin.DispatcherTest.BadInfoServer do + @moduledoc false + # A hand-written server whose server_info/0 omits the required version field. + @behaviour Urchin.Server + + @impl true + def server_info, do: %{name: "bad"} + + @impl true + def capabilities, do: %{} +end + +defmodule Urchin.DispatcherTest.BigCompletionServer do + @moduledoc false + # Returns more than the 100-value completion cap so truncation can be exercised. + use Urchin.Server, name: "big-completion", version: "1.0.0", completions: true + + @impl true + def complete(_ref, _argument, _context, _ctx) do + {:ok, %{values: Enum.map(1..150, &"v#{&1}")}} + end +end + +defmodule Urchin.DispatcherTest.BadCompletionServer do + @moduledoc false + # Returns a non-conforming completion result (values are not strings). + use Urchin.Server, name: "bad-completion", version: "1.0.0", completions: true + + @impl true + def complete(_ref, _argument, _context, _ctx) do + {:ok, %{values: [1, 2, 3]}} + end +end + defmodule Urchin.DispatcherTest do use ExUnit.Case, async: true - alias Urchin.{Context, Dispatcher} + alias Urchin.{Context, Dispatcher, Session} alias Urchin.Test.EchoServer + alias Urchin.DispatcherTest.{LoggingServer, NoLoggingServer, FailingLoggingServer} + alias Urchin.DispatcherTest.{BadInfoServer, BigCompletionServer, BadCompletionServer} - defp ctx, do: %Context{} + # The default context represents an initialized session; the lifecycle gate is exercised + # explicitly in the "initialized gating" describe with initialized: false. + defp ctx, do: %Context{initialized: true} describe "initialize/3" do test "negotiates a supported version and reports capabilities" do @@ -26,10 +95,48 @@ defmodule Urchin.DispatcherTest do end test "falls back to latest for an unsupported version" do - params = %{"protocolVersion" => "1999-01-01", "capabilities" => %{}} + params = %{ + "protocolVersion" => "1999-01-01", + "capabilities" => %{}, + "clientInfo" => %{"name" => "c", "version" => "1"} + } + assert {:ok, result, _meta} = Dispatcher.initialize(EchoServer, params, ctx()) assert result.protocolVersion == Urchin.protocol_version() end + + test "rejects a missing protocolVersion" do + params = %{"capabilities" => %{}, "clientInfo" => %{"name" => "c", "version" => "1"}} + assert {:error, error} = Dispatcher.initialize(EchoServer, params, ctx()) + assert error.code == -32_602 + end + + test "rejects a missing capabilities object" do + params = %{ + "protocolVersion" => "2025-11-25", + "clientInfo" => %{"name" => "c", "version" => "1"} + } + + assert {:error, error} = Dispatcher.initialize(EchoServer, params, ctx()) + assert error.code == -32_602 + end + + test "rejects clientInfo without a string name and version" do + params = %{"protocolVersion" => "2025-11-25", "capabilities" => %{}, "clientInfo" => %{}} + assert {:error, error} = Dispatcher.initialize(EchoServer, params, ctx()) + assert error.code == -32_602 + end + + test "rejects a serverInfo missing name or version" do + params = %{ + "protocolVersion" => "2025-11-25", + "capabilities" => %{}, + "clientInfo" => %{"name" => "c", "version" => "1"} + } + + assert {:error, error} = Dispatcher.initialize(BadInfoServer, params, ctx()) + assert error.code == -32_603 + end end describe "tools" do @@ -71,7 +178,7 @@ defmodule Urchin.DispatcherTest do test "a raising tool exposes its message when expose_internal_errors is set" do params = %{"name" => "boom", "arguments" => %{}} - ctx = %Context{expose_internal_errors: true} + ctx = %Context{expose_internal_errors: true, initialized: true} assert {:ok, result} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx) assert result.content == [%{type: "text", text: "kaboom"}] end @@ -91,7 +198,7 @@ defmodule Urchin.DispatcherTest do test "a non-binary handler error reason is exposed when configured" do params = %{"name" => "leaky", "arguments" => %{}} - ctx = %Context{expose_internal_errors: true} + ctx = %Context{expose_internal_errors: true, initialized: true} assert {:error, error} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx) assert error.message =~ "postgres" end @@ -110,7 +217,7 @@ defmodule Urchin.DispatcherTest do end test "runs the handler when the required scope is granted" do - ctx = %Context{auth: %Claims{scopes: ["secret:read"]}} + ctx = %Context{auth: %Claims{scopes: ["secret:read"]}, initialized: true} assert {:ok, %{content: [%{type: "text", text: "classified"}], isError: false}} = call_secret(ctx) @@ -120,57 +227,65 @@ defmodule Urchin.DispatcherTest do end test "denies when the granted scopes are insufficient" do - assert {:error, error} = call_secret(%Context{auth: %Claims{scopes: ["other"]}}) + assert {:error, error} = + call_secret(%Context{auth: %Claims{scopes: ["other"]}, initialized: true}) + assert error.message =~ "scope" end test "denies (fail closed) when the request carries no authorization" do - assert {:error, error} = call_secret(%Context{auth: nil}) + assert {:error, error} = call_secret(%Context{auth: nil, initialized: true}) assert error.message =~ "scope" end test "a denied call never executes the handler" do - assert {:error, _} = call_secret(%Context{auth: %Claims{scopes: ["other"]}}) + assert {:error, _} = + call_secret(%Context{auth: %Claims{scopes: ["other"]}, initialized: true}) + refute_received :secret_executed end test "scope denial has a stable error code and the required scopes in data" do - assert {:error, error} = call_secret(%Context{auth: %Claims{scopes: []}}) + assert {:error, error} = call_secret(%Context{auth: %Claims{scopes: []}, initialized: true}) assert error.code == -32_600 assert error.data == %{required_scopes: ["secret:read"]} end end describe "argument validation" do - test "rejects arguments that violate the input schema when enabled" do - ctx = %Context{validate_arguments: true} + test "an input-schema violation is an isError tool result" do params = %{"name" => "add", "arguments" => %{"a" => 1}} - assert {:error, error} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx) - assert error.code == -32_602 - assert error.message =~ "b" + assert {:ok, result} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) + assert result.isError == true + assert [%{type: "text", text: text}] = result.content + assert text =~ "b" end - test "accepts valid arguments when enabled" do - ctx = %Context{validate_arguments: true} + test "accepts valid arguments" do params = %{"name" => "add", "arguments" => %{"a" => 1, "b" => 2}} assert {:ok, %{structuredContent: %{"sum" => 3}}} = - Dispatcher.handle_request(EchoServer, "tools/call", params, ctx) + Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) end - test "does not validate when disabled (the default)" do - # Without validation the bad arguments reach the handler, which fails at runtime. - params = %{"name" => "add", "arguments" => %{"a" => 1}} - - assert {:ok, %{isError: true}} = - Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) + test "a tool that declares no schema rejects unexpected properties" do + # An omitted input_schema defaults to an object accepting no properties. + params = %{"name" => "no_schema", "arguments" => %{"extra" => 1}} + assert {:ok, result} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) + assert result.isError == true end - test "validates an omitted input_schema as an object" do - # A tool without an input_schema still must receive an object, not a bare value. - ctx = %Context{validate_arguments: true} + test "non-object arguments are a protocol error, not a tool input error" do + # CallToolRequestParams.arguments is, when present, an object. A non-object value violates the + # request shape, so it stays a JSON-RPC error rather than an isError tool result. params = %{"name" => "no_schema", "arguments" => "not-an-object"} - assert {:error, error} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx) + assert {:error, error} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) + assert error.code == -32_602 + end + + test "array arguments are rejected as a protocol error" do + params = %{"name" => "no_schema", "arguments" => ["not", "an", "object"]} + assert {:error, error} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) assert error.code == -32_602 end end @@ -249,6 +364,78 @@ defmodule Urchin.DispatcherTest do assert completion.hasMore == false end + test "caps completion values at 100 and forces hasMore when truncated" do + params = %{ + "ref" => %{"type" => "ref/prompt", "name" => "x"}, + "argument" => %{"name" => "n", "value" => "v"} + } + + assert {:ok, %{completion: completion}} = + Dispatcher.handle_request( + BigCompletionServer, + "completion/complete", + params, + ctx() + ) + + assert length(completion.values) == 100 + assert completion.hasMore == true + end + + test "rejects an argument without a string name and value" do + params = %{ + "ref" => %{"type" => "ref/prompt", "name" => "greet"}, + "argument" => %{"name" => "name"} + } + + assert {:error, error} = + Dispatcher.handle_request(EchoServer, "completion/complete", params, ctx()) + + assert error.code == -32_602 + end + + test "rejects an unknown ref type" do + params = %{ + "ref" => %{"type" => "ref/bogus"}, + "argument" => %{"name" => "name", "value" => "Sa"} + } + + assert {:error, error} = + Dispatcher.handle_request(EchoServer, "completion/complete", params, ctx()) + + assert error.code == -32_602 + end + + test "rejects non-string context.arguments values" do + params = %{ + "ref" => %{"type" => "ref/prompt", "name" => "greet"}, + "argument" => %{"name" => "name", "value" => "Sa"}, + "context" => %{"arguments" => %{"prior" => 1}} + } + + assert {:error, error} = + Dispatcher.handle_request(EchoServer, "completion/complete", params, ctx()) + + assert error.code == -32_602 + end + + test "a non-conforming completion result is an internal error" do + params = %{ + "ref" => %{"type" => "ref/prompt", "name" => "x"}, + "argument" => %{"name" => "n", "value" => "v"} + } + + assert {:error, error} = + Dispatcher.handle_request( + BadCompletionServer, + "completion/complete", + params, + ctx() + ) + + assert error.code == -32_603 + end + test "ping" do assert {:ok, %{}} = Dispatcher.handle_request(EchoServer, "ping", %{}, ctx()) end @@ -263,4 +450,153 @@ defmodule Urchin.DispatcherTest do assert error.code == -32_602 end end + + describe "logging/setLevel" do + test "succeeds as a builtin without a server callback" do + assert {:ok, %{}} = + Dispatcher.handle_request( + EchoServer, + "logging/setLevel", + %{"level" => "warning"}, + ctx() + ) + end + + test "rejects a missing level param" do + assert {:error, error} = + Dispatcher.handle_request(EchoServer, "logging/setLevel", %{}, ctx()) + + assert error.code == -32_602 + end + + test "reflects the requested level on the session" do + {:ok, _id, pid} = Session.start(server: EchoServer, protocol_version: "2025-11-25") + on_exit(fn -> Session.terminate(pid) end) + + assert {:ok, %{}} = + Dispatcher.handle_request( + EchoServer, + "logging/setLevel", + %{"level" => "error"}, + %Context{session: pid, initialized: true} + ) + + assert Session.snapshot(pid).min_log_level == "error" + end + + test "invokes an exported set_log_level/2 hook" do + ctx = %Context{assigns: %{test_pid: self()}, initialized: true} + + assert {:ok, %{}} = + Dispatcher.handle_request( + LoggingServer, + "logging/setLevel", + %{"level" => "info"}, + ctx + ) + + assert_received {:set_log_level_called, "info"} + end + + test "rejects an invalid level with -32602 and leaves the session unchanged" do + {:ok, _id, pid} = Session.start(server: EchoServer, protocol_version: "2025-11-25") + on_exit(fn -> Session.terminate(pid) end) + + assert {:error, error} = + Dispatcher.handle_request( + EchoServer, + "logging/setLevel", + %{"level" => "verbose"}, + %Context{session: pid, initialized: true} + ) + + assert error.code == -32_602 + assert Session.snapshot(pid).min_log_level != "verbose" + end + + test "is not available unless the server advertises the logging capability" do + assert {:error, error} = + Dispatcher.handle_request( + NoLoggingServer, + "logging/setLevel", + %{"level" => "info"}, + ctx() + ) + + assert error.code == -32_601 + end + + test "leaves the session unchanged when the set_log_level/2 hook fails" do + {:ok, _id, pid} = + Session.start(server: FailingLoggingServer, protocol_version: "2025-11-25") + + on_exit(fn -> Session.terminate(pid) end) + before = Session.snapshot(pid).min_log_level + + assert {:error, _error} = + Dispatcher.handle_request( + FailingLoggingServer, + "logging/setLevel", + %{"level" => "warning"}, + %Context{session: pid, initialized: true} + ) + + assert Session.snapshot(pid).min_log_level == before + end + end + + describe "initialized gating" do + test "rejects operation requests before initialized" do + ctx = %Context{initialized: false} + assert {:error, error} = Dispatcher.handle_request(EchoServer, "tools/list", %{}, ctx) + assert error.code == -32_600 + end + + test "allows ping before initialized" do + ctx = %Context{initialized: false} + assert {:ok, %{}} = Dispatcher.handle_request(EchoServer, "ping", %{}, ctx) + end + + test "rejects logging/setLevel before initialized (only ping is exempt)" do + # The lifecycle's pre-init exception for logging is the server's own requests, not the + # client's logging/setLevel, so it is gated like any other operation request. + ctx = %Context{initialized: false} + + assert {:error, error} = + Dispatcher.handle_request( + EchoServer, + "logging/setLevel", + %{"level" => "info"}, + ctx + ) + + assert error.code == -32_600 + end + + test "allows operation requests once initialized" do + ctx = %Context{initialized: true} + assert {:ok, %{tools: _}} = Dispatcher.handle_request(EchoServer, "tools/list", %{}, ctx) + end + end + + describe "tool errors" do + test "a binary tool error becomes an isError result" do + params = %{"name" => "failing", "arguments" => %{}} + assert {:ok, result} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) + assert result == %{content: [%{type: "text", text: "tool said no"}], isError: true} + end + + test "a protocol error is still surfaced as a JSON-RPC error" do + params = %{"name" => "protocol_error", "arguments" => %{}} + assert {:error, error} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) + assert error.code == -32_602 + end + + test "a raised Urchin.Error is a JSON-RPC error, not an isError result" do + params = %{"name" => "raise_protocol", "arguments" => %{}} + assert {:error, error} = Dispatcher.handle_request(EchoServer, "tools/call", params, ctx()) + assert error.code == -32_602 + assert error.message == "raised bad" + end + end end diff --git a/test/urchin/integration_test.exs b/test/urchin/integration_test.exs index 108e390..7aa2e2e 100644 --- a/test/urchin/integration_test.exs +++ b/test/urchin/integration_test.exs @@ -45,6 +45,13 @@ defmodule Urchin.IntegrationTest do {200, headers, resp} = SSEClient.request(port, "POST", "/mcp", json_headers(), body) session_id = Enum.find_value(headers, fn {k, v} -> if k == "mcp-session-id", do: v end) + + # Complete the handshake so subsequent operation requests pass the lifecycle gate. + initialized = Jason.encode!(%{jsonrpc: "2.0", method: "notifications/initialized"}) + + {202, _h, _b} = + SSEClient.request(port, "POST", "/mcp", session_headers(session_id), initialized) + {session_id, Jason.decode!(resp)} end diff --git a/test/urchin/server_test.exs b/test/urchin/server_test.exs index 290fcab..734c4b7 100644 --- a/test/urchin/server_test.exs +++ b/test/urchin/server_test.exs @@ -32,4 +32,58 @@ defmodule Urchin.ServerTest do assert [{Urchin.ServerTest.GoodScopes, _}] = Code.compile_string(source) end end + + describe "tool name validation" do + test "does not enforce a tool-name pattern (the MCP schema imposes none)" do + source = """ + defmodule Urchin.ServerTest.UnusualName do + use Urchin.Server, name: "unusual", version: "1.0.0" + + tool "bad name!" do + {:ok, [Urchin.Content.text("ok")]} + end + end + """ + + assert [{Urchin.ServerTest.UnusualName, _} | _] = Code.compile_string(source) + end + + test "rejects duplicate tool names at compile time (always on)" do + source = """ + defmodule Urchin.ServerTest.DupNames do + use Urchin.Server, name: "dup", version: "1.0.0" + + tool "dup" do + {:ok, [Urchin.Content.text("a")]} + end + + tool "dup" do + {:ok, [Urchin.Content.text("b")]} + end + end + """ + + assert_raise ArgumentError, ~r/duplicate tool name/, fn -> + Code.compile_string(source) + end + end + + test "accepts valid, unique names" do + source = """ + defmodule Urchin.ServerTest.GoodNames do + use Urchin.Server, name: "good-names", version: "1.0.0" + + tool "echo" do + {:ok, [Urchin.Content.text("a")]} + end + + tool "my.tool-1" do + {:ok, [Urchin.Content.text("b")]} + end + end + """ + + assert [{Urchin.ServerTest.GoodNames, _} | _] = Code.compile_string(source) + end + end end diff --git a/test/urchin/session_lifecycle_test.exs b/test/urchin/session_lifecycle_test.exs index 481d525..450646d 100644 --- a/test/urchin/session_lifecycle_test.exs +++ b/test/urchin/session_lifecycle_test.exs @@ -66,4 +66,33 @@ defmodule Urchin.SessionLifecycleTest do Session.terminate(pid) assert_receive :mcp_close, 1_000 end + + test "buffer_limit caps the general-stream replay buffer" do + {:ok, _id, pid} = start(buffer_limit: 1) + Session.notify(pid, "notifications/message", %{"n" => 1}) + Session.notify(pid, "notifications/message", %{"n" => 2}) + + {:ok, "g0", replay} = Session.register_general_stream(pid, self(), {"g0", 0}) + assert length(replay) == 1 + + Session.terminate(pid) + end + + test "mark_initialized/1 synchronously sets initialized" do + {:ok, _id, pid} = start([]) + refute Session.snapshot(pid).initialized + assert :ok = Session.mark_initialized(pid) + assert Session.snapshot(pid).initialized + Session.terminate(pid) + end + + test "buffer_limit: nil falls back to the default replay buffer" do + {:ok, _id, pid} = start(buffer_limit: nil) + Session.notify(pid, "notifications/message", %{"n" => 1}) + + {:ok, "g0", replay} = Session.register_general_stream(pid, self(), {"g0", 0}) + assert length(replay) == 1 + + Session.terminate(pid) + end end diff --git a/test/urchin/transport/streamable_http_auth_test.exs b/test/urchin/transport/streamable_http_auth_test.exs index 8ddcd1e..4ab4591 100644 --- a/test/urchin/transport/streamable_http_auth_test.exs +++ b/test/urchin/transport/streamable_http_auth_test.exs @@ -69,14 +69,20 @@ defmodule Urchin.Transport.StreamableHTTPAuthTest do conn = initialize([{"authorization", "Bearer alice-token"}]) [session_id] = get_resp_header(conn, "mcp-session-id") + authed = [ + {"authorization", "Bearer alice-token"}, + {"mcp-session-id", session_id}, + {"mcp-protocol-version", "2025-11-25"} + ] + + # Complete the handshake so the tool call passes the lifecycle gate. + ack = post(%{jsonrpc: "2.0", method: "notifications/initialized"}, authed) + assert ack.status == 202 + conn = post( %{jsonrpc: "2.0", id: 2, method: "tools/call", params: %{name: "whoami", arguments: %{}}}, - [ - {"authorization", "Bearer alice-token"}, - {"mcp-session-id", session_id}, - {"mcp-protocol-version", "2025-11-25"} - ] + authed ) assert conn.status == 200 diff --git a/test/urchin/transport/streamable_http_test.exs b/test/urchin/transport/streamable_http_test.exs index 66749b7..f50b1b8 100644 --- a/test/urchin/transport/streamable_http_test.exs +++ b/test/urchin/transport/streamable_http_test.exs @@ -52,6 +52,11 @@ defmodule Urchin.Transport.StreamableHTTPTest do assert conn.status == 200 [session_id] = get_resp_header(conn, "mcp-session-id") + + # Complete the handshake so subsequent operation requests pass the lifecycle gate. + ack = call_with_session(%{jsonrpc: "2.0", method: "notifications/initialized"}, session_id) + assert ack.status == 202 + {session_id, Jason.decode!(conn.resp_body)} end @@ -236,7 +241,11 @@ defmodule Urchin.Transport.StreamableHTTPTest do jsonrpc: "2.0", id: 1, method: "initialize", - params: %{"protocolVersion" => "2025-11-25", "capabilities" => %{}} + params: %{ + "protocolVersion" => "2025-11-25", + "capabilities" => %{}, + "clientInfo" => %{"name" => "c", "version" => "1"} + } }, [{"origin", "http://localhost:3000"}] ) @@ -269,6 +278,19 @@ defmodule Urchin.Transport.StreamableHTTPTest do assert wait_for_termination(session_id) == :ok end + test "DELETE rejects an unsupported MCP-Protocol-Version" do + {session_id, _} = init_session() + + conn = + conn(:delete, "/") + |> put_req_header("mcp-session-id", session_id) + |> put_req_header("mcp-protocol-version", "1999-01-01") + |> StreamableHTTP.call(@opts) + + assert conn.status == 400 + assert Jason.decode!(conn.resp_body)["error"]["code"] == -32_600 + end + test "DELETE is 405 when disabled" do opts = StreamableHTTP.init(server: EchoServer, allow_delete: false) {session_id, _} = init_session() @@ -323,4 +345,152 @@ defmodule Urchin.Transport.StreamableHTTPTest do assert status.("x-application/json") == 415 end end + + describe "initialized lifecycle gate" do + test "gates operation requests until notifications/initialized" do + init_conn = + post(%{ + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: %{ + "protocolVersion" => "2025-11-25", + "capabilities" => %{}, + "clientInfo" => %{"name" => "c", "version" => "1"} + } + }) + + assert init_conn.status == 200 + [session_id] = get_resp_header(init_conn, "mcp-session-id") + headers = [{"mcp-session-id", session_id}, {"mcp-protocol-version", "2025-11-25"}] + + # Before notifications/initialized: rejected as invalid_request. + before = post(%{jsonrpc: "2.0", id: 2, method: "tools/list"}, headers) + assert before.status == 200 + assert Jason.decode!(before.resp_body)["error"]["code"] == -32_600 + + # Acknowledge initialization. + ack = post(%{jsonrpc: "2.0", method: "notifications/initialized"}, headers) + assert ack.status == 202 + + # After: allowed. + after_conn = post(%{jsonrpc: "2.0", id: 3, method: "tools/list"}, headers) + assert after_conn.status == 200 + assert is_list(Jason.decode!(after_conn.resp_body)["result"]["tools"]) + end + end + + describe "sse_buffer_limit option" do + test "validates and defaults" do + assert %{sse_buffer_limit: nil} = StreamableHTTP.init(server: EchoServer) + assert %{sse_buffer_limit: 5} = StreamableHTTP.init(server: EchoServer, sse_buffer_limit: 5) + + assert_raise ArgumentError, fn -> + StreamableHTTP.init(server: EchoServer, sse_buffer_limit: 0) + end + + assert_raise ArgumentError, fn -> + StreamableHTTP.init(server: EchoServer, sse_buffer_limit: -1) + end + end + + test "is forwarded to the session created by the transport" do + opts = StreamableHTTP.init(server: EchoServer, sse_buffer_limit: 1) + + conn = + post( + %{ + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: %{ + "protocolVersion" => "2025-11-25", + "capabilities" => %{}, + "clientInfo" => %{"name" => "c", "version" => "1"} + } + }, + [], + opts + ) + + assert conn.status == 200 + [session_id] = get_resp_header(conn, "mcp-session-id") + pid = Urchin.Session.whereis(session_id) + + Urchin.Session.notify(pid, "notifications/message", %{"n" => 1}) + Urchin.Session.notify(pid, "notifications/message", %{"n" => 2}) + + # With the buffer capped at 1, only the most recent event is available to replay. + {:ok, "g0", replay} = Urchin.Session.register_general_stream(pid, self(), {"g0", 0}) + assert length(replay) == 1 + + Urchin.Session.terminate(pid) + end + end + + describe "tool errors over the transport" do + test "a handler {:error, message} becomes an isError result" do + {session_id, _} = init_session() + + conn = + call_with_session( + %{ + jsonrpc: "2.0", + id: 20, + method: "tools/call", + params: %{name: "failing", arguments: %{}} + }, + session_id + ) + + assert conn.status == 200 + body = Jason.decode!(conn.resp_body) + assert body["result"]["isError"] == true + assert body["result"]["content"] == [%{"type" => "text", "text" => "tool said no"}] + end + end + + describe "logging/setLevel over the transport" do + test "updates the session min_log_level" do + {session_id, _} = init_session() + pid = Urchin.Session.whereis(session_id) + + conn = + call_with_session( + %{jsonrpc: "2.0", id: 21, method: "logging/setLevel", params: %{level: "error"}}, + session_id + ) + + assert conn.status == 200 + assert Jason.decode!(conn.resp_body)["result"] == %{} + assert Urchin.Session.snapshot(pid).min_log_level == "error" + end + end + + describe "client notifications before initialized" do + test "client notifications are accepted with 202 before initialized" do + init = + post(%{ + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: %{ + "protocolVersion" => "2025-11-25", + "capabilities" => %{}, + "clientInfo" => %{"name" => "c", "version" => "1"} + } + }) + + [session_id] = get_resp_header(init, "mcp-session-id") + headers = [{"mcp-session-id", session_id}, {"mcp-protocol-version", "2025-11-25"}] + + cancelled = + post( + %{jsonrpc: "2.0", method: "notifications/cancelled", params: %{requestId: "x"}}, + headers + ) + + assert cancelled.status == 202 + end + end end