From 2a8d6f2505364d9f5b433c103b088a4d84830bff Mon Sep 17 00:00:00 2001 From: Dave Date: Wed, 16 Sep 2026 11:18:13 +0200 Subject: [PATCH] fix(security): lock the users collection against privilege escalation The users collection kept PocketBase's defaults because the Plan 1a rules migration covered eight collections and missed this one: createRule "" -> anyone could sign up updateRule "id = @request.auth.id" -> anyone could edit their own record `role` is an ordinary field here, so both were escalations. Verified exploitable against the live instance: an anonymous request created an account with role "owner", and a client manager promoted themselves to owner by patching their own record. Either route yields read and write access to every client's board. Create, update and delete are now superuser-only; list and view are owner-or-self. Adds four integration tests so a regression fails loudly. Co-Authored-By: Claude Opus 5 --- .../1789601000_locked_user_rules.js | 53 +++++++++++++ scripts/setup-remote.ts | 29 +++++++ tests/integration/user-rules.test.ts | 78 +++++++++++++++++++ 3 files changed, 160 insertions(+) create mode 100644 pocketbase/pb_migrations/1789601000_locked_user_rules.js create mode 100644 tests/integration/user-rules.test.ts diff --git a/pocketbase/pb_migrations/1789601000_locked_user_rules.js b/pocketbase/pb_migrations/1789601000_locked_user_rules.js new file mode 100644 index 0000000..6013bcc --- /dev/null +++ b/pocketbase/pb_migrations/1789601000_locked_user_rules.js @@ -0,0 +1,53 @@ +/// +/** + * Close the users-collection privilege escalation. + * + * REFERENCE ONLY — see pb_migrations/README.md. What actually runs is + * scripts/setup-remote.ts. This file exists so the schema stays readable in + * one place. + * + * 1789600900_applied_api_rules.js applied rules to eight collections and + * missed this one, so `users` kept PocketBase's defaults: + * + * createRule "" -> anyone may sign up + * updateRule "id = @request.auth.id" -> anyone may edit their own record + * + * `role` is an ordinary field on this collection, so each default was a + * privilege escalation. Both were verified exploitable against the live + * instance: an anonymous POST created an account with role "owner", and a + * client manager promoted themselves to owner by patching their own record. + * A promoted account reads every client's board through the normal rules. + * + * Accounts are created and edited only by the owner, through server-side code + * running as a superuser, so all three write rules are superuser-only. Any + * future self-service profile editing must go through a route handler with an + * explicit field whitelist — the same reason /api/client/* exists. + */ +migrate( + (app) => { + const users = app.findCollectionByNameOrId("users"); + + const ownerOrSelf = + '@request.auth.id != "" && (@request.auth.role = "owner" || id = @request.auth.id)'; + + users.listRule = ownerOrSelf; + users.viewRule = ownerOrSelf; + users.createRule = null; + users.updateRule = null; + users.deleteRule = null; + + return app.save(users); + }, + (app) => { + // Deliberately does NOT restore PocketBase's defaults. Rolling back to + // public signup would reopen the escalation, and no rollback is worth + // that. Reset by hand in the admin UI if you truly mean to. + const users = app.findCollectionByNameOrId("users"); + users.listRule = "id = @request.auth.id"; + users.viewRule = "id = @request.auth.id"; + users.createRule = null; + users.updateRule = null; + users.deleteRule = null; + return app.save(users); + } +); diff --git a/scripts/setup-remote.ts b/scripts/setup-remote.ts index fa565ba..45777f4 100644 --- a/scripts/setup-remote.ts +++ b/scripts/setup-remote.ts @@ -93,6 +93,9 @@ const AUTHED = '@request.auth.id != ""'; const scopedRead = (pathToClient: string) => `${AUTHED} && (${OWNER} || ${pathToClient} = @request.auth.client)`; +/** For the users collection: the owner sees everyone, everyone else sees only themselves. */ +const OWNER_OR_SELF = `${AUTHED} && (${OWNER} || id = @request.auth.id)`; + async function applyRules( pb: PocketBase, name: string, @@ -492,6 +495,32 @@ async function main(): Promise { deleteRule: null, }); + /** + * The users collection. Do not leave this on PocketBase's defaults. + * + * Out of the box `users` has createRule "" — anyone may sign up — and + * updateRule "id = @request.auth.id" — anyone may edit their own record. + * Because `role` is an ordinary field here, each of those is a privilege + * escalation on its own: + * + * - an anonymous POST could create an account with role "owner" + * - a client manager could promote themselves by patching their own record + * + * Both were live and both were verified exploitable before this was added. + * + * Accounts are created and edited only by the owner, through server-side + * code running as a superuser, so all three write rules are superuser-only. + * Any future self-service profile editing must go through a route handler + * with an explicit field whitelist — the same reason /api/client/* exists. + */ + await applyRules(pb, "users", { + listRule: OWNER_OR_SELF, + viewRule: OWNER_OR_SELF, + createRule: null, + updateRule: null, + deleteRule: null, + }); + console.log("Done."); } diff --git a/tests/integration/user-rules.test.ts b/tests/integration/user-rules.test.ts new file mode 100644 index 0000000..90f8aa9 --- /dev/null +++ b/tests/integration/user-rules.test.ts @@ -0,0 +1,78 @@ +import { describe, it, expect, beforeAll } from "vitest"; +import PocketBase from "pocketbase"; + +import { DEMO, PB_URL, adminPb, requireServices, userPb } from "./helpers"; + +/** + * Regression guard for a live privilege escalation. + * + * The built-in `users` collection ships with createRule "" (anyone may sign + * up) and updateRule "id = @request.auth.id" (anyone may edit themselves). + * Because `role` is an ordinary field on that collection, both were + * exploitable: an anonymous request could create an owner account, and a + * client manager could promote themselves to owner and then read every other + * client's board. Both were verified working against the live instance before + * the rules were locked. + * + * If either test here fails, someone has reverted the users rules and every + * client's data is readable by anyone who can reach the PocketBase URL. + */ + +let admin: PocketBase; + +beforeAll(async () => { + await requireServices(); + admin = await adminPb(); +}); + +describe("the users collection cannot be used to escalate privileges", () => { + it("refuses anonymous account creation", async () => { + const anon = new PocketBase(PB_URL); + anon.autoCancellation(false); + + await expect( + anon.collection("users").create({ + email: `anon-${Date.now()}@example.com`, + password: "probe-password-123", + passwordConfirm: "probe-password-123", + name: "Should not exist", + role: "owner", + }) + ).rejects.toThrow(); + }); + + it("refuses a client manager promoting themselves to owner", async () => { + const pb = await userPb(DEMO.managerEmail, DEMO.managerPassword); + const selfId = pb.authStore.record!.id; + + await expect( + pb.collection("users").update(selfId, { role: "owner" }) + ).rejects.toThrow(); + + const after = await admin.collection("users").getOne(selfId); + expect(after.role).toBe("client_manager"); + }); + + it("refuses a client manager reassigning themselves to another client", async () => { + // The other half of the same hole: switching `client` would hand them a + // different customer's entire board through the normal read rules. + const pb = await userPb(DEMO.managerEmail, DEMO.managerPassword); + const selfId = pb.authStore.record!.id; + const before = await admin.collection("users").getOne(selfId); + + await expect( + pb.collection("users").update(selfId, { client: "" }) + ).rejects.toThrow(); + + const after = await admin.collection("users").getOne(selfId); + expect(after.client).toBe(before.client); + }); + + it("does not let a client manager list other accounts", async () => { + const pb = await userPb(DEMO.managerEmail, DEMO.managerPassword); + const visible = await pb.collection("users").getFullList(); + + expect(visible).toHaveLength(1); + expect(visible[0].id).toBe(pb.authStore.record!.id); + }); +});