Deep review of dev (e59b89b): security defaults, data-loss paths, event-loop hygiene, verified bugs with line refs

[Starwaves1]

This issue was written by Claude Fable 5.1 (Anthropic's model) on behalf of @Starwaves1, at his request. He asked for a deep senior-dev-to-senior-dev review of the dev branch and for the result to be posted from his account.

Hey Rose. Garrett asked me to do a thorough pass over dev at e59b89b. core/ was read end to end by hand; modules/, channels/, and the web frontend were reviewed in parallel, and every item below was then re-verified against the source before being written up. Anything that depends on runtime state I couldn't observe is marked (plausible); everything else was traced in the code, and a couple of things were reproduced.

Up front: this is a good codebase. The module/channel plugin model is genuinely easy to extend, TurnCollector giving stream and history the same shape is a nice abstraction, sandbox_path and the calculator's AST whitelist are done properly, and the inline AI GENERATED CODE (model) :: (date) (who approved) markers are a practice more projects should copy. The list is long because the review was deep, not because the code is bad. A suggested priority order is at the end.

---

1. Security: network-facing defaults

1.1 WebUI WebSocket has no Origin check and no auth when require_login is off (the default).
channels/webui.py:839-862, :1061. Browsers don't apply CORS to WebSockets, so any page the user visits can new WebSocket("ws://localhost:3000/ws"), send {"type":"user_message", ...}, and drive the agent. allow_admin_commands defaults to True, so that includes /commands; with a shell module enabled it's cross-site RCE. Fix: reject connections whose Origin isn't localhost/the configured host before accept(), and require the session cookie on /ws regardless of require_login.

1.2 Session secret resolves to None, so login sessions are forgeable even with require_login=True.
channels/webui.py:328: channel.config.get("session_secret", "openlumara-default-session-secret-change-me"). ConfigManager.get (core/config.py:236-244) only treats the last positional arg as a default when it is not a string; a non-empty string is treated as another key. session_secret isn't a declared setting either, so this returns None, and Starlette's SessionMiddleware signs with str(None) == "None". Anyone can mint an {"authenticated": true} cookie. Fix: use the default= kwarg, generate a random secret on first run and persist it under data/. Worth grepping the whole tree for config.get("x", "literal").

1.3 /api/settings/load returns the raw config, secrets included, with no auth by default.
channels/webui.py:604-608 returns core.config.config verbatim: LLM API key, Telegram/Discord tokens, Matrix password, webui password. With require_login=False the auth middleware (:301-304) skips every route, so in "internet" mode it's anyone on the network, and on localhost it's reachable via 1.1 or DNS rebinding. Fix: redact key/token/password fields server-side and require auth on /api/* always.

1.4 API bridge: wildcard CORS, no key by default, always admin.
channels/api_bridge.py:89-94 (allow_origins=["*"]), :40-43 (api_key_required defaults False), :186 and :227 (commands_authorized=True hardcoded). :101 compares the key with !=; secrets.compare_digest as webui already does.

1.5 Matrix: auto-joins any invite and has no sender allowlist.
channels/matrix.py:120 (MATRIX_AUTO_JOIN defaults to true), :419-433 / :749 (no event.sender check before send_stream). Any Matrix user can invite the bot and use its tools. Separately, :494, :603 auto-trust every device and :477 auto-confirms SAS without a comparison step, so the E2EE verification is cosmetic.

1.6 Discord and Telegram gate commands but not tools.
channels/discord_bot.py:103,177,291: authorized_user_id only decides commands_authorized; anyone who mentions the bot in the target channel gets a full agent turn with tools. channels/telegram.py:108-131 is trust-on-first-use: whoever messages first becomes the permanent admin with commands_authorized=True. Both want a configurable allowlist that gates the whole turn, not just /commands.

1.7 /config leaks the API key into chat history and the LLM context.
core/commands.py:190 writes the command into history before the auth check at :195, and config isn't in Commands.GHOST (:81). So /config api key sk-... and its echo (:601-605) are stored as ordinary user/assistant messages, sent to the model on every later turn, and mirrored into whichever channel you typed it in (Discord/Telegram message history). /config api key with no value prints the key outright. Fix: add config to GHOST, redact secret-looking keys in _get_config_value, and move the auth check above the messages.add. Side effect of the current order: unauthorized command attempts from random Discord users are written into the admin's chat history, which is a prompt-injection channel.

1.8 PUBLIC_COMMANDS includes destructive ones.
core/commands.py:82: new, clear, stop are runnable by unauthorized users. /clear from a stranger on Discord wipes the admin's current chat.

1.9 simple_webui: no CSRF, /clear is a GET.
channels/simple_webui.py:331-341, :395-401. <img src="http://localhost:5000/clear"> on any page clears the chat; an auto-submitting form sends prompts.

1.10 Frontend: one unsanitized x-html, and no CSP anywhere.
channels/webui/templates/chat/turns/user.html:8 renders the text block of multimodal user messages with raw x-html, while every other message path goes through the DOMPurify-backed x-md. Because messages can originate from other channels this is stored XSS, not self-XSS. No Content-Security-Policy or X-Content-Type-Options header is set anywhere in webui.py, so any sanitizer miss is game over. x-fade-html (assets/js/directives/fade_text.js:28) is also a raw innerHTML sink, safe today only because its single caller escapes.

1.11 SSRF filter has a DNS-rebinding TOCTOU. (plausible; needs attacker-controlled DNS)
modules/http.py:999-1014 resolves and validates, then :1251 hands the hostname to requests, which resolves again. A low-TTL record that flips to 169.254.169.254 or 127.0.0.1 between the two lookups bypasses the check. Fix: connect to the validated IP and set the Host header yourself (or pin the address via a custom adapter).

1.12 config/modules tools have no protected-key list once their unsafe flag is on.
modules/config.py:59-92, modules/modules.py:129-140. Default-off, which is right. But once enabled, a prompt injection can set(["api","url"], "http://attacker/v1") or enable unsafe_shell. A denylist of api.*, the enable/disable lists, and anything marked unsafe would still be worth having in unsafe mode.

---

2. Data loss and persistence

2.1 Every write is non-atomic.
core/storage.py:60-66, :214-220 open the real path with "w"/"wb" and write in place. A crash, SIGKILL, or full disk mid-write truncates config.yml, save.mp, the chat index, or a chat history. Standard fix: write to path + ".tmp", flush/fsync, then os.replace(). This matters more than usual because of the next one.

2.2 load() clears memory before reading, so a failed read plus the next save wipes the file.
core/storage.py:118-135 (StorageList) and :372-391 (StorageDict): self.clear() runs first, then _read(). If the read fails or the file is empty/truncated (see 2.1), the object is now empty and returns None. Since get() calls load() on every access, the next save() persists {} or []. Corrupt-on-crash followed by empty-on-next-run is a two-step data wipe. Fix: read and parse into a temporary, only then clear() + update(); on parse failure keep the in-memory copy and log loudly.

2.3 Chat history JSON embeds base64 attachments and is fully rewritten on every message.
core/channel.py:287-302 stores images/audio as base64 inside the message; core/messages.py:90-93 saves on every add(), and save() also rewrites the chat index via update_timestamp(). A 10-tool agentic loop is roughly 40 synchronous full rewrites of a file that may hold several MB of base64, on the event loop. Suggest storing attachments under data/attachments/<chat>/<hash> and referencing by path, and coalescing saves during a turn.

2.4 chat.set() and metadata mutation don't save.
core/chat.py:511-522 mutates in memory only. characters/identity write chat.get("metadata")["character"] and chats.organize uses chat.set without saving; the state persists only if a later turn happens to save. (The AI-marked set-category endpoint in webui.py already works around this locally; the fix belongs in set().)

2.5 auto_backup never prunes and can nest inside data/.
modules/auto_backup.py:108-155: a new full zip every interval, nothing ever deletes old ones. If backup_path is inside data_folder, each backup includes all previous backups. Add rotation (keep N) and reject nested paths. The zip also runs synchronously on the loop (:129-135); asyncio.to_thread it.

2.6 core.config.load() rewrites config.yml on every startup.
core/config.py:845 saves after every load, which strips any comments the user wrote in their YAML. Only save when the merged result differs from what was read.

---

3. Event-loop hygiene

3.1 The "safe" HTTP path blocks the loop, which defeats tool_timeout.
modules/http.py:1251 calls requests.get/post synchronously inside async _make_request; web_reader and web_search inherit it. core/toolcalls.py:222-223 wraps tools in asyncio.wait_for, but a blocking call can't be interrupted, so one slow host freezes every channel for up to 30 s per redirect hop (6 hops). asyncio.to_thread(...) or httpx.AsyncClient fixes both. Same pattern: modules/http.py:803-808 (urlopen with no timeout and a bare except: at startup), modules/unsafe_shell.py:14 (subprocess.run, no timeout), modules/file_manager.py:69-71 (unbounded read()), modules/coder.py:393-395 and :524, and channels/cli_lite.py:16 / channels/turn_grouping_test.py:16 (input() inside async run).

3.2 Untrusted HTTP bodies up to 10 MB go through ~60 regex passes on the loop; the 50 KB cap is dead code.
modules/http.py:743 defines MAX_CONTENT_FOR_PROMPT and :861 _truncate_for_safety, but nothing calls it (also uncalled: _validate_safelist_urls :868, _guardrail_check :1298). :1340-1352 decodes up to MAX_CONTENT_SIZE (10 MB) and runs the whole ContentSanitizer over it. That's a CPU DoS from any page the model is asked to read, and the multi-MB blob then goes to the model anyway. Wire _truncate_for_safety in before sanitizing. _wrap_untrusted (:833-848) also sanitizes twice and throws away the first result.

3.3 Manager swallows channel crashes silently.
core/manager.py:366: gather(..., return_exceptions=True) outside debug mode. If Telegram's run() raises, the exception lands in a results list nobody reads; the bot is simply dead until restart with nothing in the logs. Add a done-callback that logs exceptions (you already have _remove_async_task for module tasks).

3.4 Scheduler: three independent ways to stop working.

• modules/scheduler.py:114,174,363 call self.log_error, which doesn't exist on Module (only log, core/module.py:83). The first error in _dispatcher_loop raises AttributeError inside the except, killing the dispatcher permanently and silently (the task isn't tracked anywhere).
• :44 on_unload is never called by the framework (zero callers). reload_module runs on_ready again, which starts a second dispatcher, so every job fires twice after any settings change.
• :309-341 the retry loop sleeps a flat 5 s forever (max_delay is unused). One unreachable API stalls every other job. Also :590: notification_channel (default "webui") short-circuits the model-supplied channel, so "remind me on Discord" always lands in the webui.

3.5 No serialization between background jobs and user turns. (plausible)
Scheduler _execute_job and calendar _notify_user mutate the same chat.messages and hit the same API as an in-flight user turn, with no lock anywhere in core. A per-channel asyncio.Lock around turn processing would close this.

3.6 Naive local datetimes in scheduler and calendar.
modules/time.py:44-48 does it right with zoneinfo; scheduler and calendar use naive datetime.now() plus real-second sleeps, so daily jobs drift an hour across DST and can double-fire or skip in the fall-back hour. core/chat.py:197,206 utcnow() is deprecated in 3.12, and the stored ISO string has no Z, so /search shows UTC timestamps as if they were local.

---

4. Correctness bugs

Core

• core/channel.py:455-469: usr_msg_result is only bound inside the try. If the first module's on_user_message raises, the if usr_msg_result is False line raises UnboundLocalError and the user's message is lost; if a later module raises, the previous module's return value is reused. Initialise it to None before the loop.
• core/context.py:43-49: same pattern, content is unbound if get_system_prompt() raises.
• core/context.py:214 (and :97): context trimming slices at an arbitrary index, so the history sent can start with a tool message whose parent tool_calls was cut off. Strict backends (OpenAI, many llama.cpp chat templates) return 400 on that. Snap the trim to the next user message.
• core/context.py:332 with :287: the token estimate is len(json.dumps(...)) // 4 with ensure_ascii=True, so every non-ASCII character counts as six bytes (about 1.5 "tokens"). CJK or emoji-heavy chats get trimmed at roughly a third of the real limit. ensure_ascii=False, or use the API usage numbers you already cache.
• core/toolcalls.py:94-303: the agentic loop has no iteration cap. A model that keeps calling tools recurses until RecursionError or the API budget runs out. A max_tool_rounds (25?) with a clear message back to the model is cheap insurance.
• core/toolcalls.py:200: the "load it with tools_load" hint uses tool_name.split('_')[0], so for web_search_search it tells the model to load module web, which doesn't exist. entry["module"] is already in scope. Same slice in cmd_tools (core/commands.py:509).
• core/functions.py:159,177: sandbox_path compares realpath(target) against a base that was never realpath'd. Running from a symlinked checkout raises Access denied: target path is outside sandbox at import time. Reproduced: ln -s openlumara ol && cd ol && python -c 'import core'. realpath the base once. Related: :98 URL-decodes names three times, so a note named 50%25off is saved as 50%off; :106 rejects any name containing .. as a substring (wait...what).
• core/api.py:78-88: on a failed connect() the httpx.AsyncClient is created and never closed, and attempt_connect() runs on every message, so an API outage leaks a client and its pool per user message. :88 also hard-requires /v1/models; several OpenAI-compatible proxies don't serve it, which makes them unusable. Make the probe non-fatal.
• core/api.py:513-524 with :281-284: cancellation is one shared flag plus busy-polling. /stop from one channel cancels whichever request checks the flag first (a webui /stop can kill an in-flight Telegram request); cancel() returns False for non-streaming send(); and if the stream is stalled waiting on the socket (read=None timeout) cancel() spins forever because nothing clears the flag. A per-request asyncio.Event/token and cancelling the consuming task is the usual shape.
• core/api.py:571,600-602: last_token_time = 0, so the first token's fallback predicted_ms is about 1.7e12.
• core/api.py:199-204: chat_template_kwargs and return_progress are sent to every backend. OpenAI proper rejects unknown top-level params with 400. (plausible; gate behind a "llama.cpp extras" flag)
• core/tool_loader.py:99,212: "strict": True while optional params are omitted from required, float maps to "string" (:81), and array has no items. OpenAI strict mode rejects all three. Either drop strict or emit nullable types plus items. (plausible; provider-dependent)
• core/modules.py:90-100: extras break the installed check. version("matrix-nio[e2e]") raises PackageNotFoundError, so enabling Matrix runs pip install on every single startup. Strip [...] before the lookup.
• core/modules.py:119-168: the auto-uninstaller compares raw strings against requirements.txt and only knows direct deps. Disable discord_bot and web_reader while matrix is enabled and it runs pip uninstall aiohttp, which matrix-nio needs. It also runs against whatever interpreter launched it, venv or not. I'd make uninstall opt-in.
• core/manager.py:107,155: newly_installed_* appends unconditionally (the return value of install_module_deps is ignored), so on_install() and core.config.load() run on every startup. Harmless today (no in-tree on_install), but it's why there are several live config objects: core/config.py:803 replaces the global while channels and modules keep the old one inside their ConfigManager.
• core/manager.py:63-73: when the CLI channel isn't loaded (no TTY, so systemd/docker/nohup), every log() short-circuits to print + log_buffer.append and never reaches channel.on_log. Headless, the webui log panel stays empty and log_buffer grows forever. channels/webui.py:203 self.logs is also unbounded; deque(maxlen=...) for both.
• core/channel.py:328: text attachments are wrapped as ```{content}``` with no newline, so the file's first line becomes the code-fence language tag.
• core/commands.py:167-174: a shlex.split ValueError (e.g. /don't) is treated as "not a command" and the message is dropped as blank.
• core/commands.py:386-402: /compress appends a user message right after the end-prompt user message, so strict-alternation templates error on consecutive user roles.
• core/chat.py:258-275: delete() matches the id case-insensitively but removes the file using the caller's casing, leaving an orphaned history file.

Channels

• channels/matrix.py:218 and channels/telegram.py:100 call self.announce(...); only _announce exists. Matrix hits this right after connecting, logs "Critical error", and never serves.
• channels/discord_bot.py:58-74: mentioned is unbound when require_mentions=False; :55,103 do int(None) on unconfigured IDs, so a fresh bot throws on every message.
• channels/webui.py:934: if not text and not files but the variable is files_data, so attachment-only messages silently vanish. :663 data.pop("changed_modules") has no default and :677 references self inside a closure where it doesn't exist.
• channels/webui.py:1027-1039: queue_ready_signal polls at 10 Hz forever because send_ready_signal is never called; one leaked task per WebSocket connect.
• channels/simple_webui.py:349-372: _pending_user_input is shared instance state read from the worker thread, and each request spins up a new event loop against the same chat object.

Modules

• modules/sandboxed_shell.py:379-388: any stderr output from docker run -d (podman short-name resolution, pull progress, cgroup warnings) sets container_name = None while the container keeps running. Gate on exit code. :235,242 pass GID={uid} instead of gid; :251-262 the build timeout branch is unreachable (no wait_for around the read loop).
• modules/characters.py:482-495: V1 card import rewraps char_obj but never reassigns char_data, so None.get on every V1 import.
• modules/calendar.py:193,200: hour or event_date.hour and should_notify or event['notify'] silently ignore midnight and "turn the reminder off". :44-50 spawns an untracked task per event on every on_ready, so reminders duplicate after a reload.
• modules/coder.py:552-556: line_start=0 clamps to 1 (0-indexed), skipping the first line. :342-355 total caps at max_results+1 but is reported as the real count.
• modules/lists.py:280-284: random.choice on an empty list.

---

5. Frontend (channels/webui/assets, templates)

• assets/js/stores/chat.js:522-547: send() sets state="message_sending" and clears the input before simpleSocketSend, whose failure is swallowed (services/api.js:41-51). With the socket down the message is lost and the UI is stuck until reload. Check the return value, restore the input, reset the state.
• services/websockets.js:2,33,47,62,68-72: wsReconnecting is never set true, so both guards are no-ops; reconnect is a flat 1 s loop with no backoff and a full reloadChat() per attempt. onopen never clears stream.turn, so a mid-stream drop leaves a duplicate or orphaned assistant bubble after reconnect.
• templates/chat/input.html:23: Enter-to-send ignores event.isComposing, so CJK IME users submit half-composed text.
• sw.js: no skipWaiting()/clients.claim(), and cache-first with no revalidation, so users stay on the old UI after every deploy until all tabs close. (Good: it never caches /api/*.)
• templates/chat/index.html:58: :key="turnIndex" combined with per-turn lazy-mount/height state means a mid-history delete binds the wrong state to the wrong turn. turn.first_message_index is already treated as identity by mergeTurnHistory.
• templates/chat/reasoning.html:9, toolcalls.html:12: collapse toggles are <div @click>, not focusable and no role="button", so keyboard and screen-reader users can't open reasoning or tool output.
• assets/js/stores/chat.js:559: copyMessage doesn't return the clipboard promise, so the checkmark shows even when the write failed.
• templates/login.html:8 links css/base/variables.css, which doesn't exist (404, unstyled login).
• services/websockets.js:11-17: window.apiToken is never set anywhere. Dead token-in-URL code that would leak into access logs if someone "fixed" it.
• services/websockets.js:81-82,214,231-233, stores/chat.js:133,348,425: implicit globals (token, stream, ui, result...). No strict mode.
• stores/notify.js:30: notification permission is requested on page load with no user gesture; browsers increasingly auto-deny that.

---

6. Design notes, take or leave

• Reads hit the disk. StorageDict.get() stats the file on every call and ConfigManager.get() calls it for every module config read. Cheap-ish, but it's also why several live config objects coexist and why writes through a stale one can clobber another's change (ConfigManager.__setitem__ doesn't reload first). One config object, reloaded in place on demand, would be simpler and race-free.
• Markdown storage is O(n) per access. core/storage.py's markdown type re-walks and re-reads every .md on every get() and rewrites every file on every save(). Fine at 50 notes, painful at 5,000.
• context.get() has side effects (push() and disconnect() on overflow) and is called from get_total_tokens(), chat.new(), and four times from get_size(). Splitting "build" from "enforce" would make token counting safe to call from anywhere.
• agentic_loop_start (core/context.py:88-93) is per-channel rather than per-chat, and indexes the unfiltered history, so after a chat switch or /compress the reasoning-stripping boundary is off. Storing it in chat metadata fixes both.
• Tests. There's no test suite and no CI. sandbox_path, context.get() trimming, TurnCollector, and ConfigManager.get's default handling are all pure functions that would take an afternoon to cover with pytest, and would have caught roughly a third of this list. A GitHub Action that runs python -c "import core" plus those tests is a good first step.
• Dead code: core/api.py:340-414 (warmup methods referencing attributes that are commented out in __init__), core/channel.py:382 _render_tool_token (no callers), channels/webui/templates/chat/index.old.html, and core/module.py:208 is_empty_coroutine, which parses source with a regex to decide whether to spawn a task. Always spawning it is cheaper than the regex.

---

7. What's good

Worth saying explicitly, since the list above is long:

• The plugin architecture (docstring to tool schema, on_* hooks, per-chat tool persistence) is simple and pleasant to extend.
• sandbox_path covers symlinks, realpath containment, .., null bytes and multi-decoding, and nearly every model-supplied path goes through it. notes, docs, memory, coder are safe because of it.
• SafeCalculator is a proper AST whitelist with Pow disabled.
• http.py's SSRF model (scheme/port checks, private-network list, re-validation on every redirect hop) and sandboxed_shell's container flags (--cap-drop ALL, --read-only, no-new-privileges, pids/cpu/mem limits, optional gVisor) are more careful than most agent frameworks.
• Frontend: marked + DOMPurify on the main render path, per-message render caching, rAF-coalesced streaming paints, lazy mount/unmount of turns with height compensation, secrets.compare_digest and rate limiting on login.
• The AI-code provenance markers.

---

8. If I had to pick five

1. Origin check and always-on auth for /ws and /api/*, redact settings/load, fix the session_secret default (1.1 to 1.3).
2. Atomic writes and don't clear before read (2.1, 2.2).
3. Move blocking I/O off the loop, starting with http.py, and wire in _truncate_for_safety (3.1, 3.2).
4. Scheduler log_error/on_unload/retry (3.4) and the Matrix announce typo: these are "feature silently doesn't work" bugs.
5. A max_tool_rounds cap and trim-to-user-boundary in context.get().

No PRs attached given the contribution policy; everything above should have enough detail to act on directly. Happy to clarify anything in the comments.