Error on unserialize() without allowed_classes option

Author: derrabus

README.md

@@ -31,6 +31,7 @@
 | `strictArrayFilter`                    | Require `array_filter()` to have a callback parameter to avoid loose comparison semantics.              |
 | `illegalConstructorMethodCall`         | Disallow calling `__construct()` on an existing object or as a static call outside of parent constructor. |
 | `closureUsesThis`                      | Closure should use `$this` directly instead of using `$this` variable indirectly.                       |
+| `unserializeAllowedClasses`            | Calls to `unserialize()` must include the `allowed_classes` option.                                     |
 
 Additional rules are coming in subsequent releases!
 
@@ -74,6 +75,7 @@ parameters:
 		disallowedShortTernary: false
 		overwriteVariablesWithLoop: false
 		closureUsesThis: false
+		unserializeAllowedClasses: false
 		matchingInheritedMethodNames: false
 		numericOperandsInArithmeticOperators: false
 		strictFunctionCalls: false

rules.neon

@@ -26,6 +26,7 @@ parameters:
 		disallowedShortTernary: %strictRules.allRules%
 		overwriteVariablesWithLoop: %strictRules.allRules%
 		closureUsesThis: %strictRules.allRules%
+		unserializeAllowedClasses: %strictRules.allRules%
 		matchingInheritedMethodNames: %strictRules.allRules%
 		numericOperandsInArithmeticOperators: %strictRules.allRules%
 		strictFunctionCalls: %strictRules.allRules%
@@ -49,6 +50,7 @@ parametersSchema:
 		disallowedShortTernary: anyOf(bool(), arrayOf(bool()))
 		overwriteVariablesWithLoop: anyOf(bool(), arrayOf(bool()))
 		closureUsesThis: anyOf(bool(), arrayOf(bool()))
+		unserializeAllowedClasses: anyOf(bool(), arrayOf(bool()))
 		matchingInheritedMethodNames: anyOf(bool(), arrayOf(bool()))
 		numericOperandsInArithmeticOperators: anyOf(bool(), arrayOf(bool()))
 		strictFunctionCalls: anyOf(bool(), arrayOf(bool()))
@@ -98,6 +100,8 @@ conditionalTags:
 		phpstan.rules.rule: %strictRules.strictArrayFilter%
 	PHPStan\Rules\Functions\ClosureUsesThisRule:
 		phpstan.rules.rule: %strictRules.closureUsesThis%
+	PHPStan\Rules\Functions\UnserializeAllowedClassesRule:
+		phpstan.rules.rule: %strictRules.unserializeAllowedClasses%
 	PHPStan\Rules\Methods\WrongCaseOfInheritedMethodRule:
 		phpstan.rules.rule: %strictRules.matchingInheritedMethodNames%
 	PHPStan\Rules\Operators\OperandInArithmeticPostDecrementRule:
@@ -229,6 +233,9 @@ services:
 	-
 		class: PHPStan\Rules\Functions\ClosureUsesThisRule
 
+	-
+		class: PHPStan\Rules\Functions\UnserializeAllowedClassesRule
+
 	-
 		class: PHPStan\Rules\Methods\WrongCaseOfInheritedMethodRule
 

src/Rules/Functions/UnserializeAllowedClassesRule.php

@@ -0,0 +1,77 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Functions;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\FuncCall;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+use function count;
+use function in_array;
+use function strtolower;
+
+/** @implements Rule<FuncCall> */
+class UnserializeAllowedClassesRule implements Rule
+{
+
+	public function getNodeType(): string
+	{
+		return FuncCall::class;
+	}
+
+	public function processNode(Node $node, Scope $scope): array
+	{
+		if (!$node->name instanceof Node\Name) {
+			return [];
+		}
+
+		if (!in_array(strtolower((string) $node->name), ['unserialize'], true)) {
+			return [];
+		}
+
+		$args = $node->getArgs();
+
+		if (count($args) < 2) {
+			return [
+				RuleErrorBuilder::message(
+					'unserialize() called without the $options argument. Always pass '
+					. "['allowed_classes' => false] or an explicit list of safe classes. "
+					. "If you truly need to allow all classes, pass ['allowed_classes' => true] "
+					. 'to make that decision explicit.',
+				)
+					->identifier('unserialize.allowedClasses')
+					->build(),
+			];
+		}
+
+		$optionsArg = $args[1]->value;
+
+		if (!$optionsArg instanceof Node\Expr\Array_) {
+			return [];
+		}
+
+		foreach ($optionsArg->items as $item) {
+			$key = $item->key;
+			if (!$key instanceof Node\Scalar\String_) {
+				continue;
+			}
+
+			if ($key->value === 'allowed_classes') {
+				return [];
+			}
+		}
+
+		return [
+			RuleErrorBuilder::message(
+				"unserialize() called without the 'allowed_classes' key in \$options. "
+				. "Always pass ['allowed_classes' => false] or an explicit list of safe classes. "
+				. "If you truly need to allow all classes, pass ['allowed_classes' => true] "
+				. 'to make that decision explicit.',
+			)
+				->identifier('unserialize.allowedClasses')
+				->build(),
+		];
+	}
+
+}

tests/Rules/Functions/UnserializeAllowedClassesRuleTest.php

@@ -0,0 +1,39 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Functions;
+
+use PHPStan\Testing\RuleTestCase;
+
+/** @extends RuleTestCase<UnserializeAllowedClassesRule> */
+class UnserializeAllowedClassesRuleTest extends RuleTestCase
+{
+
+	protected function getRule(): UnserializeAllowedClassesRule
+	{
+		return new UnserializeAllowedClassesRule();
+	}
+
+	public function testViolations(): void
+	{
+		$this->analyse([__DIR__ . '/data/unserialize-usages.php'], [
+			[
+				"unserialize() called without the \$options argument. Always pass ['allowed_classes' => false] or an explicit list of safe classes. If you truly need to allow all classes, pass ['allowed_classes' => true] to make that decision explicit.",
+				10,
+			],
+			[
+				"unserialize() called without the 'allowed_classes' key in \$options. Always pass ['allowed_classes' => false] or an explicit list of safe classes. If you truly need to allow all classes, pass ['allowed_classes' => true] to make that decision explicit.",
+				15,
+			],
+			[
+				"unserialize() called without the 'allowed_classes' key in \$options. Always pass ['allowed_classes' => false] or an explicit list of safe classes. If you truly need to allow all classes, pass ['allowed_classes' => true] to make that decision explicit.",
+				20,
+			],
+		]);
+	}
+
+	public function testNoErrors(): void
+	{
+		$this->analyse([__DIR__ . '/data/unserialize-good-usages.php'], []);
+	}
+
+}

tests/Rules/Functions/data/unserialize-good-usages.php

@@ -0,0 +1,32 @@
+<?php declare(strict_types = 1);
+
+namespace UnserializeAllowedClasses;
+
+final class UnserializeUsagesOk
+{
+
+	/** @return array<string, mixed> */
+	public function allowedClassesFalse(string $data): array
+	{
+		/** @var array<string, mixed> $result */
+		$result = unserialize($data, ['allowed_classes' => false]);
+
+		return $result;
+	}
+
+	public function allowedClassesExplicitTrue(string $data): mixed
+	{
+		return unserialize($data, ['allowed_classes' => true]);
+	}
+
+	public function allowedClassesList(string $data): mixed
+	{
+		return unserialize($data, ['allowed_classes' => [\stdClass::class]]);
+	}
+
+	public function dynamicOptions(string $data, mixed $options): mixed
+	{
+		return unserialize($data, $options);
+	}
+
+}

tests/Rules/Functions/data/unserialize-usages.php

@@ -0,0 +1,51 @@
+<?php declare(strict_types = 1);
+
+namespace UnserializeAllowedClasses;
+
+final class UnserializeUsages
+{
+
+	public function noOptions(string $data): mixed
+	{
+		return unserialize($data);
+	}
+
+	public function emptyOptions(string $data): mixed
+	{
+		return unserialize($data, []);
+	}
+
+	public function optionsWithoutAllowedClasses(string $data): mixed
+	{
+		return unserialize($data, ['some_other_key' => true]);
+	}
+
+	/** @return array<string, mixed> */
+	public function allowedClassesFalse(string $data): array
+	{
+		/** @var array<string, mixed> $result */
+		$result = unserialize($data, ['allowed_classes' => false]);
+
+		return $result;
+	}
+
+	/** @return array<string, mixed> */
+	public function allowedClassesExplicitTrue(string $data): array
+	{
+		/** @var array<string, mixed> $result */
+		$result = unserialize($data, ['allowed_classes' => true]);
+
+		return $result;
+	}
+
+	public function allowedClassesList(string $data): mixed
+	{
+		return unserialize($data, ['allowed_classes' => [\stdClass::class]]);
+	}
+
+	public function dynamicOptions(string $data, mixed $options): mixed
+	{
+		return unserialize($data, $options);
+	}
+
+}