Skip to content

Commit e238c79

Browse files
os-steveclaude
andauthored
Repair the two plugin-auth admin-audit durability swallows — batch 6 of the #12981 worklist (#13592)
* fix(plugin-auth): report the refused admin-audit writes two `catch {}` sites swallowed Two tier-1 DARK durability swallows on plugin-auth's admin surface (#12981 batch 6): an administrative action landed, its audit row was refused, and the endpoint answered 200 with nothing recorded anywhere. Control flow is unchanged at both sites — an admin operation must never fail over its own audit — but the refusal is no longer silent. Both catches were doing two jobs and were only right about one. plugin-audit UNINSTALLED means no sys_audit_log object, so nothing ever claimed the action would be audited and silence is correct. A REFUSED write is the other thing entirely, and it wore the same catch. Each site now asks getSchema('sys_audit_log') — the registry that owns the answer — rather than reading the driver's error text, which would decide the same question by guessing. getSchema is declared optional on AdminUserDataEngine and IdentityImportEngine, so it is additive; where it is absent the site cannot measure the difference and therefore reports. What was hiding in the silence: sys_account is in plugin-audit's SKIP_OBJECTS, so the row refused in writeAdminAudit was the only record that a password was administratively reset; and the run-level import row is a shape plugin-audit's actionFor structurally cannot emit, so the per-row create rows kept the trail looking complete while who ran the import and under which policy was gone. Both sinks are re-exported from index.ts and declare no error, so the LEVEL stays warn and remains #13398's question; only the SILENCE is repaired here. Each seam is pinned, plus absence-asserting cases so a seam that warns unconditionally cannot pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs * test(plugin-auth): silence an unused engine-double parameter in the batch 6 pins Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs * chore: ratchet the engine-double ledger for the batch 6 pin (1 -> 2 pinned) Coverage grew, which is the direction the shrink-only ledger wants; the gate's own verdict line asked for --write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 6c3f9f5 commit e238c79

5 files changed

Lines changed: 415 additions & 21 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': minor
3+
---
4+
5+
Report the refused admin-audit writes two `catch { }` sites swallowed (#12981 batch 6)
6+
7+
The two tier-1 DARK durability swallows on `plugin-auth`'s admin surface: an
8+
administrative action landed, its audit row was refused, and the endpoint
9+
answered `200` with nothing written anywhere. Control flow is unchanged at both
10+
sites — an admin operation must never fail over its own audit — but the refusal
11+
is no longer silent.
12+
13+
Both `catch` blocks were doing two jobs and were only right about one of them:
14+
15+
- **plugin-audit UNINSTALLED** — there is no `sys_audit_log` object at all, so
16+
nothing ever claimed the action would be audited. Silence is correct, and
17+
reporting here would put a line on every admin action in every deployment that
18+
does not run plugin-audit.
19+
- **plugin-audit INSTALLED, the write REFUSED** — the action happened, the audit
20+
record did not, and nothing retries or reconstructs it.
21+
22+
Both spelled `catch { }`. Each site now asks `getSchema('sys_audit_log')` — the
23+
registry that owns the answer — instead of reading the driver's error text,
24+
which would decide the same question by guessing. `getSchema` is declared
25+
**optional** on `AdminUserDataEngine` and `IdentityImportEngine`, so it is
26+
additive and no host that type-checks today stops doing so; where it is absent
27+
the site cannot measure the difference and therefore reports, because an
28+
unmeasurable write must not be a silent one.
29+
30+
What was hiding in the silence:
31+
32+
- `admin-user-endpoints.ts :: writeAdminAudit``sys_account` is in
33+
plugin-audit's `SKIP_OBJECTS`, so for `/admin/set-user-password` its generic
34+
writer emits **zero** rows and the row refused here was the only record that a
35+
password was ever administratively reset.
36+
- `admin-import-users.ts` run-level row — `action: 'import'` with a null
37+
`record_id` is a shape plugin-audit's `actionFor` structurally cannot emit. The
38+
per-row `create` rows still land, which is what made this dangerous: the trail
39+
looked complete while who ran the import, under which password policy, and what
40+
it did in aggregate was gone.
41+
42+
Both sinks (`AdminUserEndpointDeps.logger`, `IdentityImportDeps.logger`) are
43+
`{ warn(msg: string): void }` and both are re-exported from the package
44+
`index.ts`. Neither declares `error`, so the LEVEL stays `warn` and remains
45+
#13398's question; only the SILENCE is repaired here. Each seam is pinned by a
46+
test that fails if it goes quiet again, plus absence-asserting cases so a seam
47+
that warns unconditionally cannot pass.

packages/plugins/plugin-auth/src/admin-import-users.ts

Lines changed: 63 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,19 @@ export interface IdentityImportEngine {
8989
find(objectName: string, query?: any): Promise<any[]>;
9090
update(objectName: string, data: any, options?: any): Promise<any>;
9191
insert(objectName: string, data: any, options?: any): Promise<any>;
92+
/**
93+
* [#12981] Optional registry probe — `ObjectQL.getSchema`, which answers
94+
* `undefined` for an object no package registered.
95+
*
96+
* Separates the two outcomes the run-level audit `catch` used to spell
97+
* identically: plugin-audit UNINSTALLED (no `sys_audit_log` object — nothing
98+
* was ever claimed, so silence is correct) from plugin-audit INSTALLED AND
99+
* THE WRITE REFUSED (the import ran, its only run-level record did not land,
100+
* and the endpoint still answers 200). Optional, therefore additive: a host
101+
* or mock without it keeps type-checking, and a site that cannot measure the
102+
* difference REPORTS rather than going quiet.
103+
*/
104+
getSchema?(objectName: string): unknown;
92105
}
93106

94107
export interface IdentityImportDeps {
@@ -510,23 +523,56 @@ export async function runAdminImportUsers(
510523
// `sys_audit_log` table, and an import must not fail over its own audit.
511524
// Both facts are pinned in
512525
// `packages/qa/dogfood/test/admin-identity-audit-trail.dogfood.test.ts`.
513-
try {
514-
await engine.insert('sys_audit_log', {
515-
action: 'import',
516-
user_id: actor.id,
517-
actor: actor.id,
518-
object_name: 'sys_user',
519-
metadata: JSON.stringify({
520-
event: 'user.import_run',
521-
mode, matchBy, passwordPolicy: policy,
522-
total: prepared.rows.length,
523-
created: summary.created, updated: summary.updated,
524-
skipped: summary.skipped, errors: summary.errors + preErrors,
525-
// How `auto` (and the fixed policies) split the batch across channels.
526-
delivery,
527-
}),
528-
}, { context: SYSTEM_CTX } as any);
529-
} catch { /* audit table may not exist — never fail the import */ }
526+
//
527+
// [#12981] "Best-effort" was doing two jobs and only one of them was
528+
// right. plugin-audit UNINSTALLED means no `sys_audit_log` object, so
529+
// nothing ever claimed the run would be audited — a declared skip, taken
530+
// silently by the probe below. A REFUSED write is the other thing
531+
// entirely, and it was wearing the same `catch`.
532+
const auditRegistered = !engine.getSchema || Boolean(engine.getSchema('sys_audit_log'));
533+
if (auditRegistered) {
534+
try {
535+
await engine.insert('sys_audit_log', {
536+
action: 'import',
537+
user_id: actor.id,
538+
actor: actor.id,
539+
object_name: 'sys_user',
540+
metadata: JSON.stringify({
541+
event: 'user.import_run',
542+
mode, matchBy, passwordPolicy: policy,
543+
total: prepared.rows.length,
544+
created: summary.created, updated: summary.updated,
545+
skipped: summary.skipped, errors: summary.errors + preErrors,
546+
// How `auto` (and the fixed policies) split the batch across channels.
547+
delivery,
548+
}),
549+
}, { context: SYSTEM_CTX } as any);
550+
} catch (e) {
551+
// [#12981] An import must not fail over its own audit — control flow is
552+
// unchanged and the run still answers 200 with its summary. It must not
553+
// be SILENT either. `sys_audit_log` is registered (checked above), so
554+
// this is a refused write, and the run-level row is the ONLY record of
555+
// it: plugin-audit's `actionFor` maps afterInsert/Update/Delete to
556+
// create/update/delete and nothing else, so `action: 'import'` with a
557+
// null `record_id` is a shape its writer structurally cannot emit. The
558+
// per-row `create` rows survive, which is what makes this dangerous —
559+
// the trail looks populated while WHO ran the import, under WHICH
560+
// policy, and WHAT the run did overall is simply gone.
561+
deps.logger?.warn(
562+
`[AuthPlugin] the run-level sys_audit_log row for this user import was NOT written — `
563+
+ `the import itself SUCCEEDED (created ${summary.created}, updated ${summary.updated}, `
564+
+ `skipped ${summary.skipped}) and the endpoint answers 200, so nothing looks wrong. `
565+
+ 'plugin-audit is installed (sys_audit_log is registered), so this is a REFUSED '
566+
+ "write, not an absent plugin. plugin-audit's per-row create rows still landed, so "
567+
+ 'the audit trail LOOKS complete while the only record of who ran this import, under '
568+
+ 'which password policy, and what it did in aggregate is absent. Nothing retries it '
569+
+ 'and no later boot reconstructs it. Remedy: restore write access to sys_audit_log '
570+
+ `(permissions, driver connectivity) before the next import. Cause: ${
571+
(e as Error)?.message ?? e
572+
}`,
573+
);
574+
}
575+
}
530576
}
531577

532578
const errors = summary.errors + preErrors;

packages/plugins/plugin-auth/src/admin-user-endpoints.ts

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,24 @@ export interface AdminUserDataEngine {
116116
* isn't wired, in which case the org bind simply no-ops.
117117
*/
118118
find?(object: string, query?: unknown, opts?: unknown): Promise<unknown>;
119+
/**
120+
* [#12981] Optional registry probe — `ObjectQL.getSchema`, which answers
121+
* `undefined` for an object no package registered.
122+
*
123+
* It is here to separate two outcomes the audit `catch` below used to spell
124+
* identically: plugin-audit UNINSTALLED (no `sys_audit_log` object, so
125+
* nothing was ever claimed and silence is correct) from plugin-audit
126+
* INSTALLED AND THE WRITE REFUSED (the admin action landed, its audit row did
127+
* not, and the endpoint still answers 200). Reading the driver's error text
128+
* would decide the same question by guessing; this asks the registry that
129+
* owns the answer.
130+
*
131+
* Optional because lean mocks and hosts that wire no ObjectQL engine do not
132+
* carry it — additive, so nothing that type-checks today stops doing so. When
133+
* it is absent the site cannot tell the two apart and therefore REPORTS: an
134+
* unmeasurable write must never be a silent one.
135+
*/
136+
getSchema?(objectName: string): unknown;
119137
}
120138

121139
/** The gated caller, passed by the route after its ADR-0068 check. */
@@ -376,6 +394,12 @@ async function writeAdminAudit(
376394
): Promise<void> {
377395
const engine = deps.getDataEngine();
378396
if (!engine) return;
397+
// plugin-audit is OPTIONAL, and with it uninstalled there is no
398+
// `sys_audit_log` object at all. That case is a DECLARED skip, not a
399+
// swallow: nothing ever claimed this action would be audited, so there is
400+
// nothing to report and the channel stays quiet — which is what keeps the
401+
// `warn` below meaningful instead of one more line nobody reads.
402+
if (engine.getSchema && !engine.getSchema('sys_audit_log')) return;
379403
try {
380404
await engine.insert(
381405
'sys_audit_log',
@@ -389,9 +413,31 @@ async function writeAdminAudit(
389413
},
390414
{ context: SYSTEM_CTX },
391415
);
392-
} catch {
393-
// plugin-audit may not be installed (no sys_audit_log table) — audit is
394-
// best-effort by design here; the operation itself must not fail.
416+
} catch (error) {
417+
// [#12981] The operation itself must NOT fail over its own audit — control
418+
// flow is unchanged and the endpoint still answers 200. But it must not be
419+
// SILENT either, and this site is the sharpest case in the family: the
420+
// header above records that `sys_account` is in plugin-audit's
421+
// `SKIP_OBJECTS`, so for `/admin/set-user-password` the generic writer
422+
// emits ZERO rows and the row refused here is the ONLY record that a
423+
// password was administratively reset. Nothing retries it and no later
424+
// boot reconstructs it — the reset simply has no trail, while the admin
425+
// who performed it reads `success: true`. `sys_audit_log` is registered
426+
// (checked above), so this is a refused write, not an absent plugin.
427+
deps.logger?.warn(
428+
`[AuthPlugin] the sys_audit_log row for this administrative '${entry.action}' on sys_user `
429+
+ `${entry.recordId} was NOT written — the operation itself SUCCEEDED and the endpoint `
430+
+ 'answers 200, so nothing looks wrong. plugin-audit is installed (sys_audit_log is '
431+
+ 'registered), so this is a REFUSED write, not an absent plugin. This row carries the '
432+
+ "admin's decisions (event, passwordGenerated, mustChangePassword, placeholderEmail, "
433+
+ 'membershipCreated), none of which is derivable from the stored row, and for '
434+
+ '/admin/set-user-password it is the only audit record that exists at all because '
435+
+ "sys_account is in plugin-audit's SKIP_OBJECTS. Nothing retries this write, so the "
436+
+ 'action stays permanently untrailed. Remedy: restore write access to sys_audit_log '
437+
+ `(permissions, driver connectivity), then treat this line as the audit record. Cause: ${
438+
(error as Error)?.message ?? error
439+
}`,
440+
);
395441
}
396442
}
397443

0 commit comments

Comments
 (0)