Skip to content

Exact-path reads run under the caller's identity, so gating applies - #819

Merged
rbuergi merged 1 commit into
mainfrom
fix/mcp-get-enforces-read-permission
Aug 5, 2026
Merged

rbuergi merged 1 commit into
mainfrom
fix/mcp-get-enforces-read-permission

Conversation

@rbuergi

@rbuergi rbuergi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The hole

A signed-in user with no entitlement could read gated PAID content by exact path.

Measured on memex 2026-08-05: get @AgenticPrimerDe/02-CodeWunsch returned 79,650 characters of a paid course lesson to a user with no grant and no entitlement — while the gating data was entirely correct:

Public|AgenticPrimerDe                |Read|t   ← cover allow
Public|AgenticPrimerDe/02-CodeWunsch  |Read|f   ← DENY, present and correct

The SQL query path (GenerateAccessControlClause) hid the same node. Only the single-node read leaked it — which is why this had been filed on 2026-07-29 as an operational quirk: "children vanish from listings but direct get by path still works". That clause was the hole.

Cause

MeshNodeStreamCache.GetStreamRaw reads AccessService.Context at subscribe time to decide whether to apply its per-user read gate, and deliberately passes through to the shared system-identity upstream when that is null (background / hub-principal flows).

AccessContext is an AsyncLocal and is cleared across .Subscribe hops — the same reason ReadNodeOrUnified already captures it for the PII projection. So a lost identity did not fail closed: it read everything.

Fix

FetchNode captures the caller synchronously and re-establishes it (SwitchAccessContext) around the subscribe, so the existing gate applies. No second access model; it cannot lock out the flows the gate intentionally passes through, because a null capture switches to null — exactly today's behaviour. A denial is logged distinctly and collapses to "Not found", which does not disclose that a paid node exists at the path.

Tests

GatedNodeExactPathReadTests — the visitor is denied, the entitled buyer still reads the same node, and the cover stays public. (A paywall that denies everyone is not a fix; the middle case is what catches an over-strict gate — and it did, on my first attempt.)

Each case seeds its own plugin: the methods share a mesh, so a shared path makes the second CreateNode complete without emitting, and a deny-assertion would pass vacuously against a node that was never created.

⚠️ Honest limit — please read before merging

These tests pass with AND without the fix. The monolith harness sets the identity on the calling thread and no Rx hop clears it, so it does not reproduce the loss that happens in the deployed MCP pipeline. They pin the paywall's required behaviour, not this specific regression.

So the fix is unproven by CI. It is verified against production after deploy by re-running the repro: as an account with no entitlement, get @AgenticPrimerDe/02-CodeWunsch must be denied. I'll run exactly that once the image rolls and report the result.

The change is low-risk in the meantime: it only supplies an identity where one was absent.

🤖 Generated with Claude Code

A signed-in user with no entitlement could read gated PAID content by exact
path. Measured on memex 2026-08-05: `get @AgenticPrimerDe/02-CodeWunsch`
returned 79,650 characters of a paid course lesson to a user with no grant and
no entitlement — while the gating data was entirely correct
(`Public|…/02-CodeWunsch|Read|is_allow=false` present) and the SQL query path hid
the same node. Only the single-node read leaked it, which is why it had been
filed on 2026-07-29 as an operational quirk: "children vanish from listings but
direct get by path still works". That clause was the hole.

MeshNodeStreamCache.GetStreamRaw reads AccessService.Context AT SUBSCRIBE TIME to
decide whether to apply its per-user read gate, and deliberately passes through
to the shared system-identity upstream when that is null (background /
hub-principal flows). AccessContext is an AsyncLocal and is cleared across
.Subscribe hops — the same reason ReadNodeOrUnified already captures it for the
PII projection — so a lost identity did not fail closed: it read everything.

FetchNode now captures the caller synchronously and re-establishes it
(SwitchAccessContext) around the subscribe, so the EXISTING gate applies. This
adds no second access model and cannot lock out the flows the gate intentionally
passes through: a null capture switches to null, exactly today's behaviour. A
denial is logged distinctly and collapses to "Not found", which does not disclose
that a paid node exists at the path.

Tests (GatedNodeExactPathReadTests): the visitor is denied, the entitled buyer
still reads the same node, and the cover stays public — a paywall that denies
everyone is not a fix. Each case seeds its OWN plugin: the methods share a mesh,
so a shared path makes the second CreateNode complete without emitting and a
deny-assertion would pass VACUOUSLY against a node that was never created.

⚠️ HONEST LIMIT: these tests pass with AND without the production fix. The
monolith harness sets the identity on the calling thread and no Rx hop clears it,
so it does not reproduce the loss that happens in the deployed MCP pipeline. The
tests therefore pin the paywall's REQUIRED behaviour, not the specific
regression; the fix itself is verified against production after deploy by
re-running the repro above.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 5, 2026 15:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a security gap where exact-path single-node reads could bypass row-level access gating when AccessService.Context was lost across Rx subscription boundaries, allowing unauthorised users to read gated/paid content by path even though query-based listings were correctly filtered.

Changes:

  • Capture the caller identity in MeshOperations.FetchNode and re-establish it around the GetMeshNodeStream(...).Subscribe(...) so MeshNodeStreamCache’s existing per-user Read gate applies.
  • Collapse access-denied reads to null (surfacing as “Not found” to callers) while distinguishing denial vs routing failures in logs.
  • Add PostgreSQL integration tests that pin expected paywall behavior for exact-path reads (visitor denied, buyer allowed, cover remains public).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
test/MeshWeaver.Hosting.PostgreSql.Test/GatedNodeExactPathReadTests.cs Adds integration tests asserting exact-path reads respect gating semantics (deny visitor, allow buyer, keep cover public).
src/MeshWeaver.AI/MeshOperations.cs Captures/restores AccessContext around exact-path read subscription so the stream-cache read gate runs under the caller’s identity; adds denial-specific logging.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +99 to +112
private async Task<string> GetAs(string userId, string path)
{
var access = Mesh.ServiceProvider.GetRequiredService<AccessService>();
access.SetCircuitContext(new AccessContext { ObjectId = userId, Name = userId });
try
{
return await new MeshOperations(Mesh).Get(path)
.Should().Within(60.Seconds()).Emit();
}
finally
{
access.SetCircuitContext(null);
}
}
Comment on lines +843 to +846
if (ex is UnauthorizedAccessException)
logger.LogInformation(
"FetchNode DENIED for {Path} — caller lacks Read (gated content)",
resolvedPath);
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

1 083 tests  ±0   980 ✅ ±0   5m 16s ⏱️ ±0s
   10 suites ±0   103 💤 ±0 
   10 files   ±0     0 ❌ ±0 

Results for commit 67b3dc4. ± Comparison against base commit 885225a.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

1 024 tests  ±0   1 018 ✅ ±0   4m 50s ⏱️ +33s
    5 suites ±0       6 💤 ±0 
    5 files   ±0       0 ❌ ±0 

Results for commit 67b3dc4. ± Comparison against base commit 885225a.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

   12 files  ±0     12 suites  ±0   3m 55s ⏱️ +13s
1 080 tests ±0  1 076 ✅ ±0  4 💤 ±0  0 ❌ ±0 
1 154 runs  ±0  1 150 ✅ ±0  4 💤 ±0  0 ❌ ±0 

Results for commit 67b3dc4. ± Comparison against base commit 885225a.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

2 178 tests  +3   1 990 ✅ +3   5m 6s ⏱️ -24s
   12 suites ±0     188 💤 ±0 
   12 files   ±0       0 ❌ ±0 

Results for commit 67b3dc4. ± Comparison against base commit 885225a.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

1 217 tests  ±0   1 216 ✅ ±0   6m 0s ⏱️ +7s
   12 suites ±0       1 💤 ±0 
   12 files   ±0       0 ❌ ±0 

Results for commit 67b3dc4. ± Comparison against base commit 885225a.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

1 248 tests  ±0   1 247 ✅ ±0   9m 47s ⏱️ +34s
   10 suites ±0       1 💤 ±0 
   10 files   ±0       0 ❌ ±0 

Results for commit 67b3dc4. ± Comparison against base commit 885225a.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Test Results

   61 files  ±0     61 suites  ±0   34m 56s ⏱️ + 1m 2s
7 830 tests +3  7 527 ✅ +3  303 💤 ±0  0 ❌ ±0 
7 904 runs  +3  7 601 ✅ +3  303 💤 ±0  0 ❌ ±0 

Results for commit 67b3dc4. ± Comparison against base commit 885225a.

@rbuergi
rbuergi merged commit d0cba5b into main Aug 5, 2026
17 checks passed
rbuergi added a commit that referenced this pull request Aug 5, 2026
An unentitled signed-in user could read gated PAID content by exact path. The
silo trace names the cause exactly:

  GrainCallFilter: grain=messagehub/AgenticPrimerDe/02-CodeWunsch,
                   method=DeliverMessage, userId=system-security

The read executed as system-security, which holds Permission.All — so the gate
passed legitimately. The identity was not lost late; it was never established
for the tool call.

UserContextMiddleware sets the MCP request's AccessContext on the PORTAL hub's
AccessService — which is why git_hub_sync can resolve the caller a few methods
below and refuses without one. But `ops` is built on the SESSION hub and
AccessContext is an AsyncLocal that does not survive the hops a read takes, so
MeshNodeStreamCache.GetStreamRaw saw nobody, and deliberately passed through to
its shared upstream — opened under the cache's own SYSTEM read identity. Absent
identity did not fail closed; it read everything.

AsCaller() re-establishes the caller for the duration of the tool call, applying
the pattern git_hub_sync already uses (SwitchAccessContext) to the READ tools,
where its absence was a paywall bypass rather than a wrong audit stamp. Applied
to get and search. A caller with no resolvable identity is left exactly as
before, so background / infrastructure flows are unaffected.

Verified: the gating rows were correct throughout
(Public|AgenticPrimerDe/02-CodeWunsch|Read|is_allow=false) and the SQL query
path already hid the same node — only the exact-path read leaked it, which is
why it presented as "children vanish from listings but direct get by path still
works".

Follow-up to #819, which fixed a real identity-loss seam in
MeshOperations.FetchNode but did not close the breach because the identity was
absent one level higher.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi added a commit that referenced this pull request Aug 5, 2026
Closes the paywall bypass: an unentitled user could read gated PAID content by
exact path (`get @AgenticPrimerDe/02-CodeWunsch` returned 79,650 chars of the
lesson), and the silo trace showed the read reaching the grain as
`userId=system-security`.

The framework deliberately treats an ABSENT AccessContext as System. That is
correct for background tasks — timers and seed jobs legitimately have no
identity — and is pinned by
SubscribeRequestIdentityRoutingTest.SubscribeRequest_WithNullContext_FallsBackToSystem
("System is the right fallback for 'no identity'"). But System holds
Permission.All, so applying that default to a REQUEST-shaped read means "nobody
is asking" is served everything.

So the fix is at the agent-facing surface, not in the global fallback: this
surface is only ever reached by a caller (MCP tool, agent round), so absence of a
user means UNAUTHENTICATED, not infrastructure. MeshOperations.FetchNode now
falls back to Anonymous, which sees public content and is denied gated content
exactly like a logged-out visitor. Background flows keep null → System untouched.

An earlier attempt made the cache gate itself fail closed on a null context. It
closed the hole but fought the architecture — it broke the pinned
null-falls-back-to-System contract and the static-repo import's verification
reads. Reverted in favour of this narrow one.

Tests — PaywallIdentityMatrixTests, one gated node read by every identity shape:
no ambient identity (the production case), Anonymous, signed-in-but-unentitled,
the buyer, and the public cover. The first case FAILED before this change and
passes after; the buyer and cover cases are what catch an over-strict gate.

Suites: Hosting.PostgreSql 619 passed / 0 failed; Hosting.Monolith 467 passed /
0 failed — including the two tests the broad version regressed.

Follow-up to #819 and #821, which hardened real seams but did not close the
breach because the identity was absent rather than merely lost.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi added a commit that referenced this pull request Aug 5, 2026
…s the paywall

The bypass, finally at its root: the API token attaches the user's DB/platform
roles (e.g. the portal-admin role) as CLAIM roles, and PermissionEvaluator
folded AccessContext.Roles into every node's effective permissions AFTER the
per-scope deny subtraction. A claim role was therefore a GLOBAL, UNDENIABLE
grant on the entire mesh — while the SQL fold (search, listings, RLS) never
sees claims and correctly denied the same nodes. The two read paths
disagreeing IS the bypass: measured on memex 2026-08-05 with one credential in
one second, `search` hid the gated paid lessons and `get` by exact path served
them in full (79,650 chars of AgenticPrimerDe/02-CodeWunsch to an unentitled
caller). It is also why SST was readable, and why three upstream identity
fixes (#819, #821, #823) — all real hardening — could not close it: the
identity was correct; the evaluation was wrong.

The model (AGENTS.md): a platform role grants the PLATFORM gates, deliberately
NOT cross-partition data access — it must not read a course it has not bought.

The change, in PermissionEvaluator:
- ComputeRoleState no longer folds claim roles into roleIds. Node data
  permissions come from AccessAssignment nodes and policies ONLY — matching
  the SQL path row for row.
- Claim roles keep their one legitimate job: the API-token CAPABILITY gate.
  An IsApiToken caller passes when its NODE permissions carry Api or its
  claims include an Api-bearing built-in role (ClaimsCarryApi) — without this
  half, every MCP token would zero out on PublicRead-policy content.

Tests — PaywallRealGateShapeTests: the gate seeded with the EXACT node shapes
Store's PluginGate writes (root Public+Anonymous Viewer grants, per-child
Viewer-DENIED RoleAssignments, the buyer's root grant), read through the same
entry point the MCP get tool uses. Before the fix 4 of 7 were red — including
DbRoleClaim_DoesNotGrantNodeRead, the production scenario verbatim. After: all
7 green, with the buyer, the public cover and the ApiToken-reads-public cases
pinning against an over-strict gate. The earlier PaywallIdentityMatrixTests
seeded permissions through the test fixture helper rather than assignment
nodes, which is why its deny cases passed while production leaked.

Harness fallout, fixed the honest way: tests free-rode on the claim-Admin
superuser. Now
- TestUsers.DevLoginAdminAccess() — real STATIC root Admin assignment nodes
  for the DevLogin identities, chained into the default ConfigureMesh and the
  suites that hand-roll theirs. The static grant also keeps the evaluator's
  synchronous fast path alive where tests deliberately stall synced queries
  (CompileSourceSnapshotWedgeTest).
- SetupAccessRightsAsync runs under ImpersonateAsSystem — provisioning grants
  is a System act in tests exactly as in production (PluginGate/SystemInstall);
  it cannot bootstrap under a user who needs the grant being written.

Suites: Hosting.Monolith 467 passed / 0 failed. Hosting.PostgreSql 623 passed /
3 failed — the 3 GroupMembershipRecomputeTests failures reproduce identically
on clean main (shared-fixture state, pre-existing).

Closes #822.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi added a commit that referenced this pull request Aug 5, 2026
…s the paywall (#825)

* Claim roles are a platform capability, never node data access — closes the paywall

The bypass, finally at its root: the API token attaches the user's DB/platform
roles (e.g. the portal-admin role) as CLAIM roles, and PermissionEvaluator
folded AccessContext.Roles into every node's effective permissions AFTER the
per-scope deny subtraction. A claim role was therefore a GLOBAL, UNDENIABLE
grant on the entire mesh — while the SQL fold (search, listings, RLS) never
sees claims and correctly denied the same nodes. The two read paths
disagreeing IS the bypass: measured on memex 2026-08-05 with one credential in
one second, `search` hid the gated paid lessons and `get` by exact path served
them in full (79,650 chars of AgenticPrimerDe/02-CodeWunsch to an unentitled
caller). It is also why SST was readable, and why three upstream identity
fixes (#819, #821, #823) — all real hardening — could not close it: the
identity was correct; the evaluation was wrong.

The model (AGENTS.md): a platform role grants the PLATFORM gates, deliberately
NOT cross-partition data access — it must not read a course it has not bought.

The change, in PermissionEvaluator:
- ComputeRoleState no longer folds claim roles into roleIds. Node data
  permissions come from AccessAssignment nodes and policies ONLY — matching
  the SQL path row for row.
- Claim roles keep their one legitimate job: the API-token CAPABILITY gate.
  An IsApiToken caller passes when its NODE permissions carry Api or its
  claims include an Api-bearing built-in role (ClaimsCarryApi) — without this
  half, every MCP token would zero out on PublicRead-policy content.

Tests — PaywallRealGateShapeTests: the gate seeded with the EXACT node shapes
Store's PluginGate writes (root Public+Anonymous Viewer grants, per-child
Viewer-DENIED RoleAssignments, the buyer's root grant), read through the same
entry point the MCP get tool uses. Before the fix 4 of 7 were red — including
DbRoleClaim_DoesNotGrantNodeRead, the production scenario verbatim. After: all
7 green, with the buyer, the public cover and the ApiToken-reads-public cases
pinning against an over-strict gate. The earlier PaywallIdentityMatrixTests
seeded permissions through the test fixture helper rather than assignment
nodes, which is why its deny cases passed while production leaked.

Harness fallout, fixed the honest way: tests free-rode on the claim-Admin
superuser. Now
- TestUsers.DevLoginAdminAccess() — real STATIC root Admin assignment nodes
  for the DevLogin identities, chained into the default ConfigureMesh and the
  suites that hand-roll theirs. The static grant also keeps the evaluator's
  synchronous fast path alive where tests deliberately stall synced queries
  (CompileSourceSnapshotWedgeTest).
- SetupAccessRightsAsync runs under ImpersonateAsSystem — provisioning grants
  is a System act in tests exactly as in production (PluginGate/SystemInstall);
  it cannot bootstrap under a user who needs the grant being written.

Suites: Hosting.Monolith 467 passed / 0 failed. Hosting.PostgreSql 623 passed /
3 failed — the 3 GroupMembershipRecomputeTests failures reproduce identically
on clean main (shared-fixture state, pre-existing).

Closes #822.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Test harness stops free-riding on claim roles; contract tests flipped

CI surfaced the full blast radius of the claim-role fix — 75 tests across 7
projects. Every one was a test relying on the removed behavior, none a product
flow (DevLogin_BrandNewUser_SelfProvisions passing under the new evaluator
proves onboarding needs no claim crutch). Three moves:

- ConfigureMeshBase chains TestUsers.DevLoginAdminAccess(): a real STATIC root
  Admin assignment node for the harness's ambient DevLogin identity (Roland).
  Every suite funnels through the base, so test-body scaffolding writes work
  without per-suite patches. TestUser is deliberately EXCLUDED — suites use it
  as a SUBJECT (grant Viewer/Editor per test, assert the affordances), and a
  baked-in root Admin would shadow every such role. The node id deliberately
  differs from the runtime convention ({subject}_Access → Roland_DevLoginAccess):
  several suites CREATE _Access/Roland_Access themselves and a static node at
  the same path made that create collide (completes empty). The evaluator folds
  assignments by accessObject, not node id.

- SecurityServiceTests: the two tests that PINNED the bypass as a feature are
  flipped to pin the new contract. ClaimBasedAdmin_GrantsImmediately_
  EvenWhenNoStaticAssignment ("claim-based Admin must grant all permissions") is
  now ClaimBasedAdmin_DoesNotGrantNodePermissions; the delivery-context test
  asserts restored claim roles resolve Permission.None without assignment nodes.

- The five PostgreSql suites that hand-roll ConfigureMesh (and so skip the base)
  chain DevLoginAdminAccess explicitly.

Suites: Security 345/0, Monolith 467/0, Auth 116/0, Graph 802/0, AI 979/0,
Content 245/0, NodeOperations 85/0, Grpc 8/0. PostgreSql 623/3 — the 3
GroupMembershipRecomputeTests failures are pre-existing local fixture
pollution; green on CI's fresh database.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Gate-shape tests: one lazy class-level seed instead of seven

CI shard 5 timed out the per-test seeds' fold barriers (each of seven tests
seeded its own plugin; the process-wide security-access scope streams pile up
and the 60s deny-fold barrier expired late in the shard). Seed ONCE for the
class instead — lazily from the first test body, because a top-level Space
cannot be created during mesh INIT (SetupAccessRightsAsync-time creates of a
partition-owning node complete empty; only assignment nodes work there), and
with a per-run unique plugin name (the PostgreSql fixture database persists
across runs — a constant name collides with the previous run's plugin and the
seeding CreateNode completes empty). Barriers raised to 120s.

Same nodes, same reads, same assertions — the red-before/green-after proof of
the evaluator fix is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rbuergi
rbuergi deleted the fix/mcp-get-enforces-read-permission branch August 7, 2026 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants