Repository navigation
fix(rest): app nav prunes doc entries the caller may not read (ADR-0046 §6.7) - #20128
Conversation
…olution Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn
Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn
Co-authored-by: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn
📓 Docs Drift CheckThis PR changes 1 package(s): 2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 5 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 15 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 7ee373edfe1da10fb99433a006430370c7ff43ab && git checkout 7ee373edfe1da10fb99433a006430370c7ff43ab
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 226e00c038527234472ef89e7bbd6ee11da33dfd f2bc219be0f104aaac1d4ba84d2b84c69bf6c0fc && git checkout -B drift-repro 226e00c038527234472ef89e7bbd6ee11da33dfd && git merge --no-ff f2bc219be0f104aaac1d4ba84d2b84c69bf6c0fc
node scripts/docs-audit/affected-docs.mjs --json 226e00c038527234472ef89e7bbd6ee11da33dfd
|
Brings in PR #20128 (app nav doc-audience prune), which landed on rest-server.ts after this branch was cut. No conflict. Claude-Session: https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn Co-authored-by: Claude <noreply@anthropic.com>
…rt, meta history and search (objectstack-ai#20061, objectstack-ai#20062) (objectstack-ai#20137) Fixes objectstack-ai#20061 Fixes objectstack-ai#20062 Clause-②: no (narrowing) ## What Four published REST doors read `?limit=` with a bare `Number()`. They substituted, clamped or dropped the result and answered `200`: - `GET /data/import/jobs` - `GET /data/:object/export` - `GET /meta/:type/:name/history` - `GET /search` Each door now reads `limit` against its own declaration and refuses a value outside it. `GET /data/import/jobs` does the same for `offset`, because its declaration bounds `offset` too. The refusal is `400` with the data surface's existing `VALIDATION_FAILED` + `fields[]` envelope, and the service is never called. Nothing a conforming caller sends changes its answer. The change is one private reader in `packages/rest/src/rest-server.ts`, `readDeclaredQueryNumber`, with no new package export: - **import jobs** read through `ListImportJobsRequestSchema.shape.limit` / `.offset`; - **history** reads through `HistoryMetaItemRequestSchema.shape.limit`; - **export** and **search** declare no request schema, so they read through a module-private `z.number().int().optional()`: a whole number, nothing about range. A non-blank numeric string is coerced with `Number()` and handed to the schema. Anything else (`abc`, a blank string, a structured value) goes to the schema as it came, so the declaration refuses it by type. `Number()` never gets to invent a `0` or a `NaN` for it. ## Before and after, measured on the real `RestServer` routes (spy protocol) Every row answered `200` before this PR. | door | request | answered before | answers now | |:--|:--|:--|:--| | import jobs | `?limit=0` / `?limit=-3` | limit 50 / 1 | `400`, `fields[0]` = `limit` / `min_value` | | import jobs | `?limit=201` / `?limit=500` | limit 200 | `400`, `max_value` | | import jobs | `?limit=abc` / `1.5` / `Infinity` / blank | limit 50 / 1.5 / 200 / 50 | `400`, `invalid_type` | | import jobs | `?offset=-1` / `abc` / `1.5` | offset 0 / 0 / 1.5 | `400`, `min_value` / `invalid_type` | | export | `?limit=abc` / empty / blank | `X-Export-Limit: 1` (a one-row export) | `400`, `invalid_type` | | export | `?limit=1.5` / `Infinity` | `X-Export-Limit: 1.5` / `50000` | `400`, `invalid_type` | | meta history | `?limit=abc` / `Infinity` | limit dropped: the whole change log | `400`, `invalid_type` | | meta history | `?limit=` (empty) / blank | limit 0: zero events | `400`, `invalid_type` | | search | `?limit=abc` | `limit: NaN` handed to `searchAll`: no overall cap | `400`, `invalid_type` | | search | `?limit=1.5` / `Infinity` / blank | 1.5 / clamped to 100 / 0 | `400`, `invalid_type` | Unchanged and pinned by the lit controls: - **Import jobs:** absent or empty `limit` → 50, absent or empty `offset` → 0. Conforming values reach `findData` unchanged. - **Export:** absent → 10000. `?limit=25` → 25. The door's own range handling is untouched: `?limit=0` → 1 and `?limit=60000` → 50000. - **History:** absent → no `limit` member. `5`, `0` and `1.5` are forwarded unchanged, because the declaration is `z.number()` with no `int()` and no bounds. - **Search:** absent or empty → `undefined`. `20`, `0` and `500` reach `searchAll` unchanged, and its `[1, 100]` clamp is its own business. The refusal body carries: - `code: 'VALIDATION_FAILED'`; - `fields: [{ field: 'limit', code, message }]`; - an `error` sentence naming the parameter; - `object`, on the export door only. The handler throws `validationFailure` (`@objectstack/types`), and each door's existing catch (`handleRouteError` / `mapDataError`) writes the 400. So `rest-server.ts` gains no response write site, and `check:route-envelope`'s `siblingCode` count on it is unchanged (green). ## PM mechanism assumptions, measured 1. **The `parseIntegerParam` cycle: confirmed.** `packages/runtime/package.json` depends on `@objectstack/rest`. rest's dependencies are `core`, `metadata-core`, `observability`, `platform-objects`, `service-package`, `spec`, `types` (plus `exceljs`, `zod`). Nothing was moved; `packages/runtime/**` and `packages/spec/**` are untouched. 2. **The declared-schema idiom holds, with one measured difference in `fields[].code`.** zod 4.6.1 against the declared schemas, mapped through `zodIssuesToFields`: - `NaN`, a string, `Infinity`, and `1.5` against `int()` all give `invalid_type`; - below `min` gives `min_value`; above `max` gives `max_value`. These are all ADR-0114 catalog members, never zod's own codes. Runtime's `parseIntegerParam` answers `invalid_number` for the same `abc` / `1.5`; see the open question in the report. 3. **The doors match the card, with two corrections.** - Export serves no declared request schema. `CreateExportJobRequestSchema` is the contract of `POST /api/v1/data/:object/export`, which rest does not mount. `ExportRequestSchema` (`contract.zod.ts`) is bound to no route. - The search door holds: the only production caller of `searchAll` is the REST door (`git grep`, tests excluded). So `packages/metadata-protocol/src/protocol.ts` is not in this diff. 4. **The empty-string premise is partly falsified.** Only import jobs and search answered `?limit=` with their default. Export answered a one-row export (`Number('') || 0` → `Math.max(1, 0)`), and history answered zero events (`Number('')` = 0, forwarded). The rule applied is the one `parseEnumParam` (runtime `query-param.ts`) states for the same spelling: - Where the old answer already was the absent answer, empty stays absent. - Where the old answer was an invented `0`, empty is refused. Reading it as absent on export and history would have turned those two into a 10000-row export and the whole change log. That grows the window, the widening this claim stops on, so the reader carries an explicit per-door `emptyIsAbsent`. No door accepts anything it refused before: none refused any single-valued `limit` before this PR. No conforming value's answer changes either, so the declaration stays `no (narrowing)`. ## Tests The new file is `packages/rest/src/rest-server-limit-param-parsing.test.ts` (43 cases). Each refusal pins `status` 400, `code` `VALIDATION_FAILED`, `fields[0].field` and `fields[0].code`, and that the service spy was never called. Each lit control pins the argument the service received. - Against unfixed `6780e34a`: 24 failed (every refusal row answered 200), 19 passed (every lit control). - Against the fix: 43 passed. One existing fixture was corrected in `rest-server-closed-query-params.test.ts`. Its search closed-set loop sent `limit: 'x'` and expected 200; it now takes a valid value per name, as its export twin in the same file already does. Runs at head `74898ed5`, after merging `origin/main`: - **Rest suite:** `pnpm --filter @objectstack/rest exec vitest run --project local --maxWorkers=2` → `Test Files 196 passed (196)`, `Tests 3327 passed | 1 skipped (3328)`. - **Typecheck:** `pnpm --filter @objectstack/rest run typecheck` → exit 0. `tsc --noEmit` passes, and `check:test-typecheck: OK — ... 0 file(s) / 0 error(s)`. Ablations went through `scripts/ablation-replace.mjs` in WRAP mode, run after the fix was committed. Vitest reads `src/` through the relative import, so no dist was involved. - **Import jobs:** restoring `Math.min(200, Math.max(1, Number(q.limit) || 50))` landed on disk (anchor 1 → 0, blob `02ceccd1` → `cab07300`). Result: `Tests 8 failed | 35 passed`, exactly the 8 import-jobs `limit` refusal rows. - **Search:** restoring `req.query?.limit ? Number(req.query.limit) : undefined` landed (blob `02ceccd1` → `9b589c00`). Result: `Tests 4 failed | 39 passed`, exactly the 4 search refusal rows. - Both restores were proven: blob == HEAD `02ceccd1`, `git diff HEAD` empty. Public surface: rest's `dist/index.d.ts` and `dist/index.d.cts`, built from merge-base `bc80e162`'s `rest-server.ts` and from HEAD's, are byte-identical (`cmp`). The restore was proven by blob hash. So no importing package's types can move, and only rest's own tests are owed. ## Gates `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` on the real diff gives 61 commands, identical to the PM's list. All ran at `74898ed5` with exit codes captured before any pipe. `--ran` reconciliation: `61 derived, 59 run, 2 NOT-MEASURED, 0 UNRUN`. - **59 exit 0.** This includes `check:adr-0087-registration --base origin/main` (`not-required (no-migration-prescription)` accepted), `check:changeset-no-major`, `check:empty-changeset`, `check:route-envelope`, `check:nul-bytes`, `check:doc-authoring`, `check:published-files`, `check:issue-citations` and `check:undeclared-dep-imports`. - **NOT MEASURED: `check:dual-build-cjs-loads`**, exit 3 `PREREQUISITE NOT MET`: 43 workspace packages have no `dist/`. Declared narrowing: `require('./dist/index.cjs')` and `import('@objectstack/rest')` both load and expose `RestServer`. - **NOT MEASURED: `check:type-check-debt`**, exit 3 `PREREQUISITE NOT MET`: the full closure is not built. See the `.d.ts` identity above; CI builds the closure before this step. - **`pnpm lint`** (the lane addition) ran as the full union, `eslint . --no-inline-config`, at `74898ed5`: exit 0. ## First-party callers, measured - `@objectstack/client` sends `limit` as written: `data.listImportJobs`, `data.export`, `meta.getHistory`, `search`. - objectui at its pin `f8a9d0fb`: `ImportWizard` sends `limit: 50`, `ResourceHistoryPage` sends `limit: 100`, and the search and export emitters send `limit` only when it is a number greater than 0. None sends a value this PR refuses. ## Acceptance notes - **Same-family reads left out of this diff.** Triage's note 3 keeps them out, and the file is also held by another claim (PR objectstack-ai#20125). - Measured on the real routes with a spy protocol, each answering 200: - `GET /meta/:type/:name/history?sinceSeq=abc` drops `sinceSeq` and reads the log from the start (`rest-server.ts:8139`); - `GET /meta/:type/:name/audit?limit=abc` drops `limit`, so the producer's default 100 is served (`:8318`; declared `z.number()`, which refuses `NaN`); - `GET /search?perObject=abc` hands `perObject: NaN` to `searchAll` (`:10709`). - Read only, not measured: `GET /data/approvals/requests` `limit` / `offset` drop a non-numeric value and serve the unpaged list (`:13601`). Export `?page=` falls back to the 500-row chunk (`:10297`), which changes chunking only, not the rows returned. - **History's declared `limit` admits `1.5`, `0` and negatives.** The door forwards them as before. Whether `HistoryMetaItemRequestSchema.limit` should declare `int()` / `min()` is the spec seat's question, and this PR takes no position. - **Export and search range stay as they were:** the export floor of 1 and cap of 50000, and search's `[1, 100]` clamp. Both cards take no position on bounds. - **`GET /data/import/jobs?status=` is not checked against the declared `ImportJobStatus`.** Read only, not measured, out of scope. - **Merge:** `origin/main` was merged (no rebase) after PR objectstack-ai#20128 landed on `rest-server.ts`, with no conflict. PR objectstack-ai#20125's hunks do not overlap these doors. - **CI-owned, not run locally:** the full `Test Core` shards, `Dogfood`, `Build Core`, `Temporal Conformance`, and the workspace type-check lanes. --- _Generated by [Claude Code](https://claude.ai/code/session_01TnPAC1UsTGfHPXVUCL6iLn)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Face review (post-hoc, released)Served-tier: Scope: the changeset (and named docs) of PR #20128, released; this review cannot block anything and exists to find false released prose. Sentence verdictsChangeset
Docs
Still true on origin/main?Yes, with one relocation worth naming. The gate moved out of FindingNONE at the correction bar. The one overstatement (changeset sentence 6, the Reviewed-by: VERDICT: CLEAN Generated by Claude Code |
Fixes #19790
Clause-②: no
The server's app-nav filter (
filterAppForUserWithReason,packages/rest/src/rest-server.ts) gains the audience arm the maintainer ruled (ruling5793362670, letter A). Atype: 'doc'entry naming a doc the caller may not read is pruned. Abookentry with no page the caller may read is pruned too. Both callers apply it: the list routeGET /meta/appand the by-name routeGET /meta/app/:name. It covers the top-levelnavigation,childrenandareas[].navigation. The rule is the oneDocNavItemSchemaalready declares. Before this change only the (unshipped) objectui renderer could honour it, so every member received the entry: its label, plus the gated book or doc name.One resolution, no second resolver
A private
resolveDocsAudience(environmentId, req, books)now builds aDocsAudience: the caller, the fast path, a book lookup (declared book, else the implicit per-package book), the book's own audience gate, a per-doc reader, the readable tree, and the readable pages. Every verdict is asked of the@objectstack/spec/systemhelpers (audienceAllows,resolveDocAudiences,docAudienceAllows,resolveBookTree,resolveBookClaimedDocs,deriveImplicitPackageBook). Five doors are built on it:/meta/book,/meta/book/:name,/meta/doc,/meta/doc/:nameand/meta/book/:name/tree, which used to hold three hand-written copies of this logic, plus the new nav arm.BASEfa00ebf4'srest-server.tsagainst this branch's over 1584 requests. The matrix crossed 4 callers (anonymous, non-holder, holder, unresolvable holdings) × 3 book sets (set-gated, org and public only, none) × 3 failure modes (none, book read throws, doc read throws) × the book and doc list, item and tree routes, in both spellings, with and without?package=and?include=content. It found 0 diffs across statuses200:752 401:168 403:28 404:240 500:396, with 55 distinct responses. A control (non-holder 403 vs holder 200 on the same doc) proves the probe distinguishes callers. The existing suites (rest.test.tsADR-0046 §6.7 block,meta-audience-plural,meta-public-book-grant) stay green.Entry semantics, measured against the resolver
docalone: served iffdocAudienceAllows(resolveDocAudiences(...).get(doc), caller). This is the answer/meta/doc/:namegives.bookalone: the named book (else the implicit per-package book) must admit the caller by its own audience. That is the tree read's 401/403. And at least one of its pages must be readable. A book's pages are the docs it claims, perresolveBookClaimedDocs, the membershipresolveDocAudiencesitself uses. Measured:resolveBookTreeappends every doc the book does not claim as a synthetic Uncategorized group. Over an env-wide corpus, nearly every book's tree therefore holds some readable doc, and counting tree entries would never prune. The spec calls those orphans "not an authored membership claim". A pin shows the implicitopsbook's tree servingcrm_introunder Uncategorized while the entry is still pruned.book+doc: pruned when either the book's own audience denies the caller or the doc is unreadable. A readable doc inside an unreadable book is possible, because a doc's effective audience is the union over every book claiming it (docAudienceAllows,book.zod.ts). But the entry opens the page in that book's context, whose tree read is refused (theadmitsBookcheck in the tree route), and the entry itself names the gated book. So it is pruned.docs/nav-target. Adocnaming a doc absent from the corpus is served, because the resolver's own default for a doc it has no entry for isorg. Abooknaming nothing resolves to an implicit book with no page, and is pruned: "no readable page" is the rule's own wording. Both are pinned.GET /meta/appanswers 401 before any read (pinned). The umbrella gate admits anonymous callers only to the book and doc reads, so there is no anonymous app nav for the arm to cover.withheldis app-level only, so the arm is a barecontinue, like its neighbours. An app it empties is still served.Fails closed
docentries. Both callers must pass a gate, or the arm drops everydocentry.docentry of that response and logs awarn. The rest of the navigation is served.resolveAudienceCaller(ADR-0049).[](fetchAudienceBooks), which reads as "no gated book anywhere". Copying that would serve set-gated entries to every member. The nav gate reads the same data throughreadAudienceBooks/readDocCorpus, which report the fault, and prunes. The reads keep their answers unchanged. That fail-open is reported below as a finding.Cost per
/meta/apprequest (ruling point 4)docentry in the app list: zero added reads, only a walk of the nav trees (pinned: no book read, no doc read, no holdings).booklist read;doclist read, only off the fast path or when an entry names a book alone.resolveDocAudiencesruns oneresolveBookTreeper book over the whole corpus, shared by every entry. Per entry: adocis one map lookup. Abookalone is oneresolveBookClaimedDocs, which is oneresolveBookTreeof that book: every doc is visited once per group rule, plus one lookup per claimed page.Tests
packages/rest/src/meta-app-nav-doc-audience.test.tshas 19 cases. They drive the real list, by-name and tree routes:requiredPermissionscontrol;Evidence:
pnpm --filter @objectstack/rest exec vitest run --project local --maxWorkers=2 src/meta-app-nav-doc-audience.test.ts:Tests 19 passed (19).@objectstack/restfull local project:Test Files 195 passed (195),Tests 3284 passed | 1 skipped (3285).pnpm --filter @objectstack/rest run typecheck: exit 0, test layer0 file(s) / 0 error(s).cb800e24. The later commitf2bc219btouches only the docs page and the changeset.scripts/ablation-replace.mjsin wrap mode (anchor 1 to 0, blob5c7dbf8f1015tof0b3c3cdd872). Result:Tests 12 failed | 7 passed (19). The 12 red are every non-reader, fail-closed, parity-non-holder, fast-path-existence and two-app-cost row. The 7 green are the holder, one-readable-page, control, no-doc-entry-cost, no-book-entry and anonymous rows. The restore was proven: blob equalsHEAD, andgit diff HEADis empty. A re-run is green (19/19).node scripts/pm/dispatch-gates.mjs --commandsderived 90 commands atf2bc219b. All exit 0. Reconciled with--ran:90 derived, 90 run, 0 NOT-MEASURED(a derived zero, since every row recorded its exit code). Two gates first refused with PREREQUISITE NOT MET and were re-run green after building what they read:check:skill-examplesafter building client and client-react, andcheck:dual-build-cjs-loadsafter the workspace build.pnpm lint(full union,eslint . --no-inline-config) exited 0 in 107s atf2bc219b.Docs and changeset
content/docs/ui/apps.mdx: the Audience bullet now says the server applies the rule to the entry itself, including thebook+docrule, and which pages count..changeset/19790-nav-doc-audience-prune.md:@objectstack/restpatch.Acceptance notes
?package=,/meta/book/:name/treeresolves over the env-wide corpus. Every book's tree therefore lists every doc it does not claim under Uncategorized. That is the resolver's documented "nothing is ever dropped" rule, and it is why the nav arm counts claimed pages rather than tree entries.fetchAudienceBooksswallows the fault to[], and/meta/doc/:nameswallows a thrown corpus read the same way. Probe: a non-holder'sGET /meta/doc/crm_admin_runbookgoes from 403 to 200 with the body, andGET /meta/doclists it, once the book read rejects. The shippedgetMetaItemsrethrows everysys_metadataread failure except "unprovisioned".GET /meta/app/:name/layers, the deprecated?layers=true, andGET /meta/app/:name/publishedserve the unfiltered app document. Probe: an authenticated member holding neither the audience nor therequiredPermissionsreceives both kinds of entry./historyand/diffserve stored app versions the same way; that was read from code, not driven.packages/runtime/src/domains/meta.ts's dispatcher list and item reads apply no app-nav filter where that door answers. The MCPlist_metadata/describe_metadataimplementations are not in this tree, so they are NOT MEASURED here.doc/bookauthoring surface and adocnavigation item (maintainer's stated need) #19482 (thedocvariant) is the parent card and is not addressed here. The objectui renderer pruning (objectui#10188) stays as defence in depth, per the ruling.Generated by Claude Code