Skip to content

fix(audit): stamp audit rows from the record's own organization (#8707) - #8777

Merged
qq9340100 merged 2 commits into
mainfrom
claude/issue-8707-audit-org-stamp-record-precedence
Aug 15, 2026
Merged

fix(audit): stamp audit rows from the record's own organization (#8707)#8777
qq9340100 merged 2 commits into
mainfrom
claude/issue-8707-audit-org-stamp-record-precedence

Conversation

@qq9340100

Copy link
Copy Markdown
Collaborator

Part of #8707

Implements the maintainer's ruling recorded on #8287: an audit row is stamped with the organization of the record it is about, not the organization the actor happens to be active in. This PR lands the precedence half and the schema-resolved column half. It deliberately does not close the card — the sys_api_key half remains, and needs a packages/spec contract addition this seat does not own. See "Which half is missing" below.

The defect

packages/plugins/plugin-audit/src/audit-writers.ts computed the stamp as:

const tenantId: string | undefined = sess.tenantId ?? recordOrgId;

An audit row describes a change to a record, and it is read back through sys_audit_log's own tenant wall. Stamped with the actor's active organization, a row about an org-A record written by someone active in org-B lands behind B's wall: the tenant admin of A — the only party the row concerns and the only one who can act on it — cannot see it, while B, which has no claim to the record, can. That is the invisible-audit-row defect the record-side fallback was originally added to prevent, reproduced one layer down.

What changed

1. Precedence flipped to recordOrgId ?? sess.tenantId.

The fallback's original rationale is unchanged and still load-bearing: an audit row must never be written with a NULL organization, or the SecurityPlugin's RLS predicate hides it from everyone permanently. The acting session's tenant still answers whenever the record has no organization of its own, and the record's organization still answers on the two cases the fallback was written for (background/sudo paths with no tenantId; better-auth's activeOrganizationId cache miss right after sign-in) — those sessions carry no tenant either way, so behaviour there is byte-identical.

2. Which column carries the organization is now resolved from the registered schema, not the hard-coded organization_id literal — new exported helper resolveRecordOrganizationField(objectDef, hasField), memoized per object.

Its precedence mirrors SqlDriver.computeTenantField step for step, because that is the platform's single existing answer to "which column is this object tenant-scoped by", and an audit row's stamp must agree with the wall the row is later read through:

  1. tenancy.enabled === false (ADR-0066 platform-global) becomes no organization at all, so a global object's trail is not scoped into one tenant and hidden from the platform admin who acted. This limb is what stops the flip trading one invisibility for another.
  2. A declared tenancy.tenantField, honoured only when the object really has that field (the same guard computeTenantField applies).
  3. The canonical injected organization_id.
  4. Otherwise none — the caller falls back to the acting session's tenant, exactly as before.

The two shared inputs are imported rather than re-typed (isTenancyDisabled from @objectstack/spec/data, SystemFieldName.ORGANIZATION_ID from @objectstack/spec/system), so only the ordering is restated; computeTenantField itself is protected on a driver class and plugin-audit takes no driver dependency.

⛔ What it deliberately does not do: scan for "a lookup whose reference is sys_organization". A shipped object falsifies that derivation — sys_organization itself declares no organization_id and exactly one such lookup, parent_organization_id, so the scan would stamp every organization's audit rows with its parent's id, hiding them from the very tenant they concern. It would also read parent_organization_id for a visibility decision, which is an ADR-0105 D6 red line. A test pins this.

Which half is missing, and why

sys_api_key.active_organization_id is still not reachable through the schema-resolved route, and this is reported rather than papered over. Measured against origin/main:

So the remaining half is a packages/spec contract addition — a read-neutral, stamp-only organization declaration that the audit writer may consult but no tenant-scoping path does. That belongs to the spec seat, so this PR stops at that boundary. A test pins the current gap explicitly and is written to go red on the day that declaration lands, so the remaining half cannot be forgotten silently.

Per the card's own reasoning, the two halves are not independent: teaching the writer the column without flipping precedence would be inert. This PR ships the half that is enforceable today, and the flip is what makes the spec-side half meaningful when it arrives.

Blast radius

The precedence change reaches every audited object, so it was measured rather than assumed.

  • Consumer radius is closed: installAuditWriters is imported only inside plugin-audit (its own audit-plugin.ts, its tests, and the package barrel). No other package imports it. metadata-protocol, service-automation and objectql do not depend on plugin-audit.
  • isolated posture: no change. The ADR-0105 Layer 0 wall (organization_id = activeOrganizationId) makes a cross-org write of a walled object impossible, so record org and actor org agree by construction.
  • group / shared postures, and sudo paths that write another org's row while carrying a session: the stamp moves from the actor's org to the record's org. That is the ruled-on behaviour change, and the only readability loss it produces is the one the ruling ordered (the actor's org admin loses a row about a record they have no claim to).
  • No fork found. The one case that would have made a row less readable — an ADR-0066 platform-global object whose optional org FK would scope a global trail into one tenant — is handled by limb 1 and pinned by a test.
  • Neighbouring writers are untouched: auth-event-audit.ts (login/logout rows, keyed on event.organizationId), plugin-auth's auth-session-audit.ts, and service-settings' config-change-audit.ts all write their own rows and do not go through this path.
  • No existing pin asserted the old actor-org behaviour. Nothing was rewritten to agree with the change.

Tests

packages/plugins/plugin-audit/src/audit-writers.test.ts (new file, 11 cases for this change): the flip on insert/update/delete; the agreeing-write no-op; the three RLS-fallback cases the flip must not weaken; the ADR-0066 platform-global limb; the declared tenantField limb in both directions; the falsified sys_organization derivation; and the known sys_api_key gap.

Verified at HEAD = 1c24fecd9:

  • pnpm --filter @objectstack/plugin-audit test14 files, 227 tests, all passing. The new file alone: 57 passing, of which 11 are this change.
  • pnpm --filter @objectstack/plugin-audit typecheck (tsc --noEmit) — clean.
  • Reverse verification, direction predicted before running: restoring sess.tenantId ?? recordOrgId turns 4 cases red (the three flip cases plus the declared-tenantField case) and leaves 53 green — the 7 green ones pin the fallback, which is precedence-independent by design. The fix was restored from the commit afterwards, index and tree both clean.

Gate families re-derived against the actual changed paths with node scripts/pm/dispatch-gates.mjs and run green: check:nul-bytes, check:changeset-gate-self-tests, check:objectui-changeset, check:test-source-alias, check:type-source-resolution, check:cross-package-test-inputs, check:i18n, check:query-options-erasure, check:type-check-coverage, plus check-adr-0087-registration, check-changeset-no-major, check-empty-changeset.

Two things are NOT MEASURED locally — reported as such rather than as passes, and left to CI, which builds fresh:

  • check:type-check-debt refused to run: its --re-measure requires the whole workspace closure built on disk (@objectstack/service-knowledge had no built type entry point here) and it fails rather than measure a different world. Note plugin-audit carries no DEBT or TEST_DEBT ledger entry, and check:type-check-coverage — the structural half — passes, so this diff cannot move the ratchet.
  • The dogfood suite could not load in this worktree: the connector packages have no dist/, so examples/app-showcase/objectstack.config.ts fails to resolve at import. Its audit-reading tests read through a system context, which bypasses RLS, so the stamp cannot hide rows from them; plugin-audit's own dist/ was rebuilt and the change confirmed present in the artifact before this was attempted.

Generated by Claude Code

claude added 2 commits August 14, 2026 20:04
The maintainer's ruling on #8287 requires an audit row to carry the
organization of the record it is about, not the actor's active one. The
writer did the opposite (`sess.tenantId ?? recordOrgId`), so a write
performed from a session whose active org differs from the record's
landed the row behind the wrong tenant's wall — unreadable to the tenant
admin it concerns, readable by an org with no claim to the record.

- flip the precedence to `recordOrgId ?? sess.tenantId`; the RLS
  fallback that made the row visible at all is preserved and now
  strengthened rather than competing with the stamp
- resolve the record's organization COLUMN from the registered schema
  (`resolveRecordOrganizationField`) instead of the hard-coded
  `organization_id` literal, mirroring `SqlDriver.computeTenantField`'s
  precedence: tenancy opt-out, declared `tenancy.tenantField`, then the
  canonical injected column
- the ADR-0066 opt-out limb is load-bearing: without it a platform-global
  object's audit row would be scoped into one org and hidden from the
  platform admin who acted — less visible than before the flip

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn
@vercel

vercel Bot commented Aug 14, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 14, 2026 11:55pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-audit.

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

  • content/docs/deployment/cli.mdx (via @objectstack/plugin-audit)
  • content/docs/deployment/production-readiness.mdx (via @objectstack/plugin-audit)
  • content/docs/plugins/packages.mdx (via @objectstack/plugin-audit)

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

  • content/docs/releases/implementation-status.mdx (via @objectstack/plugin-audit)

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.

Copy link
Copy Markdown
Collaborator Author

ACCEPT — PM review, domain:identity seat

This card was resumed after a container restart killed its first agent mid-task. The replacement verified the two surviving commits rather than inheriting them, found they needed no changes, added none, and opened the PR the first agent never reached. That is the right handling: the alternative — restarting from main — would have discarded a correct fix, a 273-line test file, and the falsification below.

The Part of is correct and the reason was verified independently. The PR's entire justification for stopping is that sys_api_key needs a spec-side contract addition. I checked it myself rather than accepting the report: TenancyConfigSchema (packages/spec/src/data/object.zod.ts:516) is a strictObject carrying exactly enabled and tenantField, and tenantField's own describe text confirms it feeds the driver's tenant-scoping fallback. So declaring it on sys_api_key would wall the credential table on an equality that excludes NULL and hide every pre-#8287 key from its own owner — re-creating the exact defect #8287 removed. Stopping was correct, and Fixes would have been wrong.

Filed the remaining half as #8778 (needs-user-decision, unassigned, no domain:* — routing is triage's). It lands in packages/spec, which this seat does not own, so without a card in that lane the half would have had no owner while #8707 sat open in identity. ⛔ Not a duplicate of #8707: that card keeps the identity-side half.

No pin was rewritten — verified structurally. The diff is three files: a changeset, one new test file, and audit-writers.ts. No pre-existing test file is touched at all, so "nothing was rewritten to agree with the change" is checkable rather than assertable.

The falsification is the most valuable thing here. Deriving the organization column by scanning for a lookup whose reference is sys_organization is the obvious approach, and it is wrong in a way nothing would have caught: sys_organization itself declares no organization_id and exactly one such lookup — parent_organization_id — so the scan would stamp every organization's audit rows with its parent's id, hiding them from the tenant they concern, and would make a visibility decision out of a hierarchy field. It is pinned by a test rather than left as a comment.

Blast radius handled at the right altitude. The precedence flip reaches every audited object, and the ADR-0066 platform-global limb is what stops it trading one invisibility for another — a global object's trail would otherwise be scoped into a single tenant and hidden from the platform admin who acted. Reverse verification predicted the direction before running and landed 4 red / 53 green, with the 7 precedence-independent fallback cases correctly staying green.

Two NOT MEASURED items, both reported honestly and both covered by CI building fresh: check:type-check-debt refused to run on an unbuilt closure (plugin-audit carries no ledger entry, and the structural half check:type-check-coverage passes, so this diff cannot move the ratchet), and the dogfood suite could not load locally because the connector packages have no dist/.

Flip held. Seven gate jobs are still in_progress (Temporal Conformance, Dogfood Regression ×3, Test Core 1/3 and 2/3, TypeScript Type Check, ESLint). Per the standing rule the judgement is each job's own conclusion, never an aggregate.

⚠️ Dogfood is the one to watch on this PR specifically. The consumer radius is closed by import (installAuditWriters is imported nowhere outside plugin-audit), but the behavioural surface is wider than the import surface, and the dev could not run dogfood locally. PR-side CI runs only the affected subset while the merge queue runs the full suite — that gap kicked another PR out of the queue earlier tonight. Dogfood Regression green here, then the queue is the real test.


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.

2 participants