From 45ff840cb96ea8b485044ad0b13c0ba68498e931 Mon Sep 17 00:00:00 2001 From: NinjaLikesCheez Date: Sun, 18 Oct 2026 12:39:02 +0200 Subject: [PATCH] Validate the members file fully before an access change writes it apply() only ran the per-entry schema on the current file, wrote the new file, and then built the view, which runs the cross-file checks (an id in two entries, an admin with no entry). A hand edit that failed those checks left the file written but the view and audit record unchanged, with a generic error for the admin. Run the startup cross-checks on the current file and build the new view before writing, so the change is refused and nothing is written. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 ++++ docs/identity-and-access.md | 4 ++-- src/services/access-store.test.ts | 27 +++++++++++++++++++++++++++ src/services/access-store.ts | 15 +++++++++++---- 4 files changed, 44 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f5410c..6af0d1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ Until 1.0.0, releases stay on 0.x: `Added` bumps minor, `Fixed`/`Changed` (and t ## [Unreleased] +### Fixed + +- `/admin level set` and capability changes no longer write `members.yaml` when the file was hand-edited into a state that fails the startup cross-checks (an id in two entries, or an admin with no entry). Before, the file was saved but the in-memory view and the `access.changed` audit record were not, and the admin got a generic error. Now the change is refused with "The members file is invalid", and nothing is written. + ## [0.17.0] - 2026-10-15 ### Added diff --git a/docs/identity-and-access.md b/docs/identity-and-access.md index 8848fb1..784b5d3 100644 --- a/docs/identity-and-access.md +++ b/docs/identity-and-access.md @@ -81,9 +81,9 @@ Both files are loaded at startup into an in-memory view that is replaced as a wh **Changes go through the access store** (`core/ports/access-store.ts`, implemented by `services/access-store.ts`). One change at a time it: -1. re-reads `members.yaml`, so edits made by hand aren't lost, and refuses if it has become invalid, +1. re-reads `members.yaml`, so edits made by hand aren't lost, and refuses if it has become invalid (the same checks as at startup, including an id in two entries or an admin with no entry), 2. edits the YAML document, so comments, order and quoting survive (IDs are written as quoted strings), -3. re-validates the result and writes it to a temp file next to the original, then flushes it to disk, +3. re-validates the result, builds the new view, and writes it to a temp file next to the original, then flushes it to disk, 4. checks the file wasn't edited in the meantime, keeps the old one as `members.yaml.bak`, and renames the temp file over the original (atomic), 5. only then swaps the in-memory view and records the change. diff --git a/src/services/access-store.test.ts b/src/services/access-store.test.ts index 729f1c8..1bf2da6 100644 --- a/src/services/access-store.test.ts +++ b/src/services/access-store.test.ts @@ -401,6 +401,33 @@ describe("safe writes", () => { expect(existsSync(`${membersFile}.bak`)).toBe(false); }); + it.each([ + [ + "an id in two entries", + `members:\n - ids: ["discord:${IDS.member}"]\n tier: member\n - ids: ["discord:${IDS.member}"]\n tier: friend\n${ADMIN_ENTRY}`, + ], + ["an admin with no entry", `members:\n - ids: ["discord:${IDS.member}"]\n tier: member\n`], + ])("refuses a file that fails the cross-checks (%s), and changes nothing", async (_, broken) => { + const store = open(); + writeFileSync(membersFile, broken); + const before = store.view; + for (const change of [ + { kind: "set-tier", id: NEW_ID, tier: "friend" }, + { kind: "set-tier", id: IDS.member, tier: "friend" }, + { kind: "set-capabilities", id: IDS.member, capabilities: ["back-door"] }, + ] as const) { + const error = await store.apply(change, by).catch((e: unknown) => e as Error); + expect(error).toBeInstanceOf(AccessStoreError); + expect((error as Error).message).toMatch(/invalid.*\/admin reload/s); + expect((error as Error).message).not.toContain(IDS.member); + } + expect(read()).toBe(broken); + expect(store.view).toBe(before); + expect(existsSync(`${membersFile}.bak`)).toBe(false); + expect(entries("access.changed")).toHaveLength(0); + expect(entries("access.file_invalid").length).toBeGreaterThan(0); + }); + it("reports an unreadable file clearly, without its path", async () => { const store = open(); rmSync(membersFile); diff --git a/src/services/access-store.ts b/src/services/access-store.ts index 79d517e..fdade47 100644 --- a/src/services/access-store.ts +++ b/src/services/access-store.ts @@ -41,9 +41,10 @@ import { /** * The bot-managed members file. Every change goes through `apply`, which: * - * 1. re-reads the file (so hand edits aren't lost) and checks it is valid, + * 1. re-reads the file (so hand edits aren't lost) and checks it is valid, together + * with the admins (the same checks as at startup), * 2. edits the YAML document, so comments and order survive, - * 3. re-validates the result, writes it to a temp file and fsyncs it, + * 3. re-validates the result, builds the new view, writes it to a temp file and fsyncs it, * 4. checks the file wasn't edited meanwhile, keeps the old one as `.bak`, * 5. renames the temp file over the original (atomic on a normal filesystem), * 6. only then swaps the in-memory view and writes the audit record. @@ -233,12 +234,15 @@ export class FileAccessStore implements AccessStore { const next = doc.toString({ flowCollectionPadding: false }); // Our own edit must produce a valid file. If it doesn't, that is a bug, not user error. const nextEntries = parseMembers(next, file); + // Build the new view before writing, so a failing cross-file check can't leave the file + // changed while the view and the audit record are not. + const nextView = buildAccessConfig(this.#admins, nextEntries, this.#paths); this.#writeFile(file, raw, next); const updated = nextEntries.find((entry) => entry.ids.includes(ref)); const after = toRecord(updated as NonNullable); - this.#view = buildAccessConfig(this.#admins, nextEntries, this.#paths); + this.#view = nextView; this.#audit(change, by, before, after); return { before, after }; } @@ -262,7 +266,10 @@ export class FileAccessStore implements AccessStore { #parseCurrent(raw: string, file: string) { try { - return parseMembers(raw, file); + const entries = parseMembers(raw, file); + // The same cross-file checks as startup: an id in two entries, an admin with no entry. + buildAccessConfig(this.#admins, entries, this.#paths); + return entries; } catch (error) { if (!(error instanceof AccessConfigError)) throw error; this.#logger.warn({ event: "access.file_invalid" }, error.message);