Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .changeset/rest-method-not-allowed-names-failing-conjunct.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
"@objectstack/rest": patch
---

`OBJECT_API_METHOD_NOT_ALLOWED` now names the conjunct that actually failed, instead of one its own `allowed` array lists.

An object declaring `apiMethods: ['get','list','update','bulk']` refused `deleteMany`, `createMany` and each op of a cross-object `POST /batch` with an identical body:

```json
{ "error": "API operation 'bulk' is not allowed on object 'sys_user'",
"code": "OBJECT_API_METHOD_NOT_ALLOWED",
"allowed": ["get","list","update","bulk","aggregate","history","search","import","export"] }
```

Every one of those refusals was correct in outcome — `deleteMany` is `bulk ∧ delete`, `createMany` is `bulk ∧ create`, and `updateMany` / `batch`, which need only `bulk`, are still admitted — but the message named the half that PASSED, and the same envelope listed it as allowed. The writeMode-refined `import` had the identical shape: `import` derives from create ∨ update, so `update` alone puts `import` in the effective set while an `insert` import still needs `create`.

The message now names a conjunct that is genuinely missing: `delete`, `create` or `update` for the cases above, and still `bulk` when the `bulk` primitive itself is what the object withholds. Three requests that previously produced one indistinguishable envelope are now told apart.

**`allowed` is unchanged, in contents and in meaning** — it is still the object's declared effective operation set, not the set the gate evaluated against. That matters because the array is read as a discriminator: a declaration re-widened to create/update can still 405 for an unrelated reason, so only the set proves which gate answered. Nothing about which requests are admitted or refused moved; the HTTP status, the `code` and the `object` field are all as before. A client matching on the `error` string for these bulk and import refusals sees the new name.
8 changes: 4 additions & 4 deletions content/docs/permissions/system-context.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ not on any flag.
## How the flag is set

`isSystem` is **server-constructed and never client-supplied**. Inbound HTTP
cannot set it (`packages/rest/src/rest-server.ts:1788`, `:1817`), and neither
cannot set it (`packages/rest/src/rest-server.ts:1858`, `:1887`), and neither
can an action body (`packages/runtime/src/domains/actions.ts:414`). It is
written by internal callers only, as an option on the engine call:

Expand Down Expand Up @@ -103,7 +103,7 @@ that silently does not happen.
| 14 | MCP stdio bridge skips the object API-exposure gate | mcp | Get: the bridge reaches objects whose `apiEnabled` / `apiMethods` would refuse an external caller | `stdio-data-bridge.ts:250` |
| 15 | **Read-audit rows are not written** | plugin-audit | Lose: the "a person opened this record" trail. `sudo()` keeps the caller's `userId`, so this flag is the only thing separating a human read from a platform one | `read-audit.ts:556` |
| 16 | Approval snapshot payload redaction skipped | plugin-approvals | Get: the whole snapshot on `find` / `findOne` — the audit/replay channel. Lose: field-visibility redaction over approval payloads | `payload-redaction-middleware.ts:115` |
| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1820` |
| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1890` |

### 2. Write pipeline and data integrity

Expand Down Expand Up @@ -158,7 +158,7 @@ The largest single consumer — **17 of the 105 sites**.
|:--|:---|:---|:---|:---|
| 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes | `action-execution.ts:138` |
| 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:401` |
| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:5450`, `:6907`, `:7155`, `:7586`, `:7779` |
| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:5520`, `:6977`, `:7225`, `:7656`, `:7849` |
| 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` |
| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:421`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:552`, `external-datasource-routes.ts:302`, `package-routes.ts:97` |
| 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` |
Expand Down Expand Up @@ -199,7 +199,7 @@ assuming `isSystem` covers it is a documented source of bugs.
| "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1590` (#3493 / #6640) |
| "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` |
| "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:299` |
| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1788`, `:1817`; `domains/actions.ts:414` |
| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1858`, `:1887`; `domains/actions.ts:414` |

---

Expand Down
131 changes: 129 additions & 2 deletions packages/mcp/src/stdio-data-bridge.exposure.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,21 @@
*/

import { describe, it, expect, vi } from 'vitest';
import { DATA_ACTION_TO_API_OPERATION } from '@objectstack/spec/data';
import {
API_PRIMITIVES,
DATA_ACTION_TO_API_OPERATION,
effectiveOperationsArray,
isApiOperationAllowed,
resolveEffectiveApiMethods,
} from '@objectstack/spec/data';
import type { ExecutionContext } from '@objectstack/spec/kernel';
import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts';
import { createStdioDataBridge, GATED_ACTIONS, type McpExposureError } from './stdio-data-bridge.js';
import {
createStdioDataBridge,
enforceApiExposure,
GATED_ACTIONS,
type McpExposureError,
} from './stdio-data-bridge.js';
import { assertEngineFindOnePredicate, type EngineFindOneQueryInput } from '@objectstack/metadata-core';

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -367,3 +378,119 @@ describe('#8083 the gate runs before the existence probe', () => {
},
);
});

// ---------------------------------------------------------------------------
// #15416 — the refusal must not name an operation its own set contains
// ---------------------------------------------------------------------------

/**
* #15416 was filed from the REST door, where `deleteMany` (`bulk ∧ delete`) was
* refused with `API operation 'bulk' is not allowed` beside an `allowed` array
* containing `bulk` — the message named the conjunct that PASSED. The template
* it used lives in exactly two places, and this file guards the second one.
*
* ⭐ Measured, not assumed: stdio CANNOT currently reach that class, and the
* reason is structural rather than lucky. `enforceApiExposure` passes no
* `OperationCheckOptions` — no `bulkChild`, no `writeMode` — and without them
* `isApiOperationAllowed` reduces to membership in the very set that becomes
* `allowedOperations`. Message and set are then two reads of one set and cannot
* disagree. {@link GATED_ACTIONS} also contains neither `bulk` nor `import`,
* the only two operations the spec judges as a conjunction.
*
* So the REST repair was NOT copied here: there is nothing on this surface for
* it to repair. What is added instead is the pin that keeps that true — adding
* `bulk` or a writeMode-refined `import` to the gated set, or threading options
* into the check, reddens the sweep below instead of quietly re-opening #15416
* from the stdio door.
*/
describe('#15416 the stdio refusal never names an operation it also allows', () => {
/** Every subset of the six primitives — the whole declaration space. */
const EVERY_WHITELIST: string[][] = Array.from(
{ length: 1 << API_PRIMITIVES.length },
(_, mask) => API_PRIMITIVES.filter((_p, i) => mask & (1 << i)),
);

function metadataFor(apiMethods: string[]): IMetadataService {
return { getObject: async () => ({ name: 'task', enable: { apiMethods } }) } as unknown as IMetadataService;
}

it('holds for every gated action across every whitelist', async () => {
let refusals = 0;
for (const apiMethods of EVERY_WHITELIST) {
for (const action of Object.values(GATED_ACTIONS)) {
let err: McpExposureError | undefined;
try {
await enforceApiExposure(metadataFor(apiMethods), 'task', action, {} as ExecutionContext);
} catch (e) {
err = e as McpExposureError;
}
if (!err) continue;
expect(err.status).toBe(405);
refusals += 1;
const named = /^API operation '([^']*)' is not allowed on object '([^']*)'$/.exec(err.message)?.[1];
expect(named, `unparseable refusal for ${action} on [${apiMethods}]`).toBeTruthy();
expect(
err.allowedOperations,
`[${apiMethods}] refused ${action} by naming "${named}", which it also lists as allowed`,
).not.toContain(named);
}
}
// Vacuously-green guard: a matrix that admitted everything would satisfy
// the assertion above without measuring anything.
expect(refusals).toBeGreaterThan(100);
});

it('gates only single-conjunct operations, which is WHY the class is unreachable', () => {
// ⭐ Falsifiable, and deliberately not "verdict === membership": with no
// options passed that identity holds for EVERY operation word, so asserting
// it would be a phantom check that no drift could ever break. The property
// that actually protects this surface is narrower — the gated set must
// contain no operation whose verdict MOVES when options are supplied — and
// it is DERIVED from the spec here rather than hand-copied, so a new
// conjunction-bearing verb is caught the day it is added.
const PROBES = [
{ bulkChild: 'create' }, { bulkChild: 'update' }, { bulkChild: 'delete' },
{ bulkChild: 'upsert' }, { writeMode: 'insert' }, { writeMode: 'update' },
{ writeMode: 'upsert' },
];
const conjunctionRefined = new Set<string>();
for (const apiMethods of EVERY_WHITELIST) {
const eff = resolveEffectiveApiMethods({ apiMethods });
for (const operation of Object.values(DATA_ACTION_TO_API_OPERATION)) {
const bare = isApiOperationAllowed(eff, operation);
for (const opts of PROBES) {
if (isApiOperationAllowed(eff, operation, opts) !== bare) conjunctionRefined.add(operation);
}
}
}
// The derivation has to have FOUND the shapes #15416 was filed about, or the
// assertion below is measuring an empty set.
expect([...conjunctionRefined].sort()).toEqual(['bulk', 'import']);

for (const action of Object.values(GATED_ACTIONS)) {
const operation = DATA_ACTION_TO_API_OPERATION[action] ?? action;
expect(
conjunctionRefined.has(operation),
`bridge gates on "${action}" → "${operation}", whose verdict moves under OperationCheckOptions; ` +
'this surface passes none, so its refusal can now name a conjunct its own allowed set contains',
).toBe(false);
}
});

it('the serialized set is the same one the verdict is read from', () => {
// The half that makes the paragraph above load-bearing rather than lucky:
// for the operations this surface DOES gate, the verdict and the array the
// refusal ships are two reads of one set.
for (const apiMethods of EVERY_WHITELIST) {
const eff = resolveEffectiveApiMethods({ apiMethods });
const serialized = effectiveOperationsArray(eff) as string[];
for (const action of Object.values(GATED_ACTIONS)) {
const operation = DATA_ACTION_TO_API_OPERATION[action] ?? action;
expect(
isApiOperationAllowed(eff, operation),
`[${apiMethods}] ${action} → ${operation}: verdict diverged from the serialized set`,
).toBe(serialized.includes(operation));
}
}
});
});
Loading
Loading