What the audit is worth keeping, and where
Five passes produced a working document that said on its first line it was temporary. This is it being spent rather than abandoned. CLAUDE.md gains eleven paragraphs, each a thing that had shipped looking correct: a handler bound to a shared variable rather than its own socket, a script the base template already loads being loaded again, a control that stays clickable while it awaits permission, "is this name taken?" asked about visibility instead of ownership, root running a file the service account can write, sourcing anything under $PREFIX, a read-only command name that is not a read-only command, 0.0.0.0 being this machine, a folder that is not a label, a file that is not deleted by the row that named it, and a measuring harness that measured an unstyled page and reported a dramatic finding that was entirely an artefact. PLAN.md carries the seven things the audit found and deliberately did not fix, each with why: they change what something does rather than fix what it claims to do, which is not an audit's job. deploy/README.md says why root runs a copy, and that a host installed before this keeps the old wiring until the installer is re-run -- the button cannot fix it, because the button runs the old unit. docs/notes/audit-0.9.md is deleted, having been all three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -20,7 +20,9 @@ lembas info # paths + counts, useful when confused
|
||||
lembas secret-key # generate LEMBAS_SECRET_KEY
|
||||
lembas create-admin # create or promote an admin
|
||||
|
||||
pytest # 2088 tests, ~2min
|
||||
pytest # 2283 tests, ~2.5min
|
||||
pytest -m "not slow" # skips the 365 that stand up a real
|
||||
# server, shell, PTY or git repository
|
||||
# PLAN.md tracks what is and is not built
|
||||
ruff check . # lint (line length 100)
|
||||
python scripts/build_artwork.py # regenerate artwork (SVG + PWA icons;
|
||||
@@ -221,6 +223,10 @@ touching the code it names -- these are the same notes, not a summary.
|
||||
- `docs/notes/branding.md` -- the branding snapshot and why it is a Jinja global,
|
||||
where the instance name went and how an upgrade keeps it, how a custom theme
|
||||
inherits through `data-base`, and why `/branding.css` is a route.
|
||||
- `docs/notes/release-checklist.md` -- the manual pass before a release. Not a
|
||||
corner of the code but the opposite: the things a machine cannot test, which
|
||||
is everything needing a real endpoint, a real machine, real hardware or a real
|
||||
pair of eyes. Read it when cutting a release, not while writing code.
|
||||
- `docs/notes/subagents.md` -- the hidden chat a helper runs in, why `unattended`
|
||||
is a column and not a kind, the two halves that stop a helper stalling on a
|
||||
card nobody can see, what it may run and why Auto is never inherited, and where
|
||||
@@ -1645,6 +1651,107 @@ that the right one works, because only the second half would have passed
|
||||
throughout. When adding a control that writes, check the verb against the route,
|
||||
and assert on the row rather than on the response.
|
||||
|
||||
**A handler bound to a shared variable is bound to the wrong thing.**
|
||||
`terminal.js`'s `socket.onclose` set the module-level `socket` to null, and
|
||||
`close()` *queues* its event rather than firing it -- so on either reconnect
|
||||
path the old socket's close arrived **after** a new one had been assigned and
|
||||
nulled the live one. Output kept arriving, because `onmessage` is bound to the
|
||||
object; every send gates on the variable, so each keystroke was dropped and no
|
||||
resize was ever sent again, on a panel that looked perfectly healthy. It then
|
||||
said "Disconnected" about a shell that had just reconnected, because
|
||||
`closedOnPurpose` had already been cleared for the new connection. Each handler
|
||||
captures its own socket in a local and returns early when `socket !== opened`.
|
||||
|
||||
**A script the base template loads must not be loaded again by a page.**
|
||||
`messages/index.html` listed `composer.js` and `commands.js` in its `scripts`
|
||||
block and `base.html` already loads both, so that screen ran each **twice**.
|
||||
Each is an IIFE with its own state, and `stopPropagation()` does not stop a
|
||||
second listener already bound to the same node: two composer menus stacked, one
|
||||
Enter running a command twice, and `Alt+B`/`Alt+E`/`Alt+T`/`Alt+I` toggling
|
||||
their panel twice, which is to say doing nothing at all. Nothing about that
|
||||
looks like a double load. `tests/test_ui_js.py` sweeps every template against
|
||||
`base.html`'s list.
|
||||
|
||||
**Leave the state a control was in before you await.** `audio.js` moved the
|
||||
microphone out of `idle` only once `getUserMedia` had resolved, and the
|
||||
browser's permission prompt sits in between -- so every click while it was up
|
||||
opened another stream and another recorder, of which only the last was ever
|
||||
stopped. The browser's recording indicator then stayed on until the tab closed.
|
||||
`working` already existed and was already styled; it was the state the button
|
||||
was missing.
|
||||
|
||||
**"Is this name taken?" is a question about ownership, not visibility.**
|
||||
`skills.create` checked with `by_name`, which is scoped through
|
||||
`sharing.visible_to` -- owned *or* shared -- against a `(owner_id, name)`
|
||||
constraint. So a skill somebody shared with you took that name out of your
|
||||
library, and refused yours with "edit it instead", naming a row you cannot edit
|
||||
because sharing grants reading only. `by_name` is right for `skill_get` and
|
||||
`skill_edit`, where a shared skill is what the model is reaching for;
|
||||
`owned_by_name` is the uniqueness question. `documents.create_base` next door
|
||||
had always asked it correctly.
|
||||
|
||||
**Root must not run a file the service account can write.** The update helper's
|
||||
unit pointed `ExecStart` inside the checkout, which `install.sh` clones *as* the
|
||||
service user -- so anything able to write as that account could rewrite the
|
||||
script, create the request file it also owns, and be root. Worse, no compromise
|
||||
was needed: an update pulls new code as that user and root then runs whatever it
|
||||
fetched, so control of the branch was control of root. `install.sh` puts a
|
||||
root-owned copy at `/usr/local/lib/lembas/update.sh`. The cost -- changing
|
||||
`update.sh` needs the installer re-run -- is the right one, and `update.sh`
|
||||
detects both the stale copy and the old wiring. **The test that existed asserted
|
||||
the vulnerable `ExecStart` line verbatim**, and had pinned it in place.
|
||||
|
||||
**Nothing under `$PREFIX` may be sourced.** `update.sh` did `. "$PREFIX/.deploy-env"`.
|
||||
The file is root-owned, having been written with `sudo tee` -- but `$PREFIX` is
|
||||
the service account's own directory at mode 755, and write permission on a
|
||||
*directory* is all it takes to unlink a file and put another there. The two
|
||||
values it wants are parsed with patterns now. The test asserts nothing under
|
||||
`$PREFIX` is sourced rather than naming `.deploy-env`, because the next file
|
||||
read from there would have the same problem.
|
||||
|
||||
**A read-only *name* is not a read-only *command*.** `policy._UNSAFE` stops a
|
||||
line being composed out of two commands and says nothing about one command that
|
||||
composes another itself -- and the obvious read-only tools do: `find -exec` runs
|
||||
a program, `-fprintf` writes a file, `-delete` removes one, `rg --pre` runs a
|
||||
preprocessor per file. None needs a character `_UNSAFE` refuses. `find *` was on
|
||||
`SAFE_COMMANDS`, which is what a **subagent** is pinned to in every mode,
|
||||
unattended, with no card anybody could approve. `policy._ACTION` refuses those
|
||||
flags in `subject()` rather than the list being trimmed: a glob cannot say "and
|
||||
no dangerous flags", and "this one looks read-only" is exactly the reasoning
|
||||
that put `find` there.
|
||||
|
||||
**`0.0.0.0` is this machine.** `hosts._literal` answered from `is_loopback`, and
|
||||
the unspecified addresses are not loopback -- so it returned a *decided* False,
|
||||
which also short-circuited `resolves_here` and skipped the DNS half. Connecting
|
||||
to `0.0.0.0` or `::` goes to loopback on Linux, so an SSH profile pointed there
|
||||
reached this host's own sshd with the guard still reading "off".
|
||||
|
||||
**A folder is not a label.** `effective_system_prompt` walks up from the chat
|
||||
through its folder and that folder's parents, so a chat filed in somebody else's
|
||||
folder takes their system prompt. `_new_chat` resolved the folder, discarded it
|
||||
when it was not the caller's -- and then stored the raw `folder_id` anyway, so
|
||||
the ownership check governed which *seeds* were applied and not where the chat
|
||||
went.
|
||||
|
||||
**A file is not deleted by the row that named it.** `db.delete(chat)` cascades
|
||||
to messages and attachment *rows* and leaves every file on disk, with nothing
|
||||
that will ever look at them again -- `sweep_orphans` only considers uploads that
|
||||
were never attached. `chat_service.delete_chats` is the one way to delete a chat
|
||||
and unlinks first, while the rows still say which files they are. Underneath it,
|
||||
`files.claim` bound `message_id` and never `chat_id`, so anything picked on the
|
||||
*new-chat* screen kept an empty `chat_id` for life -- and six readers filter on
|
||||
that column, so those files were unnamed in the prompt, unopenable in the canvas
|
||||
and invisible to the one caller the cleanup had.
|
||||
|
||||
**Drive the JavaScript, and check the harness before believing it.** Three of
|
||||
the four bugs the testing pass found came from a Node DOM stub. The fourth
|
||||
lesson is about the instrument: a headless-Chromium harness that renders a page
|
||||
through `TestClient` and rewrites its asset URLs reported **all five tab panels
|
||||
visible at once** -- because the templates emit absolute `http://testserver/static/…`
|
||||
URLs via `url_for`, not `/static/…`, so the rewrite matched nothing and it was
|
||||
measuring an unstyled document. A dramatic finding that was entirely an
|
||||
artefact. Any such harness must fail loudly on an unrewritten asset URL.
|
||||
|
||||
## Releasing, and the changelog
|
||||
|
||||
**Every version bump gets a `CHANGELOG.md` entry, in the same commit.** Not
|
||||
|
||||
Reference in New Issue
Block a user