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

Fix media paths with UNC format #11982

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

bergmania
Copy link
Member

Summary

This ensures we are not calling hostingEnvironment.MapPathWebRoot when the path is already rooted.

How to test

  • Set Umbraco:CMS:Global:UmbracoMediaPhysicalRootPath to an UNC path (e.g. \\ServerName\SharedFolderName\Media)
  • Boot umbraco
  • Upload media to media section
  • Verity the media is updated to the specified path.

@p-m-j
Copy link
Contributor

p-m-j commented Feb 17, 2022

tested

  • Windows with UNC path to samba share e.g. "\\nas\umbmedia" ✔️
  • Windows with releative path e.g. "umbmedia" - ends up in wwwroot/umbmedia ✔️
  • Linux absolute path e.g. "/home/pmj/umbmedia" ✔️
  • Linux relative path e.g. "umbmedia" - ends up in wwwroot/umbmedia ✔️

@p-m-j p-m-j merged commit 76fc3f8 into v9/dev Feb 17, 2022
@p-m-j p-m-j deleted the v9/hotfix/fix_media_paths_with_unc_format branch February 17, 2022 10:59
nikolajlauridsen pushed a commit that referenced this pull request Feb 17, 2022
@nikolajlauridsen
Copy link
Contributor

nikolajlauridsen commented Feb 17, 2022

Cherry picked for 9.3.1 💪

@@ -49,7 +50,7 @@ internal static IUmbracoBuilder AddFileSystems(this IUmbracoBuilder builder)
ILogger<PhysicalFileSystem> logger = factory.GetRequiredService<ILogger<PhysicalFileSystem>>();
GlobalSettings globalSettings = factory.GetRequiredService<IOptions<GlobalSettings>>().Value;

var rootPath = hostingEnvironment.MapPathWebRoot(globalSettings.UmbracoMediaPhysicalRootPath);
var rootPath = Path.IsPathRooted(globalSettings.UmbracoMediaPhysicalRootPath) ? globalSettings.UmbracoMediaPhysicalRootPath : hostingEnvironment.MapPathWebRoot(globalSettings.UmbracoMediaPhysicalRootPath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that using Path.IsPathRooted() doesn't ensure an absolute path: it returns true for C:Media, but that is still relative to the current working directory (I guess that's the content root in an ASP.NET Core application). See: https://docs.microsoft.com/en-us/dotnet/api/system.io.path.ispathfullyqualified?view=net-5.0#remarks.

The same (wrong) assumption is made in the PhysicalFileSystem:

// rootPath should be... rooted, as in, it's a root path!
if (Path.IsPathRooted(rootPath) == false)
{
// but the test suite App.config cannot really "root" anything so we have to do it here
var localRoot = hostingEnvironment.MapPathContentRoot("~");
rootPath = Path.Combine(localRoot, rootPath);
}
// clean up root path
rootPath = Path.GetFullPath(rootPath);

Using Path.IsPathFullyQualified() seems more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It certainly seems that IsPathFullyQualified aligns better with the original intent in PhysicalFileSystem according to the comments, but I don't think it matters too much.

var localRoot = @"C:\Users\Paul Johnson\Dev\umbraco\v9\src\Umbraco.Web.UI";
var physicalMediaPath = "C:media";
var combined = Path.Combine(localRoot, physicalMediaPath);
Path.GetFullPath(combined) == Path.GetFullPath(physicalMediaPath); // true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants