fix(auth): /authorize mints a LOCAL login returnUrl, and a source-side guard holds it there

memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs

@@ -199,11 +199,22 @@ public Task<IActionResult> Authorize(
             return Task.FromResult<IActionResult>(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<IActionResult>(Redirect(loginUrl));
         }

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

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`. 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.
+>
+> 🚨 **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.
 
@@ -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` |

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: <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"><path d="M9 14 4 9l5-5"/><path d="M4 9h10.5a5.5 5.5 0 0 1 5.5 5.5v0a5.5 5.5 0 0 1-5.5 5.5H11"/></svg>
+---
+
+# 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=<absolute url>`.
+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 |