From 8ecd6153a892edf3b2aac4e71a3b687e073d9a22 Mon Sep 17 00:00:00 2001 From: Vayun Godara Date: Thu, 20 Aug 2026 18:17:19 +0200 Subject: [PATCH 1/2] security: anchor genie log parsing to the server preamble The chat genie decides who is allowed to command the panel by reading a player name out of a Minecraft server log line. Two problems made that name untrustworthy, so a wish could be attributed to the wrong player and clear the allowlist check in tick(). Unanchored carriers. All five carrier patterns in parseWish started at a bare `\]:` separator, which the regex engine finds anywhere in the line, including inside the message body. Message bodies are player-typed text. On a vanilla server the real carrier usually matched first, but a server running a chat-formatting plugin, rank prefix, shout plugin or Discord relay emits a line shape the parser does not recognise, so the engine skipped the unrecognised prefix and matched text the player supplied. A player typing "]: server ..." was then parsed as the owner. Every carrier is now anchored to the log preamble the server itself writes, so a name can only be read from the position the server controls. Unattributable carrier. Public chat also accepted a bracketed [name] shape. The server emits that shape for its own actors and for anything a plugin, datapack or command block broadcasts, so it never proved who typed the line. The old defence was a denylist of two pseudo-names ("[Rcon]", "[Server]"), which stops two names rather than forgery: a bare "[Owner] server ..." line parsed as the owner. The branch is removed. Only carriers attributable to a real player remain: chat, the whisper shapes, and the issued-server-command echo. Also removes shell: true from the Claude spawn. The arguments are already passed as an array, so the shell layer adds nothing and only widens what a future interpolated argument could reach. A genie whose input is player chat should not have a shell behind it. POSIX PATH lookup works without one; the Windows caveat is noted in a comment rather than papered over with an untested fallback. Verified against the parser copied verbatim from this file: 25 cases covering legitimate public chat, Not Secure lines, all three whisper shapes, command echoes and varying thread tags (all still work), plus bracket forgery and embedded-separator injection aimed at each carrier individually (all denied). tsc --noEmit clean. --- server/src/services/chatgenie.ts | 51 ++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/server/src/services/chatgenie.ts b/server/src/services/chatgenie.ts index f06ffdf..df0ea9c 100644 --- a/server/src/services/chatgenie.ts +++ b/server/src/services/chatgenie.ts @@ -282,8 +282,13 @@ function claudeArgs(web: boolean): string[] { } function spawnClaude(web: boolean): ChildProcessWithoutNullStreams { - const child = spawn('claude', claudeArgs(web).join(' ') === '' ? [] : claudeArgs(web), { - shell: true, + // NO shell. The args are already an array, so a shell layer buys nothing and + // only widens what a future interpolated argument could reach — a genie whose + // input is player chat should not have a shell anywhere behind it. POSIX PATH + // lookup works fine without one. (Windows note: bare `claude` resolves only + // if a real .exe is on PATH; a .cmd/.bat shim would need an explicit resolve + // rather than reinstating shell:true. Untested there, said honestly.) + const child = spawn('claude', claudeArgs(web), { // an EMPTY cwd: spawning in the panel's own directory injected the // Spawnpoint CLAUDE.md (and project skills) into every wish cwd: genieCwd(), @@ -2277,7 +2282,20 @@ const TRIGGER = /^(?:(shh+|ssh+|psst+|secret(?:\s+genie)?|quietly)|server|genie) Whispers are secret by definition. Whispering YOURSELF is the cleanest genie line there is: private, and it bothers no one. A whisper aimed at another player still needs a trigger word, so ordinary DMs stay private - conversations and don't get executed. */ + conversations and don't get executed. + + EVERY carrier below is anchored to PRE, the log preamble the server itself + writes. This is load-bearing, not tidiness. These patterns used to start at + a bare `\]:` which the regex engine will happily find ANYWHERE in the line — + including inside the message body, which is player-typed text. On a vanilla + server that was mostly survivable because the real carrier matched first, + but any server running a chat-formatting plugin, a rank prefix, a shout + plugin or a Discord relay emits a shape this parser does not recognise, so + the engine skipped the unrecognised prefix and matched the player's own + text instead. A player typing "]: server op me" then parsed AS the + owner and cleared the allowlist check in tick(). Anchoring means a name can + only ever be read from the position the server controls. */ +const PRE = String.raw`^\[\d{2}:\d{2}:\d{2}\] \[[^\]]*\]:`; function parseWish(line: string): { player: string; wish: string; secret: boolean } | null { const from = (text: string, secret: boolean, requireTrigger: boolean, player: string) => { const m = TRIGGER.exec(text.trim()); @@ -2290,15 +2308,15 @@ function parseWish(line: string): { player: string; wish: string; secret: boolea // MC 26.2 logs NO whispers itself; WhisperMod puts them back as: // [Whisper] Steve -> Steve: build me a base const w = - /\]:\s+\[Whisper\]\s+([A-Za-z0-9_]{1,16})\s*->\s*([A-Za-z0-9_]{1,16}):\s*(.*)$/.exec(line) ?? - /\]:\s+(?:\[Not Secure\]\s+)?\[([A-Za-z0-9_]{1,16})\s*->\s*([A-Za-z0-9_]{1,16})\]\s*(.*)$/.exec(line) ?? - /\]:\s+(?:\[Not Secure\]\s+)?([A-Za-z0-9_]{1,16}) whispers to ([A-Za-z0-9_]{1,16}):\s*(.*)$/.exec(line); + new RegExp(PRE + String.raw`\s+\[Whisper\]\s+([A-Za-z0-9_]{1,16})\s*->\s*([A-Za-z0-9_]{1,16}):\s*(.*)$`).exec(line) ?? + new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?\[([A-Za-z0-9_]{1,16})\s*->\s*([A-Za-z0-9_]{1,16})\]\s*(.*)$`).exec(line) ?? + new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?([A-Za-z0-9_]{1,16}) whispers to ([A-Za-z0-9_]{1,16}):\s*(.*)$`).exec(line); if (w) { const [, name, target, text] = w; if (!text.trim()) return null; return from(text, true, target !== name, name); } - const c = /\]:\s+([A-Za-z0-9_]{1,16}) issued server command: \/(msg|w|tell|teammsg|tm)\s+(.*)$/i.exec(line); + const c = new RegExp(PRE + String.raw`\s+([A-Za-z0-9_]{1,16}) issued server command: \/(msg|w|tell|teammsg|tm)\s+(.*)$`, 'i').exec(line); if (c) { const [, name, verb, rest] = c; const dm = /^(msg|w|tell)$/i.test(verb); @@ -2308,15 +2326,18 @@ function parseWish(line: string): { player: string; wish: string; secret: boolea return from(text, true, dm ? target !== name : true, name); } - // 1. public chat. Two carriers with different trust: is real player - // chat; [name] is the /say echo shape, which the server ALSO emits for its - // own actors — `say` from the console logs as "[Rcon]" / "[Server]", and - // tonight's scare countdown produced 13 such lines. Parse [name] (some chat - // mods use it) but never accept the server's own pseudo-names as players. - const m = /\]:\s+(?:\[Not Secure\]\s+)?(?:<([A-Za-z0-9_]{1,16})>|\[([A-Za-z0-9_]{1,16})\])\s+(.*)$/.exec(line); + // 1. public chat. ONLY the carrier is accepted here. [name] used to be + // parsed too, on the theory that some chat mods use it — but [name] is the + // /say echo shape, which the server emits for its own actors AND for anything + // a plugin, datapack or command block broadcasts. It never proved WHO typed + // the line. The old defence was a denylist of the server's own pseudo-names + // ("[Rcon]", "[Server]"), which blocks two names rather than forgery: a bare + // "[Owner] server op me" line parsed as the owner and passed the allowlist. + // A carrier that cannot attribute a line to a real player must not be able to + // authorize a wish, so the whole branch is gone. Fail closed. + const m = new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?<([A-Za-z0-9_]{1,16})>\s+(.*)$`).exec(line); if (!m) return null; - if (m[2] !== undefined && /^(rcon|server)$/i.test(m[2])) return null; - return from(m[3], false, true, m[1] ?? m[2]); + return from(m[2], false, true, m[1]); } async function tick(log: (msg: string) => void): Promise { From 9eba5714cc4b78e9dc534515402cd8f101aa2eb8 Mon Sep 17 00:00:00 2001 From: Vayun Godara Date: Thu, 20 Aug 2026 18:48:09 +0200 Subject: [PATCH 2/2] security: widen the anchor, hold partial log lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the anchoring commit, from two independent code reviews. The first cut was right in principle and wrong in two ways that mattered. Anchor was too narrow. PRE pinned vanilla's log4j pattern exactly, so the genie went silently dead on Forge, NeoForge, Paper and Purpur — four of the loaders servercreate.ts builds. Forge emits two logger tags, NeoForge timestamps as [20Aug2026 12:34:56.789], Paper folds the level into one bracket. No error, no log line, the feature simply stopped. PRE now accepts any bracket timestamp plus any number of bracket tags and keeps the property that matters: the match starts at column 0, where a player cannot reach. Anchoring was bypassable through the splitter. readNewLines took a byte snapshot while the server was mid-write, so the last element of a chunk was regularly half a line and the next read began inside player-typed text. That fragment reached parseWish as its own line, and a player who typed a complete fake preamble into chat could wait for a read boundary to land in front of it. Verified against the old splitter: a cut at byte 100 of a 196-byte sample produced a forged line attributed to an allowlisted player. The trailing fragment is now held until its newline arrives; the new splitter is clean at all 195 cut positions of the same sample and still delivers every complete line in order. Also anchors the join/leave digest pattern, which still carried the original unanchored form one function below the code that removed it, so " ]: Owner joined the game" forged a join event. Digest only, never authorization, but it is the same defect. Windows: bare 'claude' is resolved through `where` at startup. Without a shell, CreateProcess only appends .exe, so an npm-global claude.cmd shim would fail ENOENT on every wish. Reinstating shell: true would also fix it and is exactly what the previous commit removed. The five carrier patterns are compiled once at module scope instead of per log line. Verified: 12 loader and carrier shapes parse (vanilla, Fabric, async chat thread, Forge two-tag, NeoForge date+ms, Paper folded level, millisecond and date-prefixed timestamps, Not Secure, whispers on vanilla and Forge formats, command echo on Paper). Injection through shout plugins, Discord relays, rank prefixes and plain chat stays denied, as do bracket forgery, pseudo-names and preamble-free lines. tsc --noEmit clean. --- server/src/services/chatgenie.ts | 75 ++++++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 19 deletions(-) diff --git a/server/src/services/chatgenie.ts b/server/src/services/chatgenie.ts index df0ea9c..757d026 100644 --- a/server/src/services/chatgenie.ts +++ b/server/src/services/chatgenie.ts @@ -1,4 +1,4 @@ -import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; +import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from 'node:child_process'; import AdmZip from 'adm-zip'; import { existsSync, readFileSync, writeFileSync, mkdirSync, statSync, renameSync, openSync, readSync, closeSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; @@ -18,7 +18,7 @@ import { listSchematics, stagePlacement } from './schematics.js'; import { takeSnapshot, boxFromCommands, boxFromBlueprint, undoLast } from './undo.js'; import type { Box } from './undo.js'; import { ledgerMark, ledgerAudit, unionBox } from './ledgerverify.js'; -import { KILLABLE_SPAWN_OPTS, killTree } from './platform.js'; +import { KILLABLE_SPAWN_OPTS, killTree, IS_WIN } from './platform.js'; // The chat genie: an allowlisted player types `server ` in game // chat, the panel asks Claude (headless `claude -p`) to translate the wish @@ -106,6 +106,8 @@ function pushHistory(id: string, entry: HistEntry): void { // tail state per server: byte offset into logs/latest.log const offsets = new Map(); +// trailing half-line from the last read, waiting for its newline (see readNewLines) +const partials = new Map(); // one wish runs at a time per server (parallel wishes would fight over the // same player's inventory/position); extras wait their turn in this queue interface Job { player: string; wish: string; secret: boolean } @@ -222,7 +224,17 @@ function readNewLines(id: string): string[] { readSync(fd, buf, 0, buf.length, off); closeSync(fd); offsets.set(id, size); - return buf.toString('utf8').split(/\r?\n/).filter(Boolean); + // Hold the trailing fragment back. `size` is a byte snapshot taken while the + // server is still writing, so the last chunk element is regularly HALF a line + // and the next tick's read starts mid-message — inside player-typed text. + // Handing that fragment straight to parseWish let `^` match at the cut point, + // so a player could type a whole fake preamble into chat and wait for the + // boundary to land in front of it. Anchoring is only as strong as the + // splitter feeding it, so the remainder waits here for its newline. + const text = (partials.get(id) ?? '') + buf.toString('utf8'); + const parts = text.split(/\r?\n/); + partials.set(id, parts.pop() ?? ''); + return parts.filter(Boolean); } /** A web lookup costs ~1-3 minutes of the player's time, so it is only worth @@ -281,14 +293,20 @@ function claudeArgs(web: boolean): string[] { 'Bash,Write,Edit,NotebookEdit,Task,WebSearch,WebFetch,Read,Glob,Grep,LS,Skill,TodoWrite,BashOutput,KillShell,NotebookRead,TaskCreate,TaskUpdate,TaskList,TaskGet,TaskStop,EnterPlanMode,ExitPlanMode,AskUserQuestion,Agent,SendMessage,Monitor,Workflow,ToolSearch']; } +// Resolved once. Without a shell, Windows CreateProcess only appends .exe, so a +// bare 'claude' misses the npm-global `claude.cmd` shim and every wish dies +// ENOENT. `where` finds the real path instead. Reinstating shell:true would fix +// it too and is exactly what we removed — a genie fed by player chat gets no +// shell behind it. +const CLAUDE_BIN = IS_WIN + ? (spawnSync('where', ['claude'], { encoding: 'utf8' }).stdout?.split(/\r?\n/)[0]?.trim() || 'claude') + : 'claude'; + function spawnClaude(web: boolean): ChildProcessWithoutNullStreams { // NO shell. The args are already an array, so a shell layer buys nothing and // only widens what a future interpolated argument could reach — a genie whose - // input is player chat should not have a shell anywhere behind it. POSIX PATH - // lookup works fine without one. (Windows note: bare `claude` resolves only - // if a real .exe is on PATH; a .cmd/.bat shim would need an explicit resolve - // rather than reinstating shell:true. Untested there, said honestly.) - const child = spawn('claude', claudeArgs(web), { + // input is player chat should not have a shell anywhere behind it. + const child = spawn(CLAUDE_BIN, claudeArgs(web), { // an EMPTY cwd: spawning in the panel's own directory injected the // Spawnpoint CLAUDE.md (and project skills) into every wish cwd: genieCwd(), @@ -2294,8 +2312,27 @@ const TRIGGER = /^(?:(shh+|ssh+|psst+|secret(?:\s+genie)?|quietly)|server|genie) the engine skipped the unrecognised prefix and matched the player's own text instead. A player typing "]: server op me" then parsed AS the owner and cleared the allowlist check in tick(). Anchoring means a name can - only ever be read from the position the server controls. */ -const PRE = String.raw`^\[\d{2}:\d{2}:\d{2}\] \[[^\]]*\]:`; + only ever be read from the position the server controls. + + PRE deliberately does NOT pin one log4j pattern. A first cut hardcoded + vanilla's `[HH:mm:ss] [thread/LEVEL]:` and silently killed the genie on + every Forge, NeoForge, Paper and Purpur box — four of the loaders + servercreate.ts builds — because Forge emits two logger tags, NeoForge uses + `[20Aug2026 12:34:56.789]`, and Paper folds the level into one bracket. + Silence is the worst failure here: no error, no log, the feature is just + dead. So PRE accepts any bracket-timestamp plus any number of bracket tags, + and keeps the only property that matters — the match must start at column 0, + where a player cannot reach. */ +const PRE = String.raw`^\[[^\]]*\](?: \[[^\]]*\])*:`; +// Compiled once, not per line: tick() runs these over every new log line. +const RE_WHISPER = [ + new RegExp(PRE + String.raw`\s+\[Whisper\]\s+([A-Za-z0-9_]{1,16})\s*->\s*([A-Za-z0-9_]{1,16}):\s*(.*)$`), + new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?\[([A-Za-z0-9_]{1,16})\s*->\s*([A-Za-z0-9_]{1,16})\]\s*(.*)$`), + new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?([A-Za-z0-9_]{1,16}) whispers to ([A-Za-z0-9_]{1,16}):\s*(.*)$`), +]; +const RE_CMD = new RegExp(PRE + String.raw`\s+([A-Za-z0-9_]{1,16}) issued server command: \/(msg|w|tell|teammsg|tm)\s+(.*)$`, 'i'); +const RE_CHAT = new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?<([A-Za-z0-9_]{1,16})>\s+(.*)$`); +export const RE_JOINLEAVE = new RegExp(PRE + String.raw`\s+([A-Za-z0-9_]{3,16}) (joined|left) the game$`); function parseWish(line: string): { player: string; wish: string; secret: boolean } | null { const from = (text: string, secret: boolean, requireTrigger: boolean, player: string) => { const m = TRIGGER.exec(text.trim()); @@ -2307,16 +2344,13 @@ function parseWish(line: string): { player: string; wish: string; secret: boolea // 2/3. whisper — several log shapes exist across versions/mods, accept them all. // MC 26.2 logs NO whispers itself; WhisperMod puts them back as: // [Whisper] Steve -> Steve: build me a base - const w = - new RegExp(PRE + String.raw`\s+\[Whisper\]\s+([A-Za-z0-9_]{1,16})\s*->\s*([A-Za-z0-9_]{1,16}):\s*(.*)$`).exec(line) ?? - new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?\[([A-Za-z0-9_]{1,16})\s*->\s*([A-Za-z0-9_]{1,16})\]\s*(.*)$`).exec(line) ?? - new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?([A-Za-z0-9_]{1,16}) whispers to ([A-Za-z0-9_]{1,16}):\s*(.*)$`).exec(line); + const w = RE_WHISPER[0].exec(line) ?? RE_WHISPER[1].exec(line) ?? RE_WHISPER[2].exec(line); if (w) { const [, name, target, text] = w; if (!text.trim()) return null; return from(text, true, target !== name, name); } - const c = new RegExp(PRE + String.raw`\s+([A-Za-z0-9_]{1,16}) issued server command: \/(msg|w|tell|teammsg|tm)\s+(.*)$`, 'i').exec(line); + const c = RE_CMD.exec(line); if (c) { const [, name, verb, rest] = c; const dm = /^(msg|w|tell)$/i.test(verb); @@ -2335,7 +2369,7 @@ function parseWish(line: string): { player: string; wish: string; secret: boolea // "[Owner] server op me" line parsed as the owner and passed the allowlist. // A carrier that cannot attribute a line to a real player must not be able to // authorize a wish, so the whole branch is gone. Fail closed. - const m = new RegExp(PRE + String.raw`\s+(?:\[Not Secure\]\s+)?<([A-Za-z0-9_]{1,16})>\s+(.*)$`).exec(line); + const m = RE_CHAT.exec(line); if (!m) return null; return from(m[2], false, true, m[1]); } @@ -2351,7 +2385,7 @@ async function tick(log: (msg: string) => void): Promise { try { stats = await craftyApi.getStats(id); } catch { continue; } - if (!stats.running) { offsets.delete(id); sweptBars.delete(id); whisperState.delete(id); whisperHinted.delete(id); continue; } + if (!stats.running) { offsets.delete(id); partials.delete(id); sweptBars.delete(id); whisperState.delete(id); whisperHinted.delete(id); continue; } if ((await serverPhase(id, true)) !== 'ready') continue; if (!sweptBars.has(id)) { sweptBars.add(id); // once per server boot @@ -2367,8 +2401,11 @@ async function tick(log: (msg: string) => void): Promise { for (const line of readNewLines(id)) { // join/leave tracking for the welcome-back digest (any player, not just - // allowlisted — the digest mentions who visited) - const jl = /\]:\s+([A-Za-z0-9_]{3,16}) (joined|left) the game$/.exec(line); + // allowlisted — the digest mentions who visited). Anchored like every + // other carrier: unanchored, " ]: Owner joined the game" typed in + // chat forged a join event. Digest-only, never auth, but it was the same + // bug this commit removes upstairs, so it goes too. + const jl = RE_JOINLEAVE.exec(line); if (jl) { try { handleJoinLeave(id, jl[1], jl[2] as 'joined' | 'left'); } catch { /* digest is best-effort */ } continue;