Skip to content

Improve security by sanitizing and escaping user input#31

Open
miyanialkesh7 wants to merge 1 commit into
mantrabrain:masterfrom
miyanialkesh7:fix/security-audit
Open

Improve security by sanitizing and escaping user input#31
miyanialkesh7 wants to merge 1 commit into
mantrabrain:masterfrom
miyanialkesh7:fix/security-audit

Conversation

@miyanialkesh7

@miyanialkesh7 miyanialkesh7 commented Jul 5, 2026

Copy link
Copy Markdown

Summary

  • Hardened three latent/edge-case security issues found in a full-plugin audit: an un-whitelisted ORDER BY/LIMIT interpolation path in ProgressRepository, a missing paid-course guard in a dormant enrollment AJAX handler, and an unrestricted filter-supplied template path in the account page service.
  • Closed a WordPress Plugin Check (PCP) compliance gap: added ABSPATH direct-access guards to all 215 PHP files that were missing them across src/, includes/, and templates/ (362 files now covered, up from 147).
  • Replaced raw echo '<style>'/'<script>' output with wp_enqueue_style()/wp_add_inline_style()/wp_add_inline_script() everywhere the page-rendering architecture supports it (admin chrome CSS, checkout config), and added defense-in-depth esc_url() calls at 8 template output sites that were relying on escaping done earlier in the same file.
  • Added the missing EXTR_SKIP guard to a lone extract() call that lacked it, matching the pattern already used at the other three call sites in the codebase.

Details

Security

  • src/Database/Repositories/ProgressRepository.php: added a SAFE_ORDERBY whitelist + int-clamped paging (mirrors the existing EnrollmentRepository pattern) to findAll(), findByUser(), findByCourse(), closing an ORDER BY/LIMIT interpolation path that had no whitelist.
  • src/Ajax/FrontendAjax.php: handleEnrollCourse() now blocks free enrollment into paid courses, matching the two other active enrollment code paths.
  • src/Services/Frontend/AccountPageService.php: the sikshya_account_view_template filter result is now required to resolve inside WP_CONTENT_DIR before use, instead of only checking is_readable().
  • src/Database/Repositories/AdminTablesRepository.php: removed a confusing mixed esc_sql() + $wpdb->prepare() pattern on an internal (non-user-controlled) table-name constant.

PCP compliance

  • Added if (!defined('ABSPATH')) { exit; } guards to all 215 previously-unguarded files under src/, includes/, templates/, matching each directory's existing convention.
  • src/Admin/Admin.php: converted 4 raw echo '<style>' blocks (admin menu icon sizing, setup-wizard submenu hiding, React-shell chrome, dismissible-notice hiding) into a single wp_register_style('sikshya-admin-inline', false, ...) handle with wp_add_inline_style() calls, hooked on admin_enqueue_scripts instead of admin_head.
  • templates/checkout.php: replaced an inline <script>window.sikshyaCheckoutConfig = ...</script> block with wp_add_inline_script('sikshya-checkout-page', ..., 'before').
  • 8 template echo sites (account.php, single-lesson.php, single-quiz.php, learn.php, admin/setup-wizard.php) now call esc_url() at the point of output rather than relying on an earlier assignment.
  • src/Admin/Views/BaseView.php: added EXTR_SKIP to an extract() call that was missing it, preventing $template_path (used on the very next line) from being silently clobbered by a same-named data key.

Deliberately out of scope (flagged during audit, left unchanged by request):

  • The wp_kses iframe allowlist in LessonLearnContent.php (style/class on embedded iframes) — semi-trusted instructor-authored content, not an anonymous XSS vector.
  • The edit_posts capability check in CourseController::handleDeleteCourse — route is inert by default (gated behind the disabled SIKSHYA_LEGACY_AJAX flag).
  • Inline <script>/<style> blocks in CertificateRenderer.php and the full-screen shell templates (single-quiz.php, learn.php, order-invoice.php, account.php) — these deliberately bypass wp_head()/wp_footer() by design, so wp_enqueue_* doesn't apply architecturally without a larger restructure.

Test plan

  • php -l on all 219 changed files — no syntax errors
  • vendor/bin/phpcs -ps --standard=phpcs.xml src includes templates sikshya.php uninstall.php — 0 errors, 0 warnings
  • vendor/bin/phpunit --testsuite unit — 27/27 passing
  • Manual smoke test of checkout page (confirms window.sikshyaCheckoutConfig still populates correctly via the new wp_add_inline_script call) — see comment
  • Manual smoke test of an admin screen (confirms admin menu icon sizing / setup-wizard submenu hiding/notice hiding still render correctly via the new enqueue path) — see comment

@miyanialkesh7

Copy link
Copy Markdown
Author

Hey @ughimire, please review my PR request and feel free to share the changes if you feel any.

@miyanialkesh7

Copy link
Copy Markdown
Author

Manual smoke test results

Ran both pending test-plan items locally against this PR branch (fix/security-audit @ a899be68) on a WordPress 7.0.1 + WooCommerce 10.9.3 install.

✅ Checkout page — window.sikshyaCheckoutConfig

Loaded the checkout virtual page. The config renders correctly via the new wp_add_inline_script() call:

window.sikshyaCheckoutConfig = {"restUrl":"http://.../wp-json/sikshya/v1/","restNonce":"","courseIds":[],"isLoggedIn":false,"guestEnabled":true,"guestNonce":"..."};

It's emitted in a sikshya-checkout-page-js-before tag immediately ahead of the sikshya-checkout-page-js script tag, matching the wp_add_inline_script('sikshya-checkout-page', ..., 'before') call in templates/checkout.php.

✅ Admin screen — menu icon / setup-wizard submenu / notice hiding

Loaded wp-admin/admin.php?page=sikshya as an authenticated admin. The new sikshya-admin-inline-inline-css <style> block (registered via wp_register_style() + wp_add_inline_style() on admin_enqueue_scripts) renders, and every selector matches a real element on the page:

  • #adminmenu #toplevel_page_sikshya .wp-menu-image img → matches the top-level menu <li>'s raw <img> logo (sizing applies)
  • .wp-submenu li:has(a[href*="page=sikshya-setup"]) → matches the real "Setup Wizard" submenu item (hidden)
  • body.sikshya-react-shell #wpbody-content > .notice, ...<body> carries sikshya-react-shell, so notice-hiding is active

Note (pre-existing, not caused by this PR)

The test site had WooCommerce "Coming Soon" store mode enabled, which makes WordPress render the full page twice in a single response. Since WP's script/style printer only emits each handle once per request, the second render's inline scripts/styles get silently dropped when Coming Soon mode is active — so sikshyaCheckoutConfig (and potentially the admin CSS) could go missing on a site with that mode on. Unrelated to this PR's changes, but worth a follow-up issue if Coming Soon mode is in scope for any target site.

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.

1 participant