Skip to content

Commit 92493e3

Browse files
fix(rest): security/explain refuses an empty object with 400 VALIDATION_FAILED instead of 500 (#27892)
<!-- claude-projects-attribution: {"github_login":"os-elon-musk"} --> _Requested by **Elon** · [project thread](https://claude.ai/code/project/chan_01WedDzV26nniwxv7jC88B9D?thread=cmsg_01WedDzV26nniwxv7jC88B9D2T3MYD69CaE3sEdpqrhNgW)_ Fixes #27719 Clause-②: no Metadata-protocol: none ## 维护者速读 - **改了什么**:管理员调用访问解释接口(`POST`/`GET /api/v1/security/explain`)时,`object` 传空字符串现在直接回 400 `VALIDATION_FAILED`,不再回 500 `EXPLAIN_FAILED`。 - **为什么改**:这是调用方能自己改正的错误请求,却被当成服务器故障回给客户端、也按故障记日志;与上一张卡(空 `userId` 回 400)同一道门、同一种处理。 - **风险与回滚**:风险低——本来就失败的请求只是从 500 变成 400,合法请求一字节不变;回滚即 revert 本 PR。 - **先看**:`packages/rest/src/security-routes.ts`(`EMPTY_KEY_REMEDIES` 表与 handler 里那一个循环)、`packages/rest/src/security-routes.test.ts`(`[#27719]` 一组)。 - **两分钟验证**:`pnpm --filter @objectstack/rest exec vitest run --project local src/security-routes.test.ts`,预期 22 个用例全部通过。 - **需要你做的**:无,席位落地。 ## What changes `ExplainRequestSchema.object` is `z.string()`, so an empty `object` passed the schema, reached the explain engine, and the engine's plain `Error('[Security] explain: request.object is required')` fell to the route's catch-all `500 EXPLAIN_FAILED` arm. The route now refuses it `400 VALIDATION_FAILED`, in its own envelope and before any evaluation, through the check that already refused an empty `userId`. That check is now one table, `EMPTY_KEY_REMEDIES` (module-private, no export changes), with one row per key and the remedy its refusal names. The handler loops over it once, so the `userId` refusal is not copied. The `userId` message is byte-identical to before. Per the triage ruling, the refusal is graded at the route. `packages/spec` is not touched. Moving both keys into `ExplainRequestSchema` (`min(1)`) is a separate spec narrowing that this PR does not grade. Files: `packages/rest/src/security-routes.ts`, `packages/rest/src/security-routes.test.ts`, `.changeset/27719-explain-empty-object.md` (`@objectstack/rest` patch). ## Tests New `[#27719]` block in `packages/rest/src/security-routes.test.ts`. Its fake service throws the engine's own plain error for an empty object: - `POST` with `object: ""` gets 400, `success: false`, `error.code` `VALIDATION_FAILED`, and no verdict. The engine is never called. - `GET ?object=` is refused the same way. - A request that names an object is still explained, and the engine gets exactly the request it was sent (`POST` with `update`, and `GET` defaulting to `read`). The `[#27442]` empty-`userId` block runs unchanged and passes. Run on head `4f5150d8` (`objectstack-ai/objectstack`): <details><summary>Red first, on the unfixed route: 2 failed | 20 passed</summary> ``` pnpm --filter @objectstack/rest exec vitest run --project local --maxWorkers=2 src/security-routes.test.ts × POST object "" is refused 400 VALIDATION_FAILED in the route envelope, and the engine is never called × GET ?object= is refused the same way AssertionError: expected 500 to be 400 // Object.is equality Tests 2 failed | 20 passed (22) ``` </details> <details><summary>Green after the fix: 22/22; the whole package 347 files / 6737 tests; typecheck exit 0</summary> ``` src/security-routes.test.ts Tests 22 passed (22) pnpm --filter @objectstack/rest test Test Files 347 passed (347) · Tests 6737 passed | 332 skipped (7069) · VERDICT command-exit 0 pnpm --filter @objectstack/rest typecheck VERDICT command-exit 0 (check:test-typecheck: 0 files / 0 errors) ``` </details> <details><summary>Ablation: delete the object row from EMPTY_KEY_REMEDIES, and both new refusal pins go red; restored byte-identical</summary> ``` ablation-replace: ok mutation landed: anchor 1 -> 0, blob 73f00e0 -> 88505ad43e41 × POST object "" is refused 400 VALIDATION_FAILED in the route envelope, and the engine is never called × GET ?object= is refused the same way Tests 2 failed | 20 passed (22) ablation-replace: ok restored: blob == HEAD (73f00e0) and `git diff HEAD` is empty ``` The route test imports `./rest-server` relatively, so it runs against `src/` and no dist rebuild was needed for this leg. </details> <details><summary>End to end: real showcase boot as the seed admin, through a throwaway dogfood probe that was not committed</summary> ``` main 9ae6384: POST object="" operation=read status=500 {"success":false,"error":{"code":"EXPLAIN_FAILED","message":"[Security] explain: request.object is required"}} GET ?object=&operation=read status=500 (same body) this branch 4f5150d (rest rebuilt): POST object="" operation=read status=400 {"success":false,"error":{"code":"VALIDATION_FAILED","message":"Invalid explain request — object is empty. Give the name of the object whose access to explain."}} GET ?object=&operation=read status=400 (same body) controls, unchanged on both: POST object=showcase_task -> 200 allowed:true · POST userId="" -> 400 VALIDATION_FAILED (the userId message is byte-identical) ``` </details> ## Acceptance notes - The empty `object` refusal names its remedy ("Give the name of the object whose access to explain."). The test does not pin that wording: it asserts the code, the status, and that the engine was never called. - A missing `object` (the key absent) is still refused by the request schema's own 400. This PR leaves it alone. ## Verification Local subset on head `4f5150d8`: the gates derived by `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` from 3 changed paths (+93 / -10), run one after another with each exit code captured before any pipe. <details><summary>60 derived, 60 run, 60 exit 0; --ran reconciliation: 0 NOT-MEASURED, 0 UNRUN</summary> - `pnpm check:dual-build-cjs-loads` first exited 3 (PREREQUISITE NOT MET: 8 unrelated packages had no `dist/`). After building them it exited 0: 113 entry points across 70 packages load. The reconciliation records that rerun. - `--ran` with `:: exit N` per line: "60 derived famil(ies) accounted for — 60 run, 0 NOT-MEASURED (a DERIVED zero — all 60 recorded an exit code and none of them is 3)". - `npx eslint --no-inline-config` on both touched source files: exit 0. - Not run locally, and left to CI: `test:repo`, the full-tree `pnpm lint`, and the type-check lanes and path-scheduled jobs the derivation lists outside its total. No dogfood battery: the title has no `!` and the clause has no `(narrowing)` arm. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016WV4d1Np3hU7RQnA13YZcL Co-authored-by: Claude <noreply@anthropic.com>
1 parent 88d6020 commit 92493e3

3 files changed

Lines changed: 93 additions & 10 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/rest': patch
3+
---
4+
5+
fix(rest): `security/explain` refuses an empty `object` with `400 VALIDATION_FAILED` instead of a `500 EXPLAIN_FAILED` server fault (#27719)
6+
7+
Clause-②: no — the request already failed; only its answer moves from `500` to `400`.
8+
Metadata-protocol: none
9+
10+
- **Before.** `POST /api/v1/security/explain` with `"object": ""` (or `GET …?object=`) passed the request schema, reached the explain engine, and the engine's plain error was answered `500 EXPLAIN_FAILED` and logged as a server fault.
11+
- **Now.** The door answers `400 VALIDATION_FAILED` in its own envelope, naming the empty key and its remedy, before any evaluation — the same check, in the same place, that refuses an empty `userId`.
12+
- **Unchanged.** A request naming an object is explained exactly as before; an empty `userId` is refused `400 VALIDATION_FAILED` as before; a missing `object` is still refused by the request schema.
13+
14+
**The remedy.** Send the name of the object whose access to explain.

‎packages/rest/src/security-routes.test.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,57 @@ describe('[#27442] GET/POST /security/explain with an empty userId', () => {
202202
});
203203
});
204204

205+
// ── [#27719] an empty object is refused at the door, like an empty userId ────
206+
207+
describe('[#27719] GET/POST /security/explain with an empty object', () => {
208+
/** The engine's own answer to an empty object: a plain Error, which the route's catch-all reads as a 500. */
209+
const engineLike = () => vi.fn(async (request: any) => {
210+
if (!request.object) throw new Error('[Security] explain: request.object is required');
211+
return DECISION;
212+
});
213+
214+
it('POST object "" is refused 400 VALIDATION_FAILED in the route envelope, and the engine is never called', async () => {
215+
const explain = engineLike();
216+
const { post } = buildServer(async () => ({ explain }), { callerCtx: CALLER });
217+
const res = mockRes();
218+
await post!.handler({ method: 'POST', params: {}, headers: {}, body: { object: '', operation: 'read' } } as any, res);
219+
220+
expect(res.statusCode).toBe(400);
221+
expect(res.body.success).toBe(false);
222+
expect(res.body.error.code).toBe('VALIDATION_FAILED');
223+
expect(res.body.allowed, 'no verdict is answered').toBeUndefined();
224+
expect(explain).not.toHaveBeenCalled();
225+
});
226+
227+
it('GET ?object= is refused the same way', async () => {
228+
const explain = engineLike();
229+
const { get } = buildServer(async () => ({ explain }), { callerCtx: CALLER });
230+
const res = mockRes();
231+
await get!.handler({ method: 'GET', params: {}, headers: {}, query: { object: '', operation: 'read' } } as any, res);
232+
233+
expect(res.statusCode).toBe(400);
234+
expect(res.body.success).toBe(false);
235+
expect(res.body.error.code).toBe('VALIDATION_FAILED');
236+
expect(explain).not.toHaveBeenCalled();
237+
});
238+
239+
it('a named object is still explained with exactly the request it was sent', async () => {
240+
const explain = engineLike();
241+
const { get, post } = buildServer(async () => ({ explain }), { callerCtx: CALLER });
242+
243+
let res = mockRes();
244+
await post!.handler({ method: 'POST', params: {}, headers: {}, body: { object: 'task', operation: 'update' } } as any, res);
245+
expect(res.statusCode).toBe(200);
246+
expect(res.body).toEqual(DECISION);
247+
expect(explain).toHaveBeenLastCalledWith({ object: 'task', operation: 'update' }, CALLER);
248+
249+
res = mockRes();
250+
await get!.handler({ method: 'GET', params: {}, headers: {}, query: { object: 'task' } } as any, res);
251+
expect(res.statusCode).toBe(200);
252+
expect(explain).toHaveBeenLastCalledWith({ object: 'task', operation: 'read' }, CALLER);
253+
});
254+
});
255+
205256
// ── [#8326] batch form — recordIds on the same request shape ─────────────────
206257

207258
/**

‎packages/rest/src/security-routes.ts‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,17 @@ export interface SecurityRouteHost {
100100
enforceAuth: RestServer['enforceAuth'];
101101
}
102102

103+
/**
104+
* The explain request keys whose explicit empty string the door refuses
105+
* `400 VALIDATION_FAILED` before any evaluation, each with the remedy its
106+
* refusal names. The schema's `z.string()` admits "", so the line is drawn
107+
* here; see the check in the handler for why each key is on it.
108+
*/
109+
const EMPTY_KEY_REMEDIES: ReadonlyArray<readonly ['object' | 'userId', string]> = [
110+
['object', 'Give the name of the object whose access to explain.'],
111+
['userId', 'Omit userId to explain your own access, or give the id of the user to explain.'],
112+
];
113+
103114
/**
104115
* Register the access-explanation endpoints —
105116
* `RestServer#registerSecurityExplainEndpoints`' body, called by that method
@@ -204,16 +215,23 @@ export function registerSecurityExplainEndpoints(this: SecurityExplainRouteHost,
204215
const rawRecordIds = req.method === 'GET' && typeof src.recordIds === 'string' && src.recordIds !== ''
205216
? [src.recordIds]
206217
: src.recordIds;
207-
// [#27442] An omitted `userId` means the caller; an explicit empty
208-
// one names nobody, so it is refused rather than dropped (dropping
209-
// it answered about the caller, a principal nobody asked about).
210-
// `?userId=` names the key on GET, so it is explicit there too.
211-
// The schema's `z.string()` admits "", so the refusal is here.
212-
if (src.userId === '') {
213-
return respondError(
214-
res, 400, 'VALIDATION_FAILED',
215-
'Invalid explain request — userId is empty. Omit userId to explain your own access, or give the id of the user to explain.',
216-
);
218+
// An explicit empty string names nothing, so these keys refuse it
219+
// here: the schema's `z.string()` admits "", and `?key=` names the
220+
// key on GET, so it is explicit there too. One check, one remedy
221+
// per key, in the route's own envelope, before any evaluation.
222+
// - [#27442] `userId`: an omitted one means the caller; an empty
223+
// one was dropped and answered about the caller, a principal
224+
// nobody asked about.
225+
// - [#27719] `object`: an empty one reached the engine, whose
226+
// plain `Error` fell to the `500 EXPLAIN_FAILED` arm below — a
227+
// request the caller can fix, answered and logged as a fault.
228+
for (const [key, remedy] of EMPTY_KEY_REMEDIES) {
229+
if (src[key] === '') {
230+
return respondError(
231+
res, 400, 'VALIDATION_FAILED',
232+
`Invalid explain request — ${key} is empty. ${remedy}`,
233+
);
234+
}
217235
}
218236
const parsed = (ExplainRequestSchema as any).safeParse({
219237
object: src.object,

0 commit comments

Comments
 (0)