Repository navigation
Commit 6279efe
fix(plugin-security)!: security explain refuses a userId that no sys_user record has with 404 RECORD_NOT_FOUND (#26801)
<!-- claude-projects-attribution: {"github_login":"os-project-manager"}
-->
_Requested by **Leehom** · [project
thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1XXaahUHvAiCTPBZ9rNgWxGn)_
Fixes #26641
Clause-②: no (narrowing) — `POST` / `GET /security/explain` and the
in-process `explain()` refuse a request naming another user whose id no
`sys_user` record has, which they answered with a verdict before.
Nothing is widened.
Metadata-protocol: none
Skills-docs: updated here — `content/docs/permissions/explain.mdx` (the
refusal and its place after the 403); no `skills/**` file teaches the
explain door's `userId`
## 维护者速读
- **改了什么**:管理员在「访问解释」里查一个不存在的用户 id 时,服务端现在回 404 `RECORD_NOT_FOUND` 并点名该
id,不再回 200 `allowed: true`。
- **为什么改**:解释器为任何 id 都会合成一个基线成员身份,于是对「不存在的人」给出了「能访问」的结论,管理员据此判断是错的(QA
#26639 第 4 条)。
- **风险与回滚**:只影响「解释他人」且 id 无对应用户这一条路径;检查顺序不变(匿名 401、无权解释他人者 403
都在存在性读取之前),不泄露 id 是否存在。回滚 = revert 本 PR。
-
**先看**:`security-plugin-explain-access.ts`(新增的存在性检查及其位置)、`explain-engine.ts`(与委托人路径共用的
`readSysUserRow`)、`errors.ts`(`ExplainUserNotFoundError`)。
- **两分钟验证**:`pnpm --filter @objectstack/plugin-security exec vitest run
--maxWorkers=2 src/security-plugin.test.ts -t 26641`,预期 3 条全绿。
- **需要你做的**:无,席位落地。
## What changes
- **The check.** `explainAccessForCaller`
(`security-plugin-explain-access.ts`) now reads the explained id through
`readSysUserRow` after the caller is admitted to explain someone else
and before `buildContextForUser`. No row ⇒ `ExplainUserNotFoundError`:
`code: 'RECORD_NOT_FOUND'`, `status` / `statusCode` 404, `userId`, and a
message naming the id that says no principal was evaluated.
- **One reader.** `readSysUserRow` (`explain-engine.ts`) is the
`sys_user` lookup `resolveDelegatorContext` already made, lifted out
unchanged (system context, `findOne` by id). The delegator path calls it
inside its existing `try` and still reads a failed read as `missing`.
The explain path does not catch: a store fault is not absence, so it
propagates as a fault instead of being answered "this user does not
exist".
- **The code.** `RECORD_NOT_FOUND` / 404 is the standard catalog's pair
("Record with given ID not found", `errors.zod.ts`), the same pair
`DetailRecordNotFoundError` in this package uses and `GET
/data/sys_user/ID` answers. No ledger row, no spec change, no new code.
The REST door serves it through its existing classified-refusal arm
(#20603): a declared 4xx `code` + `status` keeps its own answer. No REST
source changes.
- **Order of checks (unchanged).** Anonymous ⇒ 401 at the door. A caller
without `manage_users`, organization administration of the user's
organization, or a covering delegated scope ⇒ the same 403
`PERMISSION_DENIED` for an unknown id as for an existing user, and no
existence read is made. An organization administrator or delegated
administrator naming an unknown id therefore gets the 403 (no membership
or scope covers nobody); a `manage_users` holder or a system caller gets
the 404.
- **Unchanged.** Explaining yourself (no `userId`, or your own id) and
explaining a user who exists.
- **Fixture triage (1 file).**
`org-admin-assignment-boundary.posture.test.ts` explained `u_target`, a
member with no `sys_user` row (the boot leaves `sys_user` unprovisioned
for its write cases), so the read hit "no such table" and the
explanation now propagates that fault. Disposition: *add the
declaration*: that one case provisions `sys_user` and inserts `u_target`
before it explains; its assertions are untouched.
- **Docs.** `content/docs/permissions/explain.mdx` names the refusal
under "Another user".
## Why
The QA run #26639 (observation 4) measured it twice on fresh showcase
boots: the seed admin's `POST /api/v1/security/explain` with
`{"object":"showcase_task","operation":"read","userId":"no_such_user_qa_123"}`
answered `200 allowed: true`, principal `positions: ["everyone"]`,
`permissionSets: ["showcase_member_default", "member_default"]`, while
`GET /data/sys_user/no_such_user_qa_123` answered 404. Triage
`6441402728` ruled: answer 404 for an unknown user, and pin it. The
delegator path already states the rule this applies
(`resolveDelegatorContext`: "The `sys_user` existence check is the only
correct fail-closed point").
## Tests
- **HTTP door, showcase boot**
(`packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts`, new, 5
cases): `[control]` the data door's 404 for the same id; `[unknown]` the
seed admin is refused 404 `RECORD_NOT_FOUND` naming the id, for `read`
and `create`, with no verdict in the body; `[anonymous]` 401;
`[unprivileged]` a plain member gets the same 403 `PERMISSION_DENIED`
for the unknown id as for the admin who exists; `[existing]` the admin
explaining the member, and the member explaining themselves (no
`userId`, and their own), answer 200 with that principal.
- **Service** (`security-plugin.test.ts`, 3 cases in the ADR-0090 D6/D12
harness): the `manage_users` caller's refusal envelope (`code`,
`status`, `statusCode`, `userId`, the id in the message) for `read` and
`create`; a plain caller and a delegated admin both get
`PermissionDeniedError` and `sys_user` is never asked about the unknown
id; an existing user and a self-explanation by an id with no row still
answer.
- **REST envelope** (`security-explain-envelope.test.ts`, 1 case): a
thrown `RECORD_NOT_FOUND` / 404 leaves both transports as the nested 404
`RECORD_NOT_FOUND` envelope, not the 500.
## Acceptance notes
- **The Studio explain panel (asked by the order).**
`AccessExplainPanel.tsx` (`ui/packages/app-shell`) is the only `ui/**`
caller that sends `userId`; the grid and detail verdict hooks
(`useRecordCrudVerdicts`, `useRecordEditable`) explain the signed-in
user only and never name another id. The panel picks the user from a
record picker, so an unknown id reaches it only for a user deleted after
it was picked. Its non-403 branch reads `data.message || data.error`; on
the server's nested envelope `error` is an object, and rendering it
unmounts the panel. Measured in a throwaway render probe over the
panel's own test harness (deleted, not committed): a 404 nested envelope
⇒ `document.body` is an empty `div`, no alert; the control, a flat `{
message }` 404 body ⇒ the alert "Explain failed — Unknown object x".
This is NOT new with this PR: every non-403 refusal the door already
sends (400 `VALIDATION_FAILED`, 404 `OBJECT_NOT_FOUND` for a mistyped
object in the free-text input, 400 `INVALID_FILTER`, 500) takes the same
branch. Reported to the seat as a finding; not fixed here.
- **Overlap.** #26337 (ADR-0142 phase 2, open) edits all three source
files of this diff. Its hunks are elsewhere: `errors.ts` adds a class
after `ExplainObjectNotFoundError` (this PR adds one before it),
`explain-engine.ts` at the deps interface and record attribution (this
PR at `resolveDelegatorContext`), `security-plugin-explain-access.ts` at
the imports, host interface and the sharing block (this PR inside the
explain-another-user branch). #26698 (same package, runtime metadata)
has no PR yet.
- **A store fault while reading the id** propagates as a fault (the
door's 500 / classified answer, as a resolver fault already did on this
path); it is never answered as "this user does not exist".
## Verification
Head `6c7cf979da` (the fix, then `origin/main` merged in: #26337 landed
in the same three source files mid-run, merge clean). Every reading
below was taken at this head unless it says otherwise; the first head
`39a455551f` (before the merge) was measured the same way and was green
throughout (full dogfood 303 files / 2510 tests).
- **Red first, by ablation** (at `39a455551f`; the fix committed first;
`scripts/ablation-replace.mjs` turned the check off behind a never-set
env marker; plugin-security rebuilt; `ablation-dist-preflight.mjs` saw
the marker in `dist/index.js` and `dist/index.mjs`): the dogfood
`[unknown]` case red with exactly the card's answer, `expected 200 to be
404`, body `"allowed":true …
"positions":["everyone"],"permissionSets":["showcase_member_default","member_default"]`;
the service case red (`expected null to match object`). The other 4
dogfood cases and 2 service cases stayed green. Restored: blob equals
HEAD, `git diff HEAD` empty, rebuilt, preflight `--absent` ✓, tree
clean.
- **Suites:** `pnpm --filter @objectstack/plugin-security test`: 231
files, 5032 passed, 45 skipped, exit 0. `pnpm --filter @objectstack/rest
test`: 326 files, 6499 passed, 326 skipped, exit 0. Before the fixture
triage, the plugin-security suite was red in exactly the 3 posture cases
named above (and nowhere else).
- **Gates:** `dispatch-gates.mjs --commands` on this diff derived 95
families; all 95 ran and exit 0 at this head (`--ran` reconciliation: 95
derived, 95 run, 0 NOT-MEASURED, 0 UNRUN). At the first head, three
first exited non-zero for reasons outside the diff and were re-run
green: `check-comment-mask-corpus` (exit 1: my throwaway UI probe file
was deleted mid-sweep), `check:skill-examples` and
`check:dual-build-cjs-loads` (exit 3, PREREQUISITE NOT MET: spec
declarations and seven unrelated packages unbuilt; built, then green).
Also green: `eslint --no-inline-config` on the 7 touched code files,
`typecheck` and `check:test-typecheck` for plugin-security and rest,
dogfood `typecheck`, `check-platform-checklist --changed-from
origin/main`, `check-nul-bytes`.
- **Full dogfood** (`(narrowing)` arm): `pnpm --filter
@objectstack/dogfood test` under the verify lock, after `pnpm --filter
'@objectstack/dogfood^...' build`: VERDICT command-exit 0, Test Files
303 passed / 3 skipped (306), Tests 2512 passed / 20 skipped (2532), no
failure.
## Human Minimum
- [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic.
(spec untouched)
- [x] 2. Every name I added uses the casing for its kind.
- [x] 3. No formatter ran; `pnpm lint` passes (eslint on the touched
files).
- [x] 4. One issue, one branch off an up-to-date `main`, one pull
request.
- [x] 5. A change that publishes carries a changeset, and both
declaration lines are filled in here and in the changeset.
- [x] 6. `packages/spec` touched: n/a.
- [x] 7. No hand edits to `content/docs/references/`,
`packages/*/CHANGELOG.md` or `content/docs/releases/`.
- [x] 8. Title, body and comments are in English (the maintainer brief
is Chinese by the repo's PR convention).
- [x] The *Before Submitting* list in CONTRIBUTING.md is done.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01FEkQ5kdwdgQcbyZc22QQYf)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent f79ec28 commit 6279efe
9 files changed
Lines changed: 300 additions & 2 deletions
File tree
- .changeset
- content/docs/permissions
- packages
- plugins/plugin-security/src
- qa/dogfood/test
- rest/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
96 | 96 | | |
97 | 97 | | |
98 | 98 | | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
99 | 104 | | |
100 | 105 | | |
101 | 106 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
341 | 341 | | |
342 | 342 | | |
343 | 343 | | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
344 | 383 | | |
345 | 384 | | |
346 | 385 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
790 | 790 | | |
791 | 791 | | |
792 | 792 | | |
| 793 | + | |
| 794 | + | |
| 795 | + | |
| 796 | + | |
| 797 | + | |
| 798 | + | |
| 799 | + | |
| 800 | + | |
| 801 | + | |
| 802 | + | |
| 803 | + | |
| 804 | + | |
| 805 | + | |
| 806 | + | |
| 807 | + | |
| 808 | + | |
| 809 | + | |
| 810 | + | |
793 | 811 | | |
794 | 812 | | |
795 | 813 | | |
| |||
864 | 882 | | |
865 | 883 | | |
866 | 884 | | |
867 | | - | |
| 885 | + | |
868 | 886 | | |
869 | 887 | | |
870 | 888 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
247 | 247 | | |
248 | 248 | | |
249 | 249 | | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
250 | 255 | | |
251 | 256 | | |
252 | 257 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
72 | 72 | | |
73 | 73 | | |
74 | 74 | | |
75 | | - | |
| 75 | + | |
76 | 76 | | |
77 | 77 | | |
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
82 | 82 | | |
| 83 | + | |
83 | 84 | | |
84 | 85 | | |
85 | 86 | | |
| |||
164 | 165 | | |
165 | 166 | | |
166 | 167 | | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
167 | 177 | | |
168 | 178 | | |
169 | 179 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3621 | 3621 | | |
3622 | 3622 | | |
3623 | 3623 | | |
| 3624 | + | |
| 3625 | + | |
| 3626 | + | |
| 3627 | + | |
| 3628 | + | |
| 3629 | + | |
| 3630 | + | |
| 3631 | + | |
| 3632 | + | |
| 3633 | + | |
| 3634 | + | |
| 3635 | + | |
| 3636 | + | |
| 3637 | + | |
| 3638 | + | |
| 3639 | + | |
| 3640 | + | |
| 3641 | + | |
| 3642 | + | |
| 3643 | + | |
| 3644 | + | |
| 3645 | + | |
| 3646 | + | |
| 3647 | + | |
| 3648 | + | |
| 3649 | + | |
| 3650 | + | |
| 3651 | + | |
| 3652 | + | |
| 3653 | + | |
| 3654 | + | |
| 3655 | + | |
| 3656 | + | |
| 3657 | + | |
| 3658 | + | |
| 3659 | + | |
| 3660 | + | |
| 3661 | + | |
| 3662 | + | |
| 3663 | + | |
| 3664 | + | |
| 3665 | + | |
| 3666 | + | |
| 3667 | + | |
| 3668 | + | |
| 3669 | + | |
| 3670 | + | |
| 3671 | + | |
| 3672 | + | |
| 3673 | + | |
| 3674 | + | |
| 3675 | + | |
| 3676 | + | |
| 3677 | + | |
| 3678 | + | |
| 3679 | + | |
| 3680 | + | |
| 3681 | + | |
| 3682 | + | |
| 3683 | + | |
| 3684 | + | |
| 3685 | + | |
3624 | 3686 | | |
3625 | 3687 | | |
3626 | 3688 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
0 commit comments