From ab5c24deb7e23cccb26aea3c4d8490fcfce8ee3f Mon Sep 17 00:00:00 2001 From: lilleman Date: Wed, 5 Aug 2026 19:52:09 +0200 Subject: [PATCH] Cut the node_modules prose to one home each; drop a stray tracked file --- .dockerignore | 3 +-- AGENTS.md | 21 +++++---------------- Dockerfile | 10 ++++------ README.md | 19 ++++++------------- json | 1 - src/compose.test.ts | 8 ++------ todo.md | 4 ++-- 7 files changed, 20 insertions(+), 46 deletions(-) delete mode 100644 json diff --git a/.dockerignore b/.dockerignore index 196eef9..b3c82df 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,6 +1,5 @@ .git -# Load-bearing: deps live at /node_modules, so a builder's stray copy would bake in at -# /app/node_modules and shadow them for every consumer of the image, production included. +# Load-bearing: a stray copy would bake in at /app/node_modules and shadow /node_modules. node_modules npm-debug.log *.log diff --git a/AGENTS.md b/AGENTS.md index 7dea587..30cd75a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -319,22 +319,11 @@ them. Revisit only if the stated reason stops holding. `e2e-tests/playwright.config.ts`). The rest write users, groups and sessions to one shared backend, where a second engine's run would race the first, so widening them means giving each engine its own stack. Screenshots are written per project name for the same reason. Decided 2026-08-05. -- **Deps install to `/node_modules`, one level above `WORKDIR /app`** — Node resolves bare specifiers - upward, so dev's `.:/app` bind mount has nothing to shadow. The usual fix, a volume at - `/app/node_modules`, is what this replaces: the daemon creates a missing mount destination **as - root whatever user the container runs as** (verified — `--user 1000:1000` still yields a root-owned - dir), so it left an empty root-owned `node_modules/` sitting in the developer's own checkout, and - the anonymous volume also went stale on a dep bump until `down -v`. Consequences worth knowing: - nothing may mount inside `/app` at that path again (locked by `src/compose.test.ts`); a path built - as `/node_modules` no longer resolves, so `src/ui/icons.test.ts` locates lucide-static by - specifier via `import.meta.resolve`; and `npm install` must not run in `/app` or it recreates the - problem — README → Extending the core documents `--package-lock-only` plus `--user` instead, which - is also why the image sets `npm_config_cache` (the host uid has no home dir here). Nothing masks - that path any more, so anything at `/app/node_modules` now **shadows `/node_modules` silently** — - wrong dependency code, no warning, and CI stays green because a fresh clone has none. An empty - leftover dir is harmless (resolution falls through); a populated one from the superseded root - `npm install` needs `sudo` to remove, which is worse than the bug this replaced. `.dockerignore`'s - `node_modules` line is what keeps a builder's stray copy out of the image. Decided 2026-08-05. +- **Deps install to `/node_modules`, above `WORKDIR /app`** — Node resolves upward, so dev's `.:/app` + bind mount has nothing to shadow. Not a volume at `/app/node_modules`: the daemon creates a mount + destination as root whatever `--user` says, leaving a root-owned dir in the checkout. Nothing may + sit at that path now — it shadows `/node_modules` silently (`src/compose.test.ts` guards the compose + files, `.dockerignore` the image). Decided 2026-08-05. ## Docker only — no host tooling diff --git a/Dockerfile b/Dockerfile index a05c15a..f077b3f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,18 +1,16 @@ # Node 24 runs TypeScript directly (type stripping) — no build step. Pinned exact tag. FROM node:24.19.0-alpine3.24 -# Deps land at /node_modules, one level above WORKDIR: Node resolves upward, so dev's `.:/app` -# bind mount cannot shadow them. Mounting a volume at /app/node_modules instead has the daemon -# create that destination in the developer's own checkout, root-owned whatever user runs the -# container. Reproducible install from the lockfile; dev deps kept so typecheck/test run in-image. +# Above WORKDIR so dev's `.:/app` bind mount can't shadow them; a volume at /app/node_modules +# instead leaves a root-owned dir in the checkout (the daemon creates mount destinations as root). +# Dev deps kept so typecheck/test run in-image. COPY package.json package-lock.json .npmrc /deps/ RUN cd /deps && npm ci && mv node_modules /node_modules && rm -rf /deps WORKDIR /app COPY . . -# Lockfile edits run as the host user (README → Extending the core) so the rewritten files stay -# theirs; that uid has no home in this image, and npm's default cache would land in unwritable /. +# The host uid running a lockfile edit has no home here, so npm's cache would land in unwritable /. ENV npm_config_cache=/tmp/.npm ENV PORT=3000 EXPOSE 3000 diff --git a/README.md b/README.md index a64dd5d..682e805 100644 --- a/README.md +++ b/README.md @@ -1768,13 +1768,9 @@ so for now the error names the rule it tripped instead. `users:`/`groups:`/`oauth2-clients:` × `read`/`write`, so a copy taken before this needs re-copying. `ADMIN_PERMISSIONS` is held to the same rule, but an unusable value there is dropped with a warning rather than failing the boot. See [Naming a permission](#naming-a-permission). -- **Deps moved to `/node_modules`, above `/app`** (2026-08-05). The dev override no longer mounts an - anonymous volume at `/app/node_modules`, so after pulling, delete the empty directory the daemon - left behind: `rmdir node_modules` — no `sudo`, it is empty. `docker volume prune` reclaims the - orphaned volumes. Keep that path clear: anything there shadows the image's deps silently, and a - populated one is root-owned and needs `sudo` to remove. Add a dependency with the - `--package-lock-only` command in [Extending the core](#extending-the-core), never a bare - `npm install` in `/app`. +- **Deps moved to `/node_modules`, above `/app`** (2026-08-05). Run `rmdir node_modules` after + pulling (it is empty — no `sudo`) and `docker volume prune`. Keep that path clear: anything there + silently shadows the image's deps. ## Observability @@ -1971,18 +1967,15 @@ README-dockerhub.md The Docker Hub repository description (docker.io/larvit/pla - **New page in a plugin:** add a route + handler to the plugin manifest and a template in its `views/`. - **Static asset:** drop it in the plugin's `public/`; served at `/public//`. -- **New dependency:** update the manifest + lockfile, then rebuild — the image is where deps - live, so there is nothing to install into the checkout: +- **New dependency:** deps live in the image, so update the manifest + lockfile and rebuild — + `--package-lock-only` writes nothing into the checkout, `--user` keeps the two files yours. + Keep deps minimal — prefer the Node standard library, and an Ory REST call over an SDK. ```bash docker compose run --rm --no-deps --user "$(id -u):$(id -g)" web npm install --package-lock-only docker compose build ``` - `--package-lock-only` writes only `package.json` + `package-lock.json`, and `--user` keeps - both yours. Keep deps minimal — prefer the Node standard library, and prefer an Ory REST call - over an SDK. - All versions are pinned to **exact, human-readable semantic versions** (no ranges, no digests): npm deps via `.npmrc` (`save-exact=true`) + the committed lockfile (`npm ci`), and container images by tag in the `Dockerfile` / compose files (e.g. `node:24.16.0-alpine3.24`, diff --git a/json b/json deleted file mode 100644 index 7dd567a..0000000 --- a/json +++ /dev/null @@ -1 +0,0 @@ -{"errors":null,"message":"not found","url":"https://gitea.larvit.se/api/swagger"} \ No newline at end of file diff --git a/src/compose.test.ts b/src/compose.test.ts index 535c2a8..340f014 100644 --- a/src/compose.test.ts +++ b/src/compose.test.ts @@ -90,13 +90,9 @@ test("a one-shot bootstrap seeds the stack before web starts", () => { }); test("deps live above WORKDIR, so no mount creates a root-owned dir in the checkout", () => { - // The daemon creates a missing mount destination as root whatever user the container runs as, so - // a volume at /app/node_modules leaves a root-owned node_modules/ in the developer's own checkout - // (dev bind-mounts `.:/app`). Installing above /app lets Node resolve upward instead — nothing to - // shadow, so nothing to mount over. + // A volume at /app/node_modules would leave a root-owned dir in the checkout (AGENTS.md). const dockerfile = read("Dockerfile"); - // Asserted, not assumed: split() returns the whole file when the marker is missing, which would - // silently widen "before WORKDIR" to "anywhere". + // split() returns the whole file when the marker is missing, widening "before" to "anywhere". assert.ok(dockerfile.includes("WORKDIR /app"), "the app dir is /app"); const beforeWorkdir = dockerfile.split("WORKDIR /app")[0]!; assert.match(beforeWorkdir, /npm ci/, "npm ci runs before WORKDIR /app"); diff --git a/todo.md b/todo.md index ae7ae9f..737be68 100644 --- a/todo.md +++ b/todo.md @@ -2,7 +2,7 @@ ## Unfinnished work -- [ ] `e2e-tests/artifacts/` is written root-owned into the checkout — same complaint as the node_modules one, different mechanism, so it was left out of that fix rather than bundled in. The Playwright container runs as root and the five e2e compose files bind `./e2e-tests/artifacts` in, so screenshots, traces and the HTML report land as `root:root` and need `sudo` to delete. Unlike the mountpoint case a container `user:` *would* fix this (the daemon only forces root on destinations it has to create), but compose has no `$UID` of its own — it needs an `.env` or `id -u` plumbed through `ci.sh` — and the CI artifact upload reads that dir, so it wants checking on the runner rather than only locally. Found while fixing the node_modules item 2026-08-05. +- [ ] `e2e-tests/artifacts/` is written `root:root` into the checkout and needs `sudo` to delete — the Playwright container runs as root. Unlike the node_modules mountpoint, a container `user:` would fix it, but compose has no `$UID` of its own (needs an `.env` or `id -u` via `ci.sh`) and CI's artifact upload reads that dir. Found 2026-08-05. - [ ] 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. - [ ] Rename the plugin "admin" to something less generic, like "auth-admin" or "users-groups-admin". - [ ] Guard the group paths to 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. Same scope the deleted Permissions screen had, and recorded in AGENTS.md as a known gap — the robust fix is a "last effective holder" check, which needs a reverse Keto query. Raised by the stability review 2026-08-05. @@ -38,7 +38,7 @@ Prioritized. Overall verdict: architecture is sound (contract-first plugin API, ## Finnished work -- [x] node_modules gets installed straight to the root folder with root permissions, it should at the very least be same owner as the one running the docker process, or built inside the docker image. (It *was* built inside the image — what landed in the checkout was an empty root-owned `node_modules/`, created by the daemon as the mountpoint for `compose.override.yml`'s `- /app/node_modules` anonymous volume, which exists only to stop the `.:/app` bind mount shadowing the image's deps. "Same owner as the one running the docker process" turned out not to be reachable: the daemon creates a missing mount destination as **root regardless of the container user** — measured, `--user 1000:1000` still yields a root-owned dir — so the volume had to go, not be re-owned. Deps now install to `/node_modules`, one level above `WORKDIR /app`; Node resolves bare specifiers upward, so there is nothing for the bind mount to shadow and no volume to mount. Drops both anonymous volumes and their stale-on-dep-bump footgun with them. Three consequences: `src/ui/icons.test.ts` located lucide-static as `/node_modules/...` and now resolves it by specifier via `import.meta.resolve`; the documented `npm install ` would have recreated a root-owned dir, so README → Extending the core now runs it `--package-lock-only --user "$(id -u):$(id -g)"` — writing only the two manifest files, as yours — and the image sets `npm_config_cache=/tmp/.npm` because that uid has no home dir in it; and `src/compose.test.ts` locks the invariant, since re-adding `- /app/node_modules` to fix a resolution problem would silently bring the root-owned dir back. Verified on a live stack: `docker compose up -d --build` boots, bootstrap seeds, `/` serves 200, and the checkout stays clean.) +- [x] node_modules gets installed straight to the root folder with root permissions, it should at the very least be same owner as the one running the docker process, or built inside the docker image. (It *was* built in the image; the checkout got an empty root-owned dir — the mountpoint for `compose.override.yml`'s `- /app/node_modules` volume. Re-owning it is impossible (the daemon creates mount destinations as root whatever `--user` says), so deps moved to `/node_modules` above `WORKDIR /app` and the volume is gone. See AGENTS.md.) - [x] Document permissions format so it is folled going forward: :, for example scheduling:read. Permission "admin" does not match this, and needs to be users:read, users:write, groups:read, groups:write. (README → [Naming a permission](README.md#naming-a-permission) is the one home for the rule, and the host *enforces* it at discovery — `isValidPermissionName` in `src/plugin-host/plugin.ts`, checked by `shapeError` over every route/nav `permission` and every declared name — so a badly-named permission stops the boot like any other bad manifest, for every plugin rather than only ones the admin GUI touches. `admin` is gone, split per screen into `users:`, `groups:` and `oauth2-clients:` × `read`/`write`. The read/write split is real, not cosmetic: `users:read` opens the list and is refused on every POST, and the Admin nav header lost its own gate so each screen is filtered by its own `:read` — hold none of the three and `composeNav` drops the emptied header (which needs the header to carry no `href`, now asserted). Two things had to be fixed to get here. The permission path validator was the *group* regex with no colon, so `/admin/permissions/scheduling:read` already 404'd. And `ADMIN_PERMISSIONS` defaulting to empty exposed that `bootstrap` never bind-mounted `plugins/` at all — it discovered only the image's empty copy, so a dropped-in plugin's permissions were never seeded; the mount lives in `compose.override.yml` (dev-only, mirroring `web`'s `.:/app`) because the base file gives both services the same baked copy and a base-file mount would collide with the e2e stacks that bind plugins *inside* that path. Quick start now says `docker compose up -d`, which re-runs the one-shot. Verified end to end on a live stack.) - [x] Permissions should be a list in code. Since no permissions exists in the database out of the box, but there are a fixed number of permissions in the plugins that the end consumer and user of plain pages can use, these permissions must surface to the UI somehow. The effects is that the permissions page should be deleted completely, and the users and groups pages should gain the functionality to add permissions to their things instead, provided the user have the right permissiosn to do so, of course. Run the product reviewer agent on this todo also. (The host collects every installed plugin's declarations into one catalog — `declaredPermissions()` → `ctx.declaredPermissions`, deduped and sorted, computed once at wiring — and that catalog *is* the fixed list. The Permissions screen is deleted outright: its module, tests, three views, two partials and 29 catalog keys per locale. Users and Groups each gained a checkbox list of the catalog, ticked where held; the whole set posts back, so what is submitted is the desired state and `grantDiff` turns it into grants + revokes. Two properties earn their tests: a crafted POST cannot grant a name no plugin declares, and a held-but-undeclared name — left over from an uninstalled plugin — is never silently revoked by an unrelated save, since the picker only speaks for what it showed. A user's own change revokes their live tokens; a group's reaches members at their next re-mint, the documented transitive lag. Keto stays optional on the Users screen: without it the page still lists and edits, minus the picker. Maintainer's call 2026-08-05 to keep the OAuth2-clients screen and gate it `oauth2-clients:read/write` — permissions and OAuth2 are orthogonal, scopes say what an *app* may see and permissions what a *user* may do, so the screen only ever needed *a* gate.) - [x] In Playwright tests, check for warnings and errors in all browsers on all the steps. If they exist, that is a failure we need to fix. (Every spec takes its `test` from `e2e-tests/console-guard.ts`, which watches every page a test opens — `console.error`, `console.warning`, and uncaught page errors — and fails the test that provoked one, at whatever step. The bar is zero rather than a curated tolerance list: the app ships no client JavaScript, so a message means a broken sub-resource, a rejected attribute, or an engine refusing a feature. Two narrow escapes, both explicit: the COOP header Chromium drops because the e2e stacks serve plain http over container hostnames (a deployment serves https, where it applies), and per-test `allowConsole(/…/)` — used once, by the 404 spec, whose own navigation Chromium and WebKit log. **All browsers** is now literal for the Ory-free suites: `visual.spec.ts` + `language.spec.ts` run in Chromium, Firefox *and* WebKit — the per-test `@engines` tag is gone, and screenshots are written per project so the three don't fight over one file — which is what makes an engine-specific message visible at all. The Ory-backed suites write users, groups and sessions to one shared backend, so they stay on Chromium; widening them needs a stack per engine. Nothing in the app had to be fixed: the sweep found only the two above. Verified by negative control — an injected `console.warn` failed the test in all three engines and an injected `console.error` failed on full-flow's shared serial page — which also caught the guard registering that page twice. `src/e2e-console-guard.test.ts` locks the wiring in the *unit* gate, since a spec importing `test` straight from Playwright would run unwatched and green.)