diff --git a/.changeset/find-data-wire-context.md b/.changeset/find-data-wire-context.md new file mode 100644 index 0000000000..24de0cf032 --- /dev/null +++ b/.changeset/find-data-wire-context.md @@ -0,0 +1,46 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): findData must not take its execution context from the request (#3960) + +Came out of the #3946 sweep's leftover question — whether `expand`'s "advanced +usage" (a caller-supplied `Record` whose sub-ASTs each carry an +`object`) is a cross-object read channel. **It is not**, and that needs saying +because the answer is load-bearing: `expandRelatedRecords` takes its target from +the parent schema (the expand KEY must be a real `reference` field; the sub-AST's +`object` is never read), re-enters `engine.find` so the referenced object's RLS + +FLS both run, `$and`-merges a nested `where` instead of spreading it over the id +filter, and caps depth. No change needed there. + +What the investigation did turn up is one layer down. `findData` built its engine +options as `{ ...request.query }` and then assigned `context` from +`request.context` **conditionally**: + +- `request.query` is the caller's raw bag on every ingress — the REST + `POST /data/:object/query` route passes `req.body` straight in as `query`; +- `context` sits in the known-params set, so it was not swept into the + implicit-filter bucket either — it survived the spread untouched; +- so when no server context resolved, the caller's `context` *became* the + operation's execution context. + +Everything hangs off that value. plugin-security's middleware opens with +`if (opCtx.context?.isSystem) return next()` — the entire RLS / FLS / CRUD chain +skipped — and `__expandRead: true` collects the #2850 waiver on the object-level +CRUD gate. Neither is ever schema-stripped on the read path: +`ExecutionContextSchema.parse` runs only in `engine.createContext`, which reads +do not use. + +Route-level `enforceAuth` is what kept this unreachable: anonymous data requests +are refused unless a deployment sets `requireAuth: false`. That makes it a +fail-OPEN default rather than a live exploit — and not something the protocol +should delegate upward. `findData` now drops any inbound `context` +unconditionally before the assignment, so the execution context can only come +from `request.context`. + +Verified end-to-end at the protocol layer (a forged +`{ isSystem, userId, __expandRead }` reached `engine.find` verbatim before, is +dropped after). The anonymous HTTP reachability half is NOT verified — see #3960 +for exactly what was and was not reproduced. No caller regresses: the only +in-repo builder of these args (`rest/src/import-runner.ts` `findArgsBase`) passes +`context` at the top level, never inside `query`. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 69b191661c..8f06558247 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -2659,6 +2659,29 @@ export class ObjectStackProtocolImplementation implements // probed for query-shape validity (nor reach the driver as a table). this.assertObjectRegistered(request.object); const options: any = { ...request.query }; + // The execution context is SERVER-DERIVED and never caller input. + // + // `request.query` is the raw request bag on every ingress that reaches + // here — the REST `POST /data/:object/query` route hands `req.body` + // straight in as `query`. `context` is in the known-params set below, so + // it was not swept into the implicit-filter bucket either: a caller's + // `context` survived this spread and, because the assignment below is + // conditional, became the operation's execution context whenever no + // server context resolved (an anonymous request on a deployment that + // set `requireAuth: false`). + // + // What rides on it is total: plugin-security's middleware opens with + // `if (opCtx.context?.isSystem) return next()` — the entire RLS / FLS / + // CRUD chain skipped — and `__expandRead` waives the object-level CRUD + // gate for public objects (#2850). Neither is ever schema-stripped on + // this path: `ExecutionContextSchema.parse` runs only in + // `engine.createContext`, which the read path does not use. + // + // Route-level `enforceAuth` is what kept that from being reachable, so + // this was a fail-OPEN default one layer down. Drop any inbound + // `context` unconditionally: the protocol must not depend on a gate + // above it staying switched on. + delete options.context; // Forward the dispatcher's ExecutionContext so RBAC/RLS middleware // can apply per-request enforcement. The protocol layer is purely // a normalizer — it must never strip security context. diff --git a/packages/metadata-protocol/src/protocol.wire-context.test.ts b/packages/metadata-protocol/src/protocol.wire-context.test.ts new file mode 100644 index 0000000000..143766f676 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.wire-context.test.ts @@ -0,0 +1,93 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// The execution context must never come off the wire. +// +// `findData` built its engine options as `{ ...request.query }` and then +// overwrote `context` from `request.context` ONLY when that was defined. The +// query bag is caller-controlled on every ingress that reaches here (the REST +// `POST /data/:object/query` route hands `req.body` straight in as `query`), and +// `context` is in the schema's known-params set, so it was NOT swept into the +// implicit-filter bucket — it survived into `engine.find` as the operation's +// execution context whenever no server context resolved. +// +// What rides on that context is total: plugin-security's middleware opens with +// `if (opCtx.context?.isSystem) return next()` — the whole RLS/FLS/CRUD chain +// skipped — and `__expandRead` waives the object-level CRUD gate for public +// objects. Neither is ever schema-stripped on the read path +// (`ExecutionContextSchema.parse` runs only in `createContext`). +// +// The auth gate is what stands between that and a live exploit: `enforceAuth` +// refuses anonymous data requests unless a deployment sets `requireAuth: false`. +// That makes this a fail-OPEN default one layer down — the protocol should not +// depend on a route-level gate staying on. These tests pin the invariant at the +// layer that owns it. + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { name: 'invoice', fields: { title: { name: 'title', type: 'text' } } }; + +function makeProtocol() { + const find = vi.fn(async () => []); + const count = vi.fn(async () => 0); + const engine = { + registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) }, + find, + count, + }; + return { p: new ObjectStackProtocolImplementation(engine as any), find }; +} + +/** The engine options `findData` handed to `engine.find`. */ +const optionsFrom = (find: any) => find.mock.calls[0][1]; + +describe('findData never takes its execution context from the caller', () => { + it('drops a body-supplied context when no server context resolved (the anonymous shape)', async () => { + const { p, find } = makeProtocol(); + await p.findData({ + object: 'invoice', + query: { context: { isSystem: true, userId: 'root', __expandRead: true } }, + } as any); + + expect(find).toHaveBeenCalledTimes(1); + expect(optionsFrom(find).context).toBeUndefined(); + }); + + it('the resolved context wins and is not merged with the caller\'s', async () => { + const { p, find } = makeProtocol(); + const resolved = { userId: 'u1', positions: [] }; + await p.findData({ + object: 'invoice', + query: { context: { isSystem: true } }, + context: resolved, + } as any); + + expect(optionsFrom(find).context).toBe(resolved); + expect((optionsFrom(find).context as any).isSystem).toBeUndefined(); + }); + + it('a caller `context` does not become an implicit field filter either', async () => { + // It must be dropped, not folded into `where` as `where.context` — that + // would turn a forged principal into a query against a column that does + // not exist (a confusing 400 instead of a clean ignore). + const { p, find } = makeProtocol(); + await p.findData({ object: 'invoice', query: { context: { isSystem: true } } } as any); + + const opts = optionsFrom(find); + expect(opts.where).toBeUndefined(); + expect(opts.context).toBeUndefined(); + }); + + it('still forwards a real query alongside a forged context', async () => { + const { p, find } = makeProtocol(); + await p.findData({ + object: 'invoice', + query: { where: { title: 'x' }, limit: 5, context: { isSystem: true } }, + } as any); + + const opts = optionsFrom(find); + expect(opts.where).toEqual({ title: 'x' }); + expect(opts.limit).toBe(5); + expect(opts.context).toBeUndefined(); + }); +});