fix: reject content/bytes on MCP tools
Harden stamp/verify/hash_report so unknown and content-shaped args fail instead of being ignored; publish additionalProperties=false and cover with schema/runtime tests. Also keep streamable-http host allowlist.
This commit is contained in:
parent
e96fa13edd
commit
326d90a9db
11 changed files with 258 additions and 9 deletions
|
|
@ -0,0 +1,4 @@
|
||||||
|
# Done and not done
|
||||||
|
|
||||||
|
Done: guard.py, server harden, tests, pytest 19 passed.
|
||||||
|
Not done: WASM client, git commit.
|
||||||
|
|
@ -0,0 +1,6 @@
|
||||||
|
# How to implement
|
||||||
|
|
||||||
|
1. Add guard.py
|
||||||
|
2. Harden in build_mcp
|
||||||
|
3. Expand tests
|
||||||
|
4. uv run pytest -q
|
||||||
|
|
@ -0,0 +1,3 @@
|
||||||
|
# Recommended changes: reject content/bytes
|
||||||
|
|
||||||
|
Review 2026-09-22. See What-to-Change.md. Run: uv run pytest -q
|
||||||
|
|
@ -0,0 +1,3 @@
|
||||||
|
# What to Change To
|
||||||
|
|
||||||
|
Reject forbidden content keys; forbid unknown args; additionalProperties false; tests pass.
|
||||||
|
|
@ -0,0 +1,3 @@
|
||||||
|
# What to Change
|
||||||
|
|
||||||
|
Tool arg models ignored unknown keys so content/bytes/file could be sent and silently dropped. Schema lacked additionalProperties=false.
|
||||||
|
|
@ -0,0 +1,4 @@
|
||||||
|
{
|
||||||
|
"head": "e96fa13eddc34c0ad368990189f6c10f36b3ac86",
|
||||||
|
"dirty": true
|
||||||
|
}
|
||||||
|
|
@ -0,0 +1,4 @@
|
||||||
|
# git-versions
|
||||||
|
|
||||||
|
- verae-mcp-timestamping-server main e96fa13-dirty
|
||||||
|
- HEAD: e96fa13eddc34c0ad368990189f6c10f36b3ac86
|
||||||
73
src/verae_mcp_timestamping_server/guard.py
Normal file
73
src/verae_mcp_timestamping_server/guard.py
Normal file
|
|
@ -0,0 +1,73 @@
|
||||||
|
"""Hash-only contract: reject content/bytes-style tool arguments."""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from typing import Any, Mapping
|
||||||
|
|
||||||
|
from pydantic import ConfigDict, Field, ValidationError, create_model
|
||||||
|
|
||||||
|
from mcp.server.mcpserver.utilities.func_metadata import ArgModelBase
|
||||||
|
|
||||||
|
FORBIDDEN_CONTENT_KEYS = frozenset(
|
||||||
|
{
|
||||||
|
"content",
|
||||||
|
"bytes",
|
||||||
|
"file",
|
||||||
|
"data",
|
||||||
|
"path",
|
||||||
|
"body",
|
||||||
|
"payload",
|
||||||
|
"blob",
|
||||||
|
"archive",
|
||||||
|
"filename",
|
||||||
|
"filepath",
|
||||||
|
"file_path",
|
||||||
|
"raw",
|
||||||
|
"plaintext",
|
||||||
|
"document",
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
_REJECT = "content/bytes paths are rejected; send sha256 hex only"
|
||||||
|
|
||||||
|
|
||||||
|
class StrictArgModel(ArgModelBase):
|
||||||
|
"""Tool args: unknown keys (including content/bytes) are errors."""
|
||||||
|
|
||||||
|
model_config = ConfigDict(arbitrary_types_allowed=True, extra="forbid")
|
||||||
|
|
||||||
|
|
||||||
|
def reject_content_keys(arguments: Mapping[str, Any] | None) -> None:
|
||||||
|
"""Raise ValidationError if arguments include any content/bytes-style key."""
|
||||||
|
if not arguments:
|
||||||
|
return
|
||||||
|
hit = sorted({k for k in arguments if k.lower() in FORBIDDEN_CONTENT_KEYS})
|
||||||
|
if not hit:
|
||||||
|
return
|
||||||
|
raise ValidationError.from_exception_data(
|
||||||
|
"HashOnlyArguments",
|
||||||
|
[
|
||||||
|
{
|
||||||
|
"type": "value_error",
|
||||||
|
"loc": (key,),
|
||||||
|
"input": arguments.get(key),
|
||||||
|
"ctx": {"error": _REJECT},
|
||||||
|
}
|
||||||
|
for key in hit
|
||||||
|
],
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def strict_arg_model(existing: type[ArgModelBase]) -> type[ArgModelBase]:
|
||||||
|
"""Rebuild a tool arg model with extra='forbid'."""
|
||||||
|
fields: dict[str, Any] = {}
|
||||||
|
for name, finfo in existing.model_fields.items():
|
||||||
|
annotation = finfo.annotation
|
||||||
|
kwargs: dict[str, Any] = {}
|
||||||
|
if finfo.alias:
|
||||||
|
kwargs["alias"] = finfo.alias
|
||||||
|
if finfo.is_required():
|
||||||
|
fields[name] = (annotation, Field(**kwargs))
|
||||||
|
else:
|
||||||
|
fields[name] = (annotation, Field(default=finfo.default, **kwargs))
|
||||||
|
return create_model(f"{existing.__name__}Strict", __base__=StrictArgModel, **fields)
|
||||||
|
|
@ -8,6 +8,7 @@ from mcp.server import MCPServer
|
||||||
from mcp.server.mcpserver.exceptions import ToolError
|
from mcp.server.mcpserver.exceptions import ToolError
|
||||||
|
|
||||||
from verae_mcp_timestamping_server.global_client import GlobalClient, GlobalLedger
|
from verae_mcp_timestamping_server.global_client import GlobalClient, GlobalLedger
|
||||||
|
from verae_mcp_timestamping_server.guard import reject_content_keys, strict_arg_model
|
||||||
from verae_mcp_timestamping_server.ledger import (
|
from verae_mcp_timestamping_server.ledger import (
|
||||||
FileLedger,
|
FileLedger,
|
||||||
Ledger,
|
Ledger,
|
||||||
|
|
@ -28,9 +29,38 @@ def default_ledger() -> Ledger:
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
_HARDENED_TOOLS: set[int] = set()
|
||||||
|
|
||||||
|
|
||||||
|
def _harden_hash_only_tools(server: MCPServer) -> None:
|
||||||
|
"""Forbid unknown args (content/bytes included); advertise additionalProperties=false."""
|
||||||
|
for tool in server._tool_manager.list_tools():
|
||||||
|
tid = id(tool)
|
||||||
|
if tid in _HARDENED_TOOLS:
|
||||||
|
continue
|
||||||
|
meta = tool.fn_metadata
|
||||||
|
object.__setattr__(meta, "arg_model", strict_arg_model(meta.arg_model))
|
||||||
|
|
||||||
|
def _make_validate(m):
|
||||||
|
def validate_arguments(arguments_to_validate):
|
||||||
|
reject_content_keys(arguments_to_validate)
|
||||||
|
arguments_pre_parsed = m.pre_parse_json(arguments_to_validate)
|
||||||
|
arguments_parsed_model = m.arg_model.model_validate(arguments_pre_parsed)
|
||||||
|
return arguments_parsed_model.model_dump_one_level()
|
||||||
|
|
||||||
|
return validate_arguments
|
||||||
|
|
||||||
|
object.__setattr__(meta, "validate_arguments", _make_validate(meta))
|
||||||
|
params = meta.arg_model.model_json_schema(by_alias=True)
|
||||||
|
params["additionalProperties"] = False
|
||||||
|
object.__setattr__(tool, "parameters", params)
|
||||||
|
_HARDENED_TOOLS.add(tid)
|
||||||
|
|
||||||
|
|
||||||
def build_mcp(ledger: Ledger | None = None) -> MCPServer:
|
def build_mcp(ledger: Ledger | None = None) -> MCPServer:
|
||||||
global _ledger
|
global _ledger
|
||||||
_ledger = ledger or default_ledger()
|
_ledger = ledger or default_ledger()
|
||||||
|
_harden_hash_only_tools(mcp)
|
||||||
return mcp
|
return mcp
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -78,11 +108,31 @@ def main() -> None:
|
||||||
if transport == "streamable-http":
|
if transport == "streamable-http":
|
||||||
host = os.environ.get("VERAE_MCP_HOST", "127.0.0.1")
|
host = os.environ.get("VERAE_MCP_HOST", "127.0.0.1")
|
||||||
port = int(os.environ.get("VERAE_MCP_PORT", "8000"))
|
port = int(os.environ.get("VERAE_MCP_PORT", "8000"))
|
||||||
|
from mcp.server.transport_security import TransportSecuritySettings
|
||||||
|
|
||||||
|
# Behind Caddy on ns1; allow public hostname + local probes.
|
||||||
|
allowed = [
|
||||||
|
h.strip()
|
||||||
|
for h in os.environ.get(
|
||||||
|
"VERAE_MCP_ALLOWED_HOSTS",
|
||||||
|
"mcp.georgelambert.org,localhost,localhost:8000,127.0.0.1,127.0.0.1:8000,10.1.10.128,10.1.10.128:8000",
|
||||||
|
).split(",")
|
||||||
|
if h.strip()
|
||||||
|
]
|
||||||
|
transport_security = TransportSecuritySettings(
|
||||||
|
enable_dns_rebinding_protection=True,
|
||||||
|
allowed_hosts=allowed,
|
||||||
|
allowed_origins=os.environ.get("VERAE_MCP_ALLOWED_ORIGINS", "*").split(","),
|
||||||
|
)
|
||||||
if os.environ.get("VERAE_MCP_CORS") == "1":
|
if os.environ.get("VERAE_MCP_CORS") == "1":
|
||||||
import uvicorn
|
import uvicorn
|
||||||
from starlette.middleware.cors import CORSMiddleware
|
from starlette.middleware.cors import CORSMiddleware
|
||||||
|
|
||||||
app = mcp.streamable_http_app(json_response=True)
|
app = mcp.streamable_http_app(
|
||||||
|
json_response=True,
|
||||||
|
transport_security=transport_security,
|
||||||
|
host=host,
|
||||||
|
)
|
||||||
app.add_middleware(
|
app.add_middleware(
|
||||||
CORSMiddleware,
|
CORSMiddleware,
|
||||||
allow_origins=["*"],
|
allow_origins=["*"],
|
||||||
|
|
@ -92,7 +142,12 @@ def main() -> None:
|
||||||
)
|
)
|
||||||
uvicorn.run(app, host=host, port=port)
|
uvicorn.run(app, host=host, port=port)
|
||||||
return
|
return
|
||||||
mcp.run(transport="streamable-http", host=host, port=port)
|
mcp.run(
|
||||||
|
transport="streamable-http",
|
||||||
|
host=host,
|
||||||
|
port=port,
|
||||||
|
transport_security=transport_security,
|
||||||
|
)
|
||||||
return
|
return
|
||||||
mcp.run()
|
mcp.run()
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -52,3 +52,26 @@ async def test_hash_report_is_json_array():
|
||||||
report = await ledger.hash_report(SHA256_EMPTY)
|
report = await ledger.hash_report(SHA256_EMPTY)
|
||||||
assert isinstance(report, list)
|
assert isinstance(report, list)
|
||||||
assert report[0].hash == SHA256_EMPTY
|
assert report[0].hash == SHA256_EMPTY
|
||||||
|
|
||||||
|
|
||||||
|
def test_validate_sha256_rejects_content_shaped_strings():
|
||||||
|
from verae_mcp_timestamping_server.ledger import validate_sha256
|
||||||
|
import base64
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
with pytest.raises(ValueError, match="sha256"):
|
||||||
|
validate_sha256("")
|
||||||
|
with pytest.raises(ValueError, match="sha256"):
|
||||||
|
validate_sha256(base64.b64encode(b"hello world file bytes").decode())
|
||||||
|
with pytest.raises(ValueError, match="sha256"):
|
||||||
|
validate_sha256("line1\nline2")
|
||||||
|
|
||||||
|
|
||||||
|
def test_reject_content_keys_lists_forbidden():
|
||||||
|
import pytest
|
||||||
|
from pydantic import ValidationError
|
||||||
|
from verae_mcp_timestamping_server.guard import reject_content_keys
|
||||||
|
|
||||||
|
with pytest.raises(ValidationError, match="content/bytes paths are rejected"):
|
||||||
|
reject_content_keys({"sha256": "a" * 64, "content": "nope"})
|
||||||
|
reject_content_keys({"sha256": "a" * 64, "scope": "local"}) # ok
|
||||||
|
|
|
||||||
|
|
@ -1,14 +1,26 @@
|
||||||
"""In-memory MCP Client against hash-only tools."""
|
"""In-memory MCP Client against hash-only tools."""
|
||||||
|
|
||||||
|
import base64
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
from mcp import Client
|
from mcp import Client
|
||||||
|
|
||||||
|
from verae_mcp_timestamping_server.guard import FORBIDDEN_CONTENT_KEYS
|
||||||
from verae_mcp_timestamping_server.ledger import SHA256_EMPTY, MemoryLedger
|
from verae_mcp_timestamping_server.ledger import SHA256_EMPTY, MemoryLedger
|
||||||
from verae_mcp_timestamping_server.server import build_mcp
|
from verae_mcp_timestamping_server.server import build_mcp
|
||||||
|
|
||||||
pytestmark = pytest.mark.anyio
|
pytestmark = pytest.mark.anyio
|
||||||
|
|
||||||
|
|
||||||
|
def _error_text(result) -> str:
|
||||||
|
parts = []
|
||||||
|
for block in result.content or []:
|
||||||
|
text = getattr(block, "text", None)
|
||||||
|
if text:
|
||||||
|
parts.append(text)
|
||||||
|
return "\n".join(parts)
|
||||||
|
|
||||||
|
|
||||||
async def test_stamp_tool_returns_receipts():
|
async def test_stamp_tool_returns_receipts():
|
||||||
mcp = build_mcp(MemoryLedger())
|
mcp = build_mcp(MemoryLedger())
|
||||||
async with Client(mcp, raise_exceptions=True) as client:
|
async with Client(mcp, raise_exceptions=True) as client:
|
||||||
|
|
@ -23,16 +35,75 @@ async def test_stamp_tool_returns_receipts():
|
||||||
assert receipts[0]["scope"] == "local"
|
assert receipts[0]["scope"] == "local"
|
||||||
|
|
||||||
|
|
||||||
async def test_stamp_does_not_accept_content_bytes():
|
async def test_stamp_and_hash_report_schemas_forbid_content_bytes():
|
||||||
mcp = build_mcp(MemoryLedger())
|
mcp = build_mcp(MemoryLedger())
|
||||||
async with Client(mcp, raise_exceptions=True) as client:
|
async with Client(mcp, raise_exceptions=True) as client:
|
||||||
listed = await client.list_tools()
|
listed = await client.list_tools()
|
||||||
stamp = next(t for t in listed.tools if t.name == "stamp")
|
by_name = {t.name: t for t in listed.tools}
|
||||||
props = stamp.input_schema["properties"]
|
for name in ("stamp", "hash_report", "verify"):
|
||||||
assert "sha256" in props
|
tool = by_name[name]
|
||||||
assert "content" not in props
|
props = tool.input_schema["properties"]
|
||||||
assert "bytes" not in props
|
for key in FORBIDDEN_CONTENT_KEYS:
|
||||||
assert "file" not in props
|
assert key not in props, f"{name} schema still lists {key}"
|
||||||
|
assert tool.input_schema.get("additionalProperties") is False
|
||||||
|
assert "sha256" in by_name["stamp"].input_schema["properties"]
|
||||||
|
assert "sha256" in by_name["hash_report"].input_schema["properties"]
|
||||||
|
|
||||||
|
|
||||||
|
async def test_stamp_rejects_content_bytes_at_runtime():
|
||||||
|
mcp = build_mcp(MemoryLedger())
|
||||||
|
async with Client(mcp) as client:
|
||||||
|
r1 = await client.call_tool(
|
||||||
|
"stamp",
|
||||||
|
{
|
||||||
|
"sha256": SHA256_EMPTY,
|
||||||
|
"scope": "local",
|
||||||
|
"content": "this is file bytes",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
assert r1.is_error is True
|
||||||
|
assert "content/bytes paths are rejected" in _error_text(r1)
|
||||||
|
|
||||||
|
r2 = await client.call_tool(
|
||||||
|
"stamp",
|
||||||
|
{
|
||||||
|
"sha256": SHA256_EMPTY,
|
||||||
|
"bytes": base64.b64encode(b"payload").decode(),
|
||||||
|
},
|
||||||
|
)
|
||||||
|
assert r2.is_error is True
|
||||||
|
assert "content/bytes paths are rejected" in _error_text(r2)
|
||||||
|
|
||||||
|
r3 = await client.call_tool(
|
||||||
|
"hash_report",
|
||||||
|
{"sha256": SHA256_EMPTY, "file": "/tmp/secret.bin"},
|
||||||
|
)
|
||||||
|
assert r3.is_error is True
|
||||||
|
assert "content/bytes paths are rejected" in _error_text(r3)
|
||||||
|
|
||||||
|
|
||||||
|
async def test_stamp_rejects_unknown_extra_keys():
|
||||||
|
mcp = build_mcp(MemoryLedger())
|
||||||
|
async with Client(mcp) as client:
|
||||||
|
result = await client.call_tool(
|
||||||
|
"stamp",
|
||||||
|
{"sha256": SHA256_EMPTY, "scope": "local", "not_a_real_field": 1},
|
||||||
|
)
|
||||||
|
assert result.is_error is True
|
||||||
|
assert "Extra inputs are not permitted" in _error_text(result)
|
||||||
|
|
||||||
|
|
||||||
|
async def test_stamp_rejects_non_digest_masquerading_as_sha256():
|
||||||
|
mcp = build_mcp(MemoryLedger())
|
||||||
|
async with Client(mcp) as client:
|
||||||
|
long_b64 = base64.b64encode(b"x" * 200).decode()
|
||||||
|
multiline = "line1\nline2\nline3"
|
||||||
|
for bad in ("not-a-hash", "", long_b64, multiline, "a" * 128):
|
||||||
|
result = await client.call_tool(
|
||||||
|
"stamp", {"sha256": bad, "scope": "local"}
|
||||||
|
)
|
||||||
|
assert result.is_error is True, bad
|
||||||
|
assert "sha256" in _error_text(result).lower()
|
||||||
|
|
||||||
|
|
||||||
async def test_verify_and_hash_report_tools():
|
async def test_verify_and_hash_report_tools():
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue