fix(themes): reject a theme with undecodable text or a repeated property anywhere

A theme is validated for the strings this version reads, but it is kept,
renamed, edited and re-serialized as a whole document. An unpaired
surrogate escape in a property name or in a value this version ignores,
or a property name that appears twice, was accepted and then threw
InvalidOperationException or ArgumentException from import, rename, copy,
edit and saved-theme repair, none of which handle those.

ReaderTheme now walks the document once and reports either case as
invalid theme JSON, the failure every caller already handles.

Also pin the code-language class length limit and that the source
attribute is allowed only during a source-aware render.

Refs #48

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs

@@ -157,6 +157,7 @@ public sealed class ReaderTheme
       using var document = JsonDocument.Parse(json, new JsonDocumentOptions { MaxDepth = 16 });
       JsonElement root = document.RootElement;
       RequireObject(root);
+      RequireReadableText(root);
       int schemaVersion = 1;
       if(root.TryGetProperty("schemaVersion", out JsonElement version) && (version.ValueKind != JsonValueKind.Number || !version.TryGetInt32(out schemaVersion) || schemaVersion < 1 || !allowNewer && schemaVersion != 1))
       {
@@ -260,15 +261,7 @@ public sealed class ReaderTheme
       {
          throw new JsonException("The theme is missing a required string: " + name);
       }
-      try
-      {
-         return value.GetString()!;
-      }
-      catch(InvalidOperationException exception)
-      {
-         // An unpaired surrogate escape is valid JSON syntax that has no string value.
-         throw new JsonException("The theme contains text that cannot be decoded: " + name, exception);
-      }
+      return value.GetString()!;
    }
 
 
@@ -286,6 +279,53 @@ public sealed class ReaderTheme
 
 
 
+   /// <summary>
+   /// Checks every property name and string in the theme once, including ones this version does
+   /// not read, because a kept theme is later renamed, edited and re-serialized as a whole. An
+   /// unpaired surrogate escape is valid JSON syntax with no string value, and a repeated
+   /// property name has no single value; both are invalid themes.
+   /// </summary>
+   private static void RequireReadableText(JsonElement root)
+   {
+      try
+      {
+         var pending = new Stack<JsonElement>();
+         pending.Push(root);
+         while(pending.TryPop(out JsonElement element))
+         {
+            if(element.ValueKind == JsonValueKind.Object)
+            {
+               var names = new HashSet<string>(StringComparer.Ordinal);
+               foreach(JsonProperty property in element.EnumerateObject())
+               {
+                  if(!names.Add(property.Name))
+                  {
+                     throw new JsonException("The theme repeats a property: " + property.Name);
+                  }
+                  pending.Push(property.Value);
+               }
+            }
+            else if(element.ValueKind == JsonValueKind.Array)
+            {
+               foreach(JsonElement item in element.EnumerateArray())
+               {
+                  pending.Push(item);
+               }
+            }
+            else if(element.ValueKind == JsonValueKind.String)
+            {
+               _ = element.GetString();
+            }
+         }
+      }
+      catch(InvalidOperationException exception)
+      {
+         throw new JsonException("The theme contains text that cannot be decoded.", exception);
+      }
+   }
+
+
+
    /// <summary>Prevents wrong-shaped JSON from reaching property accessors.</summary>
    private static void RequireObject(JsonElement element)
    {

tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs

@@ -145,6 +145,34 @@ public sealed class SanitizerAllowlistTests
 
 
 
+   /// <summary>A code-language class is kept up to 80 characters and dropped one character past that.</summary>
+   [Fact]
+   public void Render_CodeLanguageClassAtAndPastTheLengthLimit_IsBoundedExactly()
+   {
+      string atLimit = "language-" + new string('x', 71);
+      Assert.Equal(80, atLimit.Length);
+      using IHtmlDocument kept = Parse(new MarkdownRenderer().Render($"<span class=\"{atLimit}\">text</span>"));
+      Assert.Equal(atLimit, Assert.Single(kept.Body!.QuerySelectorAll("span")).GetAttribute("class"));
+      using IHtmlDocument dropped = Parse(new MarkdownRenderer().Render($"<span class=\"{atLimit}x\">text</span>"));
+      Assert.Null(Assert.Single(dropped.Body!.QuerySelectorAll("span")).GetAttribute("class"));
+   }
+
+
+
+   /// <summary>The attribute that carries source positions is allowed only during a source-aware render, whether that render finishes or fails.</summary>
+   [Fact]
+   public void RenderWithSources_AfterTheRender_NoLongerAllowsTheSourceAttribute()
+   {
+      var renderer = new MarkdownRenderer();
+      var sources = new DocumentSourceMap("# Title");
+      Assert.Contains("Title", renderer.RenderWithSources("# Title", FrontmatterMode.Hide, sources), StringComparison.Ordinal);
+      Assert.DoesNotContain(sources.AttributeName, renderer.Policy.AllowedAttributes);
+      Assert.Throws<ArgumentOutOfRangeException>(() => renderer.RenderWithSources("# Title", (FrontmatterMode)99, sources));
+      Assert.DoesNotContain(sources.AttributeName, renderer.Policy.AllowedAttributes);
+   }
+
+
+
    /// <summary>Only web and mail links keep a target; every other scheme, including ones Windows would hand to another application, is removed.</summary>
    [Theory]
    [InlineData("http://example.com/a", true)]

tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs

@@ -270,18 +270,26 @@ public sealed class UntrustedInputValidationTests
 
 
    /// <summary>
-   /// Text the JSON reader cannot decode, an unpaired surrogate escape, is an invalid theme like any
-   /// other: parsing, import and saved-theme repair all handle it as one.
+   /// Text the JSON reader cannot decode, an unpaired surrogate escape, makes the theme invalid
+   /// wherever it appears: in a value the theme reads, in a property name, or in a value this
+   /// version ignores but would later re-serialize. So does a repeated property name.
    /// </summary>
    [Theory]
    [InlineData("\"name\": \"Nord\"", "\"name\": \"\\uD83D\"")]
    [InlineData("\"background\": \"#2e3440\"", "\"background\": \"\\uD800\"")]
    [InlineData("\"body\": \"System\"", "\"body\": \"A\\uDC00B\"")]
-   public void Parse_UndecodableText_IsRejectedAsAnInvalidTheme(string valid, string undecodable)
+   [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"\\uD83D\": 1")]
+   [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"note\": \"\\uD83D\"")]
+   [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"notes\": [1, [\"ok\", {\"deep\": \"\\uDFFF\"}]]")]
+   [InlineData("\"body\": \"System\"", "\"body\": \"System\", \"\\uD800x\": \"y\"")]
+   [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"name\": \"Nord\"")]
+   [InlineData("\"body\": \"System\"", "\"body\": \"System\", \"body\": \"Other\"")]
+   [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"extra\": {\"a\": 1, \"a\": 2}")]
+   public void Parse_UndecodableTextOrRepeatedProperty_IsRejectedAsAnInvalidTheme(string valid, string invalid)
    {
       string original = ThemeCatalog.Find("Nord").Json;
       Assert.Contains(valid, original, StringComparison.Ordinal);
-      string json = original.Replace(valid, undecodable, StringComparison.Ordinal);
+      string json = original.Replace(valid, invalid, StringComparison.Ordinal);
       Assert.Throws<JsonException>(() => ReaderTheme.Parse(json));
       Assert.Throws<JsonException>(() => ReaderTheme.ParseCompatible(json));
       var settings = new AppSettings { UserThemes = [json, original] };
@@ -291,6 +299,26 @@ public sealed class UntrustedInputValidationTests
 
 
 
+   /// <summary>
+   /// A theme that parses can be renamed, copied, edited and imported under a new name, whatever
+   /// extra properties it carries: those operations re-serialize the whole document.
+   /// </summary>
+   [Fact]
+   public void Parse_AcceptedThemeWithExtraProperties_SurvivesRenameCopyEditAndImport()
+   {
+      string json = ThemeCatalog.Find("Nord").Json.Replace("\"name\": \"Nord\"", "\"name\": \"Nord\", \"note\": \"caf\\u00e9 \\ud83d\\ude00\", \"future\": {\"list\": [1, \"two\", null]}", StringComparison.Ordinal);
+      var theme = ReaderTheme.Parse(json);
+      Assert.Equal("Renamed", theme.Rename("Renamed").Name);
+      AppSettings imported = ThemeManagement.Import(new AppSettings(), theme);
+      Assert.Contains(ThemeManagement.All(imported), item => item.Name.StartsWith("Nord (", StringComparison.Ordinal));
+      Assert.EndsWith(" Copy", ThemeManagement.CopyTheme(imported, theme).Name, StringComparison.Ordinal);
+      ReaderTheme edited = theme.Edit("Edited", new Dictionary<string, string>(StringComparer.Ordinal), new Dictionary<string, double>(StringComparer.Ordinal), new Dictionary<string, string>(StringComparer.Ordinal), "");
+      Assert.Equal("Edited", edited.Name);
+      Assert.Contains("caf", edited.Json, StringComparison.Ordinal);
+   }
+
+
+
    /// <summary>Only the exact lower-case channel name selects the development identity.</summary>
    [Theory]
    [InlineData("DEV")]

tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs

@@ -50,7 +50,7 @@ public sealed class WindowsFileNamesTests
    [InlineData("CON-.md")]
    [InlineData("null.md")]
    [InlineData(" NUL.png")]
-   [InlineData("NUL .png")]
+   [InlineData("NUL\u00A0.png")]
    [InlineData("NUL\t.png")]
    [InlineData("readme.NUL")]
    [InlineData("COM⁴.png")]