feat(modules): install, uninstall, purge and restart (phase 4, slice 1)

The consumer half of a release module-uo's CI has been publishing since phase 3 closed. Before this, core had the installed_modules provenance columns and no code that could ever fill them: nothing fetched, verified, unpacked, removed or purged anything, and there was no admin route at all. Adds modules/archive.js, modules/install.js, schema.runPurge(), lifecycle.stop(), loader.stopHook(), and /api/v1/admin/modules with eight routes. 797 server tests (+76), manifest 158 -> 166 + 2 internal, OpenAPI gains 8 operations and loses nothing. Reject, never sanitise ---------------------- The download is the easy part: an https-only allowlist re-checked on every redirect hop, a declared sha256 compared against the bytes that arrived, and a byte cap. Unpacking is where the archive chooses the filenames, and core writes into a directory bind-mounted from the host, so an escape is not confined to the container. archive.js inspects the whole archive before a byte is unpacked and refuses absolute and drive-absolute paths, `..` segments, NUL bytes, backslashes, anything that is not a regular file or a directory, more than one top-level entry, and anything over the entry or byte caps. Refusing symlinks and hardlinks outright is what keeps this off the majority of node-tar's published advisories rather than depending on the library to contain them. That two-pass shape is load-bearing, and it was measured rather than assumed: extracting an archive whose fourth member escapes upward throws under node-tar 7.5.22 -- and leaves the first three members on disk. The loader scans that directory at require time on the next boot, so a half-unpacked module is a module. Everything therefore happens in a scratch directory that is removed on any failure, and the move into place is the last step. `tar` is pinned to ^7.5.22 rather than the ^6 that installs by default: 6.x is flagged critical, and reading the advisory list is what the file's header now says out loud -- almost all of it is hardlink or symlink traversal and PAX header interpretation differentials, which is exactly this feature's threat model. Two things the plan had wrong ----------------------------- The bundle's top-level directory is `module-uo-<version>`, not the module id -- so "the top-level name must equal the id" was checked against nothing real. The extractor strips that level instead, because its name belongs to whoever published the bundle and the directory it lands in is core's. What is checked instead is the unpacked module.json: a manifest promising `uo` and delivering something else is refused rather than installed under the name it promised. And purge cannot be a follow-up action (decision 5): purge.sql lives inside the directory uninstall deletes. It is offered in the uninstall flow and as a standalone action on a still-installed module, and the standalone one refuses unless the module is already disabled -- dropping tables under something that is still serving leaves it answering out of a world that no longer exists. Disable now means stopped ------------------------- lifecycle.stop() dispatches that one module's onShutdown before flipping the guard, so a module an operator switches off actually releases its sockets and closes its streams instead of merely becoming unreachable. The hook runs first and the state moves after it, because while onShutdown runs the module is still `started` and that is the only state in which its routes and the world it is tearing down agree. A hook that throws does not stop the disable -- the opposite of the boot path's rule, and deliberately. Enable is not its mirror and there is no start(id) beside it. There is no onBoot re-dispatch and the hooks were never promised re-entrant, so enable moves the row and the restart route starts it. A test pins that enable does not touch the loader, because "fixing" it is a one-line change that would put a module with closed sockets back on the nav. Restart raises SIGTERM against its own process rather than calling the shutdown path directly, so server.js's handler stays the one graceful-shutdown path and this route cannot drift from it. The allowlist bootstraps from MODULE_SOURCE_HOSTS into a settings row and is admin-managed after that (decision 6); seedDefault is INSERT IGNORE, so changing the variable on an existing deployment is a no-op by design. An empty list forbids every install rather than allowing every host -- the safe direction for a value someone might blank by accident. Verified against the real v0.3.0 release ---------------------------------------- Not a fixture: fetched the published install manifest over the real Gitea host and its redirect chain, verified the sha256, inspected and unpacked the 252,517-byte artifact to 82 files, and then booted core against the result -- the module registered its five mounts, seven streams and eight capabilities and resolved its client chunk, with no scratch directory left behind. Two defects this slice's own tooling caught, both of which had already been written down as classes: - the controller destructured runPurge at require time, capturing the function rather than the module, which made the one dependency whose ORDER matters the one that could not be substituted; - two swagger annotations carried an apostrophe inside a quoted string, dropped silently by swagger-autogen before slice 5 taught it to fail loudly. Co-Authored-By: Claude <noreply@anthropic.com>
2026-08-12 03:09:45 -05:00
parent 2cb549e9e5
commit b30e82cde2
20 changed files with 3381 additions and 3 deletions
--- a/server/test/adminModules.test.js
+++ b/server/test/adminModules.test.js
@@ -0,0 +1,463 @@
+// ── Admin · Modules: the delivery surface ──────────────────────────────────
+//
+// Phase 4, slice 1 of MODULE_SYSTEM.md §2.7.2. The controller is tested directly
+// with a mock `res` and stubbed models — the same shape adminUsers.test.js uses —
+// because what is interesting here is not the HTTP plumbing but the ORDER of
+// operations and which of the three sources of truth answers which question.
+//
+// Two of these tests exist to pin decisions that are easy to "fix" back into
+// being wrong:
+//
+//   - **enable must not touch the loader.** Disable ran the module's onShutdown;
+//     there is no onBoot re-dispatch, so flipping the record back would put a
+//     module with closed sockets and cleared timers back on the nav.
+//   - **purge must run before the directory is removed.** purge.sql lives inside
+//     that directory. Reorder those two lines and the feature silently stops
+//     working, with a 200 and no data deleted.
+//
+// Point the DB at a closed port BEFORE requiring anything that builds the pool.
+process.env.DB_HOST = '127.0.0.1'
+process.env.DB_PORT = '59999'
+
+const { test, beforeEach, after } = require('node:test')
+const assert = require('node:assert/strict')
+
+const ctrl = require('../src/router/v1/admin/modules.controller')
+const modules = require('../src/model/modules/modules.model')
+const activity = require('../src/model/activity/activity.model')
+const settings = require('../src/model/settings/settings.model')
+const loader = require('../src/modules/loader')
+const lifecycle = require('../src/modules/lifecycle')
+const install = require('../src/modules/install')
+const schema = require('../src/modules/schema')
+const db = require('../src/utils/db')
+
+after(() => db.close())
+
+function mockRes() {
+  return {
+    statusCode: 200,
+    body: null,
+    status(c) { this.statusCode = c; return this },
+    json(b) { this.body = b; return this },
+  }
+}
+
+const req = (extra = {}) => ({
+  user: { id: 1, username: 'admin' },
+  params: {},
+  query: {},
+  body: {},
+  ...extra,
+})
+
+// Everything the controller reaches for, replaced wholesale per test. Restored
+// from these originals rather than from a snapshot taken mid-run, so one test
+// leaking a stub cannot quietly become another test's fixture.
+const originals = {
+  modules: { ...modules },
+  activity: { log: activity.log },
+  settings: { get: settings.get, set: settings.set },
+  loader: { isLoaded: loader.isLoaded, list: loader.list },
+  lifecycle: { stop: lifecycle.stop },
+  install: {
+    install: install.install,
+    isInstalled: install.isInstalled,
+    purgeFile: install.purgeFile,
+    removeDir: install.removeDir,
+  },
+  schema: { runPurge: schema.runPurge },
+}
+
+let logged
+
+beforeEach(() => {
+  Object.assign(modules, originals.modules)
+  Object.assign(activity, originals.activity)
+  Object.assign(settings, originals.settings)
+  Object.assign(loader, originals.loader)
+  Object.assign(lifecycle, originals.lifecycle)
+  Object.assign(install, originals.install)
+  Object.assign(schema, originals.schema)
+
+  logged = []
+  activity.log = async (entry) => { logged.push(entry) }
+  settings.get = async () => 'gitea.whitlocktech.com'
+  loader.isLoaded = () => true
+  loader.list = () => []
+})
+
+// ── list ───────────────────────────────────────────────────────────────────
+
+test('list reconciles the row, the loader and the volume without picking a winner', async () => {
+  // The case §2.4 creates and decision 3 makes routine: the row says `enabled`
+  // because the operator just switched it back on, the loader still says
+  // `disabled` because its onShutdown has run and there is no way back without a
+  // restart. Rendering either one alone would be a lie.
+  modules.list = async () => [{
+    id: 'uo', name: 'UO', version: '1.0.0', state: 'enabled',
+    failureStage: null, failureReason: null, source: 'https://x/y.json', sha256: 'a'.repeat(64),
+    installedAt: null, startedAt: null,
+  }]
+  loader.list = () => [{ id: 'uo', name: 'UO', version: '1.0.0', state: 'disabled', stage: null, reason: null, capabilities: ['shard'] }]
+  install.isInstalled = () => true
+  install.purgeFile = () => '/modules/uo/server/db/purge.sql'
+
+  const res = mockRes()
+  await ctrl.list(req(), res)
+
+  const [m] = res.body.modules
+  assert.equal(m.state, 'enabled', 'what the operator decided')
+  assert.equal(m.liveState, 'disabled', 'what is actually answering')
+  assert.equal(m.onVolume, true)
+  assert.equal(m.canPurge, true)
+  assert.deepEqual(m.capabilities, ['shard'])
+  assert.deepEqual(res.body.sourceHosts, ['gitea.whitlocktech.com'])
+})
+
+test('list includes a module on the volume that has no row yet', async () => {
+  // A hand-placed directory before its first boot. §2.5 keeps that a supported
+  // install, and its routes are already being served — a screen showing nothing
+  // for it would be showing the wrong thing.
+  modules.list = async () => []
+  loader.list = () => [{ id: 'byhand', name: 'By Hand', version: '0.1.0', state: 'started', stage: null, reason: null, capabilities: [] }]
+  install.isInstalled = () => true
+  install.purgeFile = () => null
+
+  const res = mockRes()
+  await ctrl.list(req(), res)
+
+  assert.equal(res.body.modules.length, 1)
+  assert.equal(res.body.modules[0].id, 'byhand')
+  assert.equal(res.body.modules[0].state, null, 'no row means no recorded state, not a guessed one')
+  assert.equal(res.body.modules[0].liveState, 'started')
+})
+
+test('list survives a process where the loader never scanned', async () => {
+  loader.isLoaded = () => false
+  loader.list = () => { throw new Error('modules.list() before modules.load()') }
+  modules.list = async () => [{ id: 'uo', name: 'UO', version: '1', state: 'disabled' }]
+  install.isInstalled = () => false
+  install.purgeFile = () => null
+
+  const res = mockRes()
+  await ctrl.list(req(), res)
+
+  assert.equal(res.statusCode, 200)
+  assert.equal(res.body.modules[0].liveState, null)
+})
+
+// ── install ────────────────────────────────────────────────────────────────
+
+test('install records provenance and says a restart is needed', async () => {
+  const calls = []
+  install.install = async ({ url, hosts }) => {
+    calls.push({ url, hosts })
+    return { id: 'uo', name: 'UO', version: '1.0.0', sha256: 'b'.repeat(64), source: url, replaced: false }
+  }
+  modules.recordInstalled = async (row) => { calls.push(row); return { ...row, state: 'installed' } }
+
+  const res = mockRes()
+  await ctrl.create(req({ body: { url: 'https://gitea.whitlocktech.com/x/uo.json' } }), res)
+
+  assert.equal(res.statusCode, 201)
+  assert.equal(res.body.restartRequired, true)
+  assert.deepEqual(calls[0].hosts, ['gitea.whitlocktech.com'], 'the allowlist comes from the setting')
+  // Provenance is written HERE and nowhere else — the boot reconcile records a
+  // module with null source/sha256 and leaves what it is not given.
+  assert.equal(calls[1].source, 'https://gitea.whitlocktech.com/x/uo.json')
+  assert.equal(calls[1].sha256, 'b'.repeat(64))
+  assert.equal(logged[0].action, 'module.install')
+})
+
+test('an install refusal is reported to the operator, with its own status', async () => {
+  install.install = async () => {
+    const err = new Error('"evil.net" is not an allowed module source host')
+    err.name = 'InstallError'
+    err.status = 400
+    throw err
+  }
+
+  const res = mockRes()
+  await ctrl.create(req({ body: { url: 'https://evil.net/x.json' } }), res)
+
+  assert.equal(res.statusCode, 400)
+  // The message is the useful part: the operator pasted a URL and needs to know
+  // what was wrong with what came back.
+  assert.match(res.body.message, /not an allowed module source host/)
+  assert.equal(logged.length, 0, 'a refused install is not an audit-log entry')
+})
+
+test('an unreachable host is a 502, not a 400', async () => {
+  install.install = async () => {
+    const err = new Error('could not reach x: timeout')
+    err.name = 'InstallError'
+    err.status = 502
+    throw err
+  }
+
+  const res = mockRes()
+  await ctrl.create(req({ body: { url: 'https://gitea.whitlocktech.com/x.json' } }), res)
+  assert.equal(res.statusCode, 502)
+})
+
+test('an unexpected failure is a 500 and does not leak its message', async () => {
+  install.install = async () => { throw new Error('ENOENT /some/internal/path') }
+
+  const res = mockRes()
+  await ctrl.create(req({ body: { url: 'https://gitea.whitlocktech.com/x.json' } }), res)
+
+  assert.equal(res.statusCode, 500)
+  assert.equal(res.body.message, 'Internal Server Error')
+})
+
+// ── enable / disable ───────────────────────────────────────────────────────
+
+test('enable moves the row and does NOT touch the loader', async () => {
+  // The decision-3 invariant. Re-enabling cannot restart a module: its
+  // onShutdown has run, and MODULE_API.md has never promised onBoot is safe to
+  // run twice. Flipping the record would put it back on the nav with a
+  // torn-down world behind it.
+  let setStateCalled = false
+  loader.setState = () => { setStateCalled = true }
+  modules.enable = async (id) => ({ id, state: 'enabled' })
+
+  const res = mockRes()
+  await ctrl.enable(req({ params: { id: 'uo' } }), res)
+
+  assert.equal(res.body.module.state, 'enabled')
+  assert.equal(res.body.restartRequired, true)
+  assert.equal(setStateCalled, false, 'enable must not move the in-memory record')
+  assert.equal(logged[0].action, 'module.enable')
+  loader.setState = originals.loader.setState
+})
+
+test('enabling a module with no row is a 404', async () => {
+  modules.enable = async () => null
+  const res = mockRes()
+  await ctrl.enable(req({ params: { id: 'ghost' } }), res)
+  assert.equal(res.statusCode, 404)
+})
+
+test('an illegal transition is a 409, not a 500', async () => {
+  modules.enable = async () => {
+    const err = new Error("module 'uo': cannot move from 'x' to 'enabled'")
+    err.name = 'ModuleStateError'
+    throw err
+  }
+  const res = mockRes()
+  await ctrl.enable(req({ params: { id: 'uo' } }), res)
+  assert.equal(res.statusCode, 409)
+})
+
+test('disable stops the module and reports whether the hook ran', async () => {
+  const calls = []
+  modules.get = async (id) => ({ id, state: 'started' })
+  lifecycle.stop = async (id) => { calls.push(id); return { stopped: true, error: null } }
+
+  const res = mockRes()
+  await ctrl.disable(req({ params: { id: 'uo' } }), res)
+
+  assert.deepEqual(calls, ['uo'])
+  assert.equal(res.body.stopped, true)
+  // No restart: this is the one action that takes effect immediately, and it is
+  // the one an operator reaches for when something is going wrong.
+  assert.equal(res.body.restartRequired, undefined)
+  assert.equal(logged[0].action, 'module.disable')
+})
+
+test('a shutdown hook that failed is reported rather than swallowed', async () => {
+  modules.get = async (id) => ({ id, state: 'started' })
+  lifecycle.stop = async () => ({ stopped: false, error: 'socket would not close' })
+
+  const res = mockRes()
+  await ctrl.disable(req({ params: { id: 'uo' } }), res)
+
+  // It IS disabled either way; the operator should be told it did not close
+  // cleanly while they still have the logs in front of them.
+  assert.equal(res.statusCode, 200)
+  assert.match(res.body.shutdownError, /socket would not close/)
+})
+
+// ── uninstall and purge ────────────────────────────────────────────────────
+
+test('uninstall purges BEFORE it removes the directory', async () => {
+  // The ordering that makes decision 5 work at all: purge.sql is a file inside
+  // the directory being deleted. Swap these two and the endpoint still answers
+  // 200 and deletes nothing.
+  const order = []
+  modules.get = async (id) => ({ id, state: 'started' })
+  install.isInstalled = () => true
+  install.purgeFile = () => '/modules/uo/server/db/purge.sql'
+  schema.runPurge = async () => { order.push('purge'); return 12 }
+  lifecycle.stop = async () => { order.push('stop'); return { stopped: true, error: null } }
+  install.removeDir = async () => { order.push('removeDir'); return true }
+  modules.remove = async () => { order.push('removeRow') }
+
+  const res = mockRes()
+  await ctrl.remove(req({ params: { id: 'uo' }, query: { purge: 'true' } }), res)
+
+  assert.deepEqual(order, ['purge', 'stop', 'removeDir', 'removeRow'])
+  assert.equal(res.body.purged, 12)
+  assert.equal(res.body.restartRequired, true)
+  assert.equal(logged[0].action, 'module.purge')
+})
+
+test('a plain uninstall keeps the row and does not purge', async () => {
+  const order = []
+  modules.get = async (id) => ({ id, state: 'started' })
+  install.isInstalled = () => true
+  schema.runPurge = async () => { order.push('purge'); return 1 }
+  lifecycle.stop = async () => { order.push('stop'); return { stopped: true, error: null } }
+  install.removeDir = async () => { order.push('removeDir'); return true }
+  modules.remove = async () => { order.push('removeRow') }
+
+  const res = mockRes()
+  await ctrl.remove(req({ params: { id: 'uo' } }), res)
+
+  // §2.5's default: the directory goes, the data stays, and the disabled row is
+  // what keeps the retained data visible and the module reinstallable.
+  assert.deepEqual(order, ['stop', 'removeDir'])
+  assert.equal(res.body.purged, null)
+  assert.equal(logged[0].action, 'module.uninstall')
+})
+
+test('asking to purge a module that ships no purge.sql refuses instead of pretending', async () => {
+  modules.get = async (id) => ({ id, state: 'started' })
+  install.isInstalled = () => true
+  install.purgeFile = () => null
+  let removed = false
+  install.removeDir = async () => { removed = true; return true }
+
+  const res = mockRes()
+  await ctrl.remove(req({ params: { id: 'uo' }, query: { purge: 'true' } }), res)
+
+  assert.equal(res.statusCode, 400)
+  assert.match(res.body.message, /ships no purge.sql/)
+  // Nothing happened. The operator asked for the module AND its data to go; the
+  // data cannot go, so doing half of it silently would be the worst answer.
+  assert.equal(removed, false)
+})
+
+test('uninstalling something that is neither on the volume nor in a row is a 404', async () => {
+  modules.get = async () => null
+  install.isInstalled = () => false
+
+  const res = mockRes()
+  await ctrl.remove(req({ params: { id: 'ghost' } }), res)
+  assert.equal(res.statusCode, 404)
+})
+
+test('standalone purge refuses while the module is still running', async () => {
+  // Dropping the tables under a module that is still serving leaves it answering
+  // out of a world that no longer exists. Disabling first is one click.
+  modules.get = async (id) => ({ id, state: 'started' })
+  let ran = false
+  schema.runPurge = async () => { ran = true; return 1 }
+
+  const res = mockRes()
+  await ctrl.purge(req({ params: { id: 'uo' } }), res)
+
+  assert.equal(res.statusCode, 409)
+  assert.match(res.body.message, /Disable this module before purging/)
+  assert.equal(ran, false)
+})
+
+test('standalone purge runs on a disabled module', async () => {
+  modules.get = async (id) => ({ id, state: 'disabled' })
+  install.purgeFile = () => '/modules/uo/server/db/purge.sql'
+  schema.runPurge = async () => 7
+
+  const res = mockRes()
+  await ctrl.purge(req({ params: { id: 'uo' } }), res)
+
+  assert.equal(res.body.purged, 7)
+  assert.equal(logged[0].action, 'module.purge')
+})
+
+// ── the allowlist ──────────────────────────────────────────────────────────
+
+test('setSources stores a normalised list and audits the change', async () => {
+  let stored = null
+  settings.set = async (key, value) => { stored = { key, value } }
+
+  const res = mockRes()
+  await ctrl.setSources(req({ body: { hosts: 'Gitea.Example.com, releases.example.org' } }), res)
+
+  assert.deepEqual(res.body.sourceHosts, ['gitea.example.com', 'releases.example.org'])
+  assert.equal(stored.key, ctrl.HOSTS_KEY)
+  assert.equal(stored.value, 'gitea.example.com,releases.example.org')
+  // Before AND after: this setting decides what code the site will execute, so
+  // the audit entry has to say what it used to be.
+  assert.equal(logged[0].action, 'module.sources')
+  assert.deepEqual(logged[0].detail.before, ['gitea.whitlocktech.com'])
+})
+
+test('setSources refuses anything that is not a bare hostname', async () => {
+  let stored = false
+  settings.set = async () => { stored = true }
+
+  for (const bad of ['https://x.com', 'x.com/path', 'x.com:8443', '*.x.com', 'x_y.com']) {
+    const res = mockRes()
+    // eslint-disable-next-line no-await-in-loop
+    await ctrl.setSources(req({ body: { hosts: bad } }), res)
+    assert.equal(res.statusCode, 400, `${bad} should be refused`)
+  }
+  assert.equal(stored, false)
+})
+
+test('an empty allowlist is storable, and means no installs', async () => {
+  // Not a wildcard, and not an error: "nothing may be installed" is a position
+  // an operator is entitled to take.
+  let stored = null
+  settings.set = async (key, value) => { stored = value }
+
+  const res = mockRes()
+  await ctrl.setSources(req({ body: { hosts: '' } }), res)
+
+  assert.equal(res.statusCode, 200)
+  assert.deepEqual(res.body.sourceHosts, [])
+  assert.equal(stored, '')
+})
+
+// ── restart ────────────────────────────────────────────────────────────────
+
+test('restart answers before it signals, and signals its own process', async () => {
+  // It raises SIGTERM rather than calling the shutdown path directly, so that
+  // server.js's handler stays the ONE graceful-shutdown path and this route
+  // cannot drift from it.
+  const originalKill = process.kill
+  const signals = []
+  process.kill = (pid, signal) => { signals.push({ pid, signal }) }
+
+  try {
+    const res = mockRes()
+    ctrl.restart(req(), res)
+
+    // Answered synchronously: once the signal lands there is no listener left to
+    // flush a response through, so the operator would be told nothing.
+    assert.equal(res.statusCode, 202)
+    assert.equal(res.body.restarting, true)
+
+    await new Promise((resolve) => setTimeout(resolve, 400))
+    assert.deepEqual(signals, [{ pid: process.pid, signal: 'SIGTERM' }])
+    assert.equal(logged[0].action, 'module.restart')
+  } finally {
+    process.kill = originalKill
+  }
+})
+
+test('a failure to write the audit entry does not cancel the restart', async () => {
+  const originalKill = process.kill
+  const signals = []
+  process.kill = (pid, signal) => { signals.push(signal) }
+  activity.log = async () => { throw new Error('database is gone') }
+
+  try {
+    ctrl.restart(req(), mockRes())
+    await new Promise((resolve) => setTimeout(resolve, 400))
+    assert.deepEqual(signals, ['SIGTERM'])
+  } finally {
+    process.kill = originalKill
+  }
+})