Merge pull request #108 from jongalloway/copilot/sub-pr-106

fix: address icon pack review feedback — sanitizer correctness, registry atomicity, determinism, color consistency, XSS fix

Author: jongalloway

src/DiagramForge/DiagramRenderer.cs

@@ -210,7 +210,18 @@ private void ResolveIcons(Diagram diagram)
         foreach (var node in diagram.Nodes.Values)
         {
             if (node.IconRef is not null)
-                node.ResolvedIcon = IconRegistry.Resolve(node.IconRef);
+            {
+                var icon = IconRegistry.Resolve(node.IconRef);
+                if (icon is not null)
+                {
+                    // Sanitize the SVG content before placing it into the output SVG
+                    // to prevent XSS / injection from untrusted diagram text or icon providers.
+                    string? sanitized = SvgIconSanitizer.Sanitize(icon.SvgContent);
+                    node.ResolvedIcon = sanitized is not null
+                        ? icon with { SvgContent = sanitized }
+                        : null;
+                }
+            }
         }
     }
 

src/DiagramForge/Icons/SvgIconSanitizer.cs

@@ -30,12 +30,13 @@ internal static class SvgIconSanitizer
     /// Sanitizes an SVG fragment, removing unsafe elements and attributes.
     /// Returns <see langword="null"/> if the content is entirely unsafe or cannot be parsed.
     /// </summary>
-    public static string? Sanitize(string svgContent)
+    public static string? Sanitize(string? svgContent)
     {
         if (string.IsNullOrWhiteSpace(svgContent))
             return null;
 
         XElement root;
+        bool addedWrapper = false;
         try
         {
             // Wrap bare fragments in <svg> if needed for parsing.
@@ -50,14 +51,15 @@ internal static class SvgIconSanitizer
                 XmlResolver = null,
             };
 
-            // Try parsing as-is; if that fails, wrap in an svg element.
+            // Try parsing as-is; if that fails, wrap in a <g> for multi-element fragments.
             try
             {
                 using var reader = XmlReader.Create(new System.IO.StringReader(svgContent), settings);
                 root = XElement.Load(reader, LoadOptions.PreserveWhitespace);
             }
             catch (XmlException)
             {
+                addedWrapper = true;
                 using var reader = XmlReader.Create(new System.IO.StringReader($"<g>{svgContent}</g>"), settings);
                 root = XElement.Load(reader, LoadOptions.PreserveWhitespace);
             }
@@ -81,11 +83,15 @@ internal static class SvgIconSanitizer
             ConformanceLevel = ConformanceLevel.Fragment,
         }))
         {
-            // Write the inner content only (skip the wrapper if we added one).
-            if (root.Name.LocalName.Equals("svg", StringComparison.OrdinalIgnoreCase)
-                || root.Name.LocalName.Equals("g", StringComparison.OrdinalIgnoreCase))
+            // Strip the outer <svg> wrapper to prevent nested <svg> in the rendered output
+            // (DiagramIcon.SvgContent contract: no outer <svg>).
+            // Also strip the internal parser-added <g> wrapper — it was not part of the
+            // original content. Original <g> elements from the input are preserved as-is
+            // so that attributes like fill="none" stroke="currentColor" are not lost.
+            if (root.Name.LocalName.Equals("svg", StringComparison.OrdinalIgnoreCase) || addedWrapper)
             {
-                root.WriteTo(writer);
+                foreach (var child in root.Nodes())
+                    child.WriteTo(writer);
             }
             else
             {

src/DiagramForge/Layout/DefaultLayoutEngine.Architecture.cs

@@ -28,7 +28,7 @@ private static void LayoutArchitectureDiagram(
                 // Add space for the icon above the label.
                 if (node.ResolvedIcon is not null)
                 {
-                    double iconArea = SvgNodeWriter.DefaultIconSize + 6; // icon + gap
+                    double iconArea = SvgNodeWriter.DefaultIconSize + SvgNodeWriter.IconLabelGap;
                     node.Height += iconArea;
                     node.Width = Math.Max(node.Width, SvgNodeWriter.DefaultIconSize + 2 * theme.NodePadding);
                 }

src/DiagramForge/Models/IconRegistry.cs

@@ -27,14 +27,33 @@ public void RegisterPack(string packName, IIconProvider provider)
 
     /// <summary>
     /// Registers an icon provider under both <paramref name="packName"/> and <paramref name="alias"/>.
+    /// Both names are validated before either is registered to ensure an atomic operation.
     /// </summary>
     public void RegisterPack(string packName, string alias, IIconProvider provider)
     {
-        RegisterPack(packName, provider);
-
+        ArgumentException.ThrowIfNullOrWhiteSpace(packName);
         ArgumentException.ThrowIfNullOrWhiteSpace(alias);
+        ArgumentNullException.ThrowIfNull(provider);
+
+        // Validate both names up-front before touching the registry to keep the
+        // operation atomic — partial registration is not observable to callers.
+        if (_packs.ContainsKey(packName))
+            throw new ArgumentException($"An icon pack named '{packName}' is already registered.", nameof(packName));
+        if (_packs.ContainsKey(alias))
+            throw new ArgumentException($"An icon pack named '{alias}' is already registered.", nameof(alias));
+        if (string.Equals(packName, alias, StringComparison.OrdinalIgnoreCase))
+            throw new ArgumentException($"Pack name and alias must be different.", nameof(alias));
+
+        // TryAdd may still fail under concurrent load, but the existing single-name
+        // overload already throws in that case, so the behaviour is consistent.
+        if (!_packs.TryAdd(packName, provider))
+            throw new ArgumentException($"An icon pack named '{packName}' is already registered.", nameof(packName));
+
         if (!_packs.TryAdd(alias, provider))
+        {
+            _packs.TryRemove(packName, out _);
             throw new ArgumentException($"An icon pack named '{alias}' is already registered.", nameof(alias));
+        }
     }
 
     /// <summary>
@@ -62,10 +81,10 @@ public void RegisterPack(string packName, string alias, IIconProvider provider)
             return null;
         }
 
-        // Bare name — search all packs.
-        foreach (var provider in _packs.Values)
+        // Bare name — search all packs in sorted name order for deterministic results.
+        foreach (var packName in _packs.Keys.OrderBy(k => k, StringComparer.OrdinalIgnoreCase))
         {
-            var icon = provider.GetIcon(reference);
+            var icon = _packs[packName].GetIcon(reference);
             if (icon is not null)
                 return icon;
         }

src/DiagramForge/Rendering/SvgNodeWriter.cs

@@ -8,7 +8,7 @@ internal static class SvgNodeWriter
     private const double DefaultLabelLineHeight = 1.15;
     private const double AnnotationFontSizeRatio = 0.85;
     internal const double DefaultIconSize = 48;
-    private const double IconLabelGap = 6;
+    internal const double IconLabelGap = 6;
 
     internal static void AppendNode(StringBuilder sb, Node node, Theme theme, int nodeIndex = 0)
     {
@@ -151,7 +151,7 @@ internal static void AppendNode(StringBuilder sb, Node node, Theme theme, int no
         double iconAreaHeight = 0;
         if (hasIcon && !textOnly)
         {
-            AppendNodeIcon(sb, node, theme);
+            AppendNodeIcon(sb, node, theme, resolvedTextColor);
             iconAreaHeight = DefaultIconSize + IconLabelGap;
         }
 
@@ -204,7 +204,11 @@ private static void AppendNodeLabel(
     /// Renders a resolved icon inside a node, centered horizontally and positioned
     /// in the upper portion to leave room for the label below.
     /// </summary>
-    private static void AppendNodeIcon(StringBuilder sb, Node node, Theme theme)
+    /// <param name="resolvedTextColor">
+    /// The already-resolved text color for the node (same value used for the label),
+    /// ensuring icon and label use a consistent, contrast-aware color.
+    /// </param>
+    private static void AppendNodeIcon(StringBuilder sb, Node node, Theme theme, string resolvedTextColor)
     {
         var icon = node.ResolvedIcon;
         if (icon is null)
@@ -218,13 +222,10 @@ private static void AppendNodeIcon(StringBuilder sb, Node node, Theme theme)
         string[] viewBoxParts = icon.ViewBox.Split(' ', StringSplitOptions.RemoveEmptyEntries);
         string viewBox = viewBoxParts.Length == 4 ? icon.ViewBox : "0 0 24 24";
 
-        // Use theme-aware icon color (the icon uses currentColor for stroke/fill).
-        string iconColor = SvgRenderSupport.Escape(
-            SvgRenderSupport.ResolveNodeTextColor(
-                node.FillColor ?? theme.NodeFillColor, theme));
-
+        // Use the same resolved text color as the label so icon and label are always consistent,
+        // including when the node fill is sourced from a theme palette.
         sb.AppendLine($"""    <g transform="translate({SvgRenderSupport.F(iconX)},{SvgRenderSupport.F(iconY)})">""");
-        sb.AppendLine($"""      <svg width="{SvgRenderSupport.F(iconSize)}" height="{SvgRenderSupport.F(iconSize)}" viewBox="{SvgRenderSupport.Escape(viewBox)}" overflow="visible" color="{iconColor}">""");
+        sb.AppendLine($"""      <svg width="{SvgRenderSupport.F(iconSize)}" height="{SvgRenderSupport.F(iconSize)}" viewBox="{SvgRenderSupport.Escape(viewBox)}" overflow="visible" color="{resolvedTextColor}">""");
         sb.AppendLine($"        {icon.SvgContent}");
         sb.AppendLine("      </svg>");
         sb.AppendLine("    </g>");

tests/DiagramForge.Tests/DiagramRendererIconTests.cs

@@ -18,8 +18,8 @@ public void Render_ArchitectureDiagram_ResolvesBuiltInIcon(string icon)
         var renderer = new DiagramRenderer();
         string svg = renderer.Render($"architecture-beta\n  service svc({icon})[Label]");
 
-        // The SVG should contain the icon content rendered inside the node.
-        Assert.Contains("viewBox", svg);
+        // The SVG should contain an icon <svg> element with the 48px icon size.
+        Assert.Contains("width=\"48.00\"", svg);
     }
 
     [Fact]
@@ -49,11 +49,12 @@ public void RegisterIconPack_IconResolvesInRender()
         var renderer = new DiagramRenderer();
         renderer.RegisterIconPack("custom", new StubIconProvider("myicon"));
 
-        // Architecture diagram referencing custom:myicon — the parser treats
-        // the icon keyword as a string, so the reference stays as-is.
-        // For now, built-in architecture icons use bare names.
-        // Custom pack icons would be used via future parser support for pack:name syntax.
-        Assert.Contains("custom", renderer.IconRegistry.RegisteredPacks);
+        // Architecture diagram referencing "custom:myicon" — parser captures the
+        // pack:name token verbatim, so the registry resolves it during rendering.
+        string svg = renderer.Render("architecture-beta\n  service svc(custom:myicon)[Label]");
+
+        // The stub icon's path content must appear in the rendered SVG.
+        Assert.Contains("d=\"M0 0\"", svg);
     }
 
     [Fact]

tests/DiagramForge.Tests/Icons/SvgIconSanitizerTests.cs

@@ -96,7 +96,7 @@ public void Sanitize_JavascriptUri_IsRemoved()
     [Fact]
     public void Sanitize_Null_ReturnsNull()
     {
-        Assert.Null(SvgIconSanitizer.Sanitize(null!));
+        Assert.Null(SvgIconSanitizer.Sanitize(null));
     }
 
     [Fact]