Apply review suggestions

3 files changed, 144 insertions, 103 deletions

diff --git a/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs b/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs
index 64329feabd..3e353db8de 100644
--- a/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs
+++ b/src/Jellyfin.Drawing.Skia/SkiaEncoder.cs
@@ -191,9 +191,9 @@ public class SkiaEncoder : IImageEncoder
         var extension = Path.GetExtension(path.AsSpan());
         if (extension.Equals(".svg", StringComparison.OrdinalIgnoreCase))
         {
-            if (!SvgSecurityValidator.IsSafe(path, _logger))
+            if (!SvgSecurityValidator.IsSafe(path, out var reason))
             {
-                _logger.LogError("Refusing to determine dimensions for SVG with external references {FilePath}", path);
+                _logger.LogError("Refusing to determine dimensions for SVG {FilePath}: {Reason}", path, reason);
                 return default;
             }
 
@@ -459,9 +459,9 @@ public class SkiaEncoder : IImageEncoder
             throw new FileNotFoundException("File not found", path);
         }
 
-        if (!SvgSecurityValidator.IsSafe(path, _logger))
+        if (!SvgSecurityValidator.IsSafe(path, out var reason))
         {
-            _logger.LogError("Refusing to render SVG with external references {FilePath}", path);
+            _logger.LogError("Refusing to render SVG {FilePath}: {Reason}", path, reason);
             return null;
         }
 
diff --git a/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs b/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs
index 354c761eeb..f8a1d7d443 100644
--- a/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs
+++ b/src/Jellyfin.Drawing.Skia/SvgSecurityValidator.cs
@@ -1,10 +1,11 @@
 using System;
+using System.Buffers;
+using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.IO.Compression;
 using System.Runtime.CompilerServices;
 using System.Text;
 using System.Xml;
-using Microsoft.Extensions.Logging;
 
 [assembly: InternalsVisibleTo("Jellyfin.Drawing.Skia.Tests")]
 
@@ -21,6 +22,8 @@ internal static class SvgSecurityValidator
     // Upper bound for a decompressed svgz payload carried inside a data URI, to guard against decompression bombs.
     private const int MaxDecompressedBytes = 16 * 1024 * 1024;
 
+    private const int DecompressBufferSize = 81920;
+
     private static readonly XmlReaderSettings _scanSettings = new()
     {
         DtdProcessing = DtdProcessing.Parse,
@@ -37,37 +40,40 @@ internal static class SvgSecurityValidator
     /// to external resources.
     /// </summary>
     /// <param name="path">The path to the SVG file.</param>
-    /// <param name="logger">The logger.</param>
+    /// <param name="reason">When this method returns <c>false</c>, the reason the document was rejected.</param>
     /// <returns><c>true</c> if the document is free of external references; otherwise <c>false</c>.</returns>
-    public static bool IsSafe(string path, ILogger logger)
+    public static bool IsSafe(string path, [NotNullWhen(false)] out string? reason)
     {
         try
         {
             using var stream = File.OpenRead(path);
-            return IsSafe(stream, logger);
+            reason = Validate(stream, 0);
         }
         catch (IOException ex)
         {
-            logger.LogWarning(ex, "Unable to read SVG {Path} for validation, refusing to render", path);
-            return false;
+            reason = "Unable to read the file for validation: " + ex.Message;
         }
         catch (UnauthorizedAccessException ex)
         {
-            logger.LogWarning(ex, "Unable to read SVG {Path} for validation, refusing to render", path);
-            return false;
+            reason = "Unable to read the file for validation: " + ex.Message;
         }
+
+        return reason is null;
     }
 
     /// <summary>
     /// Determines whether the SVG in the given stream is safe to rasterize.
     /// </summary>
     /// <param name="stream">The stream containing the SVG document.</param>
-    /// <param name="logger">The logger.</param>
+    /// <param name="reason">When this method returns <c>false</c>, the reason the document was rejected.</param>
     /// <returns><c>true</c> if the document is free of external references; otherwise <c>false</c>.</returns>
-    public static bool IsSafe(Stream stream, ILogger logger)
-        => IsSafe(stream, logger, 0);
+    public static bool IsSafe(Stream stream, [NotNullWhen(false)] out string? reason)
+    {
+        reason = Validate(stream, 0);
+        return reason is null;
+    }
 
-    private static bool IsSafe(Stream stream, ILogger logger, int depth)
+    private static string? Validate(Stream stream, int depth)
     {
         try
         {
@@ -77,194 +83,219 @@ internal static class SvgSecurityValidator
                 switch (reader.NodeType)
                 {
                     case XmlNodeType.DocumentType:
+                    {
                         var subset = reader.Value;
                         if (!string.IsNullOrEmpty(subset)
                             && (subset.Contains("SYSTEM", StringComparison.OrdinalIgnoreCase)
                                 || subset.Contains("PUBLIC", StringComparison.OrdinalIgnoreCase)))
                         {
-                            logger.LogWarning("Refusing to render SVG declaring an external DTD entity");
-                            return false;
+                            return "The document declares an external DTD entity";
                         }
 
                         break;
+                    }
 
                     case XmlNodeType.Element when reader.HasAttributes:
+                    {
                         for (var i = 0; i < reader.AttributeCount; i++)
                         {
                             reader.MoveToAttribute(i);
-                            var value = reader.Value;
-                            if (reader.LocalName.Equals("href", StringComparison.OrdinalIgnoreCase))
+                            var isHref = reader.LocalName.Equals("href", StringComparison.OrdinalIgnoreCase);
+                            var reason = isHref
+                                ? ValidateReference(reader.Value, depth, "href")
+                                : ValidateCss(reader.Value, depth);
+                            if (reason is not null)
                             {
-                                if (!IsReferenceSafe(value, logger, depth))
-                                {
-                                    logger.LogWarning("Refusing to render SVG referencing external resource via href");
-                                    return false;
-                                }
-                            }
-                            else if (HasUnsafeCssReference(value, logger, depth))
-                            {
-                                logger.LogWarning("Refusing to render SVG referencing external resource via style/url()");
-                                return false;
+                                return reason;
                             }
                         }
 
                         reader.MoveToElement();
                         break;
+                    }
 
                     case XmlNodeType.Text:
                     case XmlNodeType.CDATA:
-                        if (HasUnsafeCssReference(reader.Value, logger, depth))
+                    {
+                        var reason = ValidateCss(reader.Value, depth);
+                        if (reason is not null)
                         {
-                            logger.LogWarning("Refusing to render SVG referencing external resource in style block");
-                            return false;
+                            return reason;
                         }
 
                         break;
+                    }
                 }
             }
 
-            return true;
+            return null;
         }
         catch (XmlException ex)
         {
             // Malformed markup, a forbidden DTD construct or an unresolved external entity: refuse to render.
-            logger.LogWarning(ex, "Refusing to render SVG that could not be safely parsed");
-            return false;
+            return "The document could not be safely parsed: " + ex.Message;
         }
     }
 
-    private static bool IsReferenceSafe(string? value, ILogger logger, int depth)
+    private static string? ValidateReference(ReadOnlySpan<char> value, int depth, string context)
     {
-        if (string.IsNullOrEmpty(value))
-        {
-            return true;
-        }
-
         var trimmed = value.Trim();
-        if (trimmed.Length == 0 || trimmed[0] == '#')
+        if (trimmed.IsEmpty || trimmed[0] == '#')
         {
-            return true;
+            return null;
         }
 
         if (trimmed.StartsWith("data:", StringComparison.OrdinalIgnoreCase))
         {
-            return IsDataUriSafe(trimmed, logger, depth);
+            return ValidateDataUri(trimmed, depth, context);
         }
 
-        return false;
+        return "An external resource is referenced via " + context;
     }
 
-    private static bool IsDataUriSafe(string dataUri, ILogger logger, int depth)
+    private static string? ValidateDataUri(ReadOnlySpan<char> dataUri, int depth, string context)
     {
         // "data:[<mediatype>][;base64],<payload>" (mirrors Svg.Model's data URI parsing).
-        var comma = dataUri.IndexOf(',', StringComparison.Ordinal);
+        var comma = dataUri.IndexOf(',');
         if (comma < 0)
         {
-            return false;
+            return "A malformed data URI is referenced via " + context;
         }
 
         var header = dataUri[5..comma];
-        var segments = header.Split(';');
-        var mediaType = segments.Length > 0 && segments[0].Contains('/', StringComparison.Ordinal)
-            ? segments[0].Trim()
-            : "text/plain";
+        var firstSeparator = header.IndexOf(';');
+        var mediaType = (firstSeparator < 0 ? header : header[..firstSeparator]).Trim();
 
         // Only "image/svg+xml" is re-parsed as SVG by the renderer; any other type is treated as raster data.
-        if (!mediaType.Equals("image/svg+xml", StringComparison.OrdinalIgnoreCase))
+        if (!mediaType.Contains('/') || !mediaType.Equals("image/svg+xml", StringComparison.OrdinalIgnoreCase))
         {
-            return true;
+            return null;
         }
 
         if (depth >= MaxDataUriDepth)
         {
-            logger.LogWarning("Refusing to render SVG with nested data URIs exceeding the allowed depth");
-            return false;
+            return "Nested data URIs exceed the allowed depth";
         }
 
-        var isBase64 = segments.Length > 0 && segments[^1].Trim().Equals("base64", StringComparison.OrdinalIgnoreCase);
+        var lastSeparator = header.LastIndexOf(';');
+        var isBase64 = lastSeparator >= 0
+            && header[(lastSeparator + 1)..].Trim().Equals("base64", StringComparison.OrdinalIgnoreCase);
+
+        var payload = dataUri[(comma + 1)..].Trim();
+        byte[]? buffer = null;
         try
         {
-            var payload = dataUri[(comma + 1)..];
-            byte[] bytes = isBase64
-                ? Convert.FromBase64String(payload.Trim())
-                : Encoding.UTF8.GetBytes(Uri.UnescapeDataString(payload));
+            int length;
+            if (isBase64)
+            {
+                buffer = ArrayPool<byte>.Shared.Rent((payload.Length / 4 * 3) + 3);
+                if (!Convert.TryFromBase64Chars(payload, buffer, out length))
+                {
+                    return "An undecodable data URI is referenced via " + context;
+                }
+            }
+            else
+            {
+                var unescaped = Uri.UnescapeDataString(payload.ToString());
+                buffer = ArrayPool<byte>.Shared.Rent(Encoding.UTF8.GetMaxByteCount(unescaped.Length));
+                length = Encoding.UTF8.GetBytes(unescaped, buffer);
+            }
 
-            if (bytes.Length > 2 && bytes[0] == 0x1F && bytes[1] == 0x8B)
+            if (length > 2 && buffer[0] == 0x1F && buffer[1] == 0x8B)
             {
-                bytes = Decompress(bytes);
+                using var decompressed = Decompress(buffer, length);
+                return Validate(decompressed, depth + 1);
             }
 
-            using var ms = new MemoryStream(bytes, false);
-            return IsSafe(ms, logger, depth + 1);
+            using var stream = new MemoryStream(buffer, 0, length, false);
+            return Validate(stream, depth + 1);
         }
         catch (FormatException ex)
         {
-            logger.LogWarning(ex, "Refusing to render SVG with an undecodable data URI");
-            return false;
+            return "An undecodable data URI is referenced via " + context + ": " + ex.Message;
         }
         catch (InvalidDataException ex)
         {
-            logger.LogWarning(ex, "Refusing to render SVG with an invalid compressed data URI");
-            return false;
+            return "An invalid compressed data URI is referenced via " + context + ": " + ex.Message;
+        }
+        finally
+        {
+            if (buffer is not null)
+            {
+                ArrayPool<byte>.Shared.Return(buffer);
+            }
         }
     }
 
-    private static byte[] Decompress(byte[] compressed)
+    private static MemoryStream Decompress(byte[] compressed, int length)
     {
-        using var input = new MemoryStream(compressed, false);
+        using var input = new MemoryStream(compressed, 0, length, false);
         using var gzip = new GZipStream(input, CompressionMode.Decompress);
-        using var output = new MemoryStream();
-        var buffer = new byte[81920];
-        var total = 0;
-        int read;
-        while ((read = gzip.Read(buffer, 0, buffer.Length)) > 0)
+        var output = new MemoryStream();
+        var buffer = ArrayPool<byte>.Shared.Rent(DecompressBufferSize);
+        try
         {
-            total += read;
-            if (total > MaxDecompressedBytes)
+            var total = 0;
+            int read;
+            while ((read = gzip.Read(buffer, 0, buffer.Length)) > 0)
             {
-                throw new InvalidDataException("Compressed data URI exceeds the allowed size");
-            }
+                total += read;
+                if (total > MaxDecompressedBytes)
+                {
+                    throw new InvalidDataException("Compressed data URI exceeds the allowed size");
+                }
 
-            output.Write(buffer, 0, read);
+                output.Write(buffer, 0, read);
+            }
+        }
+        catch
+        {
+            output.Dispose();
+            throw;
+        }
+        finally
+        {
+            ArrayPool<byte>.Shared.Return(buffer);
         }
 
-        return output.ToArray();
+        output.Position = 0;
+        return output;
     }
 
-    private static bool HasUnsafeCssReference(string? value, ILogger logger, int depth)
+    private static string? ValidateCss(ReadOnlySpan<char> value, int depth)
     {
-        if (string.IsNullOrEmpty(value))
+        if (value.IsEmpty)
         {
-            return false;
+            return null;
         }
 
-        var span = value.AsSpan();
         var index = 0;
         while (true)
         {
-            var found = span[index..].IndexOf("url(", StringComparison.OrdinalIgnoreCase);
+            var found = value[index..].IndexOf("url(", StringComparison.OrdinalIgnoreCase);
             if (found < 0)
             {
                 break;
             }
 
             var start = index + found + 4;
-            var close = span[start..].IndexOf(')');
+            var close = value[start..].IndexOf(')');
             if (close < 0)
             {
                 break;
             }
 
-            var target = span.Slice(start, close).Trim();
+            var target = value.Slice(start, close).Trim();
             target = target.Trim('\'');
             target = target.Trim('"').Trim();
-            if (!IsReferenceSafe(target.ToString(), logger, depth))
+            var reason = ValidateReference(target, depth, "url()");
+            if (reason is not null)
             {
-                return true;
+                return reason;
             }
 
             index = start + close + 1;
-            if (index >= span.Length)
+            if (index >= value.Length)
             {
                 break;
             }
@@ -274,31 +305,35 @@ internal static class SvgSecurityValidator
         index = 0;
         while (true)
         {
-            var found = span[index..].IndexOf("@import", StringComparison.OrdinalIgnoreCase);
+            var found = value[index..].IndexOf("@import", StringComparison.OrdinalIgnoreCase);
             if (found < 0)
             {
                 break;
             }
 
-            var rest = span[(index + found + 7)..];
+            var rest = value[(index + found + 7)..];
             var quote = rest.IndexOfAny('\'', '"');
             if (quote >= 0)
             {
                 var afterQuote = rest[(quote + 1)..];
                 var end = afterQuote.IndexOfAny('\'', '"');
-                if (end >= 0 && !IsReferenceSafe(afterQuote[..end].Trim().ToString(), logger, depth))
+                if (end >= 0)
                 {
-                    return true;
+                    var reason = ValidateReference(afterQuote[..end], depth, "@import");
+                    if (reason is not null)
+                    {
+                        return reason;
+                    }
                 }
             }
 
             index = index + found + 7;
-            if (index >= span.Length)
+            if (index >= value.Length)
             {
                 break;
             }
         }
 
-        return false;
+        return null;
     }
 }
diff --git a/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs b/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs
index 62f35694f1..30b7983ece 100644
--- a/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs
+++ b/tests/Jellyfin.Drawing.Skia.Tests/SvgSecurityValidatorTests.cs
@@ -1,5 +1,4 @@
 using System.IO;
-using Microsoft.Extensions.Logging.Abstractions;
 using Xunit;
 
 namespace Jellyfin.Drawing.Skia.Tests;
@@ -31,6 +30,8 @@ public static class SvgSecurityValidatorTests
         "<svg xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink' width='16' height='16'><image xlink:href='data:image/svg+xml;base64,PHN2ZyB4bWxucz0naHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmcnIHhtbG5zOnhsaW5rPSdodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rJyB3aWR0aD0nOCcgaGVpZ2h0PSc4Jz48aW1hZ2UgeGxpbms6aHJlZj0naHR0cDovL2V4YW1wbGUuaW52YWxpZC9uZXN0ZWQucG5nJyB3aWR0aD0nOCcgaGVpZ2h0PSc4Jy8+PC9zdmc+' width='16' height='16'/></svg>",
         // Nested SVG in a URL-encoded (non-base64) data: URI referencing an external resource
         "<svg xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink' width='16' height='16'><image xlink:href='data:image/svg+xml,%3Csvg%20xmlns%3D%27http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%27%20xmlns%3Axlink%3D%27http%3A%2F%2Fwww.w3.org%2F1999%2Fxlink%27%3E%3Cimage%20xlink%3Ahref%3D%27file%3A%2F%2F%2Fetc%2Fpasswd%27%2F%3E%3C%2Fsvg%3E' width='16' height='16'/></svg>",
+        // Nested gzip-compressed (svgz) data: URI whose inner document references an external resource
+        "<svg xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink' width='16' height='16'><image xlink:href='data:image/svg+xml;base64,H4sIAAAAAAAC/23OwQrDIBAE0F/x5s217aWK8V+E2N2laiWRKP36Nin0lNvAPIZx64Zi5FTWSVJr1QL03lW/qdeCcNVaw1fIH7EjcXmewYsxBo5Wis5zo0nepaDISG2P3nEOGMVBLC3x8V+JI+SaouKyhcQz4FvVgucz4N1+x38AdK4P3LYAAAA=' width='16' height='16'/></svg>",
     };
 
     public static TheoryData<string> SafeSvgs => new()
@@ -46,6 +47,8 @@ public static class SvgSecurityValidatorTests
         "<?xml version='1.0'?><!DOCTYPE svg [<!ENTITY col 'red'>]><svg xmlns='http://www.w3.org/2000/svg' width='16' height='16'><rect width='16' height='16' fill='&col;'/></svg>",
         // A nested data:image/svg+xml payload that is itself self-contained is allowed
         "<svg xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink' width='16' height='16'><image xlink:href='data:image/svg+xml;base64,PHN2ZyB4bWxucz0naHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmcnIHdpZHRoPSc4JyBoZWlnaHQ9JzgnPjxyZWN0IHdpZHRoPSc4JyBoZWlnaHQ9JzgnIGZpbGw9J2JsdWUnLz48L3N2Zz4=' width='16' height='16'/></svg>",
+        // A self-contained gzip-compressed (svgz) data: URI is allowed
+        "<svg xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink' width='16' height='16'><image xlink:href='data:image/svg+xml;base64,H4sIAAAAAAAC/22Muw6AIAwAf6VbN0p0MQb4GBWBBB+Bav18ZXe75C5n6h3g2fJeLUbmcyQSESW9OkqgTmtNX4EgaeFocUCIPoXIDZ0pfuZfBWvK2eKUL4/kTHu4F2NB6oFrAAAA' width='16' height='16'/></svg>",
     };
 
     [Theory]
@@ -55,7 +58,8 @@ public static class SvgSecurityValidatorTests
         var path = WriteTemp(svg);
         try
         {
-            Assert.False(SvgSecurityValidator.IsSafe(path, NullLogger.Instance));
+            Assert.False(SvgSecurityValidator.IsSafe(path, out var reason));
+            Assert.NotNull(reason);
         }
         finally
         {
@@ -70,7 +74,8 @@ public static class SvgSecurityValidatorTests
         var path = WriteTemp(svg);
         try
         {
-            Assert.True(SvgSecurityValidator.IsSafe(path, NullLogger.Instance));
+            Assert.True(SvgSecurityValidator.IsSafe(path, out var reason));
+            Assert.Null(reason);
         }
         finally
         {
@@ -81,7 +86,8 @@ public static class SvgSecurityValidatorTests
     [Fact]
     public static void IsSafe_MissingFile_ReturnsFalse()
     {
-        Assert.False(SvgSecurityValidator.IsSafe(Path.Combine(Path.GetTempPath(), "does-not-exist-" + Path.GetRandomFileName() + ".svg"), NullLogger.Instance));
+        Assert.False(SvgSecurityValidator.IsSafe(Path.Combine(Path.GetTempPath(), "does-not-exist-" + Path.GetRandomFileName() + ".svg"), out var reason));
+        Assert.NotNull(reason);
     }
 
     private static string WriteTemp(string svg)