Repository navigation
Keep media folders private to signed-in users, and apply the folder lock on every route - #2244
Conversation
rtBot
left a comment
There was a problem hiding this comment.
Code analysis identified issues
action-phpcs-code-review has identified potential problems in this pull request during automated scanning. We recommend reviewing the issues noted and that they are resolved.
phpcs scanning turned up:
Powered by rtCamp's GitHub Actions Library
🔍 WordPress Plugin Check Report
📊 Report
|
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
0 |
mismatched_plugin_name | Plugin name "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" is different from the name declared in plugin header "GoDAM". |
0 |
trademarked_term | The plugin name includes a restricted term. Your chosen plugin name - "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" - contains the restricted term "wordpress" which cannot be used at all in your plugin name. |
📁 assets/build/css/main.css (1 warning)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
0 |
EnqueuedStylesScope | This style is being loaded in all contexts. |
📁 assets/src/libs/analytics.min.js (5 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
0 |
EnqueuedScriptsScope | This script is being loaded in all frontend contexts. |
0 |
NonBlockingScripts.NoStrategy | This script on http://localhost:8880 (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead. |
0 |
NonBlockingScripts.NoStrategy | This script on http://localhost:8880/2026/10/26/hello-world/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead. |
0 |
NonBlockingScripts.NoStrategy | This script on http://localhost:8880/sample-page/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead. |
0 |
NonBlockingScripts.NoStrategy | This script on http://localhost:8880/demo-attachment-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead. |
📁 assets/build/js/main.min.js (5 warnings)
| 📍 Line | 🔖 Check | 💬 Message |
|---|---|---|
0 |
EnqueuedScriptsScope | This script is being loaded in all frontend contexts. |
0 |
NonBlockingScripts.NoStrategy | This script on http://localhost:8880 (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead. |
0 |
NonBlockingScripts.NoStrategy | This script on http://localhost:8880/2026/10/26/hello-world/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead. |
0 |
NonBlockingScripts.NoStrategy | This script on http://localhost:8880/sample-page/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead. |
0 |
NonBlockingScripts.NoStrategy | This script on http://localhost:8880/demo-attachment-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead. |
🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check
…ock on every route Reading the media-folder terms over REST now needs a signed-in user who can write posts (edit_posts), so the gallery block's folder picker keeps working for Contributors. Creating, changing or deleting folders needs upload_files. The Media Library folder tree and the block editor run signed in, so they are unaffected. The folder lock is applied where a folder changes rather than by matching one route path: - The media-folder term routes carry the check on their own permission callbacks, so it holds however a request reaches them (letter case, ?rest_route=, batch requests). It reads the folder id the way the controller does. - The assign_term capability is refused for a locked folder, which covers the core media route's media-folder field. That route also refuses to take a file out of a locked folder, as the assign-folder route does. - A new folder cannot be created inside a locked folder on any route. - An upload that names a locked folder, or one inside it, still succeeds but is left unfiled. The lock check itself moves to Media_Folder_Utils so these paths share it.
e811337 to
7fec177
Compare
rtBot
left a comment
There was a problem hiding this comment.
Code analysis identified issues
action-phpcs-code-review has identified potential problems in this pull request during automated scanning. We recommend reviewing the issues noted and that they are resolved.
phpcs scanning turned up:
Powered by rtCamp's GitHub Actions Library
rtBot
left a comment
There was a problem hiding this comment.
Code analysis identified issues
action-phpcs-code-review has identified potential problems in this pull request during automated scanning. We recommend reviewing the issues noted and that they are resolved.
phpcs scanning turned up:
Powered by rtCamp's GitHub Actions Library
|
Tested together with the other five 2.5.1 PRs on a hosted test site connected to production GoDAM Central. Full table and all recordings: https://github.com/rtCamp/godam-plugin-wp/issues/68 (recordings are in the rtCamp Test media library; sign in to watch). Tested commit
All passed. |
elifvish
left a comment
There was a problem hiding this comment.
Medium, the leak this PR closes is still open through class_list: media-folder is still a public taxonomy, so core adds media-folder-<slug> to every attachment's class_list. Logged out, GET /wp/v2/media?per_page=100 on a build of this PR returned media-folder-<slug> for both an open and a locked folder (attachment pages carry the same classes). Slugs are the folder names, and the changelog says folders are visible only to signed-in users. Suggested fix (inline): drop the taxonomy from post_class_taxonomies, or register it with public => false after checking the Media Library's query-attachments filtering.
Everything else held on the merged build: 401 for logged-out reads in any letter case, _embed returns rest_forbidden, and an Author gets 403 for rename / child-rename via /wp/v2/Media-Folder/ / body id / create-inside / move in or out of a locked folder, while title edits and open-folder changes still work.
Verification
- Head tested:
18edf5aa45. PHPUnit on this PR's head: 643/643 OK. - All seven 2.5.1 PRs merged onto
main(no conflicts): PHPUnit 915/915, Jest 690/690,build:prodOK. - Playwright API checks against that build on a local WordPress 7.1.3 site: 17/18 for this PR.
- Not covered: Classic attachment edit screen (post.php tax_input), left for a follow-up by the PR; Subscriber role: on test-this the Subscriber role holds upload_files (site config), so Subscriber results there do not reflect stock WordPress.
Screenshots and a recording of the local run are available on request.
Core adds a media-folder-<slug> class for each folder an attachment is in, and the media route's class_list and attachment pages show those classes to logged-out visitors. Folder slugs are folder names, so leave the taxonomy out of the post classes.
|
Review: Approvable ✅ Reviewed at high effort against the local WordPress core source. The fix is well-structured and the core-integration assumptions all check out. Verified correct:
Notes (non-blocking):
Nice work — approvable as-is. |
What changes
Who can read media folders.
/wp-json/wp/v2/media-folder(list and single) now needs a signed-in user who can write posts (edit_posts), so the gallery block's folder picker keeps working for Contributors. Creating, changing or deleting folders needsupload_files. The Media Library and block editor run signed in, so they are unaffected.The folder lock now holds on every route, applied where the change happens instead of by matching one route path:
rest_endpointsfilter wraps the permission callback of every handler on the two media-folder term routes (check_media_folder_request()), so the check runs whatever the letter case,?rest_route=or a batch request. It reads the folder id with$request->get_param( 'id' ), the way the controller does. The old route-text guard (Media_Library::enforce_locked_media_folderonrest_pre_dispatch) is removed.map_meta_caprefusesassign_termfor a locked folder or one inside it, which covers the core media route'smedia-folderfield and REST uploads.pre_insert_termrefuses a new folder inside a locked folder.rest_pre_insert_attachmentrefuses taking a file out of a locked folder through the core media route, with the samegodam_folder_lockederror as the assign-folder route.add_media_library_taxonomy_on_media_upload()leaves an upload unfiled when it names a locked folder (or one inside it). The upload itself still succeeds.Media_Folder_Utils::is_folder_locked()so every path shares it. The rules are unchanged: a folder is locked when it or an ancestor is; bookmarking a locked folder is still allowed; Editors and Admins lock and unlock as before.Testing
Throwaway WordPress (Playground, PHP 8.3) with the 2.5.0 release plus these files; each role signed in over HTTP with a REST nonce. Folders: open, open child, locked, and a child of the locked folder.
?rest_route=/wp/v2/media?_embedupload.phpgodam_folder_locked, nothing changed/wp/v2/media/<id>(any spelling)async-upload.php)upload_files(Contributor got 403). Reads were then opened toedit_posts; a Contributor reading folders (allowed) and changing them (403) is covered by unit tests.tests/php/MediaFolderLockTest.phpandtests/php/MediaUploadLockedFolderTest.php; full suite OK (643 tests).phpcsclean on every changed file;bin/hook-checkbalance and coverage checks pass.Worth knowing
_embedon/wp/v2/media.rest_forbiddeninstead of 403godam_folder_locked.post.phpwithtax_input[media-folder]) can file an Author's own image into a locked folder, because core only checks the genericassign_termscapability there. It needs a different hook and is left for a follow-up.QA:
/wp-json/wp/v2/media-folder: refused.Issue: rtCamp/godam-plugin-wp#72