Bound what a memory can hold and what a session is handed, page memory_list, and gate the operator login - #17
Merged
Merged
Conversation
…tor login Five related bounds on the MCP surface and the SessionStart hook. The documented default flow - setup mints a dedicated, context-scoped agent, serve, recall - is unchanged and needs no new setting. A memory is a note, not a document, and what one save stores can be pasted into every later session. So memory_save now refuses a name over 120 characters, a description over 300, a body over 16 KiB, more than 32 links or a link over 120 characters, with invalid_params naming the limit it hit. The check runs before the store is touched, so being told no does not depend on the VTA being reachable. It runs where a record comes in and never on decode: an entry stored before these limits existed, or written by another tool, stays readable and forgettable. Saves being bounded does not bound a render, because a context can hold hundreds of memories. The hook's output is capped at 32 KiB including the preamble and delimiters, with a truncation marker inside the fence saying how many of how many were shown and where the rest is. It lands in the context before the user has typed anything, which is exactly why it needs a ceiling. A person who ran recall at a terminal is not capped. memory_list returns a page: limit (default 50, clamped 1..200) and offset, with total, offset, limit, count and nextOffset in the result. The /memories and /forget commands follow nextOffset now - confirming "forget everything" against one unpaged list would have shown the user 50 memories and then deleted only those. memory_recall, memory_get and memory_list are fenced like the hook's output: preamble plus opening delimiter, the JSON, the closing delimiter, as three content blocks so the middle one still parses on its own. The serialised JSON is sanitised again on the way out, so no field can carry a delimiter shape whichever projection it came from; a delimiter token contains no quote or backslash, and neither does its replacement, so the JSON stays valid. Fields were already sanitised and labelled "trust": "untrusted-data", so this is depth rather than a fix. Tool annotations say what each tool does to the world, so a client can ask the user before the irreversible ones: destructiveHint on memory_forget and on memory_save (the same name replaces an existing memory), readOnlyHint on recall, get, list and context. A rate limit was considered and rejected: the caller is the user's own local model over stdio, so a manipulated model can call within any limit. Client-side confirmation is the control that helps. Finally, setup --use-session. It stores no key, but it authenticates as the operator's own pnm login, so the memory service inherits everything that login can reach - and the server and hook run unattended in every session, which is the wrong place for that reach to be implicit in a file written once. serve, the hook and recall/list/forget/doctor refuse such a config unless VTA_AGENT_MEMORY_ALLOW_OPERATOR_LOGIN=1 is set in the environment they run in. The refusal is shaped by who reads it: the hook stays silent and exits 0, because it must never fail a session; a person gets an error naming the env var and setup --force; memory_context still answers, because diagnostics have to work when the rest does not; and setup warns at the moment it writes such a config. Tests, behavioural per CLAUDE.md: each oversize field refused against a server with no config at all (so invalid_params proves the check precedes the store) and accepted at exactly every limit; 300 memories through the loopback transport paged by limit and offset, including the clamps, the last page and an offset past the end; get and recall asserted to have one matching open/close pair with a body and a link carrying this crate's own delimiter shape defanged but still readable; annotations asserted through the router and over stdio from the real binary; the 32 KiB cap with 100 16 KiB bodies, a render that fits, a single oversized legacy body cut on a char boundary, and the uncapped terminal path; and an operatorLogin config that yields no store, names the env var, and leaves the hook exiting 0 with empty stdout. Plugin and crate versions bumped in step: tool schemas, descriptions, the skill and the commands are plugin-visible, and claude plugin update compares that version rather than the commit. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
stormer78
force-pushed
the
sec-4045/mcp-hardening
branch
from
September 12, 2026 06:01
82ca362 to
8ae09d6
Compare
Contributor
Author
|
Rebased onto After the rebase: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five related pieces of hardening on the MCP surface and the
SessionStarthook.None of them change the documented default flow:
setup(dedicated,context-scoped agent) →
serve→ recall.Size limits on
memory_savename120 characters,description300,body16 KiB, at most 32linksof120 characters each. Over any of those the save is refused with
invalid_params, naming the limit it hit, before the store is touched — sobeing told no does not depend on the VTA being reachable.
The limits are checked where a record comes in, never on decode. A memory stored
before they existed, or by another tool, stays readable and forgettable.
A cap on what the hook injects
Session-start context is capped at 32 KiB including the preamble and delimiters,
with a truncation marker inside the fence saying how many of how many
memories were shown and where to get the rest. Saves are bounded now, but a
context can hold hundreds of memories, so the rendered total needs its own
bound. A person who ran
recallat a terminal is not capped.memory_listreturns a pagelimit(default 50, clamped to 1..200) andoffset, and the result carriestotal,offset,limit,countandnextOffset./memoriesand/forgetnow tell the model to follow
nextOffset: confirming "forget everything" againsta single unpaged list would have shown the user 50 memories and deleted only
those.
Read results are fenced, like the hook's output
memory_recall,memory_getandmemory_listreturn three content blocks —preamble + opening delimiter, the JSON, the closing delimiter — reusing the same
nonce-carrying fence the CLI hook path already used. Separate blocks keep the
JSON machine-parseable. The serialised JSON is sanitised again on the way out, so
no field can carry a delimiter shape whichever projection it came from; a
delimiter token contains no
"or\, and neither does its replacement, so theJSON stays valid.
Fields were already sanitised and labelled
"trust": "untrusted-data", so thisis defence in depth rather than a fix for an open hole.
Tool annotations clients can gate on
destructiveHintonmemory_forget(permanent, no undo) and onmemory_save(the same name replaces an existing memory),
readOnlyHintonmemory_recall,memory_get,memory_listandmemory_context. Asserted both through the toolrouter and on the wire, spelled the way the protocol spells them.
setup --use-sessionnow needs an explicit opt-in to be usedThat flag stores no key but authenticates as the operator's own
pnmlogin, sothe memory service inherits everything that login can reach. The MCP server and
the
SessionStarthook run unattended in every session, which is the wrong placefor that reach to be implicit in a config file written once.
So
serve, the hook, andrecall/list/forget/doctorrefuse such a configunless
VTA_AGENT_MEMORY_ALLOW_OPERATOR_LOGIN=1is set in the environment theyrun in. The refusal is shaped by who is reading it:
setup --force;memory_contextstill answers, because diagnostics must work when the restdoes not;
setup --use-sessionwarns, at the moment it writes the config, that this iswhat will happen.
The default dedicated-agent path needs no setting and is unchanged.
Tests
with
invalid_paramsagainst a server with no config, proving the check runsbefore the store; exactly at every limit the save proceeds and fails only for
want of a configured VTA.
total: 300andnextOffset: 50,offsetwalks the pages,limitclamps at200 and at 1, the last page reports no
nextOffset, and an offset past the endreturns an empty page with the real
total.memory_getandmemory_recall: exactly one matching open/close pair, thepreamble first, the JSON block parsing on its own, and a body and link carrying
this crate's own delimiter shape neutralised while the injected prose stays
readable.
the fence; a render that fits is untouched; a single oversized legacy body is
cut on a char boundary rather than dropped; the terminal path is uncapped.
operatorLogin: trueconfig produces no store and names the env var, andthe real binary's hook path exits 0 with empty stdout while
recallfailsloudly.
Versions
Plugin and crate versions bumped in step (0.2.0 → 0.3.0): tool schemas,
descriptions, the skill and the slash commands are all plugin-visible, and
claude plugin updatecompares that version rather than the commit.Not in this PR
Rate limiting on
memory_save/memory_forgetis deliberately absent. The calleris the user's own local model over stdio, so a limit does not address the stated
threat — a manipulated model can call within any limit. The annotations above are
the control that actually helps, because they let the client ask the user.