fix(memory): reject peer: key prefix and colon-bearing peer_id at substrate boundary#5161
Conversation
![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: 5b69a2e2e8
ℹ️ 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".
| if pid.contains(':') { | ||
| return Err(KernelOpError::InvalidInput(format!( | ||
| "peer_id '{pid}' must not contain ':' (reserved namespace separator)" | ||
| ))); |
There was a problem hiding this comment.
When the sender context comes from channels that legitimately use colon-bearing IDs, this new check makes all memory_store, memory_recall, and memory_list tool calls fail with InvalidInput. For example, the Matrix adapter puts the room id in ChannelUser.platform_id and build_sender_context falls back to that as SenderContext.user_id; Matrix room/user ids are of the form !room:server / @user:server, and WebUI IPv6 client IPs also contain :. The security boundary still needs an injective encoding/escaping scheme, but rejecting every colon disables shared memory for those supported contexts.
Useful? React with 👍 / 👎.
github-actions
Bot
added
area/docs
…strate boundary Two paired confused-deputy holes in the shared-memory substrate. (#5119) peer_scoped_key rendered peer-scoped rows as peer:{peer_id}:{key} which is non-injective when peer_id contains ':' (Slack T1:U2, IRC user:42) or is empty (peer::{key} collides with None-scope ':{key}'). (#5120) memory_store accepted any LLM-supplied key, letting an agent without peer context write peer:victim:user_name into the shared namespace and have memory_list(Some("victim")) recover it. Both vectors rejected at the KernelHandle::MemoryAccess boundary with KernelOpError::InvalidInput. peer_scoped_key now returns Result and also refuses empty peer_id. memory_list no longer blindly strip_prefixes: each recovered inner key must round-trip through the now-strict peer_scoped_key for that exact peer_id, dropping nested/double-scoped plants and structurally-impossible rows; colon-bearing list queries are rejected before recovery. WASM host_kv_set/host_kv_get mirror the peer: prefix rejection as documented defense-in-depth (the <agent_id>: prefix is what actually neutralizes a clean peer: row from a guest). Internal callers handle the new Result with a WARN-and-skip so prompt build stays best-effort. Residual requiring maintainer sign-off: a pre-fix row written by a None-scope agent at the exact bytes peer:{colon-free-pid}:{non-peer-key} is byte-identical to a legitimate post-fix row and cannot be separated in-code without a writer-attribution column; closing that finite pre-fix-window set needs a one-shot DB scrub (documented in CHANGELOG). Closes #5119 Closes #5120
5b69a2e to
3a5182a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a5182a4b4
ℹ️ 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".
| match peer_scoped_key(inner, Some(pid)) { | ||
| Ok(canonical) if canonical == k => Some(inner.to_string()), |
There was a problem hiding this comment.
When an existing database contains rows written before this fix by a colon-bearing peer ID, this round-trip still accepts the collided key for the shorter peer. For example, a historical peer_id = "victim:sub" / key = "car" row is stored as peer:victim:sub:car; memory_list(Some("victim")) strips peer:victim: and peer_scoped_key("sub:car", Some("victim")) recreates the same bytes, so it exposes sub:car, and memory_recall("sub:car", Some("victim")) reads it as well. This leaves #5119 exploitable on upgraded installs unless a migration/scrub or writer-attribution check removes/blocks those legacy collision rows.
Useful? React with 👍 / 👎.
github-actions
Bot
added
ready-for-review
1 participant
Summary
Closes two paired confused-deputy holes in the shared-memory substrate:
peer_scoped_keyrendered peer-scoped rows aspeer:{peer_id}:{key}. The pair is only injective whenpeer_idcontains no:— a Slack-stylepeer_id = "T1:U2"or IRC-style"user:42"collides with a different(peer_id, key)tuple, letting one peer enumerate or shadow another peer's keys viamemory_list(Some(pid)).memory_store(key, value, peer_id=None)accepted any LLM-suppliedkey. An agent running withoutpeer_idcontext could writekey = "peer:victim:user_name"in the shared namespace; subsequentmemory_list(Some("victim"))recovered the planted row as ifvictimwrote it.Changes
KernelHandle::MemoryAccess::memory_store / memory_recall / memory_listrejectpeer:-prefixedkeyand colon-bearingpeer_idwithKernelOpError::InvalidInput.peer_scoped_keynow returnsResult<String, KernelOpError>and refuses both vectors. Internal callers (messaging.rs,agent_execution.rs) handle the newResultby skipping theuser_namelookup with aWARNon malformedpeer_id, keeping prompt-build best-effort rather than failing the turn.host_kv_set/host_kv_get(crates/librefang-runtime/src/host_functions.rs) mirror thepeer:prefix rejection.Backward compatibility
Pre-fix rows already at
(shared_agent_id, "peer:...")stay readable via direct substrate access by internal kernel code. This change only blocks new writes from the tool layer, so no on-disk migration is required. Operators can audit residual planted rows with a one-shot SQL scan against the shared-memory agent id (00000000-0000-0000-0000-000000000001) filtering onkey LIKE 'peer:%'.Tests
kernel/tests.rs::test_peer_scoped_keyextended with rejection cases for both vectors.crates/librefang-kernel/tests/kernel_handle_contract_memory.rs:test_memory_store_rejects_peer_prefix_keytest_memory_recall_rejects_peer_prefix_keytest_memory_list_rejects_peer_id_with_colontest_memory_store_rejects_peer_id_with_colontest_memory_store_with_clean_peer_id_still_worksVerification note
The implementing agent dispatch terminated before running cargo verification locally; rustfmt clean, pre-commit hook (CHANGELOG attribution + secret scan) passed. CI is the authoritative gate for clippy + scoped tests on this PR.
Closes #5119
Closes #5120