From 69e71ffb9f2fd320724c4afead11a21970ce8cbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20Zi=C3=B3=C5=82ek?= Date: Mon, 25 May 2026 22:55:50 +0200 Subject: [PATCH 1/2] docs(approval-gate): mark role/quorum fields as advisory; runtime warns when set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The approval-gate schema accepts `allowedRoles`, `requiredApprovers`, and `requireReason`, and the documentation across `docs/reference/component-catalog.md`, `docs/guides/creating-blueprints.md`, `docs/guides/creating-documents.md`, and `docs/guides/enterprise-features.md` previously described these as enforced controls ("Supports role-based access control", "Approval gate enforces manager/director role", etc.). `SECURITY.md` classified approval-gate bypass as a reportable vulnerability. The runtime does not, in fact, verify approver identity, enforce role membership, or require multiple distinct approvers: - `packages/renderer-react/src/components/ApprovalGateRenderer.tsx:30-49` dispatches `APPROVAL_GRANTED` with a hardcoded `actor: { id: 'current-user' }` straight to `store.dispatch`, bypassing `approvalGateHandler.onAction`. The handler's `ctx.policy.enforce('approval_grant')` is unreachable from the UI. - `packages/runtime/src/core/document-store.ts:194-201` handles `APPROVAL_GRANTED` by setting `status: 'approved'` without invoking `policy.enforce` and without consulting `allowedRoles` / `requiredApprovers` / `requireReason`. - `createDocumentStore` accepts `options.registry?: AttachableRegistry` but the function body never reads it. This change aligns documentation, schema descriptions, and `SECURITY.md` with shipped behaviour, and adds a runtime warning so embedders are signalled when they declare advisory fields. No behaviour change beyond the new warning. Schema (`packages/spec/src/schemas/components/approval-gate.ts`) - `.describe()` text on `requiredApprovers`, `allowedRoles`, `requireReason` marks them advisory and points at the security-model callout. The describe for `requiredApprovers` makes explicit that the default value of `1` is equally advisory at the runtime layer. Runtime (`packages/runtime/src/core/document-store.ts`) - `warnIfAdvisoryApprovalFields(comp)` helper, called once per gate per store instance from the init loop and `updateAst`'s new-component branch. - The warning lists which advisory fields the gate declares and states explicitly that ALL approval-gate role/quorum/reason fields are advisory at the runtime layer regardless of which one triggered the warning. - JSDoc acknowledges the `console.warn` delivery limitation (suppressed in `vitest --silent`, SSR, structured-logger pipelines). A structured-logger integration is the natural follow-up but is intentionally deferred because `packages/runtime` takes no logger dependency today. Tests (`packages/runtime/tests/document-store.test.ts`) - Four new tests via `vi.spyOn(console, 'warn')` covering: (a) init with `allowedRoles` warns once, (b) `requiredApprovers > 1` + `requireReason: true` warns with both fields listed, (c) bare gate does NOT warn (regression guard), (d) gate added later via `updateAst` warns when it enters the store. Docs - `component-catalog.md`: dropped "Supports role-based access control" from the approval-gate opener. Added an "Important — security model" callout above the property table spelling out the trust boundary. Field table marks the three fields advisory. Rewrote the example description to drop the "SOX compliance requirement" framing. - `creating-blueprints.md`: two "Approval gate enforces … role" checklist items changed to "declares … (host application enforces)". - `creating-documents.md`: advisory callouts after both approval-gate examples. - `enterprise-features.md`: the "Putting It All Together" code comment notes that runtime policy enforcement applies to `webhook_call` only and points at the approval-gate security model. Security policy (`SECURITY.md`) - Reworded the approval-gate-bypass bullet so it no longer classifies advisory-by-design behaviour as a reportable vulnerability. Reports that hinge on runtime-layer enforcement of `allowedRoles` / `requiredApprovers` / `requireReason` are closed not-a-bug. Out of scope (intentionally — happy to follow up) - Wiring `approval-gate-handler` into `document-store`'s dispatch path. Requires deciding how the host injects a verified actor identity (proposed shape: `MdmaProvider currentUser={{id, role}}` flowing into `AttachableContext`). - Per-blueprint compliance claims in `blueprints/change-management`, `blueprints/clinical-ops`, `blueprints/kyc-case`, `blueprints/incident-triage`, `blueprints/customer-escalation`. Their manifests/READMEs name the approval-gate as the implementation of specific controls (SOX 404, clinical- staff restriction, etc.). Those edits warrant the maintainers' judgement. - Prompt-pack author guidance (`packages/prompt-pack/src/prompts/mdma-author/_shared.ts`) telling the LLM these fields restrict approvers. Should track whatever the enforcement-wiring decision above decides. - Refactoring advisory-fields checks into a `walkComponents(ast, visitor)` utility once more component types carry advisory metadata. --- SECURITY.md | 2 +- docs/guides/creating-blueprints.md | 4 +- docs/guides/creating-documents.md | 4 + docs/guides/enterprise-features.md | 7 +- docs/reference/component-catalog.md | 26 +++- packages/runtime/src/core/document-store.ts | 48 ++++++++ packages/runtime/tests/document-store.test.ts | 116 ++++++++++++++++++ .../src/schemas/components/approval-gate.ts | 27 +++- 8 files changed, 221 insertions(+), 13 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 86bd27d..4ac893c 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -46,7 +46,7 @@ The following are in scope: - **PII leakage** -- sensitive fields (marked `sensitive: true`) appearing in logs, event bus payloads, or rendered output. - **Redaction failures** -- the redactor (`@mobile-reality/mdma-runtime`) failing to mask, hash, or omit PII as configured. - **Event log integrity** -- tampering with or forging audit log entries (hash-chain breaks). -- **Approval gate bypass** -- approval-gate components accepting unauthorized approvers or skipping required approvals. +- **Approval gate bypass** -- the approval-gate component's `allowedRoles`, `requiredApprovers`, and `requireReason` fields are advisory metadata surfaced to the host application; the MDMA runtime does not enforce them. Host-side authentication and authorization of approvers is the responsibility of the embedding application and is out of scope for this project. Reports that hinge on those fields being enforced at the runtime layer will be closed as not-a-bug. - **Injection via bindings** -- binding expressions (`{{variable.path}}`) that resolve to executable code or leak cross-component state. - **Dependency vulnerabilities** -- known CVEs in direct dependencies that are exploitable in MDMA's usage context. diff --git a/docs/guides/creating-blueprints.md b/docs/guides/creating-blueprints.md index 398a2ad..170da6f 100644 --- a/docs/guides/creating-blueprints.md +++ b/docs/guides/creating-blueprints.md @@ -46,7 +46,7 @@ integrations: checklists: security: - PII fields (reporter_email) marked sensitive - - Approval gate enforces manager/director role + - Approval gate declares manager/director role (host application enforces — see approval-gate security model in `docs/reference/component-catalog.md`) logging: - All form submissions are logged with timestamp - Approval decisions are audit-logged @@ -234,7 +234,7 @@ The `checklists` section documents what has been verified. This serves as a comp checklists: security: - All PII fields marked sensitive - - Approval gate enforces authorized roles + - Approval gate declares authorized roles (host application enforces — MDMA does not verify approver identity) - Data encrypted at rest and in transit logging: - All form submissions audit-logged diff --git a/docs/guides/creating-documents.md b/docs/guides/creating-documents.md index 262deaa..d48780b 100644 --- a/docs/guides/creating-documents.md +++ b/docs/guides/creating-documents.md @@ -77,6 +77,8 @@ allowedRoles: In this example, the approval gate's `visible` property is bound to the `user_role` field. When `user_role` has a truthy value, the gate becomes visible. +> **Advisory.** `allowedRoles` is metadata MDMA surfaces to the host application; it is **not** a control the runtime enforces. Authenticating the user, verifying their role, and gating downstream actions are the host application's responsibility. See [approval-gate](../reference/component-catalog.md#approval-gate) for the security model. + ### Binding Syntax Rules - Must start with `{{` and end with `}}` @@ -269,6 +271,8 @@ onDeny: reopen-case requireReason: true ``` +> **Important.** `allowedRoles`, `requiredApprovers`, and `requireReason` are advisory — MDMA does not verify approver identity, enforce role membership, or require multiple distinct approvers. Host applications must authenticate users, check their roles before dispatching `APPROVAL_GRANTED`, and enforce downstream authorization on their own backend. See [approval-gate](../reference/component-catalog.md#approval-gate) for the security model. + ## Validation Checklist Before finalizing a document, verify: diff --git a/docs/guides/enterprise-features.md b/docs/guides/enterprise-features.md index 8861706..d70c99a 100644 --- a/docs/guides/enterprise-features.md +++ b/docs/guides/enterprise-features.md @@ -316,7 +316,12 @@ if (report.summary.failed > 0) { throw new Error(`Compliance check failed: ${report.summary.failed} failures`); } -// 3. Create store with policy enforcement +// 3. Create the store. +// Note: MDMA's runtime enforces policy only for the webhook integration +// (action 'webhook_call'). The approval-gate fields `allowedRoles`, +// `requiredApprovers`, and `requireReason` are advisory — the host application +// is responsible for verifying approver identity and quorum before dispatching +// APPROVAL_GRANTED. See docs/reference/component-catalog.md#approval-gate. const registry = new AttachableRegistry(); registerAllCoreAttachables(registry); diff --git a/docs/reference/component-catalog.md b/docs/reference/component-catalog.md index 369bc85..bd7a182 100644 --- a/docs/reference/component-catalog.md +++ b/docs/reference/component-catalog.md @@ -294,7 +294,20 @@ dismissible: true ## approval-gate -Blocks workflow progression until the required number of approvals is received. Supports role-based access control. +Surfaces an approval-workflow UI element. The gate's status (`pending` / `approved` / `denied`) is exposed as a binding (`{{.status}}`) for downstream components. + +> **Security model — read first.** +> +> The MDMA runtime does **not** verify approver identity, enforce role membership, or require multiple distinct approvers. The `allowedRoles`, `requiredApprovers`, and `requireReason` fields below are **advisory metadata only** — they describe the policy a host application should enforce, but MDMA itself does not enforce them. Anyone able to dispatch an `APPROVAL_GRANTED` action against the store can flip a gate to `approved` regardless of role, count, or reason. +> +> Host applications embedding MDMA are responsible for: +> +> 1. Authenticating the user and supplying a verified actor identity. +> 2. Checking that identity against `allowedRoles` before invoking `store.dispatch({ type: 'APPROVAL_GRANTED', ... })`. +> 3. Tracking distinct approvers if `requiredApprovers > 1` is intended to be meaningful. +> 4. Enforcing end-to-end authorization on any backend the approval triggers (defense in depth — client-side controls are bypassable in any browser). +> +> When the runtime detects any of these advisory fields on a gate, it emits a `console.warn` once per gate at store initialization to make the responsibility explicit. ### Properties @@ -303,11 +316,11 @@ Blocks workflow progression until the required number of approvals is received. | `type` | `"approval-gate"` | *required* | Must be `"approval-gate"`. | | `title` | `string` | *required* | Gate title. Min length 1. | | `description` | `string` | -- | Additional details about the approval. | -| `requiredApprovers` | `number` | `1` | Number of approvals needed. Positive integer. | -| `allowedRoles` | `string[]` | -- | Restrict who can approve. If omitted, anyone can approve. | +| `requiredApprovers` | `number` | `1` | **Advisory only** — not enforced by MDMA. The host application is responsible for tracking distinct approvers. Positive integer. | +| `allowedRoles` | `string[]` | -- | **Advisory only** — not enforced by MDMA. The host application is responsible for verifying the actor's role before dispatching `APPROVAL_GRANTED`. | | `onApprove` | `string` | -- | Action ID triggered when approval is granted. | | `onDeny` | `string` | -- | Action ID triggered when approval is denied. | -| `requireReason` | `boolean` | `false` | Require the denier to provide a reason. | +| `requireReason` | `boolean` | `false` | **Advisory only** — UI may surface a reason input on denial; the MDMA runtime does not block dispatch when a reason is omitted. | ### Example @@ -316,8 +329,9 @@ id: dual-approval type: approval-gate title: Production Change Approval description: > - Requires sign-off from both a tech lead and a manager. - This is a SOX compliance requirement for all production changes. + Surfaces the approval workflow for a production change. Authenticating + the signers and enforcing the role/quorum constraints below is the host + application's responsibility — see the "Security model" callout above. requiredApprovers: 2 allowedRoles: - tech-lead diff --git a/packages/runtime/src/core/document-store.ts b/packages/runtime/src/core/document-store.ts index 4229a09..96d477b 100644 --- a/packages/runtime/src/core/document-store.ts +++ b/packages/runtime/src/core/document-store.ts @@ -75,6 +75,8 @@ export function createDocumentStore( redactionCtx.sensitiveComponents.add(comp.id); } + warnIfAdvisoryApprovalFields(comp); + // Extract sensitive fields from form components if (comp.type === 'form') { for (const field of comp.fields) { @@ -286,6 +288,8 @@ export function createDocumentStore( redactionCtx.sensitiveComponents.add(comp.id); } + warnIfAdvisoryApprovalFields(comp); + if (comp.type === 'form') { for (const field of comp.fields) { if (field.sensitive) { @@ -340,6 +344,50 @@ function isPendingMdmaBlock(node: unknown): boolean { ); } +/** + * approval-gate's `allowedRoles`, `requiredApprovers`, and `requireReason` fields + * are accepted by the schema and surfaced to the host UI, but the MDMA runtime + * does NOT verify approver identity, enforce role membership, or require multiple + * distinct approvers. If a document declares any of these fields above their + * schema defaults, warn so that embedding applications are not misled into + * treating the gate as a security control. + * + * Note: `requiredApprovers: 1` is the schema default and is suppressed here to + * avoid noise on gates that carry no quorum intent. It is still advisory at the + * runtime layer regardless of value — the warning text below makes that explicit. + * + * Emitted once per gate per store instance the first time it enters the store + * (init + updateAst's "new component" branch only — re-renders do not re-warn). + * The warning is delivered via `console.warn`, which is suppressed in some + * embedder pipelines (e.g. vitest --silent, SSR redirected to a structured + * logger). If embedders need durable advisory signalling, a structured-logger + * integration is the natural follow-up; that is intentionally out of scope here + * because `packages/runtime` does not currently take a logger dependency. + */ +function warnIfAdvisoryApprovalFields(comp: MdmaBlock['component']): void { + if (comp.type !== 'approval-gate') return; + const hasAllowedRoles = Array.isArray(comp.allowedRoles) && comp.allowedRoles.length > 0; + const hasMultiApproverRequirement = + typeof comp.requiredApprovers === 'number' && comp.requiredApprovers > 1; + const hasRequireReason = comp.requireReason === true; + if (!hasAllowedRoles && !hasMultiApproverRequirement && !hasRequireReason) return; + + const fields: string[] = []; + if (hasAllowedRoles) fields.push('allowedRoles'); + if (hasMultiApproverRequirement) fields.push('requiredApprovers'); + if (hasRequireReason) fields.push('requireReason'); + + console.warn( + `[mdma] approval-gate "${comp.id}" declares ${fields.join(', ')}, ` + + 'but ALL approval-gate role/quorum/reason fields are advisory at the runtime layer. ' + + 'The MDMA runtime does not verify approver identity, enforce role membership, ' + + 'track distinct approvers (regardless of requiredApprovers value, including 1), ' + + 'or block dispatch when a reason is omitted. ' + + 'The host application is responsible for authentication and authorization. ' + + 'See docs/reference/component-catalog.md#approval-gate.', + ); +} + /** Extract the `id` field from partial YAML content. */ function extractIdFromYaml(yaml?: string): string | null { if (!yaml) return null; diff --git a/packages/runtime/tests/document-store.test.ts b/packages/runtime/tests/document-store.test.ts index 12e327a..8814ac8 100644 --- a/packages/runtime/tests/document-store.test.ts +++ b/packages/runtime/tests/document-store.test.ts @@ -196,3 +196,119 @@ describe('DocumentStore', () => { expect(comp?.values.deniedReason).toBe('Not ready'); }); }); + +describe('warnIfAdvisoryApprovalFields', () => { + it('warns at init when an approval-gate declares allowedRoles', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const ast = makeAst([ + { + id: 'gate-with-roles', + type: 'approval-gate', + title: 'Approve', + sensitive: false, + disabled: false, + visible: true, + allowedRoles: ['manager'], + requiredApprovers: 1, + requireReason: false, + }, + ]); + + createDocumentStore(ast); + + expect(warn).toHaveBeenCalledTimes(1); + const message = warn.mock.calls[0][0] as string; + expect(message).toContain('gate-with-roles'); + expect(message).toContain('allowedRoles'); + expect(message).toContain('advisory'); + warn.mockRestore(); + }); + + it('warns when requiredApprovers > 1 or requireReason: true', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const ast = makeAst([ + { + id: 'gate-quorum', + type: 'approval-gate', + title: 'Approve', + sensitive: false, + disabled: false, + visible: true, + requiredApprovers: 2, + requireReason: true, + }, + ]); + + createDocumentStore(ast); + + expect(warn).toHaveBeenCalledTimes(1); + const message = warn.mock.calls[0][0] as string; + expect(message).toContain('requiredApprovers'); + expect(message).toContain('requireReason'); + warn.mockRestore(); + }); + + it('does NOT warn for a bare approval-gate with no advisory fields set', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const ast = makeAst([ + { + id: 'bare-gate', + type: 'approval-gate', + title: 'Approve', + sensitive: false, + disabled: false, + visible: true, + }, + ]); + + createDocumentStore(ast); + + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it('warns when an approval-gate with advisory fields is added via updateAst', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const initialAst = makeAst([ + { + id: 'form1', + type: 'form', + sensitive: false, + disabled: false, + visible: true, + fields: [], + }, + ]); + const store = createDocumentStore(initialAst); + expect(warn).not.toHaveBeenCalled(); + + const updatedAst = makeAst([ + { + id: 'form1', + type: 'form', + sensitive: false, + disabled: false, + visible: true, + fields: [], + }, + { + id: 'streamed-gate', + type: 'approval-gate', + title: 'Approve', + sensitive: false, + disabled: false, + visible: true, + allowedRoles: ['admin'], + requiredApprovers: 1, + requireReason: false, + }, + ]); + store.updateAst(updatedAst); + + expect(warn).toHaveBeenCalledTimes(1); + const message = warn.mock.calls[0][0] as string; + expect(message).toContain('streamed-gate'); + expect(message).toContain('allowedRoles'); + warn.mockRestore(); + }); +}); diff --git a/packages/spec/src/schemas/components/approval-gate.ts b/packages/spec/src/schemas/components/approval-gate.ts index dd27994..1b1c154 100644 --- a/packages/spec/src/schemas/components/approval-gate.ts +++ b/packages/spec/src/schemas/components/approval-gate.ts @@ -5,11 +5,32 @@ export const ApprovalGateComponentSchema = ComponentBaseSchema.extend({ type: z.literal('approval-gate'), title: z.string().min(1), description: z.string().optional(), - requiredApprovers: z.number().int().positive().default(1), - allowedRoles: z.array(z.string()).optional(), + requiredApprovers: z + .number() + .int() + .positive() + .default(1) + .describe( + 'Advisory only — not enforced by the MDMA runtime, regardless of value (including the ' + + 'default of 1). The host application is responsible for tracking distinct approvers. ' + + 'See docs/reference/component-catalog.md#approval-gate for the security model.', + ), + allowedRoles: z + .array(z.string()) + .optional() + .describe( + 'Advisory list of approver roles surfaced to the host UI — not enforced by the MDMA ' + + 'runtime. The host application is responsible for verifying actor identity and role membership.', + ), onApprove: z.string().optional().describe('Action ID dispatched on approval'), onDeny: z.string().optional().describe('Action ID dispatched on denial'), - requireReason: z.boolean().default(false).describe('Require reason on denial'), + requireReason: z + .boolean() + .default(false) + .describe( + 'Advisory only — surfaces a reason input on denial; the MDMA runtime does not block ' + + 'dispatch when a reason is omitted. Enforcement is the host application’s responsibility.', + ), }); export type ApprovalGateComponent = z.infer; From 498bb1457b3bfd7e2385927d7f9d6267c9a65488 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20Zi=C3=B3=C5=82ek?= Date: Wed, 27 May 2026 21:24:23 +0200 Subject: [PATCH 2/2] fix(runtime): use template literal in approval-gate advisory warning Biome lint/style/useTemplate flagged the multi-line string concatenation; collapse to a single template literal to satisfy CI. --- packages/runtime/src/core/document-store.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/runtime/src/core/document-store.ts b/packages/runtime/src/core/document-store.ts index 96d477b..7d2219c 100644 --- a/packages/runtime/src/core/document-store.ts +++ b/packages/runtime/src/core/document-store.ts @@ -378,13 +378,7 @@ function warnIfAdvisoryApprovalFields(comp: MdmaBlock['component']): void { if (hasRequireReason) fields.push('requireReason'); console.warn( - `[mdma] approval-gate "${comp.id}" declares ${fields.join(', ')}, ` + - 'but ALL approval-gate role/quorum/reason fields are advisory at the runtime layer. ' + - 'The MDMA runtime does not verify approver identity, enforce role membership, ' + - 'track distinct approvers (regardless of requiredApprovers value, including 1), ' + - 'or block dispatch when a reason is omitted. ' + - 'The host application is responsible for authentication and authorization. ' + - 'See docs/reference/component-catalog.md#approval-gate.', + `[mdma] approval-gate "${comp.id}" declares ${fields.join(', ')}, but ALL approval-gate role/quorum/reason fields are advisory at the runtime layer. The MDMA runtime does not verify approver identity, enforce role membership, track distinct approvers (regardless of requiredApprovers value, including 1), or block dispatch when a reason is omitted. The host application is responsible for authentication and authorization. See docs/reference/component-catalog.md#approval-gate.`, ); }