From 39de324fc7e805e5d874c0a9284efee428292514 Mon Sep 17 00:00:00 2001 From: Georg von Kries Date: Wed, 22 May 2024 14:36:43 +0200 Subject: [PATCH 1/5] Correctly escape public media URLs. --- .../Shortcodes/AssetUrlShortcodeProvider.cs | 11 ++++++- .../Shortcodes/ImageShortcodeProvider.cs | 11 ++++++- .../IFileStore.cs | 32 +++++++++++++++++-- .../DefaultMediaFileStore.cs | 2 +- .../AssetUrlShortcodeTests.cs | 13 +++----- .../OrchardCore.Media/ImageShortcodeTests.cs | 8 ++--- 6 files changed, 59 insertions(+), 18 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs index 6ab8093d15d..f34618aa17f 100644 --- a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs +++ b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs @@ -55,7 +55,16 @@ public ValueTask EvaluateAsync(string identifier, Arguments arguments, s } else { - content = _mediaFileStore.MapPathToPublicUrl(content); + var queryIndex = content.IndexOf('?'); + string queryString = null; + + if(queryIndex >= 0) + { + queryString = content[queryIndex..]; + content = content[..queryIndex]; + } + + content = _mediaFileStore.MapPathToPublicUrl(content) + queryString; } } diff --git a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs index 6d58f73b7c1..00f1c62ff32 100644 --- a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs +++ b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs @@ -72,7 +72,16 @@ public ValueTask EvaluateAsync(string identifier, Arguments arguments, s } else { - content = _mediaFileStore.MapPathToPublicUrl(content); + var queryIndex = content.IndexOf('?'); + string queryString = null; + + if (queryIndex >= 0) + { + queryString = content[queryIndex..]; + content = content[..queryIndex]; + } + + content = _mediaFileStore.MapPathToPublicUrl(content) + queryString; } } var className = string.Empty; diff --git a/src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs b/src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs index fb27eac85e0..e4ca9df7ddc 100644 --- a/src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs +++ b/src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.IO; using System.Linq; @@ -116,6 +117,9 @@ public interface IFileStore public static class IFileStoreExtensions { + private readonly static char[] _pathSeparators = ['\\', '/']; + private readonly static char[] _trimChars = ['/', ' ']; + /// /// Combines multiple path parts using the path delimiter semantics of the abstract virtual file store. /// @@ -130,8 +134,7 @@ public static string Combine(this IFileStore fileStore, params string[] paths) var normalizedParts = paths .Select(x => fileStore.NormalizePath(x)) - .Where(x => !string.IsNullOrEmpty(x)) - .ToArray(); + .Where(x => !string.IsNullOrEmpty(x)); var combined = string.Join("/", normalizedParts); @@ -156,7 +159,30 @@ public static string NormalizePath(this IFileStore _, string path) return null; } - return path.Replace('\\', '/').Trim('/', ' '); + return path.Replace('\\', '/').Trim(_trimChars); + } + + /// + /// Normalizes a path using the path delimiter semantics of the abstract virtual file store and + /// escapes each part of the path to be usable in an URI. + /// + /// + /// Backslash is converted to forward slash and any leading or trailing slashes + /// are removed. Each part of the path will be escaped by using Uri.EscapeDataString. + /// + public static string NormalizeAndEscapePath(this IFileStore _, string path) + { + if (path == null) + { + return null; + } + + var normalizedParts = + path + .Split(_pathSeparators, StringSplitOptions.RemoveEmptyEntries) + .Select(Uri.EscapeDataString); + + return string.Join('/', normalizedParts); } } } diff --git a/src/OrchardCore/OrchardCore.Media.Core/DefaultMediaFileStore.cs b/src/OrchardCore/OrchardCore.Media.Core/DefaultMediaFileStore.cs index fb5db9b54ae..5dd33aee96c 100644 --- a/src/OrchardCore/OrchardCore.Media.Core/DefaultMediaFileStore.cs +++ b/src/OrchardCore/OrchardCore.Media.Core/DefaultMediaFileStore.cs @@ -212,7 +212,7 @@ public virtual string MapPathToPublicUrl(string path) } } - return _cdnBaseUrl + _requestBasePath + "/" + _fileStore.NormalizePath(path); + return _cdnBaseUrl + _requestBasePath + "/" + _fileStore.NormalizeAndEscapePath(path); } private void ValidateRequestBasePath(HttpContext httpContext) diff --git a/test/OrchardCore.Tests/Modules/OrchardCore.Media/AssetUrlShortcodeTests.cs b/test/OrchardCore.Tests/Modules/OrchardCore.Media/AssetUrlShortcodeTests.cs index abd8ee63602..6bd411e89dd 100644 --- a/test/OrchardCore.Tests/Modules/OrchardCore.Media/AssetUrlShortcodeTests.cs +++ b/test/OrchardCore.Tests/Modules/OrchardCore.Media/AssetUrlShortcodeTests.cs @@ -20,10 +20,10 @@ public class AssetUrlShortcodeTests [InlineData("", "foo [asset_url]//bar[/asset_url] baz", @"foo //bar baz")] [InlineData("", "foo [asset_url]bar[/asset_url] baz", @"foo /media/bar baz")] [InlineData("", @"foo [asset_url width=""100""]bar[/asset_url] baz", @"foo /media/bar?width=100 baz")] - [InlineData("", @"foo [asset_url width=""100"" height=""50"" mode=""stretch""]bar[/asset_url] baz", @"foo /media/bar?width=100&height=50&rmode=stretch baz")] + [InlineData("", @"foo [asset_url width=""100"" height=""50"" mode=""stretch""]bar[/asset_url] baz", @"foo /media/bar?width=100&height=50&rmode=stretch baz")] [InlineData("", "foo [asset_url]bar[/asset_url] baz foo [asset_url]bar[/asset_url] baz", @"foo /media/bar baz foo /media/bar baz")] - [InlineData("", @"foo baz", @"foo baz")] - [InlineData("", @"foo baz", @"foo baz")] + [InlineData("", @"foo baz", @"foo baz")] + [InlineData("", @"foo baz", @"foo baz")] public async Task ShouldProcess(string cdnBaseUrl, string text, string expected) { var fileStore = new DefaultMediaFileStore( @@ -34,8 +34,6 @@ public async Task ShouldProcess(string cdnBaseUrl, string text, string expected) [], Mock.Of>()); - var sanitizer = new HtmlSanitizerService(Options.Create(new HtmlSanitizerOptions())); - var defaultHttpContext = new DefaultHttpContext(); defaultHttpContext.Request.PathBase = new PathString("/tenant"); var httpContextAccessor = Mock.Of(hca => hca.HttpContext == defaultHttpContext); @@ -47,9 +45,8 @@ public async Task ShouldProcess(string cdnBaseUrl, string text, string expected) var processor = new ShortcodeService(new IShortcodeProvider[] { assetUrlProvider }, []); var processed = await processor.ProcessAsync(text); - // The markdown part sanitizes after processing. - var sanitized = sanitizer.Sanitize(processed); - Assert.Equal(expected, sanitized); + + Assert.Equal(expected, processed); } [Theory] diff --git a/test/OrchardCore.Tests/Modules/OrchardCore.Media/ImageShortcodeTests.cs b/test/OrchardCore.Tests/Modules/OrchardCore.Media/ImageShortcodeTests.cs index 6e7a442be38..2f3a9381c9b 100644 --- a/test/OrchardCore.Tests/Modules/OrchardCore.Media/ImageShortcodeTests.cs +++ b/test/OrchardCore.Tests/Modules/OrchardCore.Media/ImageShortcodeTests.cs @@ -13,8 +13,8 @@ public class ImageShortcodeTests [InlineData("", "foo bar baz", "foo bar baz")] [InlineData("", "foo [media]bar[/media] baz", @"foo baz")] [InlineData("", "foo [media]bar[/media] baz foo [media]bar[/media] baz", @"foo baz foo baz")] - [InlineData("", "foo [media]bàr.jpeg?width=100[/media] baz", @"foo baz")] - [InlineData("", "foo [media]bàr.jpeg?width=100 onload=\"javascript: alert('XSS')[/media] baz", @"foo baz")] + [InlineData("", "foo [media]bàr.jpeg?width=100[/media] baz", @"foo baz")] + [InlineData("", "foo [media]bàr.jpeg?width=100 onload=\"javascript: alert('XSS')[/media] baz", @"foo baz")] [InlineData("", "foo [image]bar baz", "foo [image]bar baz")] [InlineData("", @"foo [image ""bar""] baz", @"foo baz")] [InlineData("", @"foo [image src=""bar""] baz", @"foo baz")] @@ -33,8 +33,8 @@ public class ImageShortcodeTests [InlineData("", "foo [image]bar[/image] baz foo [image]bar[/image] baz foo [image]bar[/image] baz", @"foo baz foo baz foo baz")] [InlineData("", "foo [image]bar.png[/image] baz foo-extended [image]bar-extended.png[/image] baz-extended", @"foo baz foo-extended baz-extended")] [InlineData("", "foo [media]bar[/media] baz foo [image]bar[/image] baz", @"foo baz foo baz")] - [InlineData("", "foo [image]bàr.jpeg?width=100[/image] baz", @"foo baz")] - [InlineData("", "foo [image]bàr.jpeg?width=100 onload=\"javascript: alert('XSS')\"[/image] baz", @"foo baz")] + [InlineData("", "foo [image]bàr.jpeg?width=100[/image] baz", @"foo baz")] + [InlineData("", "foo [image]bàr.jpeg?width=100 onload=\"javascript: alert('XSS')\"[/image] baz", @"foo baz")] public async Task ShouldProcess(string cdnBaseUrl, string text, string expected) { var sanitizerOptions = new HtmlSanitizerOptions(); From 260fcb37650c1a6a69f873f406913bc7d8480823 Mon Sep 17 00:00:00 2001 From: Georg von Kries Date: Wed, 22 May 2024 14:39:44 +0200 Subject: [PATCH 2/5] Formatting. --- .../OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs index f34618aa17f..11c5ae5ac28 100644 --- a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs +++ b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs @@ -58,7 +58,7 @@ public ValueTask EvaluateAsync(string identifier, Arguments arguments, s var queryIndex = content.IndexOf('?'); string queryString = null; - if(queryIndex >= 0) + if (queryIndex >= 0) { queryString = content[queryIndex..]; content = content[..queryIndex]; From 29cd0d0f2806b64fc6348affcd0de3fa69e8be6c Mon Sep 17 00:00:00 2001 From: Georg von Kries Date: Wed, 22 May 2024 16:41:04 +0200 Subject: [PATCH 3/5] Remove duplicated code by adding a new helper to StringUriExtensions. --- .../Shortcodes/AssetUrlShortcodeProvider.cs | 10 +--------- .../Shortcodes/ImageShortcodeProvider.cs | 10 +--------- .../StringUriExtensions.cs | 16 ++++++++++++++++ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs index 11c5ae5ac28..89eaa596c25 100644 --- a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs +++ b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs @@ -55,15 +55,7 @@ public ValueTask EvaluateAsync(string identifier, Arguments arguments, s } else { - var queryIndex = content.IndexOf('?'); - string queryString = null; - - if (queryIndex >= 0) - { - queryString = content[queryIndex..]; - content = content[..queryIndex]; - } - + content = content.RemoveQueryString(out var queryString); content = _mediaFileStore.MapPathToPublicUrl(content) + queryString; } } diff --git a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs index 00f1c62ff32..c01540a8c06 100644 --- a/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs +++ b/src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs @@ -72,15 +72,7 @@ public ValueTask EvaluateAsync(string identifier, Arguments arguments, s } else { - var queryIndex = content.IndexOf('?'); - string queryString = null; - - if (queryIndex >= 0) - { - queryString = content[queryIndex..]; - content = content[..queryIndex]; - } - + content = content.RemoveQueryString(out var queryString); content = _mediaFileStore.MapPathToPublicUrl(content) + queryString; } } diff --git a/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs b/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs index dc5d9807f5d..a8a38ef2f43 100644 --- a/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs +++ b/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs @@ -20,5 +20,21 @@ public static string ToSnakeCase(this string str) { return string.Concat(str.Select((x, i) => i > 0 && char.IsUpper(x) ? "_" + x.ToString() : x.ToString())).ToLower(); } + + public static string RemoveQueryString(this string url, out string queryString) + { + if(url != null) + { + var queryIndex = url.IndexOf('?'); + if (queryIndex >= 0) + { + queryString = url[queryIndex..]; + return url[..queryIndex]; + } + } + + queryString = null; + return url; + } } } From 0a3df8a8f86c7615762b9896f58f318bde16128e Mon Sep 17 00:00:00 2001 From: Georg von Kries Date: Wed, 22 May 2024 16:44:56 +0200 Subject: [PATCH 4/5] Formatting again. --- src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs b/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs index a8a38ef2f43..bb4039f7d0e 100644 --- a/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs +++ b/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs @@ -23,7 +23,7 @@ public static string ToSnakeCase(this string str) public static string RemoveQueryString(this string url, out string queryString) { - if(url != null) + if (url != null) { var queryIndex = url.IndexOf('?'); if (queryIndex >= 0) From f9552200e2286fd39e1f4f98e917160f8274f157 Mon Sep 17 00:00:00 2001 From: Georg von Kries Date: Thu, 23 May 2024 09:20:54 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Hisham Bin Ateya --- .../OrchardCore.Abstractions/StringUriExtensions.cs | 2 ++ .../OrchardCore.FileStorage.Abstractions/IFileStore.cs | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs b/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs index bb4039f7d0e..95eb393281f 100644 --- a/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs +++ b/src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs @@ -29,11 +29,13 @@ public static string RemoveQueryString(this string url, out string queryString) if (queryIndex >= 0) { queryString = url[queryIndex..]; + return url[..queryIndex]; } } queryString = null; + return url; } } diff --git a/src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs b/src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs index e4ca9df7ddc..f5fd3a74a08 100644 --- a/src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs +++ b/src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs @@ -177,8 +177,7 @@ public static string NormalizeAndEscapePath(this IFileStore _, string path) return null; } - var normalizedParts = - path + var normalizedParts = path .Split(_pathSeparators, StringSplitOptions.RemoveEmptyEntries) .Select(Uri.EscapeDataString);