Validate antiforgery on the admin file manager and fix picture URLs from subfolders

src/Tests/Grand.Web.Admin.Tests/Security/AntiforgeryOptOutTests.cs

@@ -0,0 +1,35 @@
+using Grand.Web.Admin.Controllers;
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using System.Reflection;
+using Assert = Microsoft.VisualStudio.TestTools.UnitTesting.Assert;
+
+namespace Grand.Web.Admin.Tests.Security;
+
+[TestClass]
+public class AntiforgeryOptOutTests
+{
+    /// <summary>
+    ///     BaseAdminController carries [AutoValidateAntiforgeryToken]; a single [IgnoreAntiforgeryToken]
+    ///     opens the action to CSRF from an authenticated administrator's browser. The panel has no
+    ///     endpoint that needs the opt-out - every caller already sends the token, either as the
+    ///     __RequestVerificationToken field or as the X-CSRF-TOKEN header.
+    /// </summary>
+    [TestMethod]
+    public void AdminControllers_DoNotOptOutOfAntiforgery()
+    {
+        var offenders = (from controller in typeof(BaseAdminController).Assembly.GetTypes()
+                where typeof(ControllerBase).IsAssignableFrom(controller) && !controller.IsAbstract
+                from action in controller.GetMethods(BindingFlags.Public | BindingFlags.Instance |
+                                                     BindingFlags.DeclaredOnly)
+                where !action.IsSpecialName &&
+                      (action.IsDefined(typeof(IgnoreAntiforgeryTokenAttribute), true) ||
+                       controller.IsDefined(typeof(IgnoreAntiforgeryTokenAttribute), true))
+                select $"{controller.Name}.{action.Name}")
+            .Distinct()
+            .ToList();
+
+        Assert.AreEqual(0, offenders.Count,
+            $"Antiforgery validation is disabled on: {string.Join(", ", offenders)}");
+    }
+}

src/Tests/Grand.Web.Admin.Tests/Services/BaseInfoResponseConverterTests.cs

@@ -0,0 +1,63 @@
+using elFinder.Net.Core.Models.FileInfo;
+using Grand.Web.AdminShared.Services;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using System.Text.Json;
+using Assert = Microsoft.VisualStudio.TestTools.UnitTesting.Assert;
+
+namespace Grand.Web.Admin.Tests.Services;
+
+/// <summary>
+///     OpenResponse.cwd is declared as BaseInfoResponse, so these tests serialize through that same
+///     declared type - the condition under which the properties go missing.
+/// </summary>
+[TestClass]
+public class BaseInfoResponseConverterTests
+{
+    private JsonSerializerOptions _options;
+
+    [TestInitialize]
+    public void Setup()
+    {
+        _options = new JsonSerializerOptions();
+        _options.Converters.Add(new BaseInfoResponseConverter());
+    }
+
+    /// <summary>
+    ///     Losing phash costs the browser the link from the current directory to its parent - and with
+    ///     it every file URL below the volume root, because elFinder builds those by walking that chain.
+    /// </summary>
+    [TestMethod]
+    public void Directory_KeepsTheParentHashOfItsRuntimeType()
+    {
+        BaseInfoResponse cwd = new DirectoryInfoResponse {
+            name = "test", hash = "v1_XHRlc3Q", phash = "v1_", volumeid = "v1_", mime = "directory"
+        };
+
+        var json = JsonSerializer.Serialize(cwd, _options);
+
+        StringAssert.Contains(json, "\"phash\":\"v1_\"");
+        StringAssert.Contains(json, "\"volumeid\":\"v1_\"");
+    }
+
+    [TestMethod]
+    public void Root_KeepsTheRootMarkerOfItsRuntimeType()
+    {
+        BaseInfoResponse cwd = new RootInfoResponse {
+            name = "Volume", hash = "v1_", volumeid = "v1_", isroot = 1, mime = "directory"
+        };
+
+        var json = JsonSerializer.Serialize(cwd, _options);
+
+        StringAssert.Contains(json, "\"isroot\":1");
+    }
+
+    [TestMethod]
+    public void DeclaredTypeAloneStillDropsTheParentHash()
+    {
+        BaseInfoResponse cwd = new DirectoryInfoResponse { name = "test", hash = "v1_XHRlc3Q", phash = "v1_" };
+
+        var json = JsonSerializer.Serialize(cwd, new JsonSerializerOptions());
+
+        Assert.IsFalse(json.Contains("phash"), "Guards the premise of the converter - remove it and the fix is moot");
+    }
+}

src/Web/Grand.SharedUIResources/Views/Shared/EditorTemplates/Download.cshtml

@@ -24,7 +24,7 @@
                 cache: false,
                 type: "POST",
                 url: "@(Url.Action("SaveDownloadUrl", "Download", new { area }))",
-                data: { "downloadUrl": downloadUrl, DownloadType: '@ViewData["DownloadType"]', ReferenceId: '@ViewData["ReferenceId"]' },
+                data: addAntiForgeryToken({ "downloadUrl": downloadUrl, DownloadType: '@ViewData["DownloadType"]', ReferenceId: '@ViewData["ReferenceId"]' }),
                 success: function (data) {
                     if (data.success) {
                         $('#pnlDownloadURLResult@(randomNumber)').fadeIn("slow").delay(1000).fadeOut("slow");
@@ -120,7 +120,10 @@
                                         DownloadType: '@ViewData["DownloadType"]',
                                         ReferenceId: '@ViewData["ReferenceId"]'
                                     },
-                                    inputName: "file"
+                                    inputName: "file",
+                                    customHeaders: {
+                                        'X-CSRF-TOKEN': $('input[name="__RequestVerificationToken"]').val()
+                                    }
                                 },
                                 template: "@(clientId)-qq-template",
                                 multiple: false

src/Web/Grand.SharedUIResources/Views/Shared/EditorTemplates/Editor.cshtml

@@ -4,9 +4,13 @@
 
 @model string
 @inject IPermissionService permissionService
+@inject Microsoft.AspNetCore.Antiforgery.IAntiforgery antiforgery
 @{
     var allowFileman = await permissionService.Authorize(StandardPermission.HtmlEditorManagePictures);
     var random = CommonHelper.GenerateRandomInteger();
+    //the editor is not always rendered inside a form, so the token comes from IAntiforgery
+    //(which also issues the cookie) rather than from a __RequestVerificationToken input
+    var requestToken = antiforgery.GetAndStoreTokens(Context).RequestToken;
 }
 <div id="s-@(random)-summernote" class="summernote" data-name="null" data-file="null" data-format="null">
     @Html.Raw(ViewData.TemplateInfo.FormattedModelValue)
@@ -18,6 +22,9 @@
     function elfinderDialog@(random)(context) { // <------------------ +context
         var fm = $('<div/>').dialogelfinder({
             url: '@Url.Action("Connector", "ElFinder")',
+            customHeaders: {
+                'X-CSRF-TOKEN': '@requestToken'
+            },
             baseUrl: '@(Grand.SharedUIResources.Constants.WwwRoot)/administration/elfinder/',
             lang: 'en',
             width: 840,

src/Web/Grand.Web.Admin/Controllers/DownloadController.cs

@@ -44,9 +44,6 @@ public async Task<IActionResult> DownloadFile(Guid downloadGuid)
     }
 
     [HttpPost]
-
-    //do not validate request token (XSRF)
-    [IgnoreAntiforgeryToken]
     public async Task<IActionResult> SaveDownloadUrl(string downloadUrl, DownloadType downloadType = DownloadType.None,
         string referenceId = "")
     {
@@ -65,8 +62,6 @@ public async Task<IActionResult> SaveDownloadUrl(string downloadUrl, DownloadTyp
     }
 
     [HttpPost]
-    //do not validate request token (XSRF)
-    [IgnoreAntiforgeryToken]
     public virtual async Task<IActionResult> AsyncUpload(IFormFile file, DownloadType downloadType = DownloadType.None,
         string referenceId = "")
     {

src/Web/Grand.Web.Admin/Controllers/ElFinderController.cs

@@ -31,7 +31,6 @@ IElFinderViewModelService elFinderViewModelService
 
     #region Methods
 
-    [IgnoreAntiforgeryToken]
     public virtual async Task<IActionResult> Connector()
     {
         if (!await _permissionService.Authorize(StandardPermission.HtmlEditorManagePictures))

src/Web/Grand.Web.Admin/Controllers/LanguageController.cs

@@ -194,7 +194,6 @@ public async Task<IActionResult> Delete(string id)
 
     [PermissionAuthorizeAction(PermissionActionName.Preview)]
     [HttpPost]
-    [IgnoreAntiforgeryToken]
     public async Task<IActionResult> Resources(string languageId, DataSourceRequest command,
         LanguageResourceFilterModel model)
     {

src/Web/Grand.Web.AdminShared/Services/BaseInfoResponseConverter.cs

@@ -0,0 +1,37 @@
+using elFinder.Net.Core.Models.FileInfo;
+using System.Text.Json;
+using System.Text.Json.Serialization;
+
+namespace Grand.Web.AdminShared.Services;
+
+/// <summary>
+///     Writes elFinder file info by its runtime type.
+///     OpenResponse.cwd is declared as BaseInfoResponse while the instance is a DirectoryInfoResponse
+///     or a RootInfoResponse, and System.Text.Json serializes the declared type - so phash, volumeid
+///     and dirs never reach the browser. elFinder caches cwd in its file map, overwriting the complete
+///     entry it got from files[] with a parentless one, and can no longer walk a file back to the
+///     volume root. It then builds URLs as volume url + file name, dropping every subdirectory, so
+///     picking a picture out of a subfolder yields a 404.
+/// </summary>
+public class BaseInfoResponseConverter : JsonConverter<BaseInfoResponse>
+{
+    /// <summary>
+    ///     Only the declared base type is intercepted; the nested Write call resolves the runtime type
+    ///     through the default converter and does not re-enter this one.
+    /// </summary>
+    public override bool CanConvert(Type typeToConvert)
+    {
+        return typeToConvert == typeof(BaseInfoResponse);
+    }
+
+    public override BaseInfoResponse Read(ref Utf8JsonReader reader, Type typeToConvert,
+        JsonSerializerOptions options)
+    {
+        throw new NotSupportedException("The elFinder connector response is write-only");
+    }
+
+    public override void Write(Utf8JsonWriter writer, BaseInfoResponse value, JsonSerializerOptions options)
+    {
+        JsonSerializer.Serialize(writer, value, value.GetType(), options);
+    }
+}

src/Web/Grand.Web.AdminShared/Services/ElFinderViewModelService.cs

@@ -10,11 +10,14 @@
 using Microsoft.AspNetCore.Routing;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Options;
+using System.Text.Json;
 
 namespace Grand.Web.AdminShared.Services;
 
 public class ElFinderViewModelService : IElFinderViewModelService
 {
+    private readonly JsonSerializerOptions _connectorJsonOptions;
     private readonly IConnector _connector;
     private readonly IDriver _driver;
     private readonly string _fullPathToThumbs;
@@ -36,8 +39,12 @@ public ElFinderViewModelService(
         IHttpContextAccessor httpContextAccessor,
         IMediaFileStore mediaFileStore,
         LinkGenerator linkGenerator,
-        MediaSettings mediaSettings)
+        MediaSettings mediaSettings,
+        IOptions<Microsoft.AspNetCore.Mvc.JsonOptions> jsonOptions)
     {
+        _connectorJsonOptions = new JsonSerializerOptions(jsonOptions.Value.JsonSerializerOptions);
+        _connectorJsonOptions.Converters.Add(new BaseInfoResponseConverter());
+
         _driver = driver;
         _connector = connector;
         _httpContextAccessor = httpContextAccessor;
@@ -118,6 +125,11 @@ public virtual async Task<IActionResult> Connector()
         var ccTokenSource = ConnectorHelper.RegisterCcTokenSource(_httpContextAccessor.HttpContext);
         var conResult = await _connector.ProcessAsync(cmd, ccTokenSource);
         var actionResult = conResult.ToActionResult(_httpContextAccessor.HttpContext);
+
+        //serialize file info by its runtime type, otherwise cwd loses phash - see BaseInfoResponseConverter
+        if (actionResult is JsonResult jsonResult)
+            jsonResult.SerializerSettings = _connectorJsonOptions;
+
         return actionResult;
     }