diff --git a/src/Tests/Grand.Web.Admin.Tests/Security/AntiforgeryOptOutTests.cs b/src/Tests/Grand.Web.Admin.Tests/Security/AntiforgeryOptOutTests.cs new file mode 100644 index 000000000..5f0cd153b --- /dev/null +++ b/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 +{ + /// + /// 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. + /// + [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)}"); + } +} diff --git a/src/Tests/Grand.Web.Admin.Tests/Services/BaseInfoResponseConverterTests.cs b/src/Tests/Grand.Web.Admin.Tests/Services/BaseInfoResponseConverterTests.cs new file mode 100644 index 000000000..e65cd8ba3 --- /dev/null +++ b/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; + +/// +/// OpenResponse.cwd is declared as BaseInfoResponse, so these tests serialize through that same +/// declared type - the condition under which the properties go missing. +/// +[TestClass] +public class BaseInfoResponseConverterTests +{ + private JsonSerializerOptions _options; + + [TestInitialize] + public void Setup() + { + _options = new JsonSerializerOptions(); + _options.Converters.Add(new BaseInfoResponseConverter()); + } + + /// + /// 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. + /// + [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"); + } +} diff --git a/src/Web/Grand.SharedUIResources/Views/Shared/EditorTemplates/Download.cshtml b/src/Web/Grand.SharedUIResources/Views/Shared/EditorTemplates/Download.cshtml index 83e95bcc8..53f61cbdc 100644 --- a/src/Web/Grand.SharedUIResources/Views/Shared/EditorTemplates/Download.cshtml +++ b/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 diff --git a/src/Web/Grand.SharedUIResources/Views/Shared/EditorTemplates/Editor.cshtml b/src/Web/Grand.SharedUIResources/Views/Shared/EditorTemplates/Editor.cshtml index 23e0c29cb..3a55416c7 100644 --- a/src/Web/Grand.SharedUIResources/Views/Shared/EditorTemplates/Editor.cshtml +++ b/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; }
@Html.Raw(ViewData.TemplateInfo.FormattedModelValue) @@ -18,6 +22,9 @@ function elfinderDialog@(random)(context) { // <------------------ +context var fm = $('
').dialogelfinder({ url: '@Url.Action("Connector", "ElFinder")', + customHeaders: { + 'X-CSRF-TOKEN': '@requestToken' + }, baseUrl: '@(Grand.SharedUIResources.Constants.WwwRoot)/administration/elfinder/', lang: 'en', width: 840, diff --git a/src/Web/Grand.Web.Admin/Controllers/DownloadController.cs b/src/Web/Grand.Web.Admin/Controllers/DownloadController.cs index e063c67a0..2a0d0d119 100644 --- a/src/Web/Grand.Web.Admin/Controllers/DownloadController.cs +++ b/src/Web/Grand.Web.Admin/Controllers/DownloadController.cs @@ -44,9 +44,6 @@ public async Task DownloadFile(Guid downloadGuid) } [HttpPost] - - //do not validate request token (XSRF) - [IgnoreAntiforgeryToken] public async Task SaveDownloadUrl(string downloadUrl, DownloadType downloadType = DownloadType.None, string referenceId = "") { @@ -65,8 +62,6 @@ public async Task SaveDownloadUrl(string downloadUrl, DownloadTyp } [HttpPost] - //do not validate request token (XSRF) - [IgnoreAntiforgeryToken] public virtual async Task AsyncUpload(IFormFile file, DownloadType downloadType = DownloadType.None, string referenceId = "") { diff --git a/src/Web/Grand.Web.Admin/Controllers/ElFinderController.cs b/src/Web/Grand.Web.Admin/Controllers/ElFinderController.cs index 967dd0502..9c6858623 100644 --- a/src/Web/Grand.Web.Admin/Controllers/ElFinderController.cs +++ b/src/Web/Grand.Web.Admin/Controllers/ElFinderController.cs @@ -31,7 +31,6 @@ IElFinderViewModelService elFinderViewModelService #region Methods - [IgnoreAntiforgeryToken] public virtual async Task Connector() { if (!await _permissionService.Authorize(StandardPermission.HtmlEditorManagePictures)) diff --git a/src/Web/Grand.Web.Admin/Controllers/LanguageController.cs b/src/Web/Grand.Web.Admin/Controllers/LanguageController.cs index 1d9199b71..690e1aac4 100644 --- a/src/Web/Grand.Web.Admin/Controllers/LanguageController.cs +++ b/src/Web/Grand.Web.Admin/Controllers/LanguageController.cs @@ -194,7 +194,6 @@ public async Task Delete(string id) [PermissionAuthorizeAction(PermissionActionName.Preview)] [HttpPost] - [IgnoreAntiforgeryToken] public async Task Resources(string languageId, DataSourceRequest command, LanguageResourceFilterModel model) { diff --git a/src/Web/Grand.Web.AdminShared/Services/BaseInfoResponseConverter.cs b/src/Web/Grand.Web.AdminShared/Services/BaseInfoResponseConverter.cs new file mode 100644 index 000000000..6d001f896 --- /dev/null +++ b/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; + +/// +/// 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. +/// +public class BaseInfoResponseConverter : JsonConverter +{ + /// + /// 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. + /// + 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); + } +} diff --git a/src/Web/Grand.Web.AdminShared/Services/ElFinderViewModelService.cs b/src/Web/Grand.Web.AdminShared/Services/ElFinderViewModelService.cs index c8d674277..bc7dc6995 100644 --- a/src/Web/Grand.Web.AdminShared/Services/ElFinderViewModelService.cs +++ b/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 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 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; }