fix(plugin-security): bind the report sink above start()'s two bail-outs (#10706)

.changeset/security-plugin-start-logger-above-bailouts.md

@@ -0,0 +1,36 @@
+---
+"@objectstack/plugin-security": patch
+---
+
+`SecurityPlugin.start()` binds its report sink **above** the two bail-outs, so a
+degraded boot no longer leaves the plugin permanently unable to report (#10706).
+
+`private logger … = {}` is an empty object from construction, and
+`this.logger = ctx.logger` was its only assignment — sitting in the "capture
+handles" block, **below** the two `return`s that fire when `objectql`/`metadata`
+cannot be resolved, or when the engine carries no `registerMiddleware`. On
+either path the field stayed `{}` for the **lifetime of the instance**. Every
+report site is written `this.logger.warn?.(…)`, so an unbound sink is not a
+state any caller can notice: the reports simply do not happen. The assignment
+now runs immediately after the `Starting Security Plugin...` line, before either
+bail-out can be taken.
+
+Boot behaviour is otherwise unchanged, and that is pinned rather than asserted:
+both bail-outs still `return`, the middleware and the `security` service are
+still **not** registered on those paths, and both bail-outs still report through
+`ctx.logger` — which was always a real sink, so the bail-out itself was already
+loud. What was silent was the plugin's own field afterwards.
+
+Scope note: this is independent of the open design call on #10556 about what the
+default sink should be. Only the **placement** of the binding changes; the `= {}`
+default itself is untouched, and the fix is correct under every option there.
+
+Reachability, measured rather than assumed: every in-repo caller of the two
+public methods that report through the field (`checkAuthoredRowWrite`,
+`getReadFilter`) reaches them through the registered `security` service, and
+that service is registered *below* the bail-outs too — so on a bailed-out boot
+there is no live consumer. The defect was latent, not live. It is still a defect
+on its own terms: a sink that can never be bound after an early return is
+unrepresentable as a state the code can notice.
+
+New pin: `start-logger-binding.test.ts`.

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

@@ -866,6 +866,16 @@ export class SecurityPlugin implements Plugin {
  async start(ctx: PluginContext): Promise<void> {
  ctx.logger.info('Starting Security Plugin...');
 
+ // [#10706] Bind the report sink FIRST — above the two bail-outs below.
+ // Both of them `return` before the "capture handles" block, so binding the
+ // sink there left `this.logger` at its `= {}` construction default for the
+ // whole LIFETIME of the instance on a degraded boot: every report site is
+ // written `this.logger.warn?.(...)`, so an unbound sink is not a state any
+ // caller can notice — it silently reports nothing. Assigning here is
+ // correct under every option on #10556's open default-sink question, so it
+ // does not wait on that ruling; the `= {}` default itself is #10556's.
+ this.logger = ctx.logger;
+
  // Get required services
  let ql: IObjectQLEngine | undefined;
  let metadata: IMetadataService | undefined;
@@ -887,7 +897,6 @@ export class SecurityPlugin implements Plugin {
  // engine middleware AND the public getReadFilter service method.
  this.metadata = metadata;
  this.ql = ql;
- this.logger = ctx.logger;
  this.rlsCompiler.setLogger?.(ctx.logger);
  // [C2 / ADR-0095] Late-bound resolver for the optional `sharing` service.
  this.resolveKernelService = (name: string) => {

packages/plugins/plugin-security/src/start-logger-binding.test.ts

@@ -0,0 +1,168 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+//
+// [#10706] `start()` binds `this.logger` ABOVE its two bail-outs.
+//
+// The mechanic this pins: `private logger … = {}` is an empty object from
+// construction, and `this.logger = ctx.logger` is its ONLY assignment. It used
+// to sit in the "capture handles" block, BELOW the two `return`s that fire when
+// `objectql`/`metadata` are missing or when the engine carries no
+// `registerMiddleware`. On either bail-out the field therefore stayed `{}` for
+// the LIFETIME of the instance — and because every report site is written
+// `this.logger.warn?.(…)`, an unbound sink is indistinguishable from a
+// configured one at every call site: it silently reports nothing.
+//
+// ⛔ This suite is deliberately INDEPENDENT of #10556's open design call on what
+// the `= {}` default should be. It asserts only WHERE the real sink is bound,
+// which is correct under every option on that question. Nothing here reads or
+// asserts the default value itself.
+//
+// Four directions are pinned, and the middle two are the load-bearing ones:
+//
+// 1. after either bail-out, `this.logger` IS `ctx.logger` — and RECEIVES a
+// report. Identity alone is not enough, and "non-empty" would be no
+// assertion at all: `{}` and a real logger both satisfy
+// `typeof x === 'object'`.
+// 2. STILL BAILS. Both early returns still return, and the middleware is
+// still NOT registered. An implementation that "fixed" this by deleting a
+// bail-out would pass a logger-only suite while changing boot behaviour.
+// 3. the bail-out itself stays LOUD through `ctx.logger` — it already was,
+// and that must not regress.
+// 4. the normal boot path is unchanged — same sink, registration still
+// happens.
+
+import { describe, it, expect, vi } from 'vitest';
+// Relative specifier: resolves to THIS package's `src/security-plugin.ts`, never
+// to `dist/`. The only aliases in `vitest.config.ts` are anchored regexes for
+// `@objectstack/driver-sql` and `@objectstack/objectql`; neither can match a
+// relative path, so the subject here is the source in the checkout.
+import { SecurityPlugin } from './security-plugin.js';
+
+type Ctx = {
+ logger: { info: ReturnType<typeof vi.fn>; warn: ReturnType<typeof vi.fn>; error: ReturnType<typeof vi.fn> };
+ registerService: ReturnType<typeof vi.fn>;
+ getService: ReturnType<typeof vi.fn>;
+ registerMiddleware: ReturnType<typeof vi.fn>;
+};
+
+/**
+ * A boot context in one of three postures:
+ *
+ * - `'throws'` — `getService('objectql')` throws → the `:878` bail-out.
+ * - `'no-mw'` — objectql resolves but carries no `registerMiddleware`
+ * → the `:883` bail-out.
+ * - `'healthy'` — a normal boot that reaches registration.
+ */
+function makeCtx(posture: 'throws' | 'no-mw' | 'healthy'): Ctx {
+ const registerMiddleware = vi.fn();
+ const manifestService = { register: vi.fn() };
+ const ctx: any = {
+ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() },
+ registerService: vi.fn(),
+ registerMiddleware,
+ getService: vi.fn().mockImplementation((name: string) => {
+ if (name === 'manifest') return manifestService;
+ if (posture === 'throws') throw new Error('service not available');
+ if (name === 'objectql') return posture === 'no-mw' ? {} : { registerMiddleware };
+ if (name === 'metadata') return { list: vi.fn().mockResolvedValue([]) };
+ return undefined;
+ }),
+ };
+ return ctx as Ctx;
+}
+
+/** The private field, read exactly as the mechanic describes it. */
+const sinkOf = (plugin: SecurityPlugin) => (plugin as any).logger;
+
+/**
+ * Drive a REAL report through `this.logger` and return whether it landed.
+ *
+ * `getReadFilter` is a public instance method whose on-behalf-of refusal
+ * (`this.logger.error?.(…)`, the ADR-0095/#2852 fail-closed branch) depends on
+ * NONE of the handles captured below the bail-outs — no `this.ql`, no
+ * `this.metadata`. That makes it the one report site that is genuinely
+ * reachable on a bailed-out instance, which is what makes it the right probe
+ * for "the sink RECEIVES something".
+ *
+ * Pre-fix this call is silent: `{}.error` is `undefined` and `?.()` no-ops.
+ */
+async function probeSinkReceives(plugin: SecurityPlugin, ctx: Ctx): Promise<boolean> {
+ ctx.logger.error.mockClear();
+ const verdict = await plugin.getReadFilter('crm_opportunity', {
+ userId: 'u_agent',
+ onBehalfOf: { userId: 'u_delegator' },
+ });
+ // The refusal itself must still be fail-closed, independent of the sink.
+ expect(verdict).toBeTruthy();
+ return ctx.logger.error.mock.calls.length > 0;
+}
+
+describe('[#10706] start() binds the report sink above both bail-outs', () => {
+ describe.each([
+ ['getService throws — the ObjectQL/metadata bail-out', 'throws' as const],
+ ['engine without registerMiddleware — the no-middleware bail-out', 'no-mw' as const],
+ ])('%s', (_label, posture) => {
+ it('binds `this.logger` to the real sink, and that sink RECEIVES a report', async () => {
+ const plugin = new SecurityPlugin();
+ const ctx = makeCtx(posture);
+
+ // Before `start()` the field is still the construction default — this is
+ // the state the fix makes unreachable AFTER a start, and asserting it
+ // here is what makes the post-start assertion a change and not a tautology.
+ expect(sinkOf(plugin)).not.toBe(ctx.logger);
+
+ await plugin.init(ctx as any);
+ await plugin.start(ctx as any);
+
+ // Direction 1a — identity: the field IS the context's logger.
+ expect(sinkOf(plugin)).toBe(ctx.logger);
+
+ // Direction 1b — the sink RECEIVES. `typeof sink === 'object'` would be
+ // satisfied by `{}` too, so the only honest assertion is a delivered call.
+ await expect(probeSinkReceives(plugin, ctx)).resolves.toBe(true);
+ });
+
+ it('STILL BAILS — no middleware and no `security` service are registered', async () => {
+ const plugin = new SecurityPlugin();
+ const ctx = makeCtx(posture);
+
+ await plugin.init(ctx as any);
+ await plugin.start(ctx as any);
+
+ // The bail-out is the POINT of these paths. A "fix" that moved the sink
+ // by deleting a return would satisfy the logger assertions above and
+ // silently change boot behaviour; these two assertions are what refuse it.
+ expect(ctx.registerMiddleware).not.toHaveBeenCalled();
+ const registeredNames = ctx.registerService.mock.calls.map((c) => c[0]);
+ expect(registeredNames).not.toContain('security');
+ });
+
+ it('the bail-out stays LOUD — it still reports through `ctx.logger`', async () => {
+ const plugin = new SecurityPlugin();
+ const ctx = makeCtx(posture);
+
+ await plugin.init(ctx as any);
+ await plugin.start(ctx as any);
+
+ expect(ctx.logger.warn).toHaveBeenCalledWith(
+ expect.stringContaining('security middleware not registered'),
+ );
+ });
+ });
+
+ it('normal boot is unchanged — same sink, and registration still happens', async () => {
+ const plugin = new SecurityPlugin();
+ const ctx = makeCtx('healthy');
+
+ await plugin.init(ctx as any);
+ await plugin.start(ctx as any);
+
+ expect(sinkOf(plugin)).toBe(ctx.logger);
+ expect(ctx.registerMiddleware).toHaveBeenCalledWith(expect.any(Function));
+ const registeredNames = ctx.registerService.mock.calls.map((c) => c[0]);
+ expect(registeredNames).toContain('security');
+ // The healthy path must NOT take either bail-out.
+ expect(ctx.logger.warn).not.toHaveBeenCalledWith(
+ expect.stringContaining('security middleware not registered'),
+ );
+ });
+});