From c31b7eaeadf1f038fdabf24d6a49dd888086786f Mon Sep 17 00:00:00 2001 From: David Peck Date: Fri, 30 Aug 2024 09:32:16 +0100 Subject: [PATCH 1/4] Update InlineSvgTagHelper.cs to catch exception --- Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs | 39 ++++++++++++-------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs b/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs index d1dd8e1..da9edd4 100644 --- a/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs +++ b/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs @@ -28,13 +28,14 @@ public class InlineSvgTagHelper : TagHelper private OurUmbracoTagHelpersConfiguration _globalSettings; private AppCaches _appCaches; - public InlineSvgTagHelper(MediaFileManager mediaFileManager, IWebHostEnvironment webHostEnvironment, IPublishedUrlProvider urlProvider, IOptions globalSettings, AppCaches appCaches) + public InlineSvgTagHelper(MediaFileManager mediaFileManager, IWebHostEnvironment webHostEnvironment, IPublishedUrlProvider urlProvider, IOptions globalSettings, AppCaches appCaches, ILogger logger) { _mediaFileManager = mediaFileManager; _webHostEnvironment = webHostEnvironment; _urlProvider = urlProvider; _globalSettings = globalSettings.Value; _appCaches = appCaches; + _logger = logger; } /// @@ -204,25 +205,33 @@ 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"); } } - cleanedFileContents = doc.DocumentNode.OuterHtml; } return cleanedFileContents; From 46b0d29ce8e79e8f876fed7bec8eb1db55c9cab4 Mon Sep 17 00:00:00 2001 From: David Peck Date: Fri, 30 Aug 2024 09:39:20 +0100 Subject: [PATCH 2/4] Fix pervious commit --- Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs b/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs index da9edd4..1c0597f 100644 --- a/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs +++ b/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,6 +28,7 @@ public class InlineSvgTagHelper : TagHelper private IPublishedUrlProvider _urlProvider; private OurUmbracoTagHelpersConfiguration _globalSettings; private AppCaches _appCaches; + private readonly ILogger _logger; public InlineSvgTagHelper(MediaFileManager mediaFileManager, IWebHostEnvironment webHostEnvironment, IPublishedUrlProvider urlProvider, IOptions globalSettings, AppCaches appCaches, ILogger logger) { From 1fd344ace9c98d19ad31bbdbc10a82ac9dd333ee Mon Sep 17 00:00:00 2001 From: David Peck Date: Fri, 30 Aug 2024 09:40:27 +0100 Subject: [PATCH 3/4] Better to return nothing than a bad SVG? --- Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs b/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs index 1c0597f..5fff5f9 100644 --- a/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs +++ b/Our.Umbraco.TagHelpers/InlineSvgTagHelper.cs @@ -233,6 +233,7 @@ public override void Process(TagHelperContext context, TagHelperOutput output) { _logger.LogWarning(exc, "Invalid svg markup"); } + return string.Empty; } } From 3a547d72db7f2746675c3a315dd00b6317dacff8 Mon Sep 17 00:00:00 2001 From: David Peck Date: Fri, 30 Aug 2024 10:28:10 +0100 Subject: [PATCH 4/4] Missed the unit tests --- .../InlineSvgTagHelperTests.cs | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/Our.Umbraco.TagHelpers.Tests/InlineSvgTagHelperTests.cs b/Our.Umbraco.TagHelpers.Tests/InlineSvgTagHelperTests.cs index c2c993d..fc32d84 100644 --- a/Our.Umbraco.TagHelpers.Tests/InlineSvgTagHelperTests.cs +++ b/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>()); tagHelper.Process(_context, _output); @@ -67,7 +68,7 @@ public void NoOutputIfNoMediaOrFileSet() public void NoOutputIfBothMediaAndFileSet() { var umbContent = Mock.Of(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>()) { 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>()) { FileSource = "test.notsvg" }; @@ -97,7 +98,7 @@ public void NoOutputIfFileNotFound() var fileProvider = new Mock(); fileProvider.Setup(p => p.GetFileInfo(It.IsAny())).Returns(Mock.Of(f => !f.Exists)); var hostEnv = Mock.Of(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>()) { FileSource = "test.svg" }; @@ -113,7 +114,7 @@ public void ExpectedOutputIfValidFile() var fileProvider = new Mock(); fileProvider.Setup(p => p.GetFileInfo(It.IsAny())).Returns(Mock.Of(f => f.Exists && f.CreateReadStream() == new MemoryStream(Encoding.UTF8.GetBytes("test svg")))); var hostEnv = Mock.Of(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>()) { FileSource = "test.svg" }; @@ -131,7 +132,7 @@ public void NoOutputIfMediaUrlNull() { var urlProvider = new Mock(); urlProvider.Setup(p => p.GetMediaUrl(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())).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>()) { MediaItem = Mock.Of(c => c.ContentType.ItemType == PublishedItemType.Media) }; @@ -147,7 +148,7 @@ public void NoOutputIfMediaNotSvg() var umbContent = Mock.Of(c => c.ContentType.ItemType == PublishedItemType.Media); var urlProvider = new Mock(); urlProvider.Setup(p => p.GetMediaUrl(umbContent, It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())).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>()) { MediaItem = umbContent }; @@ -169,7 +170,8 @@ public void NoOutputIfMediaNotFound() null, urlProvider.Object, _settings, - null) + null, + Mock.Of>()) { MediaItem = umbContent }; @@ -191,7 +193,8 @@ public void ExpectedOutputIfValidMedia() null, urlProvider.Object, _settings, - null) + null, + Mock.Of>()) { MediaItem = umbContent }; @@ -212,7 +215,7 @@ public void SanitizesJavascript() .Setup(p => p.GetFileInfo(It.IsAny())) .Returns(Mock.Of(f => f.Exists && f.CreateReadStream() == new MemoryStream(Encoding.UTF8.GetBytes("Click hereend")))); var hostEnv = Mock.Of(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>()) { FileSource = "test.svg" };