diff --git a/docs/api-operations.md b/docs/api-operations.md index 449f7b4..9f5a746 100644 --- a/docs/api-operations.md +++ b/docs/api-operations.md @@ -169,8 +169,72 @@ Mutating key calls **also** leave the generic `api.post` / `api.delete` entry from #48. That is deliberate redundancy: it is the net for any route that forgets to audit itself. +## Files + +``` +GET /api/servers/:id/files?path= files (directory listing) +GET /api/servers/:id/files?path=&as=file files (file contents) +POST /api/servers/:id/files files { path, content } +POST /api/servers/:id/files/folder files { path, name } +POST /api/servers/:id/files/rename files { path, newName } +DELETE /api/servers/:id/files?path=&confirm=true files +``` + +**Reading needs `files`, not `view`.** `server.properties` holds the RCON +password, and whatever else an operator has pasted into a config. + +One endpoint serves both a listing and a file (`as=file`) because a caller +walking a tree does not know which it has until it looks. + +`core/serverFiles.ts` refuses to leave the server root — every entry point runs +the path through the same `safe()` check — so a traversal comes back as +`400 path-escape` rather than reading anything. What it does *not* do is stop a +caller reaching the files that decide what runs: replacing a jar is code +execution on the next start. That is not a reason to block it (an operator edits +these constantly), but it is why `files` is its own scope and why every write is +audited **with its path**. + +Deleting requires `?confirm=true`: nothing inside MSMS can bring the file back. + +## Server config + +``` +GET /api/servers/:id/config settings +POST /api/servers/:id/config/properties settings { updates } or { raw } +POST /api/servers/:id/config/java settings (partial JavaArgsConfig) +POST /api/servers/:id/config/favorite settings { favorite } +``` + +A property value containing a newline is refused with `400 newline-in-value`: +in a properties file, a newline smuggles in a second key. `updates` merges, so a +targeted write leaves the rest of the file alone; send `raw` to replace it +wholesale. + +The Java patch **merges** — send `{ maxMemoryMB: 3072 }` and the preset, +flags and jar stay as they were. + +### Three Java fields are desktop-only + +`javaPath`, `customArgs` and `extraFlags` are refused over HTTP with +`403 local-only-field`, whatever scope the caller holds. + +They decide **what program MSMS executes**: `javaPath` is spawned as the process +binary, `customArgs` *is* the whole command line when the preset is `custom`, and +`extraFlags` is appended to the real one. Accepting them from a remote caller +would make `settings` mean "run arbitrary programs as the MSMS process", which is +not a settings field. + +Over IPC they are fine, and stay editable in the desktop app: there the caller is +the operator at the machine, who already has full filesystem access, so a text +box grants them nothing new. + +A patch mixing a safe field with a forbidden one is refused **whole** — the safe +half is not applied, so a caller never has to guess which part of their request +landed. + ## Not in this surface -Still IPC-only, tracked in #53: files, server config and `server.properties`, -plugins/mods, Java install, metrics config, and creating or removing a server. -World export/import as noted above. +Still IPC-only, tracked in #53: plugins/mods search and install, Java list and +install, metrics tier config, and creating or removing a server (those are not +per-server, so they need an owner-level gate rather than a scope). World +export/import as noted above. diff --git a/src/main/core/javaArgs.ts b/src/main/core/javaArgs.ts index 6398c90..9d05166 100644 --- a/src/main/core/javaArgs.ts +++ b/src/main/core/javaArgs.ts @@ -112,7 +112,10 @@ export function buildJvmFlags(cfg: JavaArgsConfig, type: ServerType): string[] { break } } - if (cfg.extraFlags.trim()) jvm.push(...tokenize(cfg.extraFlags)) + // Defensive: config.json is hand-editable, and a config missing this key + // used to throw `undefined.trim()` from inside start(), which surfaces as + // "the server never started" with no hint as to why. + if ((cfg.extraFlags ?? '').trim()) jvm.push(...tokenize(cfg.extraFlags)) return jvm } @@ -127,7 +130,7 @@ export function buildLaunchArgs(cfg: JavaArgsConfig, type: ServerType): string[] // front of it, and the JVM takes the last definition of a property, so // anything they write still wins — but a custom command line should not be // the one place the console silently mangles Turkish. - return [...CONSOLE_UTF8, ...tokenize(cfg.customArgs), ...tokenize(cfg.extraFlags)] + return [...CONSOLE_UTF8, ...tokenize(cfg.customArgs ?? ''), ...tokenize(cfg.extraFlags ?? '')] } const isProxy = PROXY_TYPES.includes(type) diff --git a/src/main/core/serverRegistry.ts b/src/main/core/serverRegistry.ts index 02ca689..787bce2 100644 --- a/src/main/core/serverRegistry.ts +++ b/src/main/core/serverRegistry.ts @@ -134,8 +134,15 @@ export function updateServer(id: string, patch: Partial): ServerCo updateConfig((cfg) => { const s = cfg.servers.find((x) => x.id === id) if (!s) return + // Captured BEFORE the assign. `Object.assign` has already replaced `s.java` + // with the patch's partial by the time the merge line runs, so + // `{ ...s.java, ...patch.java }` was spreading the partial into itself and + // silently dropping every key the caller did not mention. Invisible while + // the only caller (the desktop args editor) sent a complete config; a + // partial patch — which the config API now makes possible — lost the preset. + const prevJava = s.java Object.assign(s, patch, { id: s.id }) - if (patch.java) s.java = { ...s.java, ...patch.java } + if (patch.java) s.java = { ...prevJava, ...patch.java } updated = s }) if (!updated) throw new Error(`Server not found: ${id}`) diff --git a/src/main/smoke.ts b/src/main/smoke.ts index fc882da..fe5bc38 100644 --- a/src/main/smoke.ts +++ b/src/main/smoke.ts @@ -46,6 +46,8 @@ import { getPanelHtml } from './web/panelHtml' import { getPublicSiteHtml } from './web/publicSiteHtml' import { removeServer } from './core/serverRegistry' import * as sf from './core/serverFiles' +import * as files from './core/serverFiles' +import * as registry from './core/serverRegistry' import * as playersMod from './core/players' import * as backupsMod from './core/backups' import * as schedulerMod from './core/scheduler' @@ -4799,6 +4801,166 @@ export async function runWebSmoke(): Promise { console.log('WEB-SMOKE: panel + site scripts parse; crate picker, storefront, map tab, detail and escaping OK') } + // ---- files + config over HTTP (#53 part 2) ---- + { + const fBase = '/api/servers/' + id + '/files' + const cBase = '/api/servers/' + id + '/config' + const af = join(auditDir(), 'audit.jsonl') + const snap = existsSync(af) ? readFileSync(af, 'utf-8') : null + // Snapshotted before anything is touched, restored in `finally`. + const javaSnapshot = getConfig().servers.find((s) => s.id === id)?.java + const motdSnapshot = files + .readProperties(id) + .entries.find((e) => e.key === 'motd')?.value + const fileKey = apikeys.createKey({ label: 'smoke_files', scopes: ['files'], servers: [id] }) + const cfgKey = apikeys.createKey({ label: 'smoke_cfg', scopes: ['settings'], servers: [id] }) + const kget = (p: string, k: string): Promise => + fetch(base + p, { headers: { 'X-API-Key': k } }) + const kpost = (p: string, body: unknown, k: string): Promise => + fetch(base + p, { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'X-API-Key': k }, + body: JSON.stringify(body) + }) + const kdel = (p: string, k: string): Promise => + fetch(base + p, { method: 'DELETE', headers: { 'X-API-Key': k } }) + try { + rmSync(af, { force: true }) + auditMod._reset() + + // Reading files needs `files`, not `view`: server.properties holds the + // RCON password, among whatever else an operator has pasted in. + r = await get(fBase, ft) + if (r.status !== 403) return fail('file list without the files scope expected 403, got ' + r.status) + r = await kget(fBase, cfgKey.secret) + if (r.status !== 403) return fail('a settings key read files, got ' + r.status) + + r = await kget(fBase, fileKey.secret) + if (r.status !== 200) return fail('file list expected 200, got ' + r.status + ' ' + (await r.text())) + const listing = (await r.json()) as { entries: { name: string }[] } + if (!listing.entries.some((e) => e.name === 'server.jar')) { + return fail('the file list is missing the fixture jar') + } + + // Traversal is refused by core, and reported as a bad request rather + // than a server error. + for (const bad of ['../../secrets', '..\\..\\secrets', '/etc/passwd']) { + r = await kget(fBase + '?path=' + encodeURIComponent(bad), fileKey.secret) + // The only thing that matters is that it is not served. Whether core + // calls it path-escape (400) or the path simply is not there (404) is + // its business, not this assertion's. + if (r.ok) return fail('a traversing path was served: ' + bad) + } + + // Write, read back, then delete — and delete needs confirmation, since + // nothing inside MSMS can bring the file back. + r = await kpost(fBase, { path: 'api-smoke.txt', content: 'hello-api' }, fileKey.secret) + if (r.status !== 200) return fail('file write expected 200, got ' + r.status + ' ' + (await r.text())) + r = await kget(fBase + '?as=file&path=api-smoke.txt', fileKey.secret) + const readBack = (await r.json()) as { content: string } + if (readBack.content !== 'hello-api') return fail('file read-back mismatch: ' + readBack.content) + if (!auditMod.query({ actions: ['file.write'] }).entries.some((e) => e.target === 'api-smoke.txt')) { + return fail('a file write was not audited with its path') + } + r = await kdel(fBase + '?path=api-smoke.txt', fileKey.secret) + if (r.status !== 400) return fail('file delete without confirm expected 400, got ' + r.status) + r = await kdel(fBase + '?path=api-smoke.txt&confirm=true', fileKey.secret) + if (r.status !== 200) return fail('file delete expected 200, got ' + r.status) + if (files.listDir(id, '').some((e) => e.name === 'api-smoke.txt')) { + return fail('the file survived its delete') + } + + // ---- config ---- + r = await kget(cBase, fileKey.secret) + if (r.status !== 403) return fail('a files key read config, got ' + r.status) + r = await kget(cBase, cfgKey.secret) + if (r.status !== 200) return fail('config read expected 200, got ' + r.status) + const cfgBody = (await r.json()) as { + server: { id: string } + properties: { entries: { key: string; value: string }[] } + } + if (cfgBody.server.id !== id) return fail('config returned the wrong server') + if (!cfgBody.properties.entries.length) return fail('config returned no properties') + + // A newline in a value would smuggle a second key into the file. + r = await kpost(cBase + '/properties', { updates: { motd: 'hi\nmax-players=999' } }, cfgKey.secret) + if (r.status !== 400) return fail('a newline in a property value expected 400, got ' + r.status) + if (((await r.json()) as { error: string }).error !== 'newline-in-value') { + return fail('the newline refusal gave the wrong error') + } + if (!auditMod.query({ actions: ['config.properties'] }).entries.some((e) => e.ok === false)) { + return fail('a refused property write was not audited') + } + + const before = Object.fromEntries( + files.readProperties(id).entries.map((e) => [e.key, e.value]) + ) + r = await kpost(cBase + '/properties', { updates: { motd: 'api-smoke-motd' } }, cfgKey.secret) + if (r.status !== 200) return fail('property write expected 200, got ' + r.status) + const after = Object.fromEntries(files.readProperties(id).entries.map((e) => [e.key, e.value])) + if (after['motd'] !== 'api-smoke-motd') return fail('the property write did not land') + // The rest of the file must be untouched — writeProperties merges. + if (after['enable-rcon'] !== before['enable-rcon']) { + return fail('a targeted property write disturbed another key') + } + // The three fields that decide what binary runs are desktop-only, + // whatever scope the caller holds. `settings` is not a licence to run + // arbitrary programs as the MSMS process. + for (const field of ['javaPath', 'customArgs', 'extraFlags']) { + r = await kpost(cBase + '/java', { [field]: 'C:/evil.exe' }, cfgKey.secret) + if (r.status !== 403) return fail(field + ' over HTTP expected 403, got ' + r.status) + const body = (await r.json()) as { error: string; fields: string[] } + if (body.error !== 'local-only-field' || !body.fields.includes(field)) { + return fail(field + ' was refused for the wrong reason: ' + JSON.stringify(body)) + } + // ...and it really did not land. + const now = getConfig().servers.find((x) => x.id === id)?.java as unknown as Record + if (now[field] === 'C:/evil.exe') return fail(field + ' was written despite the 403') + } + if (!auditMod.query({ actions: ['config.java'] }).entries.some((e) => e.ok === false)) { + return fail('a refused java field was not audited') + } + // A patch mixing a safe field with a forbidden one is refused whole, + // rather than partially applied. + r = await kpost(cBase + '/java', { minMemoryMB: 512, javaPath: 'C:/evil.exe' }, cfgKey.secret) + if (r.status !== 403) return fail('a mixed patch expected 403, got ' + r.status) + if (getConfig().servers.find((x) => x.id === id)?.java.minMemoryMB === 512) { + return fail('a refused patch still applied its safe half') + } + + // Java config merges rather than replacing, or a partial patch would + // wipe the preset it did not mention. + r = await kpost(cBase + '/java', { maxMemoryMB: 3072 }, cfgKey.secret) + if (r.status !== 200) return fail('java config write expected 200, got ' + r.status) + const javaAfter = getConfig().servers.find((s) => s.id === id)?.java + if (javaAfter?.maxMemoryMB !== 3072) return fail('the java patch did not land') + if (javaAfter?.preset !== javaSnapshot?.preset) { + return fail('a partial java patch replaced the preset: ' + String(javaAfter?.preset)) + } + if (javaAfter?.extraFlags === undefined) { + return fail('a partial java patch dropped extraFlags, which breaks the launch') + } + console.log('WEB-SMOKE: files + config over HTTP OK (scope split, traversal refused, writes audited)') + } finally { + // In `finally`, not inline. A failed assertion between the patch and an + // inline restore leaves the shared dev-root fixture with a partial java + // config, and every other gate then fails to start a server for reasons + // that have nothing to do with what they test. That already happened + // once here, which is why it moved. + if (javaSnapshot) registry.updateServer(id, { java: javaSnapshot }) + if (motdSnapshot !== undefined) files.writeProperties(id, { motd: motdSnapshot }) + apikeys.deleteKey(fileKey.key.id) + apikeys.deleteKey(cfgKey.key.id) + try { + files.deleteEntry(id, 'api-smoke.txt') + } catch { + /* already gone */ + } + if (snap == null) rmSync(af, { force: true }) + else writeFileSync(af, snap, 'utf-8') + } + } + // ---- player detail + live map (#49, #26) ---- { // Pure map math first. diff --git a/src/main/web/server.ts b/src/main/web/server.ts index ecadcbb..d8f99fa 100644 --- a/src/main/web/server.ts +++ b/src/main/web/server.ts @@ -6,6 +6,9 @@ import { getConfig } from '../config' import { uploadsDir } from '../paths' import { log } from '../logger' import { listServers, getServer } from '../core/serverRegistry' +import * as registry from '../core/serverRegistry' +import * as files from '../core/serverFiles' +import type { JavaArgsConfig } from '@shared/types' import { processManager } from '../core/processManager' import { getPlayers } from '../core/players' import * as playersMod from '../core/players' @@ -16,6 +19,7 @@ import { isGamemode, isValidMcName, isValidWorldName, + localOnlyJavaFields, moderationAuditAction, needsConfirm, sanitizeCommandArg @@ -746,6 +750,178 @@ async function handlePanel(req: IncomingMessage, res: ServerResponse): Promise { + if (!can(user, id, scope)) { + sendJson(res, 403, { error: 'forbidden', need: scope }) + return false + } + return true + } + const trail = (op: string, target: string, ok: boolean, detail?: string): void => { + audit.record({ + source: user.apiKey ? 'api' : 'webpanel', + action: op, + actor: user.username, + ok, + ip, + serverId: id, + target, + ...(detail ? { detail } : {}) + }) + } + /** + * `core/serverFiles.ts` already refuses to leave the server root — every + * entry point runs the path through `safe()`. What it does NOT do is stop a + * caller reaching the files that decide what runs: replacing a jar or + * dropping a plugin is code execution on the next start, and `eula.txt` or + * `server.properties` change what the server is. + * + * That is not a reason to block them (an operator edits these constantly), + * but it is a reason every write is audited with its path, and a reason + * `files` is its own scope rather than part of `settings`. + */ + const fileErr = (e: unknown): number => { + const msg = String((e as Error)?.message ?? e) + // A traversal attempt is a bad request; a missing file is a missing file. + if (msg === 'path-escape') return 400 + if (msg.includes('ENOENT')) return 404 + return 400 + } + + if (group === 'files') { + // Read is `files` too, not `view`: server files hold the RCON password, + // and anything else an operator has pasted into a config. + if (!gate('files')) return + const rel = url.searchParams.get('path') ?? '' + try { + if (!action && method === 'GET') { + // A directory lists; a file reads. One endpoint because a caller + // walking a tree does not know which it has until it looks. + const stat = url.searchParams.get('as') + if (stat === 'file') return sendJson(res, 200, files.readTextFile(id, rel)) + return sendJson(res, 200, { path: rel, entries: files.listDir(id, rel) }) + } + if (!action && method === 'POST') { + const b = (await readBody(req).catch(() => ({}))) as { path?: string; content?: string } + if (typeof b.path !== 'string' || typeof b.content !== 'string') { + return sendJson(res, 400, { error: 'path-and-content-required' }) + } + files.writeTextFile(id, b.path, b.content) + trail('file.write', b.path, true, b.content.length + ' bytes') + return sendJson(res, 200, { ok: true }) + } + if (!action && method === 'DELETE') { + const confirm = url.searchParams.get('confirm') === 'true' + // Deleting a server file is not recoverable from inside MSMS. + if (!confirm) { + trail('file.delete', rel, false, 'confirm-required') + return sendJson(res, 400, { error: 'confirm-required', op: 'file.delete' }) + } + files.deleteEntry(id, rel) + trail('file.delete', rel, true) + return sendJson(res, 200, { ok: true }) + } + if (action === 'folder' && method === 'POST') { + const b = (await readBody(req).catch(() => ({}))) as { path?: string; name?: string } + files.createFolder(id, b.path ?? '', b.name ?? '') + trail('file.mkdir', (b.path ?? '') + '/' + (b.name ?? ''), true) + return sendJson(res, 200, { ok: true }) + } + if (action === 'rename' && method === 'POST') { + const b = (await readBody(req).catch(() => ({}))) as { path?: string; newName?: string } + files.renameEntry(id, b.path ?? '', b.newName ?? '') + trail('file.rename', (b.path ?? '') + ' -> ' + (b.newName ?? ''), true) + return sendJson(res, 200, { ok: true }) + } + } catch (e) { + const msg = String((e as Error)?.message ?? e) + if (method !== 'GET') trail('file.' + (method === 'DELETE' ? 'delete' : 'write'), rel, false, msg) + return sendJson(res, fileErr(e), { error: msg }) + } + } + + if (group === 'config') { + if (!gate('settings')) return + try { + if (!action && method === 'GET') { + const s = getServer(id) + return sendJson(res, 200, { + server: { + id: s?.id, + name: s?.name, + type: s?.type, + mcVersion: s?.mcVersion, + path: s?.path, + favorite: s?.favorite ?? false + }, + java: s?.java ?? null, + properties: files.readProperties(id) + }) + } + if (action === 'properties' && method === 'POST') { + const b = (await readBody(req).catch(() => ({}))) as { + updates?: Record + raw?: string + } + if (typeof b.raw === 'string') { + files.writeRawProperties(id, b.raw) + trail('config.properties', 'raw', true, b.raw.length + ' bytes') + } else { + const updates = b.updates ?? {} + const keys = Object.keys(updates) + if (!keys.length) return sendJson(res, 400, { error: 'updates-required' }) + // Values are written into a properties file, not a shell or a + // console, so the only thing that can corrupt the file is a newline + // smuggling in a second key. + for (const k of keys) { + if (/[\r\n]/.test(String(updates[k]))) { + trail('config.properties', k, false, 'newline-in-value') + return sendJson(res, 400, { error: 'newline-in-value', key: k }) + } + } + files.writeProperties(id, updates) + trail('config.properties', keys.join(','), true) + } + return sendJson(res, 200, { properties: files.readProperties(id) }) + } + if (action === 'java' && method === 'POST') { + const b = (await readBody(req).catch(() => ({}))) as Partial + // javaPath / customArgs / extraFlags decide what binary runs and with + // what command line. `settings` means "edit server settings", not "run + // arbitrary programs as the MSMS process", so they are desktop-only - + // where the caller is the operator at the machine, who already has + // full filesystem access anyway. + const forbidden = localOnlyJavaFields(b as Record) + if (forbidden.length) { + trail('config.java', forbidden.join(','), false, 'local-only-field') + return sendJson(res, 403, { error: 'local-only-field', fields: forbidden }) + } + // updateServer merges `java` rather than replacing it, so a partial + // patch keeps the rest of the preset intact. + const updated = registry.updateServer(id, { java: b as JavaArgsConfig }) + trail('config.java', b.preset ?? 'update', true, JSON.stringify(b).slice(0, 200)) + return sendJson(res, 200, { java: updated?.java ?? null }) + } + if (action === 'favorite' && method === 'POST') { + const b = (await readBody(req).catch(() => ({}))) as { favorite?: boolean } + const updated = registry.updateServer(id, { favorite: !!b.favorite }) + return sendJson(res, 200, { favorite: updated?.favorite ?? false }) + } + } catch (e) { + return sendJson(res, fileErr(e), { error: String((e as Error)?.message ?? e) }) + } + } + + return sendJson(res, 404, { error: 'not-found' }) + } + // ---- operations: moderation / worlds / backups (#53) ---- // // Its own matcher rather than widening the single-segment one above: that diff --git a/src/shared/ops.ts b/src/shared/ops.ts index 7665064..3a7f2dd 100644 --- a/src/shared/ops.ts +++ b/src/shared/ops.ts @@ -117,6 +117,38 @@ export function needsConfirm(op: string): op is ConfirmableOp { return (CONFIRM_REQUIRED as readonly string[]).includes(op) } +// ---- launch configuration ---- + +/** + * Java config fields that may NOT be set over HTTP, whatever scope the caller + * holds (#53). + * + * These three decide what program MSMS executes: + * + * - `javaPath` is spawned as the process binary. Point it at any executable on + * the host and the next server start runs that instead of Java. + * - `customArgs` IS the whole command line when `preset` is `custom`. + * - `extraFlags` is appended to the real command line. + * + * Over IPC that is fine: the caller is the operator sitting at the machine, who + * already has full filesystem access, so offering them a text box is not a + * privilege they did not have. Over HTTP it is a remote scope that an API key + * can hold, and `settings` means "edit server settings" - not "run arbitrary + * programs as the MSMS process". Remote code execution is not a settings field. + * + * Deliberately a denylist rather than an allowlist of safe fields: the safe set + * is memory numbers and booleans that grow over time, and a new one being + * accidentally blocked is an annoyance, while a new dangerous one being + * accidentally allowed is this bug again. + */ +export const LOCAL_ONLY_JAVA_FIELDS = ['javaPath', 'customArgs', 'extraFlags'] as const + +/** Which forbidden fields does this patch try to set? Empty means it is fine. */ +export function localOnlyJavaFields(patch: Record | null | undefined): string[] { + if (!patch) return [] + return LOCAL_ONLY_JAVA_FIELDS.filter((f) => patch[f] !== undefined) +} + // ---- worlds ---- export type WorldAction = 'activate' | 'rename' | 'clone' | 'reset' | 'delete'