Skip to content

fix(rest)!: security/explain refuses an explicit empty userId with 400 instead of explaining the caller - #27714

Merged
objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/project-thread-sbdkgo
Oct 29, 2026
Merged

objectstack-fleet[bot] merged 2 commits into
mainfrom
claude/project-thread-sbdkgo

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

domain:server#2 · os-sales · session_01L6bhgUCizoYWQVJWjhdR6F

Requested by Sales · project thread

Fixes #27442

Clause-②: no (narrowing)
Metadata-protocol: none
Skills-docs: updated here — content/docs/permissions/explain.mdx now says an empty userId is refused 400 VALIDATION_FAILED, beside "omit userId to explain yourself". No file under skills/** teaches an empty userId: skills/objectstack-data/rules/security.md names the query parameters as a list (?object=&operation=&userId=) and says nothing about an empty value, so no skill line changes.

维护者速读

  • 改了什么:权限解释接口(GET/POST /api/v1/security/explain)收到显式的空 userId("userId": "" 或 ?userId=)时,改为回 400 VALIDATION_FAILED,不再悄悄解释调用者本人;不传 userId 仍解释调用者本人。
  • 为什么改:路由在校验前把空串当成「没传」丢掉,引擎又按真值判断,于是「解释某个空用户」答成了「解释我自己」,管理员会误以为看到的是别人的权限。
  • 风险与回滚:只收窄这一个请求值;不传 userId、传真实用户 id、传不存在的 id 三种行为与 main 一致。引擎侧同步改为 userId !== undefined 才算「解释自己」,直接调用引擎的代码传空串会得到 403 或 404。回滚即还原 security-routes.ts 与 security-plugin-explain-access.ts 各一处。
  • 先看:packages/rest/src/security-routes.ts 新增的 src.userId === '' 拒绝分支;packages/plugins/plugin-security/src/security-plugin-explain-access.ts 的 request.userId !== undefined 判断。
  • 两分钟验证:pnpm --filter @objectstack/rest exec vitest run src/security-routes.test.ts -t 27442,预期 3/3 通过。
  • 需要你做的:无,席位落地(收窄卡,进队前席位排契约复核)。

What changes

  • The door (@objectstack/rest). GET/POST {basePath}/security/explain answers 400 VALIDATION_FAILED when userId is the empty string, through the family's one refusal emitter (the same ADR-0112 envelope and code its schema refusals answer), before any explanation is computed. Before, the route dropped an explicit "" along with an omitted value, so the request fell through to a self-explanation of the caller.
  • GET counts as explicit. ?userId= names the key, and the query parser hands the route "" (measured on the showcase boot), so it is refused the same way. Only a request that does not name userId at all, or sends JSON null, is "explain me", as before.
  • The engine (@objectstack/plugin-security). explainAccessForCaller enters the other-user arm when request.userId !== undefined (was a truthy test). An empty id therefore meets the existing gates there: a caller without the right to explain others gets the same 403 PERMISSION_DENIED, and a manage_users caller gets the 404 RECORD_NOT_FOUND an unknown id gets. A direct caller of the engine can no longer reach the self-explain fold with "". Every in-tree caller of security.explain omits userId (measured: rest-server.ts, runtime/domains/automation.ts, service-storage x2, plugin-webhooks, plugin-sharing, plugin-audit), so none of them moves.
  • No new code, no spec edit. VALIDATION_FAILED is an existing ledgered code, and ExplainRequestSchema is untouched (its z.string().optional() admits "", which is why the refusal sits at the route).
  • Docs. content/docs/permissions/explain.mdx states the refusal next to "omit userId to explain yourself".

Tests

  • packages/rest/src/security-routes.test.ts — new block [#27442]: POST userId: "" → 400, success: false, error.code VALIDATION_FAILED, no allowed, explain never called; GET ?userId= → the same, explain never called; omitted userId (POST and GET) still calls explain without one, and a real other id is still passed through.
  • packages/plugins/plugin-security/src/security-plugin.test.ts — new block [#27442]: a manage_users caller asking userId: "" gets ExplainUserNotFoundError (RECORD_NOT_FOUND, 404) instead of an explanation of itself; a plain member gets PermissionDeniedError (403); an omitted userId still explains the caller.
  • packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts — new case [empty] on the showcase boot (the security explain answers 200 allowed:true for a userId that matches no sys_user row, built from the synthesized baseline principal #26641 sibling, same boot, no new one): the seed admin's POST userId: "" and GET ?userId= both answer 400 VALIDATION_FAILED with no verdict and no principal. The file's existing [existing] case keeps pinning that an omitted userId explains the caller and a real other id is answered.

Acceptance notes

  • Red first, through the door: before the fix the two route cases answered 200 (expected 200 to be 400) and the engine case answered an explanation of the caller.
  • Ablations (each restored, blob == HEAD, git diff HEAD empty):
    • Route guard disabled (if (src.userId === '') replaced by a never-true marker, rebuilt, ablation-dist-preflight proved the marker in packages/rest/dist/index.{js,cjs}): the unit file goes 2 failed | 17 passed (expected 200 to be 400) and the dogfood file 1 failed | 5 passed, answering 404 RECORD_NOT_FOUND naming user '' — the engine's own line holding once the door's is gone. Rebuilt after restore; preflight --absent clean.
    • Engine check reverted to the truthy request.userId &&: the engine block goes 1 failed (expected { allowed: true, … } to be the refusal).
  • Order of checks at the door is unchanged: anonymous 401 first, then 501 when no explain service, then this 400 with the schema refusals.
  • null in a POST body keeps reading as omitted (unchanged); the ruling refuses only an explicit empty string.
  • Branch is the harness-pinned claude/project-thread-sbdkgo (claim 6504756487 names it); card-thread session session_01L6bhgTYZasLoBBaTy45mgo.

Verification

All readings on head 821c2ced (the fix 297d9907 merged with origin/main 86f2afeb), in a worktree of objectstack-ai/objectstack, under scripts/pm/os-verify-lock.sh.

Unit suites and typecheck
  • pnpm --filter @objectstack/rest exec vitest run: Test Files 344 passed (344), Tests 6945 passed | 344 skipped (7289).
  • pnpm --filter @objectstack/plugin-security exec vitest run: Test Files 237 passed (237), Tests 5119 passed | 45 skipped (5164).
  • pnpm --filter @objectstack/rest typecheck and pnpm --filter @objectstack/plugin-security typecheck (including check:test-typecheck, 0 errors): exit 0.
  • Red before the fix: security-routes.test.ts 2 failed | 17 passed (expected 200 to be 400); the engine block 1 failed (an explanation of the caller where the refusal was expected).
Door (dogfood) and the full battery
  • pnpm --filter @objectstack/dogfood exec vitest run test/explain-unknown-user.dogfood.test.ts: Tests 6 passed (6).
  • Full battery, pnpm --filter @objectstack/dogfood test after the closure build (turbo run build --filter='@objectstack/dogfood^...'), no shard, no path: Test Files 352 passed | 3 skipped (355), Tests 2845 passed | 20 skipped (2865), lock VERDICT command-exit 0.
Gates
  • node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack derived 95 families; each was run and its exit code captured before any pipe; --ran reconciliation: 95 derived, 95 run, 0 NOT-MEASURED, 0 UNRUN.
  • Four first readings were prerequisites (exit 3, or the generated-tree refusal), not verdicts, and were rerun green after meeting them: check-plugin-teardown-shape --self-test (pinned fixture commit fetched, 48 cases pass), spec check:docs and check:skill-examples (after gen:schema), check:dual-build-cjs-loads (after building the seven packages it listed with no dist/).
  • pnpm lint (repo-wide eslint . --no-inline-config): exit 0.
  • No packages/spec, docs/adr/**, skills/** or .github/** path is touched. The claim measured no open PR editing either source file.

Generated by Claude Code

🤖 Generated with Claude Code

https://claude.ai/code/session_01L6bhgUCizoYWQVJWjhdR6F

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 <noreply@anthropic.com>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Oct 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/plugin-security, @objectstack/rest, touching 2 documentable anchor(s).

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/permissions/system-context.mdx (via explainAccessForCaller (symbol, a top-level function))
What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: VALIDATION_FAILED (literal, 37 pages)
  • the SDK route bridge reached 58 of 210 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 31 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json c5d51f0303718c40d2cc7410adb060c483aba5ab → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 9b2a6035b1007e8305dc0181bf1bca32f81bfc4c — the merge of head 821c2ced1dd2b12b9f4babbbf56f1aca7ee5a6aa into base c5d51f0303718c40d2cc7410adb060c483aba5ab, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 9b2a6035b1007e8305dc0181bf1bca32f81bfc4c && git checkout 9b2a6035b1007e8305dc0181bf1bca32f81bfc4c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin c5d51f0303718c40d2cc7410adb060c483aba5ab 821c2ced1dd2b12b9f4babbbf56f1aca7ee5a6aa && git checkout -B drift-repro c5d51f0303718c40d2cc7410adb060c483aba5ab && git merge --no-ff 821c2ced1dd2b12b9f4babbbf56f1aca7ee5a6aa

node scripts/docs-audit/affected-docs.mjs --json c5d51f0303718c40d2cc7410adb060c483aba5ab

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs c5d51f0303718c40d2cc7410adb060c483aba5ab → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#2 · os-sales · session_01L6bhgUCizoYWQVJWjhdR6F

Contract-review-claim: session_01L6bhgUCizoYWQVJWjhdR6F · head 821c2ce · 2026-10-29T20:20Z

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#2 · os-sales · session_01L6bhgUCizoYWQVJWjhdR6F

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 821c2ced1dd2b12b9f4babbbf56f1aca7ee5a6aa
Local-runs: none
Scope: packages/rest/src/security-routes.ts, packages/plugins/plugin-security/src/security-plugin-explain-access.ts, packages/rest/src/security-routes.test.ts, packages/plugins/plugin-security/src/security-plugin.test.ts, packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts + packages/spec/src/security/explain.zod.ts, packages/plugins/plugin-security/src/explain-engine.ts (readSysUserRow, buildContextForUser), packages/plugins/plugin-security/src/delegated-admin-gate.ts (organizationAdminCoversUser, scopesCoverUser, businessUnitsOfUser), packages/plugins/plugin-security/src/errors.ts, packages/plugins/plugin-security/src/security-plugin-security-service.ts, packages/plugins/plugin-security/src/security-plugin.ts, packages/types/src/response-envelope.ts, packages/spec/src/api/error-code-ledger.zod.ts, packages/rest/src/error-response.ts, packages/plugins/plugin-hono-server/src/adapter.ts (readQuery), packages/client/src/index.ts (security.explain), ui/packages/app-shell/src/views/metadata-admin/AccessExplainPanel.tsx, ui/packages/plugin-detail/src/queueClaim.ts, ui/packages/plugin-detail/src/useRecordEditable.ts, ui/packages/plugin-grid/src/hooks/useRecordCrudVerdicts.ts, packages/rest/src/rest-server.ts, packages/runtime/src/domains/automation.ts, packages/services/service-storage/src/attachment-access-hooks.ts, packages/services/service-storage/src/file-attachment-service.ts, packages/plugins/plugin-webhooks/src/redeliver-entitlement.ts, packages/plugins/plugin-audit/src/comment-access-hooks.ts, packages/plugins/plugin-sharing/src/sharing-service.ts, skills/objectstack-data/rules/security.md, scripts/pm/clause2-line.mjs; the two prose faces (.changeset/27442-explain-empty-user-id.md, content/docs/permissions/explain.mdx) are judged in ② and ④.

Inputs: card #27442 (body; comments 6491470512, 6504756487, 6507415474), PR #27714 body and file list, the net diff origin/main...pr27714 (merge base 86f2afeb), the check-runs on the head, and the cited code read at the head. Read-only: nothing built, run or re-run.

① Derived judgments

Every accept-set and public-surface change the diff implies, each judged:

  1. REST door, both transports: userId equal to the empty string answers 400 VALIDATION_FAILED before the schema and before any evaluation. Right. It is the triage's ruling (6491470512: refuse an empty userId with 400; an omitted one keeps its meaning). The guard sits after the 401 and 501 arms and before ExplainRequestSchema.safeParse, so svc.explain is never reached — pinned (explain not called) in the route test.
  2. GET ?userId= counts as explicit. Right. The Hono adapter's readQuery builds req.query from c.req.queries() and unwraps a single value, so a key present with an empty value reaches the handler as '' — the key was named on the wire. A bare ?userId (no =) is the same '' under URLSearchParams semantics. Pinned on the real showcase boot by the dogfood [empty] case.
  3. JSON null keeps reading as omitted (the caller). Right, and unchanged: src.userId != null is the read the route made before (the diff only drops the !== '' half), and it is the same != null read recordId and recordIds make two lines down. The triage refuses only an explicit empty string; the spec schema admits no null and the route shielded it from null before this PR exactly as after. null is the JSON spelling of "no value"; "" is a string value that names nobody. Consistent with the triage, the sibling reads and the spec. Nit: the tree says this only in the PR body — the route comment speaks of "omitted" without naming null.
  4. Engine (explainAccessForCaller, the security service's explain): any userId that is not undefined and is not the caller's own enters the other-user arm. Right, and declared: the changeset's "Engine" bullet and the @objectstack/plugin-security patch entry carry it. A direct in-process caller passing "" now meets the arm's existing gates — 403 PERMISSION_DENIED without the right, 404 RECORD_NOT_FOUND (ExplainUserNotFoundError) with it — instead of an explanation of itself. An off-type direct null moves the same way (null !== undefined; readSysUserRow finds no row for it); it folded to self before. Safe direction; no in-tree caller passes either.
  5. No in-tree caller moves. Measured at the head: rest-server.ts (create probe), runtime/domains/automation.ts, service-storage x2, plugin-webhooks, plugin-audit, plugin-sharing all call security.explain without userId; the client SDK forwards the body verbatim (JSON.stringify(request), so an unset optional is absent, not empty); the Console's AccessExplainPanel spreads userId only when a user is picked (...(user ? { userId: user.id } : {})), and the three other Console callers (queueClaim, useRecordEditable, useRecordCrudVerdicts) send object/operation/recordId(s) only. The card's "empty picker" is therefore a foreign-client shape today, which is the right thing for a public door to refuse.
  6. Unchanged behaviours pinned: omitted userId explains the caller on both transports (route test, plugin test, dogfood [existing]); a real other id is explained, or refused 403/404, as before.
  7. Order of refusals unchanged: 401, then 501, then this 400, then the schema's 400. Cosmetic only: a body with both an empty userId and a missing object gets the userId message — both are 400 VALIDATION_FAILED.

② Semver level

  • Changeset packages match what the diff moves: @objectstack/rest (the route) and @objectstack/plugin-security (the engine), both patch. @objectstack/dogfood is private: true and content/docs publishes nothing, so neither is owed an entry.
  • Declarations: Clause-②: no (narrowing) on the PR body and in the changeset (the changeset's trailing reason after the arm is the accepted shape — clause2-line.mjs reads the value and the arm and leaves the rest). Metadata-protocol: none on both — right: no authorable key, value or type moves. The rule gives a non-protocol breaking change patch with its BREAKING banner, which is what the changeset carries; the title carries the !.
  • ADR-0087 disposition: not-required (no-migration-prescription), one marker. The body carries no Migration heading, no rewrite table and no framed arrow — the remedy is one prose sentence (omit userId, or send a real id) — so the category's one mechanical refusal does not fire, and the category is the honest one: nothing authored or stored exists for a migration to rewrite.
  • Check-runs on the head, read at 2026-10-29T20:34Z: success — Auto Label, Check Changeset, Check Documentation Links, Check PR Size, Dogfood Regression Gate, Flag docs affected by code changes, Governed Surface Queue Guard, Lint · ESLint, Lint · PM dispatch-gates, Lint · scoped · tooling self-tests, PR-body guards, Plan Test Core shards, Spec property liveness, Test UI, Type Check · debt ledger, filter. In progress — Build Core, Build Docs, Lint · CI/release & engine gates, Lint · docs & code guards, Lint · repo & tooling gates, Lint · scoped · query-options erasure, Lint · scoped · slot lookup & corpus, Test Core 1/8 through 8/8, Type Check · consumer gates, Type Check · source gates, Type Check · workspace. Skipped — the UI lanes, the opt-in harnesses and the live-store conformance. No failure at my reading. The gate-carrying jobs still running are the landing's gate, not this record's; the ADR-0087 registration gate rides in one of them.

③ Boundary flags

  • open_questions in 6507415474: none — nothing to answer.
  • Dev flag, attribution: the PR body carries the session-URL footer and, under it, the harness's own attribution block. Acknowledged as the declared deviation; not a tree matter. No model identifier anywhere in the body, the changeset or the code; the fix commit 297d9907 ends with the model-free trailer pair and its Claude-Session names the card thread, as the claim does. Nothing to escalate.
  • Dev flag, skills/objectstack-data/rules/security.md line 55 (?object=&operation=&userId=): agree no skill line changes — skills/** is Tier H, and the line is a parameter list, not a taught empty value. One correction to the report's prose: by reading, that literal request never answered a self-explanation on main, because object: '' passes z.string(), reaches the engine's request.object is required throw and falls to the route's 500 EXPLAIN_FAILED arm; after this PR the userId guard answers 400 first. The conclusion stands (never a working request). The empty-object 500 is a sibling reading, recorded under ④.
  • Premise: holds at the merge base — the route read src.userId != null && src.userId !== '' and the engine tested request.userId for truth.
  • Governed surfaces: none in the file list; head repo is the base repo; 137 changed lines; Governed Surface Queue Guard success.

④ Design judgment

Per in-scope file, judged against the cited code at the head and the card's scenario.

  1. packages/rest/src/security-routes.ts — holds. The refusal goes through the family's one emitter (respondError → sendError → { success: false, error: { code, message } }, the ADR-0112 D5 envelope) with a ledgered code: VALIDATION_FAILED is a member of error-code-ledger.zod.ts and is the code the same handler's schema refusal answers, so no new dialect enters the family (VALIDATION_ERROR is what the query-param gates answer elsewhere; this family's request refusals were already VALIDATION_FAILED). The runtime string carries no tracker number. Break attempts: a bare ?userId → '' → refused; an array, number or boolean userId → != null → the schema's z.string() refuses it 400; a whitespace-only id → admitted by the route and the schema, refused 403/404 by the engine (no fold to the caller); null → the caller, as ruled. No path lets a present userId be answered as the caller. Observation, not a finding: the asymmetry with recordId — an empty recordId is still read as object-level — is sound, because an object-level answer is a coarser answer to the same question while the caller is a different principal. Observation, not a finding on this PR: the refusal lives at the route while ExplainRequestSchema.userId stays z.string().optional(); the route is that schema's only parser, so there is one door today, but the contract-first direction would pin the empty string in the spec, where a userId and an object minimum length belong together (see the sibling below). That is a follow-up card with its own spec-regen footprint, not a rider on this one: the ruling is met and the dispatch's file surface excluded packages/spec.
  2. packages/plugins/plugin-security/src/security-plugin-explain-access.ts — holds. readSysUserRow is findOne('sys_user', { where: { id } }) under the system context, so an empty id matches no row — measured by the dev on two stores (the plugin fixture, and the showcase boot in the route-guard ablation, where the engine answered 404 naming ''). Ahead of it the gates fail closed on an empty id: organizationAdminCoversUser returns false on a falsy targetUserId, scopesCoverUser needs held scopes and then businessUnitsOfUser('') reads nothing. The 403-before-404 order keeps security explain answers 200 allowed:true for a userId that matches no sys_user row, built from the synthesized baseline principal #26641's non-disclosure (a caller who may not explain others learns nothing about which ids exist). Nit: the new comment says any userId that is not omitted goes through the other-user arm — a userId equal to the caller's own still takes the self arm (!== callerContext?.userId), as before; the comment overstates, the code is right.
  3. packages/rest/src/security-routes.test.ts — holds. Both halves of the pin: refusal (status, success: false, nested error.code, no allowed, explain never called) on POST and GET, and preservation (omitted userId still calls explain without one on both transports; a named user is passed through). The GET case feeds the parsed shape directly; the real parser is pinned by the dogfood case.
  4. packages/plugins/plugin-security/src/security-plugin.test.ts — holds. The hr caller holds manage_users through the fixture's hr_admin set (systemPermissions: ['manage_users']), so its 404 RECORD_NOT_FOUND proves the other-user arm was entered and the existence read refused — a 403 would otherwise have answered; the plain member's 403 proves the gate; the omitted case pins the self arm. The resolved-or-rejected fold (.then(d, e)) still fails on a decision, since toMatchObject asks for the error's name and code.
  5. packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts — holds. [empty] rides the security explain answers 200 allowed:true for a userId that matches no sys_user row, built from the synthesized baseline principal #26641 boot (no new boot), pins POST and GET on the real server, and asserts the 400 body carries no verdict and no principal; [existing] keeps omitted = caller and a real other id answered.
  6. .changeset/27442-explain-empty-user-id.md — matches the diff. Both packages, the three behaviours (refused, unchanged, engine), the remedy; the Before sentence is the card's own text.
  7. content/docs/permissions/explain.mdx — matches the diff. The two added lines state exactly what the route does and sit beside the "omit userId" sentence; a hand-written tree, no new page, no meta.json change owed.

Sibling reading for the seat to file (outside this card, not in the diff, read only — not measured here): GET/POST security/explain with object present but empty passes ExplainRequestSchema (object: z.string()), reaches the engine's plain Error('[Security] explain: request.object is required'), matches neither the 403/404 arms nor classifiedRefusalAnswer (no code, no status), and falls to the route's 500 EXPLAIN_FAILED arm — a malformed request answered as a fault. The same spec pin that would carry userId non-empty carries this.

Findings: must-fix — none. should-fix — none. Nits — the route comment does not name null; the engine comment overstates "any userId that is not omitted". Observations — spec placement (follow-up card), the recordId asymmetry (sound), the empty-object 500 (sibling, to file).

Implemented-by: claude/project-thread-sbdkgo
Reviewed-by: session_01L6bhgUCizoYWQVJWjhdR6F

VERDICT: PASS

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#2 · os-sales · session_01L6bhgUCizoYWQVJWjhdR6F

维护者速读(席位终稿)

  • 改了什么:/api/v1/security/explain 收到显式的空 userId(JSON "" 或 GET ?userId=)时回 400 VALIDATION_FAILED,不再悄悄解释调用者本人;不带 userId(或 JSON null)仍解释调用者本人,和文档一致。
  • 为什么改:管理员以为自己在看别人的权限,其实看到的是自己的,会据此做错判断。
  • 风险与回滚:只拒显式空串;Console 未选用户时根本不带这个键(已核实),SDK 和服务内调用方都不受影响;插件直接调用传空串现在得 403/404 而不是解释调用者。回滚 = revert 本 PR。
  • 先看:packages/rest/src/security-routes.ts 的空 userId 判断,security-plugin-explain-access.ts 改成 !== undefined 的一行。
  • 两分钟验证:pnpm --filter @objectstack/rest exec vitest run src/security-routes.test.ts。
  • 需要你做的:无。非受管面,契约审查 PASS,席位落地。

席位意见: ACCEPT。按分诊「只拒显式空串」实现,门和插件两层都堵上;收窄已过契约审查,同族的空 object 500 另开卡。

席位核对

ACCEPT · head 821c2ced1dd2b12b9f4babbbf56f1aca7ee5a6aa · 2026-10-29T20:49Z

  • 形态: 7 files, +135/−2: the route, the plugin's target test, three test files (route, plugin, dogfood door), one docs line, one changeset. No packages/spec, no new code, no governed path; Tier S.
  • Clause-②: no (narrowing), Metadata-protocol: none, patch + BREAKING, ADR-0087 not-required (no-migration-prescription). Contract review at CONTRACT_REVIEW_TIER (claim on this PR), record 6507706351: PASS, no must-fix, no should-fix (two comment nits recorded).
  • 前提实测: the new pins red on main (the route dropped an explicit "" and the plugin's truthy test explained the caller); two ablation legs (route guard, plugin test) red, restored to the HEAD blob.
  • 门禁 check-runs: 54 on this head: 39 success, 15 skipped, 0 other (read 2026-10-29T20:49Z); mergeable_state: clean. Derived gates 95/95, 0 NOT-MEASURED; rest 344/344, plugin-security 237/237; dogfood battery 352 passed / 3 skipped, 2845 tests; pnpm lint exit 0.
  • 接受的偏差: the card thread's session is named in the body prose (the header and footer carry the seat's per the common terms); the seat adds the seat signature as the body's first line.
  • 范围外去向: an empty object at the same door answers 500 EXPLAIN_FAILED → security: POST /api/v1/security/explain with an empty object answers 500 EXPLAIN_FAILED, a malformed request answered as a server fault #27719. The review's spec-placement note (an empty userId refusal could also live in ExplainRequestSchema) rides that card's triage.
  • 散文面: changeset and explain.mdx checked against the diff by the review.
  • 落地条款: relay pr_ready + automerge_enable through the merge queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: POST /api/v1/security/explain with userId "" silently explains the caller instead of refusing

1 participant