From 297d9907a5b02fc621fc823fea89abb0508f8fee Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 29 Oct 2026 18:26:12 +0000 Subject: [PATCH] fix(rest)!: security/explain refuses an explicit empty userId with 400 An empty userId names nobody. The route dropped it before the schema, and the engine's truthy test then explained the caller. The route now answers 400 VALIDATION_FAILED for it on both transports, and the engine sends any userId that is not omitted through the other-user arm. An omitted userId still explains the caller. Claude-Session: https://claude.ai/code/session_01L6bhgTYZasLoBBaTy45mgo Co-authored-by: Claude --- .changeset/27442-explain-empty-user-id.md | 20 ++++++++ content/docs/permissions/explain.mdx | 2 + .../src/security-plugin-explain-access.ts | 5 +- .../src/security-plugin.test.ts | 28 +++++++++++ .../test/explain-unknown-user.dogfood.test.ts | 21 ++++++++ packages/rest/src/security-routes.test.ts | 48 +++++++++++++++++++ packages/rest/src/security-routes.ts | 13 ++++- 7 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 .changeset/27442-explain-empty-user-id.md 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 } : {}),