fix(persistence): make the Elasticsearch tenant→index derivation injective (#384)

[mauripunzueta]

Closes #384.

Makes the Elasticsearch tenant → index derivation injective, so that every
_id-addressed operation is confined to exactly one tenant's index. Design
settled by a five-persona council (multi-tenancy architect, healthcare
compliance auditor, migration/ops steward, scope steward, regression-prevention
engineer), each position adversarially stress-tested; the disagreements they
did not resolve among themselves are adjudicated below.

The defect

ElasticsearchBackend::index_name was
format!("{prefix}_{tenant.to_lowercase()}_{type}"). to_lowercase() is not
injective, so tenants ACME and acme resolved to the same index, and with a
document_id of {type}_{id} they shared a document address outright.
GET/PUT/DELETE /{index}/_doc/{id} admit no query filter, so the eight
_id-addressed paths had nothing else holding the boundary.

The same expression was also partial: a tenant id containing / produced a
string Elasticsearch rejects, so every write for such a tenant 500'd.

Severity — narrower than the issue implies in one direction, wider in another

Narrower. There is no cross-tenant content disclosure through the HFS API.
There are exactly two _id-addressed reads: read re-checks
_source.tenant_id (storage.rs:774-781, as the issue says), and the
create_or_update probe reads only version_id. Search and count are
unconditionally filtered by a term tenant_id clause on a keyword field.
purge_all and purge_tenant_data are term-filtered too, so no path could wipe
a colliding tenant wholesale. This is an integrity and availability defect,
not a confidentiality one — with one narrow exception: the probe is a
cross-tenant existence-and-edit-frequency oracle, and "is patient X a patient of
clinic Y" is itself protected. Now closed.

Wider. While verifying the encoding I found the client's path-escaping table
(elasticsearch-8.15.0-alpha.1/src/http/request.rs:26-31):

const PARTS_ENCODED: &AsciiSet = &percent_encoding::NON_ALPHANUMERIC
    .remove(b'_').remove(b'-').remove(b'.').remove(b',').remove(b'*');

, and * reach Elasticsearch unescaped. JwtTenantExtractor
(crates/rest/src/tenant/resolver.rs:150-156) passes an unvalidated claim
straight into TenantId::new, so a JWT with tenant_id: "acme,other" made the
index string hfs_acme,other_patient — which Elasticsearch parses as a
two-index list — and tenant_id: "*" produced hfs_*_patient, a wildcard
across every tenant's index. Search, count, get and delete-by-query all resolve
those. Until now that was held back only by the term filter in the request
body, and the probe carried no such filter at all.

This is why an allowlist encoding was chosen over the issue's option (2),
"reject non-canonical tenant ids at the ES boundary": a blocklist has already
failed once here (case-folding is the bug), and it would not have caught ,/*
either, because those are dangerous for a reason living in the client's
escaping table rather than in Elasticsearch's index-name rules.

Also worth stating plainly: this is not a fringe configuration. README.md:182
and :184 list SQLite+Elasticsearch and PostgreSQL+Elasticsearch under
"Production deployments needing robust search."

Why option (1), and why not option (2)

Option (2) is worse than it looks. Elasticsearch is never standalone —
StorageBackendMode (crates/rest/src/config.rs:78-114) only ever pairs it as a
search secondary — so a backend-local validity domain means, in a
postgres-elasticsearch deployment, that a write to tenant ACME succeeds at
the primary and fails at the secondary: the resource exists and is unfindable.
It also makes a search index the de-facto tenant-admission authority for the
whole server, overruling the REST validators that already admitted the request.
And it offers no remedy — there is no tenant-rename operation, so an existing
ACME deployment would 500 forever.

The encoding

New backends/elasticsearch/naming.rs is the single derivation. Bytes in
{a-z, 0-9, '-', '_', '.'} pass through; every other byte becomes + plus two
lowercase hex digits. + is not in the safe set, so it self-escapes.

Injective by left inverse (decode_tenant_segment; round-trip asserted over the
corpus). Legal by construction — the output alphabet excludes every forbidden
character and contains no uppercase byte. The leading-character and ./..
rules are discharged by the index prefix, which is now validated at construction;
that validation is what the legality proof actually rests on, so it is part of
the fix rather than a nicety.

+ rather than %, deliberately diverging from #271's S3 percent form: no HFS
tenant-routing surface accepts +, so it is only ever produced by us; and % in
a URL path is the double-decode hazard, where an intermediary that decodes once
turns a%252Fb into a%2Fb and Elasticsearch then reads a /. A spurious
decode of %2B merely yields +.

Migration: none, for any deployment that is not already broken

The encoding is the identity on [a-z0-9._-]*, so for an already-lowercase
tenant id the index name is byte-identical to what shipped. Those deployments
see no rename, no reindex, nothing.

Deployment	On upgrade
All-lowercase tenant ids (effectively all of them)	Nothing. hfs_acme_patient before and after.
Has an uppercase tenant id	That tenant's writes/searches move to a new, initially-empty index. Search under-reports until $reindex. Its lowercase counterpart is unaffected.
Has a /-bearing tenant id	Goes from 500 on every write to working. No index to rename — none was ever created.

This is why the property is pinned by a named test
(already_safe_tenant_ids_are_unchanged_so_conforming_deployments_do_not_migrate)
rather than left as a remark: if it ever fails, every conforming deployment on
earth needs a reindex.

No backfill tool, no dual-write, no alias flip. Elasticsearch is a derived
artifact and $reindex is online, async, per-tenant and already wired into the
composite. Rollback is safe for the same reason: nothing touches the primary.

Operator runbook (only for deployments with a non-lowercase tenant id)

1. Before upgrading, list tenant ids containing uppercase or /. Empty list →
   nothing to do.
2. Upgrade. The server logs a WARN naming any index that holds misplaced
   documents (see below).
3. POST /$reindex per affected tenant — needs the system/reindex scope, and
   $reindex-status only answers on the node that accepted the kickoff.
4. Remove the strays with a delete-by-query filtered on that tenant's exact
   tenant_id. $reindex --clearExisting will not do this, and neither will
   that tenant's future $purge: both scope to the new index pattern, so the
   pre-upgrade documents survive a tenant purge silently. This step is the one
   with a compliance consequence.

Detection, for operators who want to check before upgrading

tenant_id is mapped keyword, so this is exact:

GET /hfs_*/_search
{ "size": 0,
  "aggs": { "per_index": { "terms": { "field": "_index", "size": 1000 },
    "aggs": { "tenants":   { "terms": { "field": "tenant_id", "size": 100 } },
              "n_tenants": { "cardinality": { "field": "tenant_id" } },
              "collision": { "bucket_selector": {
                  "buckets_path": { "n": "n_tenants" }, "script": "params.n > 1" } } } } } }

Empty result = no co-mingling. Substitute your HFS_ES_INDEX_PREFIX.

Without cluster access: GET /{tenant}/Patient?_summary=count is served by the
primary while GET /{tenant}/Patient?_count=0's total comes from
Elasticsearch. On an affected deployment they diverge.

Five derivation sites collapsed to one

index_name plus four hand-rolled format!("{prefix}_{tenant.to_lowercase()}_*")
globs (delete_contained_docs, count, purge_tenant_data,
tenant_index_pattern) now all route through naming. Not tidying: had the
exact name been escaped while one glob was not, the glob would stop matching the
indices that exist, and purge_tenant_data would delete nothing while
reporting success. The most likely way this PR could have gone wrong is
missing one of the five, which is what the live count-in-both-directions
assertion exists to catch.

document_id deliberately unchanged

The issue proposes making it injective too. Rejected, unanimously by the three
council members who examined it. An Elasticsearch _id is unique only within
its index, so an injective index_name already guarantees no two tenants can
contend for one — the tenant component buys nothing. It would cost a great deal:
it changes the address of every document in every deployment including the
healthy ones, and because delete removes only the new _id, every pre-upgrade
document would linger as a permanently-undeletable duplicate search hit. That
converts a rare integrity bug into a universal, silent, non-self-healing one.
document_id_carries_no_tenant_component is the alarm if someone adds it later.

Also in this PR

• create_or_update's existence probe now checks _source.tenant_id,
  mirroring read. Redundant once the derivation is injective, but it is the one
  place this backend reads foreign state to make a write decision. A mismatch
  is treated as absent, not an error: an upgraded index can still hold
  documents a colliding tenant left behind, and erroring would brick the rightful
  owner on exactly those ids, permanently, with no operator remedy. Treating them
  as absent is self-healing and correct on the merits.
• Startup diagnostic. One aggregation over {prefix}_* warns when an index
  holds documents whose tenant_id does not re-encode to that index's tenant
  segment. Note the shape: a name-based check would have been useless, because
  the old derivation lowercased — ACME wrote to {prefix}_acme_patient, a
  perfectly well-formed name for acme. What is wrong is the contents. Best
  effort; never fails startup, since misplaced documents are inert and refusing
  to boot would turn a search-completeness problem into an outage.
• Removed schema::delete_index — dead code, and an unreachable whole-index
  DELETE is a footgun whose safety argument (it is only correct because
  index_name is injective) the first caller would not re-derive. Its plausible
  use is already served, document-level and term-filtered, by
  purge_tenant_data/purge_all.
• Corrected a wrong comment on tenant_index_pattern: it claimed tenant a
  matches tenant ab. It does not — the pattern requires the literal separator.
  The real over-match is a vs a_b, and it is deliberate (keeping _ in the
  safe set is what spares my_tenant-shaped ids from renaming); the term
  filter is what isolates.

"Address this for all DB"

The other backends are already injective on the tenant, so the honest
discharge is proof plus a guard, not change — changing correct code to satisfy a
rule is its own failure mode.

Backend	Derivation	Status
Elasticsearch	index name	Was broken — fixed here
SQLite	tenant_id TEXT NOT NULL, default BINARY collation, no COLLATE NOCASE	Correct; guard added
PostgreSQL	tenant_id TEXT NOT NULL, no COLLATE/citext	Correct (read, not tested here)
MongoDB	tenant_id document field, no collation override anywhere	Correct (read, not tested here)
S3	registry key injective via registry_object_id (#271)	Correct for the registry — see below

The SQLite guard is a real behavioural test, not a tautology, and it is free
(in-memory, runs on every PR). It is placed there specifically because
COLLATE NOCASE is used liberally in that backend's search SQL, so the idiom is
one copy-paste from a tenant predicate. PostgreSQL/MongoDB are asserted by
reading the schema; no test guards a future citext migration, and saying so is
part of the claim.

Discovered, deliberately not fixed here

S3Keyspace::with_tenant_prefix is non-injective (keyspace.rs:36-43):
tenant_id.trim_matches('/') means tenants a, /a, a/ and //a share one
data prefix, while registry_object_id gives each a distinct registry record.
admin_tenants.rs:131-139 accepts / anywhere, so this is reachable. Same
defect class as #384.

Not fixed in this PR, and the reason is not scope-tidiness: S3 is the system of
record. Re-prefixing moves live objects with no $reindex-equivalent rebuild
path, whereas the Elasticsearch fix is safe precisely because its index is
derived. Bundling a safe change with an unsafe one in a single security PR is
how the unsafe one ships unexamined.

Filed as #447 with the full reachability analysis. Summary of the calibration
there: it is a genuine data-isolation defect, but not reachable by an ordinary
authenticated user — the header and URL-path tenant routes both reject /
outright, so it needs either an operator provisioning a slash-padded id or an IdP
emitting one. The realistic failure is
DELETE /admin/tenants/%2Fa?purge=true destroying tenant a's resources and
history while deregistering only /a.

Also for #385, whose severity this raises: the ,/* index-injection vector
above is mitigated here at the Elasticsearch boundary but not at any other
backend, and it exists because JwtTenantExtractor validates nothing.

Deliberately out of scope

• Tenant ids have no canonical validator: divergent charsets and an infallible TenantId::new #385 itself. The encoding is injective over arbitrary &str, with its
  proof self-contained, so this fix never depends on a validator that does not
  yet exist (~198 TenantId::new call sites). The two ship independently, and
  landing this does not make Tenant ids have no canonical validator: divergent charsets and an infallible TenantId::new #385 less necessary.
• crates/persistence/src/backends/local_fs/mod.rs:42 — root.join(tenant_id)
  has the same defect class on case-insensitive filesystems (macOS, Windows).
  Different subsystem, not a database, and no victim on the Linux container
  runtime HFS ships. Noted here rather than filed.

Verification

No C linker in this environment, so nothing was compiled or run locally — CI is
the gate. cargo fmt clean. What was verified by hand instead:

• Elasticsearch's index-name rules against upstream documentation, and the
  client's path-escaping table against the vendored crate source, rather than
  assumed.
• The encoder's injectivity over the full adversarial corpus, its identity on
  safe ids, and the resulting index-name lengths, cross-checked against an
  independent model of the same specification.
• Every symbol, import, feature gate and call site checked by hand against the
  tree.

The two live-cluster tests run on an ordinary PR: elasticsearch_tests.rs is
gated only on #![cfg(feature = "elasticsearch")], ci.yml runs
--all-features, and ci.yml:26-30 hard-fails the job when DOCKER_HOST is
absent — so they cannot skip vacuously (cf. #390). The isolation test fails on
main today.

Residual risk

1. Pre-existing co-mingling is frozen, not repaired. Documents already lost
   to an overwrite are gone; this fix stops the bleeding and makes the state
   visible, nothing more.
2. An affected tenant's pre-upgrade $purge completeness depends on the operator
   performing runbook step 4.
3. count and list_resource_types still depend on a human remembering the
   term tenant_id filter; that invariant is review discipline, not construction.
4. The startup diagnostic reports; it does not repair, and it is silent when the
   cluster is unreachable at boot.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.