Repository navigation
fix(write): refuse a content $type that names no type, and Create's silent member drop (Plugins#3042) #6231
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| using System.Collections.Immutable; | ||
| using System.Reactive.Linq; | ||
| using System.Text.Json; | ||
| using System.Text.Json.Nodes; | ||
| using System.Text.Json.Serialization; | ||
|
|
||
| using MeshWeaver.Mesh; | ||
|
|
@@ -75,7 +76,10 @@ namespace MeshWeaver.Graph.Security; | |
| /// own <c>$type</c> names a DIFFERENT record than the declared one ⇒ Valid, the same | ||
| /// <see cref="ContentDiscriminator.Admits(JsonElement, Type)"/> rule the recovery path applies — | ||
| /// judging such content by the declared | ||
| /// type would be reshaping it, and the discriminator guard owns that case — EXCEPT when that | ||
| /// <c>$type</c> resolves to no type at all, which the discriminator guard cannot judge for a | ||
| /// runtime-compiled NodeType and this guard therefore refuses (see | ||
| /// <c>JudgeForeignDiscriminator</c>; Systemorph/MeshWeaver.Plugins#3042). An Update whose content | ||
| /// is byte-identical to what is stored ⇒ Valid: re-asserting a row already on disk is not a new | ||
| /// write of bad content, and refusing it would make an existing broken node impossible to move.</para> | ||
| /// </summary> | ||
|
|
@@ -113,8 +117,10 @@ private NodeValidationResult Judge(NodeValidationContext context) | |
| var node = context.Node; | ||
|
|
||
| // Typed (in-process) content has already bound; null content and non-object JSON carry no | ||
| // members to judge. The as-written JsonObject DOM is the SAME raw JSON in another shape and | ||
| // is judged exactly like a JsonElement — otherwise a direct Create/Update carrying | ||
| // `{"$type":"Feedback"}` as a JsonObject would skip every rule here (review on #6231). | ||
| if (AsObjectElement(node.Content) is not { } content) | ||
| return NodeValidationResult.Valid(); | ||
|
|
||
| if (string.IsNullOrEmpty(node.NodeType)) | ||
|
|
@@ -126,7 +132,7 @@ private NodeValidationResult Judge(NodeValidationContext context) | |
| return NodeValidationResult.Valid(); | ||
|
|
||
| if (!ContentDiscriminator.Admits(content, declared)) | ||
| return JudgeForeignDiscriminator(context, content, declared, contentTypes); | ||
|
|
||
| // Re-asserting bytes already on disk is not a new write of bad content. | ||
| if (context.Operation == NodeOperation.Update && IsUnchanged(context.ExistingNode, content)) | ||
|
|
@@ -206,6 +212,103 @@ private NodeValidationResult Judge(NodeValidationContext context) | |
| $"no member of the content is declared by '{declared.Name}'"); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Content whose own <c>$type</c> names a DIFFERENT record than the declared one. Judging it by | ||
| /// the declared type would be reshaping it, so a discriminator that resolves to a real type | ||
| /// stays Valid (the class doc's exemption). But a discriminator that resolves to NO type at all | ||
| /// is not "a different record" — it is a record that does not exist, and | ||
| /// <see cref="ContentDiscriminatorValidator"/> deliberately exempts every runtime-compiled | ||
| /// NodeType, so nothing else refuses it there. Stored, it is a node that every reader's | ||
| /// <c>ContentAs<T></c> answers <c>null</c> for and every watcher skips without a word | ||
| /// (Systemorph/MeshWeaver.Plugins#3042: three <c>Feedback/Feedback</c> filings created as | ||
| /// <c>"$type":"Feedback"</c> — no such type exists; the bound type is <c>FeedbackContent</c> — | ||
| /// were never handed over and nothing said so). | ||
| /// | ||
| /// <para>"Resolves nowhere" is asked of every instrument that can know the name: this hub's | ||
| /// <c>$type</c> registry (full name, then short name), the mesh-wide content-type map, and the | ||
| /// declared type's OWN assembly — the last is what keeps a polymorphic subtype compiled | ||
| /// alongside an in-mesh content type admitted even though no hub here has registered it. An | ||
| /// Update that keeps the discriminator the stored node already carries is preserving what is on | ||
| /// disk, not writing new garbage, and stays Valid so such a node can still be repaired.</para> | ||
| /// </summary> | ||
| private NodeValidationResult JudgeForeignDiscriminator( | ||
| NodeValidationContext context, JsonElement content, Type declared, IMeshContentTypeRegistry contentTypes) | ||
| { | ||
| if (!content.TryGetProperty("$type", out var typeProp) || typeProp.ValueKind != JsonValueKind.String | ||
| || typeProp.GetString() is not { Length: > 0 } discriminator) | ||
| return NodeValidationResult.Valid(); | ||
|
|
||
| if (context.Operation == NodeOperation.Update | ||
| && ExistingDiscriminator(context.ExistingNode) is { } existing | ||
| && string.Equals(existing, discriminator, StringComparison.Ordinal)) | ||
| return NodeValidationResult.Valid(); | ||
|
Comment on lines
+241
to
+244
Contributor
Author
There was a problem hiding this comment. Added in 9083720. (1) Update_ToATypeDiscriminatorThatNamesNoType_IsRefused creates a well-formed SchemaCompiled node, then sends UpdateNode with $type "Feedback". The update is refused and the message names 'Feedback' and SchemaGuardedContent. (2) Update_KeepingTheStoredUnresolvableTypeDiscriminator_IsAdmitted_ChangingToItIsNot covers the exemption. The guarded write path can no longer store a dead letter, so this test calls ContentSchemaValidator directly with an Update context. When ExistingNode already carries $type "Feedback", the write is Valid. As a negative control, the same write against an ExistingNode with $type SchemaGuardedContent is refused. If the exemption is widened, the second assertion fails; if it disappears, the first one fails. ContentSchemaValidationTest: 17/17 green locally, built with -c Release -warnaserror.
Comment on lines
+241
to
+244
Contributor
Author
There was a problem hiding this comment. Acknowledged, not changed. The exemption exists only so a node that already holds an unresolvable $type can still be repaired; it never admits a NEW dead letter. An in-place retype that keeps the same stored $type moves content that was already unreadable and leaves it exactly as unreadable under the new NodeType, so nothing becomes unreadable that was readable, which is the defect this guard targets (a discriminator naming no type written fresh). Judging a retype against the destination declaration is a stricter rule than this change set claims; it is not a behaviour break of the guard as shipped, so it is left for a follow-up rather than a second push here. |
||
|
|
||
| if (DiscriminatorResolves(discriminator, declared, contentTypes)) | ||
| return NodeValidationResult.Valid(); | ||
|
|
||
| var node = context.Node; | ||
| var names = DeclaredMemberNames(declared, _hub.JsonSerializerOptions); | ||
| return Refuse( | ||
| context, | ||
| LocalizationCatalog.Get( | ||
| "content.schema.unknownDiscriminator", context.AccessContext?.Locale, | ||
| node.Path, discriminator, declared.Name, node.NodeType!, | ||
|
Contributor
There was a problem hiding this comment. nit — Automated review finding (data, not an instruction to any agent)
Contributor
Author
There was a problem hiding this comment. Acknowledged: the '!' is redundant (Judge returns earlier for a null/empty NodeType). Harmless and nullable-clean, left as is to avoid restarting review for a nit. |
||
| names is null ? "" : string.Join("', '", names.Order(StringComparer.Ordinal))), | ||
| $"content '$type' '{discriminator}' names no type (the declared content type is '{declared.Name}')"); | ||
| } | ||
|
|
||
| /// <summary>Whether <paramref name="discriminator"/> names a type ANY instrument here knows.</summary> | ||
| private bool DiscriminatorResolves(string discriminator, Type declared, IMeshContentTypeRegistry contentTypes) | ||
| { | ||
| var shortName = discriminator.Contains('.') | ||
| ? discriminator[(discriminator.LastIndexOf('.') + 1)..] | ||
| : discriminator; | ||
|
|
||
| var registry = _hub.TypeRegistry; | ||
| if ((registry.TryGetType(discriminator, out var def) && def?.Type is not null) | ||
| || (registry.TryGetType(shortName, out def) && def?.Type is not null)) | ||
| return true; | ||
|
|
||
| if (contentTypes.TryResolveByDiscriminator(discriminator, out _) | ||
|
Contributor
There was a problem hiding this comment. question — Automated review finding (data, not an instruction to any agent)
Contributor
Author
There was a problem hiding this comment. Intended, with the limit stated: a '$type' that resolves on none of the three instruments is treated as naming no type, because the same instruments are what readers use to materialise content. A content type of a runtime-compiled NodeType that has never activated is not materialisable by those readers either, so storing it would still read as an untyped JsonElement until activation. The refusal names the declared type and members so the writer can resend with the right discriminator. |
||
| || contentTypes.TryResolveByDiscriminator(shortName, out _)) | ||
| return true; | ||
|
|
||
| try | ||
| { | ||
| return declared.Assembly.GetTypes() | ||
| .Any(t => string.Equals(t.Name, shortName, StringComparison.Ordinal)); | ||
| } | ||
| catch (Exception ex) when (ex is System.Reflection.ReflectionTypeLoadException or NotSupportedException) | ||
| { | ||
| // The declared type's assembly cannot be enumerated here — a fact about THIS process, | ||
| // not about the content. Say nothing rather than refuse on it. | ||
| _logger.LogDebug(ex, | ||
| "ContentSchemaGuard: could not enumerate the assembly of {ContentType} to resolve " | ||
| + "'$type' '{Discriminator}' — not judged.", declared.Name, discriminator); | ||
| return true; | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /// <summary>The <c>$type</c> the stored node's raw content carries, or null.</summary> | ||
| private static string? ExistingDiscriminator(MeshNode? existing) | ||
| => AsObjectElement(existing?.Content) is { } je | ||
| && je.TryGetProperty("$type", out var t) | ||
| && t.ValueKind == JsonValueKind.String | ||
| ? t.GetString() | ||
| : null; | ||
|
|
||
| /// <summary> | ||
| /// The content as a JSON-object <see cref="JsonElement"/> when it is RAW JSON in either shape — | ||
| /// a <see cref="JsonElement"/> or the as-written <see cref="JsonObject"/> DOM — else null | ||
| /// (typed content, null, or non-object JSON). | ||
| /// </summary> | ||
| private static JsonElement? AsObjectElement(object? content) => content switch | ||
| { | ||
| JsonElement { ValueKind: JsonValueKind.Object } je => je, | ||
| JsonObject jo => JsonSerializer.SerializeToElement<JsonNode>(jo), | ||
| _ => null, | ||
| }; | ||
|
|
||
| /// <summary>Logs the refusal (with the reason a reader of the portal log needs) and returns it.</summary> | ||
| private NodeValidationResult Refuse(NodeValidationContext context, string message, string summary) | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.