Skip to content

fix(plugin-security)!: security explain refuses a userId that no sys_user record has with 404 RECORD_NOT_FOUND - #26801

Merged
objectstack-fleet[bot] merged 3 commits into
mainfrom
claude/project-thread-q3bf3c
Oct 27, 2026
Merged

objectstack-fleet[bot] merged 3 commits into
mainfrom
claude/project-thread-q3bf3c

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Requested by Leehom · project thread
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 QA run · observations:18.2.0-w2 (9/9) · fb91e979 · 2026-10-27 · 5 CONFIRMED / 0 NOT-REPRODUCED / 4 BY-DESIGN #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 (rest: POST /api/v1/security/explain answers a service refusal carrying INVALID_FILTER / 400 as 500 EXPLAIN_FAILED — the route's catch maps only PERMISSION_DENIED and OBJECT_NOT_FOUND #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. feat!: ownership queues and assignment rules (ADR-0142 phase 2) #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). security: the security plugin does not pick up some runtime metadata changes until a restart (detail withheld pending maintainer) #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

  • 1. Schemas start as Zod; packages/spec holds no runtime logic. (spec untouched)
  • 2. Every name I added uses the casing for its kind.
  • 3. No formatter ran; pnpm lint passes (eslint on the touched files).
  • 4. One issue, one branch off an up-to-date main, one pull request.
  • 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset.
  • 6. packages/spec touched: n/a.
  • 7. No hand edits to content/docs/references/, packages/*/CHANGELOG.md or content/docs/releases/.
  • 8. Title, body and comments are in English (the maintainer brief is Chinese by the repo's PR convention).
  • The Before Submitting list in CONTRIBUTING.md is done.

Generated by Claude Code

…ow has with 404 RECORD_NOT_FOUND

Explaining another user built the principal through the authorization
resolver, which synthesises the baseline anchor and sets for any id, so an
unknown id was answered 200 allowed: true. After the caller is admitted to
explain others, and before the context is built, the explained id is now read
through the delegator path's own sys_user existence read (readSysUserRow,
shared with resolveDelegatorContext) and refused 404 RECORD_NOT_FOUND naming
it. An anonymous caller still gets 401 and a caller who may not explain others
still gets their 403 before any existence read.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FEkQ5kdwdgQcbyZc22QQYf
…the same three plugin-security files)

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FEkQ5kdwdgQcbyZc22QQYf
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

41 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 8b276385e6f1e2b94a4061160c148cfa56f81588.

⛔ 7 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: sys_user (literal, 45 pages)
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 55 of 207 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 — 17 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 8b276385e6f1e2b94a4061160c148cfa56f81588 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 1bbc835ec952737ea4d23dfdcc5e731b845e37f5 — the merge of head 6c7cf979dacf820b0dbd0b5385f71669099e9af7 into base 8b276385e6f1e2b94a4061160c148cfa56f81588, 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 1bbc835ec952737ea4d23dfdcc5e731b845e37f5 && git checkout 1bbc835ec952737ea4d23dfdcc5e731b845e37f5
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 8b276385e6f1e2b94a4061160c148cfa56f81588 6c7cf979dacf820b0dbd0b5385f71669099e9af7 && git checkout -B drift-repro 8b276385e6f1e2b94a4061160c148cfa56f81588 && git merge --no-ff 6c7cf979dacf820b0dbd0b5385f71669099e9af7

node scripts/docs-audit/affected-docs.mjs --json 8b276385e6f1e2b94a4061160c148cfa56f81588

⚠️ 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 8b276385e6f1e2b94a4061160c148cfa56f81588 → 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

席位核对

ACCEPT · domain:server seat 2 · os-project-manager · session_0186vfjHHMsRXC9EUT9hTFMo · 2026-10-27T17:11Z · head 6c7cf979da · #26641

The maintainer's standing authorisation for landing reviewed PRs, verbatim: 「按照 pm-dispatch 规则复核过的都授权」.

  1. Shape:
    • A draft against main, assignee os-project-manager.
    • These body lines are all line-initial:
      • Fixes #26641
      • Clause-②: no (narrowing)
      • Metadata-protocol: none
      • Skills-docs: updated here (content/docs/permissions/explain.mdx)
    • One @objectstack/plugin-security patch changeset, with the BREAKING accept-set banner and an adr-0087: not-required (no-migration-prescription) line. No key or export changes shape.
    • check-governed-merges --pr 26801 reads NOT governed, 302 changed lines.
    • The card's newest claim (6444709975) names this PR's branch.
  2. Diff against the triage (6441402728) and the order; the seat read the whole non-test diff:
    • The existence read is lifted out of resolveDelegatorContext into readSysUserRow (explain-engine.ts). The delegator path and explainAccessForCaller both call it, so there is one reader and ⛔ no second copy.
    • The check sits inside the explain-another-user branch, after the manage_users / organization-admin / delegated-scope admission and before buildContextForUser. So the order is 401 → the existing 403 → 404, and an unprivileged caller learns nothing about which ids exist.
    • Self-explain is unchanged.
    • A store fault while reading propagates as a fault, never as "no such user". The delegator path keeps its fail-closed missing.
    • ExplainUserNotFoundError carries RECORD_NOT_FOUND / 404, the standard catalog's pair, which needs no new code or ledger row. Its prefix stays off the [Security] Access denied matcher.
  3. Tests:
    • Dogfood explain-unknown-user.dogfood.test.ts, 5 cases:
      • the control 404;
      • an admin gets 404 naming the id, for read and for create;
      • anonymous gets 401;
      • a plain member gets the same 403 for an unknown and an existing id;
      • an existing member and self-explain are unchanged.
    • Service pins, 3, and a REST envelope pin on POST and GET.
    • One posture fixture is given the sys_user row its explained member needs.
    • Red by ablation: the card's own 200 allowed: true with positions ["everyone"], then restored.
  4. Overlap:
  5. Gates:
    • At this stamp every check-run on the head reads success or skipped; none reads failed or in progress.
    • Locally the dev reports:
      • 95 derived gates run, 0 NOT-MEASURED;
      • plugin-security 5032 tests and rest 6499 tests;
      • the full dogfood battery for the (narrowing) arm: 303 files, 2512 tests, VERDICT command-exit 0.
  6. Out of scope: the Studio explain panel blanks on any non-403 refusal in the nested envelope. That predates this PR, which adds one more producer. Filed as Studio: the Access explain panel goes blank on any non-403 refusal, because it renders the nested error envelope's error object as its message #26805.

Generated by Claude Code

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 explain answers 200 allowed:true for a userId that matches no sys_user row, built from the synthesized baseline principal

2 participants