Files and server config over HTTP

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.

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)

src/main/core/serverRegistry.ts

@@ -134,8 +134,15 @@ export function updateServer(id: string, patch: Partial<ServerConfig>): 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}`)

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<void> {
       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<Response> =>
+        fetch(base + p, { headers: { 'X-API-Key': k } })
+      const kpost = (p: string, body: unknown, k: string): Promise<Response> =>
+        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<Response> =>
+        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<string, unknown>
+          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.