From 8c298dc72b36970fda3c7e29fc77b03c71c89b72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= <6334612+rbuergi@users.noreply.github.com> Date: Tue, 22 Sep 2026 00:41:08 +0200 Subject: [PATCH 1/2] fix(auth): /authorize mints a LOCAL login returnUrl, and a source-side guard holds it there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every returnUrl sink in the portal validates local-only — ReturnUrlPolicy.Sanitize here, the GUI's single copy of the same rule on the login page — because an unvalidated redirect target is an open redirect. Those rules are correct. /authorize was minting {Scheme}://{Host}{Path}{QueryString}, which no sink can accept, so the target was DISCARDED rather than followed: the login page substitutes nothing for a value it will not keep, the provider link carried no returnUrl at all, and the user landed on "/" after signing in with the /authorize request gone. No authorization code was ever issued, so an MCP client waited for a completion that could not arrive and the user — who had signed in successfully — read it as the credential expiring again (#5074). The absolute form bought nothing: /login and /authorize are the same origin. OnboardingMiddleware two files away carries the same request correctly. RedirectSourcesMintLocalTargetsGuard closes the half that had no guard. RedirectSinksUseOnePolicyGuard pins that every CONSUMER routes an incoming target through the one policy, and it was green throughout this defect — rightly, since nothing consumed an unvalidated value. The new guard resolves each MINTED target to its assignment in the same file, refuses one composed as an absolute URL, classifies an inbound parameter as a forward (the sink guard's subject), and FAILS CLOSED on anything it cannot resolve. It also asserts it found a minting site, so it cannot pass vacuously if the parameter names or roots move. Measured both ways: with the pre-fix source restored, Release/-warnaserror built 0 Error(s) and the filtered run was 3 failed of 5, the guard naming the offender by name; with the fix, 20 of 20 green. The two Sanitize(absolute) == "/" cases pass in both states by design, so no constant satisfies the pair. Also in this change set: * The last-admin invariant exempts a SYSTEM-OWNED partition (#5140). A partition with a ONE-WAY _GitSync is rewritten from its repo on every sync, so AccessAssignmentGuard.IsForbiddenOnSystemOwned refuses granting another Admin there — while SpaceAdminInvariantValidator refused removing the existing one, exempting only system-managed mirrors. SystemOwnedAccessRetractionHandler's sweep could therefore only SPARE that last grant on every run, and the documented end state was unreachable by construction. Its administrator is the importer identity, not a person. A BIJECTIVE sync is deliberately NOT exempt: there the mesh nodes are somebody's working copy. Pre-fix control: only the system-owned case fails, the ordinary and two-way cases pass unchanged, so the exemption is scoped rather than the invariant switched off. * GrantingAccess.md asserted the retraction sweep "deletes such grants the moment the _GitSync is wired — so the shape is unsatisfiable". It does not; it spares the last administrator by design. That prose is what made a working sweep read as one that never fired. * TokenMintedOnOneReplicaValidatesOnAnotherTest pins the claim #5074 reasoned about from outside: a token minted by one ApiTokenService instance validates on another sharing the store. The two shapes it replaced were invisible to every single-instance test in the suite, and a claim only a second replica can falsify needs a second replica. Its companion case keeps an unknown token a definitive Invalid, never Unavailable — the distinction the 401/503 split rests on. * New doc page RedirectTargetContract.md, listed in the Architecture topic map. Co-Authored-By: Claude Opus 5 (1M context) --- .../Authentication/OAuthConnectController.cs | 17 +- .../Data/Architecture.md | 1 + .../Data/Architecture/GrantingAccess.md | 5 +- .../Architecture/RedirectTargetContract.md | 102 +++++++++ .../SpaceAdminInvariantValidator.cs | 83 +++++++- .../OAuthAuthorizeReturnUrlIsLocalTest.cs | 132 ++++++++++++ .../RedirectSourcesMintLocalTargetsGuard.cs | 194 ++++++++++++++++++ ...intedOnOneReplicaValidatesOnAnotherTest.cs | 89 ++++++++ ...variantExemptsSystemOwnedPartitionsTest.cs | 150 ++++++++++++++ 9 files changed, 759 insertions(+), 14 deletions(-) create mode 100644 src/MeshWeaver.Documentation/Data/Architecture/RedirectTargetContract.md create mode 100644 test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs create mode 100644 test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs create mode 100644 test/Memex.Portal.Shared.Test/TokenMintedOnOneReplicaValidatesOnAnotherTest.cs create mode 100644 test/MeshWeaver.Graph.Test/LastAdminInvariantExemptsSystemOwnedPartitionsTest.cs diff --git a/memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs b/memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs index 99b386c304..a7847706a9 100644 --- a/memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs +++ b/memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs @@ -199,11 +199,22 @@ public Task Authorize( return Task.FromResult(BadRequest(new { error = "invalid_request", error_description = "client_id and redirect_uri are required" })); } - // If user is not authenticated, redirect to login with return URL + // If user is not authenticated, redirect to login carrying THIS request as the return URL. + // + // 🚨 The returnUrl is a LOCAL path — `{Path}{QueryString}`, never `{Scheme}://{Host}{Path}…`. + // Every returnUrl SINK in the portal validates local-only (ReturnUrlPolicy.Sanitize here, + // LocalUrl.IsLocal in the GUI), because an unvalidated one is an open redirect. Those rules + // are correct and must not be relaxed; what was wrong was this SOURCE minting an absolute + // URL that no sink can accept. The login page's LocalOrNull then dropped it silently, the + // provider link was built with no returnUrl at all, and the user landed on "/" after signing + // in with the /authorize request gone — so no authorization code was ever issued and the MCP + // client waited forever for an "authentication successful" that could not arrive (#5074). + // The absolute form was also needless: the login page and this endpoint are the same origin. + // OnboardingMiddleware carries the same request the same way, for the same reason. if (User?.Identity?.IsAuthenticated != true) { - var authorizeUrl = $"{Request.Scheme}://{Request.Host}{Request.Path}{Request.QueryString}"; - var loginUrl = $"/login?returnUrl={Uri.EscapeDataString(authorizeUrl)}"; + var authorizePath = $"{Request.Path}{Request.QueryString}"; + var loginUrl = $"/login?returnUrl={Uri.EscapeDataString(ReturnUrlPolicy.Sanitize(authorizePath))}"; logger.LogInformation("OAuth /authorize: redirecting unauthenticated caller to {LoginUrl}", loginUrl); return Task.FromResult(Redirect(loginUrl)); } diff --git a/src/MeshWeaver.Documentation/Data/Architecture.md b/src/MeshWeaver.Documentation/Data/Architecture.md index 15f57eec65..76b4c146d8 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture.md +++ b/src/MeshWeaver.Documentation/Data/Architecture.md @@ -197,6 +197,7 @@ Each theme starts with its introductory page, followed by related architecture t - [Owner Injection](OwnerInjection) - [Permission API](PermissionApi) - [Invitation-Only Onboarding](InvitationOnlyOnboarding) +- [The Redirect-Target Contract](RedirectTargetContract) — every `returnUrl` sink validates local-only, so every source must mint local; a wrong source is refused rather than followed, which costs the whole flow and logs nothing - [Logon Actions](LogonActions) — per-user work at logon, run as the user - [Unanchored Security Reads](UnanchoredSecurityReads) — why the permission fold reads mesh-wide, and why pinning it to the viewer's partition is a silent revocation-fails-open bug - [A Denial Is an Answer](DenialIsAnAnswer) — a check on a hub with no evaluator grants Permission.All, and a refusal the mesh decided is rendered, never raised diff --git a/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md b/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md index 376f95d366..5696b51084 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md @@ -187,7 +187,9 @@ mcp create --node '{ > **This is enforced at the write boundary.** `AccessAssignmentGuard.IsScopeInvalid` (`src/MeshWeaver.Mesh.Contract/Services/AccessAssignmentGuard.cs`) refuses any `AccessAssignment` whose `mainNode` disagrees with the scope its path encodes — so `Admin/_Access/{id}` with `mainNode: ""` is rejected with *"has an EMPTY MainNode … this grants ROOT (every partition), not 'Admin'"*. A deliberately consistent root grant (`_Access/{id}` **and** `mainNode: ""`) still passes, because the test harness uses that shape; the access-control UI never offers it (`AccessAssignmentGuard.CanGrantAt` returns false at root). -> 🚨 **A SYSTEM-OWNED partition grants nobody write — and a grant COMMITTED TO A REPO can never be one.** A partition with a `{partition}/_GitSync` is rewritten from its repo on every sync, so the only identity that may write it is `system-security`. `AccessAssignmentGuard.IsForbiddenOnSystemOwned` refuses any `Admin`/`Editor`/unknown-role grant there (`Viewer`/`Commenter` entitlements and every `Denied` assignment stay legal), and `SystemOwnedAccessRetractionHandler` deletes such grants the moment the `_GitSync` is wired — so the shape is unsatisfiable, not merely inconvenient. +> 🚨 **A SYSTEM-OWNED partition grants nobody write — and a grant COMMITTED TO A REPO can never be one.** A partition with a **one-way** `{partition}/_GitSync` is rewritten from its repo on every sync, so the only identity that may write it is `system-security`. `AccessAssignmentGuard.IsForbiddenOnSystemOwned` refuses any `Admin`/`Editor`/unknown-role grant there (`Viewer`/`Commenter` entitlements and every `Denied` assignment stay legal), and `SystemOwnedAccessRetractionHandler` deletes such grants when the `_GitSync` is wired — so the shape is unsatisfiable, not merely inconvenient. A **bijective** (`twoWay: true`) sync is a different contract and is **not** system-owned: there the mesh nodes are the working copy and the people editing them keep write access. +> +> 🚨 **Two rules used to meet head-on here, and the end state above was unreachable as a result** (#5140). The retraction sweep deliberately SPARES a partition's last administrator rather than issuing a delete the pipeline would refuse — and `SpaceAdminInvariantValidator` ("a space must always have at least one admin") refused exactly that delete, because it exempted only system-managed mirror partitions. So a repo-owned space kept one human `Admin` grant for ever: the sweep spared it on every run, the guard refused any second Admin that would have let it go, and the grant still conferred write over content whose edits the next sync reverts. The invariant now exempts a system-owned partition too — its administrator is the importer identity, not a person — so the sweep converges. Reading a `_GitSync` partition that still has a human `Admin`, check the log for *"Allowed last-admin removal on SYSTEM-OWNED partition"* (it ran) before assuming the sweep did not. > > The practical consequence for anyone editing a node repo: **a privileged `_Access/*.json` in a synced data tree is dead data that logs a `fail:` line on every sync.** That is exactly what shipped in `samples/Graph/Data` until #1245 — 75 refusals in 0.36 s per sync, and the import permanently `ImportedWithErrors`. `ShippedAccessGrantsTest` (`test/MeshWeaver.Graph.Test`) now fails the build if one is re-added. Platform admin belongs in `Auth:GlobalAdmins` (`GlobalAdminSeed` writes the `Admin/_Access` grant at startup); per-space write is granted on the live mesh. @@ -259,5 +261,6 @@ After creating or updating an assignment, run through this checklist: | `src/MeshWeaver.Mesh.Contract/Security/PermissionEvaluator.cs` | Synced query that aggregates AccessAssignments per user | | `src/MeshWeaver.Mesh.Contract/Services/AccessAssignmentGuard.cs` | Write-boundary guard: `mainNode` must equal the scope the path encodes; no write-conferring grant on a system-owned (GitSynced) partition | | `src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs` | Same predicate, applied as a sweep when a `_GitSync` is wired — retracts privileged grants that predate the sync | +| `src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs` | "At least one admin per partition" — exempts system-managed mirrors AND system-owned (one-way GitSynced) partitions, so the sweep above can converge | | `test/MeshWeaver.Graph.Test/ShippedAccessGrantsTest.cs` | Structural guard: no `_Access` file committed to this repo may confer write, or name an undefined role | | `MeshWeaver.Plugins/src/MeshWeaver.Hosting.PostgreSql/PostgreSqlSchemaInitializer.cs` | `access_changed` trigger that rebuilds `partition_access` and `user_effective_permissions` | diff --git a/src/MeshWeaver.Documentation/Data/Architecture/RedirectTargetContract.md b/src/MeshWeaver.Documentation/Data/Architecture/RedirectTargetContract.md new file mode 100644 index 0000000000..d80ffd1783 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/Architecture/RedirectTargetContract.md @@ -0,0 +1,102 @@ +--- +Name: The Redirect-Target Contract — Both Ends of a returnUrl +Category: Architecture +Description: Every returnUrl sink validates local-only, so every returnUrl source must mint local — the second half of a rule that only had a guard on one end, and the silent failure mode that costs a whole sign-in flow. +Icon: +--- + +# The Redirect-Target Contract + +A `?returnUrl=` (also spelled `returnPath`, `returnTo`) carries "where the user was going" across a +flow that has to interrupt them — a sign-in, an onboarding form, a consent dialog. The portal has one +rule for it, and it is short: + +> **A redirect target is honoured only if it is LOCAL — a path on this host. Anything else falls back +> to the root.** + +`ReturnUrlPolicy.Sanitize` is that rule for the portal's server surface; the GUI has one +implementation of the same rule for its client surface. Both are correct, both are tested, and +neither is negotiable — an unvalidated redirect target is an open redirect, which is how a link that +looks like the tail of our own login flow sends a freshly-signed-in user somewhere else. + +## The contract has two ends, and only one of them had a guard + +The rule above is stated as a property of the **sink**: the thing that consumes an incoming target +and redirects to it. `RedirectSinksUseOnePolicyGuard` enforces exactly that, and the invariant it +pins is not "the policy is right" but "there is only one policy, and every sink uses it" — because +the defect it was written for was three hand-written copies of the rule, two of them subtly weaker +than the shared one. + +What nothing enforced is the **source**: the thing that MINTS a target and hands it onward. And the +two ends are not symmetric in what they cost when wrong: + +| | Wrong sink | Wrong source | +|---|---|---| +| What happens | a non-local target is followed | a non-local target is **refused** | +| Who is harmed | the user, by an open redirect | the flow, silently | +| How it surfaces | a security finding | "the feature intermittently does not work" | + +A wrong source produces no error anywhere. The sink refuses the target — doing its job — and the +flow simply carries on without it. The GUI's helper for a target it will not keep returns *nothing* +rather than substituting a destination, precisely so that a refused target does not become a +redirect to a place the user did not ask for. So the parameter is omitted, the flow completes, and +the user lands on `/`. + +## What that looked like in production + +`/authorize` (the OAuth authorization endpoint MCP clients use) minted its return target as an +absolute URL — scheme, host, path and query — and redirected to `/login` with it. Every sink +downstream then refused it, correctly, because an absolute URL is not local. The consequence, end to +end: + +1. An MCP client sends the user to `/authorize?…`. +2. The user is not signed in, so `/authorize` redirects to `/login?returnUrl=`. +3. The login page will not keep a non-local target, so the provider link carries **no** `returnUrl`. +4. The user signs in with Microsoft — successfully — and lands on `/`. +5. The `/authorize` request is gone. **No authorization code is ever issued.** +6. The client waits for an "authentication successful" that cannot arrive. + +Nothing in that sequence logs an error. From the user's side a sign-in that *worked* did not connect +anything, and from the client's side the credential looks expired — which is how it was reported +(#5074: *"Re-authenticating no longer helps"*). + +The absolute form also bought nothing: `/authorize` and `/login` are the same origin, so the scheme +and host were pure cost. `OnboardingMiddleware`, two files away, carries the same request the same +way and gets it right — `{Request.Path}{Request.QueryString}`, with a comment saying why that is +inherently local. One deviation, no guard to catch it. + +## The rule, for anyone minting one + +**Mint `{Request.Path}{Request.QueryString}`. Never `{Request.Scheme}://{Request.Host}…`.** Then pass +it through the shared policy anyway, so the intent is legible at the call site and the value is one +the policy demonstrably keeps. + +`RedirectSourcesMintLocalTargetsGuard` (`test/Memex.Portal.Shared.Test`) enforces the source end +across `memex/Memex.Portal.Shared` and `src/`. For each minting site it resolves the interpolated +value to its assignment in the same file: + +- an assignment composing a scheme and host → **refused**, naming the file and the expression; +- an inbound parameter of the enclosing method → a **forward**, not a mint; that value's safety is + the sink guard's subject where it lands; +- anything else → **fails closed**, because a site the guard cannot classify is not a clean one. + +It also asserts it found at least one minting site. A guard whose subject is renamed or moved out +from under it otherwise passes having checked nothing, and that failure is indistinguishable from +success. + +> 🚨 **A false positive here is worth reading, not suppressing.** On its first run the guard reported +> a site in each connect endpoint — both of which DOCUMENT their route as +> `GET /connect/github?returnPath={path}` in an XML comment, where the brace is a placeholder for a +> human reader and names no value at all. The fail-closed branch was right to report something it +> could not resolve; the fix was to make prose not a subject, not to loosen the classification. + +## Source references + +| File | Purpose | +|---|---| +| `memex/Memex.Portal.Shared/Authentication/ReturnUrlPolicy.cs` | The one rule for the portal's server surface | +| `memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs` | `/authorize` — mints the login return target | +| `memex/Memex.Portal.Shared/Authentication/OnboardingMiddleware.cs` | The same mint, done right, with the reason written down | +| `test/Memex.Portal.Shared.Test/RedirectSinksUseOnePolicyGuard.cs` | The sink end: every consumer routes through the one policy | +| `test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs` | The source end: every minted target is one the policy keeps | +| `test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs` | `/authorize`'s own target, with a control on each side of the change | diff --git a/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs b/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs index cf37b55cb5..37e1fff82d 100644 --- a/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs +++ b/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs @@ -25,6 +25,18 @@ namespace MeshWeaver.Graph; /// Scope: assignments at {partition}/_Access/... only. Root-scope global-admin /// assignments (_Access/{user}_Access, no partition prefix) are exempt. /// +/// 🚨 Two kinds of partition are exempt because they have no HUMAN administrator to keep, +/// and holding one there makes their documented end state unreachable: a system-managed mirror +/// (WellKnownPartitions.IsMirror), and a SYSTEM-OWNED partition — one with a ONE-WAY +/// _GitSync, whose content is rewritten from its repo on every sync so that the only writer +/// whose edits survive is the importer identity. For the second, this invariant used to meet +/// AccessAssignmentGuard.IsForbiddenOnSystemOwned head-on: that rule refuses GRANTING another +/// Admin there, this one refused REMOVING the existing one, so +/// SystemOwnedAccessRetractionHandler's sweep could only spare the last grant on every run +/// and the partition kept a human Admin over content nobody may usefully edit (#5140). A BIJECTIVE +/// sync is deliberately NOT exempt — there the mesh nodes are somebody's working copy, editing them +/// is the point, and the people doing it must keep write access. +/// /// Deadlock-safe read. The remaining-admin count comes from /// (the read-side query provider — a direct /// store/DB read), NOT a workspace.GetQuery synced subscription: the validator runs @@ -101,6 +113,63 @@ public IObservable Validate(NodeValidationContext context) if (meshService is null) return Observable.Return(NodeValidationResult.Valid()); + // 🚨 A SYSTEM-OWNED partition has no user administrator to keep, and holding one there makes + // the documented end state UNREACHABLE. Once a partition has a one-way `_GitSync` its + // content is rewritten from the repo on every sync, so the only identity that may write it + // is the importer's (AccessAssignmentGuard.IsSystemOwned). Two rules then meet head-on: + // AccessAssignmentGuard.IsForbiddenOnSystemOwned refuses GRANTING another Admin there, and + // this invariant refuses REMOVING the existing one — so + // SystemOwnedAccessRetractionHandler's sweep can only spare that last grant, every time it + // runs, and the partition keeps a human Admin over content nobody may usefully edit (whose + // edits the next sync reverts). Exempting the invariant is what lets the sweep converge: + // the partition does keep an administrator, and it is the System identity. The existing + // mirror-partition exemption above is the same reasoning one step earlier. + // + // The read is the shared authoritative one (storage, then static/config providers) rather + // than a stream subscription — this runs inside the delete/update pipeline ON the owning + // partition hub, where a synced read that round-trips back to that hub deadlocks. It is + // paid only once this operation is already known to remove an admin, so an ordinary + // _Access write costs nothing. + return NodeTypeAccessRuleGate + .ReadSubjectNode(hub, AccessAssignmentGuard.SyncConfigPath(partition)) + .Take(1) + .SelectMany(sync => + AccessAssignmentGuard.IsSystemOwned(sync, hub.JsonSerializerOptions) + ? SystemOwnedIsExempt(partition, node.Id) + : LastAdminVerdict(meshService, partition, node, context.Operation)) + .Catch(ex => + { + // Guard-rail, not a security boundary (RLS already gates who may write here): + // a lookup failure/timeout falls through to Valid rather than wedging all + // admin churn. Logged so a genuinely broken read-side is visible. + logger?.LogWarning(ex, + "SpaceAdminInvariantValidator: admin-count query failed for '{Partition}' — allowing", partition); + return Observable.Return(NodeValidationResult.Valid()); + }); + } + + /// + /// The system-owned outcome: allowed, and said out loud. Access disappearing silently is its + /// own bug class, and this is the branch that lets the last human Admin of a repo-owned space + /// actually go — so it states which rule permitted it rather than looking like no rule ran. + /// + private IObservable SystemOwnedIsExempt(string partition, string id) + { + logger?.LogInformation( + "Allowed last-admin removal on SYSTEM-OWNED partition '{Partition}' (assignment " + + "'{Id}'): it has {Partition}/_GitSync and is rewritten from its repo on every sync, " + + "so its administrator is the importer identity, not a person.", + partition, id, partition); + return Observable.Return(NodeValidationResult.Valid()); + } + + /// + /// The ordinary verdict: block when no OTHER non-denied Admin assignment remains in + /// {partition}/_Access. + /// + private IObservable LastAdminVerdict( + IMeshService meshService, string partition, MeshNode node, NodeOperation operation) + { // Count the OTHER non-denied Admin assignments remaining in {partition}/_Access. return meshService .Query(MeshQueryRequest.FromQuery( @@ -119,21 +188,15 @@ public IObservable Validate(NodeValidationContext context) logger?.LogInformation( "Blocked last-admin removal on partition '{Partition}' (assignment '{Id}', op {Op})", - partition, node.Id, context.Operation); + partition, node.Id, operation); return NodeValidationResult.Invalid( $"Cannot remove the last administrator of '{partition}'. " + "A space must always have at least one admin — grant another user the Admin role first.", NodeRejectionReason.ValidationFailed); - }) - .Catch(ex => - { - // Guard-rail, not a security boundary (RLS already gates who may write here): - // a lookup failure/timeout falls through to Valid rather than wedging all - // admin churn. Logged so a genuinely broken read-side is visible. - logger?.LogWarning(ex, - "SpaceAdminInvariantValidator: admin-count query failed for '{Partition}' — allowing", partition); - return Observable.Return(NodeValidationResult.Valid()); }); + // No Catch here: the caller wraps BOTH reads — the system-owned probe and this count — in + // the one fall-through-to-Valid handler, so a fault in either takes the same documented + // path. A second handler here would swallow the probe's fault before the caller saw it. } /// diff --git a/test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs b/test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs new file mode 100644 index 0000000000..f443b6913a --- /dev/null +++ b/test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs @@ -0,0 +1,132 @@ +using System; +using System.Threading.Tasks; +using Memex.Portal.Shared.Authentication; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace Memex.Portal.Shared.Test; + +/// +/// The returnUrl that /authorize hands to /login must be a LOCAL path. +/// +/// The defect (#5074). Every returnUrl SINK in the portal validates +/// local-only — here, the GUI's own single copy of the +/// same rule on the login page — because an unvalidated redirect target is an open redirect. +/// Those rules are right. What was wrong was this SOURCE: /authorize minted +/// {Scheme}://{Host}{Path}{QueryString}, an absolute URL that no sink can accept. The +/// login page dropped it silently (its local-or-null helper substitutes nothing rather than a +/// destination), so the provider link carried no returnUrl at all, the user landed on +/// "/" after signing in, and the /authorize request was gone. No authorization +/// code was ever issued, so an MCP client waited forever for an "authentication successful" +/// that could not arrive — and the user, who HAD signed in successfully, read it as the token +/// having expired again. +/// +/// Why the assertion is stated as a round-trip through the policy rather than as a +/// second hand-written "is it local?" check: there is exactly one such rule per assembly, and a +/// re-implementation is the bug the sink guard exists to prevent — a hand-written copy that was +/// subtly weaker than the shared rule has already shipped once. Asking the real policy whether it +/// would keep this value is the same question every sink downstream asks. +/// +/// SHOULD-FAIL-IF the absolute form comes back: +/// goes red, because +/// pins that the +/// policy sends that shape to "/". The pair is the control on each side of the change: a +/// policy that accepted everything would fail the second, and one that accepted nothing would +/// fail the first, so no constant satisfies both. +/// +public class OAuthAuthorizeReturnUrlIsLocalTest +{ + /// + /// The unauthenticated branch of /authorize resolves neither the code store nor the + /// token service (both are lazily-read properties reached only after the identity check), so + /// the action needs no services at all to exercise it. + /// + private sealed class NoServices : IServiceProvider + { + public object? GetService(Type serviceType) => null; + } + + private const string ClientId = "aB3-derived-client-id"; + private const string RedirectUri = "http://localhost:8765/callback"; + + /// The query an MCP client actually sends, escaped as it arrives on the wire. + private const string AuthorizeQuery = + "?response_type=code" + + "&client_id=" + ClientId + + "&redirect_uri=http%3A%2F%2Flocalhost%3A8765%2Fcallback" + + "&state=6ff1c0de&code_challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" + + "&code_challenge_method=S256"; + + private static OAuthConnectController Controller() => + new(new NoServices(), NullLogger.Instance) + { + ControllerContext = new ControllerContext + { + // No authenticated identity — DefaultHttpContext's principal has an + // unauthenticated identity, which is the branch under test. + HttpContext = new DefaultHttpContext + { + Request = + { + Scheme = "https", + Host = new HostString("memex.systemorph.com"), + Path = "/authorize", + QueryString = new QueryString(AuthorizeQuery), + }, + }, + }, + }; + + private static async Task LoginRedirect() + { + var result = await Controller().Authorize( + response_type: "code", + client_id: ClientId, + redirect_uri: RedirectUri, + state: "6ff1c0de", + scope: null, + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + code_challenge_method: "S256"); + + var redirect = Assert.IsType(result); + return redirect.Url; + } + + /// The returnUrl value as the login page will read it. + private static async Task CapturedReturnUrl() + { + var url = await LoginRedirect(); + const string prefix = "/login?returnUrl="; + Assert.StartsWith(prefix, url, StringComparison.Ordinal); + return Uri.UnescapeDataString(url[prefix.Length..]); + } + + [Fact] + public async Task TheLoginReturnUrlSurvivesThePolicyEverySinkApplies() + { + var returnUrl = await CapturedReturnUrl(); + + Assert.Equal(returnUrl, ReturnUrlPolicy.Sanitize(returnUrl)); + } + + [Fact] + public async Task TheLoginReturnUrlCarriesTheWholeAuthorizeRequest() + { + var returnUrl = await CapturedReturnUrl(); + + // Path AND query: dropping the query would send the user back to a /authorize with no + // client_id, which 400s — a different way of never issuing a code. + Assert.Equal("/authorize" + AuthorizeQuery, returnUrl); + } + + [Theory] + [InlineData("https://memex.systemorph.com/authorize?response_type=code&client_id=x")] + [InlineData("http://memex.systemorph.com/authorize")] + public void TheAbsoluteFormThisEndpointUsedToMint_IsRefusedByTheSamePolicy(string absolute) + // The control: this is why an absolute returnUrl never reached the login provider. + // It is not that the policy is wrong — an absolute target IS how an open redirect is + // spelled — it is that the source had to mint something the policy can keep. + => Assert.Equal("/", ReturnUrlPolicy.Sanitize(absolute)); +} diff --git a/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs b/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs new file mode 100644 index 0000000000..e7b1719523 --- /dev/null +++ b/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs @@ -0,0 +1,194 @@ +#pragma warning disable CS1591 + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using Xunit; + +namespace Memex.Portal.Shared.Test; + +/// +/// The other half of the redirect contract: every place that MINTS a ?returnUrl= must mint a +/// LOCAL path. +/// +/// Why a second guard. RedirectSinksUseOnePolicyGuard pins the SINK side — a +/// redirect that consumes a request-supplied target routes it through the one shared policy. That +/// guard was green throughout #5074, and correctly so: nothing consumed an unvalidated target. The +/// defect was at the SOURCE. /authorize minted its own return target as +/// {Scheme}://{Host}{Path}{QueryString}, and because every sink downstream refuses a +/// non-local target — which is the sinks doing their job — the value was DISCARDED rather than +/// followed. The login page substitutes nothing for a target it will not keep, so the sign-in +/// completed and landed on "/" with the original request gone: no authorization code, and an +/// MCP client waiting for a completion that could not arrive. +/// +/// So the two guards answer different questions. The sink guard asks "is an incoming target +/// validated?" — a security question. This one asks "is an OUTGOING target one that validation will +/// keep?" — a correctness question, whose failure mode is silent and looks like an expired +/// credential. Neither subsumes the other, and the first cannot see the second by construction. +/// +/// How the rule is decided. For each minting site the guard resolves the interpolated +/// value to its assignment IN THE SAME FILE and refuses one composed as an absolute URL (a scheme +/// separator, or the request's scheme read at all). A value that is an inbound parameter of the +/// enclosing method is a FORWARD, not a mint, and belongs to the sink guard. Anything the guard +/// cannot resolve to one of those two fails closed and says so — an unresolvable site is not a +/// clean one. +/// +public class RedirectSourcesMintLocalTargetsGuard +{ + /// The names a redirect target travels under — the sink guard's list, unchanged. + private static readonly string[] TargetNames = + ["returnUrl", "returnTo", "returnPath", "redirectUrl", "redirectTo"]; + + /// + /// A MINT: the parameter name inside a string, immediately followed by an interpolation hole. + /// "?returnUrl={x}" mints; "?returnPath=/" is a constant and mints nothing. + /// + private static readonly Regex MintSite = new( + @"[?&](" + string.Join("|", TargetNames) + @")=\{([^{}]{1,200})\}", + RegexOptions.Compiled | RegexOptions.IgnoreCase); + + /// Identifiers in an interpolated expression, innermost-first is not needed — all are checked. + private static readonly Regex Identifier = new(@"[A-Za-z_][A-Za-z0-9_]*", RegexOptions.Compiled); + + /// Names that are calls/types in the expression rather than the value being carried. + private static readonly string[] NotAValue = + ["Uri", "EscapeDataString", "ReturnUrlPolicy", "Sanitize", "string", "Join", "Format", + "LocalUrl", "LocalOrRoot", "LocalOrNull", "InstanceConnectFlow"]; + + private static IEnumerable Roots() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "MeshWeaver.slnx"))) + dir = dir.Parent; + Assert.NotNull(dir); + // Both trees that compose portal URLs: the auth/portal surface and the framework. + foreach (var relative in new[] { Path.Combine("memex", "Memex.Portal.Shared"), "src" }) + { + var root = Path.Combine(dir!.FullName, relative); + Assert.True(Directory.Exists(root), $"{root} not found — update this guard's roots."); + yield return root; + } + } + + [Fact] + public void Every_minted_return_target_is_a_local_path() + { + var offenders = new List(); + var sitesSeen = 0; + + foreach (var root in Roots()) + foreach (var file in Directory.EnumerateFiles(root, "*.cs", SearchOption.AllDirectories)) + { + var text = File.ReadAllText(file); + foreach (Match m in MintSite.Matches(text)) + { + // 🚨 Prose, not code. Both connect endpoints DOCUMENT their routes as + // `GET /connect/github?returnPath={path}` in an XML comment, where `{path}` is a + // placeholder for the reader and names no C# value at all — the guard's fail-closed + // branch reported it, correctly, as something it could not resolve. A comment mints + // nothing, so the subject is the code line. + if (IsInAComment(text, m.Index)) + continue; + + sitesSeen++; + var expression = m.Groups[2].Value; + var name = Path.GetFileName(file); + + var carried = Identifier.Matches(expression) + .Select(i => i.Value) + .Where(i => !NotAValue.Contains(i, StringComparer.Ordinal)) + .Distinct(StringComparer.Ordinal) + .ToArray(); + + if (carried.Length == 0) + { + offenders.Add( + $"{name}: {m.Value} — the guard could not tell what value this carries."); + continue; + } + + var verdicts = carried.Select(id => Classify(text, id, m.Index)).ToArray(); + if (verdicts.Any(v => v.Absolute)) + offenders.Add( + $"{name}: {m.Value} — carries {verdicts.First(v => v.Absolute).Detail}, " + + "which is an ABSOLUTE url."); + else if (verdicts.All(v => !v.Resolved)) + offenders.Add( + $"{name}: {m.Value} — no assignment or parameter found for " + + $"{string.Join("/", carried)}; the guard fails closed rather than " + + "assuming it is local."); + } + } + + // 🚨 The guard must have a subject. A refactor that renames these parameters, or moves the + // portal surface out of this repo, would otherwise leave it green having checked nothing. + Assert.True(sitesSeen > 0, + "No returnUrl/returnPath minting site was found in either root. Either the roots above " + + "are stale or the parameter names changed — a guard with no subject passes vacuously."); + + Assert.True( + offenders.Count == 0, + "These sites hand a redirect target onward that the shared local-only policy will NOT " + + "keep. Every sink refuses a non-local target (correctly — that is the open-redirect " + + "defence), so such a value is DISCARDED rather than followed: the user completes the " + + "flow and lands on \"/\" with the original request gone, which reads as the flow " + + "silently not working rather than as a rejected redirect (#5074). Mint " + + "{Request.Path}{Request.QueryString}, never {Request.Scheme}://{Request.Host}… — the " + + "login page and the endpoint are the same origin, so the absolute form carries no " + + "information and costs the whole flow.\n " + + string.Join("\n ", offenders)); + } + + /// + /// True when the offset sits on a line whose code has not started — a // / /// + /// line comment, or a line inside a block comment. Deliberately line-based rather than a real + /// lexer: the only thing it has to separate is a documented ROUTE from a composed URL. + /// + private static bool IsInAComment(string text, int index) + { + var lineStart = text.LastIndexOf('\n', Math.Max(0, index - 1)) + 1; + var prefix = text[lineStart..index].TrimStart(); + return prefix.StartsWith("//", StringComparison.Ordinal) + || prefix.StartsWith("*", StringComparison.Ordinal) + || prefix.StartsWith("/*", StringComparison.Ordinal); + } + + /// What a carried identifier resolves to, looking only at the file it is used in. + private readonly record struct Verdict(bool Resolved, bool Absolute, string Detail); + + /// + /// Resolves to its nearest preceding assignment, or to an inbound + /// parameter of the enclosing method. An assignment whose right-hand side composes a scheme and + /// host is ABSOLUTE; a parameter is a forwarded inbound value (the sink guard's subject). + /// + private static Verdict Classify(string text, string id, int useIndex) + { + var before = text[..useIndex]; + + var assignment = new Regex( + @"\b(?:var|string\??)\s+" + Regex.Escape(id) + @"\s*=\s*([^;]{0,400});", + RegexOptions.Compiled); + var assignments = assignment.Matches(before); + if (assignments.Count > 0) + { + var rhs = assignments[^1].Groups[1].Value; + // Either spelling of "this is a whole URL": the separator itself, or the request's + // scheme, which in this codebase is only ever read to build one. + var absolute = rhs.Contains("://", StringComparison.Ordinal) + || rhs.Contains("Request.Scheme", StringComparison.Ordinal); + return new Verdict(true, absolute, absolute ? $"`{id}` = {rhs.Trim()}" : $"`{id}`"); + } + + // A parameter of the enclosing declaration — a forwarded inbound value, governed by the + // sink guard where it lands. Looked for in the nearest preceding parameter list. + var parameter = new Regex( + @"\(\s*(?:[^()]{0,600}?[,\s])?(?:\[[^\]]{0,80}\]\s*)?[A-Za-z_][A-Za-z0-9_.<>?\[\]]*\s+" + + Regex.Escape(id) + @"\s*[,)=]", + RegexOptions.Compiled); + return parameter.IsMatch(before) + ? new Verdict(true, false, $"`{id}` (inbound parameter)") + : new Verdict(false, false, $"`{id}`"); + } +} diff --git a/test/Memex.Portal.Shared.Test/TokenMintedOnOneReplicaValidatesOnAnotherTest.cs b/test/Memex.Portal.Shared.Test/TokenMintedOnOneReplicaValidatesOnAnotherTest.cs new file mode 100644 index 0000000000..ec210ee243 --- /dev/null +++ b/test/Memex.Portal.Shared.Test/TokenMintedOnOneReplicaValidatesOnAnotherTest.cs @@ -0,0 +1,89 @@ +using System.Reactive.Linq; +using Memex.Portal.Shared.Authentication; +using MeshWeaver.Fixture; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Services; +using MeshWeaver.Messaging; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Xunit; + +namespace Memex.Portal.Shared.Test; + +/// +/// A bearer token minted by one replica is accepted by a replica that never saw it minted. +/// +/// Why this exists as a test and not as an argument. #5074 reported an MCP session that +/// alternated between working and answering 401 "token expired", and reasoned from outside the +/// cluster that the token must live in the issuing process's memory. It no longer does — +/// ApiTokenService reads the index row and the token row STRAIGHT off the shared storage +/// adapter, and CreateToken confirms both are committed before the raw token leaves the +/// server. But "it no longer does" is a claim about code, and the two shapes it replaced +/// (an in-memory dictionary, then a read that rode the mesh's cross-silo per-node-hub path) were +/// both invisible to every single-instance test in the suite. A claim that only a second replica +/// could falsify needs a second replica, or it is not being tested at all. +/// +/// Two ApiTokenService instances over one storage adapter is the same relationship two +/// pods have to one Postgres: independent service state, one authoritative store. The companion +/// case for authorization codes is OAuthCodeStoreTest's two-store exchange. +/// +/// SHOULD-FAIL-IF validation ever reads instance-local state again: the mint happens on +/// one instance and every assertion is made on the other. +/// is the control in the other +/// direction — a validator hardwired to Valid would pass the first test and fail this one, and it +/// also pins the distinction the 401/503 split rests on: an unknown token is +/// , never . +/// +public class TokenMintedOnOneReplicaValidatesOnAnotherTest(ITestOutputHelper output) + : MonolithMeshTestBase(output) +{ + private const string UserId = "replica-split-user"; + private const string UserEmail = "replica-split@example.com"; + + /// + /// One "replica": its own service instance, the SHARED store. Nothing is passed between the two + /// instances except the raw token, exactly as a client passes it. + /// + private ApiTokenService Replica() => + new( + Mesh.ServiceProvider.GetRequiredService(), + Mesh, + Mesh.ServiceProvider.GetRequiredService(), + Mesh.ServiceProvider.GetRequiredService>()); + + [Fact] + public async Task AFreshTokenIsValidOnTheReplicaThatDidNotMintIt() + { + var minting = Replica(); + var validating = Replica(); + + var created = await minting + .CreateToken(UserId, "Replica Split", UserEmail, "MintedOnA") + .Should().Emit(); + + var verdict = await validating.Validate(created.RawToken).Should().Emit(); + + verdict.Status.Should().Be( + TokenValidationStatus.Valid, + "the token's rows are in the shared store before the raw token is handed out, so the " + + "replica that receives the next request resolves it without having minted it — " + + "reason given: {0}", + verdict.Reason ?? "(none)"); + verdict.Token!.UserId.Should().Be(UserId); + } + + [Fact] + public async Task AnUnknownTokenIsADefinitiveNegativeOnEitherReplica() + { + var verdict = await Replica() + .Validate("mw_this-token-was-never-minted-anywhere") + .Should().Emit(); + + verdict.Status.Should().Be( + TokenValidationStatus.Invalid, + "an absent row is a verdict, not a failure to reach one — Unavailable is reserved for a " + + "store that could not be read, and answering 401 for that is what makes a client " + + "discard a credential that was never wrong"); + } +} diff --git a/test/MeshWeaver.Graph.Test/LastAdminInvariantExemptsSystemOwnedPartitionsTest.cs b/test/MeshWeaver.Graph.Test/LastAdminInvariantExemptsSystemOwnedPartitionsTest.cs new file mode 100644 index 0000000000..ff9f1dae8a --- /dev/null +++ b/test/MeshWeaver.Graph.Test/LastAdminInvariantExemptsSystemOwnedPartitionsTest.cs @@ -0,0 +1,150 @@ +using System.Reactive.Linq; +using System.Text.Json.Nodes; +using MeshWeaver.Fixture; +using MeshWeaver.Graph.Configuration; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Mesh.Services; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Graph.Test; + +/// +/// The last-admin invariant does not apply to a SYSTEM-OWNED partition — and still applies to every +/// other one. +/// +/// The defect (#5140), which arrived wearing three faces. A partition with a one-way +/// _GitSync is rewritten from its repo on every sync, so the only identity that may usefully +/// write it is the importer's. Two rules then met head-on: +/// AccessAssignmentGuard.IsForbiddenOnSystemOwned refuses GRANTING another Admin there, and +/// refused REMOVING the one that is already present. So +/// SystemOwnedAccessRetractionHandler's sweep could only ever SPARE that last grant — which +/// its own documentation says it does, deliberately, rather than issuing a delete the pipeline would +/// refuse. The three reported symptoms (a retraction that "never fired", a removal that is blocked, +/// a direct write that is accepted) are one mechanism: the documented end state was unreachable +/// by construction. That, not any of the three, is the defect. +/// +/// Why the exemption is sound rather than a relaxation. The invariant exists so a space +/// always has somebody who can manage it. A system-owned space HAS one — the importer identity, +/// which is also the only writer whose edits survive a sync. The mirror-partition exemption already +/// in the validator is the same reasoning one step earlier. A partition that is neither mirror nor +/// system-owned always has a human admin (its creator is granted one at create), and this invariant +/// only ever fires while REMOVING an admin, so it cannot lock out a partition that has none. +/// +/// SHOULD-FAIL-IF the exemption is widened into "the invariant is off". There is a case +/// on each side, over the same partition shape and the same delete: +/// must pass and +/// must still refuse. The third, +/// , pins the discriminator that a +/// coarser fix would lose: a BIJECTIVE sync is not system-owned, because there the mesh nodes are +/// the working copy and the people editing them must keep write access. A fix keyed on "has a +/// _GitSync" would pass the first two and fail that one. +/// +public class LastAdminInvariantExemptsSystemOwnedPartitionsTest(ITestOutputHelper output) + : MonolithMeshTestBase(output) +{ + private const string AdminUser = "the-only-admin"; + + private IStorageAdapter Storage => Mesh.ServiceProvider.GetRequiredService(); + + private SpaceAdminInvariantValidator Validator => new(Mesh); + + /// + /// The partition's single Admin grant, written to the store so the validator's remaining-admin + /// count can see it and find nothing else. + /// + private async Task TheOnlyAdminGrant(string partition) + { + var node = new MeshNode(AdminUser + "_Access", $"{partition}/_Access") + { + Name = "Admin grant", + NodeType = "AccessAssignment", + State = MeshNodeState.Active, + Content = new AccessAssignment + { + AccessObject = AdminUser, + Roles = [new RoleAssignment { Role = Role.Admin.Id }], + }, + }; + await Storage.Write(node, Mesh.JsonSerializerOptions).Should().Emit(); + return node; + } + + /// Wires {partition}/_GitSync with the given direction. + private Task WireSync(string partition, bool twoWay) => + Storage.Write( + new MeshNode(AccessAssignmentGuard.SyncConfigId, partition) + { + Name = "Sync", + NodeType = "GitHubSyncConfig", + State = MeshNodeState.Active, + // A JsonObject is one of the three shapes IsTwoWay reads, and the one the node + // builders produce — so this exercises the same tolerance production does. + Content = new JsonObject + { + ["repository"] = "Systemorph/Memex", + ["twoWay"] = twoWay, + }, + }, + Mesh.JsonSerializerOptions) + .Should().Emit(); + + /// The verdict for deleting on its own (the partition stays). + private Task DeleteVerdict(MeshNode grant) => + Validator.Validate(new NodeValidationContext + { + Operation = NodeOperation.Delete, + Node = grant, + // The grant alone is the delete root: the partition is NOT going away, which is the + // only case the invariant guards (a partition-rooted cascade is already exempt). + DeleteCascadeRootPath = grant.Path, + }) + .Should().Emit(); + + [Fact] + public async Task OnASystemOwnedPartition_TheLastAdminMayGo() + { + const string Partition = "systemownedspace"; + var grant = await TheOnlyAdminGrant(Partition); + await WireSync(Partition, twoWay: false); + + var verdict = await DeleteVerdict(grant); + + verdict.IsValid.Should().BeTrue( + "the repo owns this partition, so its administrator is the importer identity and not a " + + "person — holding the last human grant here is what made the retraction sweep unable " + + "to converge, reason given: {0}", verdict.ErrorMessage ?? "(none)"); + } + + [Fact] + public async Task OnAnOrdinaryPartition_TheLastAdminIsStillRefused() + { + const string Partition = "ordinaryspace"; + var grant = await TheOnlyAdminGrant(Partition); + // No _GitSync at all — the ordinary Space this invariant exists for. + + var verdict = await DeleteVerdict(grant); + + verdict.IsValid.Should().BeFalse( + "the control on the other side: an ordinary space left with no administrator cannot be " + + "managed by anybody, and an exemption that also covered this one would be the " + + "invariant switched off rather than scoped"); + } + + [Fact] + public async Task OnATwoWaySyncedPartition_TheLastAdminIsStillRefused() + { + const string Partition = "bijectivespace"; + var grant = await TheOnlyAdminGrant(Partition); + await WireSync(Partition, twoWay: true); + + var verdict = await DeleteVerdict(grant); + + verdict.IsValid.Should().BeFalse( + "a bijective sync preserves and commits back server-side edits, so its mesh nodes are " + + "somebody's working copy and it is NOT system-owned — the exemption keys on the " + + "DIRECTION, exactly as the write boundary does, not on the presence of a sync"); + } +} From c96c164dc05d8184936e16efcaedfa54bb4420d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= <6334612+rbuergi@users.noreply.github.com> Date: Tue, 22 Sep 2026 00:57:38 +0200 Subject: [PATCH 2/2] =?UTF-8?q?review:=20the=20#5140=20exemption=20was=20I?= =?UTF-8?q?NERT=20=E2=80=94=20the=20sweep's=20pre-spare=20had=20to=20go=20?= =?UTF-8?q?with=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot found that relaxing SpaceAdminInvariantValidator achieves nothing on its own: SystemOwnedAccessRetractionHandler PRE-COMPUTES the spare and never issues the delete, so the validator was no longer being reached for exactly the grant the change intends to remove. Its docstring said why it was built that way — "the delete pipeline would refuse exactly that delete anyway; detecting it up front turns a guaranteed rejection into the expected, stated outcome" — which was sound WHILE the validator refused, and self-invalidating the moment it did not. Half-fixed was strictly worse than unfixed: the sweep still never issues the delete, so the grant survives with nothing reporting it, and the escape the handler documents ("retracted next time, once another admin exists") can never arrive because the write boundary refuses that second Admin. A stated outcome becomes a silent one. The pre-spare is REMOVED rather than gated: the handler only reaches its sweep for a partition that IS system-owned (it returns early for a bijective sync), so the spare is now unconditionally wrong there rather than conditionally right. The circularity is written down where the old reasoning sat. Nobody is stranded, and it is structural rather than true-today. PermissionEvaluator returns Permission.All | Sync | Compile for WellKnownUsers.System BEFORE any grant is folded, so system-security administers every partition with no assignment node to delete; and a platform admin can always delete the _GitSync of a system-owned Space (already asserted by SystemOwnedSyncConfigIsVisibleToPlatformAdminsTest), which un-system-owns the partition and makes ordinary granting legal again. SystemOwnedSweepRetractsTheLastAdminTest drives the REAL sweep — SaveConfig fires the post-creation handler — and is the assertion that was missing. 🚨 Its FIRST version passed 2/2 against the unfixed sweep, i.e. it was the same defect one level up. A Space's post-creation handler grants its CREATOR Admin, so two write-conferring grants exist; the pre-spare keeps the EARLIEST-created one (the creator's) and retracts the rest, so "my grant is gone" was satisfied by both implementations. A test naming one member of a set cannot see a rule that chooses among members. The assertion is now over the SET — no write-conferring grant remains under {space}/_Access — which is both non-vacuous and the sentence the handler's own docstring makes. Corrected control: pre-fix 1 failed / 1 succeeded (the one-way case red, the bijective case green); post-fix 4 of 4 green including the sibling system-owned suite. Also from the review, all four accepted: * The guard's fail-closed branch tested All(v => !v.Resolved) where it must test Any(...) — a mixed expression (one known local plus one unclassifiable member) passed. A hole in the one branch whose purpose is not to assume. * TargetNames and NotAValue are ImmutableArray: an array is a mutable collection whatever the field's modifiers say. * The mutable List accumulator is gone rather than wrapped — the minting sites are one sequence and both the vacuity check and the verdicts derive from it, so they cannot disagree about what was scanned. * GrantingAccess.md carried the old conclusion beside the new one. Rewritten so the rule stands first and the history reads as history — keeping that the combination is what made a reporter read a working sweep as one that never ran. Co-Authored-By: Claude Opus 5 (1M context) --- .../Data/Architecture/GrantingAccess.md | 4 +- .../SystemOwnedAccessRetractionHandler.cs | 82 ++++----- .../SpaceAdminInvariantValidator.cs | 14 +- .../RedirectSourcesMintLocalTargetsGuard.cs | 119 +++++++------ ...ystemOwnedSweepRetractsTheLastAdminTest.cs | 167 ++++++++++++++++++ 5 files changed, 276 insertions(+), 110 deletions(-) create mode 100644 test/MeshWeaver.Hosting.Test/SystemOwnedSweepRetractsTheLastAdminTest.cs diff --git a/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md b/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md index 5696b51084..60923d99bc 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md @@ -187,9 +187,9 @@ mcp create --node '{ > **This is enforced at the write boundary.** `AccessAssignmentGuard.IsScopeInvalid` (`src/MeshWeaver.Mesh.Contract/Services/AccessAssignmentGuard.cs`) refuses any `AccessAssignment` whose `mainNode` disagrees with the scope its path encodes — so `Admin/_Access/{id}` with `mainNode: ""` is rejected with *"has an EMPTY MainNode … this grants ROOT (every partition), not 'Admin'"*. A deliberately consistent root grant (`_Access/{id}` **and** `mainNode: ""`) still passes, because the test harness uses that shape; the access-control UI never offers it (`AccessAssignmentGuard.CanGrantAt` returns false at root). -> 🚨 **A SYSTEM-OWNED partition grants nobody write — and a grant COMMITTED TO A REPO can never be one.** A partition with a **one-way** `{partition}/_GitSync` is rewritten from its repo on every sync, so the only identity that may write it is `system-security`. `AccessAssignmentGuard.IsForbiddenOnSystemOwned` refuses any `Admin`/`Editor`/unknown-role grant there (`Viewer`/`Commenter` entitlements and every `Denied` assignment stay legal), and `SystemOwnedAccessRetractionHandler` deletes such grants when the `_GitSync` is wired — so the shape is unsatisfiable, not merely inconvenient. A **bijective** (`twoWay: true`) sync is a different contract and is **not** system-owned: there the mesh nodes are the working copy and the people editing them keep write access. +> 🚨 **A SYSTEM-OWNED partition grants nobody write — and a grant COMMITTED TO A REPO can never be one.** A partition with a **one-way** `{partition}/_GitSync` is rewritten from its repo on every sync, so the only identity that may write it is `system-security`. Two enforcement points, one predicate (`AccessAssignmentGuard.IsSystemOwned`): `IsForbiddenOnSystemOwned` refuses any `Admin`/`Editor`/unknown-role grant there, and `SystemOwnedAccessRetractionHandler` retracts **every** such grant — including the partition's last administrator — when the `_GitSync` is wired. `Viewer`/`Commenter` entitlements, every `Denied` assignment and `system-security` itself stay. A **bijective** (`twoWay: true`) sync is a different contract and is **not** system-owned: there the mesh nodes are the working copy and the people editing them keep write access. > -> 🚨 **Two rules used to meet head-on here, and the end state above was unreachable as a result** (#5140). The retraction sweep deliberately SPARES a partition's last administrator rather than issuing a delete the pipeline would refuse — and `SpaceAdminInvariantValidator` ("a space must always have at least one admin") refused exactly that delete, because it exempted only system-managed mirror partitions. So a repo-owned space kept one human `Admin` grant for ever: the sweep spared it on every run, the guard refused any second Admin that would have let it go, and the grant still conferred write over content whose edits the next sync reverts. The invariant now exempts a system-owned partition too — its administrator is the importer identity, not a person — so the sweep converges. Reading a `_GitSync` partition that still has a human `Admin`, check the log for *"Allowed last-admin removal on SYSTEM-OWNED partition"* (it ran) before assuming the sweep did not. +> 🚨 **It did not hold until #5140, and the way it failed is worth knowing** — because three separate symptoms were one mechanism, and the middle one was the platform working as documented. The retraction sweep used to SPARE a partition's last administrator, deliberately: `SpaceAdminInvariantValidator` ("a space must always have at least one admin") would have refused that delete, so the sweep detected it up front rather than logging a guaranteed failure. But the same guard refuses granting a *second* Admin on a system-owned partition, so the sweep's stated escape — "retracted next time, once another admin exists" — could never arrive. A repo-owned space therefore kept one human `Admin` for ever, conferring write over content whose edits the next sync reverts, and a reporter reasonably read a working sweep as one that had never run. Both halves are fixed: the invariant exempts a system-owned partition (its administrator is the System identity, which holds `Permission.All` with no assignment node), and the sweep no longer spares anything. Fixing only one would have left the other silently in charge. > > The practical consequence for anyone editing a node repo: **a privileged `_Access/*.json` in a synced data tree is dead data that logs a `fail:` line on every sync.** That is exactly what shipped in `samples/Graph/Data` until #1245 — 75 refusals in 0.36 s per sync, and the import permanently `ImportedWithErrors`. `ShippedAccessGrantsTest` (`test/MeshWeaver.Graph.Test`) now fails the build if one is re-added. Platform admin belongs in `Auth:GlobalAdmins` (`GlobalAdminSeed` writes the `Admin/_Access` grant at startup); per-space write is granted on the live mesh. diff --git a/src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs b/src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs index 92183f5cc9..8336f46fa4 100644 --- a/src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs +++ b/src/MeshWeaver.GitSync/SystemOwnedAccessRetractionHandler.cs @@ -1,4 +1,3 @@ -using System.Collections.Immutable; using System.Reactive; using System.Reactive.Linq; using MeshWeaver.Mesh; @@ -39,16 +38,19 @@ namespace MeshWeaver.GitSync; /// /// What survives, deliberately: Viewer/Commenter entitlements (a /// purchase, a redeemed coupon, an admin-issued grant — the only door into a gated plugin), every -/// Denied assignment (that IS the gating), system-security itself (the identity -/// the importer writes under — retracting it would make the space unwritable by anything), and -/// the space's LAST administrator (#1120): the last-admin invariant -/// (SpaceAdminInvariantValidator — "a space must always have at least one admin") -/// outranks the sweep, so when no grant outside the doomed set still confers Admin, one admin -/// grant is spared — the earliest-created, i.e. the space's original owner — and logged at -/// Warning. The delete pipeline would refuse exactly that delete anyway; detecting it up front -/// turns a guaranteed rejection (formerly a fail-level "FAILED to retract") into the expected, -/// stated outcome. The spared grant converges like the rest of the sweep: the next time the -/// sync is (re)wired after another admin exists, it is retracted. +/// Denied assignment (that IS the gating), and system-security itself (the identity +/// the importer writes under — retracting it would make the space unwritable by anything). +/// +/// 🚨 The space's last administrator does NOT survive, and the pre-spare that used to hold +/// one back is gone (#5140). It existed because the last-admin invariant +/// (SpaceAdminInvariantValidator — "a space must always have at least one admin") refused +/// exactly that delete (#1120), which made issuing it a guaranteed rejection; the invariant now +/// exempts a system-owned partition, because its administrator is the System identity, which holds +/// Permission.All WITHOUT an assignment node. Keeping the pre-spare alongside that exemption +/// would have been worse than either alone: the sweep would still never issue the delete, so the +/// grant survived with nothing reporting it, and the stated escape — "retracted the next time the +/// sync is wired once another admin exists" — could never occur, because the write boundary refuses +/// that second Admin. That circularity is what made the documented end state unreachable. /// /// Best-effort by design (FailsCreateOnError stays false): a partition that cannot be /// swept must not block its own sync from being configured. Every retraction is logged at Warning @@ -122,51 +124,27 @@ private IObservable RetractAll(string partition, IList grants) if (doomed.Length == 0) return Observable.Return(Unit.Default); - // 🚨 The last-admin invariant OUTRANKS the sweep (#1120). SpaceAdminInvariantValidator - // refuses to delete a partition's last non-denied Admin assignment — a space must always - // keep at least one admin who can manage it (the System identity holds Permission.All - // WITHOUT an assignment node, so it does not count). Attempting that delete is therefore - // a guaranteed rejection: a known-valid guard outcome, not a fault. Detect it up front — - // when no grant OUTSIDE the doomed set still confers Admin, spare exactly one doomed - // admin grant (deterministically the earliest-created: the space's original owner) and - // retract the rest. The spared grant self-heals like the rest of the sweep: it is - // retracted the next time the sync is (re)wired once another admin exists. - var doomedPaths = doomed - .Select(c => c.Node.Path) - .ToImmutableHashSet(StringComparer.OrdinalIgnoreCase); - var adminSurvives = candidates.Any(c => - !doomedPaths.Contains(c.Node.Path) - && string.Equals(c.Node.NodeType, AccessAssignmentGuard.AccessAssignmentNodeType, - StringComparison.OrdinalIgnoreCase) - && AccessAssignmentGuard.GrantsAdmin(c.Assignment)); - - var spared = adminSurvives - ? default - : doomed - .Where(c => AccessAssignmentGuard.GrantsAdmin(c.Assignment)) - .OrderBy(c => c.Node.CreatedDate) - .ThenBy(c => c.Node.Path, StringComparer.Ordinal) - .FirstOrDefault(); - - if (spared.Node is not null) - { - logger?.LogWarning( - "[SystemOwned] keeping '{Path}' — '{Subject}' is the last administrator of " - + "'{Partition}' and a space must always have at least one admin. The grant still " - + "confers write access to a system-owned space; it is retracted the next time the " - + "sync is wired once another admin exists.", - spared.Node.Path, spared.Assignment?.AccessObject ?? "?", partition); - } - + // 🚨 EVERY doomed grant goes, the partition's last administrator included — and the + // pre-spare that used to hold one back is GONE (#5140). + // + // It was there because the last-admin invariant refused exactly that delete + // (SpaceAdminInvariantValidator, #1120), so issuing it was a guaranteed rejection and + // detecting it up front turned a fail-level "FAILED to retract" into a stated outcome. + // That reasoning was sound while the refusal stood. It no longer does: the invariant now + // exempts a system-owned partition, whose administrator is the System identity — which + // holds Permission.All WITHOUT an assignment node, which is why no grant has to survive + // to keep the partition manageable. + // + // Leaving the pre-spare in place would have made the exemption INERT, and in the worst way: + // the sweep would still never issue the delete, so the grant would survive with nothing + // reporting it, and each pass would spare it again. "Self-heals the next time the sync is + // wired once another admin exists" could never happen either, because the write boundary + // refuses that second Admin. That circularity is what made the documented end state + // unreachable, and removing this is the half that actually opens it. var doomedList = doomed .Select(c => c.Node.Path) - .Where(p => spared.Node is null - || !string.Equals(p, spared.Node.Path, StringComparison.OrdinalIgnoreCase)) .ToArray(); - if (doomedList.Length == 0) - return Observable.Return(Unit.Default); - logger?.LogWarning( "[SystemOwned] retracting {Count} privileged grant(s) — the partition is now system-owned " + "and only {System} may write it: {Paths}", diff --git a/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs b/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs index 37e1fff82d..d5f4a7a052 100644 --- a/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs +++ b/src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs @@ -33,7 +33,9 @@ namespace MeshWeaver.Graph; /// AccessAssignmentGuard.IsForbiddenOnSystemOwned head-on: that rule refuses GRANTING another /// Admin there, this one refused REMOVING the existing one, so /// SystemOwnedAccessRetractionHandler's sweep could only spare the last grant on every run -/// and the partition kept a human Admin over content nobody may usefully edit (#5140). A BIJECTIVE +/// and the partition kept a human Admin over content nobody may usefully edit (#5140). This +/// exemption is HALF the fix — the sweep's own pre-spare had to go with it, or this one is inert and +/// nothing reports that it is. A BIJECTIVE /// sync is deliberately NOT exempt — there the mesh nodes are somebody's working copy, editing them /// is the point, and the people doing it must keep write access. /// @@ -118,10 +120,12 @@ public IObservable Validate(NodeValidationContext context) // content is rewritten from the repo on every sync, so the only identity that may write it // is the importer's (AccessAssignmentGuard.IsSystemOwned). Two rules then meet head-on: // AccessAssignmentGuard.IsForbiddenOnSystemOwned refuses GRANTING another Admin there, and - // this invariant refuses REMOVING the existing one — so - // SystemOwnedAccessRetractionHandler's sweep can only spare that last grant, every time it - // runs, and the partition keeps a human Admin over content nobody may usefully edit (whose - // edits the next sync reverts). Exempting the invariant is what lets the sweep converge: + // this invariant refused REMOVING the existing one — so + // SystemOwnedAccessRetractionHandler's sweep could only spare that last grant, every time it + // ran, and the partition kept a human Admin over content nobody may usefully edit (whose + // edits the next sync reverts). Exempting the invariant is HALF of what lets the sweep + // converge; the other half is the sweep's own pre-spare, removed in the same change, because + // this exemption is inert while that is still refusing to issue the delete. Both halves: // the partition does keep an administrator, and it is the System identity. The existing // mirror-partition exemption above is the same reasoning one step earlier. // diff --git a/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs b/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs index e7b1719523..45837cfd8a 100644 --- a/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs +++ b/test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.IO; using System.Linq; using System.Text.RegularExpressions; @@ -37,8 +38,13 @@ namespace Memex.Portal.Shared.Test; /// public class RedirectSourcesMintLocalTargetsGuard { - /// The names a redirect target travels under — the sink guard's list, unchanged. - private static readonly string[] TargetNames = + /// + /// The names a redirect target travels under — the sink guard's list, unchanged. + /// ImmutableArray, not string[]: an array is a mutable collection whatever the + /// field's modifiers say, and the collections policy admits a static readonly only for a + /// constant lookup that cannot be written at runtime. + /// + private static readonly ImmutableArray TargetNames = ["returnUrl", "returnTo", "returnPath", "redirectUrl", "redirectTo"]; /// @@ -53,7 +59,7 @@ public class RedirectSourcesMintLocalTargetsGuard private static readonly Regex Identifier = new(@"[A-Za-z_][A-Za-z0-9_]*", RegexOptions.Compiled); /// Names that are calls/types in the expression rather than the value being carried. - private static readonly string[] NotAValue = + private static readonly ImmutableArray NotAValue = ["Uri", "EscapeDataString", "ReturnUrlPolicy", "Sanitize", "string", "Join", "Format", "LocalUrl", "LocalOrRoot", "LocalOrNull", "InstanceConnectFlow"]; @@ -64,7 +70,8 @@ private static IEnumerable Roots() dir = dir.Parent; Assert.NotNull(dir); // Both trees that compose portal URLs: the auth/portal surface and the framework. - foreach (var relative in new[] { Path.Combine("memex", "Memex.Portal.Shared"), "src" }) + ImmutableArray relatives = [Path.Combine("memex", "Memex.Portal.Shared"), "src"]; + foreach (var relative in relatives) { var root = Path.Combine(dir!.FullName, relative); Assert.True(Directory.Exists(root), $"{root} not found — update this guard's roots."); @@ -75,61 +82,37 @@ private static IEnumerable Roots() [Fact] public void Every_minted_return_target_is_a_local_path() { - var offenders = new List(); - var sitesSeen = 0; - - foreach (var root in Roots()) - foreach (var file in Directory.EnumerateFiles(root, "*.cs", SearchOption.AllDirectories)) - { - var text = File.ReadAllText(file); - foreach (Match m in MintSite.Matches(text)) + // One pass, no accumulator: the sites are a SEQUENCE and both readings below are derived + // from it, so the vacuity check and the verdicts cannot disagree about what was scanned. + var sites = Roots() + .SelectMany(root => Directory.EnumerateFiles(root, "*.cs", SearchOption.AllDirectories)) + .SelectMany(file => { - // 🚨 Prose, not code. Both connect endpoints DOCUMENT their routes as - // `GET /connect/github?returnPath={path}` in an XML comment, where `{path}` is a - // placeholder for the reader and names no C# value at all — the guard's fail-closed - // branch reported it, correctly, as something it could not resolve. A comment mints - // nothing, so the subject is the code line. - if (IsInAComment(text, m.Index)) - continue; - - sitesSeen++; - var expression = m.Groups[2].Value; - var name = Path.GetFileName(file); - - var carried = Identifier.Matches(expression) - .Select(i => i.Value) - .Where(i => !NotAValue.Contains(i, StringComparer.Ordinal)) - .Distinct(StringComparer.Ordinal) - .ToArray(); - - if (carried.Length == 0) - { - offenders.Add( - $"{name}: {m.Value} — the guard could not tell what value this carries."); - continue; - } - - var verdicts = carried.Select(id => Classify(text, id, m.Index)).ToArray(); - if (verdicts.Any(v => v.Absolute)) - offenders.Add( - $"{name}: {m.Value} — carries {verdicts.First(v => v.Absolute).Detail}, " - + "which is an ABSOLUTE url."); - else if (verdicts.All(v => !v.Resolved)) - offenders.Add( - $"{name}: {m.Value} — no assignment or parameter found for " - + $"{string.Join("/", carried)}; the guard fails closed rather than " - + "assuming it is local."); - } - } + var text = File.ReadAllText(file); + return MintSite.Matches(text) + // 🚨 Prose, not code. Both connect endpoints DOCUMENT their routes as + // `GET /connect/github?returnPath={path}` in an XML comment, where `{path}` is a + // placeholder for the reader and names no C# value at all — the guard's + // fail-closed branch reported it, correctly, as something it could not resolve. + // A comment mints nothing, so the subject is the code line. + .Where(m => !IsInAComment(text, m.Index)) + .Select(m => (Name: Path.GetFileName(file), Text: text, Match: m)); + }) + .ToImmutableArray(); + + var offenders = sites + .Select(site => Offence(site.Text, site.Name, site.Match)) + .Where(o => o is not null) + .ToImmutableArray(); // 🚨 The guard must have a subject. A refactor that renames these parameters, or moves the // portal surface out of this repo, would otherwise leave it green having checked nothing. - Assert.True(sitesSeen > 0, + Assert.True(!sites.IsEmpty, "No returnUrl/returnPath minting site was found in either root. Either the roots above " + "are stale or the parameter names changed — a guard with no subject passes vacuously."); Assert.True( - offenders.Count == 0, + offenders.IsEmpty, "These sites hand a redirect target onward that the shared local-only policy will NOT " + "keep. Every sink refuses a non-local target (correctly — that is the open-redirect " + "defence), so such a value is DISCARDED rather than followed: the user completes the " @@ -141,6 +124,40 @@ public void Every_minted_return_target_is_a_local_path() + string.Join("\n ", offenders)); } + /// + /// The finding for one minting site, or null when the site is clean. + /// + /// 🚨 The unresolvable branch tests Any, not All. All(v => !v.Resolved) + /// reports only when NOTHING resolved, so an expression combining a known local with one + /// unclassifiable member would pass — which contradicts the fail-closed contract this guard is + /// built on, and in the one branch whose whole purpose is not to assume. If any carried value + /// cannot be classified, the site is not clean. + /// + private static string? Offence(string text, string name, Match m) + { + var carried = Identifier.Matches(m.Groups[2].Value) + .Select(i => i.Value) + .Where(i => !NotAValue.Contains(i)) + .Distinct(StringComparer.Ordinal) + .ToImmutableArray(); + + if (carried.IsEmpty) + return $"{name}: {m.Value} — the guard could not tell what value this carries."; + + var verdicts = carried.Select(id => Classify(text, id, m.Index)).ToImmutableArray(); + + if (verdicts.Any(v => v.Absolute)) + return $"{name}: {m.Value} — carries {verdicts.First(v => v.Absolute).Detail}, " + + "which is an ABSOLUTE url."; + + var unresolved = verdicts.Where(v => !v.Resolved).ToImmutableArray(); + return unresolved.IsEmpty + ? null + : $"{name}: {m.Value} — no assignment or parameter found for " + + $"{string.Join("/", unresolved.Select(v => v.Detail))}; the guard fails closed " + + "rather than assuming it is local."; + } + /// /// True when the offset sits on a line whose code has not started — a // / /// /// line comment, or a line inside a block comment. Deliberately line-based rather than a real diff --git a/test/MeshWeaver.Hosting.Test/SystemOwnedSweepRetractsTheLastAdminTest.cs b/test/MeshWeaver.Hosting.Test/SystemOwnedSweepRetractsTheLastAdminTest.cs new file mode 100644 index 0000000000..a554441941 --- /dev/null +++ b/test/MeshWeaver.Hosting.Test/SystemOwnedSweepRetractsTheLastAdminTest.cs @@ -0,0 +1,167 @@ +using System; +using System.Linq; +using System.Reactive.Linq; +using System.Threading.Tasks; +using MeshWeaver.Fixture; +using MeshWeaver.GitSync; +using MeshWeaver.Graph; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Mesh.Services; +using MeshWeaver.Messaging; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace MeshWeaver.Hosting.Test; + +/// +/// 🚨 Wiring a ONE-WAY _GitSync retracts EVERY privileged grant on the partition — the last +/// administrator included. Wiring a BIJECTIVE one retracts nothing. +/// +/// Why this test exists, and what it would have caught. #5140 reported three symptoms of +/// one mechanism: a system-owned Space that kept a human Admin grant through two syncs, a +/// direct write by that human being accepted, and the grant being impossible to delete. Two rules +/// were holding each other up. AccessAssignmentGuard.IsForbiddenOnSystemOwned refuses granting +/// a SECOND Admin on such a partition; SpaceAdminInvariantValidator refused removing the +/// FIRST; so SystemOwnedAccessRetractionHandler pre-detected the guaranteed rejection and +/// SPARED that grant — on every pass, for ever, because the escape it documented ("retracted next +/// time, once another admin exists") could never arrive. +/// +/// Exempting the invariant is only half the fix, and on its own it is the WORSE half: the +/// handler's pre-spare would still stop the delete from ever being issued, so the exemption would be +/// inert and nothing would report it. A test that drives the validator directly cannot see that — it +/// measures the half that changed. This one drives the SWEEP, which is the thing whose outcome the +/// issue is about. +/// +/// Control on each side, over the same Space and the same grants: +/// must converge to an EMPTY set of +/// write-conferring grants and must not. A sweep that +/// retracted unconditionally would pass the first and fail the second; one that spares anything +/// fails the first. Neither is satisfiable by a constant. +/// +/// 🚨 The assertion is about the SET, and the first version of this test — which named one +/// grant's path — passed against the unfixed sweep. A Space's post-creation handler grants its +/// creator Admin, so two write-conferring grants exist; the pre-spare kept the earliest (the +/// creator's) and deleted the other, making "my grant is gone" true either way. That is the same +/// defect this whole change set is about, in the test written to catch it, and only running the +/// control against the unfixed source exposed it. +/// +public class SystemOwnedSweepRetractsTheLastAdminTest(ITestOutputHelper output) + : MonolithMeshTestBase(output) +{ + private const string SpaceOwner = "space-owner"; + private const string RepoUrl = "https://github.com/test/sweep-last-admin"; + + private static TimeSpan Budget => TestTimeouts.Convergence; + + private GitHubSyncService Sync => Mesh.ServiceProvider.GetRequiredService(); + + protected override MeshBuilder ConfigureMesh(MeshBuilder builder) + => base.ConfigureMesh(builder) + .AddGitHubSyncTypes() + .ConfigureServices(services => + { + services.AddGitHubSyncServices(); + return services; + }); + + /// + /// A Space carrying a human Admin grant written BEFORE the sync is wired — the whole window + /// #5140 is about: legal when written, wrong one second later. + /// + /// 🚨 There are TWO admin grants here, not one, and that is deliberate — the Space's + /// post-creation handler grants its CREATOR Admin as well. Asserting on one path was how the + /// first version of this test passed against the unfixed sweep: the pre-spare kept the + /// earliest-created grant (the creator's) and deleted the other, so "my grant is gone" was true + /// either way. The property is about the SET, so the assertions below are about the set. + /// + private async Task ASpaceWithAHumanAdmin() + { + var space = "Swept" + Guid.NewGuid().ToString("N")[..8]; + await NodeFactory.CreateNode(new MeshNode(space) + { + NodeType = "Space", Name = "Swept space", State = MeshNodeState.Active, + Content = new Space(), + }).Should().Within(Budget).Emit("the Space fixture must be created", + cancellationToken: TestContext.Current.CancellationToken); + + await NodeFactory.CreateNode(AssignmentNodeFactory.UserRole(SpaceOwner, "Admin", space)) + .Should().Within(Budget).Emit("a human Admin grant must exist BEFORE the sync is wired", + cancellationToken: TestContext.Current.CancellationToken); + + await WriteGrantsRemain(space).Should().Within(Budget).Emit( + "at least one write-conferring grant must be readable before the sweep runs, or every " + + "assertion below is vacuous", + cancellationToken: TestContext.Current.CancellationToken); + + return space; + } + + /// + /// Emits once NO write-conferring AccessAssignment remains under {space}/_Access — + /// the property the sweep claims. Re-queried on an interval because a query is the read-side + /// index and trails the store: waiting on the CONDITION, never on a delay. + /// + private IObservable NoWriteGrantRemains(string space) => Poll(space, anyRemain: false); + + /// Emits once at least one write-conferring grant is readable there. + private IObservable WriteGrantsRemain(string space) => Poll(space, anyRemain: true); + + private IObservable Poll(string space, bool anyRemain) + => Observable.Interval(TestTimeouts.Quick / 10) + .StartWith(0L) + .SelectMany(_ => NodeFactory + .Query(MeshQueryRequest.FromQuery( + $"namespace:{space}/_Access nodeType:{AccessAssignmentGuard.AccessAssignmentNodeType}")) + .Take(1) + .Select(change => change.Items.Any(n => + AccessAssignmentGuard.ConfersWriteAccess( + n.ContentAs(Mesh.JsonSerializerOptions))))) + .Where(remain => remain == anyRemain) + .Take(1); + + private async Task WireSync(string space, bool twoWay) + { + var config = await Sync.SaveConfig( + space, RepoUrl, "main", null, false, false, + direction: twoWay ? SyncDirection.Bidirectional : SyncDirection.ImportOnly, + twoWay: twoWay) + .Should().Within(Budget).Emit("the sync config must be written", + cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(GitHubSyncService.ConfigPath(space), config.Path); + Assert.Equal( + !twoWay, + AccessAssignmentGuard.IsSystemOwned(config, Mesh.JsonSerializerOptions)); + } + + [Fact] + public async Task AOneWaySync_LeavesNoWriteConferringGrantAtAll() + { + var space = await ASpaceWithAHumanAdmin(); + + await WireSync(space, twoWay: false); + + await NoWriteGrantRemains(space).Should().Within(Budget).Emit( + "the repo owns this Space now, so nobody else does — INCLUDING its last administrator. " + + "There is no second admin to wait for, because the write boundary refuses one, so " + + "sparing any of them is what made the documented state unreachable (#5140)", + cancellationToken: TestContext.Current.CancellationToken); + } + + [Fact] + public async Task ABijectiveSync_RetractsNothing() + { + var space = await ASpaceWithAHumanAdmin(); + + await WireSync(space, twoWay: true); + + // The negative shape: the set must NOT empty out. A bijective sync preserves and commits + // back server-side edits, so the mesh nodes are these admins' working copy and retracting + // their write access would make the feature unusable by everyone but the importer. + await NoWriteGrantRemains(space).Should().NotEmit(TestTimeouts.Quick, + "a bijective sync does not make a Space system-owned, so its administrators keep write " + + "access — the sweep keys on the DIRECTION, not on the presence of a sync"); + } +}