fix: address Plugin Check findings for WordPress.org submission#38
Open
erseco wants to merge 9 commits into
Open
fix: address Plugin Check findings for WordPress.org submission#38erseco wants to merge 9 commits into
erseco wants to merge 9 commits into
Conversation
- Replace the forbidden move_uploaded_file() in the save_elp_file REST endpoint with wp_handle_upload() + an in-place copy onto the target ELP file. The PHP-WASM truncation guard and explicit failure paths are preserved. - Wrap admin/views/editor-bootstrap.php body in an IIFE so its locals stop being reported as non-prefixed plugin globals. - Rename run_exelearning() to exelearning_run() for the global-prefix rule. - Drop the load_plugin_textdomain() call; WordPress 4.6+ auto-loads translations from /languages and the plugin requires WP 6.1+. - Extend the existing ini_set() phpcs:ignore so Plugin Check's Squiz DiscouragedFunctions sniff stays silenced with the same rationale. - Update RestApiTest save-flow assertions to match the new earlier upload_error path (wp_handle_upload rejects mocked $_FILES because is_uploaded_file is false in tests). Refs #37.
Contributor
Test in WordPress PlaygroundTest the plugin with the code from this branch:
|
Removing the call broke translations because WordPress's just-in-time loader resolves the language path from the text domain, but this plugin's folder (wp-exelearning) does not match its text domain (exelearning). Without an explicit path, JIT loading looks in the wrong directory and every string falls back to English (e.g. the embed download options). The Plugin Check finding for this function is a warning, not an error, so the call is restored with a phpcs:ignore explaining why it is needed here.
… release) The distributed plugin slug is 'exelearning', matching the text domain, so WordPress's just-in-time loader finds the translations on its own. Only this development repo (folder 'wp-exelearning') sees the mismatch, which does not affect packaged releases.
CI phpcs flagged the IIFE wrapper for incorrect indentation across the entire template body (Generic.WhiteSpace.ScopeIndent). Drop the wrapper and prefix every top-level variable with exelearning_ instead; that satisfies WordPress.NamingConventions.PrefixAllGlobals without forcing a wholesale re-indent of the file.
Thanks to @mnarvaezm for the thorough security and robustness review. This commit fixes every confirmed finding and adds regression tests. Critical — stored XSS / privilege escalation via .elpx preview iframes: The media-library, block-editor and frontend (shortcode + server-rendered block) previews embedded untrusted .elpx content with `sandbox="allow-scripts allow-same-origin"` (the block-editor preview had no sandbox at all). On the same WordPress origin that combination is escapable, so a malicious .elpx uploaded by a lower-privilege user could reach the parent admin/page DOM. Removed `allow-same-origin` from every preview iframe. Because the parent can no longer inject the teacher-mode CSS through `iframe.contentDocument`, the toggler is now hidden server-side via a `teacher_mode=0` flag honoured by the content proxy. High — unsafe ZIP extraction (zip-slip / zip-bomb): ExeLearning_Elp_File_Service::extract() used ZipArchive::extractTo() with no validation. It now extracts entry by entry, rejecting traversal, absolute paths, stream wrappers and backslashes, neutralising symlinks (always writes regular files) and capping file count and total uncompressed size. High — double extraction / orphaned directories: The global wp_handle_upload filter and the REST endpoints both extracted the same upload. Added a suspend flag so the filter skips extraction during REST flows, which own their single reprocess. High — non-unique extraction hash: sha1($path . time()) could collide within one second and let cleanup delete a freshly-created extraction. Now sha1($path . microtime(true) . wp_rand()). High — non-transactional save: save_elp_file() overwrote the original .elpx before validating the new project. It now validates + extracts the uploaded file to a fresh directory first, and only replaces the original (and swaps metadata, then cleans the old extraction) once that succeeds. Medium — concurrent saves not serialized: Added a per-attachment lock around save_elp_file() that returns 409 while a save is in progress. Medium — filter returned WP_Error from a wp_handle_upload filter: process_elp_upload() now returns array('error' => ...) on failure, so callers that read $upload['error']/$upload['file'] no longer fatal under PHP 8. Medium — export URL broke on subdirectory installs: wp-exe-download.js built the export bootstrap URL from window.location.origin. It now uses a localized exportBase derived from home_url('/'). Medium — .elp/.elpx mismatch: REST create/save fell back to .elp while the rest of the plugin is .elpx-only. Filenames are now normalized to .elpx. Medium/low — boilerplate public CPT: ExeLearning_Hooks registered a public `exelearning` post type (and a no-op the_content filter) contradicting the attachment-only architecture. Removed. Low — wrong Settings action-link basename: class-admin-settings.php referenced wp-exelearning.php; the main file is exelearning.php. Now uses plugin_basename( EXELEARNING_PLUGIN_FILE ). Tests: added/updated coverage for safe extraction, the error-array contract, the suspend flag, hash uniqueness, the save lock (409), filename normalization, the correct action-link basename, the export base, the teacher-mode injection, and the sandbox no longer allowing same-origin.
The new 'Another save is already in progress...' string introduced by the save-lock fix was untranslated, failing the check-untranslated CI step. Adds the Spanish translation and regenerates the .pot/.mo.
…ders Removing allow-same-origin from the preview iframes (the XSS hardening for @mnarvaezm's finding #1) broke rendering: the eXeLearning viewer is a same-origin app and, with a null origin, fails with "Unsafe attempt to load URL ... Domains, protocols and ports must match" and shows an empty iframe in the media library, the block editor and the frontend embeds. Restore `sandbox="allow-scripts allow-same-origin allow-popups"` on all five preview iframes so content renders again. We keep the improvements that do NOT depend on the sandbox: - teacher-mode is hidden server-side via the proxy `teacher_mode` flag (no client-side contentDocument poking), and - `allow-modals` is still withheld, so previews cannot raise "Leave site?" dialogs. Properly isolating untrusted .elpx content (finding #1) requires serving the proxy from a separate origin; that is larger architectural work and is left as follow-up. The strict Content-Security-Policy on proxied HTML remains as mitigation in the meantime.
Contributor
Author
|
Huge thanks to @mnarvaezm for the thorough security and robustness review. 🙏 Every finding was confirmed against the code and addressed in this PR, with regression tests. Fixed
Finding #1 (preview iframe sandbox) — partially addressed, follow-up neededThe So
Robustly isolating untrusted |
PHPMD (code-scanning, same ruleset CI runs) reported complexity/size findings. Resolved them by refactoring (no threshold changes): - New ExeLearning_Style_Package holds the style-ZIP validate/extract/parse logic (decomposed into small helpers); ExeLearning_Styles_Service keeps thin delegators, dropping its class complexity from 158 to <100. - ExeLearning_Elp_File_Service::extract() split into extract_entries()/ extract_entry() (was cyclomatic 18 / NPath 27672). - ExeLearning_Download_Button_Renderer::render() split into build_items()/ render_dropdown() (NPath 720). The old $elp_url ternary was a no-op (both branches returned the same URL) and was simplified. - ExeLearning_Elp_Upload_Block::render_block() split into prepare/maybe-render/ no-preview/preview helpers (cyclomatic 17, NPath 6912). - ExeLearning collapses its 16 component properties into a $components array (TooManyFields 17 -> 1). - Admin_Settings::render_styles_section() extracts its inline <script> into render_styles_section_script() (was 244 lines). `phpmd . text phpmd.xml --exclude vendor,node_modules,tests,dist` now reports no violations. Also revert the teacher-mode toggler hiding to the proven client-side approach: serving the proxy with allow-same-origin restored, the block-editor useEffect and the frontend inline script inject the hiding stylesheet into the same-origin iframe again (the server-side ?teacher_mode flag did not reliably hide it). Removed the now-unused proxy injection + its tests. AGENTS.md/Makefile: `make phpmd` now uses phpmd.xml (matching CI); documented the no-threshold-bump policy and refactoring techniques.
The PHPMD refactor moved the style-ZIP logic into ExeLearning_Style_Package, but the existing StylesServiceTest carries @Covers ExeLearning_Styles_Service, so the indirectly-executed package code was attributed to nothing and the new class reported 0% — dropping overall line coverage to 70.21% (gate: 74%). Add StylePackageTest (@Covers ExeLearning_Style_Package) exercising validate(), extract_safely(), parse_config_xml(), is_unsafe_entry(), is_allowed_filename(), find_css_files(), extract_themes_from_bundle() and build_entry() across their success and error branches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Clears the findings reported by the WordPress.org Plugin Check scan so the plugin passes the automated gate. Closes #37.
move_uploaded_file()insave_elp_filewithwp_handle_upload()and an in-placecopy()onto the existing ELP path. PHP-WASM truncation guard and explicit error returns are preserved; intermediate upload is cleaned up viawp_delete_file().editor-bootstrap.phpnon-prefixed globals (~26 warnings): wrap the template body in an IIFE so locals become function-scoped instead of file-scoped, clearing everyWordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFoundwarning in one change.run_exelearning()non-prefixed function: rename toexelearning_run().load_plugin_textdomain()discouraged: drop the call; WordPress 4.6+ auto-loads translations from/languagesandreadme.txtalready declaresRequires at least: 6.1.ini_set()discouraged: extend the existingphpcs:ignoreto also coverSquiz.PHP.DiscouragedFunctions.Discouraged(same rationale: standalone editor page output must stay clean).Test plan
make lint— no findings.make test FILTER=RestApiTest— 84 tests / 180 assertions pass.make test— only pre-existingStaticEditorInstallerTest::is_editor_installed_returns_false_when_missingfailure remains (unrelated, reproduces onmain).Notes
RestApiTestassertions were tightened to expectupload_errorinstead ofelp_not_zip/move_failed:wp_handle_upload()now rejects the mocked$_FILESearly (becauseis_uploaded_file()isfalsein unit tests). Production behavior is unchanged — real uploads still pass through the security check.