fix: CodeQL security issues #2251
Conversation
Version update v4.7.7
…2242) * chore(tests): update dependencies in wp-e2e-playwright package.json * fix(workflow): update NODE_VERSION to 20 in Playwright CI configuration * fix(workflow): revert NODE_VERSION to 17 in Playwright CI configuration
…or encoding (#2247) * fix(importer): replace die() with exit() in BPMediaAlbumimporter * Potential fix for code scanning alert no. 13: Incomplete string escaping or encoding
* fix(importer): replace die() with exit() in BPMediaAlbumimporter * fix: add explicit permissions to GitHub Actions workflows - Add permissions block to phpcs_on_pull_request.yml (contents: read, pull-requests: write) - Add permissions block to create.yml (contents: read) - Add permissions block to playwright.yml (contents: read) This follows the principle of least privilege and resolves GitHub code scanning alerts #1, #2, and #39. Fixes: rtCamp/support#245 --------- Co-authored-by: rtBot <43742164+rtBot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses security vulnerabilities identified by CodeQL analysis for the rtMedia plugin, focusing on XSS prevention, input sanitization, and secure coding practices.
Key Changes:
- Added HTML escaping and input sanitization functions to prevent XSS attacks
- Updated magnific-popup library from v1.0.0 to v1.2.0 with enhanced security controls
- Upgraded test dependencies and added GitHub Actions permission restrictions
- Fixed regex patterns and replaced unsafe HTML methods with safer alternatives
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/wp-e2e-playwright/package.json | Updated Playwright and WordPress packages to latest versions, added webpack-dev-server override |
| tests/codeception/composer.json | Upgraded wp-browser from v2.4 to v3.0 |
| app/importers/BPMediaAlbumimporter.php | Replaced die(true) with exit for proper script termination |
| app/assets/js/vendors/magnific-popup.js | Updated library to v1.2.0 with new security options for HTML handling |
| app/assets/js/rtMedia.js | Changed .html() to .text() for user content, enabled allowHTMLInTemplate flag |
| app/assets/js/rtMedia.backbone.js | Added XSS prevention functions and input sanitization for IDs and file names |
| app/assets/admin/js/settings.js | Fixed regex to use global flag for replacing all occurrences |
| .github/workflows/*.yml | Added explicit permission restrictions to workflow files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (mfp.st.allowHTMLInTemplate) { | ||
| template.find(EVENT_NS + '-'+key).html(value); | ||
| } else { | ||
| options.items = el; | ||
| el.off( eName ).on( eName, eHandler ); | ||
| template.find(EVENT_NS + '-'+key).text(value); | ||
| } |
There was a problem hiding this comment.
The magnific-popup library update includes changes to how HTML content is handled. Line 696-700 uses mfp.preloader.html(text) when allowHTMLInStatusIndicator is true, and mfp.preloader.text(text) otherwise. However, Line 803-807 also has a similar pattern for template handling with allowHTMLInTemplate. The security relies on these flags being set to false by default, which is correct, but usage in rtMedia.js sets allowHTMLInTemplate: true on line 65. This needs careful review to ensure no untrusted user input flows through these templates.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* chore: update magnific popup to v1.2.0 * fix: improve close button markup handling and prependTo selector logic * fix: resolve merge conflicts in close button markup and prependTo logic
|
Unable to PHPCS or SVG scan one or more files due to error running PHPCS/SVG scanner:
The error may be temporary. If the error persists, please contact a human (commit-ID: f0c04ba). |
Fixes all the security issues for the rtMedia plugin : #2240