WordPress / WordPress/WordPress-Coding-Standards

Review SanitizationHelperTrait::is_only_sanitized() in depth

Open
#2,357 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Component: Helpers Focus: Security Status: Needs investigation
Dominant language
PHP
Stars
2.8k
Forks
521
Avg merge
5d 20h
Merged PRs (30d)
1

Description

Per https://github.com/WordPress/WordPress-Coding-Standards/pull/2356#issuecomment-1683390661:

I still have a niggly feeling there is a logic error in the is_only_sanitized() method, but I haven't been able to figure out the reason this was originally coded this way, so I'm not touching it for now.

The logic error I suspect is in the following code:

		// The only parentheses should belong to the sanitizing function. If there's
		// more than one set, this isn't *only* sanitization.
		return ( \count( $tokens[ $stackPtr ]['nested_parenthesis'] ) === 1 );

This seems to presume that parentheses could only be for function calls, while the extra parentheses may just as well be for control structure conditions.

If the other parentheses are for control structure conditions, I believe the variable should still count as "only sanitized".
It also discounts situations where unslashing is nested within a sanitization function call (which is taken into account in the NonceVerification sniff, but that is now inconsistent).

$a = is_email( $_GET['email'] ); // is_ony_sanitized(): true
$a = is_email( wp_unslash( $_GET['email'] ) ); // is_ony_sanitized(): false
if ( is_email( $_GET['email'] ) ) {} // is_ony_sanitized(): false

This should probably be further investigated when adding dedicated tests for these methods (#2272).

Might be a good idea for this and some other issues to involve the security team for second opinions on what the sniffs should accept as valid.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with SanitizationHelperTrait::is_only_sanitized() and the logic quoted in this issue, then compare its handling of nested unslashing with the NonceVerification sniff. Use the examples in the issue and the dedicated-test work referenced by #2272 to establish expected behavior for assignments and control-structure conditions. Done means the behavior is decided, covered by tests, and any security-team feedback is resolved.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
security, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.