S3 PrefixPerTenant: with_tenant_prefix trims slashes, so slash-padded tenant ids share one data prefix (registry says N tenants, keyspace says 1)

[mauripunzueta]

Summary

S3Keyspace::with_tenant_prefix derives a tenant's data-key prefix by trimming
leading and trailing / from the tenant id. That derivation is not injective:
tenants a, /a, a/, and //a all resolve to the data prefix a. Meanwhile
the tenant registry key is injective (percent-encoded, since #271), so those
same ids are four distinct, separately-registrable tenants.

The result is an asymmetry: the registry says four tenants, the data keyspace says
one.

Found while fixing #384 (the Elasticsearch equivalent) and deliberately kept
separate — the two have very different remediation stories, see "Why this was not
folded into #384".

Root cause

crates/persistence/src/backends/s3/keyspace.rs:36-43:

pub fn with_tenant_prefix(&self, tenant_id: &str) -> Self {
    let tenant = tenant_id.trim_matches('/');
    let merged = match &self.base_prefix {
        Some(base) => format!("{}/{}", base, tenant),
        None => tenant.to_string(),
    };
    Self::new(Some(merged))
}

trim_matches('/') strips all leading and trailing slashes, so the mapping is
many-to-one.

This is the same defect class #271 fixed for the registry key. That fix introduced
the injective registry_object_id (keyspace.rs:383-395) and its doc comment is
explicit that a lossy derivation must never establish identity — but it changed
only the registry path. The data path kept the trim.

Tenant id	Data prefix (with_tenant_prefix)	Registry key (registry_object_id)
a	a	tenants/a.json
/a	a	tenants/%2Fa.json
a/	a	tenants/a%2F.json
//a	a	tenants/%2F%2Fa.json
a/b	a/b	tenants/a%2Fb.json

Note a/b is not affected — only leading/trailing slashes are trimmed, so
hierarchical ids get their own prefix. The defect is specific to slash padding.

Scope

• Only the PrefixPerTenant tenancy mode. BucketPerTenant resolves through
  an exact map lookup with no trimming (backend.rs:203-222), and fix(persistence): correct tenancy capability advertisements (#369) #379 added
  duplicate-bucket validation there.
• PrefixPerTenant is the default (config.rs:71-87) and the only mode
  crates/hfs/src/main.rs ever constructs (:462, :2305, :2464), so this is
  the mode every S3 deployment actually runs.
• Every S3 data path goes through it: tenant_location (backend.rs:194-202) is
  the single funnel for resource keys, history, and purge_tenant_data.

Reachability — narrower than #384, and worth being precise about

A slash-padded tenant id has to reach storage. Of the four routing surfaces:

• X-Tenant-ID header — rejects /. is_valid_tenant_id
  (crates/rest/src/tenant/resolver.rs:283-290) allows only
  [A-Za-z0-9_-]. Not reachable.
• URL path prefix — same charset check
  (crates/rest/src/middleware/tenant_prefix.rs:62-68). Not reachable.
• Admin provisioning — validate_tenant_id
  (crates/rest/src/handlers/admin_tenants.rs:128-139) explicitly permits /
  with no position constraint, so /a is accepted. Reachable.
• JWT tenant claim — JwtTenantExtractor (resolver.rs:149-156) passes the
  claim into TenantId::new with no validation of any kind, and
  extractors/tenant.rs:163-185 treats it as authoritative. Reachable, if the
  IdP emits such a value.

So an ordinary authenticated user on a header- or URL-routed deployment cannot
reach this. It needs either an operator provisioning a slash-padded id, or an IdP
whose tenant claim carries one.

Consequences, most concrete first

1. DELETE /admin/tenants/%2Fa?purge=true destroys tenant a's data.

purge_tenant_data (crates/persistence/src/backends/s3/storage.rs:1119-1140)
resolves the location "exactly as request handling does" — via tenant_location,
hence via the trim — and then deletes every object under resources_prefix() and
history_root_prefix(). For id /a that is tenant a's resources and its
version history.

Meanwhile deregister_tenant("/a") removes only tenants/%2Fa.json, so tenant
a's registry record survives. The end state is a tenant that is still registered,
still listed, and has had all of its data deleted. The handler reports success
(admin_tenants.rs:301-306).

This needs only the admin API and no JWT involvement.

2. Provisioning /a writes into tenant a's keyspace.

POST /admin/tenants {"id": "/a"} passes validation, finds no existing registry
record (distinct key), registers, and then calls seed_new_tenant
(admin_tenants.rs:238-240), which seeds conformance resources through the normal
write path — i.e. into prefix a. The seeded content is spec SearchParameters and
CompartmentDefinitions under fixed ids, so in practice it overwrites tenant a's
copies with identical documents. Low harm in itself; it is listed because it is
the moment the two tenants silently become one, and it happens without anyone
touching data deliberately.

3. If both ids are routable, full cross-tenant read/write/delete.

Where the JWT path supplies the tenant, a principal on /a reads, updates, and
deletes tenant a's resources through ordinary FHIR requests. This is the
isolation break proper, and it is the part gated on IdP behaviour.

Is this a security issue?

Yes, but a narrow one, and it should not be filed as a vulnerability:

• It is a genuine tenant data-isolation defect on the system of record.
• It is not reachable by an ordinary authenticated user in the deployments
  that use header or URL-path tenant routing, because both reject / outright.
• It is not a privilege escalation: the admin-reachable paths (1 and 2) are
  available to a caller who already holds cross-tenant administrative scope and
  could affect tenant a directly anyway. What is wrong is that they affect a
  different tenant than the one named, with no indication.
• Consequence 3 is a real cross-tenant read/write, but it requires the operator's
  IdP to emit a tenant claim with leading or trailing slashes — which nothing in
  HFS documents as forbidden, but which is also not a natural thing for an IdP to
  produce.

The realistic failure is an operator destroying or merging the wrong tenant's
data by provisioning or deleting a slash-padded id, not an attacker pivoting
between tenants. Calibrating it as a data-integrity defect with isolation
consequences rather than an exploitable vulnerability.

No evidence any deployment has hit this; slash-padded tenant ids are unusual and
nothing in the product suggests creating one.

Fix options

Option A — make the data prefix injective. Escape the tenant id into a single
key segment, exactly as registry_object_id already does for the registry key
(keyspace.rs:383-395); the helper exists and is tested. This is the same shape
as the #384 fix and would be the identity on tenant ids that contain no /,
\, space, or % — i.e. every id the header and URL-path routes can produce.

The obstacle is migration, and it is materially harder than #384's: S3 is the
system of record, not a derived index. For a/b-shaped ids the prefix would
change from a/b/... to a%2Fb/..., which means copying live objects, with no
$reindex-equivalent rebuild path. Any change here needs either a compatibility
read path or a documented object-migration step. That is the whole reason this is
a separate issue.

Note the migration cost falls only on deployments using hierarchical (a/b) or
slash-padded ids; ids from the header/URL charset are unaffected either way.

Option B — reject slash-padded ids at the boundary. Tighten
validate_tenant_id (admin_tenants.rs:128-139) to forbid leading/trailing /
(and, arguably, / entirely — see #385), and validate the JWT claim. Cheap, no
migration, and it closes the reachable paths. It does not make the derivation
correct, so it leaves the trap in place for any future caller that constructs a
TenantId directly.

Option C — both, with B shipping first as the immediate stop-gap.

My inclination is B first, then A if and when #385 settles the canonical tenant-id
charset — because if #385 rejects / in tenant ids outright, A's remaining value
is defence in depth rather than a fix for anything reachable.

Relation to other issues

• Elasticsearch: tenants differing only in case share an index and document _id (cross-tenant overwrite/delete) #384 — the Elasticsearch instance of this defect class, fixed in fix(persistence): make the Elasticsearch tenant→index derivation injective (#384) #446.
• Tenant ids have no canonical validator: divergent charsets and an infallible TenantId::new #385 — the canonical tenant-id validator. If it forbids /, or forbids
  leading/trailing separators, that closes the reachable paths here. This issue
  should probably be resolved after Tenant ids have no canonical validator: divergent charsets and an infallible TenantId::new #385 decides, since the decision determines
  whether option A is a fix or a hardening.
• S3 tenant registry: a tenant named tenants permanently breaks list_tenants() #271 — introduced the injective registry key but left the data path on the
  lossy trim; this is the other half of that fix.

Why this was not folded into #446

The Elasticsearch fix was safe to ship without a migration because its index is a
derived artifact rebuildable with $reindex. That argument does not transfer:
changing an S3 key prefix moves live objects that nothing else can reconstruct.
Bundling a change that needs an object migration into a PR that needs none would
have obscured exactly the risk that matters.

[mauripunzueta]

Draft PR #475 opened against `main`.

Two things the issue got wrong, recorded here rather than only in the PR:

1. This is not closed by #385 / PR #450. The issue suggested B-then-A, on the reading that #450 would close the reachable paths. It does not close consequence 1. #450 guards only the mint point (register_tenant) and deliberately leaves deregister_tenant and purge_tenant_data unguarded so operators can clean up legacy tenants. purge_tenant_data builds its TenantId directly from an operator-supplied path segment, with no validator in the path — so DELETE /admin/tenants/%2Fa?purge=true destroying tenant a survives #450 for any id already in a registry. The PR takes Option A because A is the half B structurally cannot reach, not as defence in depth.

2. There is a second defect in the same six lines. RESERVED_TENANT_IDS is compared against the whole id, so acme/resources provisions cleanly through the admin API — and then stores everything under acme/resources/…, which is exactly what tenant acme sweeps on purge and enumerates on list (list_current_keys selects on /current.json anywhere beneath the prefix). Cross-tenant delete and cross-tenant read, no IdP involvement required. Injectivity alone does not fix it — that needs prefix-disjointness, which is why the PR asserts both properties.

On the migration concern the issue raised. Option A does not need one as specified. The escape is applied only to ids that are not already safe, so a/b, default and __system__ keep their exact prefixes, and the only ids that move are those whose objects are already commingled with another tenant's or already inside another tenant's purge radius — where there is no coherent single-tenant dataset at the old location to preserve. The migration cost the issue anticipated falls to zero for every well-formed tenant.