fix(memory): stop add() metadata from setting a memory's identity scope (#6656)
This commit is contained in:
+25
-6
@@ -135,18 +135,30 @@ _SENSITIVE_SUFFIXES = (
|
||||
ENTITY_PARAMS = frozenset({"user_id", "agent_id", "run_id"})
|
||||
DELETE_ALL_BATCH_SIZE = 1000
|
||||
|
||||
# Tenant-scoping fields that update() must never let caller-supplied metadata overwrite (issues #4490, #6277).
|
||||
# Tenant-scoping fields that caller-supplied metadata must never set, on either the
|
||||
# creation or the update path (issues #4490, #6277, #6655).
|
||||
_IDENTITY_KEYS = ENTITY_PARAMS | {"actor_id"}
|
||||
|
||||
|
||||
def _strip_identity_keys(metadata: Dict[str, Any], existing_payload: Dict[str, Any]) -> Dict[str, Any]:
|
||||
"""Drop identity keys from caller metadata; they are immutable after creation (issues #4490, #6277)."""
|
||||
def _strip_identity_keys(
|
||||
metadata: Dict[str, Any],
|
||||
existing_payload: Dict[str, Any],
|
||||
*,
|
||||
context: str = "update()",
|
||||
) -> Dict[str, Any]:
|
||||
"""Drop identity keys from caller metadata; scope is set by the entity params, not metadata.
|
||||
|
||||
On the update path `existing_payload` carries the memory's current scope, so
|
||||
re-sending an identical value is silently accepted; only a changed value warns.
|
||||
On the creation path there is no prior payload, so pass an empty dict and every
|
||||
identity key present in `metadata` is dropped with a warning.
|
||||
"""
|
||||
clean = {}
|
||||
for key, value in metadata.items():
|
||||
if key not in _IDENTITY_KEYS:
|
||||
clean[key] = value
|
||||
elif value != existing_payload.get(key):
|
||||
logger.warning(f"update(): ignoring metadata['{key}'] - identity fields are immutable after creation")
|
||||
logger.warning(f"{context}: ignoring metadata['{key}'] - identity fields cannot be set through metadata")
|
||||
return clean
|
||||
|
||||
|
||||
@@ -315,7 +327,9 @@ def _build_filters_and_metadata(
|
||||
for flexible session scoping and optionally narrows queries to a specific `actor_id`. It returns two dicts:
|
||||
|
||||
1. `base_metadata_template`: Used as a template for metadata when storing new memories.
|
||||
It includes all provided session identifier(s) and any `input_metadata`.
|
||||
It includes all provided session identifier(s) and any `input_metadata`. Identity
|
||||
scope is set from the entity params only; identity keys in `input_metadata` are
|
||||
dropped, so freeform metadata cannot place a memory into an unrequested scope.
|
||||
2. `effective_query_filters`: Used for querying existing memories. It includes all
|
||||
provided session identifier(s), any `input_filters`, and a resolved actor
|
||||
identifier for targeted filtering if specified by any actor-related inputs.
|
||||
@@ -343,7 +357,12 @@ def _build_filters_and_metadata(
|
||||
scoped to the provided session(s) and potentially a resolved actor.
|
||||
"""
|
||||
|
||||
base_metadata_template = deepcopy(input_metadata) if input_metadata else {}
|
||||
# Identity scope is set below from the entity params only. Stripping the keys here
|
||||
# stops caller metadata from placing a memory into a scope the caller did not pass,
|
||||
# which the re-pins below cannot prevent for a param that was left unset (issue #6655).
|
||||
base_metadata_template = (
|
||||
_strip_identity_keys(deepcopy(input_metadata), {}, context="add()") if input_metadata else {}
|
||||
)
|
||||
effective_query_filters = deepcopy(input_filters) if input_filters else {}
|
||||
|
||||
# ---------- validate and add all provided session ids ----------
|
||||
|
||||
@@ -453,6 +453,95 @@ def test_update_memory_metadata_cannot_change_identity_fields(mocker, caplog):
|
||||
assert "ignoring metadata['user_id']" in caplog.text
|
||||
|
||||
|
||||
_ATTACKER_ADD_METADATA = {
|
||||
"agent_id": "victim-agent",
|
||||
"run_id": "victim-run",
|
||||
"actor_id": "victim-actor",
|
||||
"category": "sports",
|
||||
}
|
||||
|
||||
|
||||
def _captured_add_metadata(memory, mocker, **add_kwargs):
|
||||
"""Run add() with the pipeline stubbed and return the metadata template it produced."""
|
||||
captured = {}
|
||||
|
||||
def _capture(messages, metadata, filters, infer, **kwargs):
|
||||
captured.update(metadata)
|
||||
return []
|
||||
|
||||
mocker.patch.object(memory, "_add_to_vector_store", side_effect=_capture)
|
||||
memory.add("I like coffee", infer=False, **add_kwargs)
|
||||
return captured
|
||||
|
||||
|
||||
def test_add_metadata_cannot_set_identity_fields(mocker, caplog):
|
||||
"""Regression (issue #6655): add() metadata must not inject identity scope.
|
||||
|
||||
The caller scopes by user_id only, so the agent_id/run_id re-pins in
|
||||
_build_filters_and_metadata never fire and cannot defend the payload.
|
||||
"""
|
||||
memory = _build_memory_instance(mocker, Memory)
|
||||
|
||||
with caplog.at_level(logging.WARNING, logger="mem0.memory.main"):
|
||||
metadata = _captured_add_metadata(
|
||||
memory, mocker, user_id="attacker", metadata=dict(_ATTACKER_ADD_METADATA)
|
||||
)
|
||||
|
||||
assert metadata["user_id"] == "attacker"
|
||||
for key in ("agent_id", "run_id", "actor_id"):
|
||||
assert key not in metadata, f"{key} was injected through add() metadata"
|
||||
# Non-identity metadata is untouched.
|
||||
assert metadata["category"] == "sports"
|
||||
assert "ignoring metadata['agent_id']" in caplog.text
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_async_add_metadata_cannot_set_identity_fields(mocker):
|
||||
"""Async counterpart of test_add_metadata_cannot_set_identity_fields."""
|
||||
memory = _build_memory_instance(mocker, AsyncMemory)
|
||||
captured = {}
|
||||
|
||||
async def _capture(messages, metadata, filters, infer, **kwargs):
|
||||
captured.update(metadata)
|
||||
return []
|
||||
|
||||
mocker.patch.object(memory, "_add_to_vector_store", side_effect=_capture)
|
||||
await memory.add(
|
||||
"I like coffee",
|
||||
user_id="attacker",
|
||||
metadata=dict(_ATTACKER_ADD_METADATA),
|
||||
infer=False,
|
||||
)
|
||||
|
||||
assert captured["user_id"] == "attacker"
|
||||
for key in ("agent_id", "run_id", "actor_id"):
|
||||
assert key not in captured, f"{key} was injected through async add() metadata"
|
||||
assert captured["category"] == "sports"
|
||||
|
||||
|
||||
def test_add_entity_params_still_set_scope(mocker):
|
||||
"""The documented top-level params remain the only way to set scope."""
|
||||
memory = _build_memory_instance(mocker, Memory)
|
||||
|
||||
metadata = _captured_add_metadata(
|
||||
memory, mocker, user_id="u1", agent_id="a1", run_id="r1", metadata={"category": "sports"}
|
||||
)
|
||||
|
||||
assert metadata["user_id"] == "u1"
|
||||
assert metadata["agent_id"] == "a1"
|
||||
assert metadata["run_id"] == "r1"
|
||||
assert metadata["category"] == "sports"
|
||||
|
||||
|
||||
def test_add_without_metadata_is_unaffected(mocker):
|
||||
"""No metadata argument means no stripping and no behaviour change."""
|
||||
memory = _build_memory_instance(mocker, Memory)
|
||||
|
||||
metadata = _captured_add_metadata(memory, mocker, user_id="u1")
|
||||
|
||||
assert metadata == {"user_id": "u1"}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_async_update_memory_metadata_cannot_change_identity_fields(mocker):
|
||||
"""Async counterpart of test_update_memory_metadata_cannot_change_identity_fields."""
|
||||
|
||||
Reference in New Issue
Block a user