Skip to content

Commit 9e9f03a

Browse files
os-warrenclaude
andauthored
fix(plugin-auth): refuse a self-registration grant on a malformed permission-set row, and remove the 14 dead find() envelope limbs behind it (#15827)
* wip(plugin-auth): remove the dead {records}/{data} envelope limbs, refuse on a malformed permission-set row 14 union-normalizer blocks (13 `records`-limb, 1 `data`-limb), all reading the same concrete engine: the ObjectQL instance registered as the `objectql`/`data` kernel service. Driven rather than inferred — every read answers a bare array. The `settleSelfRegistrationGrant` block is the opposite defect (#15092's DROP shape) and is fixed in the opposite direction: its trailing filter no longer silently drops a malformed row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y * wip(plugin-auth): pin the concrete find() shape per block, and the DROP fix's direction Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y * wip(plugin-auth): changeset, ablation record, unused import Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y * fix(plugin-auth): state the Case D refusal cost, pin the org-stamped wrong grant, correct two counts - changeset: name the family that now gets a refusal where it previously got a grant (an active, correctly-named row with a missing/blank id — including a malformed GLOBAL row beside a well-formed org-scoped one), what it now sees, and that repairing or deleting the row restores the grant with no code change. - the wrong-GRANT pin now RESOLVES an organization via `getTenancy`, so it drives the shape that actually bit: `ps_global` stamped `organization_id: 'org_1'`. Asserted by column, not only by row count. - counts corrected: four guard-clause spellings (not five — the fifth match was `member-role-canonical`'s `raw.join(',')`, a different helper); and this package has 47 `.find(` sites in non-test source, a count that already includes `Array#find` (the earlier "~140" was unmeasured). Also "four local engine ports" -> "three", which is what that sentence names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 384a573 commit 9e9f03a

12 files changed

Lines changed: 613 additions & 77 deletions
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
"@objectstack/plugin-auth": patch
3+
---
4+
5+
A self-registration grant is refused, not silently redirected, when a permission-set row is malformed — and the fourteen dead `{ records }` / `{ data }` normalizer limbs behind that code are gone.
6+
7+
`plugin-auth` carried fourteen array-or-envelope normalizer blocks of the shape `Array.isArray(x) ? x : x.records ?? []` (thirteen on a `records` limb, one on a `data` limb, four of them written as a guard clause rather than a ternary). All fourteen read the same concrete engine — the `ObjectQL` instance the kernel registers as the `objectql` / `data` service — which answers a bare array on every path, populated or empty. The envelope limb was unreachable code that read as a contract, so the next author writing a defensive normalizer here believed an envelope was possible. The limbs are removed, and the three local engine ports that declared `Promise<unknown>` (`BootProbeEngine`, `DevAdminSeedProbeEngine`, `PhoneSmsTemplateEngine`) now declare the array they always returned.
8+
9+
The user-visible change is in `settleSelfRegistrationGrant`, which carried the opposite defect. Its candidate filter dropped any permission-set row whose `id` was missing or blank, silently, before choosing which row to grant:
10+
11+
- When the malformed row was the only one, the operator was told `no active sys_permission_set row named 'X' resolves` — false, since an active row named exactly that was present. That report is the only signal this path emits, and nothing retries it.
12+
- When the malformed row was the **organization-scoped** one and a global row also carried the declared name, dropping it let the `organization_id == null` arm match instead, and the self-registrant was granted the **global** permission set their organization never declared — with a success log and no other trace.
13+
14+
`active !== false` remains a selection predicate: a deactivated set still reports the ordinary "does not resolve". A malformed row is no longer a selection at all — the grant is refused and the report names the malformed row, so the ambiguity is surfaced instead of resolved by accident. A well-formed family grants exactly as before.
15+
16+
**Upgrade note — one family now gets a refusal where it previously got a grant.** If a deployment's `sys_permission_set` already contains a row that is active and carries the declared name but whose `id` is missing or blank, self-registration grants against that name now stop and report, including the case where the malformed row is one nobody was relying on: a malformed **global** row sitting alongside a well-formed **organization-scoped** row used to be dropped silently, letting the org row be granted, and is now refused. This is deliberate — the old behaviour could not tell that family apart from the one where the silent drop granted the *wrong* set — and it is fully reversible without a code change: repair or delete the malformed row and the grant proceeds exactly as before. The refusal is loud and names the row, so it is visible rather than something to discover later; nothing is written while it stands.

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

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4180,10 +4180,9 @@ export class AuthManager {
41804180
if (!engine || typeof (engine as any).find !== 'function') return false;
41814181
try {
41824182
const reader = withSystemReadContext(engine) as any;
4183-
const raw = await reader.find(SystemObjectName.USER, {
4183+
const rows: any[] = await reader.find(SystemObjectName.USER, {
41844184
limit: AuthManager.BOOTSTRAP_USER_PROBE_LIMIT,
41854185
});
4186-
const rows: any[] = Array.isArray(raw) ? raw : Array.isArray(raw?.records) ? raw.records : [];
41874186
if (rows.some(isHumanUserRow)) return false;
41884187
// A page that came back FULL of non-human rows cannot prove there is no
41894188
// human on the next page — fail closed rather than guess.
@@ -4256,14 +4255,13 @@ export class AuthManager {
42564255
const seenIds = new Set<unknown>();
42574256
for (let pageIndex = 0; pageIndex < AuthManager.PENDING_INVITATION_PROBE_MAX_PAGES; pageIndex++) {
42584257
const offset = pageIndex * page;
4259-
const raw = await reader.find('sys_invitation', {
4258+
const rows: any[] = await reader.find('sys_invitation', {
42604259
where: { status: 'pending', email: target },
42614260
limit: page,
42624261
// Omitted on the first page so the ordinary single-page read sends
42634262
// exactly the option shape every driver already answers.
42644263
...(offset > 0 ? { offset } : {}),
42654264
});
4266-
const rows: any[] = Array.isArray(raw) ? raw : Array.isArray(raw?.records) ? raw.records : [];
42674265
const idsBefore = seenIds.size;
42684266
for (const row of rows) {
42694267
if (row?.id != null) seenIds.add(row.id);
@@ -4340,11 +4338,10 @@ export class AuthManager {
43404338
if (!target) return false;
43414339
try {
43424340
const reader = withSystemReadContext(engine) as any;
4343-
const raw = await reader.find(SystemObjectName.USER, {
4341+
const rows: any[] = await reader.find(SystemObjectName.USER, {
43444342
where: { email: target },
43454343
limit: AuthManager.EXISTING_USER_PROBE_LIMIT,
43464344
});
4347-
const rows: any[] = Array.isArray(raw) ? raw : Array.isArray(raw?.records) ? raw.records : [];
43484345
return rows.some(
43494346
(row) => typeof row?.email === 'string' && row.email.trim().toLowerCase() === target,
43504347
);
@@ -4382,8 +4379,7 @@ export class AuthManager {
43824379
if (!engine || typeof (engine as any).find !== 'function') return [];
43834380
try {
43844381
const reader = withSystemReadContext(engine) as any;
4385-
const raw = await reader.find('sys_permission_set', { where: { name: setName }, limit: 50 });
4386-
return Array.isArray(raw) ? raw : Array.isArray(raw?.records) ? raw.records : [];
4382+
return await reader.find('sys_permission_set', { where: { name: setName }, limit: 50 });
43874383
} catch {
43884384
return [];
43894385
}
@@ -4606,9 +4602,36 @@ export class AuthManager {
46064602
} catch {
46074603
organizationId = null;
46084604
}
4609-
const rows = (await this.findPermissionSetRows(staged.setName)).filter(
4610-
(r) => r?.active !== false && typeof r?.id === 'string' && r.id,
4605+
// ACTIVE is a selection predicate — a deactivated set legitimately does
4606+
// not resolve, so it stays a filter. A missing or blank `id` is NOT a
4607+
// selection: it is a MALFORMED row, and dropping it silently is the
4608+
// opposite defect from a dead limb, wrong in two ways that both look
4609+
// normal from outside. (1) The family narrows to nothing and the report
4610+
// below blames "no active row named X" while an active row named X is
4611+
// sitting right there — and that report is the only signal, because
4612+
// nothing retries this. (2) Worse, when the malformed row is the
4613+
// ORG-SCOPED one, the `organization_id == null` arm below then resolves
4614+
// the GLOBAL set and grants a permission set the organization never
4615+
// declared, and the `rows.length === 1` arm can fire on a family that
4616+
// was never singular — both computed AFTER the silent removal, so the
4617+
// "unambiguous single row" this code believes it selected is not that.
4618+
// So a malformed candidate REFUSES the grant and names itself, which is
4619+
// the same gap-not-empty-answer direction the rest of this method takes.
4620+
const candidates = (await this.findPermissionSetRows(staged.setName)).filter(
4621+
(r) => r?.active !== false,
46114622
);
4623+
const malformed = candidates.filter((r) => !(typeof r?.id === 'string' && r.id));
4624+
if (malformed.length > 0) {
4625+
this.reportUngrantedSelfRegistrant(
4626+
userId,
4627+
staged.setName,
4628+
`${malformed.length} active sys_permission_set row(s) named '${staged.setName}' carry no usable id, ` +
4629+
'so which row this grant would resolve to cannot be decided — refusing rather than ' +
4630+
'silently dropping them and granting whichever row is left',
4631+
);
4632+
return;
4633+
}
4634+
const rows = candidates;
46124635
const row =
46134636
(organizationId ? rows.find((r) => r?.organization_id === organizationId) : undefined) ??
46144637
rows.find((r) => r?.organization_id == null) ??
@@ -4623,15 +4646,10 @@ export class AuthManager {
46234646
);
46244647
return;
46254648
}
4626-
const existingRaw = await sys.find('sys_user_permission_set', {
4649+
const existing: any[] = await sys.find('sys_user_permission_set', {
46274650
where: { user_id: userId, permission_set_id: row.id },
46284651
limit: 1,
46294652
});
4630-
const existing: any[] = Array.isArray(existingRaw)
4631-
? existingRaw
4632-
: Array.isArray(existingRaw?.records)
4633-
? existingRaw.records
4634-
: [];
46354653
if (existing.length > 0) return;
46364654
const id = `ups_${Date.now().toString(36)}${Math.random().toString(36).slice(2, 10)}`;
46374655
await sys.insert('sys_user_permission_set', {

packages/plugins/plugin-auth/src/backfill-account-issuer.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,7 @@ export function oauthIssuerFor(providerId: string): string {
118118

119119
async function tryFind(ql: any, object: string, where: any, limit: number): Promise<any[]> {
120120
try {
121-
const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX });
122-
return Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : [];
121+
return await ql.find(object, { where, limit }, { context: SYSTEM_CTX });
123122
} catch {
124123
return [];
125124
}

packages/plugins/plugin-auth/src/boot-sign-in-reachability.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,11 @@ export const NO_SIGN_IN_ACCOUNT_AT_BOOT = 'no_sign_in_account_at_boot';
151151
* this, so the two probes cannot drift apart on what they require of a store.
152152
*/
153153
export interface BootProbeEngine {
154-
find(object: string, query: Record<string, unknown>, options?: unknown): Promise<unknown>;
154+
find(
155+
object: string,
156+
query: Record<string, unknown>,
157+
options?: unknown,
158+
): Promise<Array<Record<string, unknown>>>;
155159
}
156160

157161
/**
@@ -179,12 +183,6 @@ export interface SignInReachabilityFacts {
179183

180184
const SYSTEM = { context: { isSystem: true } };
181185

182-
const asRows = (raw: unknown): Record<string, unknown>[] => {
183-
if (Array.isArray(raw)) return raw as Record<string, unknown>[];
184-
const records = (raw as { records?: unknown } | null | undefined)?.records;
185-
return Array.isArray(records) ? (records as Record<string, unknown>[]) : [];
186-
};
187-
188186
const usable = (engine: BootProbeEngine | undefined): engine is BootProbeEngine =>
189187
!!engine && typeof engine.find === 'function';
190188

@@ -203,8 +201,10 @@ export async function probeHumanUsersPresence(
203201
): Promise<BootStorePresence> {
204202
if (!usable(engine)) return 'unknown';
205203
try {
206-
const page = asRows(
207-
await engine.find(SystemObjectName.USER, { limit: HUMAN_POPULATION_PROBE_LIMIT }, SYSTEM),
204+
const page = await engine.find(
205+
SystemObjectName.USER,
206+
{ limit: HUMAN_POPULATION_PROBE_LIMIT },
207+
SYSTEM,
208208
);
209209
// A full page of non-humans cannot prove absence: it reads as populated.
210210
const humansExist = page.some(isHumanUserRow) || page.length >= HUMAN_POPULATION_PROBE_LIMIT;
@@ -225,7 +225,7 @@ export async function probeSignInAccountsPresence(
225225
): Promise<BootStorePresence> {
226226
if (!usable(engine)) return 'unknown';
227227
try {
228-
const rows = asRows(await engine.find(SystemObjectName.ACCOUNT, { limit: 1 }, SYSTEM));
228+
const rows = await engine.find(SystemObjectName.ACCOUNT, { limit: 1 }, SYSTEM);
229229
return rows.length > 0 ? 'present' : 'absent';
230230
} catch {
231231
return 'unknown';

packages/plugins/plugin-auth/src/dev-admin-seed-gate.ts

Lines changed: 17 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,11 @@ import { SystemObjectName } from '@objectstack/spec/system';
6363

6464
/** The bounded reads this probe performs — every data engine satisfies them. */
6565
export interface DevAdminSeedProbeEngine {
66-
find(object: string, query: Record<string, unknown>, options?: unknown): Promise<unknown>;
66+
find(
67+
object: string,
68+
query: Record<string, unknown>,
69+
options?: unknown,
70+
): Promise<Array<Record<string, unknown>>>;
6771
}
6872

6973
/**
@@ -94,12 +98,6 @@ const SYSTEM_READ = { context: { isSystem: true } } as const;
9498
/** better-auth's local password provider — the one the dev seed provisions. */
9599
export const CREDENTIAL_PROVIDER_ID = 'credential';
96100

97-
function asRows(raw: unknown): Record<string, unknown>[] {
98-
if (Array.isArray(raw)) return raw as Record<string, unknown>[];
99-
const records = (raw as { records?: unknown } | null | undefined)?.records;
100-
return Array.isArray(records) ? (records as Record<string, unknown>[]) : [];
101-
}
102-
103101
/**
104102
* Decide whether the dev-admin seed should provision on this boot.
105103
*
@@ -124,34 +122,28 @@ export async function decideDevAdminSeedGate(
124122
const spellings = [...new Set([seedEmail, seedEmail.trim().toLowerCase()])];
125123
const seedUserIds = new Set<unknown>();
126124
for (const spelling of spellings) {
127-
for (const row of asRows(
128-
await engine.find(
129-
SystemObjectName.USER,
130-
{ where: { email: spelling }, limit: 5 },
131-
SYSTEM_READ,
132-
),
125+
for (const row of await engine.find(
126+
SystemObjectName.USER,
127+
{ where: { email: spelling }, limit: 5 },
128+
SYSTEM_READ,
133129
)) {
134130
if (row?.id != null) seedUserIds.add(row.id);
135131
}
136132
}
137133
for (const userId of seedUserIds) {
138-
const accounts = asRows(
139-
await engine.find(
140-
SystemObjectName.ACCOUNT,
141-
{ where: { user_id: userId }, limit: 1 },
142-
SYSTEM_READ,
143-
),
134+
const accounts = await engine.find(
135+
SystemObjectName.ACCOUNT,
136+
{ where: { user_id: userId }, limit: 1 },
137+
SYSTEM_READ,
144138
);
145139
if (accounts.length > 0) return { act: false, reason: 'seed-address-claimed' };
146140
}
147141

148142
// (2) Does any local password login exist at all?
149-
const credentials = asRows(
150-
await engine.find(
151-
SystemObjectName.ACCOUNT,
152-
{ where: { provider_id: CREDENTIAL_PROVIDER_ID }, limit: 1 },
153-
SYSTEM_READ,
154-
),
143+
const credentials = await engine.find(
144+
SystemObjectName.ACCOUNT,
145+
{ where: { provider_id: CREDENTIAL_PROVIDER_ID }, limit: 1 },
146+
SYSTEM_READ,
155147
);
156148
if (credentials.length > 0) return { act: false, reason: 'local-login-exists' };
157149

packages/plugins/plugin-auth/src/ensure-default-organization.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,7 @@ const SYSTEM_CTX = { isSystem: true };
141141

142142
async function tryFind(ql: any, object: string, where: any, limit = 100): Promise<any[]> {
143143
try {
144-
const rows = await ql.find(object, { where, limit }, { context: SYSTEM_CTX });
145-
return Array.isArray(rows) ? rows : Array.isArray(rows?.records) ? rows.records : [];
144+
return await ql.find(object, { where, limit }, { context: SYSTEM_CTX });
146145
} catch {
147146
return [];
148147
}

0 commit comments

Comments
 (0)