Plugins record: plugins.json is rewritten in place, and an unreadable record switches every plugin back on #382

New issue

Closed

opened 2026-10-01 10:46:17 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:46:17 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, by reading how the record is written and read and simulating a write that fails part way in a test.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The code is now at plugins/record.py:162-180 (write_record, the path.write_text(...) at :171), plugins/record.py:106-129 (read_record, the except at :120-121) and plugins/record.py:208-210 (disabled_names), moved out of plugins/installing.py by #248 and still handed out under the installing names; the read-modify-write callers in plugins/installing.py are now install_wheel (:809-810), remove (:860), set_disabled (:949-953) and backfill_metadata (:999). plugins/registry.py is where it was.

write_record() truncates plugins.json and writes it again in place, and read_record() turns any parse error into "nothing installed". A write that fails half way, for example on a full disk, leaves a truncated record behind; from then on no plugin counts as disabled and none counts as installed. Every read-modify-write of the record is also done without a lock.

Where: plugins/installing.py:270-288 (write_record: path.write_text(...) at :279); plugins/installing.py:214-237 (read_record: except (OSError, ValueError): return [] at :228-229); plugins/installing.py:316-318 (disabled_names); plugins/registry.py:94-102 (_disabled) and :189-192 (_load_third_party skips only those names); the read-modify-write callers install_wheel (:983-984), remove (:1034), set_disabled (:1123-1127), backfill_metadata (:1173).
What happens:
1. A test wrote a record with postulo-a disabled, then made the next write fail with ENOSPC after 40 bytes: set_disabled raised, the file was left as {"version": 1, "plugins": [{ and disabled_names() became set(), read_record() [], installed("postulo-a") None.
2. The truncated file has a new stamp, so registry.catch_up() rebuilds in every web worker and in the scheduler, and every plugin an administrator switched off loads again.
3. Disable and Remove now answer "is not installed" for every plugin, and the next install writes a record holding only the new plugin; the others stay on the volume and loaded but invisible. plugins sync at the next upgrade has nothing to restore.
4. Less likely: a process that reads between the truncation and the write sees the same empty record for that one lookup, and two record changes at once (two administrators, or the page and the plugins command) lose one of them.
Who can trigger it: nobody deliberately; a full or failing volume while an administrator installs, removes or switches a plugin. The effect fails open: a plugin switched off, possibly for handling people's credentials badly, runs again everywhere without anybody being told.
Why it matters: the record is the only list of what is installed and what is switched off, and it is lost silently by an ordinary disk-full error.
Fix: write to a temporary file in the same directory, fsync it, and os.replace() it over plugins.json, then move the stamp. Hold an exclusive lock file in plugins_dir() around every read-modify-write and around install, removal and rollback. When the record exists but cannot be parsed, fail closed: log it, refuse writes, and have the registry keep the list it had (or treat every recorded distribution as disabled) rather than "nothing is disabled".

Done when

A test in tests/test_plugin_install.py that makes the write of plugins.json fail part way and asserts that the previous record is still readable, with its disabled plugin still disabled.
A test that a record which exists but does not parse does not make registry._disabled() return an empty set.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, by reading how the record is written and read and simulating a write that fails part way in a test. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The code is now at `plugins/record.py:162-180` (`write_record`, the `path.write_text(...)` at :171), `plugins/record.py:106-129` (`read_record`, the `except` at :120-121) and `plugins/record.py:208-210` (`disabled_names`), moved out of `plugins/installing.py` by #248 and still handed out under the `installing` names; the read-modify-write callers in `plugins/installing.py` are now `install_wheel` (:809-810), `remove` (:860), `set_disabled` (:949-953) and `backfill_metadata` (:999). `plugins/registry.py` is where it was. `write_record()` truncates `plugins.json` and writes it again in place, and `read_record()` turns any parse error into "nothing installed". A write that fails half way, for example on a full disk, leaves a truncated record behind; from then on no plugin counts as disabled and none counts as installed. Every read-modify-write of the record is also done without a lock. - **Where:** `plugins/installing.py:270-288` (`write_record`: `path.write_text(...)` at :279); `plugins/installing.py:214-237` (`read_record`: `except (OSError, ValueError): return []` at :228-229); `plugins/installing.py:316-318` (`disabled_names`); `plugins/registry.py:94-102` (`_disabled`) and `:189-192` (`_load_third_party` skips only those names); the read-modify-write callers `install_wheel` (:983-984), `remove` (:1034), `set_disabled` (:1123-1127), `backfill_metadata` (:1173). - **What happens:** 1. A test wrote a record with `postulo-a` disabled, then made the next write fail with `ENOSPC` after 40 bytes: `set_disabled` raised, the file was left as `{"version": 1, "plugins": [{` and `disabled_names()` became `set()`, `read_record()` `[]`, `installed("postulo-a")` `None`. 2. The truncated file has a new stamp, so `registry.catch_up()` rebuilds in every web worker and in the scheduler, and every plugin an administrator switched off loads again. 3. *Disable* and *Remove* now answer "is not installed" for every plugin, and the next install writes a record holding only the new plugin; the others stay on the volume and loaded but invisible. `plugins sync` at the next upgrade has nothing to restore. 4. Less likely: a process that reads between the truncation and the write sees the same empty record for that one lookup, and two record changes at once (two administrators, or the page and the `plugins` command) lose one of them. - **Who can trigger it:** nobody deliberately; a full or failing volume while an administrator installs, removes or switches a plugin. The effect fails open: a plugin switched off, possibly for handling people's credentials badly, runs again everywhere without anybody being told. - **Why it matters:** the record is the only list of what is installed and what is switched off, and it is lost silently by an ordinary disk-full error. - **Fix:** write to a temporary file in the same directory, `fsync` it, and `os.replace()` it over `plugins.json`, then move the stamp. Hold an exclusive lock file in `plugins_dir()` around every read-modify-write and around install, removal and rollback. When the record exists but cannot be parsed, fail closed: log it, refuse writes, and have the registry keep the list it had (or treat every recorded distribution as disabled) rather than "nothing is disabled". ## Done when - A test in `tests/test_plugin_install.py` that makes the write of `plugins.json` fail part way and asserts that the previous record is still readable, with its disabled plugin still disabled. - A test that a record which exists but does not parse does not make `registry._disabled()` return an empty set.

tiagoagueda added this to the 0.5.0 milestone

2026-10-01 10:46:17 +00:00

tiagoagueda added the

security

bug

labels

2026-10-01 10:46:17 +00:00

tiagoagueda referenced this issue from a commit

2026-10-04 10:04:57 +00:00

Write the plugins record atomically and fail closed when it is unreadable

tiagoagueda closed this issue

2026-10-04 10:04:57 +00:00