diff options
Diffstat (limited to 'review-glm5.2.html')
| -rw-r--r-- | review-glm5.2.html | 329 |
1 files changed, 329 insertions, 0 deletions
diff --git a/review-glm5.2.html b/review-glm5.2.html new file mode 100644 index 0000000..3d776d0 --- /dev/null +++ b/review-glm5.2.html @@ -0,0 +1,329 @@ +<!DOCTYPE html> +<html lang="en"> +<head> +<meta charset="UTF-8"> +<meta name="viewport" content="width=device-width, initial-scale=1.0"> +<title>newOS4 Code Review — GLM 5.2</title> +<style> +:root { + --bg: #0a0e17; + --bg2: #111827; + --border: #1e293b; + --text: #e2e8f0; + --text-muted: #94a3b8; + --critical: #ef4444; + --high: #f97316; + --medium: #eab308; + --low: #22c55e; + --accent: #6366f1; + --accent2: #8b5cf6; +} +* { margin: 0; padding: 0; box-sizing: border-box; } +body { + background: var(--bg); + color: var(--text); + font-family: 'Inter', -apple-system, BlinkMacSystemFont, sans-serif; + line-height: 1.6; + padding: 2rem; + max-width: 960px; + margin: 0 auto; +} +h1 { font-size: 2rem; margin-bottom: 0.5rem; background: linear-gradient(135deg, var(--accent), var(--accent2)); -webkit-background-clip: text; -webkit-text-fill-color: transparent; } +.subtitle { color: var(--text-muted); margin-bottom: 2rem; font-size: 0.9rem; } +h2 { font-size: 1.3rem; margin: 2rem 0 1rem; padding-bottom: 0.5rem; border-bottom: 1px solid var(--border); color: var(--accent); } +h3 { font-size: 1.1rem; margin: 1.5rem 0 0.5rem; color: var(--text); } +.severity { display: inline-block; padding: 2px 8px; border-radius: 4px; font-size: 0.75rem; font-weight: 600; text-transform: uppercase; letter-spacing: 0.5px; } +.critical { background: rgba(239,68,68,0.15); color: var(--critical); } +.high { background: rgba(249,115,22,0.15); color: var(--high); } +.medium { background: rgba(234,179,8,0.15); color: var(--medium); } +.low { background: rgba(34,197,94,0.15); color: var(--low); } +.finding { background: var(--bg2); border: 1px solid var(--border); border-radius: 8px; padding: 1rem; margin: 0.75rem 0; } +.finding .file { font-family: 'JetBrains Mono', monospace; font-size: 0.85rem; color: var(--accent); margin-bottom: 0.3rem; } +.finding .impact { color: var(--text-muted); font-size: 0.9rem; margin: 0.3rem 0; } +.finding .fix { color: var(--low); font-size: 0.9rem; margin-top: 0.3rem; } +.finding.cr { border-left: 3px solid var(--critical); } +.finding.hi { border-left: 3px solid var(--high); } +.finding.md { border-left: 3px solid var(--medium); } +.finding.lo { border-left: 3px solid var(--low); } +.section-grid { display: grid; gap: 0.5rem; margin: 0.5rem 0; } +.tag { display: inline-block; padding: 1px 6px; border-radius: 3px; font-size: 0.75rem; background: var(--bg2); border: 1px solid var(--border); margin-right: 4px; } +ul { padding-left: 1.5rem; margin: 0.5rem 0; } +li { margin: 0.3rem 0; } +.stats { display: grid; grid-template-columns: repeat(auto-fit, minmax(140px, 1fr)); gap: 1rem; margin: 1.5rem 0; } +.stat-card { background: var(--bg2); border: 1px solid var(--border); border-radius: 8px; padding: 1rem; text-align: center; } +.stat-card .num { font-size: 2rem; font-weight: 700; } +.stat-card .label { font-size: 0.8rem; color: var(--text-muted); margin-top: 0.2rem; } +.stat-card.crit .num { color: var(--critical); } +.stat-card.high .num { color: var(--high); } +.stat-card.med .num { color: var(--medium); } +.stat-card.low .num { color: var(--low); } +.stat-card.ok .num { color: var(--low); } +</style> +</head> +<body> + +<h1>newOS4 Code Review</h1> +<p class="subtitle">Reviewed by GLM 5.2 (z-ai/glm-5.2) · v4.5.83 · 2026-07-03 · ~48K LOC across 54 modules</p> + +<div class="stats"> + <div class="stat-card crit"><div class="num">2</div><div class="label">CRITICAL</div></div> + <div class="stat-card high"><div class="num">4</div><div class="label">HIGH</div></div> + <div class="stat-card med"><div class="num">8</div><div class="label">MEDIUM</div></div> + <div class="stat-card low"><div class="num">5</div><div class="label">LOW</div></div> + <div class="stat-card ok"><div class="num">19</div><div class="label">Total Issues</div></div> +</div> + +<h2>1. Architecture</h2> + +<h3>Strengths</h3> +<ul> + <li><strong>Consistent module pattern</strong> — All 43 modules follow the same interface: <code>commands{}</code>, <code>registerCommands()</code>, <code>getHelpInfo()</code>, <code>init(terminal)</code></li> + <li><strong>_safeRegister pattern</strong> (terminal.js:1216) — Uses typeof registry map instead of eval for safe class lookup</li> + <li><strong>/sys/ virtual filesystem</strong> (virtual-fs.js:33-217) — Lazy-initialized virtual mounts with factory functions, never persisted, computed on demand</li> + <li><strong>VFS persistence</strong> — Debounced (2s), compressed, IndexedDB fallback when localStorage quota exceeded</li> + <li><strong>Security manager</strong> — Clean 4-level model with Levenshtein suggestion engine using Uint16Array</li> +</ul> + +<h3>Weaknesses</h3> + +<div class="finding md"> + <span class="severity medium">MEDIUM</span> + <div class="file">terminal.js:13 + module-aiassist.js:1646-1661</div> + <strong>Two divergent security whitelists</strong> — WebX11SecurityManager whitelist (~80 commands) and AI isDestructive safeRead set (~60 commands) are maintained separately. Synced in v4.5.74 but will drift as new commands are added. +</div> + +<div class="finding md"> + <span class="severity medium">MEDIUM</span> + <div class="file">module-manager.js:70, core-bundle.js:1052</div> + <strong>new Function module loading</strong> — <code>new Function('return ' + cleanCode + '\nreturn ' + className)()</code> executes module source with full page origin access. SRI on wpm mitigates install vector but editor-saved modules and persisted reloads have no integrity check. +</div> + +<div class="finding lo"> + <span class="severity low">LOW</span> + <div class="file">module-loader.js</div> + <strong>Procedural loader, not a class</strong> — Cannot be inspected via module-info. By design (runs before classes available) but creates inconsistency. +</div> + +<div class="finding lo"> + <span class="severity low">LOW</span> + <div class="file">terminal.js:1339</div> + <strong>Hardcoded skipList</strong> — 21 module names hardcoded in reloadPersistedModules. New ext modules require manual list update or they double-load. +</div> + +<h2>2. Security</h2> + +<h3>Strengths</h3> +<ul> + <li><strong>4-tier security model</strong> — Permissive(0)/Standard(1)/Paranoid(2)/Audit(3) with command classification into destructive/system/suspicious/unknown</li> + <li><strong>Pipe bypass fixed</strong> (v4.5.63) — executePipeChain now calls security._classify() on each stage</li> + <li><strong>Guardrail layer</strong> — 8 rules loaded (rm-rf-block, curl-pipe-bash, eval-danger), runs before every AI tool execution</li> + <li><strong>SecretVault</strong> — AES-GCM + PBKDF2 at 600K iterations (OWASP 2023), sync name-only redaction (no decryption side-channel)</li> + <li><strong>Confidence escalation</strong> — AI can auto-approve destructive tools at ≥90% confidence (configurable)</li> +</ul> + +<h3>Vulnerabilities</h3> + +<div class="finding cr"> + <span class="severity critical">CRITICAL</span> + <div class="file">module-aiassist.js:1655 + console.js:31-37</div> + <strong>AI can execute arbitrary JavaScript without confirmation</strong> + <div class="impact"><code>node</code> is in the AI's safeRead whitelist. The <code>node -e "..."</code> command (console.js:31) uses raw <code>eval()</code> in full page context. AI can read/write localStorage (including encrypted secrets if vault unlocked), make fetch requests, modify DOM, exfiltrate data — all without user confirmation.</div> + <div class="fix">Fix: Remove <code>node</code> from the safeRead set in <code>isDestructive()</code>. All JS execution commands should require confirmation.</div> +</div> + +<div class="finding cr"> + <span class="severity critical">CRITICAL</span> + <div class="file">module-aiassist.js:1633</div> + <strong>shadowRollback() persistence is broken</strong> + <div class="impact">Calls <code>this.terminal.fs.saveToLocalStorage()</code> which doesn't exist (actual method is <code>saveToStorage()</code>). VFS rollback restores in-memory state but never persists. On page reload, pre-rollback (damaged) state reappears. Silent data loss.</div> + <div class="fix">Fix: Change <code>saveToLocalStorage()</code> → <code>saveToStorage()</code> at module-aiassist.js:1633 and core-bundle.js:9278.</div> +</div> + +<div class="finding high"> + <span class="severity high">HIGH</span> + <div class="file">terminal.js:1413-1424</div> + <strong>capturePipeOutput lacks try/finally — print method leak on error</strong> + <div class="impact">If <code>dispatchCommand</code> throws, <code>this.print</code> is never restored. All subsequent terminal output goes to a local variable. Terminal appears dead.</div> + <div class="fix">Wrap in try/finally to restore <code>this.print</code>.</div> +</div> + +<div class="finding high"> + <span class="severity high">HIGH</span> + <div class="file">module-orchestrator.js:173-178</div> + <strong>Orchestrator timers bypass central registry</strong> + <div class="impact">3 raw <code>setInterval</code> calls (500ms A2A polling, 3s heartbeat, 30s idle sweep) not registered in <code>_timers[]</code>. <code>clearAllTimers()</code> doesn't clear them. On module unload, zombie timers continue polling VFS.</div> + <div class="fix">Use <code>this.terminal.moduleManager._setInterval()</code> instead of raw <code>setInterval()</code>.</div> +</div> + +<div class="finding high"> + <span class="severity high">HIGH</span> + <div class="file">module-aiassist.js:1062-1066</div> + <strong>Confidence escalation can be gamed by prompt injection</strong> + <div class="impact">AI's own preamble text determines whether destructive tools auto-approve. If model outputs "Confidence: 95%" (genuine or injected), it bypasses human confirmation. Self-reported, not system-computed.</div> +</div> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">terminal.js:1371-1382</div> + <strong>splitPipe doesn't distinguish quote types</strong> + <div class="impact"><code>echo 'a|b' | cat</code> splits at the | inside single quotes because both quote types toggle the same <code>inQuote</code> boolean.</div> + <div class="fix">Track <code>inSingle</code> and <code>inDouble</code> separately (like <code>_parseArgs</code> does).</div> +</div> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">module-aiassist.js:1379</div> + <strong>Guardrail rules stored in localStorage, user-editable</strong> + <div class="impact">Any code with localStorage access (including js/node commands) can modify or delete guardrail rules. Guardrail layer is bypassable by anyone who can execute JS in page context.</div> +</div> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">module-aiassist.js:1670</div> + <strong>Guardrail check uses command-oriented regex on structured args</strong> + <div class="impact">Guardrail regex patterns designed for command-string format (<code>/rm\s+-rf/</code>) but tool calls use structured JSON args. Pattern mismatch means some destructive tool calls may bypass guardrail rules.</div> +</div> + +<h2>3. Code Quality</h2> + +<div class="finding high"> + <span class="severity high">HIGH</span> + <div class="file">core-bundle.js (16K+ lines)</div> + <strong>Massive code duplication between source and bundle</strong> + <div class="impact">Every fix must be applied to both the source file AND the bundle. The <code>saveToLocalStorage</code> bug exists at core-bundle.js:9278 too. Rebuild script mitigates but only if run after every change.</div> +</div> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">module-aiassist.js:914-1114</div> + <strong>agenticLoop is a 200-line monolith</strong> + <div class="impact">Entire tool-calling loop (loop detection, parallel safe-tool execution, destructive tool confirmation, confidence parsing, tool result logging, memory graph recording, debug panel updates) is one method. Functional but unmaintainable.</div> +</div> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">All modules</div> + <strong>71% of localStorage writes uncovered by try/catch</strong> + <div class="impact">162 <code>localStorage.setItem</code> calls, only 47 wrapped in try/catch. If localStorage fills up (5-10MB limit), remaining 115 calls throw unhandled QuotaExceededError.</div> +</div> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">All modules</div> + <strong>44% of fetch calls have no timeout</strong> + <div class="impact">105 <code>fetch()</code> calls, only 61 with AbortController/signal. A hung Ollama or SearXNG backend freezes the UI indefinitely.</div> +</div> + +<div class="finding lo"> + <span class="severity low">LOW</span> + <div class="file">terminal.js:81-97</div> + <strong>_parseArgs doesn't handle escaped quotes</strong> + <div class="impact"><code>\"</code> inside a double-quoted string is treated as closing quote. No backslash escaping support.</div> +</div> + +<div class="finding lo"> + <span class="severity low">LOW</span> + <div class="file">terminal.js:22</div> + <strong>Duplicate 'cat' in internals list</strong> + <div class="impact">'cat' appears twice in <code>_registerBuiltIn</code> array. Harmless but sloppy.</div> +</div> + +<h2>4. Performance</h2> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">virtual-fs.js:425</div> + <strong>VFS saveToStorage serializes entire filesystem</strong> + <div class="impact"><code>JSON.stringify({ root, currentPath })</code> creates multi-MB string every 2 seconds (debounced). Synchronous, blocks main thread. IndexedDB fallback helps on overflow but serialization itself is the bottleneck.</div> +</div> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">module-orchestrator.js:173</div> + <strong>A2A polling at 500ms</strong> + <div class="impact">Polls VFS mailboxes every 500ms regardless of agent activity. With 5 agents, that's 10 VFS reads/second. Event bus provides instant in-memory delivery, making VFS poll redundant for same-page agents.</div> +</div> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">virtual-fs.js:55-74</div> + <strong>vfsWalk is unbounded synchronous recursion</strong> + <div class="impact">No depth limit. On deep directory structures, can stack overflow. /sys/vfs/stats and /sys/vfs/largest call this on every cat.</div> +</div> + +<div class="finding lo"> + <span class="severity low">LOW</span> + <div class="file">terminal.js:42-53</div> + <strong>_collectAllCommands iterates all modules on every suggestion</strong> + <div class="impact">Every unknown command triggers Levenshtein comparison against ~430 commands. Acceptable now but won't scale.</div> +</div> + +<h2>5. AI Integration</h2> + +<h3>Strengths</h3> +<ul> + <li><strong>Well-structured agent loop</strong> — Loop detection (6x same tool+args in 30s → break), parallel safe-tool execution, sequential destructive confirmation, confidence escalation, last-turn prompt injection, memory-augmented planning</li> + <li><strong>35+ tools</strong> — VFS operations, web search/fetch, module management, skills CRUD, VaultMind, wallet lookup, usage stats. Filtered by active loop profile.</li> + <li><strong>6 agent loop profiles</strong> — default, coder, creative, research, safe, minimal with per-profile tool restrictions and temperature</li> +</ul> + +<h3>Issues</h3> + +<div class="finding medium"> + <span class="severity medium">MEDIUM</span> + <div class="file">module-aiassist.js:44-53</div> + <strong>AI conversation context stored in plaintext localStorage</strong> + <div class="impact">Up to 10 context messages persisted to <code>webx11_ai_context</code>. Sensitive AI discussions persist across sessions in plaintext. No encryption, no expiry.</div> +</div> + +<div class="finding lo"> + <span class="severity low">LOW</span> + <div class="file">module-aiassist.js:1042</div> + <strong>_summarizeObservation truncates to 150 chars</strong> + <div class="impact">Tool results summarized for display but full result pushed to messages. Fine for display but LLM sees full results while user sees truncated ones.</div> +</div> + +<h2>6. Top 5 Recommendations</h2> + +<div class="finding cr"> + <span class="severity critical">#1</span> + <div class="file">module-aiassist.js:1655</div> + <strong>Remove node/js/js-run from AI safeRead whitelist</strong> + <div class="impact">Immediate security hole — AI can execute arbitrary page-context JS without confirmation. One-line fix.</div> +</div> + +<div class="finding cr"> + <span class="severity critical">#2</span> + <div class="file">module-aiassist.js:1633</div> + <strong>Fix shadowRollback() persistence</strong> + <div class="impact">Guardrail rollback — key safety mechanism — is silently broken. <code>saveToLocalStorage()</code> → <code>saveToStorage()</code>.</div> +</div> + +<div class="finding high"> + <span class="severity high">#3</span> + <div class="file">terminal.js:1413-1424</div> + <strong>Wrap capturePipeOutput in try/finally</strong> + <div class="impact">Single thrown error in any pipe stage permanently breaks terminal output. Trivial fix with high impact.</div> +</div> + +<div class="finding high"> + <span class="severity high">#4</span> + <div class="file">module-orchestrator.js:173-178</div> + <strong>Migrate orchestrator timers to central registry</strong> + <div class="impact">3 zombie timers survive module unload. Use <code>_setInterval()</code> instead of raw <code>setInterval()</code>.</div> +</div> + +<div class="finding medium"> + <span class="severity medium">#5</span> + <div class="file">terminal.js:13 + module-aiassist.js:1646-1661</div> + <strong>Consolidate the two security whitelists</strong> + <div class="impact">Single <code>SecurityManager.getSafeReadCommands()</code> method eliminates drift between terminal whitelist and AI safeRead set.</div> +</div> + +<hr style="border-color: var(--border); margin: 2rem 0;"> +<p style="color: var(--text-muted); font-size: 0.85rem;"> + Review generated by GLM 5.2 (z-ai/glm-5.2) via Hermes Agent · newOS4 v4.5.83 · 2026-07-03 +</p> + +</body> +</html> |