diff --git a/.changeset/security-plugin-start-logger-above-bailouts.md b/.changeset/security-plugin-start-logger-above-bailouts.md new file mode 100644 index 0000000000..6ebe38b9f3 --- /dev/null +++ b/.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`. diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 356accf4f3..3a2ed20f24 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -866,6 +866,16 @@ export class SecurityPlugin implements Plugin { async start(ctx: PluginContext): Promise { 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) => { diff --git a/packages/plugins/plugin-security/src/start-logger-binding.test.ts b/packages/plugins/plugin-security/src/start-logger-binding.test.ts new file mode 100644 index 0000000000..f24cdf9a4b --- /dev/null +++ b/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; warn: ReturnType; error: ReturnType }; + registerService: ReturnType; + getService: ReturnType; + registerMiddleware: ReturnType; +}; + +/** + * 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 { + 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'), + ); + }); +});