diff --git a/.gitea/workflows/release.yml b/.gitea/workflows/release.yml index 3e7d544..1ccaf1b 100644 --- a/.gitea/workflows/release.yml +++ b/.gitea/workflows/release.yml @@ -69,8 +69,9 @@ jobs: if: always() run: | set -uo pipefail - # Cleanup, and the runner's Docker config is shared (AGENTS.md) — a lost race here must not - # fail a release that published, nor skip the overview job that follows. + # This is cleanup: it must not fail a release that published, nor skip the overview job + # that follows. The runner's Docker config is shared, so a logout can lose a race + # (docs/decisions.md → "CI docker logins share the runner host's Docker config"). docker logout gitea.larvit.se || true docker logout docker.io || true diff --git a/AGENTS.md b/AGENTS.md index b1752fc..4128197 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,8 +10,9 @@ change, sweep the prose you touched — this file, `README.md`, the example READ comments — and cut it back to what a competent reader could not infer: - **Delete history.** Git holds it. No "this moved from X", "used to be Y", "was tried and - rejected", "(declined twice)", dated changelog entries, or the symptom that prompted a fix. Record - the decision and the reason it *currently* turns on, nothing else. + rejected", "(declined twice)", dated changelog entries (a decision's date in `docs/decisions.md` + is not one), or the symptom that prompted a fix. Record the decision and the reason it + *currently* turns on, nothing else. - **Delete restatement.** A comment that says what the adjacent line says, a doc paragraph that re-explains a table above it, a file-map entry that expands the filename. The fix is deletion, not trimming. @@ -67,288 +68,72 @@ branch, create a PR and merge it when the CI/CD turns green. **fails loud at boot/discovery** rather than sandboxing at runtime. Runtime crash-isolation is a deliberate **non-goal**. -## Deliberate architectural deviations (don't re-flag) +## Decisions -Intentional, reasoned choices — an architecture review should honor them, not re-raise them. -Revisit only if the stated reason stops holding. +Each title below is an entry in `docs/decisions.md`, with its reasoning and the premise it rests +on; a review honours them. ### Structure & contracts -- **`src/` is grouped by concern**, not flat — `http/`, `auth/`, `i18n/`, `plugin-host/`, `ui/`, - and `test-tooling/` (modules only tests import), with `server.ts`/`config.ts`/`logger.ts` and - the topology-guard `*.test.ts` at the root; tests are co-located. Add a new module to the folder owning its concern. The core ships **no domain - screens** — even the admin GUI is a drop-in plugin (`examples/plugins/admin/`). -- **Plugins and config import the host only through a barrel** — `@plainpages/plugin-api` → - `plugin-api/index.ts` → `src/plugin-host/plugin-api.ts`, `#menu-config` → `src/ui/menu-config.ts`, - never a relative `../../src/*` path. These two barrels are the whole contract surface; don't "fix" - either back to a relative path. Three consequences: - - `@plainpages/plugin-api` re-exports the Ory client types (`KratosAdmin`/`KetoClient`/`HydraAdmin` + their - DTOs and error classes), so those shapes are **contract-visible** — changing them needs a major - `apiVersion` bump, not a free refactor. - - **The barrel is a package, not a `#`-import, so a plugin folder may carry its own - `package.json`** and depend on npm packages (README → Plugin dependencies). The Dockerfile links - it into `/node_modules`, above every plugin scope. Never let a copy reach a plugin's own - `node_modules`: two instances of the barrel break `instanceof` across the boundary, which - `plugin-api.test.ts` guards by asserting both paths reach one module. - - **Plugin storage hands over credentials, not a client** (README → Plugin storage). The host takes - `postgres` to run the provisioning DDL, and `storage-provisioning.ts` is the only module importing - it — `storage.ts` beside it stays pure so `web` never loads a driver (`src/postgres.test.ts` guards - both halves, because one value imported from the wrong module breaks it invisibly). It is never - re-exported through the barrel, so no driver shape enters the contract. Three properties hold the design together, so - don't trade one away in isolation: passwords are `HMAC-SHA256(PLUGIN_DB_SECRET, id)` rather than - stored, which is what keeps the host stateless — whoever holds that secret holds every plugin - database, so it ranks with the DB password itself; the provisioning DSN reaches `bootstrap` only - (`src/compose.test.ts` guards the split); and provisioning never drops anything, so uninstalling a - plugin cannot destroy data — boot logs the orphans instead. Because the host's copy sits in the - ambient `/node_modules`, a plugin can `import "postgres"` without declaring it — incidental, not a - packaging promise, and a plugin must still depend on its own driver. - - **Plugin settings are declared, not discovered** (README → Plugin settings). `settings.ts` is pure and - takes the env as an argument, so the whole matrix unit-tests without a stack. Four rules carry the - design: the prefix is `PLUGIN_SETTING_`, never bare `PLUGIN_`, because a plugin id `db` with key - `url` would otherwise name the host's own `PLUGIN_DB_URL`; keys are camelCase so the - `camelCase → SNAKE_CASE` mapping is total and no two keys collide, with the residual cross-plugin - collision caught by `findConflicts` (`plugin-host/conflicts.ts`); `required` and `default` are mutually exclusive, which is what - lets `SettingsOf` type a declared key as present rather than `T | undefined`, so no plugin author - casts; and a secret's value reaches the plugin but never a log, an error or `ctx.declaredSettings` - — not even as a mask or a length. An author mistake is refused at discovery, a bad operator value - refuses the boot, and a stray `PLUGIN_SETTING_` variable only warns (the orphan-database precedent). -- **The trust boundary is the `web` process, not the plugin.** Per-plugin databases and roles bound - *accidents*, not hostile plugins: `PLUGIN_DB_SECRET` is in `web`'s environment during `onBoot`, and - a plugin already holds `ctx.system`'s Ory admin clients — so cross-plugin DB isolation is - containment, and README says so rather than implying a sandbox. Consistent with priority #7 - (crash-isolation is a non-goal). `server.ts` still deletes the secret from `process.env` right - after `loadConfig`, which is before discovery imports any plugin module — the ordering is the whole - point, so move it earlier if anything, **never later**. **Valid while plugins are - operator-installed code, not third-party uploads.** -- **`ory/postgres/init/init.sql` is the only home for the Ory databases' ACL** — don't re-assert the - `REVOKE CONNECT` from `bootstrap`. `REVOKE` only *warns* when the caller doesn't own the database, - so under the least-privilege provisioning account the README recommends it would report success - while changing nothing, and it hard-fails whenever `PLUGIN_DB_ADMIN_URL` names a server with no - `kratos`. It runs only on **first init**, so a revoke added to it later never reaches a volume that - already exists — `docker compose down -v` is the dev remedy, a deployed install needs a migration. -- **`bootstrap.ts` stays under `src/auth/`** even though it now provisions plugin databases as well - as seeding Ory. It is the one-shot service's entrypoint, not an auth module; moving it to - `src/bootstrap.ts` would edit `compose.yml`, five e2e compose files and `src/compose.test.ts` for a - rename. Reconsider when a third seeding concern lands. -- **`BootContext.storage` keeps all six credential fields, and there is no `onShutdown` hook.** Adding - to the context costs a minor bump and removing one a major, so the shape errs small elsewhere. Pools - handed to a plugin are reaped on process exit — revisit if a plugin ever needs an orderly drain. -- **`config/` is still a plain dir — no `package.json` of its own**, or `#menu-config` resolves - against that instead and boot fails loud. An operator's menu override has no use for - dependencies; if that changes, it needs the same package treatment. -- **A plugin `package.json` without `"type": "module"` is refused, not warned.** Allowing it costs a - warning and a re-parse per file, not a break — Node detects module syntax, so even a `.js` helper - loads — and an operator on a read-only third-party mount cannot apply the remedy. Refused anyway - because the direction is safe: refuse→warn relaxes freely, warn→refuse breaks installed plugins. - **Valid while nothing is installed in the wild.** -- **`examples/` mirrors the drop-in mount dirs** — `examples/plugins//` copies to - `plugins//`, `examples/config/menu.ts` to `config/menu.ts`. Both mirrors are in - `tsconfig.include` and resolve the host through the barrels, so each typechecks in place *and* - copies across unchanged. Never commit real plugins/config into the root mount dirs — they ship empty. -- **`ctx.chrome` is lazily memoized — do not make it unconditional** or move it into the base request - context. It protects the I/O-free hot path on the public, bot-hit landing (`/`). -- **A plugin-owned render always runs on that plugin's context.** The landing slots (`home`, - `dashboard`) and an `onRequest` short-circuit build their context with `contextFor(pluginId)` - exactly as a plugin route does — otherwise `ctx.t` is the core translator and the plugin's own keys - render as bare keys on the pages it owns. -- **Email is delegated to Kratos** (it renders + sends recovery/verification mail); `web` never - touches SMTP. Customization is Kratos' `courier.template_override_path`, not app code. +- `src/` is grouped by concern, and the core ships no domain screens +- Plugins and config import the host only through the two barrels +- The Ory client types are contract-visible through the barrel +- The barrel is a package, so a plugin folder may carry its own `package.json` +- Plugin storage hands over credentials, not a client +- Plugin settings are declared, not discovered +- The trust boundary is the `web` process, not the plugin +- `ory/postgres/init/init.sql` is the only home for the Ory databases' ACL +- `bootstrap.ts` stays under `src/auth/` +- `BootContext.storage` keeps all six credential fields, and there is no `onShutdown` hook +- `config/` is a plain dir with no `package.json` of its own +- A plugin `package.json` without `"type": "module"` is refused, not warned +- `examples/` mirrors the drop-in mount dirs +- `ctx.chrome` is lazily memoized, never built unconditionally +- A plugin-owned render always runs on that plugin's context +- Email is delegated to Kratos ### Authorization -- **Vocabulary: `User` → `Group` → `Permission`, and there is no `Role`.** Keto ships no namespaces — - all four in `ory/keto/namespaces.keto.ts` are ours. A permission is one operation ("read shifts"); - a role is a *bundle*, which here is just a group with several grants (groups nest). Ory's own - "permission" (the `Resource` `permits`: view/edit/delete) is the separate per-row tier. -- **A permission name is always `:`** — `scheduling:read`, `users:write`. A bare - word names *who someone is* (a role), and roles are groups here. **Enforced at discovery** - (`isValidPermissionName` in `plugin-host/plugin.ts`, checked by `shapeError` over every route, nav and profile-menu - `permission` and every declared name), fail-loud like any other manifest rule — not only in the - admin GUI, which an operator removes by not copying it in. - - **Names are authored in plugin code; only grants live in Keto.** The host collects every installed - plugin's declarations into one catalog (`declaredPermissions` → `ctx.declaredPermissions`), and - that catalog *is* the list the admin screens offer. Hence **no Permissions admin screen**: nothing - in a GUI invents a name, and holding one is a property of a user or group, edited as a checkbox - list there. A Keto tuple naming something no installed plugin declares gates nothing, is not - offered, and is never revoked by an unrelated save — the picker only speaks for what it showed. - - `` is **global, not plugin-scoped** (hence `oauth2-clients`, not `clients`): users are - the *host's*, and cross-plugin sharing is a goal. Cost: collision-freedom is a convention rather - than structural. Accepted — the alternative penalizes the sharing case. - - **Declaring a permission stays optional.** Mandatory declaration would let `findConflicts` see all - overlaps, but would then warn on exactly that legitimate sharing case. Shape is enforced; - declaration is not. - - `ADMIN_PERMISSIONS` **defaults to empty**, and **an unusable value is dropped with a warning, - never fatal** — fail-loud belongs at the manifest boundary where a developer authored the - mistake, whereas `bootstrap` gates `web`, so refusing operator env takes the whole stack down - (`e2e-tests/compose.auth.yml` seeds a bad value to prove the container survives one). The seed is - a function of what `bootstrap` discovers, so a plugin dropped in after first boot needs - `docker compose up -d`, not `restart web`. `bootstrap`'s matching `./plugins` mount belongs in - `compose.override.yml` and nowhere else: in the base file it would desynchronise prod and collide - with the e2e stacks, which bind individual plugins *inside* `/app/plugins` (a nested mount into a - read-only parent is EROFS and the container never starts). Valid while bootstrap is the only - writer of grants. - - **`actionForMethod` is plugin-local and must not migrate into `@plainpages/plugin-api`.** Inside the admin - example it keeps the route table and the in-handler guard deriving from one function, so 29 routes - × 2 gate sites cannot drift. Generalised, it would make authorization a function of the transport - verb — a route table must answer "what does this need?" on its own. -- **A gate is one of three, named exactly once, and `session` is a first-class one.** A route or nav - node names exactly one of `public`, `session`, `permission` — discovery refuses none, two, and a - flag spelled anything but `true`, so a forgotten gate fails the boot rather than publishing a page. - `src/auth/gate.ts` is the one home of the rule the plugin router, the host's own route table and - the menu all read. Exactly-one-gate is a discovery-time rule on manifests, not a runtime - invariant: `allows({}, user)` stays open **by design**, because the central override's `groups` - builds header nodes that carry no gate. Making `allows` fail closed would hide every - operator-grouped section. `session` exists because a plugin whose data is - the visitor's own — their upstream account, their own tokens — has no distinction a permission could - name; the alternative, granting every newly registered user a permission, couples the identity - lifecycle to a Keto write that nothing retries when it fails. A page scoped to "mine" joins on - `ctx.user.id`, never the email — an address is user-changeable and can be reassigned to someone - who would then inherit the previous holder's rows. -- **The reference plugin's two shift pages duplicate a view model and markup on purpose.** An example - is read far more often than it is changed, and each page reads top to bottom on its own. **Valid - while `examples/plugins/scheduling` stays a teaching artifact rather than a maintained product.** -- **A `:read`-only holder must never be shown a write affordance.** The list/detail models carry - `canWrite` and the views drop create/save/delete/add/remove; the permission picker still renders, - disabled, because *seeing* who holds what is the point of `:read`. A **write-intent GET** (a create - form, a delete-confirm page) is the exception to `actionForMethod` and gates on `:write`. Two - grant-specific guards go with it: you cannot revoke your own **direct** grants (self-lockout would - need a `curl` against Keto to undo), and a permission held *through a group* renders - ticked-but-disabled, because unticked stated the opposite of the truth. The group paths are - guarded too: unticking a permission on a group you are in, removing yourself (or a nested group you - are in) from its members, or deleting the group is refused when you would lose a permission you - don't hold directly. -- **`users:write` and `groups:write` are equivalent to full administrative access**: `groups:write` - adds you to any group, including one holding every permission; `users:write` mints a recovery code - for any account. The containment the split buys is real on the **read** half only (`users:read` is - a safe helpdesk grant). Don't let the per-resource naming imply otherwise in docs. -- **The CSRF token is deliberately unbound to the session** (`src/auth/csrf.ts`; README → Security - model): the usual cost of a signed double-submit token, covered by `SameSite=Lax` and HSTS. - **Valid while the session cookie stays `SameSite=Lax` or stricter.** -- **Plainpages says "user" everywhere; Ory's word is "identity".** House style, not a renamed - concept. The single exception is the `Identity` DTO in `src/auth/kratos-admin.ts`, which mirrors - Kratos' wire shape — don't rename it. +- The vocabulary is `User` → `Group` → `Permission`, and there is no `Role` +- A permission name is always `:`, enforced at discovery +- Permission names are authored in plugin code; only grants live in Keto +- A permission's `` is global, not plugin-scoped +- Declaring a permission stays optional +- `ADMIN_PERMISSIONS` defaults to empty, and an unusable value is dropped with a warning +- `actionForMethod` is plugin-local and stays out of `@plainpages/plugin-api` +- A gate is one of `public`, `session` or `permission`, named exactly once +- The reference plugin's two shift pages duplicate a view model and markup +- A `:read`-only holder is never shown a write affordance +- `users:write` and `groups:write` each amount to full administrative access +- The CSRF token is unbound to the session +- Plainpages says "user" everywhere, where Ory says "identity" ### i18n -- **The locale lives in the URL, never in a cookie.** `?locale=sv-SE` → `Accept-Language` → `en-US`, - and when the URL asked for one the host carries it onto the links it renders. A cookie would make a - page's language invisible in its address and unshareable; the cost is that a plugin wraps its own - hrefs. Matching is exact on a full tag (`sv-FI` ≠ `sv-SE`), except that a lone language from - `Accept-Language` takes the first regional catalog for it. -- **The core building blocks carry the locale; a plugin doesn't have to.** The shell, `pagination`, - `filter-bar`, `data-table`, `auth-card`, `flow-body`, `field` and `menu` wrap every href in - `localeHref`; nav and sign-in are wrapped in `chrome.ts`; the two GET forms carry it as a hidden - `locale` input, since a GET submit replaces the whole query string. **A form's `action` counts as a - link** — sign-out, consent and auth-card forms carry it too, or picking a language and then saving - anything drops back to `Accept-Language`. The obligation stays on the building block, never on each - call site. `ctx.localeHref` remains for hrefs a plugin's own markup emits. The one round-trip that - cannot carry it is the Kratos sign-in POST (absolute off-site URL). -- **`locale` is a host-owned query param** — in `parseListQuery`'s reserved set, so a localized list - page doesn't hand a plugin a phantom `locale` filter. The i18n view locals (`t`, `locale`, `locales`, - `localeHref`, `localeParam`, `localeSwitch`, `dir`) are likewise reserved, merged after a handler's - `data` so a collision loses the key instead of breaking the shell — as is `assetHref`. -- **Catalogs are checked at boot, not at render.** Every locale is compared against its set's `en-US` - — keys, string-vs-plural kind, and the plural categories `Intl.PluralRules` requires — and a - mismatch stops startup. A plugin may ship fewer locales than the host (its strings fall back to - `en-US` per key), never one the host lacks. -- **`locales/` at the repo root is a drop-in mount**, like `plugins/` and `config/` — `locales/.ts` - for the core, `locales/plugins//.ts` for a plugin; a new tag adds a language, an existing - one replaces that catalog wholesale. Adding a language must never require forking the image. The - SHIPPED `en-US` stays the parity baseline even when the mount replaces it, so a mounted catalog is - checked rather than trusted (one compared only against itself would boot green with the whole UI - rendering keys). -- **The language picker is on every page, POST-rendered ones included.** A POST-rendered URL often - answers no GET (`POST /admin/users/:id/recovery`), so the host resolves the picker's target - (`app.ts` → `switchBase`): this path when it answers GET, else the same-origin Referer, else `/`. - Accepted cost: switching language there leaves that POST's own result behind. Valid while the picker - is expected on literally every page — if that softens, hiding it after a POST is simpler. -- **An unknown translation key renders as itself.** That single rule lets a nav label, branding, or a - menu `rename` be either a key or plain text without a second field or a migration. Don't "fix" it - into a loud failure: a manifest with plain labels must keep working. -- **`t()` returns raw text; the view escapes it.** Messages go through `<%= %>` like any other value; - one carrying markup uses `<%- %>`, and then its `{{vars}}` are escaped at the call site. Don't move - escaping into `t()` — every other value in a view would become the odd one out. -- **RTL is out of scope until there is a real use case.** `textDirection` sets `` because - that is free and correct, but the stylesheet keeps physical `left`/`right` properties; a genuine RTL - locale needs those moved to logical ones first. Valid while no deployment needs an RTL language. +- The locale lives in the URL, never in a cookie +- The core building blocks carry the locale, so a plugin doesn't have to +- `locale` and the i18n view locals are reserved by the host +- Catalogs are checked at boot, not at render +- `locales/` at the repo root is a drop-in mount +- The language picker is on every page, POST-rendered ones included +- An unknown translation key renders as itself +- `t()` returns raw text, and the view escapes it +- RTL is out of scope until there is a real use case ### UI -- **A dropdown is a `