fix(checks): reduce false positives for prepare() and saveHtml()#1348
Open
faisalahammad wants to merge 1 commit into
Open
fix(checks): reduce false positives for prepare() and saveHtml()#1348faisalahammad wants to merge 1 commit into
faisalahammad wants to merge 1 commit into
Conversation
- Downgrade WordPress.DB.PreparedSQL.NotPrepared from error to warning when a variable is passed through $wpdb->prepare(), since PHPCS cannot trace the chain with the spread operator. - Suppress WordPress.Security.EscapeOutput.OutputNotEscaped for DOMDocument::saveHtml() and saveXML() calls, since the output is already safe HTML/XML built through the DOM API. - Add PHPUnit tests and test data for both fixes. Fixes WordPress#1333
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @Tsjippy. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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
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
Reduce two false positives in the PHPCS-based checks. Downgrade
WordPress.DB.PreparedSQL.NotPreparedto a warning when a query is passed through$wpdb->prepare()with the spread operator, and stop reportingWordPress.Security.EscapeOutput.OutputNotEscapedforDOMDocument::saveHtml()andsaveXML()calls.Fixes #1333
Changes
includes/Checker/Checks/Plugin_Repo/Plugin_Review_PHPCS_Check.phpBefore:
After:
Why: PHPCS cannot follow a variable through
$wpdb->prepare( $query, ...$args ), so the query is flagged as not prepared even when it is. Downgrading to a warning keeps the suggestion visible without blocking plugin submissions.includes/Checker/Checks/Security/Late_Escaping_Check.phpBefore: The
OutputNotEscapedcase only assigned a docs URL.After: Adds a new helper
line_calls_save_dom_method()that reads the source line for the reported error. If the line containssaveHtml(orsaveXML(, the message is downgraded to a warning and pointed at the PHP manual page.Why:
DOMDocument::saveHtml()andsaveXML()return safe HTML/XML built through the DOM API, so no further escaping is needed. Looking at the source line (rather than the PHPCS message, which only contains the variable name like$dom) is the reliable way to detect this.Testing
Ran the full PHPUnit suite: 477 tests, 1353 assertions, all pass. Added two new test methods and one new test data plugin.
Test 1: Prepared SQL downgrade
$wpdb->get_results( $wpdb->prepare( $query, ...$args ) )WordPress.DB.PreparedSQL.NotPreparedis a warning, not an errorResult: Works as expected.
Test 2: saveHtml() suppression
echo $dom->saveHtml();EscapeOutput.OutputNotEscapedreported for that lineResult: Works as expected.
Test 3: Normal escape detection still works
echo $test;where$testis a raw stringResult: Works as expected.
Build
plugin-check-fix-1333.zip available for manual testing.
Install via WP Admin → Plugins → Upload Plugin.