Repository navigation
fix(auth): /authorize mints a LOCAL login returnUrl, and a source-side guard holds it there - #5160
Conversation
…e guard holds it there
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved validator, redirect-guard, retraction-sweep, and cross-replica test issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (5)
What changed in this PR
Fixes OAuth local return URLs, adds redirect-source validation, scopes the last-admin exemption, and adds token replication tests.
Changes:
- Mint local
/authorizereturn paths and document the redirect contract. - Add system-owned partition invariant coverage.
- Add cross-replica token validation coverage.
| File | Summary |
|---|---|
test/MeshWeaver.Graph.Test/LastAdminInvariantExemptsSystemOwnedPartitionsTest.cs |
Tests last-admin behavior across partition types. |
test/Memex.Portal.Shared.Test/TokenMintedOnOneReplicaValidatesOnAnotherTest.cs |
Tests shared-store token validation. |
test/Memex.Portal.Shared.Test/RedirectSourcesMintLocalTargetsGuard.cs |
Guards local redirect target sources. |
test/Memex.Portal.Shared.Test/OAuthAuthorizeReturnUrlIsLocalTest.cs |
Tests OAuth return URL preservation. |
src/MeshWeaver.Graph/Configuration/SpaceAdminInvariantValidator.cs |
Adds the system-owned partition exemption. |
src/MeshWeaver.Documentation/Data/Architecture/RedirectTargetContract.md |
Documents redirect-source and sink rules. |
src/MeshWeaver.Documentation/Data/Architecture/GrantingAccess.md |
Updates system-owned access guidance. |
src/MeshWeaver.Documentation/Data/Architecture.md |
Adds the architecture index entry. |
memex/Memex.Portal.Shared/Authentication/OAuthConnectController.cs |
Mints local /authorize return paths. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // 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. |
There was a problem hiding this comment.
Correct, and this is the most valuable finding on the PR — my change as it stands does not achieve what I claimed for it.
SystemOwnedAccessRetractionHandler pre-computes the spare and never issues the delete, so relaxing the validator removes a refusal that was no longer being reached. Its own docstring says 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." That reasoning was sound while the validator refused. Now that it does not, the pre-spare is the only thing left holding the grant, and my test — which drives Validate directly — measured the half I changed rather than the outcome I asserted.
Fixing it at the root: the handler only ever reaches its sweep for a partition that is system-owned (it returns early for a bijective sync), so the last-admin spare is unconditionally wrong there now rather than conditionally right. I am removing it rather than gating it, and adding an end-to-end assertion that the sweep leaves no privileged grant behind — which is the assertion that would have failed here and did not exist.
| 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."); |
There was a problem hiding this comment.
Right, and it contradicts the contract I wrote two paragraphs above it. All(v => !v.Resolved) reports only when nothing resolved, so a mixed expression — one local plus one unresolved member — passes. Changed to Any(v => !v.Resolved), which is what fail-closed means: if any carried value cannot be classified, the site is not clean.
Worth naming what this would have cost: the guard's whole claim is that an unresolvable site fails rather than being assumed local, so a hole in exactly that branch is the one defect that would make it agree with me instead of checking me.
| > **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. |
There was a problem hiding this comment.
Agreed — I softened the sentence instead of restructuring it, and the result reads as two conclusions in a row. It is also about to become wrong in the other direction once the handler's pre-spare goes (see the thread on SpaceAdminInvariantValidator), because then the sweep really does remove all of them.
Rewriting so the first paragraph states the rule as it will hold — no privileged grant survives on a system-owned partition, a bijective sync excepted — and the second carries the history as history: that the invariant used to refuse the removal, that the sweep therefore spared the last administrator, and that the combination is why a reporter read a working sweep as one that never ran. That last part is the piece worth keeping, since it is what caused the misfiling.
| private static readonly string[] TargetNames = | ||
| ["returnUrl", "returnTo", "returnPath", "redirectUrl", "redirectTo"]; |
There was a problem hiding this comment.
Accepted. A string[] is a mutable collection whatever the field's modifiers say, and this repository settled the point on itself earlier today — a static readonly array was changed to ImmutableArray on exactly this argument. Both sets here, TargetNames and NotAValue, become ImmutableArray<string>.
The neighbouring sink guard has the same shape and is out of scope for this PR, but it is the same finding and I would not want the precedent read the other way.
| [Fact] | ||
| public void Every_minted_return_target_is_a_local_path() | ||
| { | ||
| var offenders = new List<string>(); |
There was a problem hiding this comment.
Taking this, and going further than an immutable builder: the accumulator existed only because the scan was written as a nested loop with two side effects — the offender list and a sitesSeen counter. Restructuring it so the minting sites are produced as a sequence, and both the count and the offenders are derived from that sequence, removes the mutable accumulator rather than wrapping it.
That also makes the vacuity assertion read off the same sequence the verdicts come from, so "did this guard have a subject at all" and the verdicts themselves can no longer disagree.
…go with it
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<string>: an array is a mutable
collection whatever the field's modifiers say.
* The mutable List<string> 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) <noreply@anthropic.com>
What this fixes
/authorizeminted a return target that every sink downstream must refuse (#5074), and theresult was a sign-in that succeeded and connected nothing.
Every
returnUrlsink in the portal validates local-only —ReturnUrlPolicy.Sanitizeon the serversurface, 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 right and are not touched here.
/authorizewas minting{Scheme}://{Host}{Path}{QueryString}, which is not local, so the value was discarded rather thanfollowed: the login page substitutes nothing for a target it will not keep, the provider link
carried no
returnUrlat all, and the user landed on/after signing in with the/authorizerequest gone. No authorization code was issued, so the client waited for a completion that could not
arrive — and the user, who had signed in, read it as the credential expiring again.
Nothing in that sequence logs an error, which is why it was filed as an intermittent expiry. The
absolute form also bought nothing:
/loginand/authorizeare the same origin.OnboardingMiddleware, two files away, carries the same request correctly and says why.The durable half: a guard on the SOURCE end
RedirectSinksUseOnePolicyGuardpins that every consumer routes an incoming target through the onepolicy. It was green throughout this defect, and correctly so — nothing consumed an unvalidated
value. The contract has two ends and only one had a guard.
RedirectSourcesMintLocalTargetsGuardcloses the other. For each minting site acrossmemex/Memex.Portal.Sharedandsrc/it resolves the interpolated value to its assignment in thesame file and:
guard's subject where it lands;
the roots move.
On its first run it 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 ahuman. The fail-closed branch was right to report something it could not resolve; prose is now not a
subject, with the reasoning written down rather than the filter applied silently.
Measured on both sides of the change
0 Error(s)0 Error(s)The guard's pre-fix failure names the offender exactly. The two
Sanitize(absolute) == "/"casespass in both states by design, so no constant satisfies the pair — that is a control on each side
rather than a test that agrees with the change.
Also in this change set
#5140: the documented end state was unreachable by construction, and it took TWO changes. Three
reported symptoms are one mechanism. A partition with a one-way
_GitSyncis rewritten from itsrepo on every sync, so
AccessAssignmentGuard.IsForbiddenOnSystemOwnedrefuses granting another Adminthere — while
SpaceAdminInvariantValidatorrefused removing the existing one, exempting onlysystem-managed mirrors.
SystemOwnedAccessRetractionHandlertherefore pre-detected that guaranteedrejection and SPARED the last grant, on every pass, and the escape it documented ("retracted next time,
once another admin exists") could never arrive.
🚨 Exempting the invariant alone is INERT, and strictly worse than unfixed — caught in review here.
The sweep still never issues the delete, so the grant survives with nothing reporting it: a stated
outcome becomes a silent one. The pre-spare is removed rather than gated, because 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.
Nobody is stranded, and that is structural rather than true-today:
PermissionEvaluatorreturnsPermission.All | Sync | CompileforWellKnownUsers.Systembefore any grant is folded, sosystem-securityadministers every partition with no assignment node to delete; and a platform admincan always delete the
_GitSyncof a system-owned Space (already asserted bySystemOwnedSyncConfigIsVisibleToPlatformAdminsTest), which un-system-owns the partition and makesordinary granting legal again.
A bijective sync is deliberately not exempt: there the mesh nodes are somebody's working copy.
Controls, both sides, both layers. Validator: pre-fix only the system-owned case fails, the
ordinary and two-way cases pass unchanged. Sweep (
SystemOwnedSweepRetractsTheLastAdminTest, drivingthe real post-creation handler): pre-fix the one-way case is RED and the bijective case green; post-fix
4 of 4 including the sibling system-owned suite.
🚨 That end-to-end test's FIRST version passed 2/2 against the unfixed sweep — 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 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 non-vacuous and the sentence the handler's own docstring makes.GrantingAccess.mdasserted something measurably false — that the sweep "deletes such grants themoment the
_GitSyncis wired, so the shape is unsatisfiable". It does not; it spares the lastadministrator by design. That prose is what made a working sweep read as one that never fired, so it
is a documentation defect rather than a tidy-up.
A two-replica control for the token path. #5074 reasoned from outside the cluster that the token
must live in the issuing process's memory. It does not —
ApiTokenServicereads both rows straightoff the shared store — but that is a claim about code, and the two shapes it replaced were invisible
to every single-instance test in the suite.
TokenMintedOnOneReplicaValidatesOnAnotherTestmints onone service instance and validates on another sharing the store, which is the relationship two pods
have to one database. Its companion keeps an unknown token a definitive
Invalid, neverUnavailable— the distinction the 401/503 split rests on, and the one that makes the 401 in #5074a statement about the row rather than about reachability.
Issue disposition
sev:H) stays OPEN. This fixes the recovery defect — re-authentication nevercompleting — and proves it. It does not explain the intermittent 401 on an already-working
token, which is a second, unfixed defect; the issue will say both halves separately. Closure also
awaits production verification.
sev:M) — root cause fixed here; the remaining asks in it (a non-system write boundaryat the owning hub, and a forced re-sync at an unchanged fingerprint) are named in the issue.
Pairs-with: none — no public type or member is removed; the change adds a branch and three tests.
Mirror-sync: none — no localization catalog key is added or re-worded (the one new log line is
operator-facing, not user-visible).
🤖 Generated with Claude Code