From 43c2e0d6b6a98f2d6b5c291ecc1c2e31af722171 Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Sat, 26 Sep 2026 15:28:23 -0400 Subject: [PATCH] Review follow-ups for 0.10.1: a late schema defect is -32603, the missing tests, the dead clause, the pattern cost From Sean MacGuire's review of 0.10.1. A tools/call against a schema the catalog made unenforceable after startup is answered -32603 naming the tool and the keyword (it was a tool result saying "invalid arguments", blaming the client); the new test fails on 0.10.1's server. Tests for additionalProperties as a schema (it kills the review's surviving mutant, the branch replaced by :ok), a list-form type refusing a value of none of its types, and every README keyword accepted well-formed; the `when key in @enforced` clause, which no keyword could reach, and the list it read are removed. The threat model's arguments row says what `pattern` costs (PCRE's match limit, fail closed; 80 ms measured for the worst case a lane tried), inside the row so no threat or status moves. CHANGELOG: the change, and 0.10.1's `nil` reason reading `null`, recorded late. The stdio test helper says why its device is latin1. Gate: sixteen steps pass. Signed-off-by: Ayla Croft --- CHANGELOG.md | 26 ++++++ docs/threat-model.md | 2 +- lib/beam_mcp/schema.ex | 23 ----- lib/beam_mcp/server.ex | 21 +++++ .../arguments_schema_and_faults_test.exs | 91 +++++++++++++++++++ 5 files changed, 139 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd73991..7f65387 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,32 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +Follow-ups from the first human review of 0.10.1 (Sean MacGuire). No public entry moves. + +### Changed: a schema that becomes unenforceable after startup is a server fault + +- `tools/call` against a tool whose schema the catalog changed, after startup, to one this + package cannot enforce (`oneOf` and the rest; startup refuses it) is answered `-32603` + naming the tool and the keyword. 0.10.1 answered it as a tool result saying "invalid + arguments", which blamed the client for the server's defect. **How to tell whether you are + affected:** only a catalog whose `capabilities/0` answer changes after + `BeamMCP.Server.new/1`. + +### Added: tests the review found missing + +- `additionalProperties` given as a schema (each undeclared property held to it, the path + named), a list-form `type` refusing a value of none of its types, and each keyword the + README lists accepted with a well-formed value. The clause that accepted "any other + enforced keyword" is removed: every keyword has its own clause, so it could never run. + +### Changed: pages + +- The threat model's arguments row says what `pattern` costs: the host's pattern against the + client's string, bounded by PCRE's match limit and refused past it. Not a new threat or + status: the row's claim is unchanged. +- Recorded late for 0.10.1: a tool error whose reason is `nil` now reads `null` in the result + text (it read `nil`), as JSON writes it. + ## [0.10.1] - 2026-09-26 A security release. Found by the project's own security review (three independent review diff --git a/docs/threat-model.md b/docs/threat-model.md index 11e7f87..f17757b 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -60,7 +60,7 @@ reading of each entry's title, not a claim of coverage. | **Connection floods; too many in flight** | DELEGATED | Each in-flight request at the body cap costs about 1.05 MiB, linear to 8,000 concurrent with no plateau (measured 2026-09-07); the ceiling on how many is `num_acceptors × num_connections` of the server (100 × 16,384 under `Bandit`'s defaults), the host's capacity decision. The nesting bound above is what keeps that per-request figure at the body's size rather than thirty-six times it. | none (a server setting) | LLM10:2025 | | **Header count and size; TLS** | DELEGATED | Both the HTTP server's. This Plug speaks plaintext to the server that terminates TLS for it; a deployment that needs TLS configures it there. | none | none | | **Unsafe deserialization; code from input** | REFUSED by construction | Input is decoded by `Jason` to strings, numbers, lists and maps and nothing else: no `binary_to_term`, no evaluator, no module or function built from a name in the input, no atom created from a caller's key; 10,000 distinct keys through `tools/call` and `prompts/get` leave the atom table where it was. The `:xref` census over the built beams pins every module the package calls and, on the modules through which code, secrets, the OS or another node could be reached, every function. | `test/beam_mcp/argument_interning_test.exs` "10,000 distinct caller keys through prompts/get and tools/call leave the atom table where it was"; `test/beam_mcp/boundary/no_dynamic_evaluation_test.exs` "no line under lib/ evaluates code or builds a name at runtime, beyond the argument-key atoms"; `test/beam_mcp/boundary/package_reach_test.exs` "the modules the package calls are exactly the listed ones" "on the modules that could reach code, names, secrets, the OS or another node, the functions called are exactly the listed ones" | ASI05 Unexpected Code Execution | -| **Tool arguments the schema does not admit** | REFUSED | `tools/call` validates arguments against the schema `tools/list` advertised (the same struct, one lookup, so the two cannot disagree), and a call the schema does not admit, at any depth (a missing required property, a forbidden one, a value outside its `enum`, `pattern` or bounds, an array item of the wrong type), is refused before dispatch, as a tool error result (`200`, `isError: true`, the text naming the property), which is the shape the specification gives a call the tool could not take, not a JSON-RPC error. A schema keyword the server would not enforce (`oneOf`, `$ref` and the rest) is refused when the catalog is loaded, so nothing is advertised that is not held. JSON `true`, `false` and `null` reach dispatch as `true`, `false` and `nil`. What a *valid* call does is the host's tool. | `test/beam_mcp/tool_spec_schema_test.exs` "a call missing a required property the catalog declared is refused" "a call carrying a property the catalog's schema forbids is refused" "tools/list advertises the schema the catalog carries"; `test/beam_mcp/arguments_schema_and_faults_test.exs` "a nested additionalProperties: false is refused, naming the path" "a schema using a keyword the server does not enforce is refused at startup, by name" "false is false, true is true, null is nil, at the top level and nested" | ASI02 Tool Misuse (the wire half) | +| **Tool arguments the schema does not admit** | REFUSED | `tools/call` validates arguments against the schema `tools/list` advertised (the same struct, one lookup, so the two cannot disagree), and a call the schema does not admit, at any depth (a missing required property, a forbidden one, a value outside its `enum`, `pattern` or bounds, an array item of the wrong type), is refused before dispatch, as a tool error result (`200`, `isError: true`, the text naming the property), which is the shape the specification gives a call the tool could not take, not a JSON-RPC error. A schema keyword the server would not enforce (`oneOf`, `$ref` and the rest) is refused when the catalog is loaded, so nothing is advertised that is not held. JSON `true`, `false` and `null` reach dispatch as `true`, `false` and `nil`. What a *valid* call does is the host's tool. The cost of `pattern` is the host's pattern against the client's string: one that backtracks (`^(a+)+$`) runs until PCRE's default match limit and then refuses the call (fail closed); a review lane measured about 80 ms for 5,000 `a` and a `!`, and a pattern without nested quantifiers is the host's part. | `test/beam_mcp/tool_spec_schema_test.exs` "a call missing a required property the catalog declared is refused" "a call carrying a property the catalog's schema forbids is refused" "tools/list advertises the schema the catalog carries"; `test/beam_mcp/arguments_schema_and_faults_test.exs` "a nested additionalProperties: false is refused, naming the path" "a schema using a keyword the server does not enforce is refused at startup, by name" "false is false, true is true, null is nil, at the top level and nested" | ASI02 Tool Misuse (the wire half) | | **A session or identity to steal or replay** | none exists | No session identifier is issued, honoured or read; each request stands alone; identity is the host's `authorize/1`, and this package holds no key and makes no signature of its own (a host-supplied signer signs the canonical bytes through one pinned callback; the key stays with it), so there is none to steal from it. | `test/beam_mcp/boundary/no_session_test.exs` "no response carries an mcp-session-id header, and a request carrying one is answered as if it did not"; `test/beam_mcp/boundary/no_key_holding_test.exs` "no line under lib/ names key material or calls a crypto function other than :crypto.hash/2"; `test/beam_mcp/boundary/no_signature_test.exs` "no line under lib/ calls a signing or MAC primitive" | ASI03 | | **What a refusal or a fault leaks** | BOUNDED | An error carries structured fields, never an inspected Elixir term, and a `-32603` names the broken contract (the callback, the defect), never the host's module or the term it returned; a host's error term goes out as JSON; a host fault (raise, throw, exit in dispatch, a hook, or the catalog) is answered `-32603` with the id and no stacktrace, `500` over HTTP; on stdio the same object and the loop goes on, a response the encoder refuses included (until this page a fault in the host's dispatch ended the stdio loop with nothing written). On stdio the fault is reported on standard error, never standard output, which is the protocol's. The `:telemetry` exception event and both transports' fault reports carry a stacktrace of **arities**, never arguments, so no frame carries the HTTP request, its `Authorization` header or its body. The exception's own message is logged as the host's code raised it: a hook that fails to match the conn raises a `MatchError` whose message prints the conn, and that is the host's code and the host's log. The stdio transport's report of a response it could not encode names the exception, not the bytes. | `test/beam_mcp/error_payload_test.exs` "a validation failure carries structured fields, not an inspected map" "the human-readable content carries no Elixir syntax either"; `test/beam_mcp/transport/http_test.exs` "throw and exit are answered, not left as an empty 500" "a host authorize/1 that raises, throws or exits is answered, not left as a bare 500"; `test/beam_mcp/threat_model_test.exs` "a host dispatch that raises, throws or exits is answered -32603 with the id, and the loop keeps going"; `test/beam_mcp/connectome/observed_test.exs` "the :exception stacktrace carries arities, never arguments: a function_clause or a BIF error would have put the call's arguments in its top frame"; `test/beam_mcp/arguments_schema_and_faults_test.exs` "a fault's report goes to standard error, and every stdout line is a JSON-RPC message" "a reader answering an off-contract shape" "a hook that raises function_clause: the Authorization header and the body stay out of the log" | LLM02:2025 Sensitive Information Disclosure | | **A payload byte in the observed graph** | REFUSED by construction | The observed graph carries edge identity only (caller, callee, kind, a count), never an argument, a result or a message term; the tracer traces with the `:arity` flag and never reads a message. | `test/beam_mcp/readme_claims_test.exs` "the observed graph carries edge identity only, never a payload byte"; `test/beam_mcp/connectome/tracer_test.exs` "a traced call is a module-level :invoke edge from the caller's module to the callee's, and nothing of the arguments" | LLM02:2025 | diff --git a/lib/beam_mcp/schema.ex b/lib/beam_mcp/schema.ex index 48c5150..0eb5f61 100644 --- a/lib/beam_mcp/schema.ex +++ b/lib/beam_mcp/schema.ex @@ -57,27 +57,6 @@ defmodule BeamMCP.Schema do # `:dollar_endonly` keeps `$` from matching before a trailing newline. @pattern_options [:unicode, :dollar_endonly] - @enforced [ - "type", - "enum", - "const", - "properties", - "required", - "additionalProperties", - "minProperties", - "maxProperties", - "items", - "minItems", - "maxItems", - "uniqueItems", - "minLength", - "maxLength", - "pattern", - "minimum", - "maximum", - "exclusiveMinimum", - "exclusiveMaximum" - ] @annotations [ "title", "description", @@ -209,8 +188,6 @@ defmodule BeamMCP.Schema do if is_number(value), do: :ok, else: {:error, "#{where}: #{key} must be a number"} end - defp check_keyword(key, _value, _where) when key in @enforced, do: :ok - defp check_keyword(key, _value, where) when is_binary(key), do: {:error, "#{where} uses #{key}, which this server does not enforce"} diff --git a/lib/beam_mcp/server.ex b/lib/beam_mcp/server.ex index 99971cc..fedfec1 100644 --- a/lib/beam_mcp/server.ex +++ b/lib/beam_mcp/server.ex @@ -530,6 +530,9 @@ defmodule BeamMCP.Server do {:error, reason} -> result(id, tool_failure(reason)) + + {:schema_defect, reason} -> + error(id, -32_603, "Internal error: tool #{name}: #{reason}") end {state, response} @@ -908,7 +911,25 @@ defmodule BeamMCP.Server do # The advertised schema is the contract. Validate the wire form -- string keys, as the # client sent them -- before normalising, so `required` and `additionalProperties` # mean what tools/list says they mean. + # + # A schema this package cannot enforce is refused at startup, so reaching one here means the + # catalog's answer changed after it: a server defect, answered -32603 naming the tool and + # the keyword (both already advertised in tools/list), never an invalid-arguments result + # that would blame the client. defp validate_and_dispatch(state, %ToolSpec{} = spec, arguments) do + with :ok <- schema_enforceable(spec.input_schema) do + check_and_dispatch(state, spec, arguments) + end + end + + defp schema_enforceable(schema) do + case Schema.check_schema(schema) do + :ok -> :ok + {:error, reason} -> {:schema_defect, reason} + end + end + + defp check_and_dispatch(state, spec, arguments) do case Schema.validate(arguments, spec.input_schema) do :ok -> args = normalize_arguments(arguments, spec.input_schema) diff --git a/test/beam_mcp/arguments_schema_and_faults_test.exs b/test/beam_mcp/arguments_schema_and_faults_test.exs index 4a7cb41..99608cf 100644 --- a/test/beam_mcp/arguments_schema_and_faults_test.exs +++ b/test/beam_mcp/arguments_schema_and_faults_test.exs @@ -77,6 +77,29 @@ defmodule BeamMCP.ArgumentsSchemaAndFaultsTest do def read_resource("r://baditem"), do: {:ok, [%{token: "ITEMSECRET"}]} end + # A catalog whose answer can change after startup: capabilities/0 runs in the calling + # process, so the schema is read from its dictionary on every call. + defmodule ChangingCatalog do + @behaviour BeamMCP.Catalog + + @impl true + def capabilities do + %{ + tools: [ + %BeamMCP.ToolSpec{ + name: :later, + command_class: :observe, + mode: :read_only, + description: "l", + input_schema: Process.get(:later_schema, %{"type" => "object"}) + } + ], + resources: [], + prompts: [] + } + end + end + defmodule OneOfCatalog do @behaviour BeamMCP.Catalog @@ -177,6 +200,71 @@ defmodule BeamMCP.ArgumentsSchemaAndFaultsTest do assert {:dispatched, %{opts: %{"mode" => "safe"}, q: "abc", tags: ["x"]}} = dispatched() end + test "additionalProperties as a schema checks every undeclared property, naming its path" do + schema = %{ + "type" => "object", + "properties" => %{"a" => %{"type" => "string"}}, + "additionalProperties" => %{"type" => "integer"} + } + + assert {:error, reason} = BeamMCP.Schema.validate(%{"a" => "x", "extra" => "no"}, schema) + assert reason =~ "extra must be of type integer" + assert :ok = BeamMCP.Schema.validate(%{"a" => "x", "extra" => 1}, schema) + end + + test "a list-form type refuses a value of none of its types" do + call(%{"note" => 1}) + assert :not_dispatched = dispatched() + call(%{"note" => "fine"}) + assert {:dispatched, %{note: "fine"}} = dispatched() + end + + test "every keyword the README lists is accepted with a well-formed value" do + for {key, value} <- [ + {"type", "object"}, + {"enum", [1]}, + {"const", 1}, + {"properties", %{}}, + {"required", []}, + {"additionalProperties", false}, + {"minProperties", 0}, + {"maxProperties", 1}, + {"items", %{}}, + {"minItems", 0}, + {"maxItems", 1}, + {"uniqueItems", true}, + {"minLength", 0}, + {"maxLength", 1}, + {"pattern", "^a$"}, + {"minimum", 0}, + {"maximum", 1}, + {"exclusiveMinimum", 0}, + {"exclusiveMaximum", 1} + ] do + assert :ok = BeamMCP.Schema.check_schema(%{key => value}), key + end + end + + test "a schema that becomes unenforceable after startup is a server fault (-32603), never blamed on the client" do + st = Server.new(catalog: ChangingCatalog, dispatch: fn _, a, _ -> {:ok, a} end) + Process.put(:later_schema, %{"type" => "object", "oneOf" => []}) + + {_state, response} = + Server.handle_message(st, %{ + "jsonrpc" => "2.0", + "id" => 5, + "method" => "tools/call", + "params" => %{"name" => "later", "arguments" => %{}, "_meta" => @meta} + }) + + assert %{"id" => 5, "error" => %{"code" => -32_603, "message" => message}} = response + assert message =~ "tool later" + assert message =~ "oneOf" + refute Map.has_key?(response, "result") + after + Process.delete(:later_schema) + end + test "a schema using a keyword the server does not enforce is refused at startup, by name" do error = assert_raise ArgumentError, fn -> Server.new(catalog: OneOfCatalog) end assert error.message =~ "oneOf" @@ -289,6 +377,9 @@ defmodule BeamMCP.ArgumentsSchemaAndFaultsTest do end describe "stdio: standard output carries protocol messages only" do + # The device is opened as latin1 so the transport reads the input's raw bytes, byte for + # byte, as it reads a real pipe (the file is UTF-8 throughout; this is the device's mode, + # not the text's). defp drive(input, opts) do {:ok, device} = StringIO.open(input, encoding: :latin1) original = Process.group_leader()