Tenant isolation: fail-open on lookup error, and NULL-org rows visible to every tenant - #7
Merged
Merged
Conversation
`_resolve_tenant_org` caught every exception from the organization query and returned None. `__call__` cannot tell that None apart from the legitimate one -- superuser, or a user with no tenant -- so it proceeded without installing a scope. Postgres RLS reads an unset `forail.current_tenant_id` as "every row", so a transient database error during the lookup turned a tenant's request into a global one. The outer try/except in `__call__` made it worse: it looked like a guard, but the exception had already been swallowed one level down, so it never fired. A failed lookup is not evidence that the user has no tenant. It is evidence that we do not know, and the only safe answer to that is to refuse -- which is already what a failed `set_tenant_id` does two lines below. `_resolve_tenant_org` now lets the error out and `__call__` fails closed with the same 500. Gated on tenancy and RLS both being on: with either off the resolver returns before touching the database, so a single-tenant install cannot start answering 500 because of this gate. Covered by tests_standalone/test_tenant_isolation.py, a new file -- the module had none. Against the pre-fix code two of them fail: the view runs anyway, and the resolver swallows the error.
Both policy builders emitted `OR organization_id IS NULL` for every table, on the reasoning that a NULL organization marks an AWX resource shared platform-wide. That is true of a handful of inherited tables and of none of ours: 20 of the 22 covered tables have a nullable organization, so a single row saved without one -- a scan result, a drift alert, an audit event, an inventory -- was readable by every tenant. One missed assignment was enough. Which tables mean it is now explicit. `RLS_GLOBAL_NULL_ORG_TABLES` lists the five where a NULL organization is a real sharing or ownership mechanism: credentials and OAuth applications owned by a user rather than an org, globally shared execution environments, and system job templates and their runs. Adding to that list is a decision that those rows may be read by any tenant. Everything else is scoped strictly, including every Forail-authored table -- that is where the organization is assigned by our own code, so a NULL is an unset field rather than a convention. The indirect policy follows the parent table, since the parent is the row that carries the organization; the orphan bypass (`fk IS NULL`) stays, because a row with no parent has no organization to compare against and hiding it would make it unreachable rather than unscoped. Behaviour change, stated plainly in migration 0211: on a tenant-scoped request, existing rows with no organization in a strictly-scoped table stop being returned. They are not deleted, and superusers and unscoped requests still see them. Hidden is the safe direction for an ambiguous row; visible-to-every-tenant is not. If a resource disappears for a tenant, assign its organization.
…ronment `_ENV_SYNC_KEYS` mirrored `TENANCY_ENABLED` into the Setting registry and nothing else. `TENANCY_RLS_ENABLED`, `TENANCY_STRICT_ISOLATION_ENABLED` and `TENANCY_RATE_LIMITING_ENABLED` all default to False and are the flags that actually enforce anything -- so a deployment could set the one switch it was offered, reasonably expect isolation, and get the tenancy features running with no row-level security behind them and no way to turn it on short of the Settings UI. All four now sync, and all four are parsed as booleans: without that the Setting row would hold the string "false", which is truthy at every point it is read. Pairs with the chart change that exposes them as Helm values and refuses an install that enables tenancy without RLS.
krlex
force-pushed
the
fix/tenant-isolation-fail-open
branch
from
August 19, 2026 21:17
53ef959 to
0578fc6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes H3 and M6 from the 2026-08-19 Codex review. Both are ways a
tenant-scoped request could end up seeing rows outside its tenant.
H3 — a failed tenant lookup ran the request with no RLS scope
_resolve_tenant_orgcaught every exception from the organization query andreturned
None.__call__cannot tell thatNoneapart from the legitimate one— superuser, or a user with no tenant — so it proceeded without installing a
scope, and Postgres RLS reads an unset
forail.current_tenant_idas every row.A transient database error during the lookup turned a tenant's request into a
global one.
The outer
try/exceptin__call__looked like a guard but never fired: theexception had already been swallowed a level down.
A failed lookup is not evidence that the user has no tenant — it is evidence
that we do not know, and the only safe answer to that is to refuse. Which is
already what a failed
set_tenant_iddoes two lines below. The resolver now letsthe error out and the middleware fails closed with the same 500, gated on
tenancy and RLS being on so a single-tenant install cannot start answering
500 because of this gate.
M6 — RLS treated every NULL organization as globally shared
Both policy builders emitted
OR organization_id IS NULLfor every table, on thereasoning that a NULL organization marks an AWX resource shared platform-wide.
That is true of a handful of inherited tables and of none of ours — 20 of the
22 covered tables have a nullable organization. One scan result, drift alert,
audit event or inventory saved without an organization was readable by every
tenant.
RLS_GLOBAL_NULL_ORG_TABLESnow names the five where a NULL organization is areal ownership or sharing mechanism: user-owned credentials and OAuth
applications, globally shared execution environments, and system job templates
and their runs. Everything else — including every Forail-authored table, where
our own code assigns the organization — is scoped strictly. The indirect policy
follows the parent table, since the parent carries the organization; the orphan
bypass stays, because a row with no parent has no organization to compare
against.
Behaviour change, stated in migration
0211: on a tenant-scoped request,existing rows with no organization in a strictly-scoped table stop being
returned. They are not deleted, and superusers and unscoped requests still see
them. If a resource disappears for a tenant, assign its organization — do not add
its table to the global list.
Verified
tests_standalone/test_tenant_isolation.py— new file, 10 tests; themiddleware had none. Against the pre-fix code two fail for the right reason:
the view runs anyway, and the resolver swallows the database error.
tests_standalone/test_tenancy.py— 90 passed, including a closed-listassertion on the global tables so growing it cannot be quiet, and a check that
every Forail table is strictly scoped.
test_dynamic_survey_standalone.py, whichcannot be collected on
developat all — that is fixed in forail-backend#6 andis why CI still excludes it here.