From 453058c67b0e07ded70b349ec3d7423785b91299 Mon Sep 17 00:00:00 2001 From: lilleman Date: Mon, 17 Aug 2026 22:34:56 +0200 Subject: [PATCH] Refuse a shadowing barrel copy, and record the packaging contract --- .dockerignore | 3 ++- AGENTS.md | 11 +++++++++++ README.md | 32 ++++++++++++++++++++++--------- plugin-api/package.json | 1 - src/plugin-host/discovery.test.ts | 1 + src/plugin-host/discovery.ts | 9 +++++++-- todo.md | 3 ++- 7 files changed, 46 insertions(+), 14 deletions(-) diff --git a/.dockerignore b/.dockerignore index b3c82df..05a6c89 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,5 +1,6 @@ .git -# Load-bearing: a stray copy would bake in at /app/node_modules and shadow /node_modules. +# Load-bearing both ways: a stray copy would bake in at /app/node_modules and shadow /node_modules, +# and matching only the root one is what lets a baked plugin keep its own deps. Never `**/node_modules`. node_modules npm-debug.log *.log diff --git a/AGENTS.md b/AGENTS.md index d6c0385..dbb4d53 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -328,6 +328,17 @@ one-time setup. A file-map or table row gets a clause, not a paragraph. and emitted markup are author-visible. Know the hole that leaves — discovery fails loud on a bad `apiVersion`, but `include("menu", { open: true })` silently ignores a dropped option. Promotion must cover the partial vocabulary too. +- **The frozen surface also includes the packaging promises** (README → Plugin dependencies): the + barrel is ambient at `/node_modules` with nothing for a plugin to declare, `"type": "module"` is + mandatory, and the host neither upgrades nor dedupes a plugin's dependencies. Same hole as the + partials — move the publish point, rename the package or start hoisting and every installed plugin + breaks with no version signal. Note the promise is deliberately *not* "your deps are yours alone": + build-time dedupe for baked images stays open, module-instance sharing stays unpromised. +- **Publishing `@plainpages/plugin-api` to a registry is deferred, not rejected.** Today it is + `private` and shaped as a shim — `index.ts` re-exports `../src/…`, so `npm pack` would ship a + broken tree. The trigger is the same as the freeze's: the first external plugin, which is also the + first author who cannot typecheck against a mounted host tree. Whoever does it must first make the + artifact self-contained (types-only `.d.ts`, or move the barrel into `plugin-api/`). - A plugin's `apiVersion` is a **hand-written literal** semver — the host version it was built against — bumped by hand on rebuild, **never** the host's `HOST_API_VERSION` constant. Importing the constant makes every plugin always equal the host, so `checkApiVersion` can never fire. diff --git a/README.md b/README.md index 892335b..0e6189a 100644 --- a/README.md +++ b/README.md @@ -689,13 +689,18 @@ it, because that file (not the host's) is what tells Node how to parse everythin { "name": "things", "version": "0.0.0", "type": "module" } ``` -Then install into the folder. `--save-exact` pins the version: the root `.npmrc` does not reach a -`--prefix`, so without it npm writes a range. +Add a `plugins/things/.npmrc` too. The root one does not reach a `--prefix`, so without it npm writes +ranges rather than the exact pins this project keeps everywhere: + +```ini +save-exact=true +``` + +Then install into the folder: ```bash # The uid keeps the files it writes yours rather than root's. -docker compose run --rm --no-deps --user "$(id -u):$(id -g)" web \ - npm install --prefix plugins/things --save-exact ms +docker compose run --rm --no-deps --user "$(id -u):$(id -g)" web npm install --prefix plugins/things ms ``` A plugin in its own repo runs its own `npm ci` instead and mounts the result — `node_modules` @@ -706,11 +711,15 @@ Two rules follow from how Node resolves: - **Never ship a copy of `@plainpages/plugin-api`.** The host publishes it into `/node_modules`, above every plugin, and a plugin resolves it from there — nothing to declare, just import it. A - copy inside your own `node_modules` shadows it with a *second* instance of the host's contract, - and every `instanceof GuardError` a handler makes silently starts returning `false`. (A type stub - for standalone typechecking is fine — keep it out of what you mount.) -- **Your dependencies are yours alone.** Two plugins depending on the same package each get their - own copy at their own version, so neither can break the other by upgrading. + copy inside your own `node_modules` would shadow it with a *second* instance of the host's + contract, turning a sign-in redirect into a 500, so discovery refuses one at boot. (A type stub for + standalone typechecking is fine — keep it out of what you mount.) +- **The host never upgrades or dedupes your dependencies.** Two plugins depending on the same package + each get their own copy at their own version, so neither can break the other by upgrading — and + keeping yours current, and audited, is yours to own. Renovate here watches the host's manifests + only. +- **Depend on packages that ship JavaScript.** Node refuses to strip types under `node_modules`, so a + dependency whose entry is `.ts` fails at import with `ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING`. `npm run typecheck` covers `plugins/`, so a dependency shipping no types of its own needs its `@types/…` in your plugin's `devDependencies`. Typechecking a plugin repo standalone still needs the @@ -1138,6 +1147,11 @@ obey `SECURE_COOKIES`; the Kratos one takes its flags from Kratos' own config. **Offboarding is not instant by default** — a revoked permission or deactivated identity lands within one token TTL, unless the [denylist](#instant-revoke-the-optional-denylist) is on. +**A plugin, and every package it depends on, runs with the host's full privileges** — in the process +holding the JWT signing key and `ctx.system`'s Ory admin clients, on the network that reaches the +unauthenticated Ory ports. Install only what you trust, and let the plugin's own lockfile +([Plugin dependencies](#plugin-dependencies)) pin the tree you audited. + Hardening a real deploy is `REQUIRE_SECURE_SECRETS=true`, `SECURE_COOKIES=true`, and replacing **every** committed dev secret ([what you must supply](#what-you-must-supply-the-only-manual-prep)). `REQUIRE_SECURE_SECRETS` guards only `CSRF_SECRET`; nothing fails loud if you ship Ory's, Postgres' diff --git a/plugin-api/package.json b/plugin-api/package.json index 59333a5..452617a 100644 --- a/plugin-api/package.json +++ b/plugin-api/package.json @@ -1,6 +1,5 @@ { "name": "@plainpages/plugin-api", - "version": "1.0.0", "private": true, "type": "module", "exports": "./index.ts" diff --git a/src/plugin-host/discovery.test.ts b/src/plugin-host/discovery.test.ts index dad0887..f4be606 100644 --- a/src/plugin-host/discovery.test.ts +++ b/src/plugin-host/discovery.test.ts @@ -57,6 +57,7 @@ const badCases: Array<{ name: string; files: Record; match: RegE { name: "a route gating on a bare word", files: { "bare/plugin.ts": `export default { apiVersion: "1.0.0", routes: [{ method: "GET", path: "/", permission: "admin", handler: () => ({ html: "x" }) }] };` }, match: /bare.*admin.*:/s }, { name: "a nav node gating on a bare word", files: { "barenav/plugin.ts": `export default { apiVersion: "1.0.0", nav: [{ id: "n", label: "N", permission: "admin" }] };` }, match: /barenav.*admin.*:/s }, { name: "a declared permission that is a bare word", files: { "baredecl/plugin.ts": `export default { apiVersion: "1.0.0", permissions: [{ name: "admin" }] };` }, match: /baredecl.*admin.*:/s }, + { name: "a plugin shipping its own copy of the barrel", files: { "shadow/node_modules/@plainpages/plugin-api/index.js": `export class GuardError extends Error {}`, "shadow/plugin.ts": full("shadow") }, match: /shadow.*@plainpages\/plugin-api/s }, { name: "a plugin package.json that forgets type: module", files: { "cjs/package.json": `{ "name": "cjs" }`, "cjs/plugin.ts": full("cjs") }, match: /cjs.*"type": "module"/s }, { name: "a plugin package.json that is not valid JSON", files: { "bent/package.json": `{`, "bent/plugin.ts": full("bent") }, match: /bent.*package\.json.*JSON/s }, { name: "two plugins claim the public home", files: { "a/plugin.ts": `export default { apiVersion: "1.0.0", home: () => ({ html: "a" }) };`, "b/plugin.ts": `export default { apiVersion: "1.0.0", home: () => ({ html: "b" }) };` }, match: /home/ }, diff --git a/src/plugin-host/discovery.ts b/src/plugin-host/discovery.ts index 9ea9aa8..5da1975 100644 --- a/src/plugin-host/discovery.ts +++ b/src/plugin-host/discovery.ts @@ -89,9 +89,14 @@ function pluginFolders(dir: string): string[] { .sort(); } -// Without a `type`, which npm never writes, the plugin's own package.json leaves its folder -// CommonJS: a .js helper breaks outright and every .ts costs a re-parse. +// The two ways a plugin's own packaging breaks it. A barrel copy resolves before the host's, and its +// GuardError matches no `instanceof` here — the sign-in redirect silently becomes a 500. Without a +// `type`, which npm never writes, the folder is left CommonJS: a .js helper breaks, every .ts re-parses. function packagingError(folder: string): string | null { + if (existsSync(join(folder, "node_modules", "@plainpages", "plugin-api"))) { + return "ships its own copy of @plainpages/plugin-api — remove it; the host provides the one instance"; + } + const file = join(folder, "package.json"); if (!existsSync(file)) return null; diff --git a/todo.md b/todo.md index 17cd721..2a25397 100644 --- a/todo.md +++ b/todo.md @@ -3,6 +3,7 @@ ## Unfinnished work - [ ] Add a way to configure plugins directly when installing. Most reasonable is an .env file in the plugin folder, I think, but I am open to suggestions. +- [ ] Decide Renovate's reach over plugin dependencies before the first example plugin takes one. `renovate.json` extends `config:recommended`, whose `:ignoreModulesAndTests` ignores `**/examples/**`, so an example plugin's `package.json` would get no update PRs and nobody would notice. Either narrow the ignorePath or state that plugin deps are the plugin owner's to update (README → Plugin dependencies already says so for external plugins). Raised by the architecture review 2026-08-17. - [ ] Rename the plugin "admin" to something less generic, like "auth-admin" or "users-groups-admin". - [ ] Guard the group paths against self-lockout, or accept them explicitly. The self-revoke guard covers only your own *direct* grants on the Users screen; unticking a permission on a group you belong to, removing yourself from that group, or deleting it can all still strip your own effective access with no warning. Recorded in AGENTS.md as a known gap — the robust fix is a "last effective holder" check, which needs a reverse Keto query. - [ ] The permission picker has no concurrency baseline, so two operators editing the same user/group silently discard each other's change. Sketch: post the rendered set as a hidden baseline; if it no longer matches Keto, re-render with "this changed while you had the page open" rather than applying. @@ -33,7 +34,7 @@ Prioritized. Overall verdict: architecture is sound; these are refinements. - [ ] **LOW→MEDIUM — Retire `src/ui/shell-context.ts`.** `ShellModel`/`buildShellContext` has one consumer left (dashboard) and duplicates `PageChrome` on almost every field. Fold the dashboard onto `ctx.chrome` + title/breadcrumbs; keep `shellUser` as the shared primitive. - [ ] **LOW — Fix stale doc references to removed `docs/plugin-contract.md`** in `views/index.ejs` (user-visible dashboard text; also links /scheduling as if pre-installed) and `examples/plugins/scheduling/views/shifts.ejs`. - [ ] **LOW — Decide (once) on a `ctx.system` facade.** `@plainpages/plugin-api` exposes raw Ory client shapes, so an Ory client refactor is a major `apiVersion` bump. AGENTS.md accepts this; revisit only if external plugin authors appear. -- [ ] **LOW — README/AGENTS.md gaps:** state the intended lifetime/horizon explicitly, add a short domain glossary (host, manifest, chrome, nav fragment, permission token, system plugin, denylist…), and note the expected plugin-author population. +- [ ] **LOW — README/AGENTS.md gaps:** state the intended lifetime/horizon explicitly, add a short domain glossary (host, manifest, chrome, nav fragment, permission token, system plugin, denylist…), and note the expected plugin-author population and plugin count — per-plugin dependency isolation prices N dependency trees on disk and in RSS, and that number is what says whether the trade needs revisiting (build-time dedupe for baked images stays open). ## Finnished work