Skip to content

Commit cce56ae

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 cce56ae

8 files changed

Lines changed: 140 additions & 71 deletions

File tree

‎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: 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/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 & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,14 @@ final class CallableStringifierTest extends TestCase
3737
#[Test]
3838
public function itShouldNotStringifyWhenRawValueIsNotCallable(): void
3939
{
40-
$sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter());
40+
$sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter(), closureOnly: false);
4141

4242
self::assertNull($sut->stringify(1, self::DEPTH));
4343
}
4444

4545
#[Test]
46-
#[DataProvider('callableRawValuesProvider')]
47-
public function itShouldStringifyWhenRawValueIsCallable(callable $raw, string $expectedWithoutQuotes): void
46+
#[DataProvider('closureRawValuesProvider')]
47+
public function itShouldStringifyWhenRawValueIsClosure(callable $raw, string $expectedWithoutQuotes): void
4848
{
4949
$quoter = new FakeQuoter();
5050

@@ -56,6 +56,31 @@ public function itShouldStringifyWhenRawValueIsCallable(callable $raw, string $e
5656
self::assertEquals($expected, $actual);
5757
}
5858

59+
#[Test]
60+
#[DataProvider('nonClosureCallableRawValuesProvider')]
61+
public function itShouldNotStringifyNonClosureCallableByDefault(callable $raw, string $_): void
62+
{
63+
$sut = new CallableStringifier(new FakeStringifier(), new FakeQuoter());
64+
65+
self::assertNull($sut->stringify($raw, self::DEPTH));
66+
}
67+
68+
#[Test]
69+
#[DataProvider('nonClosureCallableRawValuesProvider')]
70+
public function itShouldStringifyNonClosureCallableWhenClosureOnlyIsFalse(
71+
callable $raw,
72+
string $expectedWithoutQuotes,
73+
): void {
74+
$quoter = new FakeQuoter();
75+
76+
$sut = new CallableStringifier(new FakeStringifier(), $quoter, closureOnly: false);
77+
78+
$actual = $sut->stringify($raw, self::DEPTH);
79+
$expected = $quoter->quote($expectedWithoutQuotes, self::DEPTH);
80+
81+
self::assertEquals($expected, $actual);
82+
}
83+
5984
#[Test]
6085
public function itShouldStringifyWhenRawValueIsCallableWithDefaultValues(): void
6186
{
@@ -68,7 +93,7 @@ public function itShouldStringifyWhenRawValueIsCallableWithDefaultValues(): void
6893

6994
$actual = $sut->stringify($raw, self::DEPTH);
7095
$expected = $quoter->quote(
71-
sprintf('function (int $value = %s): int', $stringifier->stringify(1, self::DEPTH + 1)),
96+
sprintf('Closure { static fn(int $value = %s): int }', $stringifier->stringify(1, self::DEPTH + 1)),
7297
self::DEPTH
7398
);
7499

@@ -86,7 +111,7 @@ public function itShouldStringifyWhenRawValueIsCallableThatDoesNotHaveAnAccessib
86111

87112
$quoter = new FakeQuoter();
88113

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

91116
$actual = $sut->stringify($raw, self::DEPTH);
92117
$expected = $quoter->quote(
@@ -98,40 +123,90 @@ public function itShouldStringifyWhenRawValueIsCallableThatDoesNotHaveAnAccessib
98123
}
99124

100125
/**
101-
* @return array<int, array{0: callable, 1: string}>
126+
* @return array<string, array{0: callable, 1: string}>
102127
*/
103-
public static function callableRawValuesProvider(): array
128+
public static function closureRawValuesProvider(): array
104129
{
105130
$var1 = 1;
106131
$var2 = 2;
107132

108133
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-
[
134+
'static closure without parameters' => [
135+
static fn() => 1,
136+
'Closure { static fn() }',
137+
],
138+
'non-static closure without parameters' => [
139+
fn() => 1,
140+
'Closure { fn() }',
141+
],
142+
'static closure with return type' => [
143+
static fn(): int => 1,
144+
'Closure { static fn(): int }',
145+
],
146+
'non-static closure with return type' => [
147+
fn(): int => 1,
148+
'Closure { fn(): int }',
149+
],
150+
'static closure with typed parameter' => [
151+
static fn(float $value): int => (int) $value,
152+
'Closure { static fn(float $value): int }',
153+
],
154+
'static closure with reference parameter' => [
155+
static fn(float &$value): int => (int) $value,
156+
'Closure { static fn(float &$value): int }',
157+
],
158+
'static closure with nullable parameter' => [
159+
static fn(?float $value): int => (int) $value,
160+
'Closure { static fn(?float $value): int }',
161+
],
162+
'static closure with constant default value' => [
163+
static fn(int $value = self::DEPTH): int => $value,
164+
'Closure { static fn(int $value = self::DEPTH): int }',
165+
],
166+
'static closure with union type parameter' => [
167+
static fn(int|float $value): int => (int) $value,
168+
'Closure { static fn(int|float $value): int }',
169+
],
170+
'static closure with intersection type parameter' => [
171+
static fn(Countable&Iterator $value): int => $value->count(),
172+
'Closure { static fn(Countable&Iterator $value): int }',
173+
],
174+
'static closure with variadic parameter' => [
175+
static fn(int ...$value): int => array_sum($value),
176+
'Closure { static fn(int ...$value): int }',
177+
],
178+
'static closure with multiple parameters' => [
119179
static fn(float $value1, float $value2): float => $value1 + $value2,
120-
'function (float $value1, float $value2): float',
180+
'Closure { static fn(float $value1, float $value2): float }',
121181
],
122-
[
182+
'static closure with single use variable' => [
123183
static function (int $value) use ($var1): int {
124184
return $value + $var1;
125185
},
126-
'function (int $value) use ($var1): int',
186+
'Closure { static fn(int $value) use ($var1): int }',
127187
],
128-
[
188+
'static closure with multiple use variables' => [
129189
static function (int $value) use ($var1, $var2): int {
130190
return $value + $var1 + $var2;
131191
},
132-
'function (int $value) use ($var1, $var2): int',
192+
'Closure { static fn(int $value) use ($var1, $var2): int }',
193+
],
194+
'non-static closure with use variable' => [
195+
function (int $value) use ($var1): int {
196+
return $value + $var1;
197+
},
198+
'Closure { fn(int $value) use ($var1): int }',
133199
],
134-
[
200+
];
201+
}
202+
203+
/**
204+
* @return array<string, array{0: callable, 1: string}>
205+
*/
206+
public static function nonClosureCallableRawValuesProvider(): array
207+
{
208+
return [
209+
'invokable object' => [
135210
new class {
136211
public function __invoke(int $parameter): never
137212
{
@@ -140,19 +215,22 @@ public function __invoke(int $parameter): never
140215
},
141216
'class->__invoke(int $parameter): never',
142217
],
143-
[
218+
'object method as array' => [
144219
[new DateTime(), 'format'],
145220
'DateTime->format(string $format)',
146221
],
147-
[
222+
'static method as array' => [
148223
['DateTime', 'createFromImmutable'],
149224
'DateTime::createFromImmutable(DateTimeImmutable $object)',
150225
],
151-
[
226+
'static method as string' => [
152227
'DateTimeImmutable::getLastErrors',
153228
'DateTimeImmutable::getLastErrors()',
154229
],
155-
['chr', 'chr(int $codepoint): string'],
230+
'function name as string' => [
231+
'chr',
232+
'chr(int $codepoint): string',
233+
],
156234
];
157235
}
158236
}

0 commit comments

Comments
 (0)