fix(persistence): make the S3 tenant→prefix derivation injective and prefix-disjoint (#447)

[mauripunzueta]

Closes #447.

What was wrong

S3Keyspace::with_tenant_prefix derived a tenant's data-key prefix by trimming leading and trailing /. That mapping is many-to-one, while the tenant registry key has been injective since #271 — so the registry said N tenants and the keyspace said 1.

The issue named one defect. There are two, in the same six lines.

D1 — slash padding collapses (the issue's subject). acme, /acme, acme/, //acme → prefix acme.

D2 — a tenant can nest inside another's sweep radius (not previously named). RESERVED_TENANT_IDS is compared against the whole id (admin_tenants.rs:123), so acme/resources provisions cleanly. It then stores everything under acme/resources/… — which is exactly acme's resources_prefix(). That prefix is what purge_tenant_data sweeps and what list_current_keys enumerates (it selects on /current.json anywhere beneath). So tenant acme both deletes and reads tenant acme/resources's data. Reachable through the admin API alone, with no IdP involvement — arguably more reachable than D1.

Injectivity alone does not fix D2, and single-segment prefixes alone do not fix D1. Both properties are needed.

Consequence

purge_tenant_data resolves through the same derivation, so on PrefixPerTenant (the default, and the only mode crates/hfs/src/main.rs constructs):

DELETE /admin/tenants/%2Facme?purge=true

deleted tenant acme's resources and its version history, while deregister_tenant("/acme") removed only tenants/%2Facme.json. End state: a tenant still registered, still listed, with all of its data gone — reported to the caller as success.

The fix

The derivation now escapes exactly the ids that are not already safe, reusing the registry_object_id helper #271 introduced, and is the identity for every id that is.

tenant id	before	after
acme	acme	acme — unchanged
acme/research	acme/research	acme/research — unchanged
__system__	__system__	__system__ — unchanged
/acme	acme ⚠ shared	%2Facme
acme/	acme ⚠ shared	acme%2F
//acme	acme ⚠ shared	%2F%2Facme
acme/resources	acme/resources ⚠ inside acme's sweep	acme%2Fresources

An id is keyspace-safe iff it contains no %, \, or space; has no empty segment; and has no segment after the first equal to a sweep root (resources, history, bulk). Safe → verbatim. Unsafe → one opaque escaped segment.

Why the guarantees hold. Every structural defect implies the id contains /, and registry_object_id escapes /, so an escaped prefix always contains % — which no safe prefix may. The two ranges are disjoint and each mapping is injective on its own domain, giving injectivity. For prefix-disjointness, an escaped prefix contains no / at all and so cannot end in /{sweep-root}, while a safe prefix is barred from a non-first sweep-root segment by construction.

tenants and _system.user-settings are deliberately not sweep roots: they live on the base keyspace, never under a tenant prefix, and are already protected structurally by the direct-child filter in list_tenants and the digest leaf in user_settings_key (#271). Listing them would relocate a top-level tenant named tenants for nothing.

Why not simply escape the whole id

Option A in the issue was an unconditional escape. That was rejected, and the reason governs the whole design:

S3 is the system of record, not a derived index. There is no $reindex that can rebuild an object at a new key. Changing a computed prefix does not move objects — it makes the existing ones unreachable, and a later purge_tenant_data will not reach them either. An unconditional escape would relocate every hierarchical id (a/b → a%2Fb), turning a bug that needs a slash-padded id nobody has into silent data loss for anyone using a documented id shape.

A compatibility read path (the legacy_tenant_registry_key trick from #271) does not transfer either: a registry record is one leaf key you can try twice; a tenant prefix is the root of an unbounded listing. Dual-prefix reads would double every LIST, leave writes ambiguous, and give purge two radii.

So the escape is conditional, and the condition is exactly "is this id currently unsafe". Every id whose prefix moves is one whose objects are already commingled with another tenant's or already inside another tenant's purge radius — there is no coherent single-tenant dataset at the old location, so relocating it is the remediation rather than a migration cost. No object belonging to a currently well-formed tenant moves.

Upgrade impact

None for any deployment whose tenant ids come from the header or URL-path routes — both reject / outright ([A-Za-z0-9_-]), so every id they can produce is keyspace-safe and keeps its exact prefix.

A deployment that provisioned a slash-padded or sweep-root-nested id through the admin API will find that tenant's prefix has moved. Its objects at the old location were never separable from the tenant it was colliding with; they stay where they are, now owned unambiguously by that tenant.

Relationship to #385 / PR #450

Complementary, and this is not merely defence in depth.

#450 is Option B done properly — one canonical TenantId::parse rejecting leading/trailing/repeated /, enforced at every ingress. It deliberately guards only the mint point (register_tenant), leaving deregister_tenant, purge_tenant_data, and every read path unguarded so operators can still clean up a non-canonical tenant that predates the validator.

purge_tenant_data builds its TenantId directly from an operator-supplied path segment. No validator is in that path. So consequence 1 above survives #450 for any id already in a registry — this PR closes it.

#450's own contract says as much: "the contract bounds the input, it does not make a lossy mapping safe. A backend with a narrower keyspace still owes an injective encoding of its own." This is that encoding.

Branches from main; touches no ingress validator and no elasticsearch/ file, so it rebases cleanly against both #450 and #446 in any merge order.

Databases — all backends addressed

The defect is a derivation. Two backends derive a storage location from the tenant id; three use it as an opaque exact-match value.

backend	tenant scoping	injective?	change
S3	derives a key prefix	was not	fixed here
Elasticsearch	derives an index name (to_lowercase)	was not	#384 — fixed in PR #446, untouched here
SQLite	tenant_id TEXT in the composite PK, bound and compared with = (BINARY collation)	identity	none needed — now asserted
PostgreSQL	tenant_id TEXT in the composite PK, bound and compared with =	identity	none needed — now asserted
MongoDB	"tenant_id": <id> exact BSON match, default case-sensitive collation	identity	none needed — now asserted
Composite	delegates to primary + secondaries	inherits	none needed

For the three "identity" rows that was a code reading — and a code reading is exactly what let this defect class sit undiscovered in both rows that derive. So it is now checked: tests/multitenancy/tenant_id_fidelity_suite.rs drives the id shapes S3 collapsed through each backend's real storage path and asserts nothing commingles, #[path]-shared into the SQLite, PostgreSQL and MongoDB test binaries (the arrangement transactions/if_match_suite.rs established).

Tests

Properties, over an adversarial id corpus (s3/keyspace.rs) — a new id shape gets added to the corpus rather than given its own test:

• tenant_prefix_derivation_is_injective — pairwise, with and without a global prefix
• no_tenant_sweep_prefix_covers_another_tenants_keys — pairwise, every sweep prefix against every sample key
• safe_tenant_ids_keep_their_existing_prefix / unsafe_tenant_ids_move_to_one_escaped_segment
• an_escaped_prefix_cannot_be_forged_by_a_literal — %2Facme as a literal must not land where /acme lands
• the_empty_tenant_id_does_not_become_the_base_keyspace
• keyspace_safety_predicate_clauses

Consequences, end to end through the mock S3 client (s3/tests.rs) — what an operator actually experiences:

• purging_a_slash_padded_tenant_leaves_the_bare_tenant_intact — current resource and version history
• slash_padded_tenants_do_not_share_resources
• a_tenant_named_after_a_sweep_root_survives_its_parents_purge — D2, covering read and delete
• safe_tenant_ids_still_resolve_to_their_original_prefix — asserts the literal object keys, so a silent relocation fails the build

All 8 of the behavioural tests fail against the previous derivation — verified by temporarily restoring trim_matches('/') and re-running (8 failed / 83 passed), then restoring the fix (91 passed).

Verification

• cargo test -p helios-persistence --features s3,sqlite --lib — 849 passed
• multitenancy_suite, s3_tests, crud_suite, versioning_suite — 150 passed
• rustfmt clean on every touched file; cargo clippy --all-targets introduces no new warning (the 4 in s3/tests.rs are pre-existing — confirmed by running clippy against the unmodified file)
• Not run locally: the PostgreSQL and MongoDB arms. Both test binaries compile, but neither can execute here — no Docker in this environment. They need CI.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!