Skip to content

Commit

Permalink
Correctly escape public media URLs.
Browse files Browse the repository at this point in the history
  • Loading branch information
@gvkries
gvkries committed May 22, 2024
1 parent 77c69c6 commit 39de324
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 18 deletions.
11 changes: 10 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/AssetUrlShortcodeProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,16 @@ public ValueTask<string> 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;
}
}

Expand Down
11 changes: 10 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Media/Shortcodes/ImageShortcodeProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,16 @@ public ValueTask<string> 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;
Expand Down
32 changes: 29 additions & 3 deletions src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -116,6 +117,9 @@ public interface IFileStore

public static class IFileStoreExtensions
{
private readonly static char[] _pathSeparators = ['\\', '/'];
private readonly static char[] _trimChars = ['/', ' '];

/// <summary>
/// Combines multiple path parts using the path delimiter semantics of the abstract virtual file store.
/// </summary>
Expand All @@ -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);

Expand All @@ -156,7 +159,30 @@ public static string NormalizePath(this IFileStore _, string path)
return null;
}

return path.Replace('\\', '/').Trim('/', ' ');
return path.Replace('\\', '/').Trim(_trimChars);
}

/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// Backslash is converted to forward slash and any leading or trailing slashes
/// are removed. Each part of the path will be escaped by using <c>Uri.EscapeDataString</c>.
/// </remarks>
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);
}
}
}
2 changes: 1 addition & 1 deletion src/OrchardCore/OrchardCore.Media.Core/DefaultMediaFileStore.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
13 changes: 5 additions & 8 deletions test/OrchardCore.Tests/Modules/OrchardCore.Media/AssetUrlShortcodeTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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&amp;height=50&amp;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 <a href=""[asset_url]bàr.jpeg onload=""javascript: alert('XSS')""[/asset_url]"">baz</a>", @"foo <a href=""/media/bàr.jpeg onload="">baz</a>")]
[InlineData("", @"foo <a href=""[asset_url]bàr.jpeg?width=100 onload=""javascript: alert('XSS')""[/asset_url]"">baz</a>", @"foo <a href=""/media/bàr.jpeg?width=100 onload="">baz</a>")]
[InlineData("", @"foo <a href=""[asset_url]bàr.jpeg[/asset_url]"">baz</a>", @"foo <a href=""/media/b%C3%A0r.jpeg"">baz</a>")]
[InlineData("", @"foo <a href=""[asset_url]bàr.jpeg?width=100[/asset_url]"">baz</a>", @"foo <a href=""/media/b%C3%A0r.jpeg?width=100"">baz</a>")]
public async Task ShouldProcess(string cdnBaseUrl, string text, string expected)
{
var fileStore = new DefaultMediaFileStore(
Expand All @@ -34,8 +34,6 @@ public async Task ShouldProcess(string cdnBaseUrl, string text, string expected)
[],
Mock.Of<ILogger<DefaultMediaFileStore>>());

var sanitizer = new HtmlSanitizerService(Options.Create(new HtmlSanitizerOptions()));

var defaultHttpContext = new DefaultHttpContext();
defaultHttpContext.Request.PathBase = new PathString("/tenant");
var httpContextAccessor = Mock.Of<IHttpContextAccessor>(hca => hca.HttpContext == defaultHttpContext);
Expand All @@ -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]
Expand Down
8 changes: 4 additions & 4 deletions test/OrchardCore.Tests/Modules/OrchardCore.Media/ImageShortcodeTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ public class ImageShortcodeTests
[InlineData("", "foo bar baz", "foo bar baz")]
[InlineData("", "foo [media]bar[/media] baz", @"foo <img src=""/media/bar""> baz")]
[InlineData("", "foo [media]bar[/media] baz foo [media]bar[/media] baz", @"foo <img src=""/media/bar""> baz foo <img src=""/media/bar""> baz")]
[InlineData("", "foo [media]bàr.jpeg?width=100[/media] baz", @"foo <img src=""/media/bàr.jpeg?width=100""> baz")]
[InlineData("", "foo [media]bàr.jpeg?width=100 onload=\"javascript: alert('XSS')[/media] baz", @"foo <img src=""/media/bàr.jpeg?width=100 onload=""> baz")]
[InlineData("", "foo [media]bàr.jpeg?width=100[/media] baz", @"foo <img src=""/media/b%C3%A0r.jpeg?width=100""> baz")]
[InlineData("", "foo [media]bàr.jpeg?width=100 onload=\"javascript: alert('XSS')[/media] baz", @"foo <img src=""/media/b%C3%A0r.jpeg?width=100 onload=""> baz")]
[InlineData("", "foo [image]bar baz", "foo [image]bar baz")]
[InlineData("", @"foo [image ""bar""] baz", @"foo <img src=""/media/bar""> baz")]
[InlineData("", @"foo [image src=""bar""] baz", @"foo <img src=""/media/bar""> baz")]
Expand All @@ -33,8 +33,8 @@ public class ImageShortcodeTests
[InlineData("", "foo [image]bar[/image] baz foo [image]bar[/image] baz foo [image]bar[/image] baz", @"foo <img src=""/media/bar""> baz foo <img src=""/media/bar""> baz foo <img src=""/media/bar""> baz")]
[InlineData("", "foo [image]bar.png[/image] baz foo-extended [image]bar-extended.png[/image] baz-extended", @"foo <img src=""/media/bar.png""> baz foo-extended <img src=""/media/bar-extended.png""> baz-extended")]
[InlineData("", "foo [media]bar[/media] baz foo [image]bar[/image] baz", @"foo <img src=""/media/bar""> baz foo <img src=""/media/bar""> baz")]
[InlineData("", "foo [image]bàr.jpeg?width=100[/image] baz", @"foo <img src=""/media/bàr.jpeg?width=100""> baz")]
[InlineData("", "foo [image]bàr.jpeg?width=100 onload=\"javascript: alert('XSS')\"[/image] baz", @"foo <img src=""/media/bàr.jpeg?width=100 onload=""> baz")]
[InlineData("", "foo [image]bàr.jpeg?width=100[/image] baz", @"foo <img src=""/media/b%C3%A0r.jpeg?width=100""> baz")]
[InlineData("", "foo [image]bàr.jpeg?width=100 onload=\"javascript: alert('XSS')\"[/image] baz", @"foo <img src=""/media/b%C3%A0r.jpeg?width=100 onload=""> baz")]
public async Task ShouldProcess(string cdnBaseUrl, string text, string expected)
{
var sanitizerOptions = new HtmlSanitizerOptions();
Expand Down

0 comments on commit 39de324

Please sign in to comment.