security/tenant-isolation #28

Merged

psycodepath merged 12 commits from security/tenant-isolation into main 2026-07-08 19:12:17 +00:00

Conversation 0 Commits 12 Files changed 60 +1377 -221

psycodepath commented 2026-07-08 19:04:51 +00:00

Owner

Copy link

Fixes the Critical and High findings from the codebase security review.

Critical

• Tenant path traversal + auto-provisioning (2a98ac6): the Host-header
  subdomain flowed unvalidated into a filesystem path and auto-created tenant
  DBs. Now charset-validated (invariant on TenantContext), must be registered
  via tenant:save, unknown → 404.
• Session cookie scoping + fixation (49a3313): cookie was shared across all
  tenant subdomains. Now scoped to the exact host, SameSite=Lax, Secure by
  real HTTPS, session_regenerate_id() on login.
• Authelia trusted-header / dev bypass (c14e95f): identity headers now only
  honored from a configured trusted proxy (AUTHELIA_TRUSTED_PROXIES); the
  Host-based localhost auto-login is removed.
• JSON-LD </script> XSS (89a3190): public shared-recipe page now encodes
  with JSON_HEX_*.
• SSRF / local-file read in recipe importer (8ab820f): new SsrfGuard +
  SafeUrlFetcher (scheme allow-list, private/reserved IP blocking, per-hop
  redirect validation, IP pinning, size cap).

High

• App-wide CSRF protection (438d3f3): per-session token, front-controller
  enforcement, hidden form fields + HTMX header; /api/* exempt.
• Recipe edit data loss (a602d82): edits preserved rating/category and no
  longer re-emit RecipeCreated.
• Catalog rename (5bd3430): upsert on id, not name.
• Taxonomy worker (4218b24): atomic claim, requeue-on-failure, dedup, and
  UNIQUE(stemmed_name) (migration 020).
• Gemini key transport, login timing, groups/id (b4f8c7a).
• LDAP logging via PSR-3 (7cb1c69).
• Tenant secret file/dir permissions (db64c26): 0600/0700, no more 0777.

Deploy notes

• Existing family subdomains must be registered with tenant:save (hard-reject policy).
• Set AUTHELIA_TRUSTED_PROXIES to the proxy IP in production (see .env.example).
• Migration 020 adds UNIQUE(stemmed_name) and dedupes existing catalog rows.

Verification

160 tests pass; phpstan level 8 clean.

Fixes the Critical and High findings from the codebase security review. ## Critical - **Tenant path traversal + auto-provisioning** (`2a98ac6`): the Host-header subdomain flowed unvalidated into a filesystem path and auto-created tenant DBs. Now charset-validated (invariant on `TenantContext`), must be registered via `tenant:save`, unknown → 404. - **Session cookie scoping + fixation** (`49a3313`): cookie was shared across all tenant subdomains. Now scoped to the exact host, `SameSite=Lax`, `Secure` by real HTTPS, `session_regenerate_id()` on login. - **Authelia trusted-header / dev bypass** (`c14e95f`): identity headers now only honored from a configured trusted proxy (`AUTHELIA_TRUSTED_PROXIES`); the Host-based localhost auto-login is removed. - **JSON-LD `</script>` XSS** (`89a3190`): public shared-recipe page now encodes with `JSON_HEX_*`. - **SSRF / local-file read in recipe importer** (`8ab820f`): new SsrfGuard + SafeUrlFetcher (scheme allow-list, private/reserved IP blocking, per-hop redirect validation, IP pinning, size cap). ## High - **App-wide CSRF protection** (`438d3f3`): per-session token, front-controller enforcement, hidden form fields + HTMX header; `/api/*` exempt. - **Recipe edit data loss** (`a602d82`): edits preserved rating/category and no longer re-emit `RecipeCreated`. - **Catalog rename** (`5bd3430`): upsert on `id`, not `name`. - **Taxonomy worker** (`4218b24`): atomic claim, requeue-on-failure, dedup, and `UNIQUE(stemmed_name)` (migration 020). - **Gemini key transport, login timing, groups/id** (`b4f8c7a`). - **LDAP logging via PSR-3** (`7cb1c69`). - **Tenant secret file/dir permissions** (`db64c26`): `0600`/`0700`, no more `0777`. ## Deploy notes - Existing family subdomains must be registered with `tenant:save` (hard-reject policy). - Set `AUTHELIA_TRUSTED_PROXIES` to the proxy IP in production (see `.env.example`). - Migration `020` adds `UNIQUE(stemmed_name)` and dedupes existing catalog rows. ## Verification 160 tests pass; phpstan level 8 clean.

psycodepath added 12 commits 2026-07-08 19:04:51 +00:00

fix(kernel): validate and require registration for tenant subdomains ... 2a98ac6bc8

The tenant id was taken from the Host-header subdomain and used
unvalidated both as a filesystem path (var/tenant_<id>.sqlite) and as
the tenant selector. An attacker-controlled Host could therefore
traverse the filesystem, and any well-formed subdomain silently
auto-provisioned a fresh tenant database, undermining tenant isolation.

- TenantContext enforces ^[a-z0-9-]{1,63}$ as a constructor invariant
  (empty string remains the "no tenant" sentinel), so a malformed id
  can no longer be constructed.
- SubdomainTenantDetector now requires a present subdomain to be both
  charset-valid and registered via `tenant:save`; otherwise it throws
  UnknownTenantException. A bare host (e.g. localhost) still maps to the
  default tenant for local development.
- The front controller maps UnknownTenantException to a 404 so
  unregistered subdomains cannot auto-provision.
- TenantAwarePdoFactory re-validates the id at the filesystem sink
  (defense in depth) and creates var/ as 0750 instead of 0777.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(kernel): harden tenant session cookie scoping and prevent fixation ... 49a3313371

The session cookie was scoped to the registrable domain (.example.com)
in production, so it was shared across every tenant subdomain — one
family's cookie was sent to another family's tenant. It also lacked a
SameSite attribute, only set Secure when APP_ENV=production, and the
session id was never regenerated on login (session fixation).

- Scope session.cookie_domain to the exact host so cookies never bleed
  across subdomains (one subdomain == one tenant).
- Set session.cookie_samesite=Lax to mitigate CSRF.
- Set Secure based on actual HTTPS (request scheme, HTTPS server param,
  or X-Forwarded-Proto), not the env name.
- Add TenantAwareSessionManager::regenerateId() and call it from
  LoginAction after successful authentication to prevent fixation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(kernel): gate Authelia header auth on trusted proxy, drop dev bypass ... c14e95f5bc

AutheliaAuthenticationService trusted the Remote-User / X-Forwarded-User
identity headers unconditionally, so if the app were reachable directly a
client could set X-Forwarded-User to impersonate any user. It also
auto-logged in as dev-user/developers when HTTP_HOST started with
"localhost:" — and HTTP_HOST is client-controlled, so that dev bypass was
forgeable in a production-only service.

- Only honor identity headers when the connecting peer (REMOTE_ADDR) is a
  configured trusted proxy; otherwise return null. Supports IPv4/IPv6
  exact IPs and CIDR ranges.
- Remove the HTTP_HOST/localhost dev bypass entirely (dev uses the local
  session strategy and never reaches this service).
- Trusted proxies come from AUTHELIA_TRUSTED_PROXIES (comma-separated),
  defaulting to loopback + private ranges to match the container topology.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(meals): prevent JSON-LD script breakout XSS on public shared recipe ... 89a3190b32

The shared-recipe page emits recipe-derived JSON-LD raw inside a
<script> block and encoded it with JSON_UNESCAPED_SLASHES, so a recipe
field containing "</script><script>...</script>" (recipe content is
often imported from arbitrary external URLs) broke out of the block and
executed on this public, unauthenticated route.

Encode with JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT
so <, >, & and quotes are hex-escaped and cannot terminate the script
element. Adds a regression test injecting a </script> payload.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(meals): block SSRF and local-file reads in recipe URL importer ... 8ab820fbbf

Both recipe importers fetched a fully user-supplied URL with
@file_get_contents and no validation, allowing SSRF (cloud metadata at
169.254.169.254, internal services), redirect-based bypass, and local
file / wrapper reads (file://, php://).

- Add SsrfGuard: allows only http/https, resolves the host and rejects
  any private/loopback/link-local/reserved address (incl. CGNAT,
  IPv4-mapped IPv6, IPv6 ULA/link-local), returning a vetted IP.
- Add SafeUrlFetcher (UrlFetcherInterface): fetches via cURL, follows
  redirects manually re-validating every hop, pins the connection to the
  vetted IP to close the DNS-rebinding window, forces http(s) protocols
  and TLS verification, and caps the response size.
- Both importers now fetch through UrlFetcherInterface instead of
  file_get_contents; DI binds it to SafeUrlFetcher.

Importer tests now stub the fetcher (no more local-file reads); adds
SsrfGuard unit tests covering blocked ranges and schemes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

feat(kernel): add app-wide CSRF protection for state-changing requests ... 438d3f3957

No CSRF protection existed on any cookie-authenticated POST (login,
settings/password change, logout, and all task/grocery/meals mutations),
so a malicious page could forge them against a logged-in user.

- CsrfTokenManager issues a per-session token stored in the tenant-scoped
  session and validates submissions in constant time (hash_equals).
- The front controller rejects unsafe methods (POST/PUT/PATCH/DELETE)
  with a missing/invalid token via a 403. The token is accepted from the
  X-CSRF-Token header or a _csrf_token body field. /api/* routes are
  exempt: they authenticate with an X-API-Key header, not an ambient
  cookie.
- The token is exposed to Twig as `csrf_token`. Plain POST forms embed it
  as a hidden field; a global htmx:configRequest handler attaches it as
  the X-CSRF-Token header to every HTMX request (covering non-form
  hx-post buttons/inputs too).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(meals): preserve rating/category on recipe edit, stop re-emitting RecipeCreated ... a602d820c3

UpdateRecipeHandler rebuilt the recipe with Recipe::create(), which
(a) omitted category and rating so save()'s upsert wiped both on every
edit, and (b) recorded a RecipeCreated event, re-triggering creation
listeners (e.g. grocery taxonomy enqueue) on each edit.

- Add Recipe::updateDetails() to mutate the editable fields in place,
  preserving category and rating and recording no event.
- UpdateRecipeHandler now loads the existing aggregate (throws if
  missing) and mutates it instead of recreating it.

Test seeds a rated, categorised recipe and asserts both survive the
edit and that no domain event is dispatched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(grocery): fix catalog product rename (upsert on id, not name) ... 5bd34302ab

SqliteCatalogProductRepository::save upserted ON CONFLICT(name), but the
aggregate carries a stable id. Renaming a product therefore tried to
INSERT a row whose id already existed, hitting "UNIQUE constraint failed:
grocery_catalog.id" and leaving the old-named row orphaned.

Upsert ON CONFLICT(id) and update name in the SET clause instead. The add
flow (RegisterCatalogProductHandler) and TaxonomySuggestor already
pre-check name/stemmed-name uniqueness, so no caller relied on the old
merge-by-name behaviour. Adds a repository test covering rename and field
updates against real SQLite.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(grocery): make taxonomy worker concurrency-safe and dedup enqueue ... 4218b24c0b

The async taxonomy pipeline could process the same pending items twice
under overlapping worker runs (cron overrun, manual + scheduled),
producing duplicate catalog rows and doubled Gemini spend, and the
enqueue listener inserted a row per ingredient with no dedup or
transaction.

- Migration 020 collapses catalog rows sharing a stemmed_name and adds a
  UNIQUE index on grocery_catalog(stemmed_name), so duplicates are
  impossible even if two writers race.
- ProcessPendingTaxonomyCommand claims pending items under a single
  BEGIN IMMEDIATE transaction (select + delete) so a concurrent worker
  blocks and then finds an empty queue; the slow Gemini calls run after
  commit. Items whose processing throws are requeued for the next run
  instead of being silently dropped.
- RegisterRecipeIngredientsInCatalogListener now runs in a transaction
  and skips blank names, in-batch duplicates, and ingredients already in
  the catalog (injecting the catalog repo + stemmer).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(kernel): harden Gemini key transport and local auth (H1/H4/H5) ... b4f8c7ae74

- H1: send the Gemini API key via the x-goog-api-key header instead of
  the URL query string so it cannot leak into access/proxy logs; assert
  TLS peer/host verification on the request.
- H4: LocalPasswordAuthenticator now always runs password_verify against
  a dummy cost-12 hash when the username is unknown, equalising response
  time to prevent user enumeration.
- H5: stop assigning every local user the privileged-sounding
  'developers' group (no local role source exists and groups are not used
  for authorization) — local users get []. LoginAction no longer
  fabricates a random identity id when the user row is missing; it fails
  the login instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(kernel): route LDAP auth logging through PSR-3 logger (H3) ... 7cb1c69a07

LdapPasswordAuthenticator logged authentication context via a bespoke
helper that wrote to a world-readable var/log/auth.log (directory created
0777) and duplicated every line to error_log, bypassing the injected
Monolog logger used everywhere else.

Inject Psr\Log\LoggerInterface and route log() through it (stdout JSON via
Monolog), removing the 0777 directory creation and the plaintext auth.log
file entirely.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

fix(kernel): restrict permissions on tenant secrets and var dirs (H2) ... db64c26a55

var/tenants.json holds tenant secrets (Gemini API keys, LDAP bind
passwords) in cleartext, and several code paths created var/ directories
as 0777 (world-writable/readable), so any local read access disclosed
every tenant's third-party credentials.

- Write var/tenants.json and chmod it to 0600 on every save.
- Create var/ directories as 0700 instead of 0777 in TenantConfigRegistry,
  TenantAwareMigrationRunner, and TenantAwarePdoFactory.

Per the deployment model (self-hosted, single operator) secrets remain
on disk but are now readable only by the app user; encryption at rest was
deliberately deferred to avoid key-management/recovery complexity.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

psycodepath merged commit 6155d51b1b into main 2026-07-08 19:12:17 +00:00

psycodepath referenced this pull request from a commit 2026-07-08 19:12:19 +00:00

Merge pull request 'security/tenant-isolation' (#28) from security/tenant-isolation into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

psycodepath/hrp!28

No description provided.