Ten review findings on Memex.Portal.Shared surfaced by the GUI extraction — open redirects, an uncapped anonymous read, HMAC-after-buffer

Automated review of MeshWeaver.Plugins#674 (the portal move) surfaced ten findings on files that the
split then moved back to core. The PR's paths are stale; the findings are not — every one
describes code live in memex/Memex.Portal.Shared (and one in MeshWeaver.Blazor/Infrastructure)
on core main today.

Filing them here because resolving the stale threads on that PR would otherwise discard them
silently, and several are security-relevant.

🚨 Not introduced by the extraction. #674 is a move; these files' contents are pre-existing. The
review simply made them visible by touching the files. That is also why they should not be fixed
inside a move PR — it would conflate a relocation with behavioural change.

---### src/Memex.Portal.Shared/Authentication/ExternalAuthController.cs
returnUrl is attacker-controlled and is passed directly to Redirect, so /auth/login?...&returnUrl=https://evil.example turns the successful sign-in callback into an open redirect. Restrict the destination to a local URL at the redirect sink (the GitHub and onboarding flows already do this).

This issue also appears on line 96 of the same file.

src/Memex.Portal.Shared/Api/WebhookInboxEndpoints.cs

The comment promises a capped reader for chunked bodies, but ReadToEndAsync has no cap. Because this endpoint is anonymous, a request without Content-Length can be fully buffered before WebhookInbox.Deliver returns TooLarge, allowing concurrent deliveries to consume memory well beyond MaxBodyBytes. Enforce the byte limit while reading and return 413 as soon as it is crossed.

src/Memex.Portal.Shared/Social/GitHubWebhookEndpoints.cs

The HMAC is checked only after the unauthenticated request body has been copied into a MemoryStream and then duplicated by ToArray(), with no endpoint-specific size limit. Invalid callers can force large allocations before authentication. Apply a tight streaming/request-body cap before buffering and reject oversized payloads with 413.

src/Memex.Portal.Shared/Authentication/McpBackConnectionService.cs

A revoked cached token can never be replaced: every later spawn returns it from this branch, and neither CLI client reports a subsequent /mcp 401 back to this service. Contrary to the class comment, there is no auth-on-exception path that evicts the entry, so mesh access remains broken until the portal process restarts. Add an invalidation/retry contract or validate and evict cached tokens before reuse.

src/MeshWeaver.Blazor/Infrastructure/UserIdentityCache.cs

This subscribe-before-read change fixes a timing-sensitive hang, but no regression test for UntilDetermined was moved into this repository. Add a deterministic hot-observable test that emits between subscription and the initial lookup and proves the determined value is delivered; also cover a throwing initial lookup disposing the hot-feed subscription. Without those cases, a future operator rewrite can reintroduce the 60-second timeout while all builds remain green.

src/Memex.Portal.Shared/Settings/UpdatePolicySettingsTab.cs

CheckedAt is a stored instant, but formatting it directly displays the container's UTC value rather than the admin's local time. Capture the viewer zone once in BuildContent, pass it into this pure formatter, and convert with DisplayTimeExtensions.ToDisplayTime before formatting.

This issue also appears in the following locations of the same file:

line 183
line 191

src/Memex.Portal.Shared/Settings/ApiTokensSettingsTab.cs

Created, expiry, and last-used values are stored instants, but all three are formatted directly in UTC. Convert each through the viewer's zone (captured once from AccessService) before formatting; even the date-only fields can otherwise be off by one day for the viewer.

src/Memex.Portal.Shared/Authentication/OnboardingMiddleware.cs

This auth-path query uses scope:subtree without a namespace/path anchor, so every role lookup unions AccessAssignment rows across every partition and gets slower as tenants are added. The limit:10 also makes users with more grants lose roles nondeterministically. Resolve roles from an indexed, user-anchored source (or maintain a dedicated access-object index) rather than scanning the whole mesh.

src/Memex.Portal.Shared/Social/GitHubLoginEndpoints.cs

This local-URL check rejects //host but accepts /\\host. Browsers normalize backslashes in special-scheme URLs, so a crafted return target can become a protocol-relative external redirect. Reject the /\\ form as the onboarding guard already does.

src/Memex.Portal.Shared/Authentication/DevAuthController.cs

Dev sign-in redirects to an unvalidated form value. Self-hosted/distributed deployments can explicitly enable dev login, so this is still an open redirect rather than a localhost-only assumption. Validate a same-site relative path before redirecting.

This issue also appears on line 233 of the same file.

Suggested triage order

Open redirects (3) — ExternalAuthController, Login.razor, DevAuthController. All take an
attacker-controlled returnUrl into a redirect. DevAuthController matters because self-hosted
and distributed deployments can explicitly enable dev login.
GitHubLoginEndpoints — the local-URL check rejects //host but accepts /\host, and
browsers normalise backslashes in special-scheme URLs. Same class, subtler.
WebhookInboxEndpoints — the comment promises a capped reader; ReadToEndAsync has no cap,
on an anonymous endpoint. A request without Content-Length is unbounded.
GitHubWebhookEndpoints — HMAC verified only after the unauthenticated body is copied into
a MemoryStream and duplicated by ToArray().
OnboardingMiddleware — scope:subtree with no namespace anchor on the AUTH path, unioning
AccessAssignment rows across every partition. This repo has been bitten by unanchored
cross-partition queries before.
McpBackConnectionService — a revoked cached token can never be replaced; every later spawn
returns it and no /mcp 401 invalidates it.
UTC formatting (2) — UpdatePolicySettingsTab and ApiTokensSettingsTab format stored
instants directly, rendering the CONTAINER's clock rather than the viewer's. AGENTS.md's
localization rule covers formatting, not only translated strings.
UserIdentityCache — no regression test for UntilDetermined accompanied the
subscribe-before-read fix (Arm the observer BEFORE the trigger — a hot emission missed has no second chance #2259).

I have not verified any of these independently — they are review output, reproduced faithfully and
relocated to the repo that owns the code. Each needs confirming before it is acted on.

Two of the ten fixed in #2374 — the open redirect (ExternalAuthController :89/:100) and the unbounded webhook body read (WebhookInboxEndpoints :60). Both verified live on main before fixing; both are in the platform half that stays in core. Note the body-read one was worse than filed: the comment beside it described a capped reader that did not exist. The remaining eight stay open here.

Working through this list. Status of the five findings I have now checked against main @ b010a3ba2:

finding	state
open redirect — `ExternalAuthController` (×2)	already fixed — both go through `ReturnUrlPolicy.Sanitize`
uncapped anonymous read — `WebhookInboxEndpoints`	already fixed — uses `BoundedBody.ReadAsync` + 413
HMAC-after-buffer — `GitHubWebhookEndpoints`	was live → PR #2572
`/\host` bypass — `GitHubLoginEndpoints.SafeLocal`	was live → PR #2573
unvalidated redirect — `DevAuthController` (×2)	was live → PR #2573

A detail worth recording from #2573: the assembly held four implementations of the local-URL rule — ReturnUrlPolicy.Sanitize (correct), EaConsentController.SafeReturnUrl (correct), GitHubLoginEndpoints.SafeLocal (wrong), DevAuthController (absent). Testing the policy harder would have found none of it, which is why that PR adds a ratchet on one implementation, every sink uses it rather than more policy tests.

`OnboardingMiddleware` — verified live, and I am NOT fixing it

workspace.GetQuery(
    $"auth:userRoles:{username}",
    $"nodeType:AccessAssignment content.accessObject:\"{username}\" scope:subtree limit:10")

Both halves of the finding are exactly as reported. The limit:10 is the sharper one: it is role loss, not just slowness. A user with more than ten AccessAssignment grants gets an arbitrary ten, so their effective roles can differ between requests — on the auth path.

How live is it? I could not count grants per user: content comes back as a shell projection in query results, so content.accessObject is not readable from a search. What I can measure is the shape — on memex-cloud the assignment set is truncated at a 200-row query, spread across many space containers (AppleMaps/_Access 31, AgenticEngineering/_Access 24, …). One assignment per space per user is the normal pattern, so a user in more than ten spaces crosses the cap, and this mesh plainly has more than ten spaces. Structurally reachable; I have not identified a specific affected user, and I am not claiming one.

Why I stopped rather than patched. The obvious edit — raise or drop limit:10 — trades silent role loss for an unanchored scope:subtree scan across every partition on every authenticated request, which is the other half of the same finding. And anchoring the query is not a one-liner either: the Admin partition is invisible to unscoped nodeType: queries (a trap that has already blinded the self-updater), so a naive anchor can silently return fewer roles rather than more — the same failure it is meant to fix, harder to see.

So this one wants the "indexed, user-anchored source" the finding proposes, chosen deliberately. It is the highest-severity item left on this list and should probably be split into its own issue rather than sitting fifth in a ten-item review dump.

Remaining unchecked here: UpdatePolicySettingsTab, ApiTokensSettingsTab (UTC formatting vs viewer zone), and the others below them.

Review: a FIFTH copy, and a guard that certified it because of its name

All ten findings now checked. Completing the triage so the next pass starts from measurements rather than re-deriving them.

#	finding	state on `main`
1	open redirect — `ExternalAuthController` (×2)	already fixed (`ReturnUrlPolicy.Sanitize`)
2	uncapped anonymous read — `WebhookInboxEndpoints`	already fixed (`BoundedBody` + 413)
3	HMAC after unbounded buffer — `GitHubWebhookEndpoints`	was live → PR #2572, merged
4	`/\host` accepted — `GitHubLoginEndpoints.SafeLocal`	was live → PR #2573
5	unvalidated redirect — `DevAuthController` (×2)	was live → PR #2573
6	UTC formatting — `ApiTokensSettingsTab` (×3)	was live → PR #2574
7	UTC formatting — `UpdatePolicySettingsTab` (×3)	was live → PR #2574
8	unanchored `scope:subtree limit:10` — `OnboardingMiddleware`	live, deliberately not fixed (see above)
9	no token eviction — `McpBackConnectionService`	live ↓
10	no `UntilDetermined` regression test — `UserIdentityCache`	live ↓

A sixth open-redirect copy turned up while fixing #4/#5 and is in PR #2573 as well: GitHubConnectEndpoints.SafeReturn checked only for a leading /, so it accepted //evil.example outright — weaker than the SafeLocal bug this issue names, and it was missed because its sink is called returnPath rather than returnUrl.

9 — `McpBackConnectionService`: confirmed, no eviction path exists

grep -n 'Remove|Evict|Invalidate|TryRemove|401|Unauthorized' McpBackConnectionService.cs
  24:  /// stays valid). A revoked token surfaces as a 401 on the next MCP call, which the

The only match is that comment. There is no code that removes or revalidates a cached entry, so the class comment describes a contract the class does not implement — exactly as reported. A revoked token stays cached until the process restarts.

10 — `UserIdentityCache`: confirmed, still untested

grep -rln UntilDetermined --include='*.cs' test/ src/
  src/MeshWeaver.Hosting.AspNetCore/Portal/UserIdentityCache.cs

One hit, and it is the implementation. No test anywhere references UntilDetermined. Note the path in this issue is stale — the file now lives under MeshWeaver.Hosting.AspNetCore/Portal, not MeshWeaver.Blazor/Infrastructure — but the gap is real and the finding's warning holds: a future operator rewrite can reintroduce the 60-second hang with every build green, because nothing exercises the emit-between-subscribe-and-lookup window.

Neither 9 nor 10 is fixed here. 9 needs an invalidation contract decided (validate-before-reuse vs evict-on-401, and which component reports the 401); 10 is a contained piece of work — a deterministic hot-observable test plus a throwing-initial-lookup case — and is the cheapest remaining item on this list.

Merge pull request #2581 from Systemorph/fix/2302-untildetermined-test

Status after #2581 (merged) and #2583 (in CI):

#	Finding	State
8	`UserIdentityCache` — no `UntilDetermined` regression test	Closed by #2581
—	`LoadUserRoles` `limit:10` truncation	Closed by #2583 (red-proofed: 14 grants → `found 10`)
5	`OnboardingMiddleware` — unanchored `scope:subtree` on the auth path	Confirmed, deliberately NOT fixed in #2583 — see below
9	`McpBackConnectionService` — revoked cached token can never be evicted	Still open

Finding 5 is real, and it is the opposite of what it looks like

Working on #2583 I read this query closely. The finding says an unanchored scope:subtree "unions AccessAssignment rows across every partition", implying the fix is to anchor it. Anchoring it would be wrong: a user legitimately holds grants in many partitions, and the query is already narrowed by content.accessObject, so a namespace anchor would silently drop grants — the same failure mode as the truncation, arriving by a different route.

The actual defect is the reverse. A pathless cross-schema query deliberately excludes the admin schema — this repo states it in its own comment in Onboarding.razor:

"Invitations live in the Admin partition, which is excluded from cross-schema global search, so a namespace:Admin-only query (no path) fans out cross-schema and never finds them"

So LoadUserRoles cannot see grants in Admin/_Access — which is exactly where platform-admin grants live (AGENTS.md: "the grant lives in Admin/_Access, never root _Access"). A platform admin's Admin role is therefore missing from the AccessContext.Roles this read stamps.

Why I did not fold it into #2583. Its blast radius is different from the other two fixes: those strictly widen a read toward the truth, whereas changing scope semantics can remove rows if I get the shape wrong, and the correct shape is a union (global for ordinary grants plus a path:Admin/_Access scope:children leg, the shape #743 established for firstUserCheck). That deserves its own change and its own red-proof, not a rider on a PR that is already green.

It may also be latent rather than live, which is the other reason not to rush it: AGENTS.md is explicit that admin gates go through hub.IsGlobalAdmin(), and PermissionEvaluator reads Admin scopes with a path-scoped query of its own — so the authoritative fold is not blind here even though this stamped role list is. Worth measuring which consumers actually read AccessContext.Roles for an admin decision before deciding the severity.

I have verified the query shape and the admin-schema exclusion; I have not demonstrated a user losing an admin capability because of it.

Correcting my own earlier comment on finding 5 — I was wrong, and acting on it would have produced a fix for a problem that does not exist.

Earlier I wrote that the finding's suggested anchoring was backwards, and that "the real defect is the mirror image: a pathless cross-schema query excludes the admin schema, so LoadUserRoles cannot see grants in Admin/_Access". I inferred that from a different observation (an unscoped nodeType:BuildCompletion returning 0 while namespace:Admin scope:descendants returned 7) and did not test it against the query this code actually issues.

I have now run that exact shape against the live mesh:

nodeType:AccessAssignment content.accessObject:"rbuergi" scope:subtree

Admin/_Access/rbuergi_Access is the FIRST result. The Admin grant is returned. The exclusion I asserted does not apply to this query, so the consequence I drew from it — a platform admin's Admin role missing from AccessContext.Roles — does not follow.

I do not yet know why this shape reaches the admin schema when the BuildCompletion one did not; the difference is likely scope:subtree or the content. predicate changing how the query is routed. That is worth understanding on its own, but it is not this finding.

What finding 5 actually is, then

Back to what it says rather than what I turned it into: an unanchored scope:subtree on the auth path unions AccessAssignment rows across every partition schema, on every sign-in. That is a scale concern and it is real — see Systemorph/MeshWeaver.Plugins#983, which measures the same union shape across ~184 schemas — but it is a cost problem, not a correctness one, and it should be fixed with that framing.

Which also means anchoring it by namespace, as the finding suggests, is still wrong for the reason I gave originally and stand by: a user legitimately holds grants in many partitions, so a namespace anchor would silently drop them. The fix has to preserve the union while making it cheaper — pinning by the accessObject index rather than by partition.

Apologies for the noise. Flagging it explicitly because a confident wrong analysis in an issue thread is worse than none: the next person would have implemented an Admin-partition union leg that fixes nothing.

Merge pull request #2607 from Systemorph/fix/2302-mcp-token-eviction

Walked all ten findings against `main` @ `aeecb0d3b` — every one is resolved

Checked each finding against live code rather than against the PRs that claimed them, since the point of this issue was that filed findings become an unwalked to-do list.

#	Finding	State on main	Evidence
1	`ExternalAuthController` open redirect	fixed	both sinks (`:89`, `:100`) are `Redirect(ReturnUrlPolicy.Sanitize(returnUrl))`
2	`WebhookInboxEndpoints` uncapped `ReadToEndAsync`	fixed	`BoundedBody.ReadAsync(request.Body, WebhookInbox.MaxBodyBytes, ct)`, plus a `ContentLength` pre-check and 413 on both
3	`GitHubWebhookEndpoints` HMAC-after-buffer	fixed	413 at `:77` precedes `VerifySignature` at `:81` — the cap is applied before buffering
4	`McpBackConnectionService` revoked token never replaced	fixed	#2607 (validate-before-reuse, tri-state verdict, evict-and-mint on `Invalid`)
5	`UserIdentityCache` `UntilDetermined` has no regression test	fixed	`test/MeshWeaver.Hosting.Monolith.Test/UntilDeterminedTest.cs` — and it covers precisely the two cases asked for: `An_emission_during_the_initial_lookup_is_still_delivered` and `A_throwing_initial_lookup_disposes_the_feed_subscription`
6	`UpdatePolicySettingsTab` formats UTC	fixed	all three sites go through `DisplayTimeExtensions.ToDisplayTime(…, zoneId)`
7	`ApiTokensSettingsTab` formats UTC	fixed	zone captured once at `:194` via `ViewerZoneId()`; created/expiry/last-used all converted
8	`OnboardingMiddleware` unanchored `scope:subtree` + `limit:10`	fixed	#2583 — `SecurityQueries.Enumeration` (non-truncatable, `limit:all`) run under `RunAsSystem`, with `IdentityReadsRunAsSystemGuard` as the ratchet
9	`GitHubLoginEndpoints` accepts `/\\host`	fixed	`SafeLocal` is now `=> ReturnUrlPolicy.Sanitize(returnUrl)` — one policy, not a second implementation
10	`DevAuthController` unvalidated redirect	fixed	both sinks (`:179`, `:243`) sanitized

The three open redirects and the /\\host bypass converged on one policy. ReturnUrlPolicy.Sanitize is now the single sink guard, and ReturnUrlPolicyTest pins the whole class in one place — including the two forms this issue called out separately, //evil.example and /\\evil.example, alongside javascript:, absolute URLs and the bare-host form. That is the durable shape: the finding was that two hand-written local-URL checks disagreed, so having one is what stops it recurring.

One I checked and was wrong about, recording it so nobody re-checks: EaConsentController:97/:102 look like raw Redirect(returnUrl) under grep, but returnUrl there is already the result of SafeReturnUrl(...) at :90 — and it is deliberately re-sanitised on the callback because the value round-trips through the IdP as state. Not a finding.

Closing. No code change was needed for this walk — the work landed across #2583, #2607 and the ReturnUrlPolicy/BoundedBody consolidations.

Navigation Menu

Repository navigation

Ten review findings on Memex.Portal.Shared surfaced by the GUI extraction — open redirects, an uncapped anonymous read, HMAC-after-buffer #2302

src/Memex.Portal.Shared/Api/WebhookInboxEndpoints.cs

src/Memex.Portal.Shared/Social/GitHubWebhookEndpoints.cs

src/Memex.Portal.Shared/Authentication/McpBackConnectionService.cs

src/MeshWeaver.Blazor/Infrastructure/UserIdentityCache.cs

src/Memex.Portal.Shared/Settings/UpdatePolicySettingsTab.cs

src/Memex.Portal.Shared/Settings/ApiTokensSettingsTab.cs

src/Memex.Portal.Shared/Authentication/OnboardingMiddleware.cs

src/Memex.Portal.Shared/Social/GitHubLoginEndpoints.cs

src/Memex.Portal.Shared/Authentication/DevAuthController.cs

This issue also appears on line 233 of the same file.

Suggested triage order

`OnboardingMiddleware` — verified live, and I am NOT fixing it

9 — `McpBackConnectionService`: confirmed, no eviction path exists

10 — `UserIdentityCache`: confirmed, still untested

Finding 5 is real, and it is the opposite of what it looks like

What finding 5 actually is, then

Walked all ten findings against `main` @ `aeecb0d3b` — every one is resolved

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Ten review findings on Memex.Portal.Shared surfaced by the GUI extraction — open redirects, an uncapped anonymous read, HMAC-after-buffer #2302

Description

src/Memex.Portal.Shared/Api/WebhookInboxEndpoints.cs

src/Memex.Portal.Shared/Social/GitHubWebhookEndpoints.cs

src/Memex.Portal.Shared/Authentication/McpBackConnectionService.cs

src/MeshWeaver.Blazor/Infrastructure/UserIdentityCache.cs

src/Memex.Portal.Shared/Settings/UpdatePolicySettingsTab.cs

src/Memex.Portal.Shared/Settings/ApiTokensSettingsTab.cs

src/Memex.Portal.Shared/Authentication/OnboardingMiddleware.cs

src/Memex.Portal.Shared/Social/GitHubLoginEndpoints.cs

src/Memex.Portal.Shared/Authentication/DevAuthController.cs

This issue also appears on line 233 of the same file.

Suggested triage order

Activity

rbuergi commented on Aug 26, 2026

rbuergi commented on Aug 28, 2026

OnboardingMiddleware — verified live, and I am NOT fixing it

rbuergi commented on Aug 28, 2026

9 — McpBackConnectionService: confirmed, no eviction path exists

10 — UserIdentityCache: confirmed, still untested

rbuergi commented on Aug 28, 2026

Finding 5 is real, and it is the opposite of what it looks like

rbuergi commented on Aug 28, 2026

What finding 5 actually is, then

rbuergi commented on Aug 28, 2026

Walked all ten findings against main @ aeecb0d3b — every one is resolved

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`OnboardingMiddleware` — verified live, and I am NOT fixing it

9 — `McpBackConnectionService`: confirmed, no eviction path exists

10 — `UserIdentityCache`: confirmed, still untested

Walked all ten findings against `main` @ `aeecb0d3b` — every one is resolved