diff --git a/.changeset/permission-denied-user-copy.md b/.changeset/permission-denied-user-copy.md new file mode 100644 index 0000000000..8b62ae85dc --- /dev/null +++ b/.changeset/permission-denied-user-copy.md @@ -0,0 +1,50 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-security": minor +--- + +fix(plugin-security,spec): the `403 PERMISSION_DENIED` from the object CRUD gate stops handing a business user internal authorization vocabulary + +An operation the caller's permission sets do not grant is correctly refused with +`403 PERMISSION_DENIED`, and the transport was never the problem. What reached +the end user was: `Error.message` is the body's human-readable string on every +transport (`mapDataError`'s `body.error`, the dispatcher's `error.message`) and +Console renders it verbatim in a toast. So an operator in a fully localized app +read + +``` +[Security] Access denied: operation 'delete' on object 'app_child_object' +is not permitted for positions [org_member, everyone] +``` + +English-only; naming a table they have never seen; ending in `positions [...]`, +internal authorization vocabulary that reads as a contradiction to someone who +does hold rights on the record they clicked. It is not confined to obviously +unauthorized actions either — `cascadeDeleteRelations` re-authorises every +cascade CHILD independently, so an ordinary delete of a parent the app +deliberately granted can surface a 403 naming a child object the operator never +addressed. + +The error now carries two messages because it has two audiences: + +- `message` — the user's half, rendered in `ExecutionContext.locale` through the + shared operation-message catalog (`@objectstack/spec/system`, the mechanism + built for `DELETE_RESTRICTED`), overridable per deployment under + `errors.permission_denied`. It names no object, no operation and no position, + in any of the four shipped locales. +- `developerMessage` — the developer's half, the previous sentence byte for + byte. It is LOGGED at the throw site, not shipped to the client. + +That last point is where this deliberately diverges from its sibling. +`DELETE_RESTRICTED` ships its developer half over the wire because the same body +already carries the API names it mentions; the 403 body does not. REST's +`mapDataError` builds `{ error, code, object? }` for a permission denial and +never reads `error.details`, so the positions, the operation and (on a cascade) +the child object's API name reach a client through nothing but the message — +shipping a `developerMessage` there would have ADDED a disclosure rather than +removed one. `developerMessage` is therefore a sibling of `details`, never a +member of it, because `details` is the field the runtime dispatcher serialises. + +Enforcement is untouched: same 403, same `PERMISSION_DENIED`, same decision +logic, and the structured `details` payload (`operation`, `object`, `positions`, +`permissionSets`) is byte-identical to before. diff --git a/packages/plugins/plugin-security/package.json b/packages/plugins/plugin-security/package.json index 1ff10b679c..4f411e1da9 100644 --- a/packages/plugins/plugin-security/package.json +++ b/packages/plugins/plugin-security/package.json @@ -26,6 +26,7 @@ "devDependencies": { "@objectstack/metadata-core": "workspace:*", "@objectstack/plugin-sharing": "workspace:*", + "@objectstack/service-i18n": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/plugins/plugin-security/src/errors.ts b/packages/plugins/plugin-security/src/errors.ts index 827d72f28d..9a677a78cc 100644 --- a/packages/plugins/plugin-security/src/errors.ts +++ b/packages/plugins/plugin-security/src/errors.ts @@ -4,15 +4,40 @@ * Typed sentinel error thrown by `SecurityPlugin` when an operation is * denied. Caught by `@objectstack/runtime`'s HTTP dispatcher and translated * to HTTP 403. + * + * ## Two messages, two audiences (#7414) + * + * `message` is what an END USER reads: both transports ship it verbatim as the + * body's human-readable string (`mapDataError`'s `error`, the dispatcher's + * `error.message`) and Console renders it as-is in a toast. `developerMessage` + * is the operator's half — English, API names, the authorization vocabulary + * that explains WHY — and it is the throw site's job to route it somewhere a + * developer reads. + * + * ⛔ `developerMessage` is a sibling of `details`, deliberately NOT a member of + * it. `details` is SERIALISED to the client on the dispatcher transport + * (`http-dispatcher.ts`: `this.error(e.message, 403, { code, ...e.details })`, + * which `buildApiError` puts on the wire as `error.details`), so anything + * placed inside it reaches the browser. A developer sentence that names + * positions and permission sets must not travel that way — see the throw site + * in `security-plugin.ts` and the measurement recorded in + * `permission-denied-user-copy.test.ts`. */ export class PermissionDeniedError extends Error { readonly code = 'PERMISSION_DENIED'; readonly statusCode = 403; readonly details?: Record; - constructor(message: string, details?: Record) { + /** + * The operator-facing half of a refusal whose `message` has been localized + * for an end user. Optional: a denial that never localized its message has + * exactly one audience and carries none. + */ + readonly developerMessage?: string; + constructor(message: string, details?: Record, developerMessage?: string) { super(message); this.name = 'PermissionDeniedError'; this.details = details; + if (developerMessage !== undefined) this.developerMessage = developerMessage; } } diff --git a/packages/plugins/plugin-security/src/permission-denied-user-copy.test.ts b/packages/plugins/plugin-security/src/permission-denied-user-copy.test.ts new file mode 100644 index 0000000000..6218d97486 --- /dev/null +++ b/packages/plugins/plugin-security/src/permission-denied-user-copy.test.ts @@ -0,0 +1,380 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7414 — the CALL SITE of the object-CRUD `403 PERMISSION_DENIED` copy, driven + * through the REAL `SecurityPlugin` middleware. + * + * The refusal was never in doubt: the gate correctly declines the operation and + * both transports ship a real 403. What reached the END USER was the problem. + * `Error.message` is the body's human-readable string on every transport + * (`mapDataError`'s `body.error`, the dispatcher's `error.message`) and Console + * renders it verbatim in a toast, so an operator in a fully localized app read: + * + * `[Security] Access denied: operation 'delete' on object 'app_child_object' + * is not permitted for positions [org_member, everyone]` + * + * English-only, naming a table they have never seen — on a cascade delete, a + * CHILD object they never addressed, because `cascadeDeleteRelations` + * re-authorises every child independently — and ending in `positions [...]`, + * internal authorization vocabulary that reads as a contradiction to someone + * who does hold rights on the record they clicked. + * + * These cases assert the SPLIT: `message` is the user's half (their locale, no + * object, no operation, no positions) and `developerMessage` is the developer's + * half, byte for byte what the message used to be. Enforcement is untouched: + * `code`, `statusCode` and the whole structured `details` payload are pinned + * unchanged. + * + * The catalog half is pinned in + * `packages/spec/src/system/operation-message.test.ts`. + * + * ## Why a real `II18nService` and not a stub + * + * A hand-written `t` can agree with a producer that disagrees with the shipped + * implementation — this repo has two brace conventions in flight (#7333), and a + * stub picks whichever one the test author had in mind. So the override rung + * below is measured against `FileI18nAdapter`, the actual `II18nService` the + * platform ships, loaded the same way `loadTranslations` loads a real bundle. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { FileI18nAdapter } from '@objectstack/service-i18n'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; +import { SecurityPlugin } from './security-plugin.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +// ── metadata ─────────────────────────────────────────────────────────────── + +/** + * The reporter's shape: an ordinary tenant business object. `crm_contract` + * stands in for the `app_child_object` of the report — a table the operator has + * never seen, whose API name is exactly what must not reach their toast. + */ +const CONTRACT_SCHEMA = { + name: 'crm_contract', + sharingModel: 'private', + fields: { + id: { name: 'id' }, + title: { name: 'title' }, + owner_id: { name: 'owner_id' }, + created_by: { name: 'created_by' }, + organization_id: { name: 'organization_id' }, + }, +}; + +const SCHEMAS: Record = { crm_contract: CONTRACT_SCHEMA }; + +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; + +/** + * Read-and-edit, no DELETE bit — so the refusal below comes from the + * OBJECT-level CRUD gate (step 2) and not from the row-level pre-image gate + * further down, which produces a different sentence. The `developerMessage` + * assertions pin which producer answered. + */ +const APP_READER: PermissionSet = PermissionSetSchema.parse({ + name: 'app_reader', + objects: { + crm_contract: { allowRead: true, allowCreate: true, allowEdit: true }, + }, +}); + +const PERMISSION_SETS: PermissionSet[] = [MEMBER_DEFAULT, APP_READER]; + +const ROW = { + id: 'ct_1', title: 'Theirs', owner_id: 'u_other', created_by: 'u_other', organization_id: 'org1', +}; + +// ── in-memory engine ─────────────────────────────────────────────────────── + +/** + * ⚠️ `find` HONOURS its predicate, and that is load-bearing rather than + * thoroughness. `SecurityPlugin.start()` seeds its bootstrap permission sets + * through `insert`, and the `dbLoader` that backs permission-set resolution + * reads them back with `find('sys_permission_set', { where: { name: { $in: … } } })`. + * A double whose `find` ignores the `where` hands EVERY seeded set back for any + * unresolved name — including `admin_full_access` — and the CRUD gate then + * admits the delete this file exists to see refused. That is exactly what the + * first draft of this harness did: 12 green-looking cases, all of them + * measuring an admission. Measured, not hypothesised. + */ +function makeEngine() { + const tables: Record = { crm_contract: [{ ...ROW }] }; + const middlewares: any[] = []; + const matches = (row: any, filter: any): boolean => { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(row[k])) return false; + continue; + } + if (row[k] !== v) return false; + } + return true; + }; + return { + _tables: tables, + _middlewares: middlewares, + registerMiddleware: (mw: any) => middlewares.push(mw), + getSchema: (name: string) => SCHEMAS[name], + async find(object: string, options: any = {}) { + return (tables[object] ??= []).filter((r) => matches(r, options?.where ?? options?.filter)); + }, + async findOne(object: string, options: any = {}) { + return (await this.find(object, options))[0] ?? null; + }, + async insert(object: string, data: any) { (tables[object] ??= []).push({ ...data }); return data; }, + // Both write verbs open with the PRODUCER's own dispatch predicate, never a + // hand-mirrored guard: a fake looser than `ObjectQL` collects greens from + // call shapes the engine would refuse. Nothing in this file reaches them + // (the gate refuses first, which is the point), so they exist to keep the + // double honest for whatever case is added next. + async update(object: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); + const rows = (tables[object] ??= []); + const targets = dispatch.kind === 'by-id' + ? rows.filter((r) => r.id === dispatch.id) + : rows.filter((r) => matches(r, options?.where)); + for (const r of targets) Object.assign(r, data); + return dispatch.kind === 'by-id' ? (targets[0] ?? null) : targets.length; + }, + async delete(object: string, options?: any) { + const dispatch = assertEngineDeleteDispatch(options); + const rows = (tables[object] ??= []); + const targets = dispatch.kind === 'by-id' + ? rows.filter((r) => r.id === dispatch.id) + : rows.filter((r) => matches(r, options?.where)); + tables[object] = rows.filter((r) => !targets.includes(r)); + return dispatch.kind === 'by-id' ? targets.length > 0 : targets.length; + }, + }; +} + +// ── the stack ────────────────────────────────────────────────────────────── + +interface Denial { + /** The END USER's half — what Console puts in the toast. */ + message: string; + /** The DEVELOPER's half — must never be the same string as `message`. */ + developerMessage?: string; + code?: string; + status?: number; + details?: Record; + /** Everything the plugin logged, so the developer half can be located. */ + logged: { warn: string[]; error: string[] }; +} + +/** + * @param i18n a real `II18nService`, or `undefined` to run the deployment that + * registers none — the built-in catalog must still localize. + */ +async function denyDelete( + locale: string | undefined, + i18n?: FileI18nAdapter, +): Promise { + const engine = makeEngine(); + const metadata = { + get: async (_type: string, name: string) => SCHEMAS[name] ?? null, + list: async () => PERMISSION_SETS, + }; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata, + 'org-scoping': { name: 'org-scoping' }, + ...(i18n ? { i18n } : {}), + }; + const warn: string[] = []; + const error: string[] = []; + const ctx: any = { + logger: { + info: vi.fn(), debug: vi.fn(), + warn: (m: string) => { warn.push(String(m)); }, + error: (m: string) => { error.push(String(m)); }, + }, + registerService: vi.fn(), + getService: (name: string) => { + // A kernel that has no such service THROWS — the shape the plugin's own + // ADR-0029 D8 contribution already guards against, and the reason the + // i18n lookup at the throw site is wrapped. + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx); + await plugin.start(ctx); + const securityMw = engine._middlewares[0]; + + const opCtx: any = { + object: 'crm_contract', + operation: 'delete', + options: { where: { id: ROW.id } }, + context: { + userId: 'u_rep', + tenantId: 'org1', + positions: ['org_member'], + permissions: ['app_reader'], + ...(locale ? { locale } : {}), + }, + }; + try { + await securityMw(opCtx, async () => { /* reached only if the gate admits */ }); + } catch (e: any) { + return { + message: String(e?.message ?? e), + developerMessage: e?.developerMessage, + code: e?.code, + status: e?.statusCode, + details: e?.details, + logged: { warn, error }, + }; + } + throw new Error('expected the CRUD gate to refuse the delete, but it admitted it'); +} + +/** The developer sentence the message used to be — the #7307 "byte for byte" rule. */ +const DEVELOPER_SENTENCE = + "[Security] Access denied: operation 'delete' on object 'crm_contract' " + + 'is not permitted for positions [org_member]'; + +/** + * Everything a business user must never read in this toast: the object's API + * name, the internal authorization noun, the machine operation token, and the + * developer prefix the sentence used to open with. + */ +const FORBIDDEN_IN_USER_COPY = ['crm_contract', 'positions', 'org_member', '[Security]', 'Access denied']; + +describe('#7414 — the 403 an end user reads', () => { + it('speaks the caller locale, not English', async () => { + const zh = await denyDelete('zh-CN'); + expect(zh.message).toBe(BUILTIN_OPERATION_MESSAGES['zh-CN'].permission_denied); + expect(zh.message).toBe('您没有执行此操作的权限,如需访问请联系管理员。'); + + const ja = await denyDelete('ja-JP'); + expect(ja.message).toBe(BUILTIN_OPERATION_MESSAGES['ja-JP'].permission_denied); + // Two locales, two different sentences — so "localized" is measured, not + // assumed from one catalog read. + expect(ja.message).not.toBe(zh.message); + }); + + it('falls back to en for a locale-less context and an uncarried locale', async () => { + for (const locale of [undefined, 'de-DE']) { + const denial = await denyDelete(locale); + expect(denial.message, `locale=${String(locale)}`) + .toBe(BUILTIN_OPERATION_MESSAGES.en.permission_denied); + } + }); + + it('names no object, no operation and no position — in every locale it can render', async () => { + const locales = Object.keys(BUILTIN_OPERATION_MESSAGES); + expect(locales.length).toBeGreaterThanOrEqual(4); + for (const locale of locales) { + const denial = await denyDelete(locale); + // Pinned to the catalog sentence FIRST: a message that had gone empty + // would satisfy every absence assertion below, so the absences are only + // meaningful on top of a positive identity. + expect(denial.message).toBe(BUILTIN_OPERATION_MESSAGES[locale].permission_denied); + expect(denial.message.length).toBeGreaterThan(10); + for (const forbidden of FORBIDDEN_IN_USER_COPY) { + expect(denial.message.toLowerCase(), `${locale} must not say "${forbidden}"`) + .not.toContain(forbidden.toLowerCase()); + } + } + }); +}); + +describe('#7414 — the developer half is moved, not lost', () => { + it('carries the previous sentence byte for byte on `developerMessage`', async () => { + const denial = await denyDelete('zh-CN'); + expect(denial.developerMessage).toBe(DEVELOPER_SENTENCE); + expect(denial.developerMessage).not.toBe(denial.message); + }); + + it('logs it server-side, in English, whatever the caller locale is', async () => { + const denial = await denyDelete('zh-CN'); + expect(denial.logged.warn).toContain(DEVELOPER_SENTENCE); + // The log must not go out in the caller's language — the operator reading + // it is not the caller. + expect(denial.logged.warn.join('\n')).not.toContain('您没有执行此操作的权限'); + }); + + it('keeps `developerMessage` OUT of `details` — `details` is what the dispatcher serialises', async () => { + // The measurement this decision rests on: `http-dispatcher.ts`'s catch does + // `this.error(e.message, 403, { code: 'PERMISSION_DENIED', ...(e.details ?? {}) })` + // and `buildApiError` puts the remainder on the wire as `error.details`. + // Anything inside `details` therefore reaches the browser; the developer + // sentence must not. + const denial = await denyDelete('zh-CN'); + expect(Object.keys(denial.details ?? {})).not.toContain('developerMessage'); + expect(JSON.stringify(denial.details)).not.toContain('[Security]'); + }); +}); + +describe('#7414 — enforcement is untouched', () => { + /** + * ⚠️ Pins in this block are NON-REGRESSION guards, not revert-detectors: + * they are green on `origin/main` too, by construction. That is the point — + * this card is copy-only, and the reverse verification for it is that these + * do NOT move while the message does. + */ + it('is still a 403 PERMISSION_DENIED', async () => { + const denial = await denyDelete('zh-CN'); + expect(denial.code).toBe('PERMISSION_DENIED'); + expect(denial.status).toBe(403); + }); + + it('carries the same structured payload as before', async () => { + const denial = await denyDelete('zh-CN'); + expect(denial.details).toEqual({ + operation: 'delete', + object: 'crm_contract', + positions: ['org_member'], + permissionSets: ['app_reader'], + }); + }); +}); + +describe('#7414 — resolution ladder, against the REAL II18nService', () => { + const bundleFor = (entries: Record) => { + const adapter = new FileI18nAdapter({ defaultLocale: 'en' }); + adapter.loadTranslations('zh-CN', entries); + return adapter; + }; + + it('a deployment override under `errors.permission_denied` wins', async () => { + const denial = await denyDelete('zh-CN', bundleFor({ + errors: { permission_denied: '此操作已被安全策略阻止,请联系系统管理员。' }, + })); + expect(denial.message).toBe('此操作已被安全策略阻止,请联系系统管理员。'); + }); + + it('a bundle that carries no such key falls through to the built-in catalog', async () => { + // The real adapter echoes the KEY back on a miss — the contract + // `renderOperationMessage` detects a miss by. Measured here rather than + // stubbed, because a stub is free to answer `undefined` and hide the fact + // that the producer must recognise an echo. + const denial = await denyDelete('zh-CN', bundleFor({ + objects: { crm_contract: { label: '合同' } }, + })); + expect(denial.message).toBe(BUILTIN_OPERATION_MESSAGES['zh-CN'].permission_denied); + }); + + it('leaves a broken override visibly broken rather than silently blank', async () => { + // This sentence takes no parameters, so an override that references one is + // an authoring mistake. It must stay legible as a mistake — the #7333 + // brace-convention trap is only findable if the placeholder survives. + const denial = await denyDelete('zh-CN', bundleFor({ + errors: { permission_denied: '无权限:{{objectLabel}}' }, + })); + expect(denial.message).toBe('无权限:{{objectLabel}}'); + }); + + it('a deployment with NO i18n service still gets the caller locale', async () => { + const denial = await denyDelete('zh-CN'); + expect(denial.message).toBe(BUILTIN_OPERATION_MESSAGES['zh-CN'].permission_denied); + }); +}); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index e2054d5321..f0fe54976e 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -4,6 +4,10 @@ import { Plugin, PluginContext, POSTURE_LADDER } from '@objectstack/core'; import type { PermissionSet, RowLevelSecurityPolicy } from '@objectstack/spec/security'; import { describeHighPrivilegeBits, describeAnchorForbiddenBits, PUBLIC_FORM_SERVER_MANAGED_FIELDS } from '@objectstack/spec/security'; import { MCP_AGENT_PERMISSION_SET_RESTRICTED } from '@objectstack/spec/ai'; +// [#7414] The SHARED operation-message catalog #7307 built for the data path's +// operation-level refusals. Second consumer, same mechanism — a second remedy +// for one defect class is what that module exists to prevent. +import { renderOperationMessage } from '@objectstack/spec/system'; import { PermissionEvaluator, crudBucketForOperation } from './permission-evaluator.js'; import { DelegatedAdminGate } from './delegated-admin-gate.js'; import { @@ -354,6 +358,48 @@ export interface SecurityPluginOptions { */ export { describeHighPrivilegeBits } from '@objectstack/spec/security'; +/** + * [#7414] The END USER's half of an object-permission refusal. + * + * Both transports put a 403's `Error.message` on the wire as the body's + * human-readable string — `@objectstack/rest`'s `mapDataError` as `body.error`, + * the runtime dispatcher as `error.message` — and Console renders that verbatim + * in a toast. The CRUD gate composed it English-only with the object's API name + * and the caller's `positions` concatenated in, so an operator in a fully + * localized app read a sentence naming a table they have never seen and an + * authorization vocabulary that reads as a contradiction to someone who does + * hold rights on the record they clicked. On a cascade delete it is worse + * still: `cascadeDeleteRelations` re-authorises every CHILD independently, so + * the object named is one the operator never addressed. + * + * Rendered through the SHARED operation-message catalog #7307 built + * (`@objectstack/spec/system`), not a second mechanism: same `errors.` + * override address, same resolution ladder (deployment override → locale + * catalog → `en` → the key), same guarantee that a misbehaving i18n service + * cannot turn a 403 into a 500. + * + * The i18n service is optional and resolved per call: it is registered by a + * different plugin, may register after this one, and a deployment that runs + * without it still gets the built-in catalog in the caller's locale. + */ +function userFacingDenialMessage(ctx: PluginContext, locale: string | undefined): string { + let translate: + | ((key: string, loc: string, params?: Record) => string) + | undefined; + try { + const i18n = ctx.getService('i18n'); + const t = i18n?.t; + if (typeof t === 'function') { + translate = (key, loc, params) => t.call(i18n, key, loc, params); + } + } catch { + // i18n is optional (ADR-0029 D8 registers it from another plugin, possibly + // later than this one). The built-in catalog still renders the caller's + // locale without it. + } + return renderOperationMessage({ messageKey: 'permission_denied' }, { locale, translate }); +} + export class SecurityPlugin implements Plugin { name = 'com.objectstack.security'; /** @@ -1156,10 +1202,54 @@ export class SecurityPlugin implements Plugin { ); if (!allowed) { - throw new PermissionDeniedError( + // [#7414] TWO messages, two audiences — the split #7307 made for + // `DELETE_RESTRICTED`, reached from the authorization side and through + // the SAME catalog. + // + // `message` is what a BUSINESS USER reads, because both transports + // ship it verbatim as the body's human-readable string and Console + // renders it as-is in a toast. It is now a localized sentence that + // names no object, no operation and no position. + // + // `developerMessage` is the previous sentence BYTE FOR BYTE. Where it + // goes is where this card had to diverge from #7307, and the reason is + // measured, not assumed. #7423 shipped its developer half over the + // wire on the grounds that "it discloses nothing the envelope did not + // already carry: `dependentObject` and `object` are API names on the + // same body". That premise does NOT hold here on both transports: + // + // - `@objectstack/rest`'s `mapDataError` 403 branch builds + // `{ error, code, object? }` and never reads `error.details`, and + // that `object` is the object the ROUTE named — so `positions`, + // the operation, and (on a cascade) the CHILD object's API name + // reach the client through NOTHING but this message today; + // - the runtime dispatcher does spread `e.details` onto the body + // (`http-dispatcher.ts` → `buildApiError` → `error.details`), so + // there they are already disclosed. + // + // One error class cannot honestly have a per-transport disclosure + // policy, and a card whose purpose is to REDUCE what a browser is told + // must not add a new disclosure on the transport that discloses less. + // So the developer half is NOT shipped: it goes to the server log, + // which is where an app builder debugging a 403 already looks. It is + // attached to the error as a SIBLING of `details`, never a member of + // it — `details` is the field the dispatcher serialises. + // + // `code` / `statusCode` / `details` are untouched: one + // `PERMISSION_DENIED` (ADR-0112), one 403, two sentences. + const developerMessage = `[Security] Access denied: operation '${opCtx.operation}' on object '${opCtx.object}' ` + - `is not permitted for positions [${positions.join(', ')}]`, + `is not permitted for positions [${positions.join(', ')}]`; + ctx.logger.warn(developerMessage, { + operation: opCtx.operation, + object: opCtx.object, + positions, + userId: opCtx.context?.userId ?? 'unknown', + }); + throw new PermissionDeniedError( + userFacingDenialMessage(ctx, opCtx.context?.locale), { operation: opCtx.operation, object: opCtx.object, positions, permissionSets: explicitPermissionSets }, + developerMessage, ); } diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index b69632e03b..027736dc5e 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -2453,6 +2453,52 @@ describe('mapDataError — schema/constraint envelopes', () => { expect(r.body).not.toHaveProperty('developerMessage'); }); + // [#7414] The 403 does NOT get #7307's treatment, and this pins WHY rather + // than merely that it does not. + // + // #7423 shipped `developerMessage` on the 409 because "it discloses nothing + // the envelope did not already carry: `dependentObject` and `object` are API + // names on the same body". That premise is FALSE for this branch: the 403 + // body below is `{ error, code, object? }` and never reads `error.details`, + // so `positions`, the operation, and — on a cascade delete, where every child + // is re-authorised independently — the CHILD object's API name are on the + // wire through NOTHING but the message. Mirroring #7307 here would therefore + // ADD a disclosure of internal authorization vocabulary, in a card whose + // whole purpose was to remove one. The developer half is logged at the throw + // site (`plugin-security`'s CRUD gate) instead. + it('never ships a PERMISSION_DENIED developer half or its structured details to the client', () => { + const r = mapDataError( + Object.assign(new Error('您没有执行此操作的权限,如需访问请联系管理员。'), { + code: 'PERMISSION_DENIED', + name: 'PermissionDeniedError', + statusCode: 403, + developerMessage: + "[Security] Access denied: operation 'delete' on object 'app_child_object' " + + 'is not permitted for positions [org_member, everyone]', + details: { + operation: 'delete', + object: 'app_child_object', + positions: ['org_member', 'everyone'], + permissionSets: ['app_reader'], + }, + }), + 'app_parent_object', + ); + expect(r.status).toBe(403); + // Positive identity first: the absence assertions below are only meaningful + // on top of a body that really carries the localized sentence. + expect(r.body.error).toBe('您没有执行此操作的权限,如需访问请联系管理员。'); + expect(r.body.code).toBe('PERMISSION_DENIED'); + expect(r.body).not.toHaveProperty('developerMessage'); + expect(r.body).not.toHaveProperty('details'); + expect(r.body).not.toHaveProperty('positions'); + expect(JSON.stringify(r.body)).not.toContain('positions'); + expect(JSON.stringify(r.body)).not.toContain('app_child_object'); + // The object the ROUTE named still rides, unchanged — it is the object the + // caller themselves addressed, and this branch always carried it. + expect(r.body.object).toBe('app_parent_object'); + }); + it('maps SQLite "has no column named" → 400 INVALID_FIELD with the field', () => { const r = mapDataError( sqliteError( diff --git a/packages/runtime/src/domains/share-links-enforcement-context.test.ts b/packages/runtime/src/domains/share-links-enforcement-context.test.ts index 460c1b3286..d63a85703d 100644 --- a/packages/runtime/src/domains/share-links-enforcement-context.test.ts +++ b/packages/runtime/src/domains/share-links-enforcement-context.test.ts @@ -55,6 +55,7 @@ import { SHARE_LINK_SERVICE } from '@objectstack/spec/contracts'; import { PermissionDeniedError, SecurityPlugin } from '@objectstack/plugin-security'; import { ShareLinkService } from '@objectstack/plugin-sharing'; import { ApiErrorSchema, BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; +import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; import { apiErrorResponse } from '../error-envelope.js'; import { handleShareLinksRequest } from './share-links.js'; import { HttpDispatcher } from '../http-dispatcher.js'; @@ -605,7 +606,17 @@ describe('[#6649] a security-middleware refusal keeps its own status through the // message trips no clause of `looksLikeInternalErrorLeak` anyway). It // pins the refusal's own reason against a FUTURE widening of that // heuristic swallowing an authorization answer. - expect(res.body.error.message).toContain('Access denied'); + // + // [#7414] Re-spelled, not weakened. This used to read + // `toContain('Access denied')`, which was the CRUD gate's developer + // sentence; that sentence is now `developerMessage` (logged, not + // shipped) and `message` is the user-facing catalog entry rendered in + // `ExecutionContext.locale` — `en` here, since this caller declares no + // locale. Asserted against the catalog constant rather than a literal so + // a future copy edit does not need to re-spell this file, and still + // proves the same thing: an authorization answer reached the client + // instead of being swallowed into the generic internal-error string. + expect(res.body.error.message).toBe(BUILTIN_OPERATION_MESSAGES.en.permission_denied); }, 30_000); it('group posture: the same denial, the same envelope — the defect was never posture-specific', async () => { diff --git a/packages/spec/src/system/operation-message.test.ts b/packages/spec/src/system/operation-message.test.ts index 8453f9be47..3bc42bb9ab 100644 --- a/packages/spec/src/system/operation-message.test.ts +++ b/packages/spec/src/system/operation-message.test.ts @@ -91,3 +91,91 @@ describe('operation message catalog', () => { expect(operationMessageTranslationKey('delete_restricted')).not.toContain('validation.field'); }); }); + +/** + * #7414 — the SECOND consumer of this catalog: plugin-security's object-CRUD + * gate (`403 PERMISSION_DENIED`). The call site is pinned in + * `packages/plugins/plugin-security/src/permission-denied-user-copy.test.ts`, + * against the real middleware and a real `II18nService`. + */ +describe('operation message catalog — permission_denied (#7414)', () => { + /** + * The vocabulary a business user must never read in a permission refusal. + * `positions` is the internal authorization noun the reporter quoted; the + * rest is the shape of the sentence it was embedded in. + */ + const DEVELOPER_VOCABULARY = [ + 'positions', + 'permissionSets', + 'permission set', + '[Security]', + 'Access denied', + 'operation', + ]; + + it('renders the caller locale, not English', () => { + expect(renderOperationMessage({ messageKey: 'permission_denied' }, { locale: 'zh-CN' })) + .toBe('您没有执行此操作的权限,如需访问请联系管理员。'); + expect(renderOperationMessage({ messageKey: 'permission_denied' }, { locale: 'en' })) + .toBe('You do not have permission to perform this action. Contact your administrator if you need access.'); + }); + + it('matches a base language against a regional catalog key (ja → ja-JP)', () => { + expect(renderOperationMessage({ messageKey: 'permission_denied' }, { locale: 'ja' })) + .toBe(BUILTIN_OPERATION_MESSAGES['ja-JP'].permission_denied); + }); + + it('falls back to the en sentence for a locale the catalog does not carry', () => { + // `de-DE` has no catalog entry and no base-language sibling. + expect(renderOperationMessage({ messageKey: 'permission_denied' }, { locale: 'de-DE' })) + .toBe(BUILTIN_OPERATION_MESSAGES.en.permission_denied); + }); + + it('names no object, no operation and no position — in EVERY locale', () => { + const locales = Object.keys(BUILTIN_OPERATION_MESSAGES); + // Guard the guard: a catalog that lost its locales would make the loop + // below vacuously true, which is exactly the shape of an assertion that + // cannot fail. + expect(locales.length).toBeGreaterThanOrEqual(4); + for (const locale of locales) { + const rendered = renderOperationMessage({ messageKey: 'permission_denied' }, { locale }); + // Non-empty and locale-specific, so the absence assertions below cannot + // be satisfied by an empty string. + expect(rendered).toBe(BUILTIN_OPERATION_MESSAGES[locale].permission_denied); + expect(rendered.length).toBeGreaterThan(10); + for (const word of DEVELOPER_VOCABULARY) { + expect(rendered.toLowerCase(), `${locale} must not say "${word}"`) + .not.toContain(word.toLowerCase()); + } + } + }); + + it('ships no unfilled placeholder in any locale — the sentence takes no params', () => { + // Asserts on the CATALOG ENTRY, not on the rendering, and that is the + // difference between a guard and a decoration. Rendering a removed key + // yields the bare messageKey — which has no braces either, so a + // rendering-based version of this case would stay green on a catalog that + // lost the key entirely. Reading the entry makes it bite twice: on a + // missing locale (the entry is `undefined`) and on a template that shipped + // a `{{name}}` / `{name}` nobody fills, which is the #7333 class of bug + // where this repo's two brace conventions get mixed up. + for (const [locale, catalog] of Object.entries(BUILTIN_OPERATION_MESSAGES)) { + expect(catalog.permission_denied, `${locale} defines permission_denied`).toBeTypeOf('string'); + expect(catalog.permission_denied, `${locale} placeholder-free`).not.toMatch(/[{}]/); + } + }); + + it('a deployment translation override wins, under the shared `errors.` address', () => { + expect(operationMessageTranslationKey('permission_denied')).toBe('errors.permission_denied'); + const translate = (key: string) => + key === 'errors.permission_denied' ? '此操作已被安全策略阻止。' : key; + expect(renderOperationMessage({ messageKey: 'permission_denied' }, { locale: 'zh-CN', translate })) + .toBe('此操作已被安全策略阻止。'); + }); + + it('a throwing i18n service does not turn a 403 into a 500', () => { + const translate = () => { throw new Error('service down'); }; + expect(renderOperationMessage({ messageKey: 'permission_denied' }, { locale: 'zh-CN', translate })) + .toBe(BUILTIN_OPERATION_MESSAGES['zh-CN'].permission_denied); + }); +}); diff --git a/packages/spec/src/system/operation-message.ts b/packages/spec/src/system/operation-message.ts index 3040ce5774..88ebf6c902 100644 --- a/packages/spec/src/system/operation-message.ts +++ b/packages/spec/src/system/operation-message.ts @@ -5,9 +5,12 @@ * * The localized message templates for the data path's OPERATION-level * refusals — a write the engine declines as a whole, rather than a constraint - * one field violated. Today that is the referential-integrity refusal - * (`409 DELETE_RESTRICTED`, `cascadeDeleteRelations`'s `restrict` branch); the - * catalog is the seat for the rest of the family as they are localized. + * one field violated. Two members today: the referential-integrity refusal + * (`409 DELETE_RESTRICTED`, `cascadeDeleteRelations`'s `restrict` branch, #7307) + * and the object-permission refusal (`403 PERMISSION_DENIED`, plugin-security's + * CRUD gate, #7414). The catalog is the seat for the rest of the family as they + * are localized — a second mechanism for the second producer is exactly what + * this module exists to prevent. * * ## Why this is a SEPARATE catalog from `validation-message.ts` * @@ -88,27 +91,45 @@ export function operationMessageTranslationKey(messageKey: string): string { * caller's locale (the API names live on `developerMessage` and on the * structured `object` / `dependentObject` fields), `{{field}}` is the * referencing field's label, `{{count}}` the number of dependent records. + * + * `permission_denied` (#7414) takes NO placeholders, and that is a deliberate + * divergence from its sibling rather than an omission. `delete_restricted` + * names the objects because the user must know WHICH related records block + * them — that is the action they can take. An object-permission refusal gives + * the user nothing to act on by naming the object, and on a cascade delete the + * object the gate refuses is a CHILD the operator never addressed and may not + * know exists (`cascadeDeleteRelations` re-authorises every child + * independently). Naming it would be accurate and still misleading, so the + * sentence names nothing: no object, no operation, no `positions`. The machine + * detail stays on the error's structured `details` and on `developerMessage`, + * which is logged server-side (see `plugin-security`'s CRUD gate). */ export const BUILTIN_OPERATION_MESSAGES: Record> = { en: { + permission_denied: + 'You do not have permission to perform this action. Contact your administrator if you need access.', delete_restricted: 'This {{object}} is still referenced by {{count}} {{dependentObject}} record(s) through “{{field}}”. Delete or reassign them first.', delete_restricted_required: 'This {{object}} is still referenced by {{count}} {{dependentObject}} record(s) through “{{field}}”, which is required and cannot be cleared. Delete or reassign them first.', }, 'zh-CN': { + permission_denied: '您没有执行此操作的权限,如需访问请联系管理员。', delete_restricted: '该{{object}}正被 {{count}} 条{{dependentObject}}记录通过「{{field}}」引用,请先删除或改派这些记录。', delete_restricted_required: '该{{object}}正被 {{count}} 条{{dependentObject}}记录通过「{{field}}」引用,且该字段为必填、无法清空,请先删除或改派这些记录。', }, 'ja-JP': { + permission_denied: 'この操作を実行する権限がありません。アクセスが必要な場合は管理者にお問い合わせください。', delete_restricted: 'この{{object}}は {{count}} 件の{{dependentObject}}レコードから「{{field}}」で参照されています。先にそれらを削除するか、参照先を変更してください。', delete_restricted_required: 'この{{object}}は {{count}} 件の{{dependentObject}}レコードから「{{field}}」で参照されています。この項目は必須のため空にできません。先にそれらを削除するか、参照先を変更してください。', }, 'es-ES': { + permission_denied: + 'No tiene permiso para realizar esta acción. Póngase en contacto con su administrador si necesita acceso.', delete_restricted: '{{count}} registro(s) de {{dependentObject}} todavía hacen referencia a este {{object}} mediante «{{field}}». Elimínelos o reasígnelos primero.', delete_restricted_required: diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 488f9b27f3..a8a4afe865 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1701,6 +1701,9 @@ importers: '@objectstack/plugin-sharing': specifier: workspace:* version: link:../plugin-sharing + '@objectstack/service-i18n': + specifier: workspace:* + version: link:../../services/service-i18n '@types/node': specifier: ^26.1.2 version: 26.1.2