Fix six tenant isolation and provisioning defects, with tests - #1054
Open
dazza-dev wants to merge 9 commits into
Open
Fix six tenant isolation and provisioning defects, with tests#1054dazza-dev wants to merge 9 commits into
dazza-dev wants to merge 9 commits into
Conversation
In the separate-database separation mode a tenant's database password is never stored: it is recomputed from the website on every connection. If the algorithm, its inputs, or the way those inputs become a string ever change, every existing tenant loses access to its database at once, and reverting the code does not help unless the old algorithm comes back with it. Five tests pin that down. Two freeze the exact hash for both branches, the keyed one and the legacy fallback for installations that never set tenancy.key. The others assert determinism, that every input feeds the hash, and that created_at does. That last one documents a sharp edge rather than endorsing it. The timestamp reaches the hash through sprintf, so anything changing how it renders as a string invalidates the password of every affected tenant. Carbon 2 and Carbon 3 were compared directly on this point and both render "2023-05-14 09:31:07", so the Carbon 3 requirement arriving with Laravel 12 does not break existing tenants. All inputs are synthetic. No real key or tenant data belongs in a test.
Connection::purge() clears both the open connection and the stored
configuration. Connection::set() does not: called with null it closes the
connection but leaves the previous tenant's database, username and password in
config('database.connections.tenant').
Nothing is open at that point, which is why it looks harmless. It is not. Any
model using UsesTenantConnection asks for that connection unconditionally, so
Laravel opens a fresh one from the leftover configuration and lands in the
previous tenant's database. TenantAwareConnection is unaffected because it
checks exists() first, but UsesTenantConnection, which is what applications use
at scale, has no such guard. Listeners\Database\ConnectsTenants passes whatever
a Websites event carries straight into set(), and that can be null.
set() now delegates to purge() when there is no website.
Two supporting additions. Environment::forgetTenant(), because there was no way
to say "no tenant is active": tenant(null) reads as releasing one but only
returns the current tenant. And Events\Websites\Forgotten as the counterpart to
Switched, so listeners holding per-tenant state can tear it down.
Ten isolation tests come with it. They assert on the exact rows visible and name
the tenant whose data appeared, because a failure here means one customer can
read another's records.
The last of them aims a connection straight at another tenant's database and
requires the server to refuse the rows. It asserts the outcome rather than the
mechanism, because engines differ: MySQL and MariaDB reject the connection,
since a tenant's user holds privileges on its own database only, while
PostgreSQL admits it and refuses at the table instead, as every role may
CONNECT to a new database by default. Both are acceptable; returning rows is
not.
A queue worker is a long-lived process handling one job after another in the same memory, so anything a job leaves active is inherited by whatever runs next. QueueProvider activated a tenant when a payload carried website_id and did nothing otherwise, which produced four distinct leaks: - a job with no website_id inherited the previous job's tenant - a finished job left the worker holding that customer's connection - a job that threw left its tenant active for the next one - a job naming a deleted website inherited whichever tenant ran before it The last is the sharpest: findById returns null, nothing is activated, and the previous tenant simply stays. This one fails silently, unlike the rest. Each tenant has its own database user, so a misrouted query normally hits an access denied error. Here the inherited connection is a valid, authenticated connection to a real tenant's database: every query succeeds, nothing is logged, and the only evidence is data ending up in the wrong customer's records. The tenant is now released when a job declares none, and restored to whatever was active beforehand when a job ends or fails. Restoring rather than clearing matters: under dispatch_sync the job runs inside a request that already has its own tenant, and wiping it would break the caller rather than protect it. The previous tenants are kept on a stack, since a synchronous job can dispatch another. Inline execution is left alone. It dies with the request that asked for it, so there is no next job to inherit anything, and DispatcherMiddleware deliberately switches the ambient tenant there. Behind tenancy.queue.reset-tenant-between-jobs, defaulting to the safe value, because this changes behaviour for every installation: jobs that relied on an ambient tenant now fail with a connection error rather than touch the wrong database. UPGRADE.md covers what to do and the escape hatch.
PostgreSQL 15 revoked the CREATE privilege the public schema granted to every role. Privileges on the database no longer cover it, so a tenant database was created and then failed every migration with "permission denied for schema public". Upstream only ever tested against 14. A schema grant only applies inside its own database, so provisioning now opens a connection to the new one to issue it. It goes to PUBLIC rather than to the tenant role: a role level grant is recorded as a dependency in pg_shdepend, and DROP USER then fails when the tenant is deleted. CONNECT is revoked from PUBLIC on the new database so that stays a narrowing. Until now any role could connect to any tenant database; rows were refused, the shape of the schema was not.
The command called db:wipe on the tenant connection, which empties every table in the database behind it. That database belongs to the tenant alone in the database and schema division modes, but in prefix mode all tenants and the system tables share one, and running it dropped them all. The command took itself down with them: its next page of websites came from the table it had just dropped. It now drops only the tables carrying the tenant's prefix, and refuses in a shared database that hands out no prefix, where a wipe cannot be aimed at anything narrower than everything. Views and user defined types are left alone there, having no prefix to tell whose they are.
Doctrine's schema manager works below the connection, so it applies neither the table prefix nor the search path. The seed tests looked for "samples" where the table is "1_samples" in prefix mode, and outside the tenant's schema in schema mode, and failed in both. ConnectionTest caught Doctrine\DBAL\Driver\PDOException around a reconnect. PDO throws the native PDOException, which is not an instance of it, so the assertion inside that block could never run. Listing databases through the connection needs a query per engine, since PostgreSQL keeps them in a catalogue rather than in information_schema. Nothing in the package uses Doctrine DBAL any more, which Laravel 11 removes.
Three tests asserted something only the database division mode provides and failed everywhere else: a database of the tenant's own on a second server, credentials of its own to cross, and a database name and user that differ between two tenants. The first two skip outside that mode. The third asks instead for whichever separator the configured mode hands out, a schema or a table prefix, which is the guarantee actually being made.
ActivatesDisk rooted the tenant disk in the active website's directory on
Identified and Switched, and did nothing on release. The disk stayed
pointed at the tenant that had just been let go, so anything writing to
Storage::disk('tenant') afterwards landed in that customer's files.
It is the filesystem counterpart of the connection keeping the previous
tenant's credentials, and it matters most on a queue worker, which now
releases the tenant between jobs.
The disk is cleared instead of repointed at the shared root, so the next
access fails rather than writing somewhere plausible but wrong.
The command switches tenant on each website in turn and left the last one active when it finished, and on the way out of an exception. From a terminal that hardly matters, since the process ends. Called through Artisan::call() from a request or a job, the caller carried on as a customer it never asked for. The commands built on processHandle() are deliberately left alone: with a single tenant in the chunk they keep the connection active, and the suite asserts that.
dazza-dev
changed the title
This was referenced Aug 29, 2026
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
1 participant
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.
Six defects in 5.9, each with a test that fails without its fix. Every commit is self-contained and passes on its own, so any subset can be taken.
The fixes
A queue worker carries one job's tenant into the next. A worker is a long-lived process, so whatever a job leaves active is inherited by whatever runs after it. Four distinct leaks, all reproduced against a real worker with real tenants:
website_idinherits the previous job's tenantThe tenant is now scoped to the job and restored afterwards. Synchronous dispatch is deliberately untouched:
DispatcherMiddlewareswitches the ambient tenant there, and the existing tests assert it stays switched.Connection::set(null)leaves the previous tenant's credentials armed. It closes the connection but leaves the database, user and password inconfig('database.connections.tenant'). Nothing is open, so it looks harmless; the next model usingUsesTenantConnectionreopens straight into that tenant's database.ConnectsTenantscallsset()with whatever aWebsitesevent carries, and that can be null.The
tenantdisk stays rooted in a tenant that was released.ActivatesDiskpoints the disk at the active tenant's directory onIdentifiedandSwitched, and does nothing on release, so anything writing toStorage::disk('tenant')afterwards lands in that customer's files. It is the filesystem counterpart of the connection above, and it matters most on a queue worker, which releases the tenant between jobs. The disk is now cleared rather than repointed at the shared root, so the next access fails instead of writing somewhere plausible but wrong.tenancy:runleaves the last tenant active. It switches tenant on each website in turn and never puts back what it found, on the way out of the loop or out of an exception. From a terminal that hardly matters, since the process ends. Called throughArtisan::call('tenancy:run')from a request or a job, the caller carries on as a customer it never asked for.PostgreSQL 15 and newer cannot provision a tenant at all. 15 revoked the CREATE privilege the
publicschema granted to every role, and the driver only grants on the database. The tenant database is created successfully and then fails every migration withpermission denied for schema public. On a PostgreSQL 16 server the suite goes from 37 errors to none.tenancy:migrate:freshwipes the whole database inprefixmode. It callsdb:wipeon the tenant connection, which empties every table behind it. That database belongs to the tenant alone in thedatabaseandschemamodes, but inprefixmode every tenant and the system tables share one, so it drops all of them,websitesandhostnamesincluded. The command takes itself down in the process: its next page of websites comes from the table it just dropped.Behaviour changes
Four, all written up in
UPGRADE.md:TENANCY_QUEUE_RESET_TENANT=falserestores the old behaviour while an application adapts.Storage::disk('tenant')outside any tenant's context now throws instead of silently using whichever tenant came last.tenancy:runrestores the tenant that was active before it ran, or releases it when there was none. The commands built onprocessHandle()are deliberately left alone: with a single tenant in the chunk they keep the connection active, and the suite asserts that.CONNECTrevoked fromPUBLIC. Any role that reached a tenant database purely through the default grant is refused; databases created before this are unaffected. The revoke is what makes the accompanying schema grant safe, so the two go together.Tests
The suite had never run outside the
databasemode on MySQL, and running it across three engines and every division mode is what surfaced the provisioning and wipe defects. Three test-only commits are included because they were needed before the other modes could report honestly:sampleswhere the table is1_samples.ConnectionTestcaughtDoctrine\DBAL\Driver\PDOException. PDO throws the nativePDOException, which is not an instance of it, so the assertion inside that block could never run.databasedivision mode provides and failed everywhere else.Nothing uses Doctrine DBAL any more, which Laravel 11 removes.
The disk and
tenancy:rundefects came out of a second pass: writing down every way a tenant's state can outlive its context, then writing the test for each one the suite did not already cover. Two of those tests failed.Verified on PHP 8.3 against MySQL 8, MariaDB 11 and PostgreSQL 16, in the
database,prefixandschemadivision modes.Disclosure
I maintain a fork at dazza-dev/hyn-multi-tenant, which picks 5.9 up for Laravel 11 and later. These six defects affect everyone on 5.9 regardless of Laravel version, so they belong here as well rather than only there.
Happy to split this into separate pull requests if that is easier to review.