Skip to content

Commit 7106979

Browse files
committed
fix: bypass the presentation cache when the key digest cannot be encoded
The digest parts come raw off the query string, and percent-decoding hands them over as bytes: expand=%FF arrives as "\xFF", which is not valid UTF-8, so json_encode() returns false - and hash() coerces that false to "" without a warning, the file not declaring strict_types. Every request carrying any malformed byte then collapsed onto one digest per presentation, serializer class included, undoing every discriminator dea76db added: an admin-shaped payload cached under the constant digest was served verbatim to a public caller whose own request also failed to encode. The trigger needs a privileged caller to emit malformed UTF-8 inside the TTL, which nothing in the platform does organically, so this is hardening rather than a live hole - but the guard is one expression: the encode result is captured, and a request whose parts cannot be keyed unambiguously skips the cache entirely, read and write both. Serving fresh is preferred over JSON_INVALID_UTF8_SUBSTITUTE, which would still merge requests differing only in which invalid byte they carried. Flagged by CodeRabbit on PR #577 (r3714097910), verified end to end in the container: Illuminate\Http\Request::create preserves the raw byte through input(), and hash('sha256', false) raises nothing at E_ALL.
1 parent 23a45c2 commit 7106979

2 files changed

Lines changed: 43 additions & 12 deletions

File tree

‎app/ModelSerializers/Summit/Presentation/PresentationSerializer.php‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -266,26 +266,29 @@ public function serialize($expand = null, array $fields = [], array $relations =
266266
// last_edited stays readable so a presentation update naturally busts every entry it has
267267
// without an explicit Cache::forget, and so an operator can still scan or drop one
268268
// presentation's entries by pattern.
269+
// The parts come raw off the query string, and percent-decoding hands them over as
270+
// bytes: a malformed sequence like %FF makes json_encode() return false, which hash()
271+
// would silently coerce to "" - collapsing class, expand, fields and relations onto one
272+
// shared digest per presentation, the exact cross-audience collision the digest exists
273+
// to prevent. A request that cannot be keyed unambiguously bypasses the cache entirely.
274+
$digest_source = json_encode
275+
([
276+
'serializer' => static::class,
277+
'expand' => $expand ?? "",
278+
'fields' => $cache_fields,
279+
'relations' => $cache_relations,
280+
]);
281+
269282
$key =
270283
sprintf
271284
(
272285
"presentation_%s_%s_%s",
273286
$presentation->getId(),
274287
$presentation->getLastEditedUTC()?->getTimestamp() ?? 0,
275-
hash
276-
(
277-
'sha256',
278-
json_encode
279-
([
280-
'serializer' => static::class,
281-
'expand' => $expand ?? "",
282-
'fields' => $cache_fields,
283-
'relations' => $cache_relations,
284-
])
285-
)
288+
hash('sha256', (string) $digest_source)
286289
);
287290

288-
$use_cache = $params['use_cache'] ?? false;
291+
$use_cache = ($params['use_cache'] ?? false) && $digest_source !== false;
289292

290293
if($use_cache){
291294
// One read, not Cache::has() followed by Cache::get(): the entry can expire on its

‎tests/PresentationSerializerCacheKeyTest.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,4 +184,32 @@ public function testExpandOrderIsNotNormalised()
184184

185185
$this->assertCount(2, $this->store);
186186
}
187+
188+
/**
189+
* The digest parts come raw off the query string, and percent-decoding hands them over as
190+
* bytes: expand=%FF arrives as "\xFF", which is not valid UTF-8, so json_encode() returns
191+
* false - and hash() coerces that false to "" without a warning. Every request carrying any
192+
* malformed byte then shares one digest per presentation, serializer class included, which
193+
* is exactly the admin-payload-to-public-caller collision the class component exists to
194+
* prevent. A request that cannot be keyed unambiguously must not touch the cache at all.
195+
*/
196+
public function testMalformedUtf8RequestsDoNotCollideOnOneEntry()
197+
{
198+
$presentation = $this->buildPresentation(90205);
199+
$context = $this->buildPublicContext();
200+
201+
$admin = (new AdminPresentationSerializer($presentation, $context))
202+
->serialize(null, ['id', 'rank', "\xFF"], [], ['use_cache' => true]);
203+
// Sanity, mirroring testAdminPayloadIsNotServedToAPublicCaller: the admin payload
204+
// really carries the field whose leak is asserted below.
205+
$this->assertSame(7, $admin['rank']);
206+
207+
$public = (new PresentationSerializer($presentation, $context))
208+
->serialize(null, ['id', "\xFE"], [], ['use_cache' => true]);
209+
210+
// Before the guard both digests collapsed to hash("") and this came back with the
211+
// admin-shaped payload, rank included.
212+
$this->assertArrayNotHasKey('rank', $public);
213+
$this->assertSame(90205, $public['id']);
214+
}
187215
}

0 commit comments

Comments
 (0)