Allow filters to be excluded from the PHPFilterFunctions restricted list - #897
Merged
Merged
Conversation
PHP 8.1 deprecated `FILTER_SANITIZE_STRING` with no direct replacement, so the recommended pattern became reading the value with `FILTER_UNSAFE_RAW` (or no filter) and sanitising it afterwards. The PHPFilterFunctions sniff flags `FILTER_UNSAFE_RAW` as doing no filtering, which left developers who had knowingly adopted that pattern with no way to silence the warning short of a blanket ignore annotation. Add a configurable `exclude_filters` property so a ruleset can remove specific constants from the restricted list, for example to permit a deliberate `FILTER_UNSAFE_RAW`. Rather than exposing the default list directly, `$restricted_filters` stays private and the effective list is derived by subtracting the excludes, reusing the same `RulesetPropertyHelper::merge_custom_array()` plus `array_diff_key()` idiom already used for the `exclude` property elsewhere in VIPCS. This keeps ownership of the defaults with VIPCS while letting users express only their delta. The subtraction is recalculated per call so an inline `phpcs:set` change is honoured. The property defaults to an empty array, so behaviour is unchanged unless a ruleset opts in. Fixes #753.
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
The
WordPressVIPMinimum.Security.PHPFilterFunctionssniff flagsFILTER_DEFAULTandFILTER_UNSAFE_RAWbecause they perform no sanitisation. That was uncontroversial until PHP 8.1 deprecatedFILTER_SANITIZE_STRINGwith no direct replacement. The pattern developers were pushed towards is to read the value withFILTER_UNSAFE_RAW(or no filter at all) and sanitise it themselves afterwards. The sniff then flags that deliberate, correct code, and there is no way to opt out short of a blanket ignore annotation, as raised in #753.This adds an
exclude_filtersproperty so a ruleset can drop specific constants from the restricted list, for example to permit a knowing use ofFILTER_UNSAFE_RAW:The property defaults to an empty array, so nothing changes unless a ruleset opts in.
Design notes for reviewers
The issue asked for the
$restricted_filtersproperty to be madepublic. I have deliberately not done that. Exposing the default list hands ownership to the user: they would have to redeclare the whole list to remove one entry, and any constant VIPCS adds to the defaults later would be silently masked by their override.Instead
$restricted_filtersstays private and the effective list is derived by subtracting the user's excludes, reusing theRulesetPropertyHelper::merge_custom_array()plusarray_diff_key()idiom already used for theexcludeproperty inRestrictedExtendClassesandAbstractVariableRestrictions. So VIPCS keeps ownership of the defaults and the user only expresses their delta.RulesetPropertyHelperis internal to WordPressCS, but VIPCS already relies on it inAbstractVariableRestrictionsSniff, so this introduces no new coupling. The subtraction is recalculated on each call rather than cached, matching that precedent, so an inlinephpcs:setchange is honoured mid-file.One naming decision is open to bikeshedding: I went with
exclude_filters(descriptive form of the existingexcludeidiom).allowed_filters(positive framing) or a plainexclude(maximal consistency with the abstract sniffs) are equally reasonable — happy to rename before merge.Fixes #753.
Test plan
Unit tests use the inline
phpcs:setmechanism to prove that excludingFILTER_UNSAFE_RAWsilences it whileFILTER_DEFAULTremains flagged. I also verified the real ruleset-property path end to end: with the default config both filters warn, and withexclude_filtersset onlyFILTER_DEFAULTwarns.composer test -- --filter PHPFilterFunctionscomposer cs -- WordPressVIPMinimum/Sniffs/Security/PHPFilterFunctionsSniff.php