Skip to content

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

Merged
KrzysztofPajak merged 2 commits into
developfrom
fix/admin-antiforgery-optouts
Aug 9, 2026
Merged

Validate antiforgery on the admin file manager and fix picture URLs from subfolders#765
KrzysztofPajak merged 2 commits into
developfrom
fix/admin-antiforgery-optouts

Conversation

@KrzysztofPajak

Copy link
Copy Markdown
Member

Type: bugfix

Issue

Two defects in the admin media surface, found while working through the security items in the architecture audit.

1. Four admin actions opted out of antiforgery validation. BaseAdminController carries [AutoValidateAntiforgeryToken], but these actions disabled it:

Action What it does without a token
ElFinderController.Connector the whole file manager — upload, rename, delete, paste, mkfile
DownloadController.SaveDownloadUrl stores an arbitrary URL as a Download record
DownloadController.AsyncUpload uploads a file into the database
LanguageController.Resources reads the resources grid

The admin panel is enabled in every installation, so exploiting this needed nothing more than luring a signed-in administrator to a foreign page. To reproduce: sign in to /admin, then from any other origin (or the browser console) POST to /admin/ElFinder/Connector with cmd=mkfile — before this change it succeeds and writes a file.

2. Picking a picture from any media subfolder 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 media root were unaffected, which is why this went unnoticed.

Solution

Antiforgery. Removed all four [IgnoreAntiforgeryToken] attributes. The client side needed no new mechanism — the patterns were already in the repository:

  • 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. The header name comes from ServiceCollectionExtensions.AddAntiForgery.
  • 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 the 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.

Picture URLs. BaseInfoResponseConverter writes file info by its runtime type and is attached to the connector's JsonResult only, so nothing else in the application changes serialization. It intercepts the declared base type alone, so the nested write resolves through the default converter rather than re-entering.

Breaking changes

ElFinderViewModelService gained a constructor parameter (IOptions<JsonOptions>). Resolved through DI everywhere in the repository, so nothing here changes, but code that constructs or derives from this service directly needs the extra argument.

Nothing else: no view model, widget zone, plugin system name, setting, permission or localization resource changed.

Testing

  1. dotnet build ./GrandNode.sln
  2. dotnet test src/Tests/Grand.Web.Admin.Tests — 55 pass, including the three new converter tests and the antiforgery guard.
  3. Sign in to /admin. In the browser console run:
    await fetch('/admin/ElFinder/Connector', {method:'POST', credentials:'same-origin',
      body:new URLSearchParams({cmd:'open', target:'', init:'1'})}).then(r=>r.status)
    Expect 400. Repeat for /admin/Download/SaveDownloadUrl and /admin/Language/Resources — both 400.
  4. Open any entity with a rich text editor (e.g. Catalog → Categories → edit → Description) and click Manage pictures. The file tree loads, upload, rename and delete all work.
  5. Put an image in a subfolder of wwwroot/assets/images/uploaded (e.g. uploaded/test/). In the file manager open that subfolder and double-click the image: the editor receives /assets/images/uploaded/test/<name> and the image renders. Before this change the inserted URL was /assets/images/uploaded/<name> and returned 404.
  6. Product → Downloadable tab: Save download URL and the file upload both succeed.
  7. Configuration → Languages → String resources: the grid loads and filters.

🤖 Generated with Claude Code

KrzysztofPajak and others added 2 commits August 9, 2026 09:15
…oad 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 9, 2026 08:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@KrzysztofPajak
KrzysztofPajak merged commit e1679e8 into develop Aug 9, 2026
6 checks passed
@KrzysztofPajak
KrzysztofPajak deleted the fix/admin-antiforgery-optouts branch August 9, 2026 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants