Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly escape public media URLs. #16135

Merged
merged 9 commits into from
May 27, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ public ValueTask<string> EvaluateAsync(string identifier, Arguments arguments, s
}
else
{
content = _mediaFileStore.MapPathToPublicUrl(content);
content = content.RemoveQueryString(out var queryString);
content = _mediaFileStore.MapPathToPublicUrl(content) + queryString;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ public ValueTask<string> EvaluateAsync(string identifier, Arguments arguments, s
}
else
{
content = _mediaFileStore.MapPathToPublicUrl(content);
content = content.RemoveQueryString(out var queryString);
content = _mediaFileStore.MapPathToPublicUrl(content) + queryString;
}
}
var className = string.Empty;
Expand Down
18 changes: 18 additions & 0 deletions src/OrchardCore/OrchardCore.Abstractions/StringUriExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,23 @@ 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];
gvkries marked this conversation as resolved.
Show resolved Hide resolved
}
}

queryString = null;

return url;
gvkries marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
31 changes: 28 additions & 3 deletions src/OrchardCore/OrchardCore.FileStorage.Abstractions/IFileStore.cs
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,29 @@ public static string NormalizePath(this IFileStore _, string path)
return null;
}

return path.Replace('\\', '/').Trim('/', ' ');
return path.Replace('\\', '/').Trim(_trimChars);
gvkries marked this conversation as resolved.
Show resolved Hide resolved
}

/// <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);
}
}
}
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
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
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