Merge pull request #72 from drpeck/patch-1

A malformed SVG can tank the entire page

Author: warrenbuckley

Our.Umbraco.TagHelpers.Tests/InlineSvgTagHelperTests.cs

@@ -1,6 +1,7 @@
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Razor.TagHelpers;
 using Microsoft.Extensions.FileProviders;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Moq;
 using NUnit.Framework;
@@ -56,7 +57,7 @@ public void Setup()
         public void NoOutputIfNoMediaOrFileSet()
         {
 
-            var tagHelper = new InlineSvgTagHelper(null, null, null, _settings, null);
+            var tagHelper = new InlineSvgTagHelper(null, null, null, _settings, null, Mock.Of<ILogger<InlineSvgTagHelper>>());
 
             tagHelper.Process(_context, _output);
 
@@ -67,7 +68,7 @@ public void NoOutputIfNoMediaOrFileSet()
         public void NoOutputIfBothMediaAndFileSet()
         {
             var umbContent = Mock.Of<IPublishedContent>(c => c.ContentType.ItemType == PublishedItemType.Media);
-            var tagHelper = new InlineSvgTagHelper(null, null, null, _settings, null)
+            var tagHelper = new InlineSvgTagHelper(null, null, null, _settings, null, Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 FileSource = "test.svg",
                 MediaItem = umbContent
@@ -81,7 +82,7 @@ public void NoOutputIfBothMediaAndFileSet()
         [Test]
         public void NoOutputIfFileNotSvg()
         {
-            var tagHelper = new InlineSvgTagHelper(null, null, null, _settings, null)
+            var tagHelper = new InlineSvgTagHelper(null, null, null, _settings, null, Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 FileSource = "test.notsvg"
             };
@@ -97,7 +98,7 @@ public void NoOutputIfFileNotFound()
             var fileProvider = new Mock<IFileProvider>();
             fileProvider.Setup(p => p.GetFileInfo(It.IsAny<string>())).Returns(Mock.Of<IFileInfo>(f => !f.Exists));
             var hostEnv = Mock.Of<IWebHostEnvironment>(e => e.WebRootFileProvider == fileProvider.Object);
-            var tagHelper = new InlineSvgTagHelper(null, hostEnv, null, _settings, null)
+            var tagHelper = new InlineSvgTagHelper(null, hostEnv, null, _settings, null, Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 FileSource = "test.svg"
             };
@@ -113,7 +114,7 @@ public void ExpectedOutputIfValidFile()
             var fileProvider = new Mock<IFileProvider>();
             fileProvider.Setup(p => p.GetFileInfo(It.IsAny<string>())).Returns(Mock.Of<IFileInfo>(f => f.Exists && f.CreateReadStream() == new MemoryStream(Encoding.UTF8.GetBytes("test svg"))));
             var hostEnv = Mock.Of<IWebHostEnvironment>(e => e.WebRootFileProvider == fileProvider.Object);
-            var tagHelper = new InlineSvgTagHelper(null, hostEnv, null, _settings, null)
+            var tagHelper = new InlineSvgTagHelper(null, hostEnv, null, _settings, null, Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 FileSource = "test.svg"
             };
@@ -131,7 +132,7 @@ public void NoOutputIfMediaUrlNull()
         {
             var urlProvider = new Mock<IPublishedUrlProvider>();
             urlProvider.Setup(p => p.GetMediaUrl(It.IsAny<IPublishedContent>(), It.IsAny<UrlMode>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Uri>())).Returns((string)null!);
-            var tagHelper = new InlineSvgTagHelper(null, null, urlProvider.Object, _settings, null)
+            var tagHelper = new InlineSvgTagHelper(null, null, urlProvider.Object, _settings, null, Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 MediaItem = Mock.Of<IPublishedContent>(c => c.ContentType.ItemType == PublishedItemType.Media)
             };
@@ -147,7 +148,7 @@ public void NoOutputIfMediaNotSvg()
             var umbContent = Mock.Of<IPublishedContent>(c => c.ContentType.ItemType == PublishedItemType.Media);
             var urlProvider = new Mock<IPublishedUrlProvider>();
             urlProvider.Setup(p => p.GetMediaUrl(umbContent, It.IsAny<UrlMode>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Uri>())).Returns("test.notsvg");
-            var tagHelper = new InlineSvgTagHelper(null, null, urlProvider.Object, _settings, null)
+            var tagHelper = new InlineSvgTagHelper(null, null, urlProvider.Object, _settings, null, Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 MediaItem = umbContent
             };
@@ -169,7 +170,8 @@ public void NoOutputIfMediaNotFound()
                 null,
                 urlProvider.Object,
                 _settings, 
-                null)
+                null, 
+                Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 MediaItem = umbContent
             };
@@ -191,7 +193,8 @@ public void ExpectedOutputIfValidMedia()
                 null,
                 urlProvider.Object,
                 _settings, 
-                null)
+                null, 
+                Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 MediaItem = umbContent
             };
@@ -212,7 +215,7 @@ public void SanitizesJavascript()
                 .Setup(p => p.GetFileInfo(It.IsAny<string>()))
                 .Returns(Mock.Of<IFileInfo>(f => f.Exists && f.CreateReadStream() == new MemoryStream(Encoding.UTF8.GetBytes("<a xlink:href=\"javascript:alert('test');\">Click here</a><script attr=\"test\">test</script>end"))));
             var hostEnv = Mock.Of<IWebHostEnvironment>(e => e.WebRootFileProvider == fileProvider.Object);
-            var tagHelper = new InlineSvgTagHelper(null, hostEnv, null, _settings, null)
+            var tagHelper = new InlineSvgTagHelper(null, hostEnv, null, _settings, null, Mock.Of<ILogger<InlineSvgTagHelper>>())
             {
                 FileSource = "test.svg"
             };

Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs

@@ -1,6 +1,7 @@
 using HtmlAgilityPack;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Razor.TagHelpers;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Our.Umbraco.TagHelpers.Configuration;
 using Our.Umbraco.TagHelpers.Utils;
@@ -27,14 +28,16 @@ public class InlineSvgTagHelper : TagHelper
         private IPublishedUrlProvider _urlProvider;
         private OurUmbracoTagHelpersConfiguration _globalSettings;
         private AppCaches _appCaches;
+        private readonly ILogger<InlineSvgTagHelper> _logger;
 
-        public InlineSvgTagHelper(MediaFileManager mediaFileManager, IWebHostEnvironment webHostEnvironment, IPublishedUrlProvider urlProvider, IOptions<OurUmbracoTagHelpersConfiguration> globalSettings, AppCaches appCaches)
+        public InlineSvgTagHelper(MediaFileManager mediaFileManager, IWebHostEnvironment webHostEnvironment, IPublishedUrlProvider urlProvider, IOptions<OurUmbracoTagHelpersConfiguration> globalSettings, AppCaches appCaches, ILogger<InlineSvgTagHelper> logger)
         {
             _mediaFileManager = mediaFileManager;
             _webHostEnvironment = webHostEnvironment;
             _urlProvider = urlProvider;
             _globalSettings = globalSettings.Value;
             _appCaches = appCaches;
+            _logger = logger;
         }
 
         /// <summary>
@@ -204,25 +207,34 @@ public override void Process(TagHelperContext context, TagHelperOutput output)
             if ((EnsureViewBox || (_globalSettings.OurSVG.EnsureViewBox && !IgnoreAppSettings)) || !string.IsNullOrEmpty(CssClass))
             {
                 HtmlDocument doc = new HtmlDocument();
-                doc.LoadHtml(cleanedFileContents);
-                var svgs = doc.DocumentNode.SelectNodes("//svg");
-                foreach (var svgNode in svgs)
-                {
-                    if (!string.IsNullOrEmpty(CssClass))
+                try {
+                    doc.LoadHtml(cleanedFileContents);
+                    var svgs = doc.DocumentNode.SelectNodes("//svg");
+                    foreach (var svgNode in svgs)
                     {
-                        svgNode.AddClass(CssClass);
+                        if (!string.IsNullOrEmpty(CssClass))
+                        {
+                            svgNode.AddClass(CssClass);
+                        }
+                        if ((EnsureViewBox || (_globalSettings.OurSVG.EnsureViewBox && !IgnoreAppSettings)) && svgNode.Attributes.Contains("width") && svgNode.Attributes.Contains("height") && !svgNode.Attributes.Contains("viewbox"))
+                        {
+                            var width = StringUtils.GetDecimal(svgNode.GetAttributeValue("width", "0"));
+                            var height = StringUtils.GetDecimal(svgNode.GetAttributeValue("height", "0"));
+                            svgNode.SetAttributeValue("viewbox", $"0 0 {width} {height}");
+    
+                            svgNode.Attributes.Remove("width");
+                            svgNode.Attributes.Remove("height");
+                        }
                     }
-                    if ((EnsureViewBox || (_globalSettings.OurSVG.EnsureViewBox && !IgnoreAppSettings)) && svgNode.Attributes.Contains("width") && svgNode.Attributes.Contains("height") && !svgNode.Attributes.Contains("viewbox"))
+                    cleanedFileContents = doc.DocumentNode.OuterHtml;
+                }
+                catch(Exception exc) {
+                    if(_logger.IsEnabled(LogLevel.Warning))
                     {
-                        var width = StringUtils.GetDecimal(svgNode.GetAttributeValue("width", "0"));
-                        var height = StringUtils.GetDecimal(svgNode.GetAttributeValue("height", "0"));
-                        svgNode.SetAttributeValue("viewbox", $"0 0 {width} {height}");
-
-                        svgNode.Attributes.Remove("width");
-                        svgNode.Attributes.Remove("height");
+                        _logger.LogWarning(exc, "Invalid svg markup");
                     }
+                    return string.Empty;
                 }
-                cleanedFileContents = doc.DocumentNode.OuterHtml;
             }
 
             return cleanedFileContents;