Refs #12163

Authored by Opus 4.8 (Claude Code). Session af888fcb-6a68-45bd-8a34-13be477733b1.

FAIR-band: in-band [8/30]

Evidence: L2 (the leak-closure is unit-verified — KnowledgeBase.TenantIsolation.spec.mjs exercises the real QueryService/DocumentService read paths through an in-memory Chroma stand-in that faithfully evaluates $and/$or/$ne). Residual: owner read-back of private content over the OIDC/proxy transport is not delivered here — see "## Residual" below (tracked as #12296). This PR ships the read-time leak-closure + fail-safe; it does not close #12163 (hence Refs, not Resolves).

Why this is needed (security)

KB reads filtered on tenantId + type only. The visibility field is stamped on every chunk at write time (VectorService, default 'team', optionally 'private') but was never consulted on any read path — so visibility: 'private' was inert: any same-tenant requester could read it. Benign while each user is their own tenant, but live-relevant the moment a tenant is shared across users — the cloud deployment's shared default-tenant tier (NEO_KB_DEFAULT_TENANT_ID) collapses many users into one tenant, so a private chunk leaked to every reader of that tenant. This closes that leak.

What changed

• ai/services/knowledge-base/readVisibilityFilter.mjs (new): one source of truth for the read-side where clause, consumed by both read services so the two layers can't drift between call sites. It combines, via $and: the tenant filter (own tenant + neo-shared) and the visibility gate ({$or: [{visibility: {$ne: 'private'}}, {originAgentIdentity: <requester>}]}).
• Ownership is matched on getAgentIdentityNodeId() — the writer's agent-identity node id, which is what VectorService stamps as originAgentIdentity, not the tenant id (in a shared tenant many users share one tenant but keep distinct identities).
• Fail-safe: a private chunk whose owner is absent matches no requester and stays hidden; when the requester has no agent identity, the ownership branch is dropped entirely (they see only non-private). This null-owner policy intentionally diverges from Memory Core's RLS (GraphService.isRlsVisible): the non-null-owner match is similar in shape, but Memory Core treats a null owner as visible whereas this fails closed.
• Offline / single-tenant (no request context) → no filter, byte-equivalent with pre-isolation behavior.
• Wired QueryService.queryDocuments + both DocumentService read paths to the helper.
• Security.md: updated to describe the now-enforced tenant + visibility layers and the OIDC owner-read-back limitation (below).

Residual — owner read-back over OIDC/proxy is deferred (#12296)

Owner matching keys on getAgentIdentityNodeId(), which is populated only in stdio / env / gh-cli contexts. The OIDC/proxy transport a cloud deployment uses resolves a userId but no agent-identity node id (TransportService.mjs proxy path; AuthService.mjs OIDC path). So in a cloud deployment a private chunk has no resolvable owner on read and fails safe — hidden from everyone, including its writer.

Net: this PR closes the cross-user leak (#12163's core threat — no other user sees a private chunk) but does not yet deliver owner read-back of private content in a cloud deployment. Restoring that requires keying ownership on the transport-populated userId — tracked as #12296. Because #12163's owner-read-back acceptance criterion is therefore not fully met over OIDC, this PR uses Refs #12163 (not Resolves) so the ticket stays open until #12296 lands.

Contract Ledger

The KB read filter (QueryService.queryDocuments, DocumentService getters) adds a visibility $or gate; offline/no-auth → no filter (unchanged); Security.md made accurate including the OIDC owner-read-back limitation. Drift vs the #12163 ledger row: the row's "private returned to its originAgentIdentity" holds where an agent identity is present (stdio) but is inert over OIDC — captured as residual #12296 rather than claimed as delivered.

Deltas from ticket

• Visibility logic factored into a shared pure helper rather than inlined per call site — DRY + single security source of truth.
• The owner-read-back limitation over OIDC was discovered during integration (the dockerized specs run that transport); split out as #12296 rather than folding an owner-key model change into this leak-closure PR.

Test Evidence

• KnowledgeBase.TenantIsolation.spec.mjs → 16 passing (tenant cases updated to {$and:[tenant, visibility]}; spy extended for $and/$or/$ne; case 9 = private hidden from same-tenant non-owner + returned to owner; case 10 = ownerless private hidden from everyone, fail-safe).
• Dockerized integration specs (KBTeamPrivateRetrieval, KBCrossTenantIsolation) reframed to the tenant axis (team visibility) — the owner axis is inert over their OIDC transport, so it is unit-covered. integration-unified is green.
• node --check + git diff --check clean.

Post-Merge Validation

• Shared-default-tenant deployment: user A writes a private chunk; user B (same tenant, different identity) queries → does not see it (leak closed).
• Known interim limitation (until #12296): user A also cannot read their own private chunk back over OIDC (fail-safe). team / neo-shared content unaffected.

Root-cause: the integration failure is a real architectural gap, not a fixture bug

@neo-gpt and I both first read the two RED specs (KBTeamPrivateRetrieval, KBCrossTenantIsolation) as "fixtures seed private without originAgentIdentity." That symptom is real, but the proposed fix (stamp originAgentIdentity on the fixture records) would not make them pass — and chasing it would paper over a genuine gap. Evidence below.

The owner key is structurally null in the transport these tests use

Both read and write scope ownership on getAgentIdentityNodeId():

• Read: readVisibilityFilter.mjs → requesterAgentId = RequestContextService.getAgentIdentityNodeId(); when null, the ownership $or branch is dropped and the clause collapses to {visibility: {$ne: 'private'}}.
• Write: KnowledgeBaseIngestionService.mjs:768 → originAgentIdentity: this.requestContextService.getAgentIdentityNodeId?.() || payload.originAgentIdentity.

But agentIdentityNodeId is only populated in the stdio / env-var / gh-cli (local-agent) contexts. The OIDC / proxy transport these integration specs exercise sets only userId / username, never agentIdentityNodeId:

• TransportService.mjs:58-62 — proxy path: userId: proxyUserId, username: proxyUserId (from x-preferred-username). No agent id.
• AuthService.mjs:167-176 — OIDC path: userId = preferred_username || sub. No agent id.

Consequence in the test (and in a real cloud OIDC deployment): the requester's getAgentIdentityNodeId() is null → ownership branch dropped → a private chunk is returned to nobody, including its writer. Stamping originAgentIdentity on the fixture does not help, because the read side has no agent id to match it against.

This is not a divergence from the MC precedent on the key — it's a divergence on null-owner policy

The ticket cites Memory Core's GraphService $or as the precedent. Reading the code (not the ticket prose): MC keys ownership on getAgentIdentityNodeId() too (GraphService.mjs:719) — same key. The real difference is the null-owner policy:

• MC (GraphService.mjs:721 + isRlsVisible :21-33): ... OR user_id IS NULL OR ... visibility = 'team' → null-owner is visible to everyone.
• #12290: null-owner private is hidden from everyone (fail-safe).

So both substrates key on an identity that is null in OIDC/proxy. That means per-user private scoping within a shared tenant — the exact threat #12163 names ("shared default tenant, many users resolve to one tenant") — is unenforceable as currently keyed, under either null-owner policy:

• null-owner hidden (#12290): secure (no leak) but over-restrictive — the writer can't read their own private back in OIDC (write-only black hole). ⇒ the two specs fail.
• null-owner visible (MC-aligned): specs pass and the writer can read back — but every OIDC user has a null owner, so null-owner-visible means a private chunk leaks to all same-tenant users. The leak #12163 set out to close is not closed in the OIDC shared-tenant case.

The only identity populated per-user in OIDC is userId (x-preferred-username / preferred_username). Real per-user private scoping in a shared tenant therefore requires keying ownership on userId (or wiring an agent-identity through the OIDC path) — a security-model decision that spans both KB and MC and exceeds this PR's scope.

Proposed resolution for #12290 (secure + honest + incremental)

1. Keep the fail-safe (null-owner private hidden). It is the secure choice; null-owner-visible would reopen the cross-user leak in the shared-tenant OIDC case.
2. Make the integration specs assert the true behavior: the "a tenant reads back its own content" property belongs to the team tier (visible to same-tenant, the correct tenant-isolation assertion); a private chunk with no matching owner is hidden in OIDC (fail-safe). The specs currently encode the pre-#12163 "tenant == owner" model.
3. Document the limitation in Security.md: owner-scoped private read-back requires a populated owner key, which the OIDC/proxy transport does not yet provide; interim posture is fail-safe.
4. Route the owner-key decision to Ideation (cross-substrate KB+MC, high-blast): owner key = userId (populated in OIDC, distinguishes users in a shared tenant) vs. wiring agentIdentityNodeId through the OIDC/proxy path.

This lands #12290 as a secure, honest, incremental improvement (closes the read-time leak on the agent-id-populated paths; fail-safe everywhere else) with the real gap tracked rather than silently shipped — instead of a fixture patch that would encode a false "owner == tenant" model.

Evidence: TransportService.mjs:58-62, AuthService.mjs:167-176, KnowledgeBaseIngestionService.mjs:768, readVisibilityFilter.mjs, GraphService.mjs:719,721,21-33.

— @neo-opus-ada

@neo-opus-ada commented on 2026-06-01T08:05:03Z

Integration CI fix (commit 10aefdf99) + the owner-key follow-up

Root cause (per the earlier root-cause comment): the two failing dockerized specs (KBTeamPrivateRetrieval, KBCrossTenantIsolation) seeded visibility: 'private' (ownerless) and expected the owning tenant to read it back. The new read-side fail-safe correctly refuses ownerless private to non-owners — and these specs run over the OIDC/proxy transport, which populates a tenant/userId but no agent-identity node id, so the per-chunk owner axis (originAgentIdentity) is structurally inert end-to-end and cannot be tested in this transport.

Fix (test-only; the read-side security clause is unchanged): reframed both specs to the tenant axis (team visibility) — same-tenant retrieval + cross-tenant isolation, which IS testable in OIDC. The per-chunk private owner-scoping axis stays covered by the unit suite (KnowledgeBase.TenantIsolation.spec.mjs), where an agent identity can be set.

What this PR ships: the read-time leak is closed (ownerless private fails safe; team/shared stay tenant-visible). What it does not yet deliver: in an OIDC deployment, a private chunk's owner cannot read it back (owner-scoping keys on agentIdentityNodeId, which OIDC doesn't populate). That's fail-safe (secure, over-restrictive), not a leak.

Follow-up fork (security-model decision, operator's call): to make owner read-back work in OIDC, the owner key would move from agentIdentityNodeId → userId (the per-user identity OIDC does populate, which also distinguishes users within a shared tenant — exactly the #12163 threat scenario). That's a read+write owner-key change beyond this PR's scope; flagging for a separate decision rather than folding a model change into the CI fix.

— @neo-opus-ada

@neo-opus-ada commented on 2026-06-01T08:36:49Z

Closing — wrong primitive for the KB (operator direction, 2026-06-01).

Owner-vs-team visibility (mirroring Memory Core's 1-bit privacy axis) is the wrong primitive for the Knowledge Base — confirmed by Discussion #12161, whose 2026-05-29 reframe explicitly states the original #12163 visibility draft is the wrong decomposition; the real requirement is tiered, entitlement-based access, scheduled after v13.

The KB is tenant-repo-based: tenant isolation (#11632, already in dev) is the correct + sufficient read-side model. Per-user owner-scoping locks out KB users who should see their tenant's content — and worse, it fails safe to hidden-from-everyone over the OIDC/proxy transport, which would damage live cloud deployments. dev already has the correct tenant-isolation-only behavior; nothing here should ship. Superseded by #12161 (entitlements, post-v13).

My error: I drove a ticket whose own successor Discussion (#12161, which I authored) had already identified as the wrong primitive + deferred. Closing.