From 88b6257333cc96ddab6f49eee55bd66789d8f7c6 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Wed, 12 Aug 2026 03:12:25 -0500 Subject: [PATCH] docs(website): record slice 1, and correct two rules it disproved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The install service and admin API as built (website#142), plus the two things §2.7.2 got wrong before there was any code to check them against. The top-level directory rule was checking against nothing real. module-uo's release workflow packs module-uo-/, not uo/, so "reject a top-level directory whose name is not the manifest's id" would have refused every bundle that exists. That level is stripped instead - its name belongs to whoever published the bundle, the directory it lands in has to be the id the loader scans for - and what replaces the check is stronger: the UNPACKED module.json must agree with the install manifest about id and version. And tar has to be pinned forward rather than merely depended on. Installing it gets 6.x, which npm audit reports as critical, and the advisory list reads as this feature's own threat model: hardlink traversal via drive-relative linkpath, symlink poisoning, hardlink escape through a symlink chain, PAX size override on GNU long-name headers, decompression DoS. Refusing symlink and hardlink entry types outright is what takes the extractor off most of that list rather than depending on the library to contain them. Also records the measurement behind the two-pass unpack, which was assumed in the plan and is now known: node-tar DOES reject an escaping member, but late. An archive whose fourth member escapes throws and leaves the first three on disk - and the loader only asks whether module.json is present, so a half-unpacked bundle is a module as far as the next boot is concerned. Refreshes api-route-inventory.json (158 -> 166 public) and the route count in BACKEND_DESIGN, and documents module_source_hosts beside the other seeded settings keys - bootstrapped from the environment, owned by the database. Co-Authored-By: Claude --- website/BACKEND_DESIGN.md | 12 ++++- website/MODULE_SYSTEM.md | 89 ++++++++++++++++++++++++++++++-- website/api-route-inventory.json | 32 ++++++++++++ 3 files changed, 126 insertions(+), 7 deletions(-) diff --git a/website/BACKEND_DESIGN.md b/website/BACKEND_DESIGN.md index d463eb2..eb25ad9 100644 --- a/website/BACKEND_DESIGN.md +++ b/website/BACKEND_DESIGN.md @@ -275,7 +275,15 @@ Seeded with the 8 spec categories: `new-player-guide, maps-atlas, systems, items Seeded keys: `site_mode` (default `maintenance`), `site_mode_changed_at`, `site_mode_changed_by`, `maintenance_message`, `status_message`, `homepage_teaser`, `contact_email` (=UOMysticmoon@gmail.com), `site_title`, `player_registration` -(default `disabled`), `mobile_app_links_enabled`. +(default `disabled`), `mobile_app_links_enabled`, `module_source_hosts`. + +`module_source_hosts` is the allowlist of hostnames a module may be installed from +(MODULE_SYSTEM.md §2.7.2 decision 6), edited in Admin → Modules and audited as +`module.sources`. It is **bootstrapped** from `MODULE_SOURCE_HOSTS` and not owned by +it: `seedDefault` is an `INSERT IGNORE`, so the environment supplies a default on a +fresh install and changing the variable later cannot reach back in and overwrite what +an operator chose. Installs are `https`-only, every redirect hop is re-checked against +this list, and an empty value forbids every install rather than allowing every host. **Keys a MODULE seeds into this table.** `settings` is core's, but a module's schema fragment may `INSERT IGNORE` its own rows into it, and module-uo seeds two: @@ -731,7 +739,7 @@ are authoritative, and they answer different questions: | Artifact | Source of truth for | Generated by | |---|---|---| -| `server/routes.manifest.json` — mirrored as [api-route-inventory.json](./api-route-inventory.json) | **What URLs CORE serves.** 158 public routes + 2 on the internal listener, sorted, method + path only. | `npm run routes:manifest`, by walking the live Express stack | +| `server/routes.manifest.json` — mirrored as [api-route-inventory.json](./api-route-inventory.json) | **What URLs CORE serves.** 166 public routes + 2 on the internal listener, sorted, method + path only. | `npm run routes:manifest`, by walking the live Express stack | | `server/swagger/swagger-output.json` — merged into `/api/docs` | **What each core route means.** Parameters, bodies, response codes, security. | `npm run swagger`, from `#swagger.*` annotations | Both are **core's**. An installed module's routes are in neither: they are in that module's own diff --git a/website/MODULE_SYSTEM.md b/website/MODULE_SYSTEM.md index 89f2be5..9288f8c 100644 --- a/website/MODULE_SYSTEM.md +++ b/website/MODULE_SYSTEM.md @@ -1511,15 +1511,19 @@ far more than it appeared to be. Core writes into a **bind-mounted directory it host** (Phase 2 PR 9), so an escape is not confined to the container's own filesystem. The extractor therefore rejects rather than sanitises — an archive that needs correcting is an -archive that should not be trusted — on: any absolute path, any `..` segment, any entry that is not a -regular file or a directory (so no symlinks, hardlinks, devices or FIFOs), more than one top-level -directory, a top-level directory whose name is not the manifest's `id`, and a total unpacked size or -entry count over its cap. Unpacking goes to a temporary directory beside the target and is moved into -place only once the whole archive has been accepted, so a rejected or interrupted install never +archive that should not be trusted — on: any absolute path (POSIX, drive-relative or UNC), any `..` +segment, a NUL byte or a backslash in a path, any entry that is not a regular file or a directory (so +no symlinks, hardlinks, devices or FIFOs), more than one top-level entry, and a total unpacked size +or entry count over its cap. Unpacking goes to a temporary directory beside the target and is moved +into place only once the whole archive has been accepted, so a rejected or interrupted install never leaves a half-module on the volume for the next boot's scan to find. `tar` is a real dependency rather than a hand-rolled parser: the format has enough shape (long-name extensions, PAX headers, sparse entries) that parsing it is exactly the kind of code this list is trying to defend against. +*(Two corrections from building it, in slice 1 below: the top-level directory's NAME is not checked +against the module id — it is `module-uo-`, not `uo` — and `tar` has to be pinned forward +rather than merely depended on.)* + ##### What Phase 4 must prove | # | Criterion | How | @@ -1529,6 +1533,81 @@ sparse entries) that parsing it is exactly the kind of code this list is trying | 3 | A hostile archive cannot write outside `modules//` | Unit tests over the rejection list above, each with a real crafted tar | | 4 | A compose-declared module resolves at container start, offline | Container boots with the network down and an already-unpacked module, and comes up unchanged | +#### Slice 1 — the install service and the admin API (website#142, 2026-08-12) + +`modules/archive.js`, `modules/install.js`, `schema.runPurge()`, `lifecycle.stop()`, +`loader.stopHook()` and eight routes under `/api/v1/admin/modules`. 797 server tests (+76), manifest +158 → 166 public + 2 internal, OpenAPI gains eight operations and loses none. + +##### The two-pass unpack is measured, not assumed + +The reason `archive.js` inspects an entire archive before `tar.x` sees it is not that node-tar fails +to reject an escaping member — it rejects it. It rejects it **late**. An archive whose fourth member +is `mod/../../ESCAPED.txt`, extracted with `strict: true` under node-tar 7.5.22: + +``` +threw: TAR_ENTRY_ERROR: path contains '..' +LEFT ON DISK after the throw: [ 'first.txt', 'second.txt' ] +``` + +The loader scans a module directory at require time and asks only whether `module.json` is there +(§1.12), so a half-unpacked bundle is a module as far as the next boot is concerned. A `filter` +callback cannot fix this either: by the time it is asked about entry 400, the first 399 are written. +Hence inspect-then-extract, and hence the scratch directory that is removed on any failure with the +move into place as the last step. + +##### `tar` is pinned forward, and its advisory list is the argument + +Installing `tar` gets 6.x, which `npm audit` reports as **critical**. Reading the list is the useful +part, because almost every entry is this feature's own threat model: hardlink path traversal via +drive-relative linkpath, symlink poisoning through insufficient path sanitisation, hardlink target +escape through a symlink chain, PAX size override on GNU long-name headers causing a parser +interpretation differential, decompression DoS via unlimited input. The dependency is pinned to +`^7.5.22`, and — more to the point — refusing symlink and hardlink entry types **outright** is what +takes this extractor off most of that list rather than leaving it depending on the library to contain +them. + +##### The bundle's top-level directory is not named after the module + +`module-uo`'s release workflow packs `dist/module-uo-/`, so §2.7.2's original rule — reject +a top-level directory whose name is not the manifest's `id` — would have refused every real bundle. +Corrected above: exactly one top-level entry is required and that level is **stripped**, because its +name belongs to whoever published the bundle while the directory it lands in has to be the id the +loader scans for. What replaces the check is stronger anyway: the **unpacked** `module.json` must +agree with the install manifest about both id and version, so a manifest promising `uo` and +delivering something else is refused rather than installed under the name it promised. + +##### Enable is pinned by a test, because "fixing" it is a one-line change + +`lifecycle.stop()` dispatches one module's `onShutdown` and then moves the record — in that order, +since while the hook runs the module is still `started`, the only state in which its routes and the +world it is tearing down agree with each other. A hook that throws does not stop the disable, which +is the opposite of the boot path's rule and deliberate: there a failure means the module never became +safe to use, here the operator has asked for it to stop answering. + +There is no `start(id)` beside it, and `enable` deliberately does not touch the loader. That is +invisible in the code — it looks like an omission — so a test asserts `loader.setState` is never +called, with the reason written next to it. + +##### Two defects, both of a class already written down + +- **The controller destructured `runPurge` at require time.** `const { runPurge } = require(…)` + captures the function, not the module, so the one dependency whose *order* matters — purge must run + before the directory containing `purge.sql` is deleted — was the one that could not be substituted + in a test. Same class as `module-uo/server/core.js`'s never-destructure-a-getter rule. Every other + import in that file was already a namespace import; the destructure was the odd one out. +- **Two `#swagger` annotations carried an apostrophe inside a quoted string.** swagger-autogen + re-quotes to `'` before evaluating, so `module's` breaks the annotation and it is dropped. Found + only because slice 5 taught the generator to fail loudly; the fix is the typographic `’`, as before. + +##### Proved against the real release, not a fixture + +The published `module-uo` v0.3.0 install manifest, fetched over the real Gitea host and its redirect +chain: sha256 verified, the 252,517-byte artifact inspected and unpacked to 82 files, no scratch +directory left behind — and then core booted against the result and the module registered its five +mounts, seven streams and eight capabilities with its client chunk resolved. That is criterion 1 +minus the screen and the restart, both of which arrive in slice 2. + ### 2.8 SPA URL namespacing — a deliberate break **Decision: module pages are namespaced, and old paths are not redirected.** The site is not public diff --git a/website/api-route-inventory.json b/website/api-route-inventory.json index c3ee498..3860ae1 100644 --- a/website/api-route-inventory.json +++ b/website/api-route-inventory.json @@ -177,6 +177,38 @@ "method": "POST", "path": "/api/v1/admin/moderation/user/:discordId/notes" }, + { + "method": "GET", + "path": "/api/v1/admin/modules" + }, + { + "method": "POST", + "path": "/api/v1/admin/modules" + }, + { + "method": "DELETE", + "path": "/api/v1/admin/modules/:id" + }, + { + "method": "POST", + "path": "/api/v1/admin/modules/:id/disable" + }, + { + "method": "POST", + "path": "/api/v1/admin/modules/:id/enable" + }, + { + "method": "POST", + "path": "/api/v1/admin/modules/:id/purge" + }, + { + "method": "POST", + "path": "/api/v1/admin/modules/restart" + }, + { + "method": "PUT", + "path": "/api/v1/admin/modules/sources" + }, { "method": "GET", "path": "/api/v1/admin/pages"