Skip to content

Added permission checks to custom JS and CSS in posts meta editor#225

Merged
nk-o merged 4 commits into
masterfrom
fix-contributor-custom-script-xss
Sep 11, 2025
Merged

Added permission checks to custom JS and CSS in posts meta editor#225
nk-o merged 4 commits into
masterfrom
fix-contributor-custom-script-xss

Conversation

@nk-o
Copy link
Copy Markdown
Collaborator

@nk-o nk-o commented Sep 7, 2025

fixes XSS vulnerability where contributors could add custom JS to posts

nk-o added 3 commits September 7, 2025 18:54
fixes XSS vulnerability where contributors could add custom JS to posts
WP generates and error because send save request with all unchanged fields
@claude
Copy link
Copy Markdown

claude Bot commented Sep 7, 2025

Security Review

This PR addresses an important XSS vulnerability by implementing proper permission checks for custom JavaScript in posts. The implementation is well-designed with good security practices.

✅ Security Strengths

Permission Model:

  • Correctly uses 'unfiltered_html' capability for JavaScript editing (line 120)
  • Uses 'edit_theme_options' for global custom code (line 129)
  • These are appropriate high-privilege capabilities for script execution

WordPress Integration:

  • Auth callbacks properly integrated with WordPress meta registration (lines 152, 162)
  • Admin-only exposure of capabilities via localized variables (line 175)
  • Comprehensive permission checking in both PHP and JavaScript layers

Elegant Solution to WordPress Core Limitation:

  • Well-documented workaround for WordPress core issue (#48426, #57745)
  • Smart approach: allow updates when values haven't actually changed to prevent bulk save failures
  • Maintains security while preventing UX issues

✅ Code Quality

Documentation:

  • Excellent inline documentation explaining the WordPress limitation and solution approach
  • Clear method naming and parameter documentation

Error Handling:

  • Proper null checks in get_meta_value_from_rest_request (lines 91, 97, 104)
  • Conservative fallbacks when request data unavailable (line 64-65)

Frontend Integration:

  • UI properly shows permission warnings with Notice components (lines 176-184, 274-288)
  • Conditional rendering based on capability checks prevents unauthorized access attempts

🔒 Security Assessment

This fix properly prevents contributors from injecting arbitrary JavaScript while maintaining a good user experience. The permission model aligns with WordPress security best practices.

Risk Mitigation:

  • Before: Contributors could add arbitrary JS → High XSS risk
  • After: Only users with 'unfiltered_html' can edit JS → Risk eliminated

The implementation correctly addresses the vulnerability described in the PR.

@claude
Copy link
Copy Markdown

claude Bot commented Sep 11, 2025

Security Review: Permission Checks for Custom JS/CSS

This PR addresses a critical XSS vulnerability by implementing proper permission checks for custom JavaScript code. After reviewing the changes, here is my analysis:

Security Improvements - Strong Implementation

PHP Security (index.php:25-181):

  • Excellent: Added unfiltered_html capability check for JS code (lines 152, 162)
  • Smart workaround: The can_edit_custom_js_permission() method (lines 25-75) cleverly handles WordPress core limitation where meta auth failures block entire post saves
  • Proper capability separation: Different permissions for local JS (unfiltered_html) vs global settings (edit_theme_options)
  • Admin-only data exposure: Capability flags only exposed in admin context (line 175)

JavaScript Security (index.js:301-318):

  • Good: Uses server-provided capability flags from window.ghostkitVariables
  • Defensive: Proper fallback for backward compatibility
  • UI Protection: Shows warning notices when users lack permissions (lines 274-288)

Technical Analysis

Permission Logic:

  • unfiltered_html - Required for JavaScript editing (appropriate for XSS prevention)
  • edit_theme_options - Required for global code changes (appropriate for site-wide impact)
  • Fallback mechanism prevents post save failures when JS fields are included but unchanged

Potential Concerns

  1. Complex Permission Logic: The fallback mechanism is sophisticated but could be fragile if WordPress core behavior changes
  2. REST Request Parsing: Direct php://input reading (line 102) is somewhat fragile
  3. Capability Exposure: While admin-only, capability flags in global variables could be more secure

Recommendations

  1. Add input validation: Consider sanitizing JS/CSS input before storage
  2. Improve error handling: Add try-catch around json_decode() on line 102
  3. Add logging: Consider logging permission denials for security monitoring
  4. Unit tests: Add tests for the complex permission logic

Overall Assessment

This is a well-implemented security fix that properly addresses the XSS vulnerability, uses appropriate WordPress capabilities, handles edge cases thoughtfully, and maintains good user experience.

Recommendation: APPROVE - This effectively closes the XSS vulnerability while maintaining functionality.

@nk-o nk-o merged commit 68f7c84 into master Sep 11, 2025
5 of 11 checks passed
@nk-o nk-o deleted the fix-contributor-custom-script-xss branch September 11, 2025 07:49
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