From c2cfd07530beccb2eeb1762314b5c846a07d1a84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Thu, 30 Jul 2026 11:12:13 -0400 Subject: [PATCH 1/4] fix(tenant): one canonical tenant-id validator, enforced at every ingress (#385) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tenant ids had no single definition. Four surfaces validated them and all four disagreed: | surface | charset | max | on invalid | |--------------------------------|--------------------|-----|------------| | tenant::resolver (header, URL) | [A-Za-z0-9_-] | 64 | ignored | | middleware::tenant_prefix | [A-Za-z0-9_-] | 64 | not a tenant | | handlers::admin_tenants | [A-Za-z0-9_.-/] | 128 | 400 | | JWT tenant claim | anything | - | - | and `TenantId::new` imposed nothing beneath them, so each backend had to defend individually — which is how the Elasticsearch case-collision (#384) became reachable. `TenantId::parse` is now the one definition; `TenantId::new` stays as the unchecked constructor for trusted reconstruction (rebuilding from a stored `tenant_id`), and `FromStr` routes through `parse`. Every ingress uses it: header, URL prefix, JWT claim, admin body. `ResourceStorage::register_tenant` re-checks it on all five implementations as a backstop, so no future ingress can mint an id that skipped validation. Two defects this closes that the issue did not name: * The header validator failed *open*. `HeaderTenantExtractor` filtered an invalid value to `None`, so the resolver fell through to the **default tenant** and served the request — a `200` reading and writing another tenant's data because of a typo. `TenantSourceExtractor::extract` now has an `Err` arm; a header that asserts an invalid tenant is a `400`, and an invalid JWT claim is a `403` (the token is authenticated; it is the authorization context that is unusable). * Reserved names were compared against the whole id, so `acme/resources` passed. On S3 its data lands at `acme/resources/resources/…`, inside the `acme/resources/` prefix that tenant `acme` lists — and that `purge_tenant_data("acme")` deletes. Reserved segments are now rejected in any position. Charset: `[A-Za-z0-9._-]` plus `/` for hierarchy, 1..=64 bytes, no leading / trailing / repeated `/`, no reserved segment. Case is preserved — folding it would silently merge tenants that every backend keeps apart, and #384 fixes the Elasticsearch collision by making that derivation injective rather than by narrowing the charset here. Databases: SQLite, PostgreSQL, and MongoDB key tenants by an exact-match, case-sensitive column/field and have no derivation to protect, so they need no change beyond the shared `register_tenant` backstop. S3's `/`-structured keyspace is what the per-segment reserved list protects. Elasticsearch is left entirely to #384/PR #446, which makes its index derivation injective; this change supplies the 64-byte bound that PR's index-length proof rests on. Deliberate tightening: ids of 65..=128 bytes and hierarchical ids containing a reserved segment are refused where the admin API used to accept them. Both were reachable only through the JWT claim, the ingress that validated nothing — the header and URL validators already capped at 64. Deliberate widening: `.` is accepted on every ingress. The admin API always allowed it, so a tenant provisioned as `tenant.example` was registered and unroutable. That required reserving `.well-known`, which would otherwise parse as a tenant and turn SMART discovery into a 404 under url_path/both routing; a regression test pins it. --- .../src/backends/mongodb/storage.rs | 5 + .../src/backends/postgres/storage.rs | 5 + .../persistence/src/backends/s3/keyspace.rs | 67 ++- crates/persistence/src/backends/s3/storage.rs | 7 + .../src/backends/sqlite/storage.rs | 6 + crates/persistence/src/composite/storage.rs | 6 + crates/persistence/src/core/storage.rs | 27 ++ crates/persistence/src/error.rs | 15 +- crates/persistence/src/tenant/id.rs | 427 +++++++++++++++++- crates/persistence/src/tenant/mod.rs | 29 +- crates/rest/src/error.rs | 5 + crates/rest/src/extractors/tenant.rs | 138 +++++- crates/rest/src/handlers/admin_tenants.rs | 82 ++-- crates/rest/src/middleware/tenant_prefix.rs | 82 +++- crates/rest/src/tenant/mod.rs | 8 +- crates/rest/src/tenant/resolver.rs | 381 +++++++++++++--- crates/rest/tests/admin_tenants.rs | 93 +++- crates/rest/tests/tenant_resolution.rs | 12 +- 18 files changed, 1236 insertions(+), 159 deletions(-) diff --git a/crates/persistence/src/backends/mongodb/storage.rs b/crates/persistence/src/backends/mongodb/storage.rs index 6bf591ddd..da6f8efa3 100644 --- a/crates/persistence/src/backends/mongodb/storage.rs +++ b/crates/persistence/src/backends/mongodb/storage.rs @@ -1507,6 +1507,11 @@ impl ResourceStorage for MongoBackend { id: &str, display_name: Option<&str>, ) -> StorageResult { + // Backstop for the canonical tenant-id contract (issue #385). MongoDB + // matches `tenant_id` as an exact BSON string under the default + // (case-sensitive) collation, so it has no derivation to protect — this + // keeps the precondition uniform across every implementation. + self.ensure_canonical_tenant_id(id)?; let db = self.get_database().await?; let tenants = db.collection::(MongoBackend::TENANTS_COLLECTION); // RFC 3339 string, matching the SQLite registry's `created_at` format so diff --git a/crates/persistence/src/backends/postgres/storage.rs b/crates/persistence/src/backends/postgres/storage.rs index fd6ef1cce..db5f4af9a 100644 --- a/crates/persistence/src/backends/postgres/storage.rs +++ b/crates/persistence/src/backends/postgres/storage.rs @@ -777,6 +777,11 @@ impl ResourceStorage for PostgresBackend { id: &str, display_name: Option<&str>, ) -> StorageResult { + // Backstop for the canonical tenant-id contract (issue #385). As with + // SQLite, PostgreSQL keys tenants by an exact-match, case-sensitive + // `tenant_id` column and has no derivation to protect — this keeps the + // precondition uniform across every implementation. + self.ensure_canonical_tenant_id(id)?; let client = self.get_client().await?; // Plain INSERT so a duplicate id surfaces as a constraint error; the // admin handler pre-checks existence and returns 409, so reaching here diff --git a/crates/persistence/src/backends/s3/keyspace.rs b/crates/persistence/src/backends/s3/keyspace.rs index 08f8b3cb2..f63c29670 100644 --- a/crates/persistence/src/backends/s3/keyspace.rs +++ b/crates/persistence/src/backends/s3/keyspace.rs @@ -304,11 +304,14 @@ impl S3Keyspace { /// `{digest}.json` leaf — and `purge_tenant_data` sweeps only those /// sub-prefixes, so it cannot delete a settings object either. /// - /// Do **not** weaken this to "tenant IDs cannot contain `.`". That is true of - /// the routing validators (`is_valid_tenant_id`), but *not* of - /// `admin_tenants::validate_tenant_id`, which permits `.` and `/`, nor of the - /// JWT tenant extractor, which validates nothing. The name is dotted to keep - /// it unroutable, but the safety of this namespace must not depend on that. + /// Do **not** weaken this to "tenant IDs cannot contain `.`". That was never + /// reliably true, and since issue #385 it is plainly false: the canonical + /// charset ([`TenantId::parse`](crate::tenant::TenantId::parse)) permits `.` + /// on every ingress, so `_system.user-settings` is a *well-formed* tenant id. + /// It is refused only because it is listed in + /// [`RESERVED_TENANT_SEGMENTS`](crate::tenant::RESERVED_TENANT_SEGMENTS) — + /// and that list is a guardrail, not this namespace's safety proof. + /// /// In particular, a future change that widened a tenant purge to sweep the /// whole tenant prefix would break the structural argument above and must /// exclude this namespace explicitly. @@ -377,9 +380,12 @@ fn sanitize(value: &str) -> String { /// tenant is worth more here than fixed-length keys. Ids reaching this point are /// length-capped upstream, so S3's 1024-byte key limit is not a concern. /// -/// Only `/` is reachable through the admin API's charset — `\` and space are -/// already rejected there — so in practice this changes the key of exactly the -/// hierarchical ids that were previously colliding. +/// Only `/` is reachable through the canonical charset +/// ([`TenantId::parse`](crate::tenant::TenantId::parse)) — `\`, space, and `%` +/// are rejected on every ingress since issue #385 — so in practice this changes +/// the key of exactly the hierarchical ids that were previously colliding. The +/// `\`/space/`%` arms are kept anyway: this function's contract is to be +/// injective for *any* input, so it does not inherit the validator's bounds. fn registry_object_id(tenant_id: &str) -> String { let mut out = String::with_capacity(tenant_id.len()); for c in tenant_id.chars() { @@ -398,6 +404,51 @@ fn registry_object_id(tenant_id: &str) -> String { mod tests { use super::*; + /// Demonstrates the collision the canonical validator's per-segment reserved + /// list exists to prevent (issue #385), and pins that the validator prevents + /// it. + /// + /// `with_tenant_prefix` does not escape, so tenant `acme/resources` nests its + /// data at `acme/resources/resources/…` — *inside* the `acme/resources/` + /// prefix that tenant `acme` lists and that `purge_tenant_data("acme")` + /// deletes. This is a cross-tenant destructive collision, and it survives + /// entirely at the keyspace level: nothing here can tell the two apart. + /// + /// The fix is upstream — such an id can no longer be constructed — so this + /// test asserts both halves: the keyspace really does overlap, and + /// `TenantId::parse` really does refuse the id. If someone ever removes the + /// reserved segment, the second assertion fails and points at the first. + #[test] + fn hierarchical_id_naming_a_keyspace_namespace_would_overlap_its_parent() { + use crate::tenant::TenantId; + + let base = S3Keyspace::new(None); + let parent = base.with_tenant_prefix("acme"); + let nested = base.with_tenant_prefix("acme/resources"); + + // The overlap is real: everything the nested tenant writes sits under the + // prefix the parent sweeps. + let parent_scan = parent.resources_prefix(); + assert!( + nested.resources_prefix().starts_with(&parent_scan), + "expected {} to sit inside {parent_scan}", + nested.resources_prefix() + ); + assert!( + nested.history_root_prefix().starts_with(&parent_scan), + "expected {} to sit inside {parent_scan}", + nested.history_root_prefix() + ); + + // Which is why the id is unconstructible. + assert!( + TenantId::parse("acme/resources").is_err(), + "the reserved segment is what keeps the overlap unreachable" + ); + // The parent itself stays perfectly valid. + assert!(TenantId::parse("acme").is_ok()); + } + /// The registry key establishes tenant identity: ids that a lossy sanitiser /// would collapse together must map to distinct objects. A collision here /// lets one tenant read, overwrite, and deregister another's record. diff --git a/crates/persistence/src/backends/s3/storage.rs b/crates/persistence/src/backends/s3/storage.rs index 0dfd33864..6680fc753 100644 --- a/crates/persistence/src/backends/s3/storage.rs +++ b/crates/persistence/src/backends/s3/storage.rs @@ -1057,6 +1057,13 @@ impl ResourceStorage for S3Backend { id: &str, display_name: Option<&str>, ) -> StorageResult { + // Backstop for the canonical tenant-id contract (issue #385). S3 is the + // backend with the most to lose here: `with_tenant_prefix` gives `/` + // structural meaning, so tenant `a/resources` would store under + // `a/resources/…` — inside the prefix tenant `a` lists and + // `purge_tenant_data("a")` deletes. `TenantId::parse` rejects the + // reserved segment, making that shape unconstructible. + self.ensure_canonical_tenant_id(id)?; let location = self .registry_location() .ok_or_else(|| self.tenant_registry_unsupported())?; diff --git a/crates/persistence/src/backends/sqlite/storage.rs b/crates/persistence/src/backends/sqlite/storage.rs index e0fbfb3ef..939b241bc 100644 --- a/crates/persistence/src/backends/sqlite/storage.rs +++ b/crates/persistence/src/backends/sqlite/storage.rs @@ -834,6 +834,12 @@ impl ResourceStorage for SqliteBackend { id: &str, display_name: Option<&str>, ) -> StorageResult { + // Backstop for the canonical tenant-id contract (issue #385). SQLite + // keys tenants by an exact-match `tenant_id` column, so it has no + // collision of its own to defend against — this guards the *registry* + // from minting an id the other backends could not keep distinct, and + // keeps the precondition uniform across every implementation. + self.ensure_canonical_tenant_id(id)?; let conn = self.get_connection()?; // Plain INSERT so a duplicate id surfaces as a constraint error; the // admin handler pre-checks existence and returns 409, so reaching here diff --git a/crates/persistence/src/composite/storage.rs b/crates/persistence/src/composite/storage.rs index a2ebc3672..42f3484fb 100644 --- a/crates/persistence/src/composite/storage.rs +++ b/crates/persistence/src/composite/storage.rs @@ -1037,6 +1037,12 @@ impl ResourceStorage for CompositeStorage { id: &str, display_name: Option<&str>, ) -> StorageResult { + // Checked here as well as in the primary (issue #385). The composite is + // what a search-backed deployment actually calls, and its secondaries — + // Elasticsearch above all — derive index names from the tenant id. The + // primary's own check would still catch this, but only after the + // composite had already accepted the id as legitimate. + self.ensure_canonical_tenant_id(id)?; self.primary.register_tenant(id, display_name).await } diff --git a/crates/persistence/src/core/storage.rs b/crates/persistence/src/core/storage.rs index 82f2c826e..29c89931f 100644 --- a/crates/persistence/src/core/storage.rs +++ b/crates/persistence/src/core/storage.rs @@ -778,9 +778,36 @@ pub trait ResourceStorage: Send + Sync { Ok(None) } + /// Rejects a tenant id that does not satisfy the canonical definition. + /// + /// Every implementation of [`register_tenant`](Self::register_tenant) calls + /// this first. It is a **backstop**, not the primary control: ids are + /// validated at each REST ingress (header, URL prefix, JWT claim, admin + /// body). Its job is to make the storage-layer precondition documented in + /// [`crate::tenant`] true by construction, so a future ingress — or an + /// embedder using this crate directly — cannot mint a tenant that skipped + /// validation. Issue #385. + /// + /// 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; rejecting there would make bad data + /// permanently undeletable. Only the mint point is guarded. + fn ensure_canonical_tenant_id(&self, id: &str) -> StorageResult<()> { + crate::tenant::TenantId::parse(id).map(|_| ()).map_err(|e| { + StorageError::Tenant(crate::error::TenantError::NonCanonicalTenantId { + tenant_id: id.to_string(), + reason: e, + }) + }) + } + /// Registers (provisions) a new tenant, stamping `created_at` with the /// current time. Returns [`StorageError`] if the tenant is already /// registered. Default: unsupported-capability error. + /// + /// Implementations must call + /// [`ensure_canonical_tenant_id`](Self::ensure_canonical_tenant_id) before + /// writing anything. async fn register_tenant( &self, _id: &str, diff --git a/crates/persistence/src/error.rs b/crates/persistence/src/error.rs index 36a32ec5d..1a7d99559 100644 --- a/crates/persistence/src/error.rs +++ b/crates/persistence/src/error.rs @@ -8,7 +8,7 @@ use std::fmt; use thiserror::Error; -use crate::tenant::TenantId; +use crate::tenant::{TenantId, TenantIdError}; /// The primary error type for all storage operations. /// @@ -168,6 +168,19 @@ pub enum TenantError { tenant_id: TenantId, }, + /// A tenant id offered for provisioning is not canonical. + /// + /// Distinct from [`InvalidTenant`](Self::InvalidTenant) because it carries + /// the *reason*: this reaches an operator through the admin API, where "the + /// id is 71 bytes" is actionable and "invalid tenant" is not. + #[error("tenant id '{tenant_id}' is not valid: {reason}")] + NonCanonicalTenantId { + /// The rejected identifier, as supplied. + tenant_id: String, + /// Why [`TenantId::parse`](crate::tenant::TenantId::parse) rejected it. + reason: TenantIdError, + }, + /// Tenant is suspended and cannot perform operations. #[error("tenant suspended: {tenant_id}")] TenantSuspended { diff --git a/crates/persistence/src/tenant/id.rs b/crates/persistence/src/tenant/id.rs index b2278fefd..c0328a7da 100644 --- a/crates/persistence/src/tenant/id.rs +++ b/crates/persistence/src/tenant/id.rs @@ -15,6 +15,102 @@ use serde::{Deserialize, Serialize}; /// CodeSystems, ValueSets, and other terminology resources. pub const SYSTEM_TENANT: &str = "__system__"; +/// Maximum length of a tenant id, in bytes. +/// +/// 64 rather than something larger for two reasons. It is what the REST +/// tenant-routing surfaces have always enforced, so it is the widest bound that +/// every *routable* tenant already satisfies. And the Elasticsearch backend's +/// index-name derivation escapes unsafe bytes as three-byte sequences against a +/// 255-byte Elasticsearch limit; 64 × 3 = 192 leaves room for the index prefix +/// and resource type, whereas a 128-byte id could not fit. +pub const MAX_TENANT_ID_LEN: usize = 64; + +/// Segments a tenant id may not contain, in any position. +/// +/// These name control-plane namespaces inside a storage backend's keyspace. +/// The check is **per segment**, not on the whole id, because the hierarchy +/// separator is what makes them dangerous: on S3 a tenant's data lives under +/// `{tenant}/resources/…` and `{tenant}/history/…`, so tenant `a/resources` +/// stores at `a/resources/resources/…` — inside the prefix tenant `a` scans and +/// purges. `S3Backend::purge_tenant_data("a")` would delete it. Rejecting the +/// segment anywhere makes that shape unconstructible. +/// +/// `__system__` is listed here too so `a/__system__` cannot be used to smuggle +/// the shared-tenant sentinel past a check that only compares the whole id. +/// +/// Kept in sync with `backends::s3::keyspace::S3Keyspace`, which owns +/// `tenants/`, `resources/`, `history/`, `bulk/`, and `_system.user-settings/`. +/// Each of those is *also* safe structurally — a tenant so named writes to a +/// `resources/`/`history/` **sub**-prefix, which can never equal a control-plane +/// leaf — so this list is defence in depth, not the proof. Do not delete the +/// structural arguments in `S3Keyspace` on the strength of it. +pub const RESERVED_TENANT_SEGMENTS: &[&str] = &[ + SYSTEM_TENANT, + "tenants", + "resources", + "history", + "bulk", + "_system.user-settings", + // Relative-path segments. No backend resolves `..`, but a tenant id flows + // into object keys and filesystem-shaped paths, where a normalising + // intermediary would make `a/../b` and `b` the same location. + ".", + "..", +]; + +/// Why a string is not a valid tenant id. +/// +/// Returned by [`TenantId::parse`], the canonical validating constructor. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum TenantIdError { + /// The id was empty. + Empty, + /// The id exceeded [`MAX_TENANT_ID_LEN`] bytes. + TooLong { + /// The offending length, in bytes. + len: usize, + }, + /// The id contained a character outside the permitted set. + InvalidCharacter { + /// The first offending character. + found: char, + }, + /// The id had a leading `/`, a trailing `/`, or an empty segment (`a//b`). + EmptySegment, + /// A segment named a reserved control-plane namespace. + ReservedSegment { + /// The offending segment. + segment: String, + }, +} + +impl fmt::Display for TenantIdError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Empty => write!(f, "tenant id must not be empty"), + Self::TooLong { len } => write!( + f, + "tenant id is {len} bytes; the maximum is {MAX_TENANT_ID_LEN}" + ), + Self::InvalidCharacter { found } => write!( + f, + "tenant id contains {found:?}; only letters, digits, '-', '_', '.', \ + and '/' (the hierarchy separator) are permitted" + ), + Self::EmptySegment => write!( + f, + "tenant id must not begin or end with '/' or contain an empty segment" + ), + Self::ReservedSegment { segment } => write!( + f, + "tenant id segment '{segment}' is reserved for internal use" + ), + } + } +} + +impl std::error::Error for TenantIdError {} + /// An opaque tenant identifier with hierarchical namespace support. /// /// `TenantId` supports hierarchical organization using a `/` separator, @@ -44,7 +140,23 @@ pub const SYSTEM_TENANT: &str = "__system__"; pub struct TenantId(String); impl TenantId { - /// Creates a new tenant ID from the given string. + /// Creates a tenant ID **without validating it**. + /// + /// This is the constructor for *trusted reconstruction*: rebuilding a + /// `TenantId` from a value that was already validated when it entered the + /// system — a `tenant_id` column read back out of a row, a value round-tripped + /// through a `TenantContext`, a literal in a test. It stores the string + /// verbatim and cannot fail. + /// + /// Use [`parse`](Self::parse) for anything derived from a request: an + /// `X-Tenant-ID` header, a URL path prefix, a JWT claim, an admin-API body. + /// Those are the paths that must not admit an id the storage backends cannot + /// keep distinct. + /// + /// It stays infallible deliberately. Making it fallible would force an + /// `unwrap()` at every trusted reconstruction site, which converts a + /// data-integrity problem into a panic without catching anything new — the + /// value is already in the database by then. /// /// # Arguments /// @@ -62,11 +174,109 @@ impl TenantId { Self(id.into()) } + /// Parses and validates a tenant ID — the canonical constructor for any id + /// that arrives from outside the server. + /// + /// # The charset + /// + /// A valid tenant id is 1..=[`MAX_TENANT_ID_LEN`] bytes drawn from ASCII + /// letters, ASCII digits, `-`, `_`, `.`, and `/`. `/` separates hierarchy + /// levels (see [`parent`](Self::parent)), so it may not lead, trail, or + /// repeat, and no segment may name a [reserved namespace](RESERVED_TENANT_SEGMENTS). + /// + /// Case is **preserved**, not folded: `ACME` and `acme` are two different + /// tenants and every backend keeps them apart. Folding would be a silent + /// data-visibility change for any deployment already using a mixed-case id. + /// + /// # Why this exists + /// + /// Before this, four surfaces validated tenant ids and all four disagreed — + /// the header and URL extractors on one charset, the admin API on a wider + /// one, the JWT claim on nothing at all — and `new` imposed no constraint + /// beneath them. Backends were left to defend individually, which is how the + /// Elasticsearch case-collision (issue #384) became reachable. One + /// definition here lets a backend state a precondition instead of deriving + /// its own (issue #385). + /// + /// What this does *not* cover, deliberately: whether an id shadows a FHIR + /// resource type or a reserved route (`metadata`, `console`, …). That is a + /// property of the REST routing surface, not of storage, and depends on the + /// configured FHIR version — so it is layered on top in `helios-rest`, not + /// duplicated here. + /// + /// # Examples + /// + /// ``` + /// use helios_persistence::tenant::{TenantId, TenantIdError}; + /// + /// assert_eq!(TenantId::parse("acme").unwrap().as_str(), "acme"); + /// assert_eq!(TenantId::parse("acme/research").unwrap().depth(), 1); + /// // Case is preserved, so these are distinct tenants. + /// assert_ne!(TenantId::parse("ACME").unwrap(), TenantId::parse("acme").unwrap()); + /// + /// assert!(matches!(TenantId::parse(""), Err(TenantIdError::Empty))); + /// assert!(matches!( + /// TenantId::parse("acme corp"), + /// Err(TenantIdError::InvalidCharacter { found: ' ' }) + /// )); + /// // Would otherwise land inside the prefix tenant `acme` scans and purges. + /// assert!(matches!( + /// TenantId::parse("acme/resources"), + /// Err(TenantIdError::ReservedSegment { .. }) + /// )); + /// ``` + pub fn parse(id: &str) -> Result { + if id.is_empty() { + return Err(TenantIdError::Empty); + } + // Bytes, not chars: the length that matters is what reaches a storage + // key. The charset check below rejects every non-ASCII byte anyway, so + // the two measures coincide for accepted ids — this only makes the + // rejection of an over-long non-ASCII id report a truthful length. + if id.len() > MAX_TENANT_ID_LEN { + return Err(TenantIdError::TooLong { len: id.len() }); + } + if let Some(found) = id + .chars() + .find(|c| !(c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.' | '/'))) + { + return Err(TenantIdError::InvalidCharacter { found }); + } + // `split('/')` yields an empty piece for a leading `/`, a trailing `/`, + // and each `//`, so one emptiness check covers all three shapes. + for segment in id.split('/') { + if segment.is_empty() { + return Err(TenantIdError::EmptySegment); + } + if RESERVED_TENANT_SEGMENTS.contains(&segment) { + return Err(TenantIdError::ReservedSegment { + segment: segment.to_string(), + }); + } + } + Ok(Self(id.to_string())) + } + + /// Returns `true` if this id satisfies [`parse`](Self::parse)'s rules. + /// + /// For auditing ids that entered through [`new`](Self::new) — a value read + /// back from storage that predates validation, say. Prefer `parse` when you + /// are accepting an id, so the caller gets a reason rather than a bool. + pub fn is_canonical(&self) -> bool { + Self::parse(&self.0).is_ok() + } + /// Returns the system tenant ID. /// /// The system tenant is used for shared resources that should be /// accessible across all tenants. /// + /// This is trusted internal construction, so it bypasses + /// [`parse`](Self::parse) — which rejects `__system__` precisely so that no + /// client-supplied id can ever name it. [`is_canonical`](Self::is_canonical) + /// is therefore `false` for the system tenant, and that is correct: it means + /// "not a value a client may assert", not "malformed". + /// /// # Examples /// /// ``` @@ -220,20 +430,29 @@ impl fmt::Debug for TenantId { } } +/// Validating. `str::parse` is where a Rust reader expects a fallible, +/// checked conversion, so it routes through [`TenantId::parse`]. +/// +/// The `From` impls below are deliberately *not* validating — they mirror +/// [`TenantId::new`], the unchecked trusted-reconstruction constructor, because +/// an infallible `From` has nowhere to report a rejection. The asymmetry is the +/// point: `"acme".parse::()` checks, `TenantId::from("acme")` does not. impl FromStr for TenantId { - type Err = std::convert::Infallible; + type Err = TenantIdError; fn from_str(s: &str) -> Result { - Ok(TenantId::new(s)) + TenantId::parse(s) } } +/// Unchecked — see the note on [`FromStr`]'s impl above. impl From<&str> for TenantId { fn from(s: &str) -> Self { TenantId::new(s) } } +/// Unchecked — see the note on [`FromStr`]'s impl above. impl From for TenantId { fn from(s: String) -> Self { TenantId::new(s) @@ -365,4 +584,206 @@ mod tests { let tenant2: TenantId = String::from("my-tenant").into(); assert_eq!(tenant2.as_str(), "my-tenant"); } + + // ── The canonical validator (issue #385) ──────────────────────────────── + + #[test] + fn parse_accepts_every_shape_the_routing_surfaces_already_accepted() { + // The union of what `resolver.rs`, `tenant_prefix.rs`, and the admin API + // accepted before this validator existed. Anything here that started + // failing would orphan a deployment's data, since no rename exists. + for id in [ + "acme", + "tenant-123", + "my_tenant", + "ABC123", + "tenant.example", + "acme/research", + "acme/research/oncology", + "a", + ] { + assert!( + TenantId::parse(id).is_ok(), + "{id:?} was accepted before this validator and must stay valid" + ); + } + // Exactly at the cap, which both routing validators already enforced. + let at_cap = "a".repeat(MAX_TENANT_ID_LEN); + assert!(TenantId::parse(&at_cap).is_ok()); + } + + #[test] + fn parse_preserves_case_so_mixed_case_tenants_stay_distinct() { + // Folding case would silently merge two tenants that every backend + // currently keeps apart — a data-visibility change, not a fix. The + // Elasticsearch collision (#384) is closed by making *its* derivation + // injective, not by narrowing the charset here. + let upper = TenantId::parse("ACME").expect("uppercase is valid"); + let lower = TenantId::parse("acme").expect("lowercase is valid"); + assert_eq!(upper.as_str(), "ACME"); + assert_ne!(upper, lower); + } + + #[test] + fn parse_rejects_empty_and_over_long() { + assert_eq!(TenantId::parse(""), Err(TenantIdError::Empty)); + + let long = "a".repeat(MAX_TENANT_ID_LEN + 1); + assert_eq!( + TenantId::parse(&long), + Err(TenantIdError::TooLong { + len: MAX_TENANT_ID_LEN + 1 + }) + ); + } + + #[test] + fn parse_rejects_characters_outside_the_charset() { + // Whitespace and the shapes that break a storage key or a URL. `%` and + // `+` matter specifically: both are escape introducers in a backend + // keyspace encoding, so letting one through as a literal would make the + // escape forgeable. + for (id, found) in [ + ("acme corp", ' '), + ("acme\tcorp", '\t'), + ("acme:corp", ':'), + ("acme%2Fcorp", '%'), + ("acme+corp", '+'), + ("acme\\corp", '\\'), + ("acme?x", '?'), + ("acme#x", '#'), + ("acme*", '*'), + ("acmé", 'é'), + ] { + assert_eq!( + TenantId::parse(id), + Err(TenantIdError::InvalidCharacter { found }), + "{id:?} must be rejected" + ); + } + } + + #[test] + fn parse_rejects_malformed_hierarchy() { + for id in ["/acme", "acme/", "acme//research", "/", "//"] { + assert_eq!( + TenantId::parse(id), + Err(TenantIdError::EmptySegment), + "{id:?} must be rejected" + ); + } + } + + /// Guards the S3 cross-tenant *destructive* collision that motivated the + /// per-segment check. + /// + /// On S3 a tenant's data lives under `{tenant}/resources/…` and + /// `{tenant}/history/…`. Tenant `acme/resources` therefore stores at + /// `acme/resources/resources/…`, which is inside the `acme/resources/` + /// prefix that tenant `acme` lists — and that + /// `S3Backend::purge_tenant_data("acme")` deletes. The admin API's old + /// whole-id reserved list did not catch this because the id is not equal to + /// a reserved name, it merely contains one as a segment. + #[test] + fn parse_rejects_reserved_segments_in_any_position() { + for id in [ + "resources", + "acme/resources", + "acme/history/x", + "tenants", + "acme/bulk", + "__system__", + "acme/__system__", + "..", + "acme/../evil", + ".", + "acme/./x", + ] { + assert!( + matches!( + TenantId::parse(id), + Err(TenantIdError::ReservedSegment { .. }) + ), + "{id:?} must be rejected as reserved, got {:?}", + TenantId::parse(id) + ); + } + + // A reserved word is only reserved as a whole segment — it must stay + // usable as a substring, or we would reject ordinary names. + assert!(TenantId::parse("resources-team").is_ok()); + assert!(TenantId::parse("acme/resources-archive").is_ok()); + assert!(TenantId::parse("prehistory").is_ok()); + } + + #[test] + fn parse_is_injective_over_accepted_ids() { + // `parse` stores the input verbatim — it normalises nothing. That is + // what lets a backend treat the validated id as the identity it keys on. + for id in [ + "acme", + "ACME", + "AcMe", + "acme.corp", + "acme_corp", + "acme-corp", + ] { + assert_eq!(TenantId::parse(id).unwrap().as_str(), id); + } + } + + #[test] + fn system_tenant_is_trusted_construction_not_a_parsable_id() { + // `system()` bypasses `parse` by design; `parse` rejects the sentinel so + // no client-supplied id can name it. + let system = TenantId::system(); + assert!(system.is_system()); + assert!( + !system.is_canonical(), + "the sentinel is reachable only through trusted construction" + ); + assert!(TenantId::parse(SYSTEM_TENANT).is_err()); + } + + #[test] + fn from_str_validates_but_from_does_not() { + // The deliberate asymmetry documented on the impls. + assert!("acme corp".parse::().is_err()); + assert!("acme".parse::().is_ok()); + // `From` mirrors the unchecked `new`, so it still accepts anything. + assert_eq!(TenantId::from("acme corp").as_str(), "acme corp"); + } + + #[test] + fn is_canonical_flags_legacy_ids_reconstructed_through_new() { + // `new` is unchecked, so a value that predates the validator round-trips + // out of storage unchanged; `is_canonical` is how an operator finds it. + assert!(TenantId::new("acme").is_canonical()); + assert!(!TenantId::new("acme corp").is_canonical()); + assert!(!TenantId::new("a".repeat(MAX_TENANT_ID_LEN + 1)).is_canonical()); + } + + #[test] + fn error_messages_name_the_reason() { + // These strings reach the client in a 400/403 body, so they have to be + // actionable rather than a bare "invalid". + assert!( + TenantId::parse("acme corp") + .unwrap_err() + .to_string() + .contains('\'') + ); + assert!( + TenantId::parse(&"a".repeat(100)) + .unwrap_err() + .to_string() + .contains("100") + ); + assert!( + TenantId::parse("acme/resources") + .unwrap_err() + .to_string() + .contains("resources") + ); + } } diff --git a/crates/persistence/src/tenant/mod.rs b/crates/persistence/src/tenant/mod.rs index 591ef5fd6..b1ba2e44e 100644 --- a/crates/persistence/src/tenant/mod.rs +++ b/crates/persistence/src/tenant/mod.rs @@ -35,6 +35,33 @@ //! resource *type* is tenant-scoped or shared across tenants, not where a //! tenant's records physically live. //! +//! # The tenant-id contract +//! +//! [`TenantId::parse`] is the single canonical validator. Every id that enters +//! the server from outside — `X-Tenant-ID`, a URL path prefix, a JWT tenant +//! claim, an admin-API provisioning body — goes through it, and +//! [`ResourceStorage::register_tenant`](crate::core::ResourceStorage::register_tenant) +//! re-checks it as a backstop so no future ingress can mint an id that skipped +//! validation. +//! +//! **What a backend may rely on.** A tenant id reaching storage is 1..=64 bytes +//! of ASCII letters, digits, `-`, `_`, `.`, and `/`; `/` neither leads, trails, +//! nor repeats; and no segment is a reserved control-plane namespace (see +//! [`RESERVED_TENANT_SEGMENTS`]). Case is significant. +//! +//! **What a backend may *not* assume.** That the id is safe to fold, truncate, +//! or otherwise map lossily into its own keyspace. The contract guarantees a +//! bounded, printable charset — it does not guarantee that the charset is legal +//! everywhere. A backend whose keyspace is narrower than this (Elasticsearch +//! index names must be lowercase; S3 prefixes give `/` structural meaning) still +//! owes an **injective** encoding of its own. The precondition exists so that +//! encoding has a bounded input to handle, not so it can be skipped: two +//! distinct ids must always address distinct storage. +//! +//! Ids stored before this validator existed are unaffected — they round-trip +//! through the unchecked [`TenantId::new`]. [`TenantId::is_canonical`] identifies +//! them. +//! //! # Examples //! //! ## Creating a Tenant Context @@ -90,7 +117,7 @@ mod permissions; mod tenancy; pub use context::{TenantContext, TenantContextBuilder}; -pub use id::{SYSTEM_TENANT, TenantId}; +pub use id::{MAX_TENANT_ID_LEN, RESERVED_TENANT_SEGMENTS, SYSTEM_TENANT, TenantId, TenantIdError}; pub use permissions::{ CompartmentRestriction, Operation, TenantPermissions, TenantPermissionsBuilder, }; diff --git a/crates/rest/src/error.rs b/crates/rest/src/error.rs index 66d3804f6..fc64448c1 100644 --- a/crates/rest/src/error.rs +++ b/crates/rest/src/error.rs @@ -677,6 +677,11 @@ impl From for RestError { TenantError::InvalidTenant { .. } => RestError::BadRequest { message: err.to_string(), }, + // The caller supplied an id that is not a valid tenant id — a + // malformed request, not a permissions problem. + TenantError::NonCanonicalTenantId { .. } => RestError::BadRequest { + message: err.to_string(), + }, TenantError::TenantSuspended { .. } => RestError::Forbidden { message: err.to_string(), }, diff --git a/crates/rest/src/extractors/tenant.rs b/crates/rest/src/extractors/tenant.rs index 0d9e76336..9b5b3241f 100644 --- a/crates/rest/src/extractors/tenant.rs +++ b/crates/rest/src/extractors/tenant.rs @@ -25,7 +25,28 @@ use axum::{ use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; use crate::state::AppState; -use crate::tenant::{ResolvedTenant, TenantResolver, TenantSource, TenantValidator}; +use crate::tenant::{ + ResolvedTenant, TenantResolver, TenantSource, TenantSourceError, TenantValidator, +}; + +/// Maps a rejected tenant assertion onto a status code by *who* asserted it. +/// +/// A malformed `X-Tenant-ID` or URL prefix is the client's mistake, so `400`. A +/// malformed JWT claim is not — the request is well-formed and the token is +/// authenticated; it is the authorization context that cannot be used, so `403`, +/// matching the claimless-token path in [`TenantExtractor::resolve`]. +fn source_error_to_rejection(err: TenantSourceError) -> (StatusCode, String) { + match err.source { + TenantSource::JwtClaim => ( + StatusCode::FORBIDDEN, + format!("Authenticated token carries an invalid tenant claim: {err}"), + ), + _ => ( + StatusCode::BAD_REQUEST, + format!("Invalid tenant id from {}: {err}", err.source), + ), + } +} /// Axum extractor for tenant context. /// @@ -162,12 +183,29 @@ impl TenantExtractor { // confusion between URL context and actual data access. if let Some(principal) = parts.extensions.get::() { if let Some(jwt_tenant) = principal.tenant_id().filter(|t| !t.is_empty()) { + // The claim is authoritative for every request below, so it is + // validated *first* — before any consistency check and before it + // can reach a storage key. It used to be validated by nothing at + // all (issue #385), making it the one ingress that could put an + // arbitrary string into a backend keyspace. + // + // 403 rather than 400: the token is well-formed and + // authenticated, so nothing about the *request* is malformed — + // it is the authorization context that is unusable. This matches + // the claimless-token case below. + let jwt_tenant = TenantId::parse(jwt_tenant).map_err(|e| { + ( + StatusCode::FORBIDDEN, + format!("Authenticated token carries an invalid tenant claim: {e}"), + ) + })?; if config.multitenancy.strict_validation { let resolver = TenantResolver::new(&config.multitenancy); - let resolved = - resolver.resolve(parts, &config.multitenancy, &config.default_tenant); + let resolved = resolver + .resolve(parts, &config.multitenancy, &config.default_tenant) + .map_err(source_error_to_rejection)?; // If a non-default source provided a tenant, it must match the JWT - if !resolved.is_default() && resolved.tenant_id_str() != jwt_tenant { + if !resolved.is_default() && resolved.tenant_id_str() != jwt_tenant.as_str() { return Err(( StatusCode::BAD_REQUEST, format!( @@ -179,7 +217,7 @@ impl TenantExtractor { } } return Ok(TenantExtractor::new( - jwt_tenant, + jwt_tenant.as_str(), crate::tenant::TenantSource::JwtClaim, )); } @@ -197,7 +235,9 @@ impl TenantExtractor { // If nothing (or only the default) was requested, fall back to the // configured default so single-tenant-with-auth deployments keep working. let resolver = TenantResolver::new(&config.multitenancy); - let resolved = resolver.resolve(parts, &config.multitenancy, &config.default_tenant); + let resolved = resolver + .resolve(parts, &config.multitenancy, &config.default_tenant) + .map_err(source_error_to_rejection)?; // An empty resolved tenant means nothing real was requested — e.g. an // empty JWT tenant claim re-surfaced by the resolver's JwtTenantExtractor // (which runs even in HeaderOnly mode), with no header/URL tenant @@ -227,7 +267,9 @@ impl TenantExtractor { let resolver = TenantResolver::new(&config.multitenancy); // Resolve tenant from request - let resolved = resolver.resolve(parts, &config.multitenancy, &config.default_tenant); + let resolved = resolver + .resolve(parts, &config.multitenancy, &config.default_tenant) + .map_err(source_error_to_rejection)?; // Validate consistency if strict mode is enabled if config.multitenancy.strict_validation { @@ -328,6 +370,7 @@ mod tests { use axum::http::{HeaderValue, Request, StatusCode}; use helios_auth::{Principal, ScopeSet}; use helios_persistence::backends::sqlite::SqliteBackend; + use helios_persistence::tenant::MAX_TENANT_ID_LEN; use super::super::*; use crate::config::{MultitenancyConfig, ServerConfig, TenantRoutingMode}; @@ -532,5 +575,86 @@ mod tests { .expect_err("empty claim + non-default header should be forbidden"); assert_eq!(err.0, StatusCode::FORBIDDEN); } + + // ── Canonical tenant-id validation at the ingresses (issue #385) ──── + + /// The JWT claim is authoritative and used to be validated by nothing, + /// making it the one ingress that could put an arbitrary string into a + /// storage key. + /// + /// `403` rather than `400`: the request is well-formed and the token is + /// authenticated — it is the authorization context that is unusable, + /// matching the claimless-token path above. + #[tokio::test] + async fn invalid_jwt_tenant_claim_is_forbidden() { + let state = make_state(false, "test-tenant"); + let too_long = "a".repeat(MAX_TENANT_ID_LEN + 1); + + for bad in [ + "acme corp", // whitespace + "acme/resources", // reserved segment — the S3 purge collision + "__system__", // the shared-tenant sentinel + "acme%2Fresearch", // escape introducer + "../etc", // relative-path segment + "acme/", // malformed hierarchy + too_long.as_str(), // over the canonical length cap + ] { + let mut parts = make_parts(None, Some(make_principal(Some(bad)))); + let err = TenantExtractor::from_request_parts(&mut parts, &state) + .await + .err() + .unwrap_or_else(|| panic!("{bad:?} must be rejected, but resolved")); + assert_eq!( + err.0, + StatusCode::FORBIDDEN, + "{bad:?} should be 403, got {err:?}" + ); + } + } + + /// Regression for the silent cross-tenant fallback. + /// + /// With auth disabled, a malformed `X-Tenant-ID` used to be filtered to + /// `None`, so the resolver fell through to the **default tenant** and the + /// request was served against it — a `200` reading and writing someone + /// else's data because of a typo. + #[tokio::test] + async fn invalid_header_is_bad_request_not_the_default_tenant() { + let state = make_state(false, "test-tenant"); + let too_long = "a".repeat(MAX_TENANT_ID_LEN + 1); + + for bad in [ + "acme corp", + "acme/resources", + "__system__", + too_long.as_str(), + ] { + let mut parts = make_parts(Some(bad), None); + let err = TenantExtractor::from_request_parts(&mut parts, &state) + .await + .unwrap_err(); + assert_eq!( + err.0, + StatusCode::BAD_REQUEST, + "{bad:?} should be 400, got {err:?}" + ); + } + } + + /// A valid hierarchical id resolves through the header, and a dotted id + /// — provisionable by the admin API but previously unroutable — now works. + #[tokio::test] + async fn canonical_ids_the_old_header_charset_rejected_now_resolve() { + let state = make_state(false, "test-tenant"); + + for good in ["acme/research", "tenant.example"] { + let mut parts = make_parts(Some(good), None); + let extractor = TenantExtractor::from_request_parts(&mut parts, &state) + .await + .unwrap_or_else(|e| panic!("{good:?} should resolve, got {e:?}")); + assert_eq!(extractor.tenant_id(), good); + assert_eq!(extractor.source(), TenantSource::Header); + } + } } } diff --git a/crates/rest/src/handlers/admin_tenants.rs b/crates/rest/src/handlers/admin_tenants.rs index 4e805b2c0..3b68a786e 100644 --- a/crates/rest/src/handlers/admin_tenants.rs +++ b/crates/rest/src/handlers/admin_tenants.rs @@ -28,7 +28,7 @@ use axum::{ }; use helios_audit::{AuditAction, AuditEventBuilder}; use helios_persistence::core::ResourceStorage; -use helios_persistence::tenant::SYSTEM_TENANT; +use helios_persistence::tenant::{SYSTEM_TENANT, TenantId}; use serde::Deserialize; use serde_json::json; use tracing::debug; @@ -36,15 +36,9 @@ use tracing::debug; use crate::error::{RestError, RestResult}; use crate::state::AppState; -/// Maximum accepted tenant-id length. Generous, but bounds the value that flows -/// into schema names / storage keys. -const MAX_TENANT_ID_LEN: usize = 128; - -/// Tenant ids that name a control-plane namespace in a storage backend's -/// keyspace. Currently the S3 sibling prefixes of `tenants/` (see -/// `S3Keyspace`); `__system__` is checked separately as the shared-tenant -/// sentinel. -const RESERVED_TENANT_IDS: &[&str] = &["tenants", "resources", "history", "bulk"]; +// The tenant-id length cap and reserved-name list that used to live here are +// now `helios_persistence::tenant::{MAX_TENANT_ID_LEN, RESERVED_TENANT_SEGMENTS}`, +// applied by `TenantId::parse` — see `validate_tenant_id` below (issue #385). /// Request body for `POST /admin/tenants`. #[derive(Debug, Deserialize)] @@ -93,51 +87,31 @@ async fn seed_new_tenant( .await; } -/// Validates a tenant id: non-empty, within length, no whitespace, a -/// conservative identifier charset, and never the internal system sentinel. +/// Validates a tenant id against the canonical definition. +/// +/// This used to be a third, independent charset — wider than the two the REST +/// routing surfaces enforced (it permitted `.` and `/`) and with a different +/// length cap. Provisioning a tenant it accepted but routing did not produced a +/// registered tenant nobody could address. Validation now lives in one place +/// ([`TenantId::parse`], issue #385) and this handler only translates the +/// rejection into a `400`. +/// +/// Two rules the old local version had are now *stronger*, not merely moved: +/// +/// - The reserved-name check compares each `/`-separated **segment**, not the +/// whole id. `acme/resources` used to pass, and on S3 its objects land inside +/// the `acme/resources/` prefix that tenant `acme` lists and +/// `purge_tenant_data("acme")` deletes. +/// - The length cap is 64, not 128. An id in 65..=128 bytes was already +/// unroutable through both header and URL routing (each capped at 64), so it +/// was reachable only through the JWT claim — the ingress that validated +/// nothing. fn validate_tenant_id(id: &str) -> RestResult<()> { - if id.is_empty() { - return Err(RestError::BadRequest { - message: "tenant id must not be empty".to_string(), - }); - } - if id.len() > MAX_TENANT_ID_LEN { - return Err(RestError::BadRequest { - message: format!("tenant id exceeds {MAX_TENANT_ID_LEN} characters"), - }); - } - if id == SYSTEM_TENANT { - return Err(RestError::BadRequest { - message: "'__system__' is reserved for internal shared resources".to_string(), - }); - } - // Names that collide with a control-plane namespace in the S3 keyspace. - // - // Defense in depth and a UX guardrail only — it is *not* what makes the - // keyspace safe. The S3 backend is safe structurally (registry records are - // direct `{id}.json` children of `tenants/`, tenant data is always nested - // deeper), because this check is reachable from the admin API alone: the JWT - // tenant extractor validates nothing. See `S3Keyspace::tenant_registry_prefix` - // and issue #271 — do not conclude from this list that the reader-side filter - // is redundant. - if RESERVED_TENANT_IDS.contains(&id) { - return Err(RestError::BadRequest { - message: format!("'{id}' is reserved for internal use"), - }); - } - // Allow the characters that are safe across routing (header / URL prefix / - // JWT claim) and storage keys: alphanumerics plus `-`, `_`, `.`, and `/` - // (the hierarchy separator). Reject everything else, including whitespace. - if !id - .chars() - .all(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.' | '/')) - { - return Err(RestError::BadRequest { - message: "tenant id may contain only letters, digits, '-', '_', '.', and '/'" - .to_string(), - }); - } - Ok(()) + TenantId::parse(id) + .map(|_| ()) + .map_err(|e| RestError::BadRequest { + message: e.to_string(), + }) } /// `GET /admin/tenants` diff --git a/crates/rest/src/middleware/tenant_prefix.rs b/crates/rest/src/middleware/tenant_prefix.rs index ef1022f41..7bb7bb217 100644 --- a/crates/rest/src/middleware/tenant_prefix.rs +++ b/crates/rest/src/middleware/tenant_prefix.rs @@ -5,6 +5,7 @@ use axum::{extract::Request, http::Uri, middleware::Next, response::Response}; use helios_fhir::{FhirResourceTypeProvider, FhirVersion}; +use helios_persistence::tenant::TenantId; /// Non-resource reserved paths (FHIR system endpoints, API prefixes). /// Resource types are checked dynamically via helios-fhir's FhirResourceTypeProvider. @@ -23,6 +24,12 @@ const RESERVED_SYSTEM_PATHS: &[&str] = &[ // can never be named `console` under URL-path routing, keeping the console // paths unambiguous with the authz console guard (see `middleware::auth`). "console", + // SMART discovery (`/.well-known/smart-configuration`). Reserved since issue + // #385: the canonical charset permits `.`, which the old private validator + // here rejected, so `.well-known` would otherwise parse as a tenant and this + // middleware would rewrite the path to `/smart-configuration` — a 404 for + // SMART discovery under `url_path`/`both` routing. + ".well-known", ]; /// Checks if a path segment is a reserved path (not a tenant identifier). @@ -59,12 +66,20 @@ fn is_fhir_resource_type(type_name: &str, fhir_version: &FhirVersion) -> bool { } } -/// Validates that a string could be a tenant ID. +/// Validates that a path segment could be a tenant ID. +/// +/// Delegates to [`TenantId::parse`], the canonical validator (issue #385). This +/// used to be a private copy of the charset, one of three that disagreed; the +/// copy in `tenant::resolver` is gone for the same reason. Keeping the two in +/// agreement is load-bearing: `authz_middleware` calls +/// [`extract_tenant_from_path`] to decide which path it is authorizing, so if +/// this accepted a segment the resolver rejected, authorization and data access +/// would disagree about which tenant the request is for. +/// +/// A URL prefix is a single path segment, so a hierarchical id never reaches +/// here — `parse` will simply see the first segment. fn is_valid_tenant_id(s: &str) -> bool { - !s.is_empty() - && s.len() <= 64 - && s.chars() - .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') + TenantId::parse(s).is_ok() } /// Extracts tenant from URL path if present. @@ -218,6 +233,27 @@ mod tests { assert!(extract_tenant_from_path("/console/metrics/uptime", &version).is_none()); } + /// Regression for a break this change would otherwise have introduced. + /// + /// The canonical charset (issue #385) permits `.`, which this module's old + /// private validator rejected. Without reserving `.well-known`, + /// `/.well-known/smart-configuration` would parse `.well-known` as a tenant + /// and rewrite the path to `/smart-configuration`, which matches no route — + /// turning SMART discovery into a 404 under `url_path`/`both` routing. + #[test] + fn smart_discovery_is_not_parsed_as_a_tenant_prefix() { + let version = default_version(); + assert!(extract_tenant_from_path("/.well-known/smart-configuration", &version).is_none()); + assert!(is_reserved_path(".well-known", &version)); + + // A dotted id that is *not* a reserved route still routes as a tenant — + // the point of widening the charset. + let (tenant, remaining) = + extract_tenant_from_path("/tenant.example/Patient", &version).unwrap(); + assert_eq!(tenant, "tenant.example"); + assert_eq!(remaining, "/Patient"); + } + #[test] fn test_is_reserved_path() { let version = default_version(); @@ -253,10 +289,42 @@ mod tests { assert!(is_valid_tenant_id("tenant-123")); assert!(is_valid_tenant_id("my_tenant")); assert!(is_valid_tenant_id("ABC123")); + // `.` is now accepted: the canonical charset is the union of what the + // three old validators accepted, and the admin API always allowed it. + // A tenant provisioned as `tenant.example` was previously unroutable. + assert!(is_valid_tenant_id("tenant.example")); assert!(!is_valid_tenant_id("")); // empty - assert!(!is_valid_tenant_id("tenant.com")); // dot - assert!(!is_valid_tenant_id("tenant/path")); // slash + assert!(!is_valid_tenant_id("tenant/path")); // never one URL segment assert!(!is_valid_tenant_id(&"a".repeat(100))); // too long + assert!(!is_valid_tenant_id("tenant corp")); // whitespace + // Reserved control-plane segments are not tenants (issue #385). + assert!(!is_valid_tenant_id("resources")); + assert!(!is_valid_tenant_id("__system__")); + } + + /// `authz_middleware` and the tenant resolver must classify a path segment + /// identically, or the request would be authorized as one tenant and served + /// as another. Both now call `TenantId::parse`; this pins that they agree. + #[test] + fn tenant_prefix_and_canonical_validator_agree() { + for candidate in [ + "acme", + "ACME", + "tenant.example", + "my_tenant", + "tenant-1", + "", + "resources", + "__system__", + "tenant corp", + "tenant%2F", + ] { + assert_eq!( + is_valid_tenant_id(candidate), + TenantId::parse(candidate).is_ok(), + "{candidate:?} classified differently from the canonical validator" + ); + } } #[test] diff --git a/crates/rest/src/tenant/mod.rs b/crates/rest/src/tenant/mod.rs index 732c00541..82c6b993d 100644 --- a/crates/rest/src/tenant/mod.rs +++ b/crates/rest/src/tenant/mod.rs @@ -40,8 +40,10 @@ //! let config = MultitenancyConfig::default(); //! let resolver = TenantResolver::new(&config); //! -//! // In an Axum handler: -//! let resolved = resolver.resolve(&parts, &config, "default"); +//! // In an Axum handler. `resolve` fails when a source *asserted* a tenant id +//! // that is not canonical (see `helios_persistence::tenant::TenantId::parse`); +//! // a request that asserts nothing resolves to the default tenant. +//! let resolved = resolver.resolve(&parts, &config, "default")?; //! println!("Tenant: {} (from {})", resolved.tenant_id_str(), resolved.source); //! ``` @@ -50,7 +52,7 @@ mod source; mod validation; pub use resolver::{ - HeaderTenantExtractor, JwtTenantExtractor, ResolvedTenant, TenantResolver, + HeaderTenantExtractor, JwtTenantExtractor, ResolvedTenant, TenantResolver, TenantSourceError, TenantSourceExtractor, UrlPathTenantExtractor, }; pub use source::TenantSource; diff --git a/crates/rest/src/tenant/resolver.rs b/crates/rest/src/tenant/resolver.rs index c1359b4ae..9c5660d60 100644 --- a/crates/rest/src/tenant/resolver.rs +++ b/crates/rest/src/tenant/resolver.rs @@ -5,7 +5,8 @@ use axum::http::request::Parts; use helios_fhir::{FhirResourceTypeProvider, FhirVersion}; -use helios_persistence::tenant::TenantId; +use helios_persistence::tenant::{MAX_TENANT_ID_LEN, TenantId, TenantIdError}; +use tracing::{debug, warn}; use crate::config::{MultitenancyConfig, TenantRoutingMode}; use crate::middleware::tenant::X_TENANT_ID; @@ -31,6 +32,10 @@ const RESERVED_SYSTEM_PATHS: &[&str] = &[ // named `console` and strict-validation never mis-resolves the console path // to a `console` tenant under URL-path routing. "console", + // SMART discovery. Same rationale as `console`, and newly load-bearing since + // issue #385 widened the tenant charset to include `.` — see the twin list + // in `middleware::tenant_prefix`. + ".well-known", ]; /// Result of resolving a tenant from a request. @@ -64,12 +69,54 @@ impl ResolvedTenant { /// Trait for extracting tenant information from a specific source. pub trait TenantSourceExtractor: Send + Sync { /// Attempts to extract a tenant ID from the request. - fn extract(&self, parts: &Parts, config: &MultitenancyConfig) -> Option; + /// + /// The three-way return is the whole point, and the distinction is easy to + /// collapse by accident: + /// + /// - `Ok(Some(id))` — this source asserted a tenant and it is valid. + /// - `Ok(None)` — this source asserted **nothing**. Fall through to the next + /// source, and ultimately to the default tenant. + /// - `Err(_)` — this source asserted a tenant and it is **invalid**. The + /// request must be rejected, not silently redirected somewhere else. + /// + /// Before issue #385 there was no `Err` arm: an invalid `X-Tenant-ID` was + /// filtered to `None`, which fell through to the default tenant, so a client + /// typo silently read and wrote *another tenant's* data under a `200`. + /// + /// [`UrlPathTenantExtractor`] is the deliberate exception — for it, "not a + /// valid tenant id" genuinely means "not a tenant prefix" (`/Patient/123`), + /// so it returns `Ok(None)` and never `Err`. + fn extract( + &self, + parts: &Parts, + config: &MultitenancyConfig, + ) -> Result, TenantSourceError>; /// Returns the source type this extractor handles. fn source_type(&self) -> TenantSource; } +/// A tenant id asserted by a request source that failed canonical validation. +/// +/// Carries the source so the caller can pick the right status: a bad header is +/// the client's fault (`400`), a bad JWT claim is an unusable authorization +/// context (`403`). +#[derive(Debug, Clone)] +pub struct TenantSourceError { + /// Which source carried the invalid id. + pub source: TenantSource, + /// Why it was rejected. + pub error: TenantIdError, +} + +impl std::fmt::Display for TenantSourceError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.error) + } +} + +impl std::error::Error for TenantSourceError {} + /// Extracts tenant from URL path prefix. /// /// First checks for a tenant extracted by the middleware (stored in extensions). @@ -78,12 +125,22 @@ pub trait TenantSourceExtractor: Send + Sync { pub struct UrlPathTenantExtractor; impl TenantSourceExtractor for UrlPathTenantExtractor { - fn extract(&self, parts: &Parts, _config: &MultitenancyConfig) -> Option { - // First, check if middleware already extracted the tenant + /// Never returns `Err`: a first path segment that is not a valid tenant id + /// is not a *malformed tenant*, it is an ordinary untenanted path + /// (`/Patient/123`). Turning that into an error would 400 every non-tenanted + /// request. See [`TenantSourceExtractor::extract`]. + fn extract( + &self, + parts: &Parts, + _config: &MultitenancyConfig, + ) -> Result, TenantSourceError> { + // First, check if middleware already extracted the tenant. The + // middleware applies the same canonical check before storing it, so this + // is trusted reconstruction. if let Some(ExtractedTenantFromUrl(tenant)) = parts.extensions.get::() { - return Some(TenantId::new(tenant)); + return Ok(Some(TenantId::new(tenant))); } // Fall back to checking the original path (if stored) or current path @@ -97,21 +154,22 @@ impl TenantSourceExtractor for UrlPathTenantExtractor { let path = path.strip_prefix('/').unwrap_or(path); // Get the first segment (before the next slash or end of path) - let tenant = path.split('/').next()?; + let Some(tenant) = path.split('/').next() else { + return Ok(None); + }; // Skip reserved paths that are not tenant identifiers // Use the default FHIR version for resource type checking let fhir_version = FhirVersion::default_enabled(); if is_reserved_path(tenant, &fhir_version) { - return None; - } - - // Validate tenant ID format (non-empty, reasonable characters) - if tenant.is_empty() || !is_valid_tenant_id(tenant) { - return None; + return Ok(None); } - Some(TenantId::new(tenant)) + // A URL prefix is a single path segment by construction, so a + // hierarchical id can never appear here — `acme/research` arrives as the + // segment `acme` followed by a path. Hierarchical tenants are therefore + // addressable by header and JWT claim, but not by URL-prefix routing. + Ok(TenantId::parse(tenant).ok()) } fn source_type(&self) -> TenantSource { @@ -124,13 +182,41 @@ impl TenantSourceExtractor for UrlPathTenantExtractor { pub struct HeaderTenantExtractor; impl TenantSourceExtractor for HeaderTenantExtractor { - fn extract(&self, parts: &Parts, _config: &MultitenancyConfig) -> Option { - parts + /// A present-but-invalid header is an error, not an absent tenant. + /// + /// This used to `filter` an invalid value away to `None`, which made the + /// resolver fall through to the **default tenant** — so `X-Tenant-ID: + /// acme.corp` (a `.`, which the old header charset rejected) silently read + /// and wrote the default tenant's data and returned `200`. That is a + /// cross-tenant read/write triggered by a typo. It is now a `400`. + /// + /// An absent header, and a header whose bytes are not valid UTF-8 or ASCII + /// (`to_str()` fails), remain `Ok(None)`: neither asserts a tenant. + fn extract( + &self, + parts: &Parts, + _config: &MultitenancyConfig, + ) -> Result, TenantSourceError> { + let Some(raw) = parts .headers .get(&X_TENANT_ID) .and_then(|v| v.to_str().ok()) - .filter(|s| !s.is_empty() && is_valid_tenant_id(s)) - .map(TenantId::new) + else { + return Ok(None); + }; + // An empty header is treated as "not sent" rather than rejected, which + // is what it meant before and what a client emitting an unset variable + // produces. + if raw.is_empty() { + return Ok(None); + } + TenantId::parse(raw).map(Some).map_err(|error| { + debug!(error = %error, "Rejecting invalid X-Tenant-ID header"); + TenantSourceError { + source: TenantSource::Header, + error, + } + }) } fn source_type(&self) -> TenantSource { @@ -147,12 +233,33 @@ impl TenantSourceExtractor for HeaderTenantExtractor { pub struct JwtTenantExtractor; impl TenantSourceExtractor for JwtTenantExtractor { - fn extract(&self, parts: &Parts, _config: &MultitenancyConfig) -> Option { - parts + /// The claim is **authoritative** — [`crate::extractors::TenantExtractor`] + /// prefers it over header and URL — and it used to be validated by nothing + /// at all, making it the one path that could put an arbitrary string into a + /// storage key. It is now held to the same charset as every other source. + /// + /// An empty claim stays `Ok(None)`: the extractor already treats "empty" as + /// "no claim" and has a dedicated fail-loud path for it. + fn extract( + &self, + parts: &Parts, + _config: &MultitenancyConfig, + ) -> Result, TenantSourceError> { + let Some(raw) = parts .extensions .get::() .and_then(|p| p.tenant_id()) - .map(TenantId::new) + .filter(|t| !t.is_empty()) + else { + return Ok(None); + }; + TenantId::parse(raw).map(Some).map_err(|error| { + warn!(error = %error, "Rejecting invalid tenant claim in validated token"); + TenantSourceError { + source: TenantSource::JwtClaim, + error, + } + }) } fn source_type(&self) -> TenantSource { @@ -205,36 +312,47 @@ impl TenantResolver { /// Resolves the tenant from the request. /// - /// Returns a [`ResolvedTenant`] with the tenant ID and source information. + /// Returns a [`ResolvedTenant`] with the tenant ID and source information, + /// or [`TenantSourceError`] when a source **asserted** a tenant id that fails + /// canonical validation. + /// + /// An invalid assertion aborts resolution rather than being skipped. Skipping + /// it would hand the request to the next source — in practice the default + /// tenant — which is exactly the silent cross-tenant fallback issue #385 + /// exists to remove. The first invalid source wins, in priority order, so the + /// reported reason is the one the caller most likely meant. pub fn resolve( &self, parts: &Parts, config: &MultitenancyConfig, default_tenant: &str, - ) -> ResolvedTenant { + ) -> Result { let mut all_sources = Vec::new(); // Try each extractor in priority order for extractor in &self.extractors { - if let Some(tenant_id) = extractor.extract(parts, config) { + if let Some(tenant_id) = extractor.extract(parts, config)? { all_sources.push((extractor.source_type(), tenant_id)); } } // Select the highest priority source that provided a tenant if let Some((source, tenant_id)) = all_sources.first().cloned() { - ResolvedTenant { + Ok(ResolvedTenant { tenant_id, source, all_sources, - } + }) } else { - // Fall back to default tenant - ResolvedTenant { + // Fall back to default tenant. Constructed unchecked: it is operator + // configuration, not request input, and is validated once at startup + // (see `ServerConfig::validate_default_tenant`) so a misconfiguration + // is a boot failure rather than a per-request rejection. + Ok(ResolvedTenant { tenant_id: TenantId::new(default_tenant), source: TenantSource::Default, all_sources, - } + }) } } } @@ -279,15 +397,9 @@ fn is_fhir_resource_type(type_name: &str, fhir_version: &FhirVersion) -> bool { } } -/// Validates that a string is a valid tenant ID. -fn is_valid_tenant_id(s: &str) -> bool { - // Tenant IDs should be alphanumeric with hyphens and underscores - // and have a reasonable length - !s.is_empty() - && s.len() <= 64 - && s.chars() - .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') -} +// The local `is_valid_tenant_id` that used to live here is gone: it was one of +// three divergent charsets (issue #385). Validation is now `TenantId::parse`, +// the single canonical definition in `helios-persistence`. #[cfg(test)] mod tests { @@ -305,6 +417,19 @@ mod tests { request.into_parts().0 } + /// `Ok(Some(id))` unwrapped, for the many assertions that only care about + /// the resolved value. + fn extracted( + extractor: &dyn TenantSourceExtractor, + parts: &Parts, + config: &MultitenancyConfig, + ) -> Option { + extractor + .extract(parts, config) + .expect("extractor must not reject") + .map(|t| t.as_str().to_string()) + } + #[test] fn test_url_path_extractor() { let extractor = UrlPathTenantExtractor; @@ -312,20 +437,38 @@ mod tests { // Valid tenant in URL let parts = make_parts("/acme/Patient/123", None); - assert_eq!( - extractor - .extract(&parts, &config) - .map(|t| t.as_str().to_string()), - Some("acme".to_string()) - ); + assert_eq!(extracted(&extractor, &parts, &config), Some("acme".into())); // Reserved path (should not extract) let parts = make_parts("/Patient/123", None); - assert_eq!(extractor.extract(&parts, &config), None); + assert_eq!(extracted(&extractor, &parts, &config), None); // System endpoint (should not extract) let parts = make_parts("/metadata", None); - assert_eq!(extractor.extract(&parts, &config), None); + assert_eq!(extracted(&extractor, &parts, &config), None); + } + + /// The URL extractor must never return `Err`, or every untenanted request + /// would 400. A first segment that is not a valid tenant id simply is not a + /// tenant prefix. + #[test] + fn url_path_extractor_never_errors_on_a_non_tenant_segment() { + let extractor = UrlPathTenantExtractor; + let config = MultitenancyConfig::default(); + + for path in [ + "/Patient/123", + "/metadata", + "/resources/x", // reserved segment + "/__system__/x", // reserved sentinel + "/a%20b/Patient", + ] { + let parts = make_parts(path, None); + assert!( + extractor.extract(&parts, &config).is_ok(), + "{path} must fall through, not reject" + ); + } } #[test] @@ -335,20 +478,93 @@ mod tests { // Valid header let parts = make_parts("/Patient/123", Some("acme")); - assert_eq!( - extractor - .extract(&parts, &config) - .map(|t| t.as_str().to_string()), - Some("acme".to_string()) - ); + assert_eq!(extracted(&extractor, &parts, &config), Some("acme".into())); // Missing header let parts = make_parts("/Patient/123", None); - assert_eq!(extractor.extract(&parts, &config), None); + assert_eq!(extracted(&extractor, &parts, &config), None); - // Empty header + // Empty header — "not sent", not "malformed". let parts = make_parts("/Patient/123", Some("")); - assert_eq!(extractor.extract(&parts, &config), None); + assert_eq!(extracted(&extractor, &parts, &config), None); + } + + /// Regression for the silent cross-tenant fallback (issue #385). + /// + /// A present-but-invalid `X-Tenant-ID` used to be filtered away to `None`, + /// so the resolver fell through to the **default tenant** and the request + /// was served — a `200` against someone else's data because of a typo. + #[test] + fn invalid_header_is_rejected_not_silently_defaulted() { + let extractor = HeaderTenantExtractor; + let config = MultitenancyConfig::default(); + + let too_long = "a".repeat(MAX_TENANT_ID_LEN + 1); + for bad in [ + "acme corp", + "acme/", + "__system__", + "resources", + too_long.as_str(), + ] { + let parts = make_parts("/Patient/123", Some(bad)); + let err = extractor + .extract(&parts, &config) + .expect_err(&format!("{bad:?} must be rejected")); + assert_eq!(err.source, TenantSource::Header); + } + } + + /// The whole-resolver view of the same regression: the invalid header must + /// abort resolution rather than let the default tenant win. + #[test] + fn resolver_rejects_invalid_header_instead_of_falling_back_to_default() { + let config = MultitenancyConfig { + routing_mode: TenantRoutingMode::HeaderOnly, + ..Default::default() + }; + let resolver = TenantResolver::new(&config); + + let parts = make_parts("/Patient/123", Some("acme corp")); + let err = resolver + .resolve(&parts, &config, "default") + .expect_err("an invalid asserted tenant must not resolve to the default"); + assert_eq!(err.source, TenantSource::Header); + } + + /// `.` was accepted by the admin API but by neither routing validator, so a + /// tenant provisioned as `tenant.example` was registered and unreachable. + /// The canonical charset is the union, which closes that gap. + #[test] + fn header_accepts_dotted_ids_the_admin_api_could_already_provision() { + let extractor = HeaderTenantExtractor; + let config = MultitenancyConfig::default(); + + let parts = make_parts("/Patient/123", Some("tenant.example")); + assert_eq!( + extracted(&extractor, &parts, &config), + Some("tenant.example".into()) + ); + } + + /// Hierarchical ids reach storage by header (and JWT claim), never by URL + /// prefix — a URL prefix is one path segment by construction. + #[test] + fn hierarchical_ids_route_by_header_but_not_by_url_prefix() { + let config = MultitenancyConfig::default(); + + let parts = make_parts("/Patient/123", Some("acme/research")); + assert_eq!( + extracted(&HeaderTenantExtractor, &parts, &config), + Some("acme/research".into()) + ); + + // The URL form yields the first segment only. + let parts = make_parts("/acme/research/Patient", None); + assert_eq!( + extracted(&UrlPathTenantExtractor, &parts, &config), + Some("acme".into()) + ); } #[test] @@ -361,13 +577,13 @@ mod tests { // Header provided let parts = make_parts("/Patient/123", Some("acme")); - let resolved = resolver.resolve(&parts, &config, "default"); + let resolved = resolver.resolve(&parts, &config, "default").expect("valid"); assert_eq!(resolved.tenant_id_str(), "acme"); assert_eq!(resolved.source, TenantSource::Header); // No header - falls back to default let parts = make_parts("/Patient/123", None); - let resolved = resolver.resolve(&parts, &config, "default"); + let resolved = resolver.resolve(&parts, &config, "default").expect("valid"); assert_eq!(resolved.tenant_id_str(), "default"); assert_eq!(resolved.source, TenantSource::Default); } @@ -382,13 +598,13 @@ mod tests { // Tenant in URL let parts = make_parts("/acme/Patient/123", None); - let resolved = resolver.resolve(&parts, &config, "default"); + let resolved = resolver.resolve(&parts, &config, "default").expect("valid"); assert_eq!(resolved.tenant_id_str(), "acme"); assert_eq!(resolved.source, TenantSource::UrlPath); // No tenant in URL (reserved path) - falls back to default let parts = make_parts("/Patient/123", None); - let resolved = resolver.resolve(&parts, &config, "default"); + let resolved = resolver.resolve(&parts, &config, "default").expect("valid"); assert_eq!(resolved.tenant_id_str(), "default"); assert_eq!(resolved.source, TenantSource::Default); } @@ -403,14 +619,14 @@ mod tests { // Both URL and header - URL wins let parts = make_parts("/acme/Patient/123", Some("other")); - let resolved = resolver.resolve(&parts, &config, "default"); + let resolved = resolver.resolve(&parts, &config, "default").expect("valid"); assert_eq!(resolved.tenant_id_str(), "acme"); assert_eq!(resolved.source, TenantSource::UrlPath); assert_eq!(resolved.all_sources.len(), 2); // Only header (reserved URL path) let parts = make_parts("/Patient/123", Some("acme")); - let resolved = resolver.resolve(&parts, &config, "default"); + let resolved = resolver.resolve(&parts, &config, "default").expect("valid"); assert_eq!(resolved.tenant_id_str(), "acme"); assert_eq!(resolved.source, TenantSource::Header); } @@ -459,15 +675,42 @@ mod tests { assert!(!is_reserved_path("my_tenant", &version)); } + /// Replaces `test_is_valid_tenant_id`, which exercised this module's private + /// copy of the charset. That copy is gone (issue #385) — validation is + /// `TenantId::parse` — so the property worth pinning here is that the + /// *header source* accepts exactly what the canonical validator accepts. + /// + /// Two of the old assertions were inverted by this change, deliberately: + /// `tenant.com` and any dotted id are now **valid** (the admin API could + /// always provision one, so rejecting it here made such a tenant + /// unroutable). `tenant/path` stays invalid *for this source* only because a + /// header carrying it is valid — see + /// `hierarchical_ids_route_by_header_but_not_by_url_prefix` — while a URL + /// prefix is a single segment and can never carry it. #[test] - fn test_is_valid_tenant_id() { - assert!(is_valid_tenant_id("acme")); - assert!(is_valid_tenant_id("tenant-123")); - assert!(is_valid_tenant_id("my_tenant")); - assert!(is_valid_tenant_id("ABC123")); - assert!(!is_valid_tenant_id("")); // empty - assert!(!is_valid_tenant_id("tenant.com")); // dot - assert!(!is_valid_tenant_id("tenant/path")); // slash - assert!(!is_valid_tenant_id(&"a".repeat(100))); // too long + fn header_source_accepts_exactly_the_canonical_charset() { + let extractor = HeaderTenantExtractor; + let config = MultitenancyConfig::default(); + let too_long = "a".repeat(MAX_TENANT_ID_LEN + 1); + + for candidate in [ + "acme", + "tenant-123", + "my_tenant", + "ABC123", + "tenant.com", + "acme/research", + "resources", + "__system__", + "tenant path", + too_long.as_str(), + ] { + let parts = make_parts("/Patient/123", Some(candidate)); + assert_eq!( + extractor.extract(&parts, &config).is_ok(), + TenantId::parse(candidate).is_ok(), + "{candidate:?} classified differently from the canonical validator" + ); + } } } diff --git a/crates/rest/tests/admin_tenants.rs b/crates/rest/tests/admin_tenants.rs index 207badd57..b809756bd 100644 --- a/crates/rest/tests/admin_tenants.rs +++ b/crates/rest/tests/admin_tenants.rs @@ -140,9 +140,10 @@ async fn invalid_ids_are_rejected() { /// Ids naming a control-plane namespace are refused at creation (issue #271). /// -/// This is a guardrail, not the fix: the backend is safe structurally, because -/// this check only covers the admin API and the JWT tenant extractor validates -/// nothing. +/// Still defence in depth rather than what makes the S3 keyspace safe — that is +/// structural — but it is no longer admin-API-only. Since issue #385 the same +/// rule is applied by `TenantId::parse` at every ingress, including the JWT +/// claim, and again by `ResourceStorage::register_tenant` on all backends. #[tokio::test] async fn control_plane_namespace_ids_are_reserved() { let server = create_test_server().await; @@ -155,6 +156,92 @@ async fn control_plane_namespace_ids_are_reserved() { } } +/// A reserved name is reserved in **every** segment, not just as the whole id +/// (issue #385). +/// +/// `acme/resources` passed the old whole-id check. On S3 its objects land under +/// `acme/resources/…`, which is inside the prefix tenant `acme` lists — and that +/// `purge_tenant_data("acme")` deletes. Two tenants, one keyspace, with the +/// deletion crossing the boundary. +#[tokio::test] +async fn reserved_names_are_rejected_in_any_hierarchy_segment() { + let server = create_test_server().await; + for bad in [ + "acme/resources", + "acme/history", + "acme/bulk", + "acme/tenants", + "acme/__system__", + "acme/../evil", + ] { + server + .post("/admin/tenants") + .json(&json!({ "id": bad })) + .await + .assert_status(StatusCode::BAD_REQUEST); + } + + // Only whole segments are reserved — a substring must stay usable. + server + .post("/admin/tenants") + .json(&json!({ "id": "acme/resources-archive" })) + .await + .assert_status(StatusCode::CREATED); +} + +/// The length cap is the canonical 64, not the 128 this handler used to apply +/// on its own (issue #385). +/// +/// An id in 65..=128 bytes was registrable but unroutable: both the header and +/// URL-prefix validators capped at 64, so it could only ever be reached through +/// the JWT claim — the ingress that validated nothing at all. +#[tokio::test] +async fn ids_longer_than_the_canonical_cap_are_rejected() { + let server = create_test_server().await; + + server + .post("/admin/tenants") + .json(&json!({ "id": "a".repeat(64) })) + .await + .assert_status(StatusCode::CREATED); + + server + .post("/admin/tenants") + .json(&json!({ "id": "a".repeat(65) })) + .await + .assert_status(StatusCode::BAD_REQUEST); +} + +/// Malformed hierarchy is rejected: `/` is a separator, so it cannot lead, +/// trail, or repeat. +#[tokio::test] +async fn malformed_hierarchy_is_rejected() { + let server = create_test_server().await; + for bad in ["/acme", "acme/", "acme//research"] { + server + .post("/admin/tenants") + .json(&json!({ "id": bad })) + .await + .assert_status(StatusCode::BAD_REQUEST); + } +} + +/// The rejection body names the reason, so an operator can act on it without +/// reading the source. +#[tokio::test] +async fn rejection_explains_why() { + let server = create_test_server().await; + let body = server + .post("/admin/tenants") + .json(&json!({ "id": "acme corp" })) + .await + .text(); + assert!( + body.contains("letters, digits"), + "expected an actionable reason, got: {body}" + ); +} + /// Hierarchical ids stay valid — `TenantId` models `/` as the hierarchy /// separator (`is_descendant_of`, `parent`, `ancestors`), so tightening the /// charset here would contradict a shipped feature of the domain type. diff --git a/crates/rest/tests/tenant_resolution.rs b/crates/rest/tests/tenant_resolution.rs index b0ca1542e..0d41e72ae 100644 --- a/crates/rest/tests/tenant_resolution.rs +++ b/crates/rest/tests/tenant_resolution.rs @@ -608,7 +608,9 @@ mod tenant_source_tests { let resolver = TenantResolver::new(&config); let parts = make_parts("/acme/Patient/123", Some("other")); - let resolved = resolver.resolve(&parts, &config, "default"); + let resolved = resolver + .resolve(&parts, &config, "default") + .expect("valid tenant"); assert_eq!(resolved.tenant_id_str(), "acme"); assert_eq!(resolved.source, TenantSource::UrlPath); @@ -624,7 +626,9 @@ mod tenant_source_tests { let resolver = TenantResolver::new(&config); let parts = make_parts("/Patient/123", None); - let resolved = resolver.resolve(&parts, &config, "default-tenant"); + let resolved = resolver + .resolve(&parts, &config, "default-tenant") + .expect("valid tenant"); assert_eq!(resolved.tenant_id_str(), "default-tenant"); assert_eq!(resolved.source, TenantSource::Default); @@ -640,7 +644,9 @@ mod tenant_source_tests { let resolver = TenantResolver::new(&config); let parts = make_parts("/acme/Patient/123", Some("acme")); - let resolved = resolver.resolve(&parts, &config, "default"); + let resolved = resolver + .resolve(&parts, &config, "default") + .expect("valid tenant"); // Both sources found the same tenant assert_eq!(resolved.all_sources.len(), 2); From e04162880d1c1e8c193348fbfc4714855e2746f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Thu, 30 Jul 2026 11:23:09 -0400 Subject: [PATCH 2/4] fix(rest): scope MAX_TENANT_ID_LEN to the test module It is only used by tests, so importing it at module scope tripped `-D unused-imports` in the lib build. --- crates/rest/src/tenant/resolver.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/rest/src/tenant/resolver.rs b/crates/rest/src/tenant/resolver.rs index 9c5660d60..54a2f3379 100644 --- a/crates/rest/src/tenant/resolver.rs +++ b/crates/rest/src/tenant/resolver.rs @@ -5,7 +5,7 @@ use axum::http::request::Parts; use helios_fhir::{FhirResourceTypeProvider, FhirVersion}; -use helios_persistence::tenant::{MAX_TENANT_ID_LEN, TenantId, TenantIdError}; +use helios_persistence::tenant::{TenantId, TenantIdError}; use tracing::{debug, warn}; use crate::config::{MultitenancyConfig, TenantRoutingMode}; @@ -405,6 +405,7 @@ fn is_fhir_resource_type(type_name: &str, fhir_version: &FhirVersion) -> bool { mod tests { use super::*; use axum::http::{HeaderValue, Request, Uri}; + use helios_persistence::tenant::MAX_TENANT_ID_LEN; fn make_parts(path: &str, tenant_header: Option<&str>) -> Parts { let mut builder = Request::builder().uri(Uri::try_from(path).unwrap()); From a37c4e0982698497f91641da1852dffe0e18132b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Thu, 30 Jul 2026 12:06:29 -0400 Subject: [PATCH 3/4] feat(admin): report tenant-id canonicality in GET /admin/tenants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The canonical validator refuses ids the old surfaces accepted, so a deployment may hold data under an id no ingress can now reach. Nothing told an operator which ones. Each row gains `canonical`, and the body a `non_canonical_count`, so the stranded set is one request away. Only reporting — `DELETE /admin/tenants/{id}` still accepts a non-canonical id, deliberately, so they remain cleanable. --- crates/rest/src/handlers/admin_tenants.rs | 24 +++++++++++++++++ crates/rest/tests/admin_tenants.rs | 33 +++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/crates/rest/src/handlers/admin_tenants.rs b/crates/rest/src/handlers/admin_tenants.rs index 3b68a786e..0565dd650 100644 --- a/crates/rest/src/handlers/admin_tenants.rs +++ b/crates/rest/src/handlers/admin_tenants.rs @@ -122,6 +122,16 @@ fn validate_tenant_id(id: &str) -> RestResult<()> { /// from stored data (`registered: false`, `created_at: null`) so nothing with /// data is hidden. Each row carries the current non-deleted `resources` count. /// The internal system tenant is never listed. +/// +/// Each row also carries `canonical` — whether the id satisfies +/// [`TenantId::parse`]. It is `true` for everything this API can create, so the +/// field only ever matters for ids that predate the canonical validator (issue +/// #385): those were stored under a wider (or absent) charset and are no longer +/// reachable through any ingress. This is how an operator finds them. Rows with +/// `canonical: false` and a non-zero `resources` count are the ones needing +/// attention — the data is still there, but requests naming that tenant are now +/// rejected. `DELETE /admin/tenants/{id}` still accepts them, deliberately, so +/// they can be cleaned up. pub async fn list_tenants_handler(State(state): State>) -> RestResult where S: ResourceStorage + Send + Sync, @@ -137,6 +147,11 @@ where .into_iter() .collect(); + // `new` (unchecked) then `is_canonical`, not `parse().is_ok()`, to say + // plainly what this is: reporting on a value that is already stored, never + // admitting one. + let is_canonical = |id: &str| helios_persistence::tenant::TenantId::new(id).is_canonical(); + let mut seen = std::collections::HashSet::new(); let mut tenants = Vec::new(); for rec in ®istered { @@ -146,6 +161,7 @@ where "display_name": rec.display_name, "created_at": rec.created_at, "registered": true, + "canonical": is_canonical(&rec.id), "resources": counts.get(&rec.id).copied().unwrap_or(0), })); } @@ -162,12 +178,20 @@ where "display_name": null, "created_at": null, "registered": false, + "canonical": is_canonical(id), "resources": n, })); } + let non_canonical = tenants + .iter() + .filter(|t| t["canonical"] == json!(false)) + .count(); let body = json!({ "tenant_count": tenants.len(), + // Surfaced at the top level so an operator does not have to scan the + // rows to learn whether the upgrade stranded anything. + "non_canonical_count": non_canonical, "tenants": tenants, }); Ok((StatusCode::OK, Json(body)).into_response()) diff --git a/crates/rest/tests/admin_tenants.rs b/crates/rest/tests/admin_tenants.rs index b809756bd..a6e81f173 100644 --- a/crates/rest/tests/admin_tenants.rs +++ b/crates/rest/tests/admin_tenants.rs @@ -111,6 +111,39 @@ async fn create_then_list_round_trips() { assert_eq!(acme["resources"], 0); } +/// The listing reports whether each id satisfies the canonical validator, so an +/// operator can find tenants stranded by the tightening in issue #385. +/// +/// Only the `true` side is reachable here, and that is the point: since this +/// change, neither the admin API nor `ResourceStorage::register_tenant` will +/// mint a non-canonical id, and a resource cannot be written under one either +/// (the tenant header is validated). A `canonical: false` row can therefore only +/// come from data that predates the validator — which a fresh test database has +/// none of. The `false` side is covered where it can be constructed, against the +/// unchecked constructor: `TenantId::is_canonical`'s unit tests in +/// `helios-persistence`. +#[tokio::test] +async fn listing_reports_canonicality_so_legacy_ids_can_be_found() { + let server = create_test_server().await; + server + .post("/admin/tenants") + .json(&json!({ "id": "acme/research" })) + .await + .assert_status(StatusCode::CREATED); + seed_for(&server, "beta", "Patient").await; + + let list = server.get("/admin/tenants").await.json::(); + + // Registered and data-discovered rows both carry the flag. + assert_eq!( + find(&list, "acme/research").expect("acme")["canonical"], + true + ); + assert_eq!(find(&list, "beta").expect("beta")["canonical"], true); + // And the summary an operator actually reads. + assert_eq!(list["non_canonical_count"], 0); +} + #[tokio::test] async fn duplicate_create_conflicts() { let server = create_test_server().await; From 18f8bb0eb2b7dae22af7dae8791a9b9083ced6a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Thu, 30 Jul 2026 13:53:25 -0400 Subject: [PATCH 4/4] fix(rest): tenant-prefix validation is a total delegation, not a stricter check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_valid_tenant_id` asserted `tenant/path` was invalid while delegating wholly to `TenantId::parse`, which accepts a hierarchical id — so the test failed. The assertion was wrong, not the code: the "a tenant prefix is one segment" constraint lives in the caller, which passes `path.split('/').next()`, so a `/` never reaches this function. Adding a local `/` check would guard nothing reachable while reintroducing a second charset alongside the canonical one. Fixed the assertion, said why in the doc comment, and added the hierarchical case to the agreement test so a future divergence fails loudly. --- crates/rest/src/middleware/tenant_prefix.rs | 42 +++++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/crates/rest/src/middleware/tenant_prefix.rs b/crates/rest/src/middleware/tenant_prefix.rs index 7bb7bb217..ce2c3ccbe 100644 --- a/crates/rest/src/middleware/tenant_prefix.rs +++ b/crates/rest/src/middleware/tenant_prefix.rs @@ -76,8 +76,14 @@ fn is_fhir_resource_type(type_name: &str, fhir_version: &FhirVersion) -> bool { /// this accepted a segment the resolver rejected, authorization and data access /// would disagree about which tenant the request is for. /// -/// A URL prefix is a single path segment, so a hierarchical id never reaches -/// here — `parse` will simply see the first segment. +/// The delegation is total: whatever `parse` accepts, this accepts. In +/// particular it does **not** additionally reject `/`, even though a tenant +/// prefix is by definition a single path segment. That is deliberate — the only +/// caller, [`extract_tenant_from_path`], passes `path.split('/').next()`, so a +/// string containing `/` never reaches here. Adding a local `/` check would look +/// like a guard while guarding nothing reachable, and would reintroduce exactly +/// what issue #385 removed: a second, subtly different charset alongside the +/// canonical one. fn is_valid_tenant_id(s: &str) -> bool { TenantId::parse(s).is_ok() } @@ -294,12 +300,38 @@ mod tests { // A tenant provisioned as `tenant.example` was previously unroutable. assert!(is_valid_tenant_id("tenant.example")); assert!(!is_valid_tenant_id("")); // empty - assert!(!is_valid_tenant_id("tenant/path")); // never one URL segment assert!(!is_valid_tenant_id(&"a".repeat(100))); // too long assert!(!is_valid_tenant_id("tenant corp")); // whitespace // Reserved control-plane segments are not tenants (issue #385). assert!(!is_valid_tenant_id("resources")); assert!(!is_valid_tenant_id("__system__")); + + // A hierarchical id *is* valid — this is a total delegation to + // `TenantId::parse`, not a stricter single-segment check. The earlier + // version of this test asserted the opposite and failed, which is the + // useful thing it did: the constraint that a tenant prefix is one + // segment lives in the caller (`path.split('/').next()`), not here. + // `hierarchical_id_is_accepted_here_but_unreachable_from_a_url` shows + // why that is safe. + assert!(is_valid_tenant_id("tenant/path")); + } + + /// A `/` never reaches [`is_valid_tenant_id`] from a real request, so its + /// accepting one costs nothing — and a URL still cannot address a + /// hierarchical tenant, because only the first segment is taken. + #[test] + fn hierarchical_id_is_accepted_here_but_unreachable_from_a_url() { + let version = default_version(); + + // Accepted in isolation… + assert!(is_valid_tenant_id("acme/research")); + + // …but the URL form yields the first segment only, so `acme/research` + // is addressable by header or JWT claim and never by URL prefix. + let (tenant, remaining) = + extract_tenant_from_path("/acme/research/Patient", &version).unwrap(); + assert_eq!(tenant, "acme"); + assert_eq!(remaining, "/research/Patient"); } /// `authz_middleware` and the tenant resolver must classify a path segment @@ -313,6 +345,10 @@ mod tests { "tenant.example", "my_tenant", "tenant-1", + // Included so the delegation stays *total*: a future local `/` + // check here would diverge from the canonical validator and fail + // this test, which is the point. + "acme/research", "", "resources", "__system__",