Skip to content

Commit 8e0bb68

Browse files
os-zhuangclaude
andauthored
fix(plugin-security): let a member revoke their OWN API key (#8053) (#8127)
The residual of #7727, one layer down. That fix opened the method gate and registered the ADR-0092 D2 column whitelist, but the object-CRUD layer was untouched: `member_default` granted only `allowRead` across the better-auth managed identity tables, so `update` on `sys_api_key` resolved for `admin_full_access` alone. A member could mint a personal key and then not revoke it — 403 PERMISSION_DENIED, row unchanged, key still authenticating — while the `revoke_api_key` row action rendered in their own My Keys grid. `member_default` now carries an explicit `sys_api_key` entry with `allowEdit`. The grant is bounded by two pre-existing mechanisms rather than by the permission-set boolean: the `sys_api_key_self` RLS carve-out decides which rows (cross-owner revocation still 403), and ADR-0092 D2's column whitelist decides which fields (`revoked` alone; `key` and `user_id` stripped even when smuggled alongside a legal `revoked`). `create`/`delete` stay closed at 405. The regression pin runs as the key's OWNER, not as an admin — the persona gap that let this survive #7727's own test suite. Verified by removing the grant and re-running: the four owner-path cases go red, the refusal cases stay green. Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 37b82ed commit 8e0bb68

5 files changed

Lines changed: 445 additions & 2 deletions

File tree

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
---
2+
"@objectstack/plugin-security": patch
3+
---
4+
5+
fix(plugin-security): a member can revoke their OWN API key — owner-scoped `update` on `sys_api_key` for `member_default` (#8053)
6+
7+
An ordinary member who minted a personal API key could not revoke it.
8+
`PATCH /api/v1/data/sys_api_key/{their own id} {"revoked": true}` answered **403
9+
`PERMISSION_DENIED`**, the row stayed `revoked: false`, and the key kept
10+
authenticating. The `revoke_api_key` / `restore_api_key` row actions rendered in
11+
that member's own **My Keys** grid the whole time — a dead affordance on the
12+
persona the surface is built for.
13+
14+
A personal API key acts as its owner ("treat it like a password", per the
15+
console's own mint screen), and the owner is the person who discovers it leaked.
16+
Their only remedy was to find an admin.
17+
18+
This is the residual of #7727, one layer down. That fix was correct as far as it
19+
went: the method gate opened (`enable.apiMethods` gained `update`) and ADR-0092
20+
D2's column whitelist registered `revoked`. But the **object-CRUD** layer was
21+
untouched — the platform `member_default` set granted only `allowRead` across the
22+
better-auth-managed identity tables, so `update` on `sys_api_key` resolved for
23+
`admin_full_access` and nobody else. `GET /api/v1/security/explain` said so
24+
outright, as that member: *"No resolved permission set grants update on
25+
sys_api_key"*. Because #7727's tests all drove the admin, the member half stayed
26+
hidden behind its fix.
27+
28+
`member_default` now carries an explicit `sys_api_key` entry with `allowEdit`.
29+
Two pre-existing mechanisms bound it, and the grant is deliberately not bounded
30+
by the permission-set boolean alone:
31+
32+
- **which rows** — the `sys_api_key_self` RLS carve-out
33+
(`user_id == current_user.id`), which already made the row owner-*visible*;
34+
there was simply no `allowEdit` to go with it. A member PATCHing another
35+
user's key is still refused **403**, row unchanged.
36+
- **which fields** — ADR-0092 D2's identity write guard, whose per-object update
37+
whitelist for this table lists `revoked` alone. `key` stays unwritable (a
38+
rotated hash would mint a credential nobody holds) and `user_id` stays
39+
unwritable (re-owning a key is privilege transfer) — both are stripped even
40+
when smuggled alongside a legal `revoked`.
41+
42+
**Unaffected, and pinned as such:** cross-owner revocation stays 403; a
43+
non-`revoked` column stays refused for the owner too; `create` / `delete` stay
44+
**405** at the method gate (minting remains `POST /api/v1/keys`, the only path
45+
that returns the raw secret once, and rows retire by revoking, not deleting);
46+
show-once semantics are intact. Every other better-auth-managed identity table
47+
stays write-denied — `sys_api_key` is the one exception, and it is one because
48+
that table is hand-rolled ObjectStack rather than better-auth-owned, with a
49+
registered whitelist already governing its single platform-owned column.
50+
51+
The regression pin runs as the key's **owner**, not as an admin — the persona
52+
gap that let this survive #7727's own test suite.

packages/plugins/plugin-security/src/member-default-explicit-allow.test.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,11 +114,35 @@ describe('[#5491] what the baseline still declares, it still enforces', () => {
114114
it('better-auth identity tables stay WRITE-DENIED (the door is better-auth, not CRUD)', () => {
115115
for (const object of BETTER_AUTH_MANAGED_OBJECTS) {
116116
expect(allows('insert', [MEMBER_DEFAULT], object), `${object} insert`).toBe(false);
117-
expect(allows('update', [MEMBER_DEFAULT], object), `${object} update`).toBe(false);
117+
// [#8053] `sys_api_key` is the ONE update exception: a member revokes
118+
// their own personal key. It is not a hole in the "door is better-auth"
119+
// rule — that table is hand-rolled ObjectStack (better-auth's `apiKey`
120+
// plugin is not loaded), and the write is narrowed to the owner's rows by
121+
// the `sys_api_key_self` RLS policy and to `revoked` by ADR-0092 D2's
122+
// column whitelist. Neither narrowing is expressible as a permission-set
123+
// boolean, which is why this axis has to be asserted per object here.
124+
expect(allows('update', [MEMBER_DEFAULT], object), `${object} update`).toBe(
125+
object === 'sys_api_key',
126+
);
118127
expect(allows('delete', [MEMBER_DEFAULT], object), `${object} delete`).toBe(false);
119128
}
120129
});
121130

131+
it('[#8053] the update exception is `sys_api_key` alone, and it does not leak onto the other axes', () => {
132+
// Stated positively and separately so the loop above cannot be "fixed" by
133+
// widening the condition: every other managed table must still refuse
134+
// update, and `sys_api_key` itself must still refuse insert and delete.
135+
const alsoUpdatable = BETTER_AUTH_MANAGED_OBJECTS.filter(
136+
(o) => o !== 'sys_api_key' && allows('update', [MEMBER_DEFAULT], o),
137+
);
138+
expect(alsoUpdatable, 'no other managed identity table may become updatable').toEqual([]);
139+
140+
expect(allows('update', [MEMBER_DEFAULT], 'sys_api_key')).toBe(true);
141+
expect(allows('insert', [MEMBER_DEFAULT], 'sys_api_key'), 'minting stays POST /keys').toBe(false);
142+
expect(allows('delete', [MEMBER_DEFAULT], 'sys_api_key'), 'rows retire by revoking').toBe(false);
143+
expect(allows('find', [MEMBER_DEFAULT], 'sys_api_key')).toBe(true);
144+
});
145+
122146
it('self-service preferences survive the wildcard removal as an EXPLICIT grant', () => {
123147
// `sys_user_preference` is not a better-auth table, so the managed-deny
124148
// block does not cover it, and its `sys_user_preference_self` RLS policy

packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,19 @@ describe('BETTER_AUTH_MANAGED_OBJECTS ↔ schemas (drift pin, #3325)', () => {
3838
});
3939
});
4040

41+
/**
42+
* [#8053] The single, deliberate exception to the blanket managed-object edit
43+
* deny: `member_default` may EDIT `sys_api_key`, so a member can revoke their
44+
* own personal key. Bounded elsewhere and not by the permission-set boolean —
45+
* the `sys_api_key_self` RLS carve-out decides which rows, ADR-0092 D2's column
46+
* whitelist (`revoked` alone) decides which fields.
47+
*
48+
* Encoded as an exact (set, object) pair rather than by loosening the loop, so
49+
* a second entry — or the same one on another set — still fails this pin. The
50+
* create/delete/read axes are NOT excepted and are still asserted below.
51+
*/
52+
const EDIT_EXCEPTIONS = new Set(['member_default::sys_api_key']);
53+
4154
describe('default permission sets carry the managed denies (static baseline)', () => {
4255
it('each write-granting target set denies create/edit/delete on every managed object', () => {
4356
for (const setName of MANAGED_DENY_TARGET_SETS) {
@@ -47,13 +60,34 @@ describe('default permission sets carry the managed denies (static baseline)', (
4760
const entry = set.objects[obj];
4861
expect(entry, `${setName} has entry for ${obj}`).toBeTruthy();
4962
expect(entry.allowCreate).toBe(false);
50-
expect(entry.allowEdit).toBe(false);
63+
expect(entry.allowEdit, `${setName}.${obj} allowEdit`).toBe(
64+
EDIT_EXCEPTIONS.has(`${setName}::${obj}`),
65+
);
5166
expect(entry.allowDelete).toBe(false);
5267
expect(entry.allowRead).toBe(true);
5368
}
5469
}
5570
});
5671

72+
it('the edit exception is exactly one (set, object) pair, and it is the API-key one', () => {
73+
// The exception list is itself pinned: a future widening has to edit THIS
74+
// assertion, which is the moment someone is asked whether the new pair
75+
// really rides an owner-scoping RLS policy and a column whitelist the way
76+
// `sys_api_key` does. Without this, `EDIT_EXCEPTIONS` could grow silently.
77+
expect([...EDIT_EXCEPTIONS]).toEqual(['member_default::sys_api_key']);
78+
79+
const member = setByName('member_default');
80+
expect(member.objects.sys_api_key.allowEdit).toBe(true);
81+
// The owner scoping the grant leans on must exist, or the edit bit is
82+
// table-wide on a credential table.
83+
const selfPolicy = (member.rowLevelSecurity ?? []).find(
84+
(p: any) => p.object === 'sys_api_key' && p.name === 'sys_api_key_self',
85+
);
86+
expect(selfPolicy, 'member_default must keep the sys_api_key_self RLS carve-out').toBeTruthy();
87+
expect(selfPolicy.using).toBe('user_id == current_user.id');
88+
expect(['all', 'update']).toContain(selfPolicy.operation);
89+
});
90+
5791
it('admin_full_access keeps its bare wildcard (zero per-object entries) — admin rescue path', () => {
5892
const admin = setByName('admin_full_access');
5993
expect(admin).toBeTruthy();

packages/plugins/plugin-security/src/objects/default-permission-sets.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,52 @@ const baseDefaultPermissionSets: PermissionSet[] = [
354354
// but the grant itself: it is what keeps `/auth/me`, the org switcher and
355355
// the Account app working for a member with no application profile.
356356
...denyWritesOnManagedObjects(),
357+
// [#8053] The ONE override of the block just above: a member may revoke
358+
// their OWN API key. #7727 opened the method gate and registered the
359+
// ADR-0092 D2 column whitelist, but left this object-CRUD layer
360+
// untouched, so `update` on `sys_api_key` resolved for
361+
// `admin_full_access` alone — the `revoke_api_key` row action rendered in
362+
// the member's own My Keys grid and answered 403 for them. A personal key
363+
// "acts as you — treat it like a password", and the owner is the person
364+
// who discovers it leaked; their only remedy was to find an admin.
365+
//
366+
// This is a restoration of declared-≠-enforced intent, not a new grant:
367+
// the row action, the checklist persona and the `sys_api_key_self` policy
368+
// below (which already makes the row owner-VISIBLE) all say the owner
369+
// path was intended — there was simply no `allowEdit` to go with it.
370+
//
371+
// The opening is bounded by TWO pre-existing mechanisms, and it is
372+
// deliberately not bounded by this line alone:
373+
// - WHICH ROWS: the `sys_api_key_self` RLS carve-out below
374+
// (`user_id == current_user.id`, `operation: 'all'`), enforced on
375+
// by-id writes through the security middleware's pre-image check. A
376+
// member PATCHing another user's key is still refused.
377+
// - WHICH FIELDS: ADR-0092 D2's identity write guard, whose per-object
378+
// update whitelist for this table lists `revoked` alone
379+
// (plugin-auth `MANAGED_EXTENSION_EDITABLE_FIELDS`). `key` stays
380+
// unwritable (a rotated hash mints a credential nobody holds) and
381+
// `user_id` stays unwritable (re-owning a key is privilege transfer).
382+
//
383+
// `allowCreate` / `allowDelete` stay false and are NOT an oversight:
384+
// minting is `POST /api/v1/keys` (the only path that returns the raw
385+
// secret once) and rows are retired by revoking, not deleting, so history
386+
// survives. `allowDelete` also stays false because this set is bound to
387+
// the `everyone` anchor and must remain anchor-safe (ADR-0090 D5).
388+
//
389+
// ⚠️ Not a pattern to copy across the managed list. Every OTHER
390+
// better-auth table here stays write-denied because its mutations must
391+
// flow through an auth endpoint; this one is a hand-rolled ObjectStack
392+
// table (`packages/core/src/security/api-key.ts` mints and verifies it,
393+
// better-auth's `apiKey` plugin is not loaded) whose one platform-owned
394+
// column already has a registered whitelist. Widening `update` on
395+
// `sys_api_key` beyond owner-scoped-plus-one-column would close #8053 and
396+
// open a worse defect on a table whose rows act as the user.
397+
//
398+
// Being an EXPLICIT entry is what makes it survive `kernel:ready`:
399+
// `applyManagedWriteDenies` injects its deny only for managed objects a
400+
// target set does not already name (`name in objects` → skip), so this
401+
// line is preserved rather than overwritten.
402+
sys_api_key: { allowRead: true, allowCreate: false, allowEdit: true, allowDelete: false },
357403
// Self-service preferences. NOT a better-auth table, so it is not covered
358404
// by the block above, and its `sys_user_preference_self` RLS policy below
359405
// (`operation: 'all'`) declares exactly this intent: a member reads and

0 commit comments

Comments
 (0)