From e7ecb377ab43faf214919e186e9232b57546043b Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 9 Aug 2026 09:15:07 +0200 Subject: [PATCH 1/2] fix(admin): validate antiforgery tokens on the file manager and download endpoints BaseAdminController applies [AutoValidateAntiforgeryToken], but four actions opted out of it. The worst is ElFinderController.Connector - one action serving the whole file manager, so upload, rename, delete and paste were all reachable through CSRF from an authenticated administrator's browser. DownloadController.SaveDownloadUrl and AsyncUpload were equally open, and LanguageController.Resources opted out for no reason at all. The client side needed no new mechanism. elFinder and fineUploader now send customHeaders: { 'X-CSRF-TOKEN': ... }, the same way Picture.cshtml already did for an AsyncUpload that never carried the opt-out. Editor.cshtml takes the token from an injected IAntiforgery rather than from a __RequestVerificationToken input, because the editor is not always rendered inside a form and GetAndStoreTokens issues the cookie as well. The language resources grid already called addAntiForgeryToken in additionalData(), so removing its attribute was enough. AntiforgeryOptOutTests replaces the hand-kept list: it walks the panel assembly and fails if any action carries IgnoreAntiforgeryTokenAttribute. Verified that it fails on the old behaviour, naming exactly those four actions. The opt-outs in Grand.Module.Api are left alone - TokenController and TokenWebController are anonymous JSON endpoints issuing JWTs, with no cookie authentication for CSRF to ride on. Co-Authored-By: Claude Opus 5 --- .../Security/AntiforgeryOptOutTests.cs | 35 +++++++++++++++++++ .../Shared/EditorTemplates/Download.cshtml | 7 ++-- .../Shared/EditorTemplates/Editor.cshtml | 7 ++++ .../Controllers/DownloadController.cs | 5 --- .../Controllers/ElFinderController.cs | 1 - .../Controllers/LanguageController.cs | 1 - 6 files changed, 47 insertions(+), 9 deletions(-) create mode 100644 src/Tests/Grand.Web.Admin.Tests/Security/AntiforgeryOptOutTests.cs 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/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) { From fa90b53c90e6fe405f633d2e23ad3965ad41f066 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 9 Aug 2026 10:09:59 +0200 Subject: [PATCH 2/2] fix(admin): keep the parent hash in the elFinder connector response Picking a picture out of any subfolder of the media library inserted a URL that 404s: the subdirectory was missing from it. OpenResponse.cwd is declared as BaseInfoResponse while the instance is a DirectoryInfoResponse or RootInfoResponse, which add phash, volumeid and dirs. System.Text.Json serializes the declared type, so those three never reached the browser. elFinder caches cwd in its file map, overwriting the complete entry it already had from files[] with a parentless one; path2array then stops at the current directory instead of walking up to the volume root, and url() builds volume url + file name with every intermediate directory dropped. Files sitting directly in the root were unaffected, which is why this went unnoticed. BaseInfoResponseConverter writes file info by its runtime type and is attached to the connector's JsonResult only, so nothing else in the app changes serialization. It intercepts the declared base type alone, so the nested write resolves through the default converter rather than re-entering. Verified in the browser: cwd now carries phash and volumeid, the path resolves to Volume/test/PHOTO-1.jpg, the URL returns 200 instead of 404, and double-clicking a picture inserts /assets/images/uploaded/test/PHOTO-1.jpg into the editor. Co-Authored-By: Claude Opus 5 --- .../BaseInfoResponseConverterTests.cs | 63 +++++++++++++++++++ .../Services/BaseInfoResponseConverter.cs | 37 +++++++++++ .../Services/ElFinderViewModelService.cs | 14 ++++- 3 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 src/Tests/Grand.Web.Admin.Tests/Services/BaseInfoResponseConverterTests.cs create mode 100644 src/Web/Grand.Web.AdminShared/Services/BaseInfoResponseConverter.cs 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.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; }