The two identity reads run as System, and read every grant by rbuergi · Pull Request #2583 · Systemorph/MeshWeaver

rbuergi · 2026-08-28T08:50:23Z

OnboardingMiddleware.LoadUserRoles is the read that decides a viewer's roles — a security-fold read wearing ordinary clothes. It was written as neither, and carried two defects. Both fail the same silent way: a shortened grant list is indistinguishable from holding fewer roles, so each surfaces only as screens answering "Access denied", with nothing thrown and nothing logged. The caller's own remarks already describe that outcome for the timeout case; these are two more ways to reach it.

1. `limit:10` — proven, and it is #2011 re-introduced

The query never reached SecurityQueries.Enumeration, whose doc states the rule it breaks:

"A query string that never reaches this class is the only way back to the defect … a truncated result reads exactly like 'this grants nothing'."

A viewer with more than ten AccessAssignment nodes got an arbitrary ten; the rest of their roles vanished. The trigger is growth, not a change — so it appears on the largest install first, and nobody will have touched anything.

Red-proof: 14 grants seeded → Expected collection to have 14 item(s) … but found 10 on the unfixed code. Green after.

2. It ran under the caller's identity

workspace.GetQuery re-applies per-user RLS at the consumer (WrapWithPerUserRls), and that filter resolves permissions through PermissionEvaluator — which reads AccessAssignment nodes. So establishing an identity re-entered the permission fold from the request path, gating the answer on the identity it exists to build.

The escape hatch is documented at SyncedQueryDataSourceExtensions.cs:244 — seeing System, the wrap short-circuits to the raw upstream: "no per-user filter, no service resolution, no recursion". SecurityService.ObserveScopeAssignments already reads these very nodes that way, "by design". This aligns an outlier with the existing contract rather than inventing a bypass.

It grants a viewer nothing. The filter can only ever remove grants they legitimately hold.

FindUserByEmail gets the same treatment: leaving the first read of the pipeline wrapped keeps the same edge reachable, and its failure mode is the one this file exists to prevent (#637) — an RLS-stripped User node reads as "no such account", and the caller's response is to bounce a signed-in user to /onboarding, inviting a second account. Its limit:1 stays; that is a unique-key point lookup, not a set being truncated.

RunAsSystem, never Observable.Using(() => ImpersonateAsSystem(), …) — store and restore land on different threads and leave the subscriber latched (AGENTS.md).

🚨 What the tests do and do not prove

Stated plainly because one of them is easy to misread, and it is labelled as such in-file:

Test	Status
`Every_grant_is_read_not_the_first_page_of_them`	Red-proof. `found 10` of 14 on the unfixed code.
`The_answer_does_not_depend_on_who_is_asking`	Ratchet, NOT a red-proof. It passes on the unfixed code too.

The identity test passes on main even with both ambient context slots set to an unprivileged stranger — I checked, rather than assuming. WrapWithPerUserRls returns the upstream unchanged when the hub has no EffectivePermissionsDelegate, and a monolith test mesh does not configure one, so the per-user filter is inert there and the recursion cannot be reproduced at this level. The test pins the invariant — that the answer is identity-independent — and fails if a future change makes this read consult the asker.

MeshWeaver.Auth.Test: 148 passed, 0 failed (same count as before the change, so no test was lost).

🤖 Generated with Claude Code

`OnboardingMiddleware.LoadUserRoles` decides a viewer's roles, so it is a security-fold read wearing ordinary clothes. It was written as neither, and carried two defects that both fail the same silent way — a shorter list of grants is indistinguishable from holding fewer roles, so each one surfaces only as screens answering "Access denied", with nothing thrown and nothing logged. 1. `limit:10`, and it never reached SecurityQueries.Enumeration. A viewer with more than ten AccessAssignment nodes got an arbitrary ten and lost the rest. This is the truncation #2011 fixed for the security fold, and SecurityQueries exists to make it structurally impossible: "a query string that never reaches this class is the only way back to the defect." This was one. It fires on GROWTH, so it appears on the largest install first. Red-proofed: 14 grants seeded, `found 10` on the unfixed code. 2. It ran under the caller's identity. `workspace.GetQuery` re-applies per-user RLS at the consumer, and that filter resolves permissions through PermissionEvaluator — which reads AccessAssignment nodes. So establishing an identity re-entered the permission fold from the request path, and the answer was gated on the identity being established. Seeing System, WrapWithPerUserRls short-circuits to the raw upstream: "no per-user filter, no service resolution, no recursion". SecurityService .ObserveScopeAssignments already reads these nodes that way, "by design" — this aligns an outlier with the existing contract rather than inventing a bypass. It grants a viewer nothing: the filter can only ever REMOVE grants they legitimately hold. FindUserByEmail gets the same treatment. Leaving the FIRST read of the pipeline wrapped keeps the same edge reachable, and its failure mode is the one this file exists to prevent (#637): an RLS-stripped User node reads as "no such account", and the caller's response is to bounce a signed-in user to /onboarding — inviting a second account. Its `limit:1` stays; that is a unique-key point lookup, not a set being truncated. RunAsSystem, not `Observable.Using(() => ImpersonateAsSystem(), …)`, whose store and restore land on different threads and leave the subscriber latched. The identity test is labelled in-file as a RATCHET, not a red-proof: it passes on the unfixed code too, because the per-user filter is inert without an EffectivePermissionsDelegate, which a monolith test mesh does not configure. It pins that the answer is identity-independent. The truncation test is the half that goes red. MeshWeaver.Auth.Test: 148 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

rbuergi · 2026-08-28T08:51:31Z

This closes finding 8 of #2302 (the limit:10 truncation in LoadUserRoles), and goes past it: the same read was also running under the caller's identity, which is the re-entrancy half described in the body.

Findings 9 (McpBackConnectionService has no eviction path) remains open and untouched here.

Copilot

Pull request overview

This PR hardens the sign-in/onboarding identity reads so they can’t silently under-read authorization data (either by query paging/truncation or by being filtered by the caller’s ambient identity), which previously manifested as unexplained “Access denied” outcomes.

Changes:

Wrap LoadUserRoles’ AccessAssignment query through SecurityQueries.Enumeration(...) so it can’t be truncated by an in-query limit:10.
Run both FindUserByEmail and LoadUserRoles reads under System identity via RunAsSystem(...) to avoid per-user RLS recursion/truncation during identity establishment.
Add regression/ratchet tests plus a “What’s New” entry documenting the fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`memex/Memex.Portal.Shared/Authentication/OnboardingMiddleware.cs`	Runs the two identity-defining reads as System and routes the role query through `SecurityQueries.Enumeration` to prevent silent truncation.
`test/MeshWeaver.Auth.Test/LoadUserRolesIsASystemReadTests.cs`	Adds coverage for “reads all grants” (red-proof) and identity-independence (ratchet) of role resolution.
`src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-28-all-your-roles-are-read.md`	Documents the user-facing impact and the behavioral change.

Suppressed comments (1)

memex/Memex.Portal.Shared/Authentication/OnboardingMiddleware.cs:532

Same issue as FindUserByEmail: RunAsSystem invokes the work() factory inside Subscribe without converting synchronous exceptions into OnError. If SecurityQueries.Enumeration(...) or workspace.GetQuery(...) throws synchronously, the exception escapes the subscription and bypasses IdentityRead.Bounded’s Catch/Timeout classification.

Wrap the inner query pipeline in Observable.Defer so synchronous failures are classified as IdentityReadOutcome.Unavailable rather than surfacing as an unhandled request error.

        return IdentityRead.Bounded(
            accessService.RunAsSystem(() =>
                workspace.GetQuery(
                        $"auth:userRoles:{username}",
                        // Through the SecurityQueries seam, which overwrites any limit with

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The defect this PR fixes was reachable because nothing enforced the rule. SecurityQueries already says so about its own half — "a query string that never reaches this class is the only way back to the defect" — and LoadUserRoles was exactly such a string. IdentityReadsRunAsSystemGuard now fails the build when a pre-identity read runs unwrapped, naming file, method and line. Red-proofed by removing one wrapper: InvitationService.cs: FindPendingInvitation reads via GetQuery without RunAsSystem (line 52) Scope is narrow on purpose. "Every auth query must be System" would be WRONG: ApiTokenService.GetTokensForUser answers "show me MY tokens" for an established viewer, and per-user filtering there is the feature, not the bug. The rule covers only reads that run BEFORE a viewer has a mesh identity — every GetQuery in OnboardingMiddleware, plus FindPendingInvitation by name. It carries an anti-vacuity floor (>= 3 calls examined) so a rename cannot leave it green while guarding nothing, and it fails with "update this guard" if the files move — which is live, not hypothetical, with Memex.Portal.Shared mid-carve-out. Writing it turned up a third instance. InvitationService.FindPendingInvitation's summary ALREADY claimed it "runs as System"; it did not. The prose described a guarantee the code never made. It is a pre-identity read against the Admin partition, where an invitation filtered out is indistinguishable from one never issued — onboarding would treat an invited person as not invited and block them. Now the code matches its documentation. MeshWeaver.Auth.Test: 148 passed, 0 failed. Guard: passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

rbuergi · 2026-08-28T08:59:09Z

Pushed a second commit adding the guard and a third fix.

The invariant is now enforced, not remembered

IdentityReadsRunAsSystemGuard fails the build when a pre-identity read runs unwrapped, naming file, method and line. Red-proofed by removing one wrapper:

memex/Memex.Portal.Shared/Authentication/InvitationService.cs: FindPendingInvitation
reads via GetQuery without RunAsSystem (line 52)

This is the point the fix would otherwise miss: SecurityQueries already states the rule for its half — "a query string that never reaches this class is the only way back to the defect" — and LoadUserRoles was exactly such a string. Repairing it without a guard just resets the clock.

Scope is narrow deliberately. "Every auth query must be System" would be wrong: ApiTokenService.GetTokensForUser answers "show me MY tokens" for an established viewer, and per-user filtering there is the feature. The rule covers only reads that run before a viewer has a mesh identity — every GetQuery in OnboardingMiddleware, plus FindPendingInvitation by name. It carries an anti-vacuity floor (≥3 calls examined) so a rename cannot leave it green while checking nothing, and fails with "update this guard" if the files move — live rather than hypothetical, with Memex.Portal.Shared mid-carve-out.

Third instance, found by writing the guard

InvitationService.FindPendingInvitation's summary already claimed it "runs as System". It did not — the prose described a guarantee the code never made. It is a pre-identity read against the Admin partition, where an invitation filtered out is indistinguishable from one never issued, so onboarding would treat an invited person as not invited and block them. The code now matches its own documentation.

MeshWeaver.Auth.Test: 148 passed, 0 failed. Guard: passed.

Review catch on this PR, and it was a real regression I introduced. Replacing `Observable.Defer(...)` with `RunAsSystem(...)` moved synchronous composition throws OUT of the fault channel: SubscribeScopedObservable invoked the factory with no catch, so the exception escaped Subscribe instead of arriving as OnError. IdentityRead.Bounded classifies faults off the SEQUENCE — it maps OnError to IdentityReadOutcome.Unavailable — so an escaping throw bypassed that mapping and turned "we could not find out" back into an unclassified request failure. That is the #637 collapse this file exists to prevent, and the code I deleted carried a comment saying exactly why it was there ("Deferred so a synchronous throw while composing is classified too"). Fixed at the ROOT rather than at the two call sites. Re-adding Defer where the reviewer pointed would work and would leave the trap armed for the other ~65 RunAs* call sites, with correctness resting on each author remembering. The scope now mirrors Observable.Defer: the FACTORY is guarded, and an exception raised by the subscribe itself still propagates as Rx expects. The comment that made the wrong claim is corrected in place rather than deleted, so the file records what was wrong and what makes it right. Red-proof discriminates: against main's ImpersonationScopeExtensions the two throw tests fail and the control test (A_normal_sequence_is_unaffected) stays green, so they pin this behaviour and not something incidental. The SubscribeRestoring branch (caller identity present) is covered too, since the factory is guarded before either branch is chosen. MeshWeaver.Messaging.Hub.Test: 303 passed, 0 failed. MeshWeaver.Auth.Test: 148 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-08-28T09:26:59Z