Run the full WordPress standard in phpcs - #760
Merged
Merged
Conversation
The ruleset composed WordPress-Core, WordPress-Docs and WordPress-Extra directly rather than referencing the WordPress standard. Only the meta standard carries the `namespace="WordPressCS\WordPress"` attribute that pulls in every WordPressCS sniff, so WordPress.Security.ValidatedSanitizedInput was never running. WordPress-Extra references EscapeOutput, SafeRedirect and NonceVerification individually, which is all we had. Switch to `<rule ref="WordPress">` and move the WordPress-Docs block above it. WordPress includes WordPress-Docs itself, and the reference that registers a sniff first is the one whose exclude-pattern applies, so the parser-file exemption has to be declared first or the block pattern parsers pick up docblock errors. `<arg value="psn"/>` reports errors only, which hid the nonce warnings, so promote WordPress.Security to error rather than dropping the `n` and surfacing ~380 unrelated alignment warnings. Also move `text_domain` off the comma-separated string syntax, deprecated in PHPCS 3.3.0 and removed in 4.0, and drop two excludes naming sniffs that moved to PHPCSExtra in WPCS 3.0 and no longer match anything. That leaves 34 findings, fixed here: * themes/.../functions.php passed $_POST['report-details'] to sanitize_text_field() without wp_unslash(), so slashes were persisting into flag excerpts. Both report fields now read through guarded locals. * $_GET/$_REQUEST reads in the theme patterns, the patterns list table and the pattern creator are unslashed and sanitized. * locale() unslashes once and compares the value against its own sanitized form, keeping the reject-rather-than-coerce behaviour. Six annotations remain for nonce false positives on read-only GET paths and one deliberate raw comparison. Each was checked by removing it and running phpcs again. Adds tests for locale() and display_post_states(); the bootstrap now loads pattern-translations. Both fixes were checked by reverting them and confirming the tests fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
What
phpcs.xml.distcomposedWordPress-Core,WordPress-DocsandWordPress-Extradirectly instead of referencing theWordPressstandard. Only the meta standard carries thenamespace="WordPressCS\WordPress"attribute that auto-includes every WordPressCS sniff — the sub-rulesets reference sniffs one by one, andWordPress-Extraonly listsEscapeOutput,SafeRedirectandNonceVerification. SoWordPress.Security.ValidatedSanitizedInputwas never loaded.Separately,
<arg value="psn"/>reports errors only, which meant the nonce findings were warnings nobody saw.Between the two,
npm run lint:phpreported zero security issues on trunk while 34 were present.Ruleset changes
<rule ref="WordPress-Core">→<rule ref="WordPress">, absorbing the oneWordPress-Extraexclude.WordPress-Docsblock moved above theWordPressrule.WordPressincludesWordPress-Docsitself, and whichever reference registers a sniff first is the one whose<exclude-pattern>applies. Declared after, the parser-file exemption is silently ignored and the block pattern parsers pick up 36 docblock errors. There's a comment on the block; it's worth keeping in mind if anyone reorders the file.WordPress.Securitypromoted to<type>error</type>. Dropping thenfrompsninstead would surface 490 warnings, ~380 of them array alignment, which is a different piece of work.text_domainmoved off the comma-separated string syntax — deprecated in PHPCS 3.3.0, removed in 4.0. It was printing aDEPRECATEDnotice on every run.Generic.Arrays.DisallowShortArraySyntax.FoundandWordPress.PHP.DisallowShortTernary.Found. Both moved to PHPCSExtra in WPCS 3.0 and no longer matched anything; short ternaries are already covered by the workingUniversal.Operators.DisallowShortTernaryrule at the bottom of the file. Confirmed no-ops —phpcs -eoutput is identical with and without them.Checked with
phpcs -ethat the active sniff set is a superset of trunk: 182 → 186, nothing lost.Code changes
One actual bug:
No
wp_unslash(), andsanitize_text_field()doesn't unslash, so a report containingit'swas stored asit\'s, compounding on re-save. Both report fields now go through guarded locals withisset()defaults.The rest are unslash/sanitize on read-only request values in the theme patterns, the patterns list table and the pattern creator.
locale()in pattern-translations was restructured so the value is unslashed once and compared against its own sanitized form, which keeps the existing reject-rather-than-coerce behaviour.$_SERVER['REQUEST_URI']usesesc_url_raw().sanitize_text_field()would strip percent-encoded octets; checked against eight real REST URI shapes andesc_url_rawis byte-identical to raw on all of them.Six
phpcs:ignore/disableannotations remain — nonce false positives on read-only GET paths, plus one deliberate raw comparison inlocale(). Each was checked by deleting it and re-running phpcs; all are load-bearing.Tests
New coverage for
locale()(9 tests) anddisplay_post_states()(13). The test bootstrap now loads pattern-translations, which activates its filters for the whole suite — no effect on the existing tests, but worth knowing.Both fixes were verified by reverting them and confirming a test fails. Worth noting because my first version of the
display_post_statestest passed against the broken code — asserting "no<script>in the output" is true either way. Rewrote it aroundsanitize_key()case-folding, which actually discriminates.76 tests / 244 assertions, up from 54.
phpcsexits 0.Not covered
allow_reading_global_styles()early-returns unlessREST_REQUESTis defined, and defining that constant would leak into every subsequent test, so it's untested.Two pre-existing things noticed but left alone, both unchanged by this PR:
intval( $_POST['report-reason'] )returns1for any non-empty array, so?report-reason[]=99assigns term ID 1 rather than rejecting.strpos( $request_uri, '/wp/v2/global-styles' )misses the?rest_route=%2Fwp%2Fv2%2F...encoded form that plain-permalink sites use.