docs: add an MCP security guide and point the README at it - #29
Conversation
The README answers "which package do I install" and "how do I publish". Both are useful and neither is a question anyone types. Nothing in this repo answered "how do I secure an MCP server", which is the query cluster these packages exist to serve and where we are currently absent. docs/mcp-security.md covers the failure modes at the tool boundary (tool poisoning, injection that becomes a tool call, excessive agency, confused deputy, exfiltration by chaining, unbounded consumption), why WAF and gateway controls cannot see them, what to record if you want to detect them, and the OWASP mapping. Written to be useful whatever you instrument with. The what-to-record table names no vendor, the OWASP section says the agentic list is still moving, and the cerberus-mcp section states its limits plainly: it observes a server rather than a model, gateway coverage trades arguments for reach, and the package is the sensor rather than the detection. The README intro now leads with what the packages give you instead of describing itself as a monorepo, and links the guide.
|
Review: docs: add an MCP security guide and point the README at it Docs-only PR (README intro/nav + new Accuracy issue: source-IP pseudonymization is stated as unconditional, but it isn't
and the new README line makes the same claim repo-wide:
In This matters because the doc's own config example a few lines above omits mcp = CerberusMCP("my-server", cerberus_config={"token": "...", "client_id": "...", "ws_url": "wss://..."})A reader who copies that snippet gets plaintext source IPs, directly contradicting the sentence that follows it. Given the PR's stated goal of being "deliberately honest about limits," this is the one place the guide overstates a guarantee — worth either adding Minor / non-blocking
Overall: solid, accurate-by-default writing with a clear non-vendor-locked framing (the "what to record" table and OWASP mapping apply regardless of instrumentation choice). Fix the IP-pseudonymization overstatement and this is good to merge. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6cbebf64c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The guide said gateway spans carry tool names without arguments, full stop, and that server-side instrumentation was the only way to see arguments at all. The first half is true only of MCP spans. The gateway's LLM spans carry the model's requested tool calls, and the bridge already reconstructs them onto the LLM event by default, so a gateway does give argument-level visibility on Anthropic including streaming, and on OpenAI non-streaming and the Responses API. The real limit is narrower and worth stating precisely: OpenAI streaming loses tool-call arguments upstream, and streaming is what most agent traffic uses. Adds a section separating requested arguments from observed ones, with the per-path coverage table, and notes that a gap between the two is itself a signal that something rewrote the call.
A per-provider coverage table on a page about securing MCP servers is the wrong place for it. Three lines and a pointer to the gateway README, which already documents this properly.
Keeps the row, so anyone deploying against streaming OpenAI knows tool arguments will not be there rather than discovering it by debugging. Drops the upstream mechanism, the note about how much traffic it affects, and the steer toward other backends. The scope stays, since knowing Anthropic and non-streamed OpenAI are unaffected is what makes the row actionable.
ReviewWell-written, well-scoped docs addition — clear structure, no fluff, and the "why network controls miss this" and requested-vs-observed-arguments sections are genuinely useful rather than marketing copy. Link check confirms all 26 relative links resolve, and there are no em-dashes introduced in the new content. The gateway argument-capture claims (Anthropic streaming captured, OpenAI streaming lost, Two accuracy issues in the "What cerberus-mcp does" section (
Neither blocks merge on its own, but both are one-sentence fixes, and this doc is likely to get cited/quoted externally per the PR own SEO goal — worth getting the guarantees exactly right. No test coverage concerns — this is docs-only, nothing executable changed. |
ReviewDocs-only PR — new Issues1. "source IPs are HMAC-SHA256 pseudonymized" overstates the guarantee ( This is stated unconditionally, but in if self._secret_key and source_ip != "mcp-local":
source_ip = hash_pii(source_ip, self._secret_key)
elif source_ip != "mcp-local" and not self._warned_no_secret_key:
...
logger.warning(
"[CerberusMCP] Sending source IP in plaintext — no secret_key configured. "
"Add secret_key to cerberus_config to enable PII hashing."
)Given this doc's whole thesis is "sanitize before transmission, not after storage" and it explicitly calls out pseudonymization as a safety practice, stating the guarantee as unconditional is the kind of claim that gets quoted back at Cerberus later. Suggest: "source IPs are HMAC-SHA256 pseudonymized when 2. "Arguments and headers are sanitized by key name" is inaccurate — cerberus-mcp never has headers (same section)
Minor / non-blocking
Nice work on the substance; the two accuracy points above are worth a quick fix given this doc is explicitly meant to be cited externally. |
Review: docs: add an MCP security guide and point the README at itDocs-only PR (README + new docs/mcp-security.md, ~1,300 words). Overall this is well-written, well-scoped, and consistent with CLAUDE.md's public-repo rules (wire contract described, no backend internals, no roadmap for defeating telemetry). What checked out
One accuracy issue worth fixing
Minor / non-blocking
Test coverage Summary |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4c3032ff5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
All four from Codex on #29, all verified against the source before changing anything. They share a shape: the docs promised a guarantee where the code offers a default. 1. Source-IP pseudonymization is conditional on `secret_key` (server.py:433). Without it the IP is normalized and sent in the clear behind a single log warning. The guide claimed it flatly, and its config example omitted the key, so following the example exactly would export the PII you thought you were hashing. The key is now in the example, in the guide, and in cerberus-mcp's config table, where it was missing entirely. That omission was the root cause. 2. Redaction matches key names against SENSITIVE_KEYS, not values (sanitization.py). A secret under an unremarkable key is transmitted as-is. Both READMEs said "sensitive values are redacted" without qualification. Now stated as what it is: a safety net for the usual suspects. 3. Session and client identity only populate for handlers that declare a FastMCP `Context` parameter (server.py:208). Handlers without one still emit call records, but nothing to group them by, which is what the sequence-level detection depends on. 4. Server-side telemetry has the declared tool inventory and the observed calls, not the user's task. It shows undeclared tools, definitions that changed after approval, and calls outside the declared surface. Comparing against what the user actually asked for needs intent telemetry from the LLM side. Point 2's unconditional phrasing also predates this branch, in the root README's "How they fit together". Fixed there too.
ReviewDocs-only PR (new Verified against source
CLAUDE.md compliance
Content quality
Minor / non-blocking suggestions
RisksNone found. This is additive documentation with no code or behavior changes; the only "risk" is prose accuracy, which checks out against the current implementation. Nothing here touches sanitization logic, Nice work — this is a good example of documentation that discloses real limitations (key-name-only redaction, no secret_key = plaintext IPs, sensor-not-detection) rather than glossing over them, which lines up with the repo's stated bar of not overselling what these packages do. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09feca8aa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| This repository is a monorepo of the **client-side** Cerberus integrations — the libraries and gateway policy you add to your application (or place in front of it) to send request metrics to Cerberus. Each integration targets a different runtime but emits the **same event schema**, so they're interchangeable from the backend's point of view. | ||
| These are the libraries and gateway policies you add to your application, or place in front of it, so that a request, an MCP tool call, or an LLM call becomes something you can see. Each integration targets a different runtime and they all emit the same event schema, so they are interchangeable and can be mixed in one deployment. | ||
|
|
||
| Values under known sensitive key names are redacted in the client, and source IPs are HMAC pseudonymized when a secret key is configured, both before anything is transmitted. |
There was a problem hiding this comment.
The new key-name qualification still fails when a tool schema contains a sensitive default or example under a known key: _report_schema() copies descriptions and input_schema directly into custom_data and queues the event without calling sanitize_arguments() or sanitize_dict() (cerberus-mcp/src/cerberus_mcp/server.py:295-300,355-378). In that scenario the value leaves the client unredacted despite this guarantee, so either sanitize schema-report metadata or explicitly exclude it from the promise.
Useful? React with 👍 / 👎.
| - **Redaction is by key name.** Arguments and headers pass through | ||
| [`cerberus-core`](../cerberus-core/README.md), which redacts values whose |
There was a problem hiding this comment.
For HTTP/SSE MCP handlers, _emit_event() hardcodes headers=None (cerberus-mcp/src/cerberus_mcp/server.py:450), so request headers neither pass through cerberus-core nor reach the emitted telemetry. This wording can make operators expect header visibility and redaction that the integration does not provide; limit the statement to arguments.
Useful? React with 👍 / 👎.
| Instrument the gateway in front of them. You keep the call graph and the | ||
| arguments the model requested. What you give up is confirmation that the |
There was a problem hiding this comment.
For streamed OpenAI traffic, the linked gateway README states that LLM tool-call arguments are not captured, while its MCP spans carry only the tool name, so deploying the gateway does not preserve “the arguments the model requested” in that scenario. Qualify this FAQ answer to the provider/streaming modes that actually expose arguments instead of presenting it as an unconditional property of gateway instrumentation.
Useful? React with 👍 / 👎.
1 participant
First of the GitHub SEO work, after setting the repo description and topics.
The gap
The README answers "which package do I install" and "how do I publish". Both useful, neither a question anyone types into a search box or an assistant. Nothing in this repo answered "how do I secure an MCP server against tool poisoning", which is exactly the query cluster these packages serve and where the 2026-07-31 AI answer baseline showed us absent from all 20 unbranded answers.
That baseline also showed what gets cited for these queries: vendor product and docs pages plus standards bodies (
genai.owasp.org,modelcontextprotocol.io,learn.microsoft.com, arXiv). Zero review sites, zero listicles. Own docs with explicit standards alignment is the shape that wins here.What this adds
docs/mcp-security.md, about 1,300 words:cerberus-mcpdoes, and three plain statements of what it does notThe README intro now leads with what the packages give you rather than describing itself as a monorepo, and links the guide from a new Guides section above Packages.
Notes
Deliberately honest about limits, because that is what got our glossary cited in the first place: the guide says gateway instrumentation trades observed arguments for reach, that the package is a sensor rather than detection, and that read-only tools are the first half of an exfiltration chain rather than a reason to skip recording.
All 26 relative links verified. No em-dashes in new content. Checked against the backend table and column list from the hygiene sweep: nothing internal.