From e775a58bbc69f5ea2f7ad883117d1acf8414ce43 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 12:33:50 +0000 Subject: [PATCH] refactor(runtime): `PermissionDeniedError` has ONE declaration again (#7270) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `security/resolve-execution-context.ts` re-declared `PermissionDeniedError` and `isPermissionDeniedError` character-for-character from `@objectstack/plugin-security`'s `errors.ts`, with a doc comment asking the next editor to keep them "structurally identical" and nothing enforcing it. Both fields of the ADR-0112 denial envelope are load-bearing — `statusCode` is what the dispatcher answers with, `code` is what a matcher keys on — so editing one copy's `403` left every test in the repo green while one dispatch path answered a denial with the wrong status. `@objectstack/plugin-security` throws these (23 call sites); the runtime only catches them. The plugin now owns the single declaration and the runtime module re-exports it. `@objectstack/plugin-security` was already a plain `dependencies` entry of `@objectstack/runtime`, so this adds no dependency, and tsup externalizes workspace dependencies — the bundle gained an `import "@objectstack/plugin-security"` and lost the duplicated class (ESM 428.21 KB -> 428.02 KB). The symbols stay exported from the runtime module rather than being deleted, because `http-dispatcher.ts` imports `isPermissionDeniedError` from that path. Nothing outside the package is affected: `security/index.ts` never re-exported either symbol, so neither was reachable from the public barrel. The matcher is unchanged and stays duck-typed (`name` / `code` / message-prefix, never `instanceof`), which is what makes the re-export safe: dual CJS/ESM output and bundling can still hand the two sides distinct class objects. The new `permission-denied-error-parity.test.ts` pins both halves — that the two import paths reach the same declaration (this assertion fails against the old copy), and that an instance built from a deliberately foreign class of the same shape is still matched. No behaviour change: `name`, `code: 'PERMISSION_DENIED'` and `statusCode: 403` are byte-identical to what the runtime copy produced. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015TbH9juzW7PvJzbsdpUnEp --- ...mission-denied-error-single-declaration.md | 53 ++++++++ .../permission-denied-error-parity.test.ts | 114 ++++++++++++++++++ .../src/security/resolve-execution-context.ts | 39 ++---- 3 files changed, 180 insertions(+), 26 deletions(-) create mode 100644 .changeset/permission-denied-error-single-declaration.md create mode 100644 packages/runtime/src/security/permission-denied-error-parity.test.ts diff --git a/.changeset/permission-denied-error-single-declaration.md b/.changeset/permission-denied-error-single-declaration.md new file mode 100644 index 0000000000..7dce99d023 --- /dev/null +++ b/.changeset/permission-denied-error-single-declaration.md @@ -0,0 +1,53 @@ +--- +"@objectstack/runtime": patch +--- + +refactor(runtime): `PermissionDeniedError` has ONE declaration again (#7270) + +`security/resolve-execution-context.ts` re-declared `PermissionDeniedError` and +`isPermissionDeniedError` character-for-character from +`@objectstack/plugin-security`'s `errors.ts`, with a doc comment asking the next +editor to keep them "structurally identical" and **nothing enforcing it**: + +```ts +// runtime/src/security/resolve-execution-context.ts ← the copy +export class PermissionDeniedError extends Error { + readonly code = 'PERMISSION_DENIED'; + readonly statusCode = 403; + … +``` + +Two hand-maintained declarations of an ADR-0112 denial envelope, where both +fields are load-bearing. `statusCode` is what the dispatcher answers with, and +`code` is what a matcher keys on — edit one copy's `403` and every test in the +repo still passes while one dispatch path starts answering a denial with the +wrong status. A comment is not a constraint. + +`@objectstack/plugin-security` is the package that *throws* these (23 call sites +across `security-plugin.ts`, `delegated-admin-gate.ts`, `predicate-guard.ts`, +`system-write-guard.ts`, `suggested-audience-bindings.ts`); the runtime only ever +*catches* them. So the plugin owns the declaration and the runtime module now +re-exports it. `@objectstack/plugin-security` was already a plain `dependencies` +entry of `@objectstack/runtime`, so this adds no dependency — and `tsup` +externalizes workspace dependencies, so the built bundle gained an +`import "@objectstack/plugin-security"` and lost the duplicated class (ESM +428.21 KB → 428.02 KB). + +The symbols stay exported from `security/resolve-execution-context.ts` rather +than being deleted outright, because `http-dispatcher.ts` imports +`isPermissionDeniedError` from that module path. Nothing outside the package is +affected either way: `runtime/src/security/index.ts` never re-exported either +symbol, so neither was reachable from `@objectstack/runtime`'s public barrel. + +The matcher itself is unchanged and stays **duck-typed** (`name` / `code` / +message-prefix, never `instanceof`), which is what makes the re-export safe: dual +CJS/ESM output and bundling can still hand the two sides distinct class objects, +and a denial crossing that boundary is recognized regardless. A new +`security/permission-denied-error-parity.test.ts` pins both halves — that the two +import paths reach the same declaration (the assertion that fails against the old +copy), and that an instance built from a *deliberately foreign* class of the same +shape is still matched, so the duck-typed property is held independently of +whether the two ever collapse to one class object. + +No behaviour change: `name`, `code: 'PERMISSION_DENIED'` and `statusCode: 403` +are byte-identical to what the runtime copy produced. diff --git a/packages/runtime/src/security/permission-denied-error-parity.test.ts b/packages/runtime/src/security/permission-denied-error-parity.test.ts new file mode 100644 index 0000000000..d50597ae6a --- /dev/null +++ b/packages/runtime/src/security/permission-denied-error-parity.test.ts @@ -0,0 +1,114 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7270] `PermissionDeniedError` / `isPermissionDeniedError` — ONE declaration. + * + * `security/resolve-execution-context.ts` used to re-declare both symbols + * character-for-character from `@objectstack/plugin-security`, with nothing + * enforcing the identity. Two hand-maintained copies of an ADR-0112 denial + * envelope (`code: 'PERMISSION_DENIED'`, `statusCode: 403`) are free to drift: + * edit one `statusCode` and every test still passes while the dispatcher starts + * answering a denial with the wrong HTTP status on one path only. + * + * The duplicate is gone — the runtime module re-exports the plugin's + * declaration. These tests pin BOTH halves of why that is safe: + * + * 1. the two import paths reach the SAME declaration (the assertion that would + * have failed while the copy existed), and + * 2. the matcher is duck-typed, NOT `instanceof`-based — so an instance built + * from a *distinct* class object (what dual CJS/ESM output or a bundler + * duplicating the module actually produces at runtime) is still recognized. + * + * (2) is what makes the re-export sound: it holds whether or not the two sides + * ever collapse to one class object in a given deployment's module graph. + */ + +import { describe, it, expect } from 'vitest'; + +import { + PermissionDeniedError as PluginPermissionDeniedError, + isPermissionDeniedError as pluginIsPermissionDeniedError, +} from '@objectstack/plugin-security'; + +import { + PermissionDeniedError as RuntimePermissionDeniedError, + isPermissionDeniedError as runtimeIsPermissionDeniedError, +} from './resolve-execution-context.js'; + +/** + * A stand-in for "the same class, loaded twice" — a second class object with the + * identical shape, exactly what a CJS/ESM dual load or a bundled second copy of + * `plugin-security` hands the other side of the boundary. Declared locally on + * purpose: it must NOT be either package's class, or it proves nothing. + */ +class ForeignPermissionDeniedError extends Error { + readonly code = 'PERMISSION_DENIED'; + readonly statusCode = 403; + readonly details?: Record; + constructor(message: string, details?: Record) { + super(message); + this.name = 'PermissionDeniedError'; + this.details = details; + } +} + +describe('PermissionDeniedError — single declaration across packages', () => { + it('runtime re-exports the plugin declaration rather than re-declaring it', () => { + expect(RuntimePermissionDeniedError).toBe(PluginPermissionDeniedError); + expect(runtimeIsPermissionDeniedError).toBe(pluginIsPermissionDeniedError); + }); + + it('carries the ADR-0112 denial envelope', () => { + const e = new PluginPermissionDeniedError('[Security] Access denied: nope', { reason: 'rls' }); + + expect(e).toBeInstanceOf(Error); + expect(e.name).toBe('PermissionDeniedError'); + expect(e.code).toBe('PERMISSION_DENIED'); + expect(e.statusCode).toBe(403); + expect(e.message).toBe('[Security] Access denied: nope'); + expect(e.details).toEqual({ reason: 'rls' }); + }); + + it('omits `details` when none is supplied', () => { + expect(new PluginPermissionDeniedError('denied').details).toBeUndefined(); + }); +}); + +describe('isPermissionDeniedError — cross-package recognition', () => { + const matchers: Array<[string, (e: unknown) => boolean]> = [ + ['@objectstack/plugin-security', pluginIsPermissionDeniedError], + ['@objectstack/runtime', runtimeIsPermissionDeniedError], + ]; + + for (const [owner, isPermissionDenied] of matchers) { + describe(`matcher from ${owner}`, () => { + it("matches an instance of the plugin's own class", () => { + expect(isPermissionDenied(new PluginPermissionDeniedError('denied'))).toBe(true); + }); + + it('matches an instance of a DISTINCT class object of the same shape', () => { + const foreign = new ForeignPermissionDeniedError('denied'); + + // The point of the assertion: not the same class, still recognized. + expect(foreign).not.toBeInstanceOf(PluginPermissionDeniedError); + expect(isPermissionDenied(foreign)).toBe(true); + }); + + it('matches on `name` alone', () => { + expect(isPermissionDenied({ name: 'PermissionDeniedError' })).toBe(true); + }); + + it('matches on `code` alone', () => { + expect(isPermissionDenied({ code: 'PERMISSION_DENIED' })).toBe(true); + }); + + it('rejects unrelated errors and non-objects', () => { + expect(isPermissionDenied(new Error('boom'))).toBe(false); + expect(isPermissionDenied({ name: 'NotFoundError', code: 'NOT_FOUND' })).toBe(false); + expect(isPermissionDenied(null)).toBe(false); + expect(isPermissionDenied(undefined)).toBe(false); + expect(isPermissionDenied('PermissionDeniedError')).toBe(false); + }); + }); + } +}); diff --git a/packages/runtime/src/security/resolve-execution-context.ts b/packages/runtime/src/security/resolve-execution-context.ts index 2d26966549..3506b002c7 100644 --- a/packages/runtime/src/security/resolve-execution-context.ts +++ b/packages/runtime/src/security/resolve-execution-context.ts @@ -234,31 +234,18 @@ export async function resolveExecutionContext(opts: ResolveOptions): Promise; - constructor(message: string, details?: Record) { - super(message); - this.name = 'PermissionDeniedError'; - this.details = details; - } -} - -export function isPermissionDeniedError(e: unknown): e is PermissionDeniedError { - if (!e || typeof e !== 'object') return false; - const anyE = e as any; - return ( - anyE.name === 'PermissionDeniedError' || - anyE.code === 'PERMISSION_DENIED' || - (typeof anyE.message === 'string' && anyE.message.startsWith('[Security] Access denied')) - ); -} +export { PermissionDeniedError, isPermissionDeniedError } from '@objectstack/plugin-security';