Skip to content

fix(metadata-protocol): scope the metadata audit read to the caller's organization (#8747) - #8803

Merged
hotlong merged 3 commits into
mainfrom
claude/issue-8747-audit-org-scope
Aug 15, 2026
Merged

fix(metadata-protocol): scope the metadata audit read to the caller's organization (#8747)#8803
hotlong merged 3 commits into
mainfrom
claude/issue-8747-audit-org-scope

Conversation

@hotlong

Copy link
Copy Markdown
Contributor

Fixes#8747

Implements the maintainer ruling recorded on this card (comment 5299824146, delegated adjudication): wire it, do not delete it — build the $or the method's own comment describes, thread organizationId from the REST route, and pin both the scoped read and the comment now describing behaviour that exists.

The defect

ObjectStackProtocolImplementation.auditMetaItem declared organizationId?: string | null and never read it. Directly above its query sat a comment describing the filter it would have built — "include rows for the specific org AND env-wide (organization_id IS NULL) rows" — while the where was exactly { type, name }. The parameter was dead on the caller side too: GET /api/v1/meta/:type/:name/audit never passed one.

Re-measured on current main, not carried from the escalation

The earlier measurement was taken at ab8b10f82. #8777 (the write half, #8707) landed at 00:31Z in between, so the premise was re-measured rather than inherited. The disclosure reproduces identically. Three saves of one view name under org_alpha / org_beta / env-wide through the real saveMetaItem path, then one auditMetaItem({ type, name }):

auditMetaItem() ACTORS: [ 'package-installer', 'bob@beta.example', 'alice@alpha.example' ]

Three rows, three organizations, one caller — plus each row's actor, note, lock_state, code, operation, source and request_id.

#8777 did not move the target state: it changed plugin-audit's sys_audit_log stamping, whereas sys_metadata_audit rows are stamped by metadata-protocol's own recordMetadataAudit (organization_id: entry.organizationId ?? null), which it did not touch.

The fix

1. The query now builds the described filter (packages/metadata-protocol/src/protocol.ts, auditMetaItem): the caller's organization plus env-wide (organization_id IS NULL) rows, and nothing else. An unresolved organization is fail-closed onto the env-wide rows via ?? null — the same normalization the sibling /published door applies, and symmetric with what an org-less write produces — so omitting the parameter is no longer a skeleton key.

The scope is unconditional: it does not depend on a posture, a principal, or a layer below choosing to act. That is the point of the ruling, and it is why the three "compensated lower down" candidates were eliminated by measurement rather than by reading — the driver's tenant wall is armed only from an execution context this read never passed; plugin-security's Layer 0 short-circuits on a principal-less call long before the field gate that would have carried it; and no tenancy posture supplies the scope either.

2. The REST route supplies the organization (packages/rest/src/rest-server.ts) from resolveExecCtx, which this file already calls in 40-plus handlers including the PUT /meta twin. computeExecCtx assembles tenantId from the shared resolveAuthzContext (an API key's principal tenant, else the session's activeOrganizationId).

No new seam. The PM flagged rest-server.ts's /published comment, which records that packages/rest carries no resolveActiveOrganizationId and no org plumbing, and that inventing it under a bug fix would be "a new seam smuggled in under a bug fix". That constraint is respected: this reads a field the execution context already carries, and adds no organization-resolution plumbing.

3. Swept in the same diff: the call no longer passes environmentId, which the request type never declared and the body never read. Environment scoping is unaffected — it comes from which protocol resolveProtocol hands back, not from the request payload.

Why the env-wide limb is load-bearing, not garnish

Measured: the REST PUT /meta/:type/:name door passes noorganizationId, so every audit row it writes is stamped organization_id: null. An equality-only filter would have looked correct in any shape assertion and blanked the audit tab on every REST-authored deployment. "Correctly scoped" and "hides everything" are different behaviours, and the tests pin them apart.

Tests — both directions, three layers

  • packages/runtime/src/audit-meta-item-org-scope.integration.test.ts (7 cases) — real ObjectQL + real SqlDriver (sqlite), three organizations on one (type, name) seeded through the real write path. Own-org and env-wide rows visible; a third organization's not; symmetry for the second org; an org with no rows of its own still sees env-wide; org-less and explicit-null reads are env-wide only.
  • packages/metadata-protocol/src/protocol.audit-org-scope.test.ts (7 cases) — the query shape for every call spelling, plus a guard that the comment's claims and the code backing them cannot drift apart again.
  • packages/rest/src/rest-server-audit-org-scope.test.ts (6 cases) — the route threads the organization, is fail-closed without one, and no longer passes environmentId.

Reverse verification, direction predicted before running: ablating the scope turns 6 of 7 integration cases red and leaves exactly 1 green — the seeding precondition, which asserts only write-side stamps and is read-independent. Observed exactly that. Because this suite resolves @objectstack/metadata-protocol from its built dist (runtime's vitest aliases other packages to src, not this one), each leg was mutate → rebuild → prove the marker's presence/absence in dist → run; the fix was restored from its commit and confirmed byte-identical, and dist rebuilt so no ablated artifact survives.

One assumption was corrected by measurement and the test now pins the real behaviour: an unresolvable execution context never reaches the read at all — the anonymous floor refuses it with a 401 first. That floor is the route's only gate; it carries no capability gate, unlike the PUT twin's manage_metadata, which is exactly why the organization scope has to do the tenant separation.

Verification, at HEAD = 22c032dd9

All of the below were run after the final commit (the origin/main merge), against that tree.

  • pnpm --filter @objectstack/metadata-protocol test92 files, 1376 tests, all passing
  • pnpm --filter @objectstack/rest test118 files, 1948 tests, all passing
  • pnpm --filter @objectstack/runtime test158 files, 2400 tests, all passing
  • pnpm --filter @objectstack/rest --filter @objectstack/runtime typecheck — clean. (metadata-protocol declares no typecheck script; stated rather than implied.)

Gate families re-derived against the actual changed paths with node scripts/pm/dispatch-gates.mjs, all green: check:nul-bytes, check:authz-resolver, check:route-envelope, check:filter-alias-parity, check:durability-log-level, check:cross-package-test-inputs, check:query-options-erasure, check:type-check-coverage, check:changeset-gate-self-tests, check:objectui-changeset, plus check-adr-0087-registration, check-changeset-no-major, check-empty-changeset.

check:type-check-debt (the ratchet half) was run properly rather than skipped — full workspace closure built first, as lint.yml does — and reports "none above its recorded number". The -1 surplus it notes on @objectstack/lint is pre-existing and in a package this diff does not touch.

Scope

Region-declared work: auditMetaItem only in protocol.ts (re-derived from the merged tree at :6097; siblings searchAll:8603 and diffMetaItem:15400 are untouched and thousands of lines away), plus the audit handler in rest-server.ts and the co-located pins. main was merged before opening this PR. Nothing unrelated is folded in, and the write path is untouched — that is #8707's lane.


Generated by Claude Code

… organization (#8747)
auditMetaItem declared organizationId and never read it, while the comment
above the query described the org scope it would have built. Measured: one
context-less read returned three organizations' rows for one (type, name).
Build the $or the comment describes — the specific organization plus env-wide
(organization_id IS NULL) rows — and thread the organization from the REST
route via the resolveExecCtx this file already calls in 40+ handlers. Also
drops the dead environmentId from the same call.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8
…on scope in both directions (#8747)
Three pins, each covering what the others cannot:
- runtime integration test: real ObjectQL + real SqlDriver, three orgs on one
(type, name). Own-org AND env-wide rows visible, a third org's not. The
env-wide case is the discriminating control that separates 'correctly scoped'
from 'hides everything'.
- metadata-protocol: the query shape, plus a guard that the comment's claims
and the code that backs them cannot drift apart again.
- rest: the route threads the organization, is fail-closed without one, and no
longer passes the dead environmentId.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8
@vercel

vercelBot commented Aug 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 15, 2026 3:30am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/metadata-protocol, @objectstack/rest.

11 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/ai/connect-mcp.mdx(via @objectstack/rest)
  • content/docs/api/error-handling-server.mdx(via @objectstack/rest)
  • content/docs/api/index.mdx(via @objectstack/rest)
  • content/docs/concepts/metadata-lifecycle.mdx(via @objectstack/metadata-protocol)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/metadata-protocol)
  • content/docs/permissions/authentication.mdx(via @objectstack/rest)
  • content/docs/permissions/system-context.mdx(via packages/rest)
  • content/docs/plugins/index.mdx(via @objectstack/rest)
  • content/docs/plugins/packages.mdx(via @objectstack/rest)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/metadata-protocol, @objectstack/rest)
  • content/docs/protocol/kernel/i18n-standard.mdx(via packages/rest)

4 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/rest)
  • content/docs/releases/v12.mdx(via @objectstack/rest)
  • content/docs/releases/v17.mdx(via @objectstack/rest)
  • content/docs/releases/v9.mdx(via @objectstack/metadata-protocol)

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.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — ✅ the fix is right; ⚠️ one composition finding filed as #8805. Not a blocker.

PM session session_01XeQRiAa7vYRVX5Fog7Zby8, round 5. Reviewing against the ruling in #8747 comment 5299824146.

What I verified rather than took on trust

The :6731 obstacle is genuinely cleared, not argued around. I flagged at dispatch that option A walks into the recorded precedent that packages/rest carries no org plumbing and that inventing it "would be a new seam smuggled in under a bug fix". Measured on origin/main: organizationId occurs exactly twice in the whole of rest-server.ts, and both occurrences are inside that very comment (:6731, :6741) — there is no executable organizationId in the file. So reading tenantId off the execution context resolveExecCtx already assembles adds nothing the precedent forbids. Assumption confirmed by measurement, which is what I asked for.

The env-wide limb is load-bearing and the PR is right to insist on it. An equality-only filter would have passed any "is it scoped?" shape assertion and blanked the audit tab on every REST-authored deployment. Pinning "correctly scoped" apart from "hides everything" is the discriminating control that makes the green mean something.

The dist trap was handled correctly — marker counted 1 / 0 / 1 across fix → ablation → restore in the built artifact, with the restore proven byte-identical by an empty git diff HEAD. That is the trap that produced false-green ablations elsewhere tonight; naming the alias asymmetry (runtime's vitest aliases core/rest/spec/types/platform-objects to src but not metadata-protocol) is what makes the claim checkable.

⚠️#8805 — what this PR does NOT close, composed from three measured facts

Filed separately because it is only visible when the read filter is composed with the write stamping, which lives in another package and another card's lane:

  1. organizationId in rest-server.ts is comment-only ⇒ the REST metadata write door passes no organization.
  2. recordMetadataAudit (protocol.ts:10319) stamps organization_id: entry.organizationId ?? null ⇒ every REST-authored write lands null.
  3. This PR's read correctly returns own-org plus env-wide (null).

Compose them: a caller in org_alpha sees org_alpha's rows and every null-stamped row — so on a REST-authored deployment, every tenant still sees every REST-authored audit row.

This is not an argument against the PR and not a defect in it. The read half is correct, the ruling is implemented, and the fix is a strict improvement: rows carrying an explicit organization are now properly separated, and the read is fail-closed rather than a skeleton key. The residue is entirely on the write side, which this card explicitly excluded as #8707's lane and which the dev correctly stayed out of.

What #8805 asks, and deliberately does not answer: is a REST-authored metadata write supposed to be env-wide? If that door is an environment-admin surface, null is correct and there is nothing to fix. If a tenant admin holding manage_metadata can author for their own organization through it, the row belongs to that organization and the stamping is a write-side defect. The door already resolves a context carrying tenantId, so the information is present — but whether it should be used is a scoping decision, not an inference from availability. ⛔ I did not measure which it is and did not guess.

Docs

The drift check flagged 11 pages. Checked and no action — no page documents the audit endpoint's tenancy behaviour, so nothing contradicts the narrowing. (Reverse-controlled: the search instrument was validated against a term known to be present before any zero was trusted.)

Landing

Gate-bearing jobs will be read job-by-job, not off an aggregate, and landing confirmed on both readings — the queue/merge timeline event and the artifact on origin/main.


Generated by Claude Code

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@hotlong@claude