diff --git a/.changeset/27442-explain-empty-user-id.md b/.changeset/27442-explain-empty-user-id.md new file mode 100644 index 00000000000..e1ac184609f --- /dev/null +++ b/.changeset/27442-explain-empty-user-id.md @@ -0,0 +1,20 @@ +--- +'@objectstack/rest': patch +'@objectstack/plugin-security': patch +--- + +fix(rest)!: `security/explain` refuses an explicit empty `userId` with 400 instead of explaining the caller (#27442) + +Clause-②: no (narrowing) — `GET`/`POST /api/v1/security/explain` refuses a request whose `userId` is the empty string, which it answered before. Nothing is widened. +Metadata-protocol: none + + + +**BREAKING** accept-set narrowing at the access-explanation door (`@objectstack/rest`), held by the engine too (`@objectstack/plugin-security`). + +- **Before.** `POST /api/v1/security/explain` with `"userId": ""` (or `GET …?userId=`) dropped the empty id and answered `200` with an explanation of the CALLER. A client that lost the selected user (an empty picker) got a confident answer about a principal nobody asked about. +- **Now.** The door answers `400 VALIDATION_FAILED`, in the same envelope as its other request refusals, and no explanation is computed. On `GET`, `?userId=` names the key, so it counts as explicit and is refused the same way. +- **Unchanged.** An omitted `userId` still explains the caller; a `userId` naming another user is explained, refused `403`, or refused `404 RECORD_NOT_FOUND` exactly as before. +- **Engine.** `explainAccessForCaller` no longer reads an empty `userId` as "the caller": it goes through the other-user arm, so a direct caller gets the `403` (no right to explain others) or the `404 RECORD_NOT_FOUND` an unknown id gets. + +**The remedy.** Omit `userId` to explain your own access, or send the id of the user to explain. diff --git a/content/docs/permissions/explain.mdx b/content/docs/permissions/explain.mdx index 0891f53efdd..b34b2c84ebd 100644 --- a/content/docs/permissions/explain.mdx +++ b/content/docs/permissions/explain.mdx @@ -71,6 +71,8 @@ curl -X POST -H "Authorization: Bearer $TOKEN" -H 'Content-Type: application/jso `operation` is one of `read | create | update | delete | transfer | restore | purge | export` (defaults to `read`); omit `userId` to explain **yourself**. +An empty `userId` (`"userId": ""`, or `?userId=` with no value) names nobody, so +it is refused `400 VALIDATION_FAILED` rather than read as "yourself". `export` is the one operation whose answer you cannot infer from `read`. Since the [export axis](/docs/permissions/permission-sets#allowexport--the-export-axis) diff --git a/packages/plugins/plugin-security/src/security-plugin-explain-access.ts b/packages/plugins/plugin-security/src/security-plugin-explain-access.ts index 5f067ae283f..72a499a8319 100644 --- a/packages/plugins/plugin-security/src/security-plugin-explain-access.ts +++ b/packages/plugins/plugin-security/src/security-plugin-explain-access.ts @@ -133,7 +133,10 @@ export async function explainAccessForCaller( const recordId = request?.recordId != null && request.recordId !== '' ? String(request.recordId) : undefined; let targetContext = callerContext ?? {}; - if (request.userId && request.userId !== callerContext?.userId) { + // [#27442] Any `userId` that is not omitted, the empty string included, goes + // through the other-user arm: an empty id names nobody, so it is refused + // there like an unknown one, never folded into explaining the caller. + if (request.userId !== undefined && request.userId !== callerContext?.userId) { const callerIsSystem = callerContext?.isSystem === true; if (!callerIsSystem) { const callerSets = await this.resolvePermissionSetsForContext(callerContext).catch(() => []); diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 11703d08b1b..a87ee9c85dc 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -3891,6 +3891,34 @@ describe('explainAccessForCaller (ADR-0090 D6/D12)', () => { }); }); + // [#27442] An explicit empty `userId` names nobody, so it is never read as + // "explain me": it goes through the other-user arm, where it is refused like + // any id no `sys_user` row has. The REST door refuses it 400 before this; the + // engine holds the same line for a caller that reaches it directly. + describe('[#27442] an explicit empty userId', () => { + it('is never answered as the caller: manage_users gets 404, a plain member 403', async () => { + const { plugin } = await boot(); + const hr = { userId: 'u_hr', positions: [], permissions: ['hr_admin'] }; + const admin: any = await plugin + .explainAccessForCaller({ object: 'task', operation: 'read', userId: '' }, hr) + .then((d) => d, (e) => e); + expect(admin, 'refused, not an explanation of the caller').toMatchObject({ + name: 'ExplainUserNotFoundError', code: 'RECORD_NOT_FOUND', status: 404, userId: '', + }); + const plain = { userId: 'u_east_1', positions: [], permissions: [] }; + await expect( + plugin.explainAccessForCaller({ object: 'task', operation: 'read', userId: '' }, plain), + ).rejects.toMatchObject({ name: 'PermissionDeniedError', code: 'PERMISSION_DENIED' }); + }); + + it('an omitted userId still explains the caller', async () => { + const { plugin } = await boot(); + const plain = { userId: 'u_east_1', positions: [], permissions: [] }; + const self = await plugin.explainAccessForCaller({ object: 'task', operation: 'read' }, plain); + expect(self.principal.userId).toBe('u_east_1'); + }); + }); + // [#20515] Explaining ANOTHER user resolves that user's grants in the CALLER's // organization — the one the explain right was checked in. A grant scoped to // an organization applies only while that organization is the one resolved diff --git a/packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts b/packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts index 7ad7e34e193..edd707a8a5b 100644 --- a/packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts +++ b/packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts @@ -23,6 +23,9 @@ * `PERMISSION_DENIED` it gets naming a user who exists. * - `[unknown]` the seed admin naming the unknown id → 404 `RECORD_NOT_FOUND` * naming it, for `read` and `create`, consistent with the data door's 404. + * - `[empty]` (#27442) an explicit empty `userId`, in the body or as + * `?userId=`, is refused 400 `VALIDATION_FAILED`: it names nobody, so it is + * never answered as the caller. * - `[existing]` the seed admin explaining the member who exists is answered * as before (200, the principal is that member), and explaining yourself is * unchanged. @@ -108,6 +111,24 @@ describe('#26641: security explain refuses a userId no sys_user row has', () => } }); + // [#27442] The same door's remaining gap: an explicit empty `userId` names + // nobody, and was dropped before the schema, so it explained the CALLER. It + // is refused 400 on both transports; an omitted `userId` (below) is the caller. + it('[empty] an explicit empty userId is refused 400 VALIDATION_FAILED, never answered as the caller', async () => { + const posted = await explain(adminTok, { object: OBJECT, operation: 'read', userId: '' }); + const queried = await stack.apiAs(adminTok, 'GET', `/security/explain?object=${OBJECT}&operation=read&userId=`); + const queriedText = await queried.text(); + let queriedJson: any = null; + try { queriedJson = JSON.parse(queriedText); } catch { /* the status still reports */ } + for (const [label, r] of [['POST', posted], ['GET', { status: queried.status, text: queriedText, json: queriedJson }]] as const) { + expect(r.status, `${label}: ${r.text}`).toBe(400); + expect(r.json?.success, `${label}: ${r.text}`).toBe(false); + expect(r.json?.error?.code, `${label}: ${r.text}`).toBe('VALIDATION_FAILED'); + expect(r.json?.allowed, `${label}: no verdict is answered`).toBeUndefined(); + expect(r.json?.principal, `${label}: no principal is answered`).toBeUndefined(); + } + }); + it('[existing] the seed admin explaining the member who exists, and anyone explaining themselves, is answered', async () => { const other = await explain(adminTok, { object: OBJECT, operation: 'read', userId: memberId }); expect(other.status, other.text).toBe(200); diff --git a/packages/rest/src/security-routes.test.ts b/packages/rest/src/security-routes.test.ts index 73bed1d4c90..df2a89d0796 100644 --- a/packages/rest/src/security-routes.test.ts +++ b/packages/rest/src/security-routes.test.ts @@ -154,6 +154,54 @@ describe('GET/POST /security/explain (ADR-0090 D6)', () => { }); }); +// ── [#27442] an explicit empty userId is refused, an omitted one is the caller ─ + +describe('[#27442] GET/POST /security/explain with an empty userId', () => { + it('POST userId "" is refused 400 VALIDATION_FAILED and nothing is explained', async () => { + const explain = vi.fn().mockResolvedValue(DECISION); + const { post } = buildServer(async () => ({ explain }), { callerCtx: CALLER }); + const res = mockRes(); + await post!.handler({ method: 'POST', params: {}, headers: {}, body: { object: 'task', operation: 'read', userId: '' } } as any, res); + + expect(res.statusCode).toBe(400); + expect(res.body.success).toBe(false); + expect(res.body.error.code).toBe('VALIDATION_FAILED'); + expect(res.body.allowed, 'no verdict is answered').toBeUndefined(); + expect(explain).not.toHaveBeenCalled(); + }); + + it('GET ?userId= names the key too, so it is refused the same way', async () => { + const explain = vi.fn().mockResolvedValue(DECISION); + const { get } = buildServer(async () => ({ explain }), { callerCtx: CALLER }); + const res = mockRes(); + await get!.handler({ method: 'GET', params: {}, headers: {}, query: { object: 'task', userId: '' } } as any, res); + + expect(res.statusCode).toBe(400); + expect(res.body.error.code).toBe('VALIDATION_FAILED'); + expect(explain).not.toHaveBeenCalled(); + }); + + it('an omitted userId still explains the caller, and a named user is still passed through', async () => { + const explain = vi.fn().mockResolvedValue(DECISION); + const { get, post } = buildServer(async () => ({ explain }), { callerCtx: CALLER }); + + let res = mockRes(); + await post!.handler({ method: 'POST', params: {}, headers: {}, body: { object: 'task', operation: 'read' } } as any, res); + expect(res.statusCode).toBe(200); + expect(explain).toHaveBeenLastCalledWith({ object: 'task', operation: 'read' }, CALLER); + + res = mockRes(); + await get!.handler({ method: 'GET', params: {}, headers: {}, query: { object: 'task' } } as any, res); + expect(res.statusCode).toBe(200); + expect(explain).toHaveBeenLastCalledWith({ object: 'task', operation: 'read' }, CALLER); + + res = mockRes(); + await post!.handler({ method: 'POST', params: {}, headers: {}, body: { object: 'task', operation: 'read', userId: 'u_target' } } as any, res); + expect(res.statusCode).toBe(200); + expect(explain).toHaveBeenLastCalledWith({ object: 'task', operation: 'read', userId: 'u_target' }, CALLER); + }); +}); + // ── [#8326] batch form — recordIds on the same request shape ───────────────── /** diff --git a/packages/rest/src/security-routes.ts b/packages/rest/src/security-routes.ts index 78050aee2e8..dddcada6564 100644 --- a/packages/rest/src/security-routes.ts +++ b/packages/rest/src/security-routes.ts @@ -204,10 +204,21 @@ export function registerSecurityExplainEndpoints(this: SecurityExplainRouteHost, const rawRecordIds = req.method === 'GET' && typeof src.recordIds === 'string' && src.recordIds !== '' ? [src.recordIds] : src.recordIds; + // [#27442] An omitted `userId` means the caller; an explicit empty + // one names nobody, so it is refused rather than dropped (dropping + // it answered about the caller, a principal nobody asked about). + // `?userId=` names the key on GET, so it is explicit there too. + // The schema's `z.string()` admits "", so the refusal is here. + if (src.userId === '') { + return respondError( + res, 400, 'VALIDATION_FAILED', + 'Invalid explain request — userId is empty. Omit userId to explain your own access, or give the id of the user to explain.', + ); + } const parsed = (ExplainRequestSchema as any).safeParse({ object: src.object, operation: src.operation ?? 'read', - ...(src.userId != null && src.userId !== '' ? { userId: src.userId } : {}), + ...(src.userId != null ? { userId: src.userId } : {}), // [C2 / ADR-0095] Optional record id — explains ONE concrete // row at record granularity; omitted stays object-level. ...(src.recordId != null && src.recordId !== '' ? { recordId: src.recordId } : {}),