Skip to content

Commit ecdcc84

Browse files
henriquemoodyhenriquemoody
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 cea134c commit ecdcc84

File tree

8 files changed

+139
-73
lines changed

8 files changed

+139
-73
lines changed

‎README.md‎

Lines changed: 1 addition & 16 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-
121106
echo stringify(static fn(int $foo): string => '') . PHP_EOL;
122-
// `function (int $foo): string`
107+
// `Closure { static 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
@@ -17,4 +17,8 @@
1717
<rule ref="PSR1.Classes.ClassDeclaration.MissingNamespace">
1818
<exclude-pattern>tests/fixtures/</exclude-pattern>
1919
</rule>
20+
<rule ref="SlevomatCodingStandard.Functions.StaticClosure.ClosureNotStatic">
21+
<exclude-pattern>tests/integration</exclude-pattern>
22+
<exclude-pattern>tests/unit/Stringifiers/CallableStringifierTest.php</exclude-pattern>
23+
</rule>
2024
</ruleset>

‎src/Stringifiers/CallableStringifier.php‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,29 +44,36 @@ 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
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

6465
if (is_array($raw) && is_object($raw[0])) {
65-
return $this->buildMethod(new ReflectionMethod($raw[0], $raw[1]), $raw[0], $depth);
66+
/** @var string $method */
67+
$method = $raw[1];
68+
69+
return $this->buildMethod(new ReflectionMethod($raw[0], $method), $raw[0], $depth);
6670
}
6771

6872
if (is_array($raw) && is_string($raw[0])) {
69-
return $this->buildStaticMethod(new ReflectionMethod($raw[0], $raw[1]), $depth);
73+
/** @var string $method */
74+
$method = $raw[1];
75+
76+
return $this->buildStaticMethod(new ReflectionMethod($raw[0], $method), $depth);
7077
}
7178

7279
/** @var callable-string $raw */
@@ -81,7 +88,7 @@ public function stringify(mixed $raw, int $depth): ?string
8188
return $this->buildFunction(new ReflectionFunction($raw), $depth);
8289
}
8390

84-
public function buildFunction(ReflectionFunction $raw, int $depth): ?string
91+
public function buildFunction(ReflectionFunction $raw, int $depth): string
8592
{
8693
return $this->quoter->quote($this->buildSignature($raw, $depth), $depth);
8794
}
@@ -104,8 +111,7 @@ private function buildStaticMethod(ReflectionMethod $reflection, int $depth): st
104111

105112
private function buildSignature(ReflectionFunctionAbstract $function, int $depth): string
106113
{
107-
$signature = $function->isClosure() ? 'function ' : $function->getName();
108-
$signature .= sprintf(
114+
$signature = sprintf(
109115
'(%s)',
110116
implode(
111117
', ',
@@ -135,7 +141,11 @@ private function buildSignature(ReflectionFunctionAbstract $function, int $depth
135141
$signature .= ': ' . $this->buildType($returnType, $depth);
136142
}
137143

138-
return $signature;
144+
if ($function->isClosure()) {
145+
return sprintf('Closure { %sfn %s }', $function->isStatic() ? 'static ' : '', $signature);
146+
}
147+
148+
return $function->getName() . $signature;
139149
}
140150

141151
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
@@ -62,8 +62,10 @@ public static function createDefault(): self
6262
self::MAXIMUM_NUMBER_OF_PROPERTIES
6363
)
6464
);
65-
$stringifier->prependStringifier($callableStringifier = new CallableStringifier($stringifier, $quoter));
66-
$stringifier->prependStringifier(new FiberObjectStringifier($callableStringifier, $quoter));
65+
$stringifier->prependStringifier(new CallableStringifier($stringifier, $quoter));
66+
$stringifier->prependStringifier(
67+
new FiberObjectStringifier(new CallableStringifier($stringifier, $quoter, closureOnly: false), $quoter)
68+
);
6769
$stringifier->prependStringifier(new EnumerationStringifier($quoter));
6870
$stringifier->prependStringifier(new ObjectWithDebugInfoStringifier($arrayStringifier, $quoter));
6971
$stringifier->prependStringifier(new ArrayObjectStringifier($arrayStringifier, $quoter));

‎tests/fixtures/WithMethods.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
final class WithMethods
1212
{
13-
public function publicMethod(Iterator&Countable $parameter): ?static
13+
public function publicMethod(Iterator&Countable $parameter): static
1414
{
1515
return new static();
1616
}

‎tests/integration/stringify-callable.phpt‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,22 +8,12 @@ require 'vendor/autoload.php';
88
$variable = new WithInvoke();
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/src/Double/FakeStringifier.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818

1919
final class FakeStringifier implements Stringifier
2020
{
21-
public function stringify(mixed $raw, int $depth): ?string
21+
public function stringify(mixed $raw, int $depth): string
2222
{
2323
return implode('.', ['fake', $depth, hash('crc32', serialize($raw))]);
2424
}

‎tests/unit/Stringifiers/CallableStringifierTest.php‎

Lines changed: 105 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,6 @@
2525
use function array_sum;
2626
use function sprintf;
2727

28-
use const PHP_MAJOR_VERSION;
29-
use const PHP_MINOR_VERSION;
30-
3128
#[CoversClass(ObjectHelper::class)]
3229
#[CoversClass(CallableStringifier::class)]
3330
final class CallableStringifierTest extends TestCase
@@ -37,14 +34,14 @@ final class CallableStringifierTest extends TestCase
3734
#[Test]
3835
public function itShouldNotStringifyWhenRawValueIsNotCallable(): void
3936
{
40-
$sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter());
37+
$sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter(), closureOnly: false);
4138

4239
self::assertNull($sut->stringify(1, self::DEPTH));
4340
}
4441

4542
#[Test]
46-
#[DataProvider('callableRawValuesProvider')]
47-
public function itShouldStringifyWhenRawValueIsCallable(callable $raw, string $expectedWithoutQuotes): void
43+
#[DataProvider('closureRawValuesProvider')]
44+
public function itShouldStringifyWhenRawValueIsClosure(callable $raw, string $expectedWithoutQuotes): void
4845
{
4946
$quoter = new FakeQuoter();
5047

@@ -56,6 +53,31 @@ public function itShouldStringifyWhenRawValueIsCallable(callable $raw, string $e
5653
self::assertEquals($expected, $actual);
5754
}
5855

56+
#[Test]
57+
#[DataProvider('nonClosureCallableRawValuesProvider')]
58+
public function itShouldNotStringifyNonClosureCallableByDefault(callable $raw, string $_): 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+
5981
#[Test]
6082
public function itShouldStringifyWhenRawValueIsCallableWithDefaultValues(): void
6183
{
@@ -68,7 +90,7 @@ public function itShouldStringifyWhenRawValueIsCallableWithDefaultValues(): void
6890

6991
$actual = $sut->stringify($raw, self::DEPTH);
7092
$expected = $quoter->quote(
71-
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)),
7294
self::DEPTH
7395
);
7496

@@ -86,7 +108,7 @@ public function itShouldStringifyWhenRawValueIsCallableThatDoesNotHaveAnAccessib
86108

87109
$quoter = new FakeQuoter();
88110

89-
$sut = new CallableStringifier(new FakeStringifier(), $quoter);
111+
$sut = new CallableStringifier(new FakeStringifier(), $quoter, closureOnly: false);
90112

91113
$actual = $sut->stringify($raw, self::DEPTH);
92114
$expected = $quoter->quote(
@@ -98,40 +120,90 @@ public function itShouldStringifyWhenRawValueIsCallableThatDoesNotHaveAnAccessib
98120
}
99121

100122
/**
101-
* @return array<int, array{0: callable, 1: string}>
123+
* @return array<string, array{0: callable, 1: string}>
102124
*/
103-
public static function callableRawValuesProvider(): array
125+
public static function closureRawValuesProvider(): array
104126
{
105127
$var1 = 1;
106128
$var2 = 2;
107129

108130
return [
109-
[static fn() => 1, 'function ()'],
110-
[static fn(): int => 1, 'function (): int'],
111-
[static fn(float $value): int => (int) $value, 'function (float $value): int'],
112-
[static fn(float &$value): int => (int) $value, 'function (float &$value): int'],
113-
[static fn(?float $value): int => (int) $value, 'function (?float $value): int'],
114-
[static fn(int $value = self::DEPTH): int => $value, 'function (int $value = self::DEPTH): int'],
115-
[static fn(int|float $value): int => (int) $value, 'function (int|float $value): int'],
116-
[static fn(Countable&Iterator $value): int => $value->count(), 'function (Countable&Iterator $value): int'],
117-
[static fn(int ...$value): int => array_sum($value), 'function (int ...$value): int'],
118-
[
131+
'static closure without parameters' => [
132+
static fn() => 1,
133+
'Closure { static fn () }',
134+
],
135+
'non-static closure without parameters' => [
136+
fn() => 1,
137+
'Closure { fn () }',
138+
],
139+
'static closure with return type' => [
140+
static fn(): int => 1,
141+
'Closure { static fn (): int }',
142+
],
143+
'non-static closure with return type' => [
144+
fn(): int => 1,
145+
'Closure { fn (): int }',
146+
],
147+
'static closure with typed parameter' => [
148+
static fn(float $value): int => (int) $value,
149+
'Closure { static fn (float $value): int }',
150+
],
151+
'static closure with reference parameter' => [
152+
static fn(float &$value): int => (int) $value,
153+
'Closure { static fn (float &$value): int }',
154+
],
155+
'static closure with nullable parameter' => [
156+
static fn(?float $value): int => (int) $value,
157+
'Closure { static fn (?float $value): int }',
158+
],
159+
'static closure with constant default value' => [
160+
static fn(int $value = self::DEPTH): int => $value,
161+
'Closure { static fn (int $value = self::DEPTH): int }',
162+
],
163+
'static closure with union type parameter' => [
164+
static fn(int|float $value): int => (int) $value,
165+
'Closure { static fn (int|float $value): int }',
166+
],
167+
'static closure with intersection type parameter' => [
168+
static fn(Countable&Iterator $value): int => $value->count(),
169+
'Closure { static fn (Countable&Iterator $value): int }',
170+
],
171+
'static closure with variadic parameter' => [
172+
static fn(int ...$value): int => array_sum($value),
173+
'Closure { static fn (int ...$value): int }',
174+
],
175+
'static closure with multiple parameters' => [
119176
static fn(float $value1, float $value2): float => $value1 + $value2,
120-
'function (float $value1, float $value2): float',
177+
'Closure { static fn (float $value1, float $value2): float }',
121178
],
122-
[
179+
'static closure with single use variable' => [
123180
static function (int $value) use ($var1): int {
124181
return $value + $var1;
125182
},
126-
'function (int $value) use ($var1): int',
183+
'Closure { static fn (int $value) use ($var1): int }',
127184
],
128-
[
185+
'static closure with multiple use variables' => [
129186
static function (int $value) use ($var1, $var2): int {
130187
return $value + $var1 + $var2;
131188
},
132-
'function (int $value) use ($var1, $var2): int',
189+
'Closure { static fn (int $value) use ($var1, $var2): int }',
190+
],
191+
'non-static closure with use variable' => [
192+
function (int $value) use ($var1): int {
193+
return $value + $var1;
194+
},
195+
'Closure { fn (int $value) use ($var1): int }',
133196
],
134-
[
197+
];
198+
}
199+
200+
/**
201+
* @return array<string, array{0: callable, 1: string}>
202+
*/
203+
public static function nonClosureCallableRawValuesProvider(): array
204+
{
205+
return [
206+
'invokable object' => [
135207
new class {
136208
public function __invoke(int $parameter): never
137209
{
@@ -140,19 +212,22 @@ public function __invoke(int $parameter): never
140212
},
141213
'class->__invoke(int $parameter): never',
142214
],
143-
[
215+
'object method as array' => [
144216
[new DateTime(), 'format'],
145217
'DateTime->format(string $format)',
146218
],
147-
[
219+
'static method as array' => [
148220
['DateTime', 'createFromImmutable'],
149221
'DateTime::createFromImmutable(DateTimeImmutable $object)',
150222
],
151-
[
223+
'static method as string' => [
152224
'DateTimeImmutable::getLastErrors',
153225
'DateTimeImmutable::getLastErrors()',
154226
],
155-
['chr', 'chr(int $codepoint): string'],
227+
'function name as string' => [
228+
'chr',
229+
'chr(int $codepoint): string',
230+
],
156231
];
157232
}
158233
}

0 commit comments

Comments
 (0)