Repository navigation
fix: the delivery gate consults the node type's own access rule (#3061) - #3100
Merged
meshweaver-cloud[bot] merged 1 commit intoSep 2, 2026
Merged
Conversation
`ValidateDeleteRequest` carries `[RequiresPermission(Permission.Delete)]`, and `AccessControlPipeline` evaluated it through `GetPermissionChecks`, whose default implementation yields a RAW `(hubPath, Permission)` pair. It never consulted `INodeTypeAccessRule` — while `SatelliteAccessRule`, registered by `ActivityNodeType` and every other satellite type, maps a satellite's Delete to `Permission.Update` on its `MainNode`. #2913 reconciled exactly that mismatch in the delete PRE-FLIGHT (`MeshExtensions.CheckDeletePermissionForNode`) and not in the delivery gate, which runs FIRST — so the gate won. Measured on memex.systemorph.com: a recursive delete of the orphan NodeType `Edu/Course` refused with "Access denied: user 'rbuergi' lacks Delete permission on 'Edu/Course/_Activity/compile-…'" for all 72 of its `_Activity` satellites, so the one repair for a dangling NodeType was unavailable through any API. One shared seam so the two cannot drift again: `NodeTypeAccessRuleGate` owns resolution AND evaluation — the three terminals (verdict / fault / completed-without-emitting), the `TakeDecisionOutsideGate` hop, and the "the caller-visible detail names the exception TYPE, never its message" rule. The pre-flight, which used to write the evaluation out inline, now calls it; so does the gate. In the gate the rule is reached ONLY from a definitive denial: a grant is delivered untouched (a hot SubscribeRequest reads no node) and an Undetermined fold stays Undetermined ("we could not check" is not a rule question). Four conditions leave the denial exactly as it was — the check is not on this hub's own path, the permission names no operation a rule can decide about that node (`Permission.Create` maps to none: a create's subject does not exist yet and the gate sits on the PARENT's path), nothing is served at that path, or no rule governs. A node read that FAULTS answers Undetermined rather than the original denial, so a transient storage fault cannot silently restore the pre-fix behaviour. `SatelliteDeliveryGateTest` pins the three verdicts on one mesh: the Editor's satellite pre-flight is served, a Viewer's is still refused Unauthorized (the rule DENIES), and the same Editor at a plain Markdown child is still refused (no rule, nothing widened). Reverting `AccessControlPipeline.cs` alone reproduces the production message verbatim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPe8CH4mFU9E74HafWppqL
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes a core authorization gate (AccessControlPipeline) behavior in a security-sensitive path, so it warrants final human review despite strong test coverage.
Pull request overview
This PR fixes a mismatch between the message-delivery access gate (AccessControlPipeline) and node-type-specific access rules (INodeTypeAccessRule): when a [RequiresPermission] check is denied on a node whose type declares a governing access rule (e.g., satellites like _Activity/*), the gate now re-decides using that rule (aligning delivery-time gating with delete pre-flight behavior).
Changes:
- Introduces
NodeTypeAccessRuleGateas the shared implementation for (permission→operation) mapping, rule lookup, rule evaluation (including the three terminals), and served-node resolution. - Updates
AccessControlPipelineto reconsider definitive denials via the node type’s rule (without affecting granted/undetermined outcomes). - Adds a regression test plus doc + What’s New updates to pin and explain the behavior.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Graph.Test/SatelliteDeliveryGateTest.cs | Adds a focused regression test pinning the three verdicts that distinguish “consult the rule” from “widen Delete”. |
| src/MeshWeaver.Mesh.Contract/Services/NodeTypeAccessRuleGate.cs | New shared seam for rule lookup + evaluation + subject-node reading, to prevent drift between delivery gate and delete pre-flight. |
| src/MeshWeaver.Mesh.Contract/MeshExtensions.cs | Routes delete pre-flight rule resolution/evaluation through NodeTypeAccessRuleGate to keep seams aligned. |
| src/MeshWeaver.Hosting/Security/AccessControlPipeline.cs | Re-decides denied [RequiresPermission] checks through the node type’s access rule when applicable. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-deleting-a-node-takes-its-bookkeeping-with-it.md | Adds a user-facing “Fix” entry describing the behavioral correction at a high level. |
| src/MeshWeaver.Documentation/Data/Architecture/AccessControl.md | Updates the access-control architecture doc to reflect and justify the delivery-gate change (and its boundaries). |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
rbuergi
marked this pull request as ready for review
September 2, 2026 11:13
This was referenced Sep 2, 2026
rbuergi
added a commit
that referenced
this pull request
Sep 2, 2026
… creates no false refusal Making the session's Recycle pre-flight live (previous commit) was only safe if it asks what the OWNER asks. Since #3061/#3100 the delivery gate re-decides a denied raw check through the node's registered INodeTypeAccessRule — 'a satellite's write is Update on its MainNode'. The pre-flight was a raw path fold, so switching it on would have refused an editor entitled via a satellite's MainNode whom the owner grants: a new false refusal shipped by a fix for a legibility bug. It now takes the same second opinion, from the same NodeTypeAccessRuleGate helpers, on the same condition — a definitive denial only, never an undetermined fold. Pinned by RecyclingASatelliteFollowsItsNodeTypesRule_NotTheRawPath. Negative control: with the reconsideration removed it fails 'Expected "Recycled" ... but found "Error"'. Its fixture points the satellite's MainNode into another partition on purpose — a satellite under its own main node inherits that node's grants by path, so raw and rule agree by construction and the case would prove nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rbuergi
added a commit
that referenced
this pull request
Sep 2, 2026
…swer (#3121) (#3127) * fix: a session's permission check can actually fail, and a denial is an answer (#3121) An MCP `recycle` on a node the caller could not write reached the client as `"recycle" threw an unhandled exception` plus a stack trace — indistinguishable from a broken tool. `MeshOperations.Recycle` has carried a legible refusal since #2901; it had never run. `HubPermissionExtensions.ResolveEvaluator` reads the evaluator off the hub's own Configuration, does not walk the parent chain, and falls back to `DefaultEvaluator` => `Permission.All`. A hub's configuration starts empty, and `AddRowLevelSecurity()` installs the evaluator on the mesh hub and per-node hubs only. `SessionHubFactory` — the one factory behind MCP, REST, gRPC and the CLI — never copied it, so every client-side check those surfaces issued answered granted, for every caller, on every path. Measured on a real mesh with a Viewer (Read|Execute|Api), through a real session hub: recycle a NodeType THREW UnauthorizedAccessException -> refusal envelope recycle a plain node {"status":"Recycled"} (disposed!) -> refusal envelope create Created: ... (node written!) -> Access denied export every node in the subtree -> only permitted ones The recycle-a-plain-node case is why the 2026-08-30 fix could not close this: the release-request stamp only writes on a NodeType, so on anything else nothing was posted, nothing was gated, and the destructive half ran free. Authorization that is a side effect of a write disappears when the write does. Two halves: - `SessionHubFactory` copies the mesh hub's `EffectivePermissionsDelegate` into every session hub — exactly what `MeshExtensions.NodeOperationExecutionHub` already does, for the reason its comment spells out. Nothing is widened: the owner already refused everything the pre-flight now refuses, and a mesh built without RLS stays ungated because there is no delegate to copy. - `RecycleCore` renders the OWNER's refusal in the operation's own envelope instead of re-raising it. A pre-flight is check-then-act; the owner stays the authority, at a different hub and a later moment. A verdict and a non-verdict stay apart. `MeshOperations.IsWriteDenial` decides on the typed failure — `UnauthorizedAccessException` / `DeliveryFailure{Unauthorized}` are the only verdicts the write path mints; `ShuttingDown`, `Unavailable`, a timeout and anything unrecognised are not, because the default must never accuse (#974, #3017). Also corrects `MeshOperations.Export`'s comment, which claimed its per-node Export filter ran on the mesh hub. It runs on the session hub — which is why the filter was inert, and why this change repairs it. Doc: Doc/Architecture/DenialIsAnAnswer + a What's New entry. Not localised, deliberately: these are JSON envelope fields on the machine tool surface, read by an agent runtime that renders its own text to the human — the same side of the line as an LLM tool parameter's [Description]. There are zero `Localize` calls in MeshWeaver.Mesh.Operations; the human-facing refusal is the portal's typed error card, which is localised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review: name the fixture the pre-fix measurement came from, and why the envelope proves nothing was performed Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Doc: name create's own mechanism — RlsNodeValidator resolved against the session hub is not a legibility problem Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * The pre-flight takes the node type's own second opinion, so waking it creates no false refusal Making the session's Recycle pre-flight live (previous commit) was only safe if it asks what the OWNER asks. Since #3061/#3100 the delivery gate re-decides a denied raw check through the node's registered INodeTypeAccessRule — 'a satellite's write is Update on its MainNode'. The pre-flight was a raw path fold, so switching it on would have refused an editor entitled via a satellite's MainNode whom the owner grants: a new false refusal shipped by a fix for a legibility bug. It now takes the same second opinion, from the same NodeTypeAccessRuleGate helpers, on the same condition — a definitive denial only, never an undetermined fold. Pinned by RecyclingASatelliteFollowsItsNodeTypesRule_NotTheRawPath. Negative control: with the reconsideration removed it fails 'Expected "Recycled" ... but found "Error"'. Its fixture points the satellite's MainNode into another partition on purpose — a satellite under its own main node inherits that node's grants by path, so raw and rule agree by construction and the case would prove nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3061.
The defect
ValidateDeleteRequestcarries[RequiresPermission(Permission.Delete)]. The delivery gate(
AccessControlPipeline) evaluates it throughRequiresPermissionAttribute.GetPermissionChecks,whose default implementation is literally
yield return (hubPath, Permission)— a rawDeleteon the satellite's own path. It never consulted
INodeTypeAccessRule.ActivityNodeType(and every other satellite type) registersSatelliteAccessRule, which maps asatellite's Delete to
Permission.Updateon itsMainNode— creating a satellite is amodification of its main node, and so is removing it. #2913 reconciled exactly that mismatch in
the delete pre-flight (
MeshExtensions.CheckDeletePermissionForNode) but not in the delivery gate,which runs first — so the gate won.
Measured on production (memex.systemorph.com, 2026-09-02): a recursive delete of the orphan NodeType
Edu/Courserefused withfor all 72 of its
_Activitysatellites — a check the node's own registered rule says should pass.The one repair for a dangling NodeType was therefore unavailable through any API.
The fix
One shared seam so the two cannot drift again. New
NodeTypeAccessRuleGate(src/MeshWeaver.Mesh.Contract/Services/NodeTypeAccessRuleGate.cs) ownsresolution and evaluation — the three terminals (verdict / fault / completed-without-emitting), the
TakeDecisionOutsideGatehop, and the "the caller-visible detail names the exception TYPE, never itsmessage" rule.
CheckDeletePermissionForNodeandCheckDeletePermissionByRulewere rewritten tocall it (they used to write the evaluation out inline — which is how the seams diverged), and
AccessControlPipelinenow calls it too.Where the reconsideration sits in the gate, reached ONLY from a definitive denial:
(hubPath, Permission)SubscribeRequestreads no nodeUnavailable— "we could not check" is not a rule questionFour conditions leave the denial exactly as it was: the check is not on this hub's own path; the
permission names no operation a rule can decide about that node; nothing is served at that path; or
no rule governs
(node type, operation).Permission.Createdeliberately maps to no operation: a create names a node that does not existyet and the gate evaluates on the PARENT's hub path, so the type a lookup here would find is the
parent's, and the parent's rule has no standing over a child's creation. Everything outside the CRUD
four maps to nothing for the matching reason —
INodeTypeAccessRule.SupportedOperationsis expressedin
NodeOperations.No skip-trapdoor: a node read that FAULTS answers
Undetermined, never the original denial.Falling back to the denial would let a transient storage fault silently restore the pre-fix
behaviour — the gate's own input deciding whether the gate ran.
Verification
dotnet build … -c Release -warnaserror, one project per invocation,0 Error(s)/0 Warning(s)on all nine touched-or-dependent projects.
AccessControlPipeline.csreverted toorigin/mainand everything else in place,AnEditorsSatellitePreflight_IsNotRefusedByTheGatefails with the production message verbatim:
Access denied: user 'satellite-editor' lacks Delete permission on 'SatelliteDeleteGate/Doc/_Activity/run1'.--filter, so the ratchet guards actually run) ofMeshWeaver.Graph.Test,MeshWeaver.Documentation.Test,MeshWeaver.Hosting.Test,MeshWeaver.Security.MeshTest,MeshWeaver.Data.Test,MeshWeaver.Messaging.Hub.Test.SatelliteDeliveryGateTestpins three verdicts on one mesh, so a fix that merely widened Deletewould fail two of them: the Editor's satellite pre-flight is served; a Viewer's is still refused
Unauthorized(the rule DENIES — consulting a rule never means granting); and the same Editor at aplain
Markdownchild of the same node is still refused (no rule, nothing widened).No band-aids
No timeout was raised, no retry added, no non-answer treated as consent, no permission widened. The
only behaviour change is that a denial on a rule-governed node type is now decided by that rule —
the same authority the delete pre-flight has consulted since #2913.
Docs
Doc/Architecture/AccessControl.md— the paragraph that used to say the message-level gate wasdeliberately not routed through the rule ("a separate decision with a much larger blast radius; it
has not been made") is replaced with the decision, the measurement, the ordering table and the two
deliberate exclusions. Plus a
Category: FixWhat's New entry.🚩 One thing that does NOT fully reconstruct, and the maintainer asked to hear it
The mechanism is confirmed and now pinned by a test that reproduces the production message verbatim.
The production grant topology, however, does not by itself explain the reported denial, and I
could not close that gap read-only:
Edu/_Accessholds exactly three assignments —system-security,Public — Viewer,Anonymous — Viewer. There is norbuergigrant anywhere underEdu, no root_Accessgrant for
rbuergi(his 34 assignments are all per-partition; the platform-admin one isAdmin/_Access, which by design confers no data access), and zeroGroupMembershipnodes.Edu/_Policyis{ publicRead: true }— no cap, nobreaksInheritance.pre-validate-descendantsstage, which only happens afterthe ROOT check (
DeleteonEdu/Course.MainNode ?? Path=Edu/Course) has passed.GetScopeHierarchyis a plain path walk, so any inheritable Delete grant that let the rootthrough would also have covered
Edu/Course/_Activity/compile-…— and the gate would have grantedthere too.
So the grant that let the root through is not visible in the mesh's
AccessAssignmentrows —most likely a static assignment seeded into the deployment (
CollectStaticAccessAssignments,invisible to
search) — and if it is inheritable the two facts do not sit together. This does notchange what the fix does or that it is correct, but the exact production trigger is not fully
reconstructed and someone with a shell on that portal could settle it in one
GetEffectivePermissionscall.Secondary defects — NOT fixed here, please file
AccessControlPipeline's fold has noTimeout. Correct as stated for a stall: a fold thatnever emits parks
Concatand the delivery is dropped with noDeliveryFailureand no log line.Left out on purpose — the file carries an explicit, argued "No Timeout here" decision, the
cold-start silence measured on DeleteNode pre-validate-descendants: 150 of 150 Release records of an orphan NodeType never answer ValidateDeleteRequest — the orphan cannot be deleted #3061 was per-node-hub ACTIVATION (upstream of this fold, so a gate
timeout would not have produced that message), and bounding it needs its own rung on the
MeshOperationOptionsladder plus a measurement of what actually stalls.Correction to the brief: the empty-completion half is already covered —
CheckPermissionOutcomematerialises the silent terminal asUndetermined(A starved permission fold parks every [RequiresPermission] delivery until the caller's own timeout — AccessControlPipeline has no terminal, and the bounded twin attributes the same event correctly #2742), soDefaultIfEmpty()here can only mean "every check was granted".PreValidateDescendantsObsbounds an O(N) cold fan-out with ONE stage budget. Real, and thefix is a per-descendant rung, not a bigger number — a design change with the A recursive delete's commit stage can stall for its whole budget — pre-flight fan-out and budget nesting are fixed and deployed; the commit stall is unmeasured on memex-cloud #1198 ladder
invariant at stake.
HandleValidateDeleteRequestputsNestedTimeouton the storage read only;RunDeletionValidatorsObsruns unbounded. Also real, and also a ladder decision: rung 2 isalready spent on the read and rung 3 is
RlsNodeValidator's own budget, so bounding the validatorchain needs a new rung — equal budgets are exactly the "inner bound can never fire" defect
A recursive delete's commit stage can stall for its whole budget — pre-flight fan-out and budget nesting are fixed and deployed; the commit stall is unmeasured on memex-cloud #1198 exists to remove.
AccessControl.mdcitedDeleteHonoursNodeTypeAccessRuleTestattest/MeshWeaver.Security.Test, a path that does notexist in this repo. The test is real but lives in MeshWeaver.Plugins
(
src/MeshWeaver.Security.Test); the reference now says so.🤖 Generated with Claude Code
https://claude.ai/code/session_01PPe8CH4mFU9E74HafWppqL