Skip to content

Commit ce07549

Browse files
committed
Restrict CallableStringifier to closures to prevent data leakage
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.
1 parent 6d38b05 commit ce07549

6 files changed

Lines changed: 137 additions & 75 deletions

File tree

README.md

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -103,23 +103,8 @@ echo stringify(new class { public int $property = 42; }) . PHP_EOL;
103103
echo stringify(new class extends WithProperties { }) . PHP_EOL;
104104
// `WithProperties@anonymous { +$publicProperty=true #$protectedProperty=42 }`
105105

106-
echo stringify('chr') . PHP_EOL;
107-
// `chr(int $codepoint): string`
108-
109-
echo stringify([new WithMethods(), 'publicMethod']) . PHP_EOL;
110-
// `WithMethods->publicMethod(Iterator&Countable $parameter): ?static`
111-
112-
echo stringify('WithMethods::publicStaticMethod') . PHP_EOL;
113-
// `WithMethods::publicStaticMethod(int|float $parameter): void`
114-
115-
echo stringify(['WithMethods', 'publicStaticMethod']) . PHP_EOL;
116-
// `WithMethods::publicStaticMethod(int|float $parameter): void`
117-
118-
echo stringify(new WithInvoke()) . PHP_EOL;
119-
// `WithInvoke->__invoke(int $parameter = 0): never`
120-
121-
echo stringify(static fn(int $foo): string => '') . PHP_EOL;
122-
// `function (int $foo): string`
106+
echo stringify(fn(int $foo): string => '') . PHP_EOL;
107+
// `Closure { fn(int $foo): string }`
123108

124109
echo stringify(new DateTime()) . PHP_EOL;
125110
// `DateTime { 2023-04-21T11:29:03+00:00 }`

phpcs.xml.dist

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,4 +32,8 @@
3232
<rule ref="Generic.PHP.CharacterBeforePHPOpeningTag.Found">
3333
<exclude-pattern>tests/integration/</exclude-pattern>
3434
</rule>
35+
<rule ref="SlevomatCodingStandard.Functions.StaticClosure.ClosureNotStatic">
36+
<exclude-pattern>tests/integration</exclude-pattern>
37+
<exclude-pattern>tests/unit/Stringifiers/CallableStringifierTest.php</exclude-pattern>
38+
</rule>
3539
</ruleset>

src/Stringifiers/CallableStringifier.php

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -44,43 +44,44 @@ final class CallableStringifier implements Stringifier
4444
public function __construct(
4545
private readonly Stringifier $stringifier,
4646
private readonly Quoter $quoter,
47+
private readonly bool $closureOnly = true,
4748
) {
4849
}
4950

5051
public function stringify(mixed $raw, int $depth): string|null
5152
{
52-
if (!is_callable($raw)) {
53-
return null;
54-
}
55-
5653
if ($raw instanceof Closure) {
5754
return $this->buildFunction(new ReflectionFunction($raw), $depth);
5855
}
5956

57+
if ($this->closureOnly || !is_callable($raw)) {
58+
return null;
59+
}
60+
6061
if (is_object($raw)) {
6162
return $this->buildMethod(new ReflectionMethod($raw, '__invoke'), $raw, $depth);
6263
}
6364

64-
if (is_array($raw) && is_object($raw[0]) && is_string($raw[1])) {
65+
if (is_array($raw) && isset($raw[0], $raw[1]) && is_object($raw[0]) && is_string($raw[1])) {
6566
return $this->buildMethod(new ReflectionMethod($raw[0], $raw[1]), $raw[0], $depth);
6667
}
6768

68-
if (is_array($raw) && is_string($raw[0]) && is_string($raw[1])) {
69+
if (is_array($raw) && isset($raw[0], $raw[1]) && is_string($raw[0]) && is_string($raw[1])) {
6970
return $this->buildStaticMethod(new ReflectionMethod($raw[0], $raw[1]), $depth);
7071
}
7172

72-
if (is_string($raw) && str_contains($raw, ':')) {
73+
if (!is_string($raw)) {
74+
return null;
75+
}
76+
77+
if (str_contains($raw, ':')) {
7378
/** @var class-string $class */
7479
$class = (string) strstr($raw, ':', true);
7580
$method = substr((string) strrchr($raw, ':'), 1);
7681

7782
return $this->buildStaticMethod(new ReflectionMethod($class, $method), $depth);
7883
}
7984

80-
if (!is_string($raw)) {
81-
return null;
82-
}
83-
8485
return $this->buildFunction(new ReflectionFunction($raw), $depth);
8586
}
8687

@@ -107,8 +108,7 @@ private function buildStaticMethod(ReflectionMethod $reflection, int $depth): st
107108

108109
private function buildSignature(ReflectionFunctionAbstract $function, int $depth): string
109110
{
110-
$signature = $function->isClosure() ? 'function ' : $function->getName();
111-
$signature .= sprintf(
111+
$signature = sprintf(
112112
'(%s)',
113113
implode(
114114
', ',
@@ -138,7 +138,11 @@ private function buildSignature(ReflectionFunctionAbstract $function, int $depth
138138
$signature .= ': ' . $this->buildType($returnType, $depth);
139139
}
140140

141-
return $signature;
141+
if ($function->isClosure()) {
142+
return sprintf('Closure { %sfn%s }', $function->isStatic() ? 'static ' : '', $signature);
143+
}
144+
145+
return $function->getName() . $signature;
142146
}
143147

144148
private function buildParameter(ReflectionParameter $reflectionParameter, int $depth): string

src/Stringifiers/CompositeStringifier.php

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,10 @@ public static function createDefault(): self
6060
self::MAXIMUM_NUMBER_OF_PROPERTIES,
6161
),
6262
);
63-
$stringifier->prependStringifier($callableStringifier = new CallableStringifier($stringifier, $quoter));
64-
$stringifier->prependStringifier(new FiberObjectStringifier($callableStringifier, $quoter));
63+
$stringifier->prependStringifier(new CallableStringifier($stringifier, $quoter));
64+
$stringifier->prependStringifier(
65+
new FiberObjectStringifier(new CallableStringifier($stringifier, $quoter, closureOnly: false), $quoter),
66+
);
6567
$stringifier->prependStringifier(new EnumerationStringifier($quoter));
6668
$stringifier->prependStringifier(new ObjectWithDebugInfoStringifier($arrayStringifier, $quoter));
6769
$stringifier->prependStringifier(new ArrayObjectStringifier($arrayStringifier, $quoter));

tests/integration/stringify-callable.phpt

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,25 +5,15 @@ declare(strict_types=1);
55

66
require 'vendor/autoload.php';
77

8-
$variable = new WithInvoke();
8+
$variable = true;
99

1010
outputMultiple(
11-
'chr',
12-
$variable,
13-
[new WithMethods(), 'publicMethod'],
14-
'WithMethods::publicStaticMethod',
15-
['WithMethods', 'publicStaticMethod'],
16-
static fn(int $foo): bool => (bool) $foo,
11+
fn(int $foo): bool => (bool) $foo,
1712
static function (int $foo) use ($variable): string {
1813
return $variable::class;
1914
},
2015
);
2116
?>
2217
--EXPECT--
23-
`chr(int $codepoint): string`
24-
`WithInvoke->__invoke(int $parameter = 0): never`
25-
`WithMethods->publicMethod(Iterator&Countable $parameter): ?static`
26-
`WithMethods::publicStaticMethod(int|float $parameter): void`
27-
`WithMethods::publicStaticMethod(int|float $parameter): void`
28-
`function (int $foo): bool`
29-
`function (int $foo) use ($variable): string`
18+
`Closure { fn(int $foo): bool }`
19+
`Closure { static fn(int $foo) use ($variable): string }`

tests/unit/Stringifiers/CallableStringifierTest.php

Lines changed: 105 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@
1717
use PHPUnit\Framework\Attributes\DataProvider;
1818
use PHPUnit\Framework\Attributes\Test;
1919
use PHPUnit\Framework\TestCase;
20+
use Respect\Stringifier\Helpers\ObjectHelper;
2021
use Respect\Stringifier\Stringifiers\CallableStringifier;
2122
use Respect\Stringifier\Test\Double\FakeQuoter;
2223
use Respect\Stringifier\Test\Double\FakeStringifier;
2324

2425
use function array_sum;
2526
use function sprintf;
2627

28+
#[CoversClass(ObjectHelper::class)]
2729
#[CoversClass(CallableStringifier::class)]
2830
final class CallableStringifierTest extends TestCase
2931
{
@@ -32,14 +34,14 @@ final class CallableStringifierTest extends TestCase
3234
#[Test]
3335
public function itShouldNotStringifyWhenRawValueIsNotCallable(): void
3436
{
35-
$sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter());
37+
$sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter(), closureOnly: false);
3638

3739
self::assertNull($sut->stringify(1, self::DEPTH));
3840
}
3941

4042
#[Test]
41-
#[DataProvider('callableRawValuesProvider')]
42-
public function itShouldStringifyWhenRawValueIsCallable(callable $raw, string $expectedWithoutQuotes): void
43+
#[DataProvider('closureRawValuesProvider')]
44+
public function itShouldStringifyWhenRawValueIsClosure(callable $raw, string $expectedWithoutQuotes): void
4345
{
4446
$quoter = new FakeQuoter();
4547

@@ -51,6 +53,31 @@ public function itShouldStringifyWhenRawValueIsCallable(callable $raw, string $e
5153
self::assertEquals($expected, $actual);
5254
}
5355

56+
#[Test]
57+
#[DataProvider('nonClosureCallableRawValuesProvider')]
58+
public function itShouldNotStringifyNonClosureCallableByDefault(callable $raw, string $useless): void
59+
{
60+
$sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter());
61+
62+
self::assertNull($sut->stringify($raw, self::DEPTH));
63+
}
64+
65+
#[Test]
66+
#[DataProvider('nonClosureCallableRawValuesProvider')]
67+
public function itShouldStringifyNonClosureCallableWhenClosureOnlyIsFalse(
68+
callable $raw,
69+
string $expectedWithoutQuotes,
70+
): void {
71+
$quoter = new FakeQuoter();
72+
73+
$sut = new CallableStringifier(new FakeStringifier(), $quoter, closureOnly: false);
74+
75+
$actual = $sut->stringify($raw, self::DEPTH);
76+
$expected = $quoter->quote($expectedWithoutQuotes, self::DEPTH);
77+
78+
self::assertEquals($expected, $actual);
79+
}
80+
5481
#[Test]
5582
public function itShouldStringifyWhenRawValueIsCallableWithDefaultValues(): void
5683
{
@@ -63,7 +90,7 @@ public function itShouldStringifyWhenRawValueIsCallableWithDefaultValues(): void
6390

6491
$actual = $sut->stringify($raw, self::DEPTH);
6592
$expected = $quoter->quote(
66-
sprintf('function (int $value = %s): int', $stringifier->stringify(1, self::DEPTH + 1)),
93+
sprintf('Closure { static fn(int $value = %s): int }', $stringifier->stringify(1, self::DEPTH + 1)),
6794
self::DEPTH,
6895
);
6996

@@ -77,7 +104,7 @@ public function itShouldStringifyWhenRawValueIsCallableThatDoesNotHaveAnAccessib
77104

78105
$quoter = new FakeQuoter();
79106

80-
$sut = new CallableStringifier(new FakeStringifier(), $quoter);
107+
$sut = new CallableStringifier(new FakeStringifier(), $quoter, closureOnly: false);
81108

82109
$actual = $sut->stringify($raw, self::DEPTH);
83110
$expected = $quoter->quote(
@@ -88,40 +115,87 @@ public function itShouldStringifyWhenRawValueIsCallableThatDoesNotHaveAnAccessib
88115
self::assertEquals($expected, $actual);
89116
}
90117

91-
/** @return array<int, array{0: callable, 1: string}> */
92-
public static function callableRawValuesProvider(): array
118+
/** @return array<string, array{0: callable, 1: string}> */
119+
public static function closureRawValuesProvider(): array
93120
{
94121
$var1 = 1;
95122
$var2 = 2;
96123

97124
return [
98-
[static fn() => 1, 'function ()'],
99-
[static fn(): int => 1, 'function (): int'],
100-
[static fn(float $value): int => (int) $value, 'function (float $value): int'],
101-
[static fn(float &$value): int => (int) $value, 'function (float &$value): int'],
102-
// phpcs:ignore SlevomatCodingStandard.TypeHints.DNFTypeHintFormat
103-
[static fn(?float $value): int => (int) $value, 'function (?float $value): int'],
104-
[static fn(int $value = self::DEPTH): int => $value, 'function (int $value = self::DEPTH): int'],
105-
[static fn(int|float $value): int => (int) $value, 'function (int|float $value): int'],
106-
[static fn(Countable&Iterator $value): int => $value->count(), 'function (Countable&Iterator $value): int'],
107-
[static fn(int ...$value): int => array_sum($value), 'function (int ...$value): int'],
108-
[
125+
'static closure without parameters' => [
126+
static fn() => 1,
127+
'Closure { static fn() }',
128+
],
129+
'non-static closure without parameters' => [
130+
fn() => 1,
131+
'Closure { fn() }',
132+
],
133+
'static closure with return type' => [
134+
static fn(): int => 1,
135+
'Closure { static fn(): int }',
136+
],
137+
'non-static closure with return type' => [
138+
fn(): int => 1,
139+
'Closure { fn(): int }',
140+
],
141+
'static closure with typed parameter' => [
142+
static fn(float $value): int => (int) $value,
143+
'Closure { static fn(float $value): int }',
144+
],
145+
'static closure with reference parameter' => [
146+
static fn(float &$value): int => (int) $value,
147+
'Closure { static fn(float &$value): int }',
148+
],
149+
'static closure with nullable parameter' => [
150+
static fn(float|null $value): int => (int) $value,
151+
'Closure { static fn(?float $value): int }',
152+
],
153+
'static closure with constant default value' => [
154+
static fn(int $value = self::DEPTH): int => $value,
155+
'Closure { static fn(int $value = self::DEPTH): int }',
156+
],
157+
'static closure with union type parameter' => [
158+
static fn(int|float $value): int => (int) $value,
159+
'Closure { static fn(int|float $value): int }',
160+
],
161+
'static closure with intersection type parameter' => [
162+
static fn(Countable&Iterator $value): int => $value->count(),
163+
'Closure { static fn(Countable&Iterator $value): int }',
164+
],
165+
'static closure with variadic parameter' => [
166+
static fn(int ...$value): int => array_sum($value),
167+
'Closure { static fn(int ...$value): int }',
168+
],
169+
'static closure with multiple parameters' => [
109170
static fn(float $value1, float $value2): float => $value1 + $value2,
110-
'function (float $value1, float $value2): float',
171+
'Closure { static fn(float $value1, float $value2): float }',
111172
],
112-
[
173+
'static closure with single use variable' => [
113174
static function (int $value) use ($var1): int {
114175
return $value + $var1;
115176
},
116-
'function (int $value) use ($var1): int',
177+
'Closure { static fn(int $value) use ($var1): int }',
117178
],
118-
[
179+
'static closure with multiple use variables' => [
119180
static function (int $value) use ($var1, $var2): int {
120181
return $value + $var1 + $var2;
121182
},
122-
'function (int $value) use ($var1, $var2): int',
183+
'Closure { static fn(int $value) use ($var1, $var2): int }',
123184
],
124-
[
185+
'non-static closure with use variable' => [
186+
function (int $value) use ($var1): int {
187+
return $value + $var1;
188+
},
189+
'Closure { fn(int $value) use ($var1): int }',
190+
],
191+
];
192+
}
193+
194+
/** @return array<string, array{0: callable, 1: string}> */
195+
public static function nonClosureCallableRawValuesProvider(): array
196+
{
197+
return [
198+
'invokable object' => [
125199
new class {
126200
public function __invoke(int $parameter): never
127201
{
@@ -130,19 +204,22 @@ public function __invoke(int $parameter): never
130204
},
131205
'class->__invoke(int $parameter): never',
132206
],
133-
[
207+
'object method as array' => [
134208
[new DateTime(), 'format'],
135209
'DateTime->format(string $format)',
136210
],
137-
[
211+
'static method as array' => [
138212
['DateTime', 'createFromImmutable'],
139213
'DateTime::createFromImmutable(DateTimeImmutable $object)',
140214
],
141-
[
215+
'static method as string' => [
142216
'DateTimeImmutable::getLastErrors',
143217
'DateTimeImmutable::getLastErrors()',
144218
],
145-
['chr', 'chr(int $codepoint): string'],
219+
'function name as string' => [
220+
'chr',
221+
'chr(int $codepoint): string',
222+
],
146223
];
147224
}
148225
}

0 commit comments

Comments
 (0)