Skip to content

filter_var() should return non empty string only when it will not be sanitized #650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Aug 29, 2021
Merged

filter_var() should return non empty string only when it will not be sanitized #650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
260d542
Add support for FILTER_VALIDATE_DOMAIN
devbanana Aug 28, 2021
5742978
Only return non-empty-string if there is no chance of sanitization
devbanana Aug 28, 2021
c16a335
Add Scope type declaration
devbanana Aug 28, 2021
5962964
Make boolean explicit in condition
devbanana Aug 28, 2021
b237ee3
Move bitmask to constant
devbanana Aug 28, 2021
21f4c8a
Fix coding style
devbanana Aug 28, 2021
3b55c95
Fix checking for Stringtype
devbanana Aug 29, 2021
1961a37
Move check for StringType to calling condition
devbanana Aug 29, 2021
015bc7b
Add test to ensure it only operates on string filters
devbanana Aug 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/Type/Php/FilterVarDynamicReturnTypeExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@
class FilterVarDynamicReturnTypeExtension implements DynamicFunctionReturnTypeExtension
{

/**
* All validation filters match 0x100.
*/
private const VALIDATION_FILTER_BITMASK = 0x100;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, do we know if this is intentional, or a coincidence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks intentional to me. I went through the constants and all validation filters start with 0x100, and all sanitization filters start with 0x200. FILTER_CALLBACK is 0x400. I figured taking advantage of the pattern was better than iterating through every possible filter that should match.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for checking that


private ReflectionProvider $reflectionProvider;

private ConstantStringType $flagsString;
Expand Down Expand Up @@ -66,6 +71,7 @@ private function getFilterTypeMap(): array
$this->getConstant('FILTER_SANITIZE_STRING') => $stringType,
$this->getConstant('FILTER_SANITIZE_URL') => $stringType,
$this->getConstant('FILTER_VALIDATE_BOOLEAN') => $booleanType,
$this->getConstant('FILTER_VALIDATE_DOMAIN') => $stringType,
$this->getConstant('FILTER_VALIDATE_EMAIL') => $stringType,
$this->getConstant('FILTER_VALIDATE_FLOAT') => $floatType,
$this->getConstant('FILTER_VALIDATE_INT') => $intType,
Expand Down Expand Up @@ -130,7 +136,9 @@ public function getTypeFromFunctionCall(
$type = $this->getFilterTypeMap()[$filterValue] ?? $mixedType;
$otherType = $this->getOtherType($flagsArg, $scope);

if ($inputType->isNonEmptyString()->yes()) {
if ($inputType->isNonEmptyString()->yes()
&& $type instanceof StringType
&& !$this->canStringBeSanitized($filterValue, $flagsArg, $scope)) {
$type = new IntersectionType([$type, new AccessoryNonEmptyStringType()]);
}

Expand Down Expand Up @@ -222,4 +230,22 @@ private function getFlagsValue(Type $exprType): Type
return $exprType->getOffsetValueType($this->flagsString);
}

private function canStringBeSanitized(int $filterValue, ?Node\Arg $flagsArg, Scope $scope): bool
{
// If it is a validation filter, the string will not be changed
if (($filterValue & self::VALIDATION_FILTER_BITMASK) !== 0) {
return false;
}

// FILTER_DEFAULT will not sanitize, unless it has FILTER_FLAG_STRIP_LOW,
// FILTER_FLAG_STRIP_HIGH, or FILTER_FLAG_STRIP_BACKTICK
if ($filterValue === $this->getConstant('FILTER_DEFAULT')) {
return $this->hasFlag($this->getConstant('FILTER_FLAG_STRIP_LOW'), $flagsArg, $scope)
|| $this->hasFlag($this->getConstant('FILTER_FLAG_STRIP_HIGH'), $flagsArg, $scope)
|| $this->hasFlag($this->getConstant('FILTER_FLAG_STRIP_BACKTICK'), $flagsArg, $scope);
}

return true;
}

}
42 changes: 41 additions & 1 deletion tests/PHPStan/Analyser/data/filter-var-returns-non-empty-string.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,51 @@ public function run(string $str): void
$return = filter_var($str, FILTER_DEFAULT);
assertType('non-empty-string|false', $return);

$return = filter_var($str, FILTER_SANITIZE_STRING);
$return = filter_var($str, FILTER_DEFAULT, FILTER_FLAG_STRIP_LOW);
assertType('string|false', $return);

$return = filter_var($str, FILTER_DEFAULT, FILTER_FLAG_STRIP_HIGH);
assertType('string|false', $return);

$return = filter_var($str, FILTER_DEFAULT, FILTER_FLAG_STRIP_BACKTICK);
assertType('string|false', $return);

$return = filter_var($str, FILTER_VALIDATE_EMAIL);
assertType('non-empty-string|false', $return);

$return = filter_var($str, FILTER_VALIDATE_REGEXP);
assertType('non-empty-string|false', $return);

$return = filter_var($str, FILTER_VALIDATE_URL);
assertType('non-empty-string|false', $return);

$return = filter_var($str, FILTER_VALIDATE_URL, FILTER_NULL_ON_FAILURE);
assertType('non-empty-string|null', $return);

$return = filter_var($str, FILTER_VALIDATE_IP);
assertType('non-empty-string|false', $return);

$return = filter_var($str, FILTER_VALIDATE_MAC);
assertType('non-empty-string|false', $return);

$return = filter_var($str, FILTER_VALIDATE_DOMAIN);
assertType('non-empty-string|false', $return);

$return = filter_var($str, FILTER_SANITIZE_STRING);
assertType('string|false', $return);

$return = filter_var($str, FILTER_VALIDATE_INT);
assertType('int|false', $return);

$str2 = '';
$return = filter_var($str2, FILTER_DEFAULT);
assertType('string|false', $return);

$return = filter_var($str2, FILTER_VALIDATE_URL);
assertType('string|false', $return);

$str2 = 'foo';
$return = filter_var($str2, FILTER_VALIDATE_INT);
assertType('int|false', $return);
}
}