Repository navigation
fix(security): SaveMeshNodeRequest is a checked write; participant ingress refuses [InfrastructureOnly] messages; workspace creates run create validators - #5985
Merged
Conversation
…own post; participant ingress deliveries of [InfrastructureOnly] messages are refused; workspace MeshNode creates run the create validators - SaveMeshNodeRequest: any sender but the receiving hub is forwarded to the checked CreateOrUpdateNodeRequest under its own identity; no identity is refused. - [InfrastructureOnly] + ParticipantIngress: an ingress stamps every participant delivery (FromParticipant); the receiving hub refuses a stamped infrastructure message as Forbidden once it is typed. Marked: SaveMeshNodeRequest, the six partition-storage requests, DispatchCompileTrigger, TrackActivityRequest, InitializeHubRequest, ShutdownRequest, SetCurrentRequest, UpdateStreamRequest. - CreateReleaseRequest requires Update, RunTestsRequest Execute. - MeshNodeCreationDataValidator: a MeshNode entering a workspace through a DataChangeRequest (Creations, or Updates for a path storage does not hold) runs the create-validator chain. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Test Results (shard 2)1 693 tests +583 1 693 ✅ +583 4m 15s ⏱️ +21s Results for commit 8bb30cb. ± Comparison against base commit 86c840b. This pull request removes 80 and adds 661 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 1) 2 files - 1 2 suites - 1 4m 49s ⏱️ -45s Results for commit 8bb30cb. ± Comparison against base commit 86c840b. This pull request removes 661 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 4)635 tests - 132 635 ✅ + 61 7m 8s ⏱️ + 5m 7s Results for commit 8bb30cb. ± Comparison against base commit 86c840b. This pull request removes 563 and adds 431 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 5)1 530 tests 1 530 ✅ 9m 39s ⏱️ Results for commit 8bb30cb. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 3) 3 files - 1 3 suites - 1 15m 57s ⏱️ + 2m 29s Results for commit 8bb30cb. ± Comparison against base commit 86c840b. This pull request removes 431 and adds 1522 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results 16 files - 4 16 suites - 4 44m 54s ⏱️ + 4m 13s Results for commit 8bb30cb. ± Comparison against base commit 86c840b. This pull request removes 317 and adds 39 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…ssuming the second read is a barrier The owner answers a MeshNodeReference read from the read-validator pipeline's Subscribe, off its turn, so two reads' answers are not ordered; CI run 36926964455 shard 3 read Dropped=0. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![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)
Reviewed head 8bb30cb from the item's complete diff (no cut patches). The change: (1) the SaveMeshNodeRequest handler writes raw only for the receiving hub's own post — sender equal to hub.Address with the host part ignored and no ingress stamp — and forwards every other sender to a checked CreateOrUpdateNodeRequest under the delivery's own access context, refusing an identity-less delivery with a log-only Processed; (2) a new [InfrastructureOnly] attribute plus the ParticipantIngress contract (FromParticipant, IsFromParticipant, Refuses) enforced at the receiving hub in MessageService.UnpackIfNecessary, failing a stamped delivery of an infrastructure type with ErrorType.Forbidden before any handler runs, with the recorded classification now carried into the NACK; (3) MeshNodeCreationDataValidator runs the create-validator chain for a MeshNode entering a workspace through a DataChangeRequest — Creations, and Updates whose path storage does not hold, read authoritatively and fail-safe to create when no storage adapter is registered; (4) [RequiresPermission] on CreateReleaseRequest (Update) and RunTestsRequest (Execute); (5) the participant-ingress architecture doc. Three new executing test files pin each layer with paired negative controls, and OneReadOneAnswerTest is de-flaked by waiting for the drop. Checked: the self-post predicate and forward logic, the refusal's placement after deserialization and before handlers, attribute coverage against the PR's own inventory, validator precedence and fail-safe branches, permission attributes, and the MeshWeaver hard rules — no async outside IIoPool in the production diff, immutable collections, tests that execute (the `!` occurrences are test-only and filed as a nit). Could not verify from the diff: base members referenced but not defined here (Failed with an ErrorType overload, GetFailureErrorType, ErrorType, Package/RawJson, NodeOperationIssuingHub) — they must pre-exist, and the item shows no failed CI jobs; the suite results quoted in the PR body, including the one known Messaging.Hub test failure in code this PR does not touch; and whether any production ingress stamps deliveries — none does at this head, which is the one material gap: until the paired MeshWeaver.Plugins change lands, only the SaveMeshNodeRequest handler half is live. Verdict: 1 should-fix, 2 questions, 2 nits; no blocking finding, so the event is COMMENT.
Findings: 0 blocking · 1 should-fix · 2 question · 2 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
should-fix src/MeshWeaver.Messaging.Contract/ParticipantIngress.cs
At this head no production code stamps a delivery: `FromParticipant`'s only callers in this diff are the tests, and the PR body places the real callers — `SignalRConnectionRegistry.Deliver` and `GrpcConnectionRegistry.Deliver` — in MeshWeaver.Plugins, as a follow-up that consumes this contract. Until that lands, `ParticipantIngress.Property` never appears on a production delivery, so the refusal added in `MessageService.UnpackIfNecessary` never fires and the `!IsFromParticipant()` leg of `IsOwnPersistencePost` never saves anything: every type this PR marks `[InfrastructureOnly]` — `WriteBatchRequest`/`DeleteBatchRequest` (raw partition writes), `ReadNodeRequest`/`ExistsRequest`/`ListChildPathsRequest`/`ListDescendantPathsRequest` (reads past RLS), `DispatchCompileTrigger`, `TrackActivityRequest`, `InitializeHubRequest`, `ShutdownRequest`, `SetCurrentRequest`, `UpdateStreamRequest` — stays reachable by any client exactly as before, and even `SaveMeshNodeRequest` stays raw-writable by a delivery that forges the target hub's address as `Sender` (`IsOwnPersistencePost` compares the sender to `hub.Address` with the host part ignored, and an unstamped forge matches; the posing-as-hub test passes only because the test stamps the delivery itself). The `SaveMeshNodeRequest` handler half is genuinely closed, the sequencing is forced, and the body is open about it — but this PR alone does not close the exposure its title names, so the paired Plugins change needs the same release window, and the new doc page presents the stamping as in place without noting that dependency.
question src/MeshWeaver.Graph/MeshDataSource.cs
`ForwardSaveToCheckedWrite` refuses a non-self save that carries no identity — log-only, `request.Processed()`, nothing written, no error to the sender. The rewritten `SaveMeshNodeRequest` doc names the senders as the hub's own persistence posts plus in-mesh force-persist posters under a caller's identity, but the doc it replaces also named `MeshNodeTypeSource` as a poster, and `MeshNodeTypeSource` is not in this diff. If any surviving poster sends the save from a hub other than the target with no AccessContext on the delivery, its nodes silently stop persisting after this change. The new tests exercise the workspace-create persistence path only under TestUsers.Admin (`AWorkspaceMeshNodeCreateIsValidatedTest` posts `ChangedBy = Admin`), so an identity-less cross-hub poster would not be caught; the sender audit is not verifiable from the diff.
nit test/MeshWeaver.Hosting.Test/ParticipantIngressRefusesInfrastructureTest.cs
`ShutdownRequest` is marked `[InfrastructureOnly]` (src/MeshWeaver.Messaging.Hub/ShutdownRequest.cs) but is the one marked type missing from the `InfrastructureTypes` inventory, while the new ParticipantIngress.md rule says a type marked `[InfrastructureOnly]` is also pinned here — as written, a later removal of the attribute from `ShutdownRequest` fails no test. If the record's internal visibility is what keeps it out of the inventory, InternalsVisibleTo or making it public (as `InitializeHubRequest` is) restores the pin.
nit test/MeshWeaver.Hosting.Test/SaveMeshNodeRequestIsACheckedWriteTest.cs
The MeshWeaver rules ban `!` used to silence a warning, and the new tests have several, all on reflection, deserialization or nullable results: `JsonSerializer.Deserialize<IMessageDelivery>(json, options)!` here, `(IMessageDelivery)Activator.CreateInstance(...)!` in ParticipantIngressRefusesInfrastructureTest, `failure!` and `stored!` on nullable locals here, and `held.Path!`/`free.Path!` in AWorkspaceMeshNodeCreateIsValidatedTest. The production diff has none. If the rule is scoped to production code these are fine; otherwise they are re-patternable (null-check plus ThrowIfNull).
Internal review of 8bb30cb738f3e923dea8b304b383b0fbd4d097b7 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| /// Request to track a user's activity (navigation, login, etc). | ||
| /// Handled by the hub to persist <see cref="UserActivityRecord"/> nodes. | ||
| /// </summary> | ||
| [InfrastructureOnly] |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
This type is now blanket-refused from participants, but its own doc comment says it tracks `a user's activity (navigation, login, etc.)` — a record a client is a natural poster of. Which senders legitimately post `TrackActivityRequest` cannot be determined from the diff: if any SPA or React Native client posts it directly for its own navigation, that telemetry is silently dropped the moment the Plugins ingresses start stamping, and this diff's own rule (ParticipantIngress.md, `Adding a message type`) says a type a client legitimately sends gets `[RequiresPermission]` instead — never `[InfrastructureOnly]`. If only in-mesh middleware posts it on the client's behalf, the blanket refusal is the right call.
Contributor
Author
There was a problem hiding this comment.
Checked every poster across the organisation (core, MeshWeaver.Plugins, Memex; REST code search plus git grep on both mains) — only platform code running in-process posts it, no client does:
UserContextMiddleware.TrackLogin(core,MeshWeaver.Hosting.AspNetCore) — the ASP.NET middleware, on sign-in.NavigationService(MeshWeaver.Plugins,MeshWeaver.Hosting.Blazor, line ~1078) — the Blazor circuit's server-side navigation service, posting on the portal's own hub.- tests (
ActivityTrackingUpsertTest, the PluginsQuery.Test/Auth.Testsuites), which post on the mesh hub.
Neither production poster crosses a participant ingress (SignalRConnectionRegistry.Deliver / GrpcConnectionRegistry.Deliver): both run inside the host, so the FromParticipant stamp never applies to them. No TypeScript, React Native or other out-of-process client names the type anywhere in the organisation's repositories, so nothing a client sends today is dropped.
And the refusal is the right polarity for this type rather than [RequiresPermission]: the request carries the UserId whose activity is recorded in its own body, so a participant allowed to post it could write activity (recents, last-login) for any user. If a remote client ever needs to report its own navigation, that is a new, checked message whose subject is the stamped identity — per the rule in Doc/Architecture/ParticipantIngress → Adding a message type. No change to the diff.
This was referenced Oct 2, 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 the triage item
rbuergi/Feedback/save-meshnode-request-raw-storage-write(memex.systemorph.com). Follows #5976.Root cause
SaveMeshNodeRequestis the per-node hub's own persistence step. Its handler wrote whatever node any sender named straight toIStorageAdapter.Write, with no permission check and no validator. Every ingress forwards any delivery to any address:SignalRConnectionRegistry.Deliver, andGrpcConnectionRegistry.Deliveron both the public and the trusted port. The client writes the whole envelope, includingSender, and only the identity is re-stamped. So any connection could overwrite any node, an anonymous one included.The same forwarding exposes the rest of the mesh's plumbing. Each of these handlers trusts the message (read from source, not exercised live):
WriteBatch/DeleteBatch, andReadNode/Exists/ListChildPaths/ListDescendantPaths, which bypass RLS;DispatchCompileTrigger: compiles a NodeType from the node snapshot in the payload;TrackActivityRequest: records activity for any user id;InitializeHubRequest,ShutdownRequest,SetCurrentRequest,UpdateStreamRequest: hub and stream internals.Fix (core half)
1. Handler.
SaveMeshNodeRequestwrites raw only for the hub's own post. That means the sender is the receiving hub (the sampler and the deferred re-post) and the delivery carries no ingress stamp. Any other sender is forwarded as aCreateOrUpdateNodeRequestunder the delivery's ownAccessContext. That applies Create or Update permission and every node validator. A delivery with no identity is refused. MeshWeaver.Plugins' force-persist posters (ModelProviderService.RotateKey,ProviderCredentialSeed,ModelProviderLayoutAreas, the ProvidersApp in-mesh source) keep working as checked writes under their caller.2. Ingress contract.
[InfrastructureOnly]attribute, placed on every type listed above.ParticipantIngresshelper.delivery.FromParticipant(user, "signalr")stamps the validated identity plus an ingress property, after the client's envelope is read.MessageService.UnpackIfNecessaryrefuses a stamped delivery of an[InfrastructureOnly]type withErrorType.Forbidden, before any handler runs. This happens on the target with the message typed, because a type the ingress hub does not know crosses the mesh as raw JSON. The participant receives thatDeliveryFailure.Unknownbefore.3. Permissions.
CreateReleaseRequestnow requires[RequiresPermission(Update)]andRunTestsRequestrequiresExecute. Both used to start a compile or test run for any sender.4. Workspace creates. A
MeshNodeentering a workspace through aDataChangeRequestwas written raw byMeshNodeTypeSourceand checked only by RLS.MeshNodeCreationDataValidatornow runs the create-validator chain for:Creations;Updateswhose path storage does not hold. The caller picks the list, and the type source writes both lists the same way.This is the counterpart of #5976's
MeshNodeDeletionDataValidator.Evidence
All with
-c Release:SaveMeshNodeRequestIsACheckedWriteTest: Viewer and AnonymousSaveMeshNodeRequestIsACheckedWriteTest: participant save (typed and raw JSON)DeliveryFailurewithForbidden; the same delivery unstamped writesSaveMeshNodeRequestIsACheckedWriteTest: participant posing as the hub (Sender= target)SaveMeshNodeRequestIsACheckedWriteTest: canonical update still persistsParticipantIngressRefusesInfrastructureTestAWorkspaceMeshNodeCreateIsValidatedTest(Creations / Updates)CommittedFailed; the unrefused sibling is writtenThe hijack payload bumps
Version. Without that, the raw save's duplicate-write suppression would hide the hole.Full suites:
MeshWeaver.Hosting.Test: 1087 passed, 0 failed, 1 skipped.MeshWeaver.Graph.Test: 2400/2400.MeshWeaver.Documentation.Test: 657/657.MeshWeaver.Messaging.Hub.Test: 549/550. The one failure isContentTypeRegistrationFanOutTest.TheWaitedType_IsStillDelivered_OffTheRegisteringThread, a thread-id equality race in code this PR does not touch. Filed asrbuergi/Feedback/content-type-fanout-thread-test-races-pool-reuse-20261001T2115Zand not re-run.-warnaserroris clean on every touched project, built through those test projects.Not closed here (filed)
rbuergi/Feedback/patch-path-skips-app-integrity-update-validators-20261001T2130Z. An UPDATE of an existing node through a rawPatchDataRequest/DataChangeRequestskips the non-owner-enforcedINodeValidators, which run only client-side inNodeUpdatePipeline.Doc:
Doc/Architecture/ParticipantIngress(new, listed in the Architecture topic map).Deploy
Nothing to recycle beyond the roll. Handlers and validators bind at hub activation, which a roll renews. The ingress half lands in MeshWeaver.Plugins (
FromParticipantin both registries) once this is in the sealed set. Until then the handler half already closes the raw write for every sender except a client that forges the target's own address asSender.Pairs-with: none — nothing public removed; the Plugins ingress PR consumes the new
ParticipantIngress.FromParticipantand follows this.Implementers: none — no interface member added.
Mirror-sync: none — no catalog key added or changed.
🤖 Generated with Claude Code