Skip to content

fix(metadata-protocol): global search skips objects and fields the caller cannot read - #21879

Merged
objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/issue-21836-search-skip-unreadable
Oct 5, 2026
Merged

objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/issue-21836-search-skip-unreadable

Conversation

@objectstack-fleet

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

Copy link
Copy Markdown
Contributor

Fixes #21836

Clause-②: no

What changed

searchAll (packages/metadata-protocol/src/protocol.ts) no longer answers a member's whole global search with 403 PERMISSION_DENIED because one object in scope is unreadable.

  • Object level. Before an object is queried, the sweep asks the security service's canReadObject with the caller's context. That method is the engine middleware's own read gate, arm for arm (ISecurityService.canReadObject). An object it refuses is skipped.
  • Field level (found on the real boot). With only the object-level skip, a restricted member's UNSCOPED search was still 403. The cause: sys_user's server-resolved search fields include admin-only columns (role, ban_reason, last_login_ip), and the engine's predicate guard refuses a search that matches on a field the caller may not query. Each object is now searched only on the fields the caller may query (getQueryableFields), handed to the engine as searchFields. ADR-0061 says that key only ever narrows. An object left with no queryable search field is skipped. When the caller may query every search field, no searchFields is sent, so the request is the same as before.
  • No leak from a skipped object. It is never queried, never named and never counted in totalObjects. The decision is made before any row is read, so no hit, count or timing depends on what it holds. No objectsSkipped field was added, because that count would itself describe objects the caller cannot see. An explicit objects= naming an unreadable object gets the same answer as a name that matches no object.
  • Still fails closed. A read error on a readable object propagates, envelope intact. If canReadObject or getQueryableFields throws, the search fails instead of shrinking. Without a security service, or with one that lacks these methods, there is no pre-filter, and find still enforces. Calls that carry no context are not pre-filtered.
  • The misleading comment is fixed. The per-object catch said object authorization is enforced at the REST door (enforceAuth) first. That door checks authentication only. The comment now says where admission is decided.

Route chosen: the pre-filter, not catching the typed denial

The triage comment chose to catch the engine's typed denial. The dispatch preferred the pre-filter. I took the pre-filter for two reasons:

  1. The middleware throws the same PermissionDeniedError for very different causes: "permission subsystem unavailable", an unresolved object posture, a missing delegator, a field-level filter-oracle refusal. Catching the class would swallow every one of them, including a field-level refusal on an object the caller may read (the 403 this card's dogfood hit). With the shipped plugin-security, a permission outage is not distinguished by either route: canReadObject answers false on a resolution failure, so the pre-filter skips objects during an outage too (see the Acceptance note below). The deciding reasons are item 2 and the field-level fix, not outage handling.
  2. The single-authority concern is met by contract: canReadObject is specified to compute the middleware's verdict from the same resolution, never a re-derivation.

Tests

Unit tests are in packages/metadata-protocol/src/protocol.search-skip-unreadable.test.ts, 12 cases:

  • a control that reproduces the 403 when no security service is wired;
  • a mixed scope: only readable hits, and the unreadable objects are not queried, named or counted;
  • the answer is the same whether or not a skipped object's rows would match;
  • an explicit objects= with an unreadable object answers the same as a nonexistent name;
  • an all-access caller gets the same answer as with no service;
  • a read error on a readable object still fails the search;
  • an admission check that throws fails the search;
  • a call without a context is not pre-filtered;
  • a partial queryable set is sent as searchFields;
  • an object with no queryable search field is skipped;
  • a full queryable set, or no answer from getQueryableFields, sends no searchFields;
  • a field check that throws fails the search.

Dogfood: packages/qa/dogfood/test/search-skip-unreadable.dogfood.test.ts boots a real kernel with the real SecurityPlugin over HTTP. Its two app objects hold rows matching one term, and the member can read one of them. It covers:

  • control: the walled object is 403 at its own /data door;
  • the member's unscoped search is 200 with the readable hit and never names the walled object;
  • objects=open,walled returns the readable hit only;
  • objects=walled gives the same answer as a nonexistent name;
  • the admin still gets both hits.

Ablation (dogfood, one-off, nothing left in the tree)

  • Mutation. node scripts/ablation-replace.mjs replaced the canReadObject skip line with a constant-false guard carrying the marker ABLATED_21836 (anchor hits 1 to 0, blob 66cc5f6879b0 to 5d37739011c9).
  • Build and check. Ran OS_SKIP_DTS=1 for the metadata-protocol build, then ablation-dist-preflight confirmed the marker is present in dist/index.js and dist/index.cjs.
  • Result. Dogfood went 2 failed / 2 passed. Both member cases got 403 PERMISSION_DENIED ("You do not have permission to perform this action.").
  • Restore. The tool restored the file, with blob equal to HEAD and an empty git diff HEAD. The rebuilt closure then passed ablation-dist-preflight --absent, with the marker absent from all 24 built files and the tree clean.

Local verification (at the final head)

  • pnpm --filter @objectstack/metadata-protocol exec vitest run over the 7 search test files: 68 passed.
  • Dogfood file: 4 passed.
  • typecheck for metadata-protocol and dogfood: exit 0. Both programs include the new test files (--listFilesOnly).
  • The derived gate families from dispatch-gates.mjs --commands are green, plus check:type-check-debt, which ran under the verify lock. The exception is check:dual-build-cjs-loads, which printed PREREQUISITE NOT MET because unrelated packages in this worktree have no dist/. That gate was NOT MEASURED and is declared to CI.
  • check:engine-double-contract asked for the new double to be pinned, and that pin is committed.
  • Lint, narrowed to the 3 changed TS files with eslint --no-inline-config --format json: 3 files, 0 errors, 0 warnings. Type-aware linting is not enabled in eslint.config.mjs (no parserOptions.project), so this diff cannot change the verdict on any untouched file.

Acceptance notes

  • Out of scope here, and remains open as a separate finding: the same field-level refusal at GET /api/v1/data/sys_user?search=admin. Measured on a fresh boot as a plain member: 403 PERMISSION_DENIED ("query on 'sys_user' references field(s) not readable by the caller: role, ban_reason, last_login_ip"). The same member gets 200 from GET /api/v1/data/sys_user. The caller named no field. The server picked the search fields and then refused the caller for them. The upstream fix belongs in the engine's search expansion (expandSearchOnAst), which could narrow to the caller's queryable fields. If that lands, the field narrowing in searchAll becomes redundant and can be deleted. It is reported for filing by the seat.
  • canReadObject in plugin-security returns false, logged at error, when permission resolution throws. It does not throw. During a permission outage the search therefore skips objects instead of failing. That is the method's documented fail-closed contract, and nothing leaks, but it is not the propagate-on-outage behaviour of the rest of this sweep. No defect is claimed; noted only.

Generated by Claude Code

…rch sweep

canReadObject in plugin-security answers false on a permission-resolution
failure, so an outage skips objects rather than failing the search. Also note
why an undefined getQueryableFields answer narrows nothing here.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zT8d8NpiQ1ExhuNd5TxY6
Resolve the engine-double ledger by regenerating it with
check-engine-double-contract --write (both pins kept).

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018zT8d8NpiQ1ExhuNd5TxY6
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, 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/concepts/metadata-lifecycle.mdx (via ObjectStackProtocolImplementation (symbol, a top-level class))

⛔ 3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via ObjectStackProtocolImplementation (symbol, a top-level class))
  • content/docs/releases/v17/17-0.mdx (via ObjectStackProtocolImplementation (symbol, a top-level class))
  • content/docs/releases/v17/17-1.mdx (via searchAll (symbol, a method of class ObjectStackProtocolImplementation))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 54 of 206 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 — 11 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 e864db56dffc2dec3290e5f0700f9d1606a1c830 → packageMentionDocs.

Which tree this was computed on

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

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

⚠️ 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 e864db56dffc2dec3290e5f0700f9d1606a1c830 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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/l tests tooling

Projects

None yet

2 participants