diff --git a/.changeset/meta-bind-unregistered-kinds.md b/.changeset/meta-bind-unregistered-kinds.md new file mode 100644 index 0000000000..b6fb9d0a14 --- /dev/null +++ b/.changeset/meta-bind-unregistered-kinds.md @@ -0,0 +1,59 @@ +--- +"@objectstack/spec": minor +--- + +fix(spec): `webhook` / `connector` / `sharing_rule` are validated at the `/meta` write door (#6245) + +Three stack collections could be written through `PUT /api/v1/meta//:name` +with **zero validation**. A spec-invalid webhook body was stored verbatim and +answered `success: true` — the repo pinned that behaviour itself +(`protocol-meta.test.ts`, which saved `{ name, url, events: ['x.created'] }` and +asserted success, where `events` is an alias of `triggers` and `'x.created'` is +not a `WebhookTriggerType`; that webhook subscribed to nothing). + +This is the hole #5271 closed for `api`, arriving through three more doors: +the kinds are produced and consumed today — artifact ingest maps +`defineStack({ webhooks, connectors, sharingRules })` onto items of exactly +these type names — but none is a member of `MetadataTypeSchema`, so +`getMetadataTypeSchema()` returned `undefined`, `resolveOverlaySchema()` +returned `null`, and `saveMetaItem` took its documented "unregistered type → +store without validation" branch. Enforced but undeclared. + +**FROM** `PUT /meta/webhook/my_hook` with any JSON → `200 { success: true }`, +stored unvalidated. +**TO** a malformed body → `422 INVALID_METADATA` with structured `issues[]`, +the same envelope every other kind already returned. A well-formed body is +accepted exactly as before. + +Each type binds the **same schema its stack collection is validated against**, +so no body can be legal in a stack and illegal through `/meta` or the reverse: +`WebhookSchema`, `DeclarativeConnectorEntrySchema`, `SharingRuleSchema`. +`connector` binds the *declarative entry* schema rather than the bare +`ConnectorSchema` deliberately — the entry schema carries the ADR-0097 §3/§5 +rules (a provider-bound instance may not inline credentials via +`authentication`, nor author `actions`/`triggers`), and binding the base would +have left the inline-secret shape a stack refuses reachable through `/meta`, +which is this very bug class wearing a different key. + +**No new capability surface.** These are bound for shape validation only: no +`MetadataTypeSchema` member, no `DEFAULT_METADATA_TYPE_REGISTRY` entry, so +every authorization verdict keeps taking the identical "no static entry ⇒ +synthesised `allowRuntimeCreate: true`" branch. The write *door* is unchanged; +only the 422 is new. #2657's B/C decision on whether these should become kinds +is untouched and unprejudged. + +Graded **minor**, following #5271: a write that previously returned 200 can now +return 422. Nothing well-formed changes behaviour, but a caller relying on the +API accepting malformed bodies will see the difference. + +**One schema change rides along, and it is load-bearing.** +`CriteriaSharingRule` / `SharingRule` now declare the ADR-0010 protection +envelope (`_lock`, `_lockReason`, `_lockSource`, `_lockDocsUrl`, `_packageId`, +`_packageVersion`, `_provenance`). Both metadata load paths call +`applyProtection` on **every** type, so a package-loaded sharing rule already +carries those keys — and this shape is `.strict()`, so it did not drop them, it +*rejected* them. That was invisible only while the type resolved no schema at +the overlay door. Binding the door without this spread would have aimed the new +422 at the runtime's own stamp instead of at malformed author input. The +existing guard in `metadata-type-schemas.test.ts` names this failure exactly and +prescribes this fix. Additive and internal-only — no authored field changes. diff --git a/content/docs/references/security/sharing.mdx b/content/docs/references/security/sharing.mdx index 5e8de7166b..aa91e11602 100644 --- a/content/docs/references/security/sharing.mdx +++ b/content/docs/references/security/sharing.mdx @@ -37,6 +37,13 @@ const result = CriteriaSharingRuleSchema.parse(data); | **active** | `boolean` | optional | | | **accessLevel** | `Enum<'read' \| 'edit'>` | optional | | | **sharedWith** | `{ type: Enum<'user' \| 'team' \| 'position' \| 'unit_and_subordinates' \| 'business_unit'>; value: string }` | ✅ | The recipient of the shared access | +| **_lock** | `Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>` | optional | Item-level lock — controls overlay & delete (ADR-0010). | +| **_lockReason** | `string` | optional | Human-readable reason shown when a write is refused by _lock. | +| **_lockSource** | `Enum<'artifact' \| 'package' \| 'env-forced'>` | optional | Layer that set _lock (artifact \| package \| env-forced). | +| **_provenance** | `Enum<'package' \| 'org' \| 'env-forced'>` | optional | Origin of the item (package \| org \| env-forced). | +| **_packageId** | `string` | optional | Owning package machine id. | +| **_packageVersion** | `string` | optional | Owning package version. | +| **_lockDocsUrl** | `string` | optional | Optional documentation link surfaced next to _lockReason. | | **type** | `'criteria'` | ✅ | | | **condition** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | ✅ | Predicate (CEL). e.g. P`record.department == "Sales"` | @@ -91,6 +98,13 @@ const result = CriteriaSharingRuleSchema.parse(data); | **active** | `boolean` | optional | | | **accessLevel** | `Enum<'read' \| 'edit'>` | optional | | | **sharedWith** | `{ type: Enum<'user' \| 'team' \| 'position' \| 'unit_and_subordinates' \| 'business_unit'>; value: string }` | ✅ | The recipient of the shared access | +| **_lock** | `Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>` | optional | Item-level lock — controls overlay & delete (ADR-0010). | +| **_lockReason** | `string` | optional | Human-readable reason shown when a write is refused by _lock. | +| **_lockSource** | `Enum<'artifact' \| 'package' \| 'env-forced'>` | optional | Layer that set _lock (artifact \| package \| env-forced). | +| **_provenance** | `Enum<'package' \| 'org' \| 'env-forced'>` | optional | Origin of the item (package \| org \| env-forced). | +| **_packageId** | `string` | optional | Owning package machine id. | +| **_packageVersion** | `string` | optional | Owning package version. | +| **_lockDocsUrl** | `string` | optional | Optional documentation link surfaced next to _lockReason. | | **type** | `'criteria'` | ✅ | | | **condition** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | ✅ | Predicate (CEL). e.g. P`record.department == "Sales"` | diff --git a/packages/objectql/src/protocol-meta.test.ts b/packages/objectql/src/protocol-meta.test.ts index 677064f29e..557f0cad88 100644 --- a/packages/objectql/src/protocol-meta.test.ts +++ b/packages/objectql/src/protocol-meta.test.ts @@ -1445,8 +1445,19 @@ describe('ObjectStackProtocolImplementation - Metadata Persistence', () => { // would have kept the assertion green through the *other* branch of // `isRuntimeCreateAllowed` (statically registered with // `allowRuntimeCreate: true`) while the test claimed to prove the - // "no static entry" fall-through. `theme` and `webhook` still exercise - // that branch; `api`'s own behaviour is pinned in the test below. + // "no static entry" fall-through. + // + // [#6245] `webhook` left it too, for the SAME reason and by the same + // rule — but note the difference from `api`: webhook/connector/ + // sharing_rule gained a SCHEMA, not a registry entry, so they still + // take this test's "no static entry" branch for AUTHORIZATION. What + // they no longer do is accept any JSON, which is what disqualifies the + // old specimen: its body (`events`, an alias of `triggers`, plus the + // non-enum trigger `'x.created'`) is spec-INVALID and now 422s. Kept + // here it would have flipped this test red for a reason that has + // nothing to do with what it proves. `theme` and `policy` still carry + // the branch — neither resolves a schema. Each newly-bound type's own + // behaviour, door and 422 both, is pinned in the tests below. // ─────────────────────────────────────────────────────────────── it('accepts brand-new plugin-registered type (no static registry entry)', async () => { @@ -1458,15 +1469,149 @@ describe('ObjectStackProtocolImplementation - Metadata Persistence', () => { item: { name: 'my_theme', label: 'Test', tokens: {} }, organizationId: 'org_alpha', }); - const webhookResult = await scoped.saveMetaItem({ + const policyResult = await scoped.saveMetaItem({ + type: 'policy', + name: 'my_policy', + item: { name: 'my_policy', label: 'Test' }, + organizationId: 'org_alpha', + }); + + expect(themeResult.success).toBe(true); + expect(policyResult.success).toBe(true); + }); + + // ─────────────────────────────────────────────────────────────── + // [#6245] `webhook` / `connector` / `sharing_rule` — the write door is + // UNCHANGED, the shape check is new. Same shape as the `api` pair + // below, and asserted the same way: a change that quietly CLOSED the + // door would fail the "accepts" half, which is why both halves are + // pinned per type rather than just the 422. + // + // These three resolve a schema WITHOUT a `DEFAULT_METADATA_TYPE_REGISTRY` + // entry, so `isRuntimeCreateAllowed` still takes its synthesised + // `allowRuntimeCreate: true` fall-through — the authorization verdict is + // byte-identical to before. + // ─────────────────────────────────────────────────────────────── + + it('accepts a spec-valid `webhook` item (write door unchanged by the schema binding)', async () => { + mockEngine.findOne.mockResolvedValue(null); + + const result = await scoped.saveMetaItem({ type: 'webhook', name: 'my_webhook', - item: { name: 'my_webhook', url: 'https://e.example/x', events: ['x.created'] }, + item: { + name: 'my_webhook', + object: 'case', + triggers: ['create'], + url: 'https://e.example/x', + }, organizationId: 'org_alpha', }); - expect(themeResult.success).toBe(true); - expect(webhookResult.success).toBe(true); + expect(result.success).toBe(true); + }); + + it('refuses a spec-INVALID `webhook` item with 422 instead of storing it unvalidated', async () => { + mockEngine.findOne.mockResolvedValue(null); + + // The exact body the "plugin-registered types" case above used to + // save with `success: true`: `events` is an ALIAS of `triggers` + // (so the strict surface names the real key), and `'x.created'` is + // not a `WebhookTriggerType`. Stored verbatim, this webhook + // subscribed to nothing. + await expect( + scoped.saveMetaItem({ + type: 'webhook', + name: 'my_webhook', + item: { name: 'my_webhook', url: 'https://e.example/x', events: ['x.created'] }, + organizationId: 'org_alpha', + }), + ).rejects.toMatchObject({ + code: 'INVALID_METADATA', + status: 422, + }); + }); + + it('accepts a spec-valid `connector` item (write door unchanged by the schema binding)', async () => { + mockEngine.findOne.mockResolvedValue(null); + + const result = await scoped.saveMetaItem({ + type: 'connector', + name: 'my_connector', + item: { name: 'my_connector', label: 'My Connector', type: 'api' }, + organizationId: 'org_alpha', + }); + + expect(result.success).toBe(true); + }); + + it('refuses a `connector` that breaks an ADR-0097 authoring rule with 422', async () => { + mockEngine.findOne.mockResolvedValue(null); + + // Bound to `DeclarativeConnectorEntrySchema` — the same schema + // `stack.zod.ts` validates `connectors:` with — so the ADR-0097 §3 + // rule that a provider-bound instance may not inline credentials + // holds at BOTH authoring doors, not just the stack one. + await expect( + scoped.saveMetaItem({ + type: 'connector', + name: 'my_connector', + item: { + name: 'my_connector', + label: 'My Connector', + type: 'api', + provider: 'github', + authentication: { type: 'basic', username: 'u', password: 'p' }, + }, + organizationId: 'org_alpha', + }), + ).rejects.toMatchObject({ + code: 'INVALID_METADATA', + status: 422, + }); + }); + + it('accepts a spec-valid `sharing_rule` item (write door unchanged by the schema binding)', async () => { + mockEngine.findOne.mockResolvedValue(null); + + const result = await scoped.saveMetaItem({ + type: 'sharing_rule', + name: 'sales_records', + item: { + name: 'sales_records', + type: 'criteria', + object: 'case', + condition: "record.department == 'Sales'", + sharedWith: { type: 'team', value: 'sales' }, + }, + organizationId: 'org_alpha', + }); + + expect(result.success).toBe(true); + }); + + it('refuses a spec-INVALID `sharing_rule` item with 422 instead of storing it unvalidated', async () => { + mockEngine.findOne.mockResolvedValue(null); + + // No `condition` and no `object`: a criteria rule that names + // neither what it shares nor which records. Stored unvalidated this + // is the #3896 shape — a rule that binds to nothing, or worse + // compiles to the empty filter. + await expect( + scoped.saveMetaItem({ + type: 'sharing_rule', + name: 'sales_records', + item: { + name: 'sales_records', + type: 'criteria', + sharedWith: { type: 'team', value: 'sales' }, + }, + organizationId: 'org_alpha', + }), + ).rejects.toMatchObject({ + code: 'INVALID_METADATA', + status: 422, + }); }); // ─────────────────────────────────────────────────────────────── diff --git a/packages/spec/authorable-surface/security.json b/packages/spec/authorable-surface/security.json index 42b4088b9d..25a10d4882 100644 --- a/packages/spec/authorable-surface/security.json +++ b/packages/spec/authorable-surface/security.json @@ -26,6 +26,13 @@ "security/CapabilityDeclaration:name", "security/CapabilityDeclaration:packageId", "security/CapabilityDeclaration:scope", + "security/CriteriaSharingRule:_lock", + "security/CriteriaSharingRule:_lockDocsUrl", + "security/CriteriaSharingRule:_lockReason", + "security/CriteriaSharingRule:_lockSource", + "security/CriteriaSharingRule:_packageId", + "security/CriteriaSharingRule:_packageVersion", + "security/CriteriaSharingRule:_provenance", "security/CriteriaSharingRule:accessLevel", "security/CriteriaSharingRule:active", "security/CriteriaSharingRule:condition", @@ -133,6 +140,13 @@ "security/RowLevelSecurityPolicy:priority [RETIRED]", "security/RowLevelSecurityPolicy:tags", "security/RowLevelSecurityPolicy:using", + "security/SharingRule:_lock", + "security/SharingRule:_lockDocsUrl", + "security/SharingRule:_lockReason", + "security/SharingRule:_lockSource", + "security/SharingRule:_packageId", + "security/SharingRule:_packageVersion", + "security/SharingRule:_provenance", "security/SharingRule:accessLevel", "security/SharingRule:active", "security/SharingRule:condition", diff --git a/packages/spec/src/kernel/metadata-type-schemas.ts b/packages/spec/src/kernel/metadata-type-schemas.ts index 04abd6e6eb..73c067b0c3 100644 --- a/packages/spec/src/kernel/metadata-type-schemas.ts +++ b/packages/spec/src/kernel/metadata-type-schemas.ts @@ -45,6 +45,11 @@ import { ReportSchema } from '../ui/report.zod'; import { DatasetSchema } from '../ui/dataset.zod'; import { FlowSchema } from '../automation/flow.zod'; +import { WebhookSchema } from '../automation/webhook.zod'; + +import { DeclarativeConnectorEntrySchema } from '../integration/connector.zod'; + +import { SharingRuleSchema } from '../security/sharing.zod'; import { ApiEndpointSchema } from '../api/endpoint.zod'; @@ -142,6 +147,67 @@ const BUILTIN_METADATA_TYPE_SCHEMAS: Partial> = skill: SkillSchema, }; +/** + * Schema bindings for stack collections that are **not** metadata KINDS. + * + * [#6245, the same shape #5271 closed for `api`] `webhook`, `connector` and + * `sharing_rule` are produced and consumed today — artifact ingest maps + * `defineStack({ webhooks, connectors, sharingRules })` onto items of exactly + * these three type names (`ARTIFACT_FIELD_TO_TYPE`) — yet none of them is a + * member of `MetadataTypeSchema`. `getMetadataTypeSchema()` therefore answered + * `undefined`, `resolveOverlaySchema()` answered `null`, and `saveMetaItem` + * took its documented "unregistered type → store without validation" branch: + * `PUT /api/v1/meta/webhook/:name` accepted ANY JSON and returned + * `success: true`. Enforced but undeclared — the mirror of declared-but- + * unenforced, and the same hole, three more doors. + * + * This map is separate from `BUILTIN_METADATA_TYPE_SCHEMAS` above because that + * one is keyed by `MetadataType`, and these three are deliberately NOT that. + * Binding a schema here is a SHAPE check and nothing else: + * - no `MetadataTypeSchema` enum member; + * - no `DEFAULT_METADATA_TYPE_REGISTRY` entry — so every authorization + * verdict (`isRuntimeCreateAllowed` and friends) keeps taking the very same + * "no static entry ⇒ synthesised `allowRuntimeCreate: true`" branch it takes + * today. The write DOOR is byte-identical; only the 422 for a malformed + * body is new; + * - no new capability surface, no permission change. + * #2657's B/C decision on whether these should become kinds is untouched and + * unprejudged — this is the enforce-what-we-already-accept half only. + * + * Each entry binds the SAME schema its stack collection is validated against in + * `stack.zod.ts`, so no body can be legal in a stack and illegal through `/meta` + * or the reverse. + */ +const UNREGISTERED_KIND_SCHEMAS: Record = { + // `stack.zod.ts`: `webhooks: z.array(WebhookSchema)` + webhook: WebhookSchema, + + // `stack.zod.ts`: `connectors: z.array(DeclarativeConnectorEntrySchema)`. + // + // Bound to the DECLARATIVE ENTRY schema, not the bare `ConnectorSchema`. The + // base is a plain object so connector subtypes can `.extend()` it; the entry + // schema adds the ADR-0097 cross-field rules that apply to an AUTHORED + // connector — a provider-bound instance may not inline credentials via + // `authentication` (§3), nor author `actions`/`triggers` the provider derives + // (§5). `PUT /meta/connector/:name` is an authoring door in exactly the sense + // `connectors:` is, and those rules are the ones a second door must not + // reopen: binding the base here would leave the inline-secret shape ADR-0097 + // forbids in a stack reachable through `/meta`, which is this very bug class + // wearing a different key. The rules fire only for provider-bound instances, + // so catalog descriptors are unaffected. + connector: DeclarativeConnectorEntrySchema as unknown as z.ZodType, + + // `stack.zod.ts`: `sharingRules: z.array(SharingRuleSchema)`. + // + // This shape is `.strict()` with no stored/stamped envelope, so it is an + // AUTHOR-shape check and is applied only where author shapes are submitted: + // the write door. It is not a filter on rows already in `sys_sharing_rule`. + // Nothing re-parses stored rows through this registry — the read path only + // DECORATES a document with `_diagnostics` (`computeMetadataDiagnostics`) and + // never refuses one. Same treatment #5271/#5312 gave their strict surfaces. + sharing_rule: SharingRuleSchema, +}; + /** Runtime-extensible overlay populated via `registerMetadataTypeSchema`. */ const EXTRA_METADATA_TYPE_SCHEMAS = new Map(); @@ -153,7 +219,11 @@ const EXTRA_METADATA_TYPE_SCHEMAS = new Map(); * which is runtime-created by the datasource Sync wizard — ADR-0062/0088). */ export function getMetadataTypeSchema(type: string): z.ZodType | undefined { - return EXTRA_METADATA_TYPE_SCHEMAS.get(type) ?? BUILTIN_METADATA_TYPE_SCHEMAS[type as MetadataType]; + return EXTRA_METADATA_TYPE_SCHEMAS.get(type) + ?? BUILTIN_METADATA_TYPE_SCHEMAS[type as MetadataType] + // #6245 — last, so a plugin's `registerMetadataTypeSchema()` override still + // wins over a built-in binding here, exactly as it does over the map above. + ?? UNREGISTERED_KIND_SCHEMAS[type]; } /** @@ -190,6 +260,16 @@ export function listMetadataTypeSchemaTypes(): string[] { for (const t of EXTRA_METADATA_TYPE_SCHEMAS.keys()) types.add(t); return Array.from(types).sort(); } +// [#6245] `UNREGISTERED_KIND_SCHEMAS` is deliberately NOT enumerated here. +// This list is the REGISTERED METADATA TYPE set, and the repo treats it as a +// contract surface, not a convenience index: `metadata-type-schemas.test.ts` +// walks it to enforce the #4001 unknown-key closure campaign and the ADR-0010 +// protection-envelope declaration on every member, `metadata-create-seeds` +// requires a create seed per member, and the campaign count is asserted against +// it. Those are obligations of being a KIND. webhook/connector/sharing_rule are +// bound above for SHAPE VALIDATION precisely because they are not kinds, so +// enrolling them here would claim a status this change is careful not to grant +// (and #2657's B/C decision is exactly the one left open). // ========================================== // Metadata Type Actions (type-level buttons) diff --git a/packages/spec/src/security/sharing.zod.ts b/packages/spec/src/security/sharing.zod.ts index d9def338a9..16ef907868 100644 --- a/packages/spec/src/security/sharing.zod.ts +++ b/packages/spec/src/security/sharing.zod.ts @@ -9,6 +9,7 @@ import { strictUnknownKeyError } from '../shared/suggestions.zod'; * The baseline security posture for an object. */ import { lazySchema } from '../shared/lazy-schema'; +import { MetadataProtectionFields } from '../kernel/metadata-protection.zod'; export const OWDModel = z.enum([ 'private', // Only owner can see 'public_read', // Everyone can see, owner can edit @@ -169,6 +170,22 @@ const BaseSharingRuleSchema = z.object({ type: ShareRecipientType, value: z.string().describe('ID or code of the recipient (user / team / position / business unit)'), }, { error: sharingRecipientUnknownKeyError }).strict().describe('The recipient of the shared access'), + + // ADR-0010 — runtime protection envelope (internal — set by loader). + // + // [#6245] Declared for the reason `webhook.zod.ts` states for its own spread: + // BOTH metadata load paths call `applyProtection` on EVERY type, so a + // package-loaded sharing rule already carries these keys by the time anything + // re-parses it. This shape is `.strict()`, so until now that stamped envelope + // was not merely dropped — it was REJECTED, which stayed invisible only + // because the type resolved no schema at the overlay door and nothing parsed + // it there. #6245 binds that door, so declaring the envelope is what keeps + // the new 422 aimed at malformed AUTHOR input instead of at the runtime's own + // stamp. `metadata-type-schemas.test.ts` names this failure exactly ("is + // strict and does not declare the ADR-0010 envelope, so `applyProtection` + // output fails to parse — a hard 422 on the overlay path") and prescribes + // this spread as the fix. + ...MetadataProtectionFields, }, { error: sharingRuleUnknownKeyError }).strict(); /**