Repository navigation
fix(security): runtime grants/denies decide the next check; DeleteMeshNodeRequest and workspace MeshNode deletes run the delete checks - #5976
Merged
Conversation
…hNodeRequest and workspace MeshNode deletes run the delete checks Three paths around the access model, each with a failing-first test and its control. 1. PermissionEvaluator emitted a synchronous snapshot built from the STATIC assignments and policies before the enriched fold. One-shot consumers (the delivery gate's Take(1), CheckPermission awaited once) took it as the verdict, so a runtime deny of a statically granted role was ignored for the life of the process (fold emitted [true, false]), and a runtime grant layered on a static one answered a spurious first denial. The seed is removed; the first emission is the enriched fold, which already unions the static layer. 2. The per-node DeleteMeshNodeRequest handler called IStorageAdapter.Delete on any path with no permission check and no validator. SignalR/gRPC ingress forwards any delivery to any address (unauthenticated = Anonymous) and in-mesh code holds a hub, so it was an unchecked delete. It now forwards to DeleteNodeRequest under the delivery's own identity (refusing a delivery with none); the record is [Obsolete] and no longer [SystemMessage]. 3. A MeshNode removed through a DataChangeRequest (MeshNodeTypeSource -> DeleteAndPublish) ran RlsDataValidator only. MeshNodeDeletionDataValidator runs the INodeValidator delete chain beside it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Test Results (shard 1) 3 files + 2 3 suites +2 5m 40s ⏱️ + 1m 0s Results for commit e953e59. ± Comparison against base commit ba76c8a. This pull request removes 1 and adds 1219 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 4)767 tests - 71 574 ✅ - 73 2m 2s ⏱️ - 5m 50s Results for commit e953e59. ± Comparison against base commit ba76c8a. This pull request removes 643 and adds 556 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 3) 4 files - 1 4 suites - 1 13m 36s ⏱️ + 5m 12s Results for commit e953e59. ± Comparison against base commit ba76c8a. This pull request removes 2337 and adds 2439 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 2)1 110 tests +77 1 110 ✅ +77 4m 1s ⏱️ +32s Results for commit e953e59. ± Comparison against base commit ba76c8a. This pull request removes 3 and adds 78 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 5) 4 files ± 0 4 suites ±0 12m 41s ⏱️ + 1m 30s Results for commit e953e59. ± Comparison against base commit ba76c8a. This pull request removes 2436 and adds 1115 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results 20 files +1 20 suites +1 41m 13s ⏱️ + 2m 29s Results for commit e953e59. ± Comparison against base commit ba76c8a. This pull request removes 29 and adds 16 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
The change closes three gaps in one pass. (1) `PermissionEvaluator.GetEffectivePermissionsCore` drops its synchronous static-only snapshot, so every one-shot verdict — the delivery gate's `Take(1)`, a single `CheckPermission`, the RLS validator — is the enriched fold's first emission, and runtime denies/caps bind even when a static grant supplies the role (fail-closed, the right direction). (2) The `DeleteMeshNodeRequest` per-node handler forwards to the validated `DeleteNodeRequest` under the delivery's own `AccessContext` (context-less deliveries refused) instead of a raw unchecked `IStorageAdapter.Delete` of any path; the record is `[Obsolete]` and no longer `[SystemMessage]`. (3) A new `MeshNodeDeletionDataValidator` runs the node delete-validator chain for MeshNodes removed through a workspace `DataChangeRequest`, beside `RlsDataValidator`. Three executing tests with sibling/admin controls pin each fix, and the doc changes match the code. Checked from the diff: the forwarder's identity handling (identity from the delivery context, request fields derived from it — nothing spoofable from message content), the fold's fail-closed direction, the validator's operation/entity guard, and the `PostCommitFlushRegistry.Forget` duty moving to the shared delete path. Not verifiable from the diff: the claim that nothing in core or Plugins posts `DeleteMeshNodeRequest` (grep asserted), internals visibility for the new `internal` `RunDeletionValidators` call from MeshWeaver.Graph, the CI claims (suites green, warnaserror clean), Postgres-plugin read-after-write latency (explicitly not measured), and `MeshNodeTypeSource.DeleteAndPublish`'s recursion. One blocking finding: the `#pragma warning disable CS0618` sites in production code, against the repo's no-pragma-to-silence-a-warning rule.
Findings: 1 blocking · 0 should-fix · 3 question · 1 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
question src/MeshWeaver.Mesh.Contract/Security/PermissionEvaluator.cs
At the top of the same method a synchronous fast path survives untouched: `... && IsHubReadableScope(userId, nodePath)) return Observable.Return(Permission.Read);`. This change removes the static snapshot seed for exactly that reason — a synchronous verdict computed without the runtime layer cannot see a runtime subtraction — and AccessControl.md now states 'The seed is gone: the first emission is the enriched fold's'. The diff does not show the guard's full condition: if `IsHubReadableScope` resolves from static inputs only, a runtime deny at such a scope still does not bind one-shot Read checks, and the first emission for those paths is not the enriched fold's — the same hazard class this change fixes for the fold. If the check is runtime-aware this is moot; the diff cannot answer it.
Internal review of 85d2c9b273f9841dfb276343495b479cee62096d — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| // and keeps the data source pure — no debounce buffer, no FlushOnDispose, | ||
| // no IStorageAdapter dependency in the type source itself. | ||
| .WithHandler<SaveMeshNodeRequest>(HandleSaveMeshNode) | ||
| #pragma warning disable CS0618 // forwarded to the validated delete — see HandleDeleteMeshNode |
Contributor
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
MeshWeaver's hard rules include: no `!` or `#pragma` to silence a warning. This change introduces three `#pragma warning disable CS0618` sites to keep referencing `DeleteMeshNodeRequest` after marking it `[Obsolete]`: line 285 around the `.WithHandler<DeleteMeshNodeRequest>` registration, a second disable/restore pair around the `HandleDeleteMeshNode` signature (~line 518), and a file-level disable in test/MeshWeaver.Hosting.Test/DeleteMeshNodeRequestIsAValidatedDeleteTest.cs. Each suppression is scoped and states its reason, so nothing is hidden — but the rule as written carries no exemption, and a compliant shape exists: the forwarder, not the attribute, is what closes the hole, so the record can be marked `[Obsolete]` in a follow-up change once nothing references it, keeping pragma-suppressed warnings out of production code.
Contributor
Author
There was a problem hiding this comment.
Fixed in e953e59. [Obsolete] is gone from DeleteMeshNodeRequest, and with it all three #pragma warning disable CS0618 sites (the handler registration, the handler signature, and the test file).
The record's doc now says it is legacy and why it is not [Obsolete]: the forwarder is what closes the hole, not the attribute.
|
|
||
| return seed.Concat(enriched) | ||
| return enriched | ||
| .SelectMany(state => |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
Removing the seed makes every one-shot verdict wait for the enriched fold's first emission, which waits for the anchored/synced reads. The comment this diff deletes states the seed existed precisely because skipping it 'left role-less readers of a public catalog blocked on the synced cold-start path — the "No suitable agent" race during execution', and the PR measures only the in-memory mesh — the Postgres pg_notify path is explicitly 'not measured here'. The new tests pin verdict correctness on a hot, already-opened query, not first-emission latency for a cold role-less reader of a public catalog. Waiting is fail-closed and the right direction, but the cold-start envelope for the un-seeded fold — including the plugin storage path — appears neither re-measured nor pinned anywhere in this change; without that, the race the seed was added to fix may be reintroduced.
Contributor
Author
There was a problem hiding this comment.
That's right: this change does not measure or pin cold-start latency, on in-memory or on Postgres. I'll say exactly what is and isn't known.
Why the seed cannot stay. The seed was not a latency optimisation that happened to be imprecise. It was the verdict. AccessControlPipeline takes the first emission (Take(1)), so the static-only snapshot answered every one-shot check. A runtime deny over a static grant never bound. That is measured: RuntimeAccessChangeIsEffectiveTest fails on the parent commit with "found True". The convergence contract in AccessControl.md already reaches this conclusion for single-leg seeds: no permissive seed is not a hole. That rule applies here unchanged.
What the reader waits on now. A cold role-less reader waits for the four anchored queries' first answer. If one never answers, the fold terminates as Undetermined → ErrorType.Unavailable. It does not hang silently, and it is fail-closed. The suites that exercise first-emission behaviour on the in-memory mesh pass: MeshWeaver.Hosting.Test (1057 passed) and MeshWeaver.Graph.Test (2400). That includes PermissionFoldGateTests, PermissionFoldLegSeedGuardTest and AFreshGrantIsVisibleToTheNextCheckTest.
Not established.
- The Postgres cold-start envelope.
- The AI engine's "No suitable agent" path. It lives in MeshWeaver.Plugins and is not run here.
If that path regresses, the cure is a faster answer from the anchored reads, not a seed. I'm leaving this thread open for that measurement.
|
|
||
| /// <inheritdoc /> | ||
| public IObservable<DataValidationResult> Validate(DataValidationContext context) | ||
| { |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The validator runs the delete chain for each MeshNode the change's `Deletions` names, and `RunDeletionValidators` passes `new DeleteNodeRequest(node.Path)` with `Recursive` at its default false — the named node only, non-recursively. Whether the storage delete behind `MeshNodeTypeSource.DeleteAndPublish` cascades to descendants is not visible in this diff; if it does, a `DataChangeRequest` naming only the parent removes descendants that never pass this validator, and the descendant guards (partition-root, in-use, keep-one-admin) remain reachable-around by posting a change instead of a recursive delete — the same bypass class this change closes for named nodes. If `DeleteAndPublish` is non-recursive this is moot; the diff does not show which.
Contributor
Author
There was a problem hiding this comment.
It is non-recursive. DeleteAndPublish is adapter.Delete(path).Do(publish), and IStorageAdapter.Delete is documented as "Deletes a node from storage and emits the deleted path". That is one row; the recursive delete does its own descendant fan-out in the DeleteNodeRequest handler.
So a DataChangeRequest naming a parent removes that row only. Validating the named node non-recursively matches what storage actually removes. Descendants are not deleted by this path, so they need no validation on it.
| Observable.Interval(TimeSpan.FromMilliseconds(50)).StartWith(0L) | ||
| .SelectMany(_ => Storage.Read(path, Mesh.JsonSerializerOptions).DefaultIfEmpty(null).Take(1)) | ||
| .Where(node => node is null) | ||
| .Select(_ => true) |
Contributor
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The private helper `StoredAt` is defined but never called in this file — both waits go through `Gone`. The sibling test uses `StoredAt(target)` for its refused-node storage assertion; here the helper is dead code.
Contributor
Author
There was a problem hiding this comment.
Removed in e953e59. The refused-node assertion there is the Gone(...).NotEmit window, so the helper was unused.
…equest through the issuing seam - Hosting.Test measured 364-367 s solo on recent PR runs (76% of the 480 s per-project cap; the table said 27 s) and hit exit=124 on this branch once the new security tests added their negative waits. Split rule (2) applies. - MeshDataSource now calls NodeOperationIssuingHub() on its `hub` receiver (the DeleteMeshNodeRequest forwarder), which declares that receiver router-capable for RouterAsRouterCapableReceiverRatchetGuard; the one remaining targeted post on it (HandleRunTests -> SubmitCodeRequest) goes through the same seam — the identity function on a per-node hub. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ions); drop a dead test helper Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Oct 1, 2026
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
1 participant
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 three triage items from
rbuergi/Feedbackon memex.systemorph.com:delete-meshnode-request-raw-storage-delete,runtime-deny-grant-not-applied-in-monolith-fixtureandworkspace-meshnode-delete-skips-node-validators.1. A runtime deny was ignored when a static grant supplied the role
Root cause.
PermissionEvaluator.GetEffectivePermissionsCoreemitted a synchronous "fast" snapshot built only from the STATIC assignments and policies (AddMeshNodes/IStaticNodeProvider). It then concatenated the enriched fold. One-shot consumers take the first emission as the verdict: the delivery gate'sTake(1), aCheckPermissionawaited once, and the RLS node validator. So any runtime change that subtracts (aDeniedrole, a capping or inheritance-breaking_Policy) did not bind them while a static grant or a staticPublicReadpolicy supplied the role.[true, false].Docpartition ships a staticPublic→Viewergrant and a staticPublicReadpolicy.Fix. Removed the seed. The first emission is now the enriched fold, which already unions the static layer.
Read-after-write. On the in-memory mesh the partition query already reflects the write on the next check.
RuntimeAccessChangeIsEffectiveTestchecks this on a hot, already-snapshotted query, andAFreshGrantIsVisibleToTheNextCheckTestcovers grants. The Postgres pg_notify path lives in MeshWeaver.Plugins and is not measured here.2.
DeleteMeshNodeRequestwas an unchecked raw delete reachable from ingressRoot cause. The per-node handler called
IStorageAdapter.Delete(path)on any path. It ran no permission check and no validator, and the record was[SystemMessage]with no[RequiresPermission]. Nothing in core or Plugins posts it, but every ingress can deliver it (read from source, not exercised live):SignalRConnectionHub.DeliverMessageforwards any delivery to any address. Anonymous callers are stamped Anonymous.MeshGrpcService.Deliver/Open, mapped withAllowAnonymous.IMessageHub Mesh.WithHandler<T>registers the type, so the message deserializes on the target hub.Fix. The handler now forwards to the validated delete: a
DeleteNodeRequestwith the sameRecursiveflag andDeletedBy, posted under the delivery's ownAccessContext. That gives it the Delete gate, the validator chain and the recursive pre-flight. A delivery that carries no context is refused. The record is kept as a forwarder for in-mesh source (documented as legacy, not[Obsolete], so no warning suppression is needed) and is no longer[SystemMessage].3. A workspace MeshNode delete skipped the node validators
Root cause. A
MeshNodeleaving a hub's collection through aDataChangeRequestis deleted raw byMeshNodeTypeSource→DeleteAndPublish, and onlyRlsDataValidatorchecked it.Reachability, measured. This path is only reached while the collection keeps another node. Removing a hub's ONLY node empties the collection, and the activation race guard treats that as a no-op that still answers
Committed. When a second node sits in the collection (added through a data change), the delete reaches storage.Fix.
MeshNodeDeletionDataValidatorruns theINodeValidatordelete chain beside RLS.Evidence
Each test failed on the unfixed code and passes with the fix.
RuntimeAccessChangeIsEffectiveTest: deny caseRuntimeAccessChangeIsEffectiveTest: grant caseDeleteMeshNodeRequestIsAValidatedDeleteTest(Viewer and Anonymous)AWorkspaceMeshNodeDeleteIsValidatedTestEach test carries its control:
Full
MeshWeaver.Hosting.Testpassed: 1057 passed, 0 failed, 1 skipped. FullMeshWeaver.Graph.Testpassed: 2400 total, 0 failed.-c Release -warnaserroris clean on Mesh.Contract, Graph, Messaging.Hub, Documentation and Hosting.Test.Not done here, filed instead
rbuergi/Feedback/save-meshnode-request-raw-storage-write:SaveMeshNodeRequesthas the same ingress exposure as a raw WRITE. It cannot be forwarded the same way, because it IS the persistence step. It needs an ingress filter or a self-post check.DataChangeRequestis written raw byMeshNodeTypeSource, and only RLS checks it. The test above uses that to put a second node in the collection.Deploy
No recycle needed beyond the roll. Permission folds are built per check, and the handler and validator bind at hub activation, which a roll renews.
Pairs-with: none — no public type or member removed;
DeleteMeshNodeRequeststays (now a forwarder), and it has no poster in core or MeshWeaver.Plugins src (grep).Implementers: none — no interface member added.
Mirror-sync: none — no catalog key added or changed (log text only).
🤖 Generated with Claude Code