aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAgando <roduccomm@gmail.com>2026-10-23 20:37:54 +0200
committerAgando <roduccomm@gmail.com>2026-10-23 20:37:54 +0200
commit4d717018ea8a748b8bb16ad71c3deab6d8c20545 (patch)
treedc4996d6904d2f89120b07cfa2c59c0fb41c8e75
parente34c1203500451e65f0439435df55a2fa4c0f4ab (diff)
fix(chat): GLM review batch — corruption, traversal, cursor truncation, double print (v4.6.78-80)
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.
-rw-r--r--_docs/chat-review-glm-2026-10-23.md51
-rw-r--r--api/chat/lib.php25
-rw-r--r--api/chat/poll.php13
-rw-r--r--main.html6
-rw-r--r--modules/core/core-bundle.js21
-rw-r--r--modules/core/module-debug-supervisor.js21
-rw-r--r--modules/core/module-loader.js2
-rwxr-xr-xscripts/chat.py39
8 files changed, 145 insertions, 33 deletions
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 @@
</script>
<!-- v4.5.63: A2A scratchpad fixes + agent-register arg fix -->
-<script src="modules/core/core-bundle.js?v=v4.6.77"></script>
+<script src="modules/core/core-bundle.js?v=v4.6.80"></script>
<!-- Async dependency-ordered module loader now only initializes terminal and loads optional ext modules -->
-<script src="modules/core/module-loader.js?v=v4.6.77"></script>
+<script src="modules/core/module-loader.js?v=v4.6.80"></script>
<!-- Critical ext bundle loads AFTER webx11 exists so registerModule calls work -->
-<script src="modules/ext/critical-ext-bundle.js?v=v4.6.77"></script>
+<script src="modules/ext/critical-ext-bundle.js?v=v4.6.80"></script>
<script>
// ═══ ONE TRUE VERSION SOURCE ═══
diff --git a/modules/core/core-bundle.js b/modules/core/core-bundle.js
index 90e2593..73f5ab3 100644
--- a/modules/core/core-bundle.js
+++ b/modules/core/core-bundle.js
@@ -20751,6 +20751,7 @@ class DebugSupervisorModule {
const err = new Error((j && j.error) || ('HTTP ' + resp.status));
err.status = resp.status;
err.code = (j && j.code) || '';
+ err.retryAfterMs = (j && j.retry_after_ms) || null;
throw err;
}
const st = { token: token, cursor: j.epoch || 0, thread: j.thread || '' };
@@ -20758,7 +20759,7 @@ class DebugSupervisorModule {
return j;
}
- _chatPoll(force) {
+ _chatPoll(force, silent) {
const st = this._chatThread();
const token = st.token;
if (!token) return Promise.resolve(null);
@@ -20779,7 +20780,7 @@ class DebugSupervisorModule {
if ((m.epoch || 0) > (this._chatThread().cursor || 0)) {
this._chatSaveState({ token: token, cursor: m.epoch || 0, thread: j.thread || '' });
}
- if (m.author === 'hermes') this.terminal.print('🤖 ' + m.text, '');
+ if (m.author === 'hermes' && !silent) this.terminal.print('🤖 ' + m.text, '');
});
return j;
})
@@ -21578,7 +21579,7 @@ class DebugSupervisorModule {
_cmdFeedback(args) {
const msg = (Array.isArray(args) ? args.join(' ') : String(args || '')).trim();
if (!msg) {
- this.terminal.print('usage: feedback <message> (alias: chat <message>)', 'error');
+ this.terminal.print('usage: hermes <message> (alias: feedback <message>)', 'error');
return;
}
this.terminal.print('📨 Sending…', '');
@@ -21588,9 +21589,10 @@ class DebugSupervisorModule {
' — reply arrives here; `hermes-poll` to check now', 'success'))
.catch(e => {
// 503 is nginx's limit_req, not ours: it arrives as HTML, so status is all we have.
- const hint = e.status === 429 ? ' (rate limited — retry later)'
+ const wait = e.retryAfterMs ? ' in ' + Math.ceil(e.retryAfterMs / 1000) + 's' : ' later';
+ const hint = e.status === 429 ? ' (rate limited — retry' + wait + ')'
: e.status === 503 ? ' (host busy — retry)'
- : e.status === 401 ? ' (bad token — try chat-reset)'
+ : e.status === 401 ? ' (bad token — try hermes-reset)'
: '';
this.terminal.print('❌ ' + e.message + hint, 'error');
});
@@ -21598,10 +21600,15 @@ class DebugSupervisorModule {
_cmdFeedbackInbox() {
this.terminal.print('📬 Checking…', '');
+ // _chatPoll prints the 🤖 lines (once); this loop skips hermes so it is not shown
+ // twice. Making the poll silent instead would print the reply nowhere.
this._chatPoll(true).then(j => {
if (!j) { this.terminal.print('❌ no answer from the chat endpoint', 'error'); return; }
- const msgs = j.messages || [];
- if (!msgs.length) this.terminal.print('(no new messages)', 'info');
+ const incoming = j.messages || [];
+ // hermes lines were already printed by the poll above; only count them as
+ // 'new' so the box does not say "no new messages" right under a new one
+ const msgs = incoming.filter(m => m.author !== 'hermes');
+ if (!incoming.length) this.terminal.print('(no new messages)', 'info');
msgs.forEach(m => this.terminal.print(
(m.author === 'hermes' ? '🤖 ' : m.author === 'agent' ? '🧠 ' : '👤 ') + m.text, ''));
if (j.awaiting) this.terminal.print('⏳ waiting for Hermes', 'info');
diff --git a/modules/core/module-debug-supervisor.js b/modules/core/module-debug-supervisor.js
index 79c9acc..25e0566 100644
--- a/modules/core/module-debug-supervisor.js
+++ b/modules/core/module-debug-supervisor.js
@@ -134,6 +134,7 @@ class DebugSupervisorModule {
const err = new Error((j && j.error) || ('HTTP ' + resp.status));
err.status = resp.status;
err.code = (j && j.code) || '';
+ err.retryAfterMs = (j && j.retry_after_ms) || null;
throw err;
}
const st = { token: token, cursor: j.epoch || 0, thread: j.thread || '' };
@@ -141,7 +142,7 @@ class DebugSupervisorModule {
return j;
}
- _chatPoll(force) {
+ _chatPoll(force, silent) {
const st = this._chatThread();
const token = st.token;
if (!token) return Promise.resolve(null);
@@ -162,7 +163,7 @@ class DebugSupervisorModule {
if ((m.epoch || 0) > (this._chatThread().cursor || 0)) {
this._chatSaveState({ token: token, cursor: m.epoch || 0, thread: j.thread || '' });
}
- if (m.author === 'hermes') this.terminal.print('🤖 ' + m.text, '');
+ if (m.author === 'hermes' && !silent) this.terminal.print('🤖 ' + m.text, '');
});
return j;
})
@@ -961,7 +962,7 @@ class DebugSupervisorModule {
_cmdFeedback(args) {
const msg = (Array.isArray(args) ? args.join(' ') : String(args || '')).trim();
if (!msg) {
- this.terminal.print('usage: feedback <message> (alias: chat <message>)', 'error');
+ this.terminal.print('usage: hermes <message> (alias: feedback <message>)', 'error');
return;
}
this.terminal.print('📨 Sending…', '');
@@ -971,9 +972,10 @@ class DebugSupervisorModule {
' — reply arrives here; `hermes-poll` to check now', 'success'))
.catch(e => {
// 503 is nginx's limit_req, not ours: it arrives as HTML, so status is all we have.
- const hint = e.status === 429 ? ' (rate limited — retry later)'
+ const wait = e.retryAfterMs ? ' in ' + Math.ceil(e.retryAfterMs / 1000) + 's' : ' later';
+ const hint = e.status === 429 ? ' (rate limited — retry' + wait + ')'
: e.status === 503 ? ' (host busy — retry)'
- : e.status === 401 ? ' (bad token — try chat-reset)'
+ : e.status === 401 ? ' (bad token — try hermes-reset)'
: '';
this.terminal.print('❌ ' + e.message + hint, 'error');
});
@@ -981,10 +983,15 @@ class DebugSupervisorModule {
_cmdFeedbackInbox() {
this.terminal.print('📬 Checking…', '');
+ // _chatPoll prints the 🤖 lines (once); this loop skips hermes so it is not shown
+ // twice. Making the poll silent instead would print the reply nowhere.
this._chatPoll(true).then(j => {
if (!j) { this.terminal.print('❌ no answer from the chat endpoint', 'error'); return; }
- const msgs = j.messages || [];
- if (!msgs.length) this.terminal.print('(no new messages)', 'info');
+ const incoming = j.messages || [];
+ // hermes lines were already printed by the poll above; only count them as
+ // 'new' so the box does not say "no new messages" right under a new one
+ const msgs = incoming.filter(m => m.author !== 'hermes');
+ if (!incoming.length) this.terminal.print('(no new messages)', 'info');
msgs.forEach(m => this.terminal.print(
(m.author === 'hermes' ? '🤖 ' : m.author === 'agent' ? '🧠 ' : '👤 ') + m.text, ''));
if (j.awaiting) this.terminal.print('⏳ waiting for Hermes', 'info');
diff --git a/modules/core/module-loader.js b/modules/core/module-loader.js
index b177626..acff509 100644
--- a/modules/core/module-loader.js
+++ b/modules/core/module-loader.js
@@ -4,7 +4,7 @@
// Core module bundle loaded synchronously in main.html.
// This loader only handles version cache-busting, splash, terminal init, and optional ext modules.
- const LATEST_VER = 'v4.6.77';
+ const LATEST_VER = 'v4.6.80';
const CACHE = '?v=' + LATEST_VER;
const stored = localStorage.getItem('webx11_cache_ver');
if (stored !== LATEST_VER) {
diff --git a/scripts/chat.py b/scripts/chat.py
index 5b44b64..821bb31 100755
--- a/scripts/chat.py
+++ b/scripts/chat.py
@@ -24,11 +24,22 @@ def now_ms():
def load(path):
- try:
- with open(path) as f:
- return json.load(f)
- except Exception:
+ """Read a thread. Missing is empty; corrupt is an error.
+
+ Returning [] for both let reply() rewrite a damaged thread with a single message —
+ history gone, and nothing said so.
+ """
+ if not os.path.exists(path):
return []
+ with open(path) as f:
+ raw = f.read()
+ try:
+ data = json.loads(raw)
+ except json.JSONDecodeError as exc:
+ raise SystemExit('thread file is corrupt (%s): %s' % (path, exc))
+ if not isinstance(data, list):
+ raise SystemExit('thread file is not a list: ' + path)
+ return data
def threads():
@@ -38,7 +49,14 @@ def threads():
for name in sorted(os.listdir(CHAT_DIR)):
if not name.endswith('.json') or name.startswith('.'):
continue
- msgs = load(os.path.join(CHAT_DIR, name))
+ try:
+ msgs = load(os.path.join(CHAT_DIR, name))
+ except SystemExit as exc:
+ # one unreadable file must not make the whole listing fail: that is how
+ # awaiting messages silently stop being listed
+ out.append({'thread': name[:-5], 'count': -1, 'last_ts': '', 'last_author': 'corrupt',
+ 'awaiting': True, 'preview': 'CORRUPT: ' + str(exc)[:90]})
+ continue
if not msgs:
continue
last = msgs[-1]
@@ -62,6 +80,10 @@ def reply(thread, text):
raise SystemExit('no such thread: ' + thread)
lockpath = os.path.join(CHAT_DIR, '.lock-' + thread)
with open(lockpath, 'a+') as lock:
+ # created here as darkbot (0644 under umask 022); PHP opens the same lock as
+ # www-data, so group write is mandatory or later posts to this thread 500
+ try: os.chmod(lockpath, 0o664)
+ except OSError: pass
fcntl.flock(lock, fcntl.LOCK_EX)
try:
msgs = load(path)
@@ -102,10 +124,15 @@ def main():
return 0
for t in rows:
flag = 'AWAITING' if t['awaiting'] else 'replied '
- print(f"{t['thread']} {flag} {t['count']:>3} msg {t['last_ts']} {t['preview']}")
+ if t['count'] == -1:
+ flag = 'CORRUPT '
+ print(f"{t['thread']} {flag} {max(t['count'], 0):>3} msg {t['last_ts']} {t['preview']}")
return 0
if cmd == 'show' and len(sys.argv) >= 3:
+ # reply() validated the id, show() did not: '../../x' joined straight onto CHAT_DIR
+ if not THREAD_RE.match(sys.argv[2]):
+ raise SystemExit('invalid thread id (expected 16 lowercase hex chars)')
msgs = load(os.path.join(CHAT_DIR, sys.argv[2] + '.json'))
if not msgs:
print('(empty or unknown thread)')