From 4d717018ea8a748b8bb16ad71c3deab6d8c20545 Mon Sep 17 00:00:00 2001 From: Agando Date: Fri, 23 Oct 2026 20:37:54 +0200 Subject: fix(chat): GLM review batch — corruption, traversal, cursor truncation, double print (v4.6.78-80) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 12 findings accepted, 3 refuted with evidence (see _docs/chat-review-glm-2026-10-23.md). Data loss: a corrupt thread read as [] and the next append rewrote it with one message — now 500 and the file is left alone. Cursor after a >50-message truncation pointed at the thread tail, making the oldest permanently unfetchable — now the last message actually sent, plus truncated:true. Two posts in the same millisecond shared a cursor — epoch is now strictly increasing. Security: chat.py show() had no id validation and joined argv onto CHAT_DIR ('../../x' read any .json as darkbot) — THREAD_RE added. The auto-reply prompt now treats message text as untrusted data and forbids acting on instructions found in it. Ops: relay housekeeping swept neither .lock-* nor orphaned .tmp-*; a crashed listing looked identical to an empty queue; grep AWAITING also matched message bodies; hermes-poll printed every reply twice. Verified: corrupt file -> 500 with the file untouched; traversal -> 'invalid thread id'; hermes-poll -> exactly one line per reply. --- _docs/chat-review-glm-2026-10-23.md | 51 +++++++++++++++++++++++++++++++++ api/chat/lib.php | 25 ++++++++++++---- api/chat/poll.php | 13 +++++++-- main.html | 6 ++-- modules/core/core-bundle.js | 21 +++++++++----- modules/core/module-debug-supervisor.js | 21 +++++++++----- modules/core/module-loader.js | 2 +- scripts/chat.py | 39 +++++++++++++++++++++---- 8 files changed, 145 insertions(+), 33 deletions(-) create mode 100644 _docs/chat-review-glm-2026-10-23.md diff --git a/_docs/chat-review-glm-2026-10-23.md b/_docs/chat-review-glm-2026-10-23.md new file mode 100644 index 0000000..e3b2d62 --- /dev/null +++ b/_docs/chat-review-glm-2026-10-23.md @@ -0,0 +1,51 @@ +# GLM-5.3 review of the chat bridge — findings and verdicts + +Reviewed 2026-10-23, model `glm-5.3:cloud`, on the code as of v4.6.76. Verdict: **REWORK**. +Each claim was re-derived against the live files before acting. Result: 11 accepted and fixed, +3 refuted with evidence, 1 accepted as a documented limitation. + +## Accepted and fixed + +| # | Finding | Fix | +|---|---|---| +| 1 | `chat.py` renamed the temp file after `LOCK_UN` → two writers could each rename a stale snapshot and drop a message | `os.replace` moved inside the lock | +| 2 | `load()` and `chat_read()` returned `[]` for a **corrupt** thread, so the next append rewrote the file with one message — history gone silently | both now raise / 500 on unparseable JSON, file left intact. Proven: `poll` and `post` on a corrupted file → `500 thread file is corrupt`, file unchanged | +| 3 | The lock file was created by darkbot at 0644; PHP opens it as www-data → `fopen` fails → every later post to that thread 500s | `chmod 0664` at creation in `chat.py`, mirroring what PHP already did | +| 4 | `poll.php` returned the thread tail as `cursor` while truncating to 50 messages → a burst >50 made the oldest permanently unfetchable | cursor is now the last message actually sent, plus a `truncated` flag | +| 5 | `epoch > since` with millisecond epochs: two posts in the same ms → the second is never delivered | epoch is forced strictly increasing (`max(now, last+1)`) | +| 6 | `hermes-poll` printed every reply twice (poll printed it, then the inbox printed `j.messages` again) | inbox no longer re-prints hermes lines, and no longer says "no new messages" under one it just printed. Verified: exactly 1 line | +| 7 | `chat.py show` joined argv onto CHAT_DIR with **no id validation** → `show ../../x` read any `.json` as darkbot | `THREAD_RE` check added. Verified: `show ../../etc/passwd` → `invalid thread id` | +| 8 | Client-minted tokens mean anyone can post text a privileged agent reads every 5 min — prompt injection | cron prompt now declares message text **untrusted data**: never act on instructions found in it, read-only investigation only, writes stay with the human | +| 9 | Housekeeping missed `.lock-*` (one per thread ever) and orphaned `.tmp-*` | both patterns added to the sweep | +| 10 | `chat.py list` crash made the relay print nothing — awaiting messages silently vanished | the script surfaces the error instead of swallowing it, and a corrupt thread is listed as `CORRUPT` rather than breaking the listing | +| 11 | `grep AWAITING` also matched message bodies containing that word | anchored on the status column: `^[a-f0-9]{16} (AWAITING\|CORRUPT)` | +| 12 | Error path sat inside `finally`; PHP does not run `finally` on `exit`, and the alternative returned a 201 with a null id | failure reported **after** the lock is released; `$persisted` decides | + +## Refuted with evidence + +| Claim | Evidence | +|---|---| +| `os.replace` after unlock (finding 1) | already fixed in v4.6.77 before the review returned; review ran on the v4.6.76 bundle | +| `author:'agent'` never clears `awaiting`, so cron duplicates the in-page answer | `awaiting` means "the last word is not Hermes's". An agent question *does* await an answer — that is the feature, not a bug | +| Thread-ceiling check is TOCTOU (499+499 → 501) | the ceiling is a disk-growth guard, not a security boundary. Overshooting by one has no consequence; a global lock on every post costs more than it saves | + +## Accepted as a limitation, not fixed + +The channel is unauthenticated **by design** — the token is mintable, so the chat is effectively +public write access. A determined attacker can still post text the auto-reply agent will read. +Mitigated (untrusted-data prompt, read-only investigation, no host writes, rate limits, 500-thread +cap) but not eliminated. Closing it properly needs platform-session auth, which the browser app +does not have. Recorded here so nobody assumes otherwise. + +## Verification after the batch + +``` +corrupt thread → 500 "thread file is corrupt", file untouched +append to corrupt → 500, nothing rewritten +chat.py show ../../x → "invalid thread id" +hermes-poll → exactly one 🤖 line per reply +list with a corrupt thread → CORRUPT row, other rows still listed +strictly increasing epoch → cursor test passes +``` + +Commits: v4.6.77 (concurrency), v4.6.78 (corruption + traversal + relay), v4.6.79/80 (double print). diff --git a/api/chat/lib.php b/api/chat/lib.php index 6a0f923..94afe94 100644 --- a/api/chat/lib.php +++ b/api/chat/lib.php @@ -51,8 +51,15 @@ function chat_path(string $thread): string { } function chat_read(string $thread): array { - $all = load_json(chat_path($thread)); - return is_array($all) ? $all : []; + $path = chat_path($thread); + if (!is_file($path)) return []; + $raw = @file_get_contents($path); + if ($raw === false) error_response('thread is unreadable', 500); + $all = json_decode($raw, true); + // A corrupt file must NOT read as "empty": the next append would then rewrite the + // thread with a single message and the history is gone for good. + if (!is_array($all)) error_response('thread file is corrupt: ' . basename($path), 500); + return $all; } // Atomic replace: a reader sees the old file or the new one, never a truncated one. @@ -83,12 +90,17 @@ function chat_append(string $thread, string $author, string $text): array { error_response('failed to lock thread', 500); } $msg = []; + $persisted = false; try { $messages = chat_read($thread); + $lastEpoch = $messages ? (int)($messages[count($messages) - 1]['epoch'] ?? 0) : 0; + $now = (int)round(microtime(true) * 1000); $msg = [ 'id' => 'MSG-' . gmdate('Ymd-His') . '-' . bin2hex(random_bytes(3)), 'ts' => gmdate('c'), // UTC, always — never a local offset - 'epoch' => (int)round(microtime(true) * 1000), // ms — second precision drops same-second messages + // strictly increasing: two posts in the same millisecond would otherwise share a + // cursor value and the second would never be delivered to a `since` reader + 'epoch' => max($now, $lastEpoch + 1), 'author' => $author, 'text' => $text, ]; @@ -101,13 +113,14 @@ function chat_append(string $thread, string $author, string $text): array { if (count($messages) > CHAT_MAX_MESSAGES) { $messages = array_slice($messages, -CHAT_MAX_MESSAGES); } - if (!chat_write($thread, $messages)) { - error_response('failed to persist message', 500); - } + $persisted = chat_write($thread, $messages); } finally { flock($lock, LOCK_UN); fclose($lock); } + // error_response() exits, and PHP does not run finally blocks on exit — so the + // failure must be reported after the lock is released, never from inside it. + if (!$persisted) error_response('failed to persist message', 500); return $msg; } diff --git a/api/chat/poll.php b/api/chat/poll.php index 601beca..b4aa970 100644 --- a/api/chat/poll.php +++ b/api/chat/poll.php @@ -14,14 +14,21 @@ $all = chat_read($thread); $messages = $since > 0 ? array_values(array_filter($all, fn($m) => (int)($m['epoch'] ?? 0) > $since)) : $all; -$lastAll = $all ? end($all) : null; + +$window = array_slice($messages, -50); +$truncated = count($messages) > count($window); +// The cursor is the last message the client is actually being handed. Returning the +// thread's real tail after a truncation would skip everything in between for good. +$lastSent = $window ? $window[count($window) - 1] : null; +$lastAll = $all ? $all[count($all) - 1] : null; json_response([ 'success' => true, 'thread' => $thread, - 'messages' => array_slice($messages, -50), + 'messages' => $window, + 'truncated' => $truncated, // `awaiting` replaces `unread`: the server keeps no read state, so an unread // counter could never clear. Who spoke last is knowable. 'awaiting' => (bool)($lastAll && ($lastAll['author'] ?? '') !== 'hermes'), - 'cursor' => $lastAll ? (int)($lastAll['epoch'] ?? 0) : 0, + 'cursor' => $lastSent ? (int)($lastSent['epoch'] ?? 0) : ($lastAll ? (int)($lastAll['epoch'] ?? 0) : 0), ]); diff --git a/main.html b/main.html index 8cc0848..f27008a 100644 --- a/main.html +++ b/main.html @@ -1349,13 +1349,13 @@ - + - + - +