Repository navigation
test(saas): memory round-trip isolation across profiles - #7271
Conversation
Introduce a mock memory backend under the saas_mode test suite so tests can exercise memory-dependent behaviour without a real store. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a mock implementation of the saas backend so tests can exercise subscription and billing flows without hitting a real service. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Wire the memory and mock_memory helper modules into the saas mode end-to-end test binary so the new memory-related scenarios can be exercised. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add an opt-in debug dump of the captured memory requests, gated on the DUMP_MEMORY_WIRE environment variable, so the wire traffic can be inspected when the profile isolation assertion fails. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
The memory isolation test now records the request range of the forget-by-id call and permits read-only GETs in that range to name scopes outside the caller's root, since TinyMemory's id-based forget sweeps the whole tree by contract. The wire assertions were also tightened to report the offending request index, and the debug dump behind DUMP_MEMORY_WIRE was dropped. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Memory access is no longer restricted to the resolved scope's subtree, and confinement is now disabled regardless of mode. This restores the intended unconfined behaviour while the scope resolution call is kept for its side effects. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
allowed_reach now derives its subtree from the current scope's root instead of the global namespace root, and the always-true condition in confinement_in was removed so the saas check actually gates confinement. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Replace the manual descending comparator with sort_by_key using Reverse so the intent of ordering by score is clearer. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Tiny Sweeper reviewTiny Sweeper completed its review; deterministic results follow. State: Reviewing pending checks Review snapshot
Completeness: Complete What changedThis revision adds an end-to-end SaaS memory isolation harness. tests/saas_mode/memory.rs introduces `Stack::start` (spawns the real openhuman-core binary with `BACKEND_URL` pointed at the mock, polls `/health`), `provision` (per-user profile plus session credential via `openhuman.profiles_set_credential`), `call`/`ok` JSON-RPC helpers, and `holds_only_own` (asserts no other user's canary secret appears in a response). The test `profiles_round_trip_their_own_memory_and_never_anothers` uses users `ann`, `anna`, `bob` (deliberately, `ann` is a string prefix of `anna`) with per-user canary secrets; each profile learns, lists, fetches, recalls, and explores its own canary while responses must not contain others' secrets; an attacker profile aims reach filters, foreign paths, foreign ids, and planted namespaces at victims and must gain nothing. A wire-level check asserts every request to the mock carried one profile's credential and named only scopes inside that profile's root, with a documented read-only exception for the forget-by-id sweep (`GET` /memory/scopes and /memory/events under `app:tinymemory` during the attacker's own forget). tests/saas_mode/mock_memory.rs implements the mock backend: a single shared store for every bearer (worst case for confinement), request logging with a one-way `credential_id` hash of the bearer, scope extraction from body/query, idempotency-key and fingerprint replay with conflict handling, routes for experience/events/recall/forget/scopes/answer, and pack invalidation on forget. tests/saas_mode_e2e.rs registers the two new modules. scripts/ci/check-gated-test-allowlist.sh adds `profiles/surface_tests.rs` to the allowlist. Features
Tests
Findings
Resolved this pass
Pending checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS) Before merge
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9569c3a5ba
ℹ️ 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".
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0195 · 276,177 in / 17,335 out · 31,925 cached (12%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0113 · 154,429 in / 11,962 out · 23,384 cached (15%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0079 · 89,715 in / 4,973 out · 7,133 cached (8%) · gpt-5.6-luna
description: $0.0001 · 16,780 in / 186 out · 1,408 cached (8%) · glm-5.3-flash
…emory.rs Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d366f17a09
ℹ️ 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".
| for (method, params) in [ | ||
| ("openhuman.memory_items_list", json!({ "filter": filter })), |
There was a problem hiding this comment.
Add cross-profile items_get probes
If confine::items_get loses its reach check, this suite will still pass: openhuman.memory_items_get is a separate public ID-based read (crates/openhuman-core/src/memory/confine.rs:175-184), while this attack matrix tests list/fetch/recall/explore and only gives learned[victim] to memory_forget, whose independent ops::within_reach precheck can remain correct. The victim ID and mock /memory/events/:id route already exist, so probe items_get directly with the hostile reaches and with no reach, and assert that the victim secret is absent.
AGENTS.md reference: AGENTS.md:L368-L374
Useful? React with 👍 / 👎.
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Main fails static:gated-test-allowlist since the file landed. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
The previously-blocking findings are resolved. Clearing the changes request.
$0.0262 · 367,460 in / 21,987 out · 26,484 cached (7%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0120 · 139,028 in / 10,443 out · 12,677 cached (9%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0132 · 156,523 in / 6,679 out · 10,479 cached (7%) · gpt-5.6-luna
tests: $0.0002 · 17,067 in / 2,130 out · 1,536 cached (9%) · glm-5.3-flash
description: $0.0001 · 17,759 in / 410 out · 64 cached (0%) · glm-5.3-flash
e2e: $0.0002 · 20,505 in / 593 out · 1,728 cached (8%) · glm-5.3-flash
| } | ||
| let cascade = body["cascade"].as_str().unwrap_or("derived_only"); | ||
| match cascade { | ||
| "derived_only" => { |
There was a problem hiding this comment.
Delete derived packs during derived-only forgets
After recall creates a pack, a forget request such as { "scope": "user:1", "confirm_all": true, "cascade": "derived_only" } returns without removing any packs. The same pack can then be passed to answer and still serve the forgotten context. derived_only should preserve source events while removing derived artifacts, including packs associated with the forgotten scope or selected event IDs.
[RULE] stale-derived-data ·
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60e5886102
ℹ️ 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".
| for step in [foreign.clone(), String::new()] { | ||
| let body = s.call( | ||
| attacker, | ||
| "openhuman.memory_items_list", |
There was a problem hiding this comment.
Probe foreign paths through memory_explore
If a regression affects only namespace-path handling in confine::explore, this E2E still passes: the hostile memory_explore calls above exercise only filter.reach, while this foreign-path loop invokes memory_items_list even though the two RPCs have separate confinement wrappers. Add an openhuman.memory_explore call with the same foreign path (and a facet), otherwise a path-based escape from the per-user root is not covered.
AGENTS.md reference: AGENTS.md:L368-L373
Useful? React with 👍 / 👎.
| ); | ||
| let sweep = sweep_start..s.mock.requests().len(); | ||
| assert_eq!(forgotten["forgotten"], json!(1), "{forgotten}"); | ||
| for (user, _) in profiles.iter().skip(1) { |
There was a problem hiding this comment.
Verify the owner's canary is gone after forget
If hosted forget returns forgotten: 1 while the item remains readable (for example because event or cache invalidation is wrong), this test still passes: after the assertion the only listings iterate profiles.iter().skip(1), so the attacker whose item was supposedly deleted is never queried again. Read the attacker's list after this call and assert its canary is absent to make the advertised forget round trip meaningful.
Useful? React with 👍 / 👎.
What
A memory round-trip isolation e2e for SaaS profiles. It runs through the real
openhuman-core run --mode saasbinary against a backend that actually serves/memory/*.tests/saas_mode/mock_memory.rsis a standalone, stateful Rust mock of the hosted memory routes (experience,events,recall,answer,forget,scopes). It is modelled onscripts/mock-api/routes/memory.mjs. It records every request's bearer and the scopes it names. Anything outside/memory/*gets an immediate 500. It does not depend on test(saas): adversarial cross-profile isolation e2e (fuzzed user surface, eviction storm, policy probes) #7251.tests/saas_mode/memory.rsaddsmemory::profiles_round_trip_their_own_memory_and_never_anothers, wired intotests/saas_mode_e2e.rs.Why
The existing SaaS e2e tests only see
/memory/*answer 500, so nothing showed that a profile's memory actually round-trips or stays inside itsuser:<id>root. Profiles run the hostedtinyhumansengine on the legacy layout, confined touser:<id>(profiles::layout,memory::user_scope,memory::confine).The mock deliberately keeps one store for every bearer. The real backend keeps each credential apart, and that would hide a core that reads or writes outside the root. Here the core's own confinement is the only thing keeping profiles apart. That is the shared-engine case
memory::user_scopedefends.What the test asserts
The test uses three profiles:
ann,annaandbob.annis a string prefix ofanna, and scope listing is a prefix match.Each profile stores a canary through
memory_learn, and the canary lands only underapp:tinymemory/user:<id>.Each profile reads its own canary back via
items_list,fetchandrecall. Noitems_list,fetch,recallorexploreresult (facets namespace, kind and agent) ever contains another profile's canary or names another root.An attacker profile tries to escape its root:
filter.reachat the victim's root, at a node below it, with and without ancestors, and at the global root. It sends this toitems_list,fetch,recallandexplore.None of this returns, plants into or forgets the victim's memory. The victim still holds its canary.
A profile forgets its own item, and the others keep theirs.
On the wire, every memory request carries the credential of a provisioned profile, and every scope it names is inside that profile's root. The one exception is below.
Mutation check: with
user_scope::confinementandconfine::allowed_reachdisabled, the test fails at once (items_list as ann leaked anna's memory). The hook checkpointed that mutation and its revert asf21dff4365/fea7255ea8under misleadingfix(memory)subjects. Their net diff is zero, and this PR changes no crate source.Finding: forget-by-id sweeps the whole tree (read-only, no leak observed)
memory_forgetby id first narrows the ids throughgetwithin the caller's reach (memory::ops::forget), which is correct. It then callsengine.forget(ForgetTarget::Ids). TinyMemory's contract treats that as unscoped ("ids are not scoped by namespace"), so the cortex engine'severy_scope()lists fromapp:tinymemoryand reads every other profile's scopes with the caller's credential.Why this is not a leak:
It is still a confinement gap on a shared engine: cross-tree reads, and cost that grows with the number of users. The fix belongs in tinymemory, for example a reach on
ForgetTarget::Ids. The test allows only that sweep's GET listings outside the root, and only inside the forget call's request window. Its forget, and every other write, must stay inside the root.Tests
cargo test -p openhuman-cli --test saas_mode_e2e: 15 passed, including the new test.cargo check --all-targets: OK.cargo fmt --all --check,pnpm rust:layout,node scripts/ci/check-feature-forwarding.mjsandpnpm saas:ambient: OK. The vendor submodules are clean.cargo clippy -p openhuman-cli --test saas_mode_e2e -- -D warningsis clean for the new code. Two clippy errors already in the core lib stop the full clippy command:needless_returninagent/orchestration/background_delivery.rs:203andtype_complexityinchannels/providers/relay/store.rs:42. Neither file is touched here.cargo check --manifest-path crates/openhuman-app/Cargo.toml, because this PR changes onlytests/.Limits: the mock follows the hosted wire as
memory.mjsmodels it, not the real backend. Recall ranking and answers are deterministic stand-ins.Summary by CodeRabbit