Skip to content

Commit 73dc89b

Browse files
os-zhuangclaude
andauthored
fix(auth): canonicalise sys_member.role at the write, and converge existing rows (#8417)
* fix(auth): canonicalise sys_member.role at the write (#8317) better-auth reads sys_member.role with a raw split(',') -- no trim, no lower-case -- so a row stored as 'Owner' or ' owner' is an owner to the #5942 grade ladder and a plain member to the vendor. Its 'only an owner may remove an owner' branch therefore never fires and the request falls through to hasPermission({ member: ['delete'] }), which an org admin passes: an org admin could remove an owner. Maintainer ruling 2026-08-13, option A -- normalise at the write: beforeInsert/beforeUpdate hooks on sys_member plus a one-off convergent boot pass for existing rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 * test(auth): pin the #8317 inversion against better-auth's own extracted predicates Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 * fix(auth): match the kernel Logger's error() arity; add the changeset Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 * test(auth): drop import.meta from the vendor-source pin (TS1470 in this CJS-typed package) plugin-auth publishes CommonJS, so under module: NodeNext any import.meta is a TS1470 — which drifted the package's frozen TEST_DEBT ledger entry 111 -> 112. Fixed the type rather than the ledger: reuse the findUp-from-CWD idiom rate-limit-storage-isolation.test.ts already established here, and seed createRequire from the package root so the better-auth file read is the one THIS package is pinned to. Re-measure is back at 111. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2c0b2f3 commit 73dc89b

5 files changed

Lines changed: 1116 additions & 0 deletions

File tree

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
---
4+
5+
fix(plugin-auth): canonicalise `sys_member.role` at the write, so an org admin can no longer remove an owner (#8317)
6+
7+
**Security — authorization inversion.** A membership stored with a non-canonical
8+
role — `Owner`, `' owner'`, `OWNER` — was an **owner** to every ObjectStack-side
9+
check and a **plain member** to better-auth.
10+
11+
better-auth `1.7.0-rc.2` reads that column with a raw `role.split(",")`, with no
12+
`trim()` and no `toLowerCase()`, in three branches of
13+
`dist/plugins/organization/routes/crud-members.mjs`: `removeMember`'s "only an
14+
owner may remove an owner", `updateMemberRole`'s creator protection, and
15+
`organization/leave`'s last-owner count. ObjectStack's own readers all trim and
16+
lower-case (the #5942 grade ladder, `mapMembershipRole`). So on such a row the
17+
vendor never entered its owner branch at all and fell through to
18+
`hasPermission({ member: ['delete'] })`**which an org admin passes**. An org
19+
admin could remove, demote, or count out an owner that every ObjectStack check
20+
treated as an owner.
21+
22+
Not reachable through the ordinary invite/accept path (better-auth's own writes
23+
are canonical). Reachable through anything else that writes the column: an
24+
operator SQL fix-up, a data import, a SCIM group mapping, a script.
25+
26+
**The fix normalises at the write**, so the disagreement is unrepresentable
27+
rather than adjudicated per reader:
28+
29+
- ObjectQL `beforeInsert` / `beforeUpdate` hooks on `sys_member` canonicalise
30+
`role` on every write path, in every context (system and better-auth adapter
31+
writes included — those are the paths this exists for). They run at priority
32+
5, ahead of the ADR-0092 identity write guard and the ADR-0024 D5.2
33+
break-glass guard, so both judge the value's normal form.
34+
- A **one-off convergent pass runs at boot** and canonicalises rows that already
35+
exist. It is idempotent, safe to re-run, and logs a census of every distinct
36+
non-canonical spelling it found with row counts.
37+
38+
Canonicalisation is per token: a token that is a membership role (ADR-0108's
39+
closed vocabulary) is trimmed and lower-cased; any other token is preserved
40+
verbatim apart from trimming, because `mapMembershipRole` passes an unknown
41+
value through with its case and it becomes a position name a permission set may
42+
be bound to. A value carrying no known role at all is left completely untouched
43+
and only reported — it cannot produce the inversion.
44+
45+
No API, schema or configuration change: `sys_member.role`'s option list is
46+
unchanged, and canonicalisation never moves a membership's grade, so no
47+
membership gains or loses authority as a result of this fix.

packages/plugins/plugin-auth/src/auth-plugin.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ import {
4343
type SecondaryStorageLike,
4444
} from './identity-write-guard.js';
4545
import { registerLastAdminGuard } from './last-admin-guard.js';
46+
import { registerMemberRoleCanonicalization } from './member-role-canonical.js';
4647
import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields.js';
4748
import { MANAGED_EXTENSION_EDITABLE_FIELDS } from './managed-extension-fields.js';
4849
import { runSetInitialPassword } from './set-initial-password.js';
@@ -843,6 +844,26 @@ export class AuthPlugin implements Plugin {
843844
}
844845
});
845846

847+
// [#8317] The one-off half of the ruling: rows written BEFORE the
848+
// canonicalisation hook above — or written outside ObjectQL entirely by an
849+
// operator SQL fix-up, an import or a SCIM group remap — still carry a
850+
// non-canonical `sys_member.role`, and each one is a live authorization
851+
// inversion (owner to us, plain member to better-auth). The pass is
852+
// convergent, idempotent and reports a census of every distinct spelling it
853+
// found, so a database already canonical costs one query.
854+
ctx.hook('kernel:ready', async () => {
855+
try {
856+
const ql = ctx.getService<IDataEngine>('objectql');
857+
if (!ql) return;
858+
const { canonicalizeStoredMemberRoles } = await import('./member-role-canonical.js');
859+
await canonicalizeStoredMemberRoles(ql, { logger: ctx.logger });
860+
} catch (e) {
861+
ctx.logger.warn?.('[auth] sys_member.role canonicalisation pass failed', {
862+
error: (e as Error).message,
863+
});
864+
}
865+
});
866+
846867
// ADR-0081 D1 — single-org default-organization bootstrap. Every WALLED
847868
// posture (`group` and `isolated`) keeps its existing owner: the enterprise
848869
// organizations package, which runs the same idempotent helper with the
@@ -1019,6 +1040,19 @@ export class AuthPlugin implements Plugin {
10191040
if (object === SystemObjectName.USER) continue; // sys_user tiering above
10201041
registerManagedUpdateWhitelist(object, fields);
10211042
}
1043+
// [#8317] Canonicalise `sys_member.role` on every ObjectQL write, at
1044+
// priority 5 — AHEAD of both guards below. better-auth reads that
1045+
// column with a raw `split(',')` (no trim, no lower-case), so a stored
1046+
// `Owner` / `' owner'` is an owner to our grade ladder and a plain
1047+
// member to the vendor, and its "only an owner may remove an owner"
1048+
// branch never fires: an org admin could remove an owner. Normalising
1049+
// at the write makes that disagreement unrepresentable instead of
1050+
// adjudicated per-reader (maintainer ruling 2026-08-13, option A), and
1051+
// it runs first so both guards judge the value's normal form.
1052+
registerMemberRoleCanonicalization(engine, {
1053+
packageId: 'com.objectstack.plugin-auth.member-role-canonical',
1054+
logger: ctx.logger,
1055+
});
10221056
registerIdentityWriteGuard(engine, {
10231057
packageId: 'com.objectstack.plugin-auth.identity-write-guard',
10241058
logger: ctx.logger,

packages/plugins/plugin-auth/src/index.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@ export * from './identity-write-guard.js';
2828
// has to be able to register the invariant itself rather than ship an
2929
// environment that can ban or delete its last administrator.
3030
export * from './last-admin-guard.js';
31+
// [#8317] `sys_member.role` canonicalisation — the write-path hooks and the
32+
// one-off convergent pass. Exported for the same reason the two guards above
33+
// are, plus one of its own: a host that upgrades outside this plugin's boot
34+
// path still has to converge its stored rows, because every non-canonical one
35+
// is an authorization inversion (an owner to ObjectStack, a plain member to
36+
// better-auth's raw `split(',')`).
37+
export * from './member-role-canonical.js';
3138
export * from './sys-user-writable-fields.js';
3239
export * from './otp-send-guard.js';
3340
// ADR-0069 D2 / #4772 — the cross-node rate-limit counter store (kernel cache,

0 commit comments

Comments
 (0)