fix(tenant): one canonical tenant-id validator, enforced at every ingress (#385)

[mauripunzueta]

Closes #385.

What was wrong

Tenant ids had no single definition. Four surfaces validated them and all four disagreed, and TenantId::new imposed nothing beneath them:

surface	charset	max	on invalid
tenant::resolver (header + URL)	[A-Za-z0-9_-]	64	silently ignored
middleware::tenant_prefix (URL strip)	[A-Za-z0-9_-]	64	not a tenant prefix
handlers::admin_tenants (provisioning)	[A-Za-z0-9_.-/]	128	400
JWT tenant claim	anything	—	—
TenantId::new	anything	—	—

So each backend defended individually, which is how the Elasticsearch case-collision (#384) became reachable.

Two defects the issue did not name

1. The header validator failed open. HeaderTenantExtractor::extract used .filter(…is_valid_tenant_id), so an invalid value became None and TenantResolver::resolve fell through to the default tenant. X-Tenant-ID: acme.corp — a ., which the old header charset rejected but the admin API always accepted — silently read and wrote the default tenant's data and returned 200. A typo produced a cross-tenant read/write.

2. Reserved names were compared against the whole id. acme/resources passed. S3Keyspace::with_tenant_prefix does not escape, and resources_prefix() is join(["resources/"]), so:

• tenant acme scans and purges acme/resources/
• tenant acme/resources stores at acme/resources/resources/… and acme/resources/history/…

S3Backend::purge_tenant_data("acme") deletes the other tenant's data. keyspace.rs now carries a test asserting both that the prefixes really do overlap and that the id is unconstructible.

The change

TenantId::parse is the one definition. TenantId::new stays as the unchecked constructor for trusted reconstruction (rebuilding from a stored tenant_id — 213 call sites, where a fallible constructor would only add unwrap()s), and FromStr routes through parse since str::parse is where a reader expects validation. The two private is_valid_tenant_id copies are gone.

Charset: [A-Za-z0-9._-] plus / for hierarchy. 1..=64 bytes. No leading, trailing, or repeated /. No segment equal to __system__, tenants, resources, history, bulk, _system.user-settings, ., or ... Case preserved.

Enforced at every ingress, with the failure semantics distinguished:

• TenantSourceExtractor::extract gains an Err arm. Ok(None) still means "this source asserted nothing"; Err means "asserted, and invalid".
• Header / URL assertion → 400.
• JWT claim → 403. The token is authenticated and the request well-formed; it is the authorization context that is unusable, matching the existing claimless-token path.
• UrlPathTenantExtractor never returns Err — for it "not a valid tenant id" genuinely means "not a tenant prefix" (/Patient/123). A naive fallible refactor would 400 every untenanted request.

Databases — all backends addressed

backend	change	why
SQLite	register_tenant backstop	exact-match tenant_id column, BINARY collation — no derivation to protect
PostgreSQL	register_tenant backstop	same, case-sensitive TEXT
MongoDB	register_tenant backstop	exact BSON match, default (case-sensitive) collation
S3	register_tenant backstop	the /-structured keyspace is what the per-segment reserved list protects
Composite	register_tenant backstop	what a search-backed deployment actually calls
Elasticsearch	untouched	left to #384 / #446

The backstop is ResourceStorage::ensure_canonical_tenant_id, a provided trait method each implementation calls first. It is deliberately not applied to deregister_tenant, purge_tenant_data, or any read path — those are how an operator cleans up a non-canonical tenant that predates this validator, and guarding them would make bad data permanently undeletable. Only the mint point is guarded.

crates/persistence/src/tenant/mod.rs now documents what a backend may rely on, and — as importantly — what it may not: 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.

Relationship to #384 / PR #446

Complementary and independent; this branches from main and touches no elasticsearch/ file.

#446 makes the tenant→index derivation injective for any id, so it does not need this charset. This change supplies the 64-byte bound that #446's index-name legality proof rests on ("≤255 bytes … for every id the validated routing accepts" — a +xx escape triples worst case, so 64→192 fits and 128→384 would not). Whichever merges second rebases cleanly.

Compatibility

Fixed, not broken: a malformed X-Tenant-ID returns 400 instead of silently serving the default tenant.

Deliberate tightening — the JWT claim is the big one, and it deserves stating plainly rather than as a footnote. That claim previously accepted any string; it now accepts only the canonical charset. For an auth-enabled deployment this is the largest behavioural change here. A claim shaped like urn:oid:1.2.3 or acme@corp — anything carrying :, @, whitespace, or non-ASCII — now returns 403 where it used to resolve and read/write data under that literal string.

Such a tenant was already unusable through the header and URL routes (both charsets excluded those characters, as did the admin API), so it could only ever be reached through this one unvalidated ingress. But if an IdP issues claims in that shape, this PR breaks that deployment, and the remedy is to change the claim — not to widen the validator, which would re-open the class the issue is about.

Two further shapes, both previously accepted only by the admin API:

• ids of 65..=128 bytes — already unroutable via header and URL (both capped at 64), so reachable only through the JWT claim;
• hierarchical ids containing a reserved segment — the acme/resources class above.

Deliberate widening: . is accepted on every ingress, so a tenant provisioned as tenant.example is now routable instead of registered-and-unreachable. This required reserving .well-known: without it, /.well-known/smart-configuration would parse .well-known as a tenant and rewrite the path to /smart-configuration, turning SMART discovery into a 404 under url_path/both routing. Caught during review; smart_discovery_is_not_parsed_as_a_tenant_prefix pins it.

Finding what breaks. Ids already stored are untouched — they round-trip through the unchecked new, so no data moves and nothing is rewritten. But an id that is no longer reachable would otherwise be invisible, so GET /admin/tenants now reports canonical per row and non_canonical_count in the body. Rows with canonical: false and a non-zero resources count are exactly the stranded set. DELETE /admin/tenants/{id} still accepts a non-canonical id, deliberately, so they stay cleanable — the backstop guards only the mint point.

Not fixed here

strip_tenant_prefix (routing/fhir_routes.rs:164) runs before route matching under url_path/both, and RESERVED_SYSTEM_PATHS does not list every registered top-level route. export-status, export-file, bulk-submit-file, bulk-submit-status, and ws all parse as tenant ids today and are rewritten out of existence under those routing modes. That predates this change (their segments were valid under the old charset too) and is a route-reservation defect rather than a tenant-id one, so it is left alone rather than folded in — fixing it properly means auditing every registered route. .well-known is fixed here only because this change is what would have broken it.

Verification

A user-local C toolchain was set up for this task (no root; gcc-13 extracted from .debs), so this branch is verified locally now — earlier notes in this PR saying otherwise are superseded.

Locally green: cargo fmt --all --check; cargo test --workspace --lib (all crates); helios-persistence lib 813 passed (includes the tenant validator and the S3 keyspace tests); helios-rest lib 466 passed; helios-rest --test admin_tenants 31 passed; --test tenant_resolution 41 passed. Clippy is clean over the changed files (local clippy 1.96 flags two pre-existing lints elsewhere — crates/serde-support docs and crates/rest/src/error.rs:473 — that CI's older stable does not; untouched deliberately).

Running the suite locally caught a real defect in this PR that review had not: is_valid_tenant_id asserted tenant/path was invalid while delegating wholly to TenantId::parse, which accepts hierarchical ids. The assertion was wrong, not the code — the single-segment constraint lives in the caller, which passes path.split('/').next(). Fixed, explained in the doc comment, and the hierarchical case added to the agreement test so a future divergence fails loudly.

Still CI-only: anything needing Docker (testcontainers). CI's Test Rust has not yet produced a verdict on this branch for an unrelated reason — the shared Docker host is out of disk (disk quota exceeded on containerd's metadata store), which fails ~148 testcontainers cases in helios-hts and, because cargo test --workspace fails fast, prevents helios-persistence and helios-rest from running at all. Every branch in the repo is affected; two reruns reproduced it identically. Linting and Security Audit are green.

Test coverage added: 12 cases on TenantId::parse (including a "shapes accepted before this validator must stay valid" guard against orphaning data), the header fail-closed regression at both extractor and resolver level, header/URL classifier agreement (load-bearing — authz_middleware and the resolver must agree on which tenant a path is for), JWT-claim rejection at the extractor, per-segment reserved names and the 64-byte cap through the admin API, the S3 prefix-overlap demonstration, the .well-known regression, and the admin listing's canonicality reporting.

[codecov]

Codecov Report

❌ Patch coverage is 95.46436% with 21 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/rest/src/tenant/resolver.rs	95.48%	7 Missing ⚠️
crates/persistence/src/core/storage.rs	37.50%	5 Missing ⚠️
crates/rest/src/extractors/tenant.rs	95.18%	4 Missing ⚠️
crates/rest/src/error.rs	0.00%	3 Missing ⚠️
crates/persistence/src/tenant/id.rs	98.72%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!