diff --git a/CLAUDE.md b/CLAUDE.md index cfbbdf11..f42a751d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -438,6 +438,10 @@ explaining why it was safe. mounted and what it knows, the URL-vs-channel split for panel-to-panel communication, and the persistence key families. Read before building a panel app. Its defect list is `docs/workspace-panel-todo.md`. +- `docs/plugin-route-authorization.md` — **design, not built**: making a plugin's settings routes + owner-only while the plugin itself stays grantable. What the proxy should pass (`isOwner` and a level, + never the role name), why declaring owner-only paths in the manifest beats trusting every plugin to + remember, and the credential-resolution work that has to land first - `docs/secret-store.md` — **design, not built**: moving the encryption and signing keys out of `.env` into a SQLite store, why they cannot live in Postgres, and key rotation. Also records the core/plugin split it assumes — light plus `officer-headscale` is the core; Vaultwarden and the wallet are plugins diff --git a/TODO.md b/TODO.md index b5a41bab..898bf2ab 100644 --- a/TODO.md +++ b/TODO.md @@ -14,6 +14,14 @@ the owner's OS user and can never be granted. Indirection there really is accide ## Multi-user +- [ ] **A plugin's settings routes are reachable by anyone the plugin is granted to.** Jellyfin's + `/_config` stores a server's admin token and drives provisioning, and a Member granted the plugin + can reach it. Agreed 2026-08-21 that the plugin should enforce this from facts the platform passes + (`isOwner` plus the effective level, never the role name), with a proposal to declare owner-only + path prefixes in the manifest so forgetting the check is not a silent hole. **Blocked on credential + resolution:** connections are per-user, so locking `/_config` today leaves a Member with a plugin + that only shows a connect form they may not submit. See `docs/plugin-route-authorization.md`. + - [ ] **`deprovisionOsAccount` is written but has never run against a real account.** Landed 2026-08-12 in `os-user-deprovision.ts` and wired into `deleteUserHandler`, which now refuses to delete the row when the Linux teardown fails — so a failure is retryable instead of forgotten. Only the pure guards diff --git a/docs/plugin-route-authorization.md b/docs/plugin-route-authorization.md new file mode 100644 index 00000000..fc76f24c --- /dev/null +++ b/docs/plugin-route-authorization.md @@ -0,0 +1,131 @@ +# Plugin route authorization + +**Design, not built.** Agreed 2026-08-21, deferred deliberately. Nothing here is implemented. + +The question: a plugin's **settings** routes should be reachable only by the owner, even when the plugin +itself is granted to other accounts. Jellyfin is the case that raised it — connecting a server, storing +its admin token and provisioning accounts is not something a Member should reach, but Jellyfin as a whole +is exactly the sort of plugin a Member should have. + +## The shape we agreed + +Authorization stays where the platform already puts it — the gate answers "may this account reach this +plugin, and at what level" — and the plugin decides what that means **inside** its own surface. That is +not a new principle. `src/servers/plugins/manifest.ts` already says it, in the comment refusing an +`ownerOnly` field on a permission: + +> Every plugin permission is grantable, per role, at read or write … Finer visibility — whose rows a +> member sees, what a read means for this plugin's data — is the PLUGIN's business and lives in its own +> queries. + +"Which of my routes are administrative" is the same kind of question as "whose rows does a read return". +It belongs to the plugin, because only the plugin knows that `/_config` is settings and `/_jf` is bytes. + +So: **the platform tells the plugin who is asking; the plugin decides what they may do.** + +### What gets passed, and what does not + +Today the proxy sends exactly one fact (`src/servers/sidecar/create-proxy.ts`): + +``` +X-Officer-User: +``` + +The extension is one or two more headers alongside it. Two points about how, both load-bearing: + +**Pass derived facts, not the role name.** The instinct is to send `X-Officer-Role: Super Admin`. Don't. +Roles are the platform's vocabulary and they are not frozen — `Admin`, `Member`, `Developer` today, with +grants keyed on the role string in `role_permissions`. A plugin that branches on a role name gives every +plugin repository an opinion about that vocabulary, and renaming a role silently changes authorization +in code the platform does not own and cannot see. + +Send instead what a plugin can act on without knowing anything: + +``` +X-Officer-User: +X-Officer-Owner: 1 | 0 — is this the server owner +X-Officer-Level: none|read|write — this account's effective level for THIS plugin's permission +``` + +`isOwner` is the fact that is structurally stable: user id 1, pinned by a CHECK constraint, bypassing +every permission check. It will mean the same thing in a year. `Super Admin` might not. + +A plugin that genuinely needs a third tier — "Admins may also do this" — should not get it by reading a +role. It should declare a second permission key and let the owner grant it. That keeps the answer to +"who may do this" on the permissions page, where the owner can see and change it, rather than compiled +into a plugin's `if`. + +**Nothing may be trusted from the caller.** The proxy builds a fresh header object and copies a +three-item allowlist; it does not forward arbitrary inbound headers. That is why `X-Officer-User` is +safe to trust, and any new header inherits the same safety **only for as long as the allowlist stays an +allowlist**. If that loop is ever changed to a copy-everything-except, a client-supplied +`X-Officer-Owner: 1` becomes a privilege escalation against every plugin at once. + +**Absent means denied.** A sidecar that does not see the headers — reached directly, an older platform, +a bug — must resolve to no privileges rather than to the owner. Same rule as `authorize.ts`, where every +catch returns deny. + +## Where I disagree, and it is one point + +Plugin-side enforcement has a failure mode the platform has already been bitten by, and it is worth +naming before it happens again rather than after. + +`assertPermissionTotality` exists because a Member could 403 on `GET /api/tasks` and open +`/api/tasks/pipeline/ws` with a 101 in the same minute. The lesson written down at the time was +"**a patch does not survive the next door; refusing to boot does.**" Pure plugin-side authorization is +that same shape: jellyfin will check, because we are about to write the check. The next plugin's author +forgets one route, and nothing anywhere notices — no boot failure, no warning, no test. + +So the recommendation is not "platform enforces instead". It is **declare in the manifest, enforce in the +proxy, and pass the facts to the sidecar as well**: + +```ts +// manifest.ts +ownerOnlyPaths: ['/_config'], +``` + +- **Declared by the plugin**, because the plugin knows which of its paths are administrative. +- **Enforced by the platform**, in the proxy, before the request reaches the sidecar — so forgetting the + check inside the plugin is not a hole, and the rule holds for a route added later by someone who never + read this file. +- **Visible at install**, next to the permissions the owner already reviews, and greppable across every + plugin from one place. +- **Still passed to the sidecar**, because the plugin's own finer rules — whose rows, which fields — are + genuinely its business and cannot be expressed as a path prefix. + +This does not contradict the `manifest.ts` comment above. That comment refuses to make a **permission** +owner-only, because a permission is a thing the owner delegates and one nobody can delegate is a +contradiction. A path prefix is not a grant; it is a statement that a route administers the plugin +rather than uses it. + +## The frontend is a courtesy, not the lock + +Hiding the Settings entry in the plugin's nav for a non-owner is worth doing, and it is not security. +Same rule as `usePermissions`, which fails open on purpose: hiding an entry is politeness, the 403 is the +lock. Write the guard on the route first and the nav second, never only the second. + +## What must be settled before any of this lands + +**A member with no server has no Jellyfin.** Connections are stored strictly per user — every query in +`plugins/jellyfin/db/queries.ts` filters on `eq(jellyfinServers.userId, userId)`. Lock `/_config` to the +owner today and a Member gets a plugin that can only ever show them the connect form they are forbidden +to submit. + +The resolution that fits what already exists: a non-owner resolves to the **owner's active server URL** +authenticated with **their own token**, which provisioning already mints and stores in +`jellyfin_accounts`. That is arguably why those tokens are minted at all — per-user identity upstream, so +resume points, watched marks and next-up stay each person's own rather than everyone sharing the owner's. + +That is a change to credential resolution, not to authorization, and it is the actual prerequisite. The +guard is a few lines; this is the work. + +## Status + +| | | +| ----------- | ---------------------------------------------------------------------------------- | +| Agreed | plugin decides what a level means inside its own surface | +| Agreed | pass `isOwner` and the effective level; never the role name | +| Agreed | fail closed when the headers are absent | +| Proposed | `ownerOnlyPaths` in the manifest, enforced by the proxy — pending the owner's call | +| Blocked on | credential resolution for non-owners, above | +| Not started | all of it |