fix: tenancy, boot-config, audit and session-cookie bug batch (#494 #495 #497 #498 #499 #503 #506 #554)

CHANGELOG.md

@@ -12,6 +12,26 @@ All notable changes to this project are documented in this file. The format is b
 ## [Unreleased]
 
 ### Changed
+- **Behaviour change (#494):** a pre-app settings read that fails (connection,
+  query, import error) is no longer treated as a fresh install. Outside
+  development and testing the host and the Celery worker refuse to boot with
+  `HostSettingsUnavailableError` instead of building with defaults (which
+  silently disabled `multi_tenant`). Only a missing settings table falls back
+  quietly. Likewise the secret key: an ephemeral key is used only before the
+  settings table exists; otherwise boot fails with `SecretKeyUnavailableError`,
+  and a blank stored key is replaced atomically.
+- **Behaviour change (#495):** the session cookie is `Secure` outside
+  development and testing. New `HostSettings.session_cookie_secure`
+  (`SM_SESSION_COOKIE_SECURE`); set `SM_TRUSTED_PROXY` behind a TLS terminator,
+  and `false` only for installs that serve browsers plain HTTP on purpose.
+- **Behaviour change (#498):** with `audit_log` installed, writes made in Celery
+  tasks and durable outbox handlers are audited (including `TaskExecution`
+  creation and status changes; its heartbeat, timestamps and retry counter are
+  excluded, so a heartbeat-only update writes no audit row, and the
+  `OutboxMessage` delivery bookkeeping is not audited at all). The audit callback is bound per engine instead of a
+  process-global, through the new `simple_module.audit_sinks` entry-point
+  group. Worker tests that build a database without the audit table need the
+  sink off (see `background_tasks/tests/conftest.py`).
 - **Behaviour change (#530):** password-login lockout now holds across workers,
   source IPs and restarts. `users.login_lockout_account_failures` (new, default
   10) failures for one account within `login_rate_limit_window_seconds`, from
@@ -348,6 +368,12 @@ All notable changes to this project are documented in this file. The format is b
   *complete* verdict now keeps answering while one refresh runs.
 
 ### Security
+- Tenant and soft-delete filters now cover Core statements that used to leak
+  other tenants' or deleted rows (#506): outer and FULL joins over
+  `Model.__table__`, table aliases, subqueries, CTEs, scalar/correlated
+  subqueries, `exists()` over joins or in ON clauses, `union`/`intersect`/
+  `except`, tables joined into UPDATE/DELETE, and `insert().from_select()`
+  sources. Raw `text()` SQL remains unfiltered by design.
 - `smpy new` no longer writes generated secrets into the committed
   `.env.example` (#441). `SM_SECRET_KEY` and the users token secrets go to a
   gitignored `.env` (created if absent; an existing one only gains missing
@@ -364,6 +390,18 @@ All notable changes to this project are documented in this file. The format is b
   `tenants` resolver it selects among the user's own memberships.
 
 ### Fixed
+- A stored host setting that parses but fails validation (or an unrecognised
+  boolean like `"tru"`) is dropped with a WARNING instead of crashing boot or
+  reading as false (#497); one failing module settings read no longer resets
+  that module or poisons the others' reads.
+- Every `HostSettings` field `create_app` reads is marked restart-required, so
+  the settings editor says so (#499).
+- An established install whose setup check raises no longer drops into the
+  first-run wizard.
+- The admin mobile sidebar drawer closes on Escape, traps focus and is a
+  labelled dialog while open (#554).
+- Docs describe the real migration graph (two roots, four heads, inherited
+  labels), pinned by a test (#503).
 - `file_storage` browse page: a caller without `file_storage.manage` sees their
   own usage and no tenant quota, instead of the tenant-wide total; the
   authenticated download now carries the same sandbox CSP as public serving.

CLAUDE.md

@@ -234,7 +234,8 @@ by `InertiaLayoutDataMiddleware`; `auth.user` is null unless
 **CSRF.** Baseline is `SameSite=Lax` on the Starlette session cookie; raw
 `fetch()` in pages needs no token. Defence in depth: `RequiresCsrf` router
 dependency + `get_csrf_token(request)` as a view prop, echoed as `X-CSRF-Token`
-(`simple_module_hosting.csrf`).
+(`simple_module_hosting.csrf`). The session cookie is `Secure` outside
+development and testing (`session_cookie_secure`).
 
 ## Database
 
@@ -275,19 +276,27 @@ dependency + `get_csrf_token(request)` as a view prop, echoed as `X-CSRF-Token`
   never by importing `tenants`. `docs/framework/multi-tenancy.md`.
 - **Migrations** live in `host/migrations/versions/`, never in module packages.
   `host/migrations/env.py` builds metadata from every installed module and runs
-  `ensure_db_extensions` first. The history has **several heads** (one per
-  labelled branch), so apply with `upgrade heads` (what `make migrate` does)
-  and autogenerate against one branch, since plain `revision --autogenerate`
-  refuses with "Multiple heads":
+  `ensure_db_extensions` first. The history is **not** one linear chain: it has
+  two roots (`77162e7b184b` initial schema, `e36ab09c4a92` tenants) and four
+  heads (`audit_log`, `tenants`, `keycloak`, and the users trunk, which Alembic
+  reports as `api_tokens`). Apply with `upgrade heads` (what `make migrate`
+  does); plain `revision --autogenerate` refuses with "Multiple heads", so name
+  the branch:
 
   ```
   uv run --project host alembic -c host/alembic.ini revision --autogenerate \
       -m "<msg>" --head <branch_label>@head
   ```
 
-  A module's first revision sets `branch_labels = ("<module_name>",)` (older
-  modules lack it); the label is a named target, not a per-module rollback:
-  `downgrade <module>@base` walks everything beneath it.
+  Labels are inherited by every descendant and are only named targets, not
+  per-module rollbacks: `downgrade <label>@base` unwinds the label's revision,
+  everything above it and everything beneath it on that root, other modules
+  included. Known mislabels (existing files are left alone, applied databases
+  stay valid): `51b2058d8afd` puts `api_tokens` on the trunk, so users,
+  file_storage and background_tasks revisions report it, and keycloak's
+  `168a2882f443` branches off the users revision `873ca2015033`. Give an
+  unlabelled module a label on its next revision. `host/tests/test_migration_graph.py`
+  pins the roots, heads and owners; update it with any new root or head.
   `docs/database/migrations.md`.
 - Postgres extensions: override `ModuleBase.db_extensions()`; `LTree`/`TSVector`
   types are `TEXT` on SQLite; tests needing Postgres carry
@@ -316,6 +325,12 @@ dependency + `get_csrf_token(request)` as a view prop, echoed as `X-CSRF-Token`
   lifespan opens the DB, so a `HostSettings` field read at build time needs
   that pre-read. `SM_SECRET_KEY` is optional: unset generates and stores a key
   atomically; an explicit value wins and is never copied into the DB.
+  A pre-app read failure is not a fresh install: outside development it
+  refuses to boot (`HostSettingsUnavailableError`/`SecretKeyUnavailableError`);
+  only a missing settings table falls back to defaults and an ephemeral key.
+  Stored values parse through `simple_module_core.stored_settings`; an invalid
+  row is dropped with a WARNING. Every `HostSettings` field `create_app` reads
+  is marked restart-required (`test_build_time_settings_marked.py`).
   Module settings are a `DbBackedSettings` subclass (constructor + DB, no env
   source) registered in `register_settings` through
   `settings.registration.register_module_settings(app, package, SettingsCls,

docs/database/migrations.md

@@ -4,11 +4,41 @@ All migrations live in `host/migrations/versions/` — **not** in module package
 
 ## Why centralized?
 
-- **Dependency ordering is global.** If `invoices` depends on `orders_order.id`, their migrations must order correctly. One linear Alembic history enforces this.
+- **Dependency ordering is global.** If `invoices` depends on `orders_order.id`, their migrations must order correctly. One shared Alembic history, with `down_revision` links between modules, enforces this.
 - **Autogenerate sees everything.** `host/migrations/env.py` calls `build_module_metadata()` to union every installed module's `MetaData`. Autogenerate diffs the DB against that union and writes one migration covering all changes.
 - **Operators run one command.** `make migrate` is the only target. No "did you also run `orders/migrate`?" footgun.
 
-Each module's *first* migration sets `branch_labels = ("<module_name>",)`. The label is a named target for that revision; it does **not** isolate the module's history, because every first revision chains off the previous head. `alembic downgrade <module>@base` therefore walks the whole chain beneath it, other modules included. See [Removing one module's schema](#removing-one-modules-schema).
+## The real graph
+
+The history is **not** a single linear chain. It has several roots and several heads, and `make migrate` runs `upgrade heads` for that reason. On `main` today:
+
+| Root | Branch |
+|---|---|
+| `77162e7b184b` initial schema | the users trunk (also holds file_storage, background_tasks, api_tokens revisions) |
+| `e36ab09c4a92` tenants initial schema | tenants (`f1a9c3d7b2e4` head) |
+
+| Head | Owning module | Label Alembic reports |
+|---|---|---|
+| `c8e4f1a2b6d9` | users (login lockout) | `api_tokens` |
+| `a7c2e91d4b58` | audit_log | `audit_log` |
+| `f1a9c3d7b2e4` | tenants | `tenants` |
+| `f6a3b9d2e8c4` | keycloak | `keycloak` |
+
+`audit_log` forks off the trunk at `41cf2c53660e`, and `keycloak` forks off the users revision `873ca2015033`.
+
+A module's first migration sets `branch_labels = ("<module_name>",)`. Alembic **inherits** a label onto every descendant, so the label names a revision and everything above it, and it does not isolate the module. It is only a named target (`upgrade <label>@head`, `revision --head <label>@head`) and the source of the Doctor's `module` column, which reads the first label on each revision.
+
+### Known mislabels
+
+Only four of the revisions set `branch_labels` (`audit_log`, `api_tokens`, `keycloak`, `tenants`). The graph carries these defects; they are documented rather than rewritten, because changing labels on revisions that installed databases already applied is not worth the risk:
+
+- `51b2058d8afd_api_tokens_initial` sits in the middle of the trunk, so every revision above it (file_storage, background_tasks including the outbox, and the users revisions through `c8e4f1a2b6d9`) reports the `api_tokens` label. The `api_tokens` head is therefore really a users revision.
+- `keycloak` branches off a users revision, so a Keycloak-only install still builds the users schema.
+- `users`, `file_storage`, `background_tasks`, `settings`, `permissions` and `feature_flags` have no label of their own.
+
+Guidance: give an unlabelled module a `branch_labels` on its **next** revision. Adding a label to an existing revision file is technically possible, because `alembic_version` stores revision ids and never labels, but it also relabels every descendant and changes what `<label>@head` resolves to, so treat it as a deliberate, reviewed change rather than a cleanup.
+
+`host/tests/test_migration_graph.py` pins the roots, heads, each revision's owning module and the mislabels above. Adding a root, head or revision fails it until the table is updated.
 
 ## Day-to-day workflow
 
@@ -41,12 +71,12 @@ make downgrade                                                 # back one revisi
 uv run --project host alembic -c host/alembic.ini downgrade <revision_id>   # to a specific revision
 ```
 
-Do **not** use `downgrade orders@base` to roll back just the orders module. The `branch_labels` marker names a revision, but `orders@base` resolves to the base of the linear chain that revision sits on, so it rolls back every revision beneath it, including other modules'.
+Do **not** use `downgrade orders@base` to roll back just the orders module. The label names a revision, and `orders@base` unwinds that revision, all of its descendants, and everything beneath it down to the root it sits on, including other modules'. With this graph, `downgrade api_tokens@base` would drop the whole users trunk (users, file_storage, the background_tasks outbox and the initial schema), not only the api_tokens tables.
 
 ### Removing one module's schema
 
-- If the module's revisions are the latest in the chain (nothing from another module sits above them), downgrade to the `down_revision` of the module's *first* revision: `alembic -c host/alembic.ini downgrade <that down_revision id>`.
-- Otherwise write a dedicated migration that drops the module's tables (and anything depending on them). History stays linear and other modules are untouched.
+- If the module's revisions are the latest on their branch (nothing from another module sits above them), downgrade to the `down_revision` of the module's *first* revision: `alembic -c host/alembic.ini downgrade <that down_revision id>`.
+- Otherwise write a dedicated migration that drops the module's tables (and anything depending on them). Other modules are untouched.
 
 ## First migration of a new module
 
@@ -61,7 +91,7 @@ branch_labels = ("orders",)  # ← add this
 depends_on = None
 ```
 
-The marker names the revision so you can target it (for example `alembic upgrade orders@head`); later `orders` revisions do not need it. It does not turn the module into an independent branch.
+The marker names the revision so you can target it (for example `alembic upgrade orders@head`); later `orders` revisions do not need it. It does not turn the module into an independent branch, and it is inherited by every revision stacked on top of it, so put it on the module's own first revision and not on a revision that other modules build on.
 
 ## Alembic environment setup
 
@@ -94,7 +124,7 @@ context.configure(
 
 ## Boot-time migration check
 
-On startup, the framework compares `alembic_version` to the migration head:
+On startup, the framework compares `alembic_version` to **every** head (`resolve_head_revisions` in `simple_module_hosting.migrations`; the history has several, so it never asks for a single head):
 
 - **Development:** prints a warning if the DB is behind. Result stored on `app.state.migration`.
 - **Production:** fails boot with `SM010`. Don't ship a web process that's pointed at an unmigrated DB.
@@ -116,7 +146,7 @@ The dev-mode boot log prints the offending table names. Resolution: run `make mi
 If `invoices` has an FK to `orders_order.id`:
 
 - Alembic will emit `ADD CONSTRAINT` in the invoices table's migration.
-- The migration that creates `invoices_invoice` must come **after** the one that creates `orders_order` in linear history.
+- The migration that creates `invoices_invoice` must come **after** the one that creates `orders_order` in the history (an ancestor, not a sibling branch).
 - `smpy create-module` + `make migration` handle this naturally as long as `depends_on` is correct in `ModuleMeta`.
 
 All tables share the host's single schema on both Postgres and SQLite, so a cross-module FK is just an ordinary same-schema reference (`orders_order.id ← invoices_invoice.order_id`).
@@ -149,7 +179,7 @@ revision A  ← you created
 revision B  ← teammate's branch created
 ```
 
-Both have `down_revision = <previous>`. Alembic will refuse `upgrade head` on a tree with multiple heads. Resolve by:
+Both have `down_revision = <previous>`, so the same branch now has two heads. (Heads on *different* labelled branches are expected and need no merge; `upgrade heads` applies them all.) Resolve a same-branch fork by:
 
 1. `uv run alembic heads` — list them.
 2. `uv run alembic merge -m "merge A and B" <rev-A> <rev-B>` — creates a merge revision with both as parents.
@@ -179,7 +209,7 @@ async def test_migration_up_then_down(tmp_path):
     cfg = Config("host/alembic.ini")
     cfg.set_main_option("sqlalchemy.url", db_url)
 
-    command.upgrade(cfg, "head")
+    command.upgrade(cfg, "heads")
     command.downgrade(cfg, "base")
 ```
 

docs/framework-conventions.md

@@ -284,7 +284,9 @@ not get the row torn out from under them.
 The soft-delete and tenant filters are attached by a `do_orm_execute` listener
 (`simple_module_db.query_filter`). Loader criteria go on for *every* registered
 soft-deletable / tenant-scoped model, not only the entities a statement names,
-and top-level Core tables get an explicit `WHERE`. All of these are filtered:
+and Core tables — at any depth, including outer joins (predicates in `ON`),
+subqueries, CTEs and unions (GH #506) — get explicit predicates. All of these
+are filtered:
 
 ```python
 select(Model)  # entity select
@@ -294,6 +296,8 @@ select(func.count()).select_from(select(Model).subquery())
 select(func.count()).select_from(Model.__table__)  # pure Core
 select(A).outerjoin(B)  # B filtered in the ON clause
 select(func.count()).select_from(A.__table__.join(B.__table__, ...))  # Core inner join
+select(func.count()).select_from(A.__table__.outerjoin(B.__table__, ...))  # B in ON
+union(select(A.__table__.c.id), select(A.__table__.c.id))  # Core, nested
 ```
 
 Before GH #332 only the first two were — the rest returned trashed rows, and on

docs/framework/multi-tenancy.md

@@ -37,15 +37,43 @@ statement names, so all of these are scoped (#332):
 - subqueries: `select(Doc.id).where(...).exists()`, a bare Core
   `exists().where(Doc.x == ...)`, `in_(select(Doc.x))`, scalar subqueries;
 - counts: `select(func.count()).select_from(Doc)`, counts over a subquery;
-- Core statements on `Doc.__table__` at the top level — `select`, `update`,
-  `delete` get `WHERE tenant_id = …` (and `is_deleted IS false` on reads),
-  `insert` is stamped.
-
-With no tenant under strict mode, a statement that *names* a tenant model or
-table — or reads one through a bare Core `exists()` — raises; an indirect ORM
-reference (a join, a subquery) matches nothing. The one-time cost is a scan of
-the WHERE/column clauses for `exists()` (about 10 µs per statement), skipped
-when no tenant-scoped or soft-deletable model is installed.
+- Core statements on `Doc.__table__` (or `Doc.__table__.alias()`) at any
+  depth (#506): the top-level `select`, `update` and `delete` target get
+  `WHERE tenant_id = …` (and `is_deleted IS false` on reads), `insert` is
+  stamped, and so is every other Core reference — a subquery, a CTE, a
+  scalar or `exists()` subquery (also inside a join's `ON`), each member of a
+  `union` / `intersect` / `except`, a table an `update()`/`delete()` joins in
+  through its `WHERE`, and the `select` behind `insert().from_select()`;
+- Core **joins**: an outer join's nullable side gets its predicates in the
+  `ON` clause, so `tag.outerjoin(link, …)` keeps tags without links and never
+  counts another tenant's links. A `full=True` join puts both sides'
+  predicates in `ON` and adds `WHERE (pred OR <side's primary key> IS NULL)`
+  per side, which is the same result as filtering both inputs first. The
+  `select(...).outerjoin(Doc.__table__, onclause)` form is scoped the same
+  way. A `full=True` step there also preserves the FROM built so far: the
+  plain tables on its left side get `(pred OR <pk> IS NULL)` in that step's
+  `ON` and in `WHERE`, so rows padded on the left survive instead of the
+  join collapsing into a RIGHT join. Without an explicit `onclause` the one
+  SQLAlchemy infers from the foreign keys is used. Two shapes still
+  over-filter (never leak): an ORM entity on a FULL step's left side, whose
+  loader criteria sit in `WHERE`, and a table with no primary key, whose
+  padding cannot be told apart from a real row.
+
+With no tenant under strict mode, a statement that *names* a tenant model, or
+reads a tenant table through Core anywhere in it, raises; an indirect ORM
+reference (a join, a subquery) matches nothing. The cost is one walk of the
+statement looking for a plain (non-ORM) reference to a scoped table (about
+20–40 µs for a typical ORM select); only a statement that has one is cloned
+and rewritten. Both are skipped when no tenant-scoped or soft-deletable model
+is installed.
+
+Not covered, by design:
+
+- raw SQL — `text("SELECT … FROM docs_doc")` is opaque to the filter; scope it
+  yourself;
+- soft-delete on an `update()`/`delete()` (its target *or* any table it reads):
+  writes may touch trashed rows — that is how restore and purge work. Tenant
+  scoping still applies to them.
 
 ### One session, one tenant