From 676f823920b4872e2ba6d5fe6174d6e55b1ab85d Mon Sep 17 00:00:00 2001 From: Henrique Moody Date: Mon, 19 Jan 2026 00:46:23 +0100 Subject: [PATCH] Restrict `CallableStringifier` to closures to prevent data leakage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `CallableStringifier` is undeniably useful, but its convenience comes at a security risk. By allowing strings and arrays to be interpreted as callables by default, we risk exposing sensitive data—like passwords or hostnames—hidden within those structures. Furthermore, it is often frustrating to have arbitrary strings automatically turned into callable representations. To fix this, I am moving to a "secure-by-default" stance. The stringifier now defaults to closure-only mode. Closures are significantly less likely to expose sensitive data in their contracts, making this a much safer baseline. I have made an exception for Fibers. Since a Fiber explicitly contains a callable, we know the usage is intentional, and being explicit about the underlying callable is important for debugging clarity. If someone still needs the original behavior, they can have it, but they must be intentional. They can restore the previous functionality by manually defining the `$closureOnly` parameter as `false` in the constructor. --- README.md | 19 +-- phpcs.xml.dist | 4 + src/Stringifiers/CallableStringifier.php | 32 +++-- src/Stringifiers/CompositeStringifier.php | 6 +- tests/integration/stringify-callable.phpt | 18 +-- .../Stringifiers/CallableStringifierTest.php | 131 ++++++++++++++---- 6 files changed, 135 insertions(+), 75 deletions(-) diff --git a/README.md b/README.md index f455d23..7c78457 100644 --- a/README.md +++ b/README.md @@ -103,23 +103,8 @@ echo stringify(new class { public int $property = 42; }) . PHP_EOL; echo stringify(new class extends WithProperties { }) . PHP_EOL; // `WithProperties@anonymous { +$publicProperty=true #$protectedProperty=42 }` -echo stringify('chr') . PHP_EOL; -// `chr(int $codepoint): string` - -echo stringify([new WithMethods(), 'publicMethod']) . PHP_EOL; -// `WithMethods->publicMethod(Iterator&Countable $parameter): ?static` - -echo stringify('WithMethods::publicStaticMethod') . PHP_EOL; -// `WithMethods::publicStaticMethod(int|float $parameter): void` - -echo stringify(['WithMethods', 'publicStaticMethod']) . PHP_EOL; -// `WithMethods::publicStaticMethod(int|float $parameter): void` - -echo stringify(new WithInvoke()) . PHP_EOL; -// `WithInvoke->__invoke(int $parameter = 0): never` - -echo stringify(static fn(int $foo): string => '') . PHP_EOL; -// `function (int $foo): string` +echo stringify(fn(int $foo): string => '') . PHP_EOL; +// `Closure { fn(int $foo): string }` echo stringify(new DateTime()) . PHP_EOL; // `DateTime { 2023-04-21T11:29:03+00:00 }` diff --git a/phpcs.xml.dist b/phpcs.xml.dist index 1e228c6..0ead2f2 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -32,4 +32,8 @@ tests/integration/ + + tests/integration + tests/unit/Stringifiers/CallableStringifierTest.php + diff --git a/src/Stringifiers/CallableStringifier.php b/src/Stringifiers/CallableStringifier.php index 6158199..2725abc 100644 --- a/src/Stringifiers/CallableStringifier.php +++ b/src/Stringifiers/CallableStringifier.php @@ -44,32 +44,37 @@ final class CallableStringifier implements Stringifier public function __construct( private readonly Stringifier $stringifier, private readonly Quoter $quoter, + private readonly bool $closureOnly = true, ) { } public function stringify(mixed $raw, int $depth): string|null { - if (!is_callable($raw)) { - return null; - } - if ($raw instanceof Closure) { return $this->buildFunction(new ReflectionFunction($raw), $depth); } + if ($this->closureOnly || !is_callable($raw)) { + return null; + } + if (is_object($raw)) { return $this->buildMethod(new ReflectionMethod($raw, '__invoke'), $raw, $depth); } - if (is_array($raw) && is_object($raw[0]) && is_string($raw[1])) { + if (is_array($raw) && isset($raw[0], $raw[1]) && is_object($raw[0]) && is_string($raw[1])) { return $this->buildMethod(new ReflectionMethod($raw[0], $raw[1]), $raw[0], $depth); } - if (is_array($raw) && is_string($raw[0]) && is_string($raw[1])) { + if (is_array($raw) && isset($raw[0], $raw[1]) && is_string($raw[0]) && is_string($raw[1])) { return $this->buildStaticMethod(new ReflectionMethod($raw[0], $raw[1]), $depth); } - if (is_string($raw) && str_contains($raw, ':')) { + if (!is_string($raw)) { + return null; + } + + if (str_contains($raw, ':')) { /** @var class-string $class */ $class = (string) strstr($raw, ':', true); $method = substr((string) strrchr($raw, ':'), 1); @@ -77,10 +82,6 @@ public function stringify(mixed $raw, int $depth): string|null return $this->buildStaticMethod(new ReflectionMethod($class, $method), $depth); } - if (!is_string($raw)) { - return null; - } - return $this->buildFunction(new ReflectionFunction($raw), $depth); } @@ -107,8 +108,7 @@ private function buildStaticMethod(ReflectionMethod $reflection, int $depth): st private function buildSignature(ReflectionFunctionAbstract $function, int $depth): string { - $signature = $function->isClosure() ? 'function ' : $function->getName(); - $signature .= sprintf( + $signature = sprintf( '(%s)', implode( ', ', @@ -138,7 +138,11 @@ private function buildSignature(ReflectionFunctionAbstract $function, int $depth $signature .= ': ' . $this->buildType($returnType, $depth); } - return $signature; + if ($function->isClosure()) { + return sprintf('Closure { %sfn%s }', $function->isStatic() ? 'static ' : '', $signature); + } + + return $function->getName() . $signature; } private function buildParameter(ReflectionParameter $reflectionParameter, int $depth): string diff --git a/src/Stringifiers/CompositeStringifier.php b/src/Stringifiers/CompositeStringifier.php index 21926f9..b659073 100644 --- a/src/Stringifiers/CompositeStringifier.php +++ b/src/Stringifiers/CompositeStringifier.php @@ -60,8 +60,10 @@ public static function createDefault(): self self::MAXIMUM_NUMBER_OF_PROPERTIES, ), ); - $stringifier->prependStringifier($callableStringifier = new CallableStringifier($stringifier, $quoter)); - $stringifier->prependStringifier(new FiberObjectStringifier($callableStringifier, $quoter)); + $stringifier->prependStringifier(new CallableStringifier($stringifier, $quoter)); + $stringifier->prependStringifier( + new FiberObjectStringifier(new CallableStringifier($stringifier, $quoter, closureOnly: false), $quoter), + ); $stringifier->prependStringifier(new EnumerationStringifier($quoter)); $stringifier->prependStringifier(new ObjectWithDebugInfoStringifier($arrayStringifier, $quoter)); $stringifier->prependStringifier(new ArrayObjectStringifier($arrayStringifier, $quoter)); diff --git a/tests/integration/stringify-callable.phpt b/tests/integration/stringify-callable.phpt index 4ad861d..a1e8905 100644 --- a/tests/integration/stringify-callable.phpt +++ b/tests/integration/stringify-callable.phpt @@ -5,25 +5,15 @@ declare(strict_types=1); require 'vendor/autoload.php'; -$variable = new WithInvoke(); +$variable = true; outputMultiple( - 'chr', - $variable, - [new WithMethods(), 'publicMethod'], - 'WithMethods::publicStaticMethod', - ['WithMethods', 'publicStaticMethod'], - static fn(int $foo): bool => (bool) $foo, + fn(int $foo): bool => (bool) $foo, static function (int $foo) use ($variable): string { return $variable::class; }, ); ?> --EXPECT-- -`chr(int $codepoint): string` -`WithInvoke->__invoke(int $parameter = 0): never` -`WithMethods->publicMethod(Iterator&Countable $parameter): ?static` -`WithMethods::publicStaticMethod(int|float $parameter): void` -`WithMethods::publicStaticMethod(int|float $parameter): void` -`function (int $foo): bool` -`function (int $foo) use ($variable): string` \ No newline at end of file +`Closure { fn(int $foo): bool }` +`Closure { static fn(int $foo) use ($variable): string }` diff --git a/tests/unit/Stringifiers/CallableStringifierTest.php b/tests/unit/Stringifiers/CallableStringifierTest.php index fb298bb..7c607ff 100644 --- a/tests/unit/Stringifiers/CallableStringifierTest.php +++ b/tests/unit/Stringifiers/CallableStringifierTest.php @@ -32,14 +32,14 @@ final class CallableStringifierTest extends TestCase #[Test] public function itShouldNotStringifyWhenRawValueIsNotCallable(): void { - $sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter()); + $sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter(), closureOnly: false); self::assertNull($sut->stringify(1, self::DEPTH)); } #[Test] - #[DataProvider('callableRawValuesProvider')] - public function itShouldStringifyWhenRawValueIsCallable(callable $raw, string $expectedWithoutQuotes): void + #[DataProvider('closureRawValuesProvider')] + public function itShouldStringifyWhenRawValueIsClosure(callable $raw, string $expectedWithoutQuotes): void { $quoter = new FakeQuoter(); @@ -51,6 +51,31 @@ public function itShouldStringifyWhenRawValueIsCallable(callable $raw, string $e self::assertEquals($expected, $actual); } + #[Test] + #[DataProvider('nonClosureCallableRawValuesProvider')] + public function itShouldNotStringifyNonClosureCallableByDefault(callable $raw, string $useless): void + { + $sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter()); + + self::assertNull($sut->stringify($raw, self::DEPTH)); + } + + #[Test] + #[DataProvider('nonClosureCallableRawValuesProvider')] + public function itShouldStringifyNonClosureCallableWhenClosureOnlyIsFalse( + callable $raw, + string $expectedWithoutQuotes, + ): void { + $quoter = new FakeQuoter(); + + $sut = new CallableStringifier(new FakeStringifier(), $quoter, closureOnly: false); + + $actual = $sut->stringify($raw, self::DEPTH); + $expected = $quoter->quote($expectedWithoutQuotes, self::DEPTH); + + self::assertEquals($expected, $actual); + } + #[Test] public function itShouldStringifyWhenRawValueIsCallableWithDefaultValues(): void { @@ -63,7 +88,7 @@ public function itShouldStringifyWhenRawValueIsCallableWithDefaultValues(): void $actual = $sut->stringify($raw, self::DEPTH); $expected = $quoter->quote( - sprintf('function (int $value = %s): int', $stringifier->stringify(1, self::DEPTH + 1)), + sprintf('Closure { static fn(int $value = %s): int }', $stringifier->stringify(1, self::DEPTH + 1)), self::DEPTH, ); @@ -77,7 +102,7 @@ public function itShouldStringifyWhenRawValueIsCallableThatDoesNotHaveAnAccessib $quoter = new FakeQuoter(); - $sut = new CallableStringifier(new FakeStringifier(), $quoter); + $sut = new CallableStringifier(new FakeStringifier(), $quoter, closureOnly: false); $actual = $sut->stringify($raw, self::DEPTH); $expected = $quoter->quote( @@ -88,40 +113,87 @@ public function itShouldStringifyWhenRawValueIsCallableThatDoesNotHaveAnAccessib self::assertEquals($expected, $actual); } - /** @return array */ - public static function callableRawValuesProvider(): array + /** @return array */ + public static function closureRawValuesProvider(): array { $var1 = 1; $var2 = 2; return [ - [static fn() => 1, 'function ()'], - [static fn(): int => 1, 'function (): int'], - [static fn(float $value): int => (int) $value, 'function (float $value): int'], - [static fn(float &$value): int => (int) $value, 'function (float &$value): int'], - // phpcs:ignore SlevomatCodingStandard.TypeHints.DNFTypeHintFormat - [static fn(?float $value): int => (int) $value, 'function (?float $value): int'], - [static fn(int $value = self::DEPTH): int => $value, 'function (int $value = self::DEPTH): int'], - [static fn(int|float $value): int => (int) $value, 'function (int|float $value): int'], - [static fn(Countable&Iterator $value): int => $value->count(), 'function (Countable&Iterator $value): int'], - [static fn(int ...$value): int => array_sum($value), 'function (int ...$value): int'], - [ + 'static closure without parameters' => [ + static fn() => 1, + 'Closure { static fn() }', + ], + 'non-static closure without parameters' => [ + fn() => 1, + 'Closure { fn() }', + ], + 'static closure with return type' => [ + static fn(): int => 1, + 'Closure { static fn(): int }', + ], + 'non-static closure with return type' => [ + fn(): int => 1, + 'Closure { fn(): int }', + ], + 'static closure with typed parameter' => [ + static fn(float $value): int => (int) $value, + 'Closure { static fn(float $value): int }', + ], + 'static closure with reference parameter' => [ + static fn(float &$value): int => (int) $value, + 'Closure { static fn(float &$value): int }', + ], + 'static closure with nullable parameter' => [ + static fn(float|null $value): int => (int) $value, + 'Closure { static fn(?float $value): int }', + ], + 'static closure with constant default value' => [ + static fn(int $value = self::DEPTH): int => $value, + 'Closure { static fn(int $value = self::DEPTH): int }', + ], + 'static closure with union type parameter' => [ + static fn(int|float $value): int => (int) $value, + 'Closure { static fn(int|float $value): int }', + ], + 'static closure with intersection type parameter' => [ + static fn(Countable&Iterator $value): int => $value->count(), + 'Closure { static fn(Countable&Iterator $value): int }', + ], + 'static closure with variadic parameter' => [ + static fn(int ...$value): int => array_sum($value), + 'Closure { static fn(int ...$value): int }', + ], + 'static closure with multiple parameters' => [ static fn(float $value1, float $value2): float => $value1 + $value2, - 'function (float $value1, float $value2): float', + 'Closure { static fn(float $value1, float $value2): float }', ], - [ + 'static closure with single use variable' => [ static function (int $value) use ($var1): int { return $value + $var1; }, - 'function (int $value) use ($var1): int', + 'Closure { static fn(int $value) use ($var1): int }', ], - [ + 'static closure with multiple use variables' => [ static function (int $value) use ($var1, $var2): int { return $value + $var1 + $var2; }, - 'function (int $value) use ($var1, $var2): int', + 'Closure { static fn(int $value) use ($var1, $var2): int }', ], - [ + 'non-static closure with use variable' => [ + function (int $value) use ($var1): int { + return $value + $var1; + }, + 'Closure { fn(int $value) use ($var1): int }', + ], + ]; + } + + /** @return array */ + public static function nonClosureCallableRawValuesProvider(): array + { + return [ + 'invokable object' => [ new class { public function __invoke(int $parameter): never { @@ -130,19 +202,22 @@ public function __invoke(int $parameter): never }, 'class->__invoke(int $parameter): never', ], - [ + 'object method as array' => [ [new DateTime(), 'format'], 'DateTime->format(string $format)', ], - [ + 'static method as array' => [ ['DateTime', 'createFromImmutable'], 'DateTime::createFromImmutable(DateTimeImmutable $object)', ], - [ + 'static method as string' => [ 'DateTimeImmutable::getLastErrors', 'DateTimeImmutable::getLastErrors()', ], - ['chr', 'chr(int $codepoint): string'], + 'function name as string' => [ + 'chr', + 'chr(int $codepoint): string', + ], ]; } }