From 76804e44db3d09d3b9e6e5e74f272e25aabbcf34 Mon Sep 17 00:00:00 2001 From: JpMaxMan Date: Mon, 3 Aug 2026 23:36:19 -0500 Subject: [PATCH 01/10] fix: gate public presentation serialization on display_on_site for media uploads Public/anonymous callers to the events/published endpoints could pull full PresentationMediaUpload data -- including live public S3 URLs -- for draft (display_on_site=false) uploads via ?expand=media_uploads, since PresentationSerializer read the unfiltered media uploads collection. Reported externally: a third party's calendar-scraping agent recovered pre-event draft slide decks this way. getVisibleMediaUploads() now reuses the existing admin/editor privilege check to filter to display_on_site=true uploads for Public callers, at all three call sites in this file. AdminPresentationCSVSerializer (admin-only) is untouched. --- .../Presentation/PresentationSerializer.php | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index f31fbde16..45747c397 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -91,6 +91,26 @@ protected function getMediaUploadsSerializerType():string{ return $serializerType; } + /** + * Media uploads visible to the resolved serializer type. A Public caller (no admin/editor + * privilege on this presentation) only ever sees uploads marked display_on_site=true — an + * uploaded-but-not-yet-approved draft (display_on_site=false, the model's own default) must + * never reach an unauthenticated/public response, even via ?expand=media_uploads on the + * public events/published endpoints. + * @return \Doctrine\Common\Collections\Collection|PresentationMediaUpload[] + */ + protected function getVisibleMediaUploads() + { + $presentation = $this->object; + $mediaUploads = $presentation->getMediaUploads(); + if ($this->getMediaUploadsSerializerType() === SerializerRegistry::SerializerType_Private) { + return $mediaUploads; + } + return $mediaUploads->filter(function ($mediaUpload) { + return $mediaUpload->getDisplayOnSite(); + }); + } + /** * @param null $expand @@ -130,7 +150,7 @@ public function serialize($expand = null, array $fields = [], array $relations = { $media_uploads = []; - foreach ($presentation->getMediaUploads() as $mediaUpload) { + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { $media_uploads[] = SerializerRegistry::getInstance()->getSerializer ( $mediaUpload, $this->getMediaUploadsSerializerType() @@ -195,7 +215,7 @@ public function serialize($expand = null, array $fields = [], array $relations = if(in_array('media_uploads', $relations)) { $media_uploads = []; - foreach ($presentation->getMediaUploads() as $mediaUpload) { + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { $media_uploads[] = $mediaUpload->getId(); } @@ -337,7 +357,7 @@ public function serialize($expand = null, array $fields = [], array $relations = case 'media_uploads':{ $media_uploads = []; - foreach ($presentation->getMediaUploads() as $mediaUpload) { + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { $media_uploads[] = SerializerRegistry::getInstance()->getSerializer ( $mediaUpload, $this->getMediaUploadsSerializerType() From 0c34adc3b1cd49b18aa8d9edfd192d492570a20b Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 10:51:51 -0300 Subject: [PATCH 02/10] fix: grant media upload visibility to summit admins and snapshot service accounts getMediaUploadsSerializerType() only recognised isAdmin() and memberCanEdit(), so two callers that OAuth2SummitEventsApiController::getSerializerType() already treats as privileged fell through to Public and lost every media upload once the display_on_site filter landed: - summit admins (summit-front-end-administrators). The event grid requests media_uploads.display_on_site, so the operator could no longer see the upload whose checkbox is the only thing that would make it visible again. - the content-snapshot service account. pub-api reads media_uploads with a client_credentials token, which carries no user_id, so getCurrentUser() is null by construction; the snapshot emptied and dropbox-materializer, which does not filter on the flag itself, staged nothing for every session. Service accounts are gated on a dedicated scope rather than on ApplicationType_Service alone, which would have handed drafts to every service client. The scope is registered with no endpoint association on purpose: endpoint scopes are matched with array_intersect (any-of), so associating it would admit a token holding only this scope to that endpoint. It is read straight off the token and never consulted through endpoint_api_scopes. Rollout order: the scope has to exist in openstackid and be granted to the content-snapshot client, and be added to pub-api's CONTENT_SNAPSHOT_OAUTH2_SCOPES, before this ships - until then that client still resolves Public. --- .../Presentation/PresentationSerializer.php | 31 +++++++++- app/Security/SummitScopes.php | 1 + .../config/Version20260804120000.php | 61 +++++++++++++++++++ database/seeders/ApiScopesSeeder.php | 5 ++ 4 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 database/migrations/config/Version20260804120000.php diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index 45747c397..b4dfe7a03 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -12,9 +12,11 @@ * limitations under the License. **/ +use App\Security\SummitScopes; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; use Libs\ModelSerializers\AbstractSerializer; +use models\oauth2\IResourceServerContext; use models\summit\Presentation; use models\summit\PresentationType; @@ -79,13 +81,40 @@ class PresentationSerializer extends SummitEventSerializer ]; /** + * Resolves who is allowed to see every media upload attached to this presentation, approved + * or not. Kept aligned with OAuth2SummitEventsApiController::getSerializerType(), which is + * what decides the serializer type of the presentation itself - when the two disagree the + * presentation is served Private while its uploads are served Public, which is the bug this + * method used to have for summit admins and for service accounts. + * + * Two distinct privileged callers: + * + * - Service accounts (client_credentials, so getCurrentUser() is null by construction) that + * hold the dedicated snapshot scope. The content pipeline stages files pre-event, so it + * needs unapproved uploads. Gated on the scope and not on ApplicationType_Service alone: + * the application type on its own would hand drafts to every service client. + * - Members with an admin-level group, or with edit rights over this presentation + * (creator / moderator / speaker). + * * @return string */ protected function getMediaUploadsSerializerType():string{ + // && short-circuits, so getCurrentScope() is only reached for service accounts + $isSnapshotClient = + $this->resource_server_context->getApplicationType() === IResourceServerContext::ApplicationType_Service + && in_array + ( + SummitScopes::ReadAllPresentationMediaUploads, + $this->resource_server_context->getCurrentScope() + ); + + if ($isSnapshotClient) + return SerializerRegistry::SerializerType_Private; + $serializerType = SerializerRegistry::SerializerType_Public; $currentUser = $this->resource_server_context->getCurrentUser(); $presentation = $this->object; - if(!is_null($currentUser) && ( $currentUser->isAdmin() || $presentation->memberCanEdit($currentUser))){ + if(!is_null($currentUser) && ( $currentUser->isAdmin() || $currentUser->isSummitAdmin() || $presentation->memberCanEdit($currentUser))){ $serializerType = SerializerRegistry::SerializerType_Private; } return $serializerType; diff --git a/app/Security/SummitScopes.php b/app/Security/SummitScopes.php index 5911424aa..b8ed477e2 100644 --- a/app/Security/SummitScopes.php +++ b/app/Security/SummitScopes.php @@ -22,6 +22,7 @@ final class SummitScopes const ReadSummitData = SCOPE_BASE_REALM.'/summits/read'; const ReadAllSummitData = SCOPE_BASE_REALM.'/summits/read/all'; const ReadOverflowEvents = SCOPE_BASE_REALM.'/summits/events/overflow/read'; + const ReadAllPresentationMediaUploads = SCOPE_BASE_REALM.'/summits/presentations/media-uploads/read/all'; // me const MeRead = SCOPE_BASE_REALM.'/me/read'; diff --git a/database/migrations/config/Version20260804120000.php b/database/migrations/config/Version20260804120000.php new file mode 100644 index 000000000..3f07c297d --- /dev/null +++ b/database/migrations/config/Version20260804120000.php @@ -0,0 +1,61 @@ +addSql($this->insertApiScope( + self::API_NAME, + SummitScopes::ReadAllPresentationMediaUploads, + 'Read All Presentation Media Uploads', + 'Grants read access to presentation media uploads regardless of display_on_site, for trusted service accounts feeding the content pipeline' + )); + } + + public function down(Schema $schema): void + { + $this->addSql($this->deleteApiScopes(self::API_NAME, [SummitScopes::ReadAllPresentationMediaUploads])); + } +} diff --git a/database/seeders/ApiScopesSeeder.php b/database/seeders/ApiScopesSeeder.php index 6eab76a31..de32ab477 100644 --- a/database/seeders/ApiScopesSeeder.php +++ b/database/seeders/ApiScopesSeeder.php @@ -68,6 +68,11 @@ private function seedSummitScopes() 'short_description' => 'Read Summit Overflow Events Data', 'description' => 'Grants read only access to published summit events currently in OVERFLOW occupancy, including overflow streaming URLs and tokens', ], + [ + 'name' => SummitScopes::ReadAllPresentationMediaUploads, + 'short_description' => 'Read All Presentation Media Uploads', + 'description' => 'Grants read access to presentation media uploads regardless of display_on_site, for trusted service accounts feeding the content pipeline', + ], [ 'name' => SummitScopes::MeRead, 'short_description' => 'Get own summit member data', From 4edf3b89a212e970ce5f6334a10b3831c9089349 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 10:52:09 -0300 Subject: [PATCH 03/10] test: repair PresentationMediaUploadsTests fixture The file is under tests/ root and no CI job filter covers it (push.yml runs tests/oauth2/, tests/Unit/*, tests/Repositories/), so it rotted unnoticed and failed on any environment. Three separate causes: - setUp() read SummitMediaFileType via findAll() before insertSummitTestData(), which opens with DELETE FROM SummitMediaFileType. The entity it kept pointed at a deleted row, so the flush died on the SummitMediaUploadType.TypeID FK. - self::$default_media_file_type is not a usable substitute: it carries ".PDF", while SummitMediaUploadType::isValidExtension() compares strtoupper($ext) against explode('|', ...), so a leading dot can never match. The test builds its own type declaring PNG, matching the png it uploads and the format the seeder uses (JPG|JPEG|PNG). - the fixture declared Swift public storage, and serializing public_url builds a download strategy for it, which needs an authUrl that neither the local container nor CI provides. Local needs no credentials and the assertion is about public_url being serialized, not about the backend behind it. Green and repeatable across consecutive runs. --- tests/PresentationMediaUploadsTests.php | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/tests/PresentationMediaUploadsTests.php b/tests/PresentationMediaUploadsTests.php index 7bd1e5d5b..77d73e338 100644 --- a/tests/PresentationMediaUploadsTests.php +++ b/tests/PresentationMediaUploadsTests.php @@ -44,17 +44,32 @@ protected function setUp():void { parent::setUp(); self::$media_file_type_repository = EntityManager::getRepository(SummitMediaFileType::class); - $types = self::$media_file_type_repository->findAll(); self::insertSummitTestData(); + + // Built here rather than read from the repository: insertSummitTestData() opens with + // DELETE FROM SummitMediaFileType, so anything fetched before it is a detached row by + // the time we flush. It cannot reuse self::$default_media_file_type either - that one + // carries ".PDF", and SummitMediaUploadType::isValidExtension() compares + // strtoupper($ext) against explode('|', ...), so a leading dot never matches. + $media_file_type = new SummitMediaFileType(); + $media_file_type->setName("PNG_".rand(1, 100)); + $media_file_type->setDescription("PNG"); + $media_file_type->setAllowedExtensions("PNG"); + self::$em->persist($media_file_type); + self::$media_upload_type = new SummitMediaUploadType(); - self::$media_upload_type->setType($types[0]); + self::$media_upload_type->setType($media_file_type); self::$media_upload_type->setName('TEST'); self::$media_upload_type->setDescription("TEST"); self::$media_upload_type->setMaxSize(2048); self::$media_upload_type->setMinUploadsQty(2); self::$media_upload_type->setMaxUploadsQty(4); - self::$media_upload_type->setPrivateStorageType(\App\Models\Utils\IStorageTypesConstants::DropBox); - self::$media_upload_type->setPublicStorageType(\App\Models\Utils\IStorageTypesConstants::Swift); + self::$media_upload_type->setPrivateStorageType(\App\Models\Utils\IStorageTypesConstants::Local); + // Local, not Swift: serializing public_url builds a download strategy for whatever the + // type declares, and the Swift one needs an authUrl that neither this container nor CI + // provides. The assertion is about public_url being serialized at all, not about which + // backend serves it. + self::$media_upload_type->setPublicStorageType(\App\Models\Utils\IStorageTypesConstants::Local); self::$presentation = new Presentation(); $event_types = self::$summit->getEventTypes(); From f399ddfdac64546177155abe8921d0a4af86ca5a Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 11:21:02 -0300 Subject: [PATCH 04/10] fix: keep media_uploads out of the presentation cache, resolve it per caller getMediaUploadsSerializerType() resolves per user and per OAuth scope, but the cache key is built from id + LastEditedUTC + expand + fields + relations and has no audience component. A payload built for a privileged caller could therefore be served verbatim to an unprivileged one within the 1200s TTL, handing out display_on_site=false uploads the display_on_site filter was added to withhold. Adding the serializer class to the key would not close it: a speaker on the presentation and a plain attendee both serialize through PresentationSerializer, and a service account with ReadAllPresentationMediaUploads and one without it both serialize through AdminPresentationSerializer. Each pair shares a class and disagrees on this field. So the field is never stored. Cache::put receives a copy with media_uploads removed, and a new private withMediaUploads() resolves it fresh on the way out of every path -- cache hit, cache miss, and the non-cached branch alike. It opens by unsetting the field, so a payload written before this change cannot leak one either. Request shape is preserved: an id list for relations=media_uploads, serialized objects for expand=media_uploads, expand winning when both are present. This also removes the three scattered copies of that expansion logic, which were the reason the cache-hit branch could drift from the others in the first place. --- .../Presentation/PresentationSerializer.php | 124 ++++++++++-------- 1 file changed, 67 insertions(+), 57 deletions(-) diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index b4dfe7a03..765b6f9ab 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -140,6 +140,62 @@ protected function getVisibleMediaUploads() }); } + /** + * Sets media_uploads on an already-built payload for whoever is asking right now, in the + * shape the request asked for: an id list for ?relations=media_uploads, serialized objects + * for ?expand=media_uploads, expand winning when both are present. + * + * This is the only place that decides the value, and it runs on every path - including + * after a cache read - because getMediaUploadsSerializerType() resolves per user and per + * scope, which nothing in the cache key expresses. Two callers can share a key and still + * disagree here: a speaker on the presentation and a plain attendee both serialize through + * PresentationSerializer, and a service account holding ReadAllPresentationMediaUploads and + * one without it both serialize through AdminPresentationSerializer. Adding the serializer + * class to the key would not separate either pair. + * + * @param array $values + * @param null $expand + * @param array $fields + * @param array $relations + * @return array + */ + private function withMediaUploads(array $values, $expand, array $fields, array $relations): array + { + // Nothing asked for it: drop whatever a cached payload may be carrying, so a stale + // entry can never contribute this field to a response that did not request it. + unset($values['media_uploads']); + + if (in_array('media_uploads', $relations)) { + $media_uploads = []; + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { + $media_uploads[] = $mediaUpload->getId(); + } + $values['media_uploads'] = $media_uploads; + } + + if (!empty($expand)) { + foreach (explode(',', $expand) as $relation) { + if (trim($relation) !== 'media_uploads') continue; + + $media_uploads = []; + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { + $media_uploads[] = SerializerRegistry::getInstance()->getSerializer + ( + $mediaUpload, $this->getMediaUploadsSerializerType() + )->serialize + ( + AbstractSerializer::filterExpandByPrefix($expand, 'media_uploads'), + AbstractSerializer::filterFieldsByPrefix($fields, 'media_uploads'), + AbstractSerializer::filterFieldsByPrefix($relations, 'media_uploads'), + ); + } + $values['media_uploads'] = $media_uploads; + } + } + + return $values; + } + /** * @param null $expand @@ -171,32 +227,7 @@ public function serialize($expand = null, array $fields = [], array $relations = if($use_cache && Cache::has($key)){ $values = json_decode(Cache::get($key), true); Log::debug(sprintf("PresentationSerializer::serialize cache hit for presentation %s", $presentation->getId())); - if (!empty($expand)) { - foreach (explode(',', $expand) as $relation) { - $relation = trim($relation); - switch ($relation) { - case 'media_uploads': - { - $media_uploads = []; - - foreach ($this->getVisibleMediaUploads() as $mediaUpload) { - $media_uploads[] = SerializerRegistry::getInstance()->getSerializer - ( - $mediaUpload, $this->getMediaUploadsSerializerType() - )->serialize - ( - AbstractSerializer::filterExpandByPrefix($expand, $relation), - AbstractSerializer::filterFieldsByPrefix($fields, $relation), - AbstractSerializer::filterFieldsByPrefix($relations, $relation), - ); - } - - $values['media_uploads'] = $media_uploads; - } - } - } - } - return $values; + return $this->withMediaUploads($values, $expand, $fields, $relations); } $values = parent::serialize($expand, $fields, $relations, $params); @@ -241,16 +272,6 @@ public function serialize($expand = null, array $fields = [], array $relations = $values['videos'] = $videos; } - if(in_array('media_uploads', $relations)) - { - $media_uploads = []; - foreach ($this->getVisibleMediaUploads() as $mediaUpload) { - $media_uploads[] = $mediaUpload->getId(); - } - - $values['media_uploads'] = $media_uploads; - } - if(in_array('extra_questions', $relations)) { $answers = []; @@ -383,24 +404,6 @@ public function serialize($expand = null, array $fields = [], array $relations = $values['videos'] = $videos; } break; - case 'media_uploads':{ - $media_uploads = []; - - foreach ($this->getVisibleMediaUploads() as $mediaUpload) { - $media_uploads[] = SerializerRegistry::getInstance()->getSerializer - ( - $mediaUpload, $this->getMediaUploadsSerializerType() - )->serialize - ( - AbstractSerializer::filterExpandByPrefix($expand, $relation), - AbstractSerializer::filterFieldsByPrefix($fields, $relation), - AbstractSerializer::filterFieldsByPrefix($relations, $relation), - ); - } - - $values['media_uploads'] = $media_uploads; - } - break; case 'extra_questions':{ $answers = []; foreach ($presentation->getExtraQuestionAnswers() as $answer) { @@ -450,9 +453,16 @@ public function serialize($expand = null, array $fields = [], array $relations = } } - if($use_cache) - Cache::put($key, json_encode($values), self::CacheTTL); + if($use_cache) { + // media_uploads is deliberately kept out of the stored payload: it is the one field + // here whose value depends on who is asking, and the key has no audience component. + // Storing it would make correctness depend on every future reader remembering to + // recompute it. + $cacheable = $values; + unset($cacheable['media_uploads']); + Cache::put($key, json_encode($cacheable), self::CacheTTL); + } - return $values; + return $this->withMediaUploads($values, $expand, $fields, $relations); } } From fdb1b2d768ca976937c05bf54dc55a4e2a6ece35 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 11:41:20 -0300 Subject: [PATCH 05/10] test: rename PresentationMediaUploadsTests to the discoverable Test.php suffix The testsuite in phpunit.xml scans ./tests/ with the default suffix, which is Test.php, so a file ending in Tests.php is never collected. The class was reachable only by passing its path explicitly - `--filter PresentationMediaUploadsTests` answered "No tests executed!" - which is a large part of why it rotted unnoticed until the fixture repair two commits ago. The class is renamed alongside the file: autoload-dev maps Tests\ to tests/ via PSR-4, so the two have to agree. No call sites to update; nothing referenced the old name. --- ...MediaUploadsTests.php => PresentationMediaUploadsTest.php} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename tests/{PresentationMediaUploadsTests.php => PresentationMediaUploadsTest.php} (98%) diff --git a/tests/PresentationMediaUploadsTests.php b/tests/PresentationMediaUploadsTest.php similarity index 98% rename from tests/PresentationMediaUploadsTests.php rename to tests/PresentationMediaUploadsTest.php index 77d73e338..834d076d6 100644 --- a/tests/PresentationMediaUploadsTests.php +++ b/tests/PresentationMediaUploadsTest.php @@ -19,9 +19,9 @@ use models\summit\Presentation; use models\summit\SummitMediaUploadType; /** - * Class PresentationMediaUploadsTests + * Class PresentationMediaUploadsTest */ -class PresentationMediaUploadsTests +class PresentationMediaUploadsTest extends ProtectedApiTestCase { use InsertSummitTestData; From 35a7d9c3e203aa1ac1059b862267cf43b5645ddf Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 11:41:45 -0300 Subject: [PATCH 06/10] test: cover every privilege branch of getMediaUploadsSerializerType Six cases, one per branch, as asked for in the PR thread on this method: anonymous, plain attendee, speaker on the presentation, summit admin, service account holding ReadAllPresentationMediaUploads, and service account without it. They assert on the output of serialize() - the media upload ids the caller receives - rather than on the serializer-type string the method returns. What the change is about is who sees an unapproved upload; the type is the mechanism, and pinning it would tie the suite to the current implementation of a decision that could be reached another way. The relations=media_uploads shape gives a bare id list, so an assertion can name the exact uploads without dragging PresentationMediaUploadSerializer, storage backends and public_url generation into a unit test. Narrowing fields to id keeps the attribute-mapping loop off every other getter on the mock. Each case was checked against a broken implementation rather than assumed to bite: - dropping isSummitAdmin() from the member condition fails the summit admin case alone, which is the regression that blanked the admin event grid - gating service accounts on ApplicationType_Service without the scope check fails the without-scope case alone - removing the display_on_site filter from getVisibleMediaUploads() fails all three unprivileged cases The suite also gets a matrix entry, by path: no job runs the tests/ root, only its subdirectories, so both this file and the one renamed in the previous commit would otherwise run nowhere. --- .github/workflows/push.yml | 3 + ...PresentationMediaUploadsVisibilityTest.php | 209 ++++++++++++++++++ 2 files changed, 212 insertions(+) create mode 100644 tests/PresentationMediaUploadsVisibilityTest.php diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 51960448f..1388b4b5c 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -66,6 +66,9 @@ jobs: - { name: "Repositories", filter: "tests/Repositories/" } - { name: "Services", filter: "tests/Unit/Services/" } - { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" } + # Named by path because no job in this matrix runs the tests/ root, only its + # subdirectories - a file added there runs nowhere unless it is listed here. + - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php" } env: OTEL_SERVICE_ENABLED: false APP_ENV: testing diff --git a/tests/PresentationMediaUploadsVisibilityTest.php b/tests/PresentationMediaUploadsVisibilityTest.php new file mode 100644 index 000000000..f937d0762 --- /dev/null +++ b/tests/PresentationMediaUploadsVisibilityTest.php @@ -0,0 +1,209 @@ +shouldReceive('getId')->andReturn(self::ApprovedUploadId); + $approved->shouldReceive('getDisplayOnSite')->andReturn(true); + + $draft = Mockery::mock(PresentationMediaUpload::class); + $draft->shouldReceive('getId')->andReturn(self::DraftUploadId); + $draft->shouldReceive('getDisplayOnSite')->andReturn(false); + + $presentation = Mockery::mock(Presentation::class); + $presentation->shouldReceive('getId')->andReturn($identifier); + $presentation->shouldReceive('getLastEditedUTC')->andReturn(null); + $presentation->shouldReceive('getMediaUploads') + ->andReturn(new ArrayCollection([$approved, $draft])); + $presentation->shouldReceive('memberCanEdit')->andReturn($member_can_edit); + + return $presentation; + } + + /** + * A member-backed caller: a browser client carrying a user token. + * @param bool $is_admin global administrator. + * @param bool $is_summit_admin summit-front-end-administrators, the show-admin operators. + * @return IResourceServerContext + */ + private function buildMemberContext(bool $is_admin = false, bool $is_summit_admin = false): IResourceServerContext + { + $member = Mockery::mock(Member::class); + $member->shouldReceive('isAdmin')->andReturn($is_admin); + $member->shouldReceive('isSummitAdmin')->andReturn($is_summit_admin); + + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn($member); + + return $context; + } + + /** + * A client_credentials caller. getCurrentUser() is null by construction - there is no user + * behind the token - which is why the scope, and not the member, is what grants access here. + * @param bool $with_scope whether the token carries ReadAllPresentationMediaUploads. + * @return IResourceServerContext + */ + private function buildServiceContext(bool $with_scope): IResourceServerContext + { + $scopes = ['%s/summits/read']; + if ($with_scope) $scopes[] = SummitScopes::ReadAllPresentationMediaUploads; + + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType') + ->andReturn(IResourceServerContext::ApplicationType_Service); + $context->shouldReceive('getCurrentScope')->andReturn($scopes); + $context->shouldReceive('getCurrentUser')->andReturn(null); + + return $context; + } + + /** + * @param Presentation $presentation + * @param IResourceServerContext $context + * @return array the media upload ids this caller receives + */ + private function serializeMediaUploadIds(Presentation $presentation, IResourceServerContext $context): array + { + $serializer = new PresentationSerializer($presentation, $context); + // fields is narrowed to id so the attribute-mapping loop in AbstractSerializer only + // reaches Presentation::getId(); every other mapped getter is irrelevant here and would + // otherwise have to be stubbed for no gain. + $values = $serializer->serialize(null, ['id'], ['media_uploads']); + + $this->assertArrayHasKey('media_uploads', $values); + return $values['media_uploads']; + } + + public function testAnonymousCallerSeesOnlyApprovedUploads() + { + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn(null); + + $ids = $this->serializeMediaUploadIds($this->buildPresentation(90101), $context); + + $this->assertSame([self::ApprovedUploadId], $ids); + } + + public function testPlainAttendeeSeesOnlyApprovedUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90102, false), + $this->buildMemberContext() + ); + + $this->assertSame([self::ApprovedUploadId], $ids); + } + + public function testSpeakerOnThePresentationSeesDraftUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90103, true), + $this->buildMemberContext() + ); + + $this->assertSame([self::ApprovedUploadId, self::DraftUploadId], $ids); + } + + /** + * The regression this pins: before the fix a summit admin resolved Public here while + * OAuth2SummitEventsApiController::getSerializerType() resolved Private for the presentation + * itself, so the summit-admin event grid stopped showing the upload whose display_on_site + * checkbox is the only way to approve it. + */ + public function testSummitAdminSeesDraftUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90104, false), + $this->buildMemberContext(false, true) + ); + + $this->assertSame([self::ApprovedUploadId, self::DraftUploadId], $ids); + } + + public function testServiceAccountWithScopeSeesDraftUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90105), + $this->buildServiceContext(true) + ); + + $this->assertSame([self::ApprovedUploadId, self::DraftUploadId], $ids); + } + + /** + * The other half of that gate: the access is granted by the scope, not by the application + * type, so a service client without it stays where every other unprivileged caller is. + */ + public function testServiceAccountWithoutScopeSeesOnlyApprovedUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90106), + $this->buildServiceContext(false) + ); + + $this->assertSame([self::ApprovedUploadId], $ids); + } +} From 229e0a01cba0a198d67e6bf7fe12127fb8eb9074 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 11:55:52 -0300 Subject: [PATCH 07/10] fix: read the presentation cache once instead of has() then get() Cache::has() vouching for a key does not guarantee Cache::get() returns it: the entry can reach its TTL, be evicted under memory pressure, or be dropped by a flush in the window between the two calls. json_decode(null, true) is null, and since the previous commit that null reaches withMediaUploads(array $values, ...) and raises a TypeError, so the race now answers 500 where it used to answer a quietly wrong payload. Only the voteable-presentation endpoints pass use_cache, which is why this had not surfaced. One Cache::get(), and anything that does not decode to an array falls through to the normal build path - the race, an ordinary miss, and a truncated write all take the same route, which is the one that was always going to be correct. Two tests, each checked against a broken implementation rather than assumed to bite: - testUnavailableCachedValueIsTreatedAsAMiss reproduces the race and fails with exactly that TypeError against the previous code. - testCacheHitIsServedButMediaUploadsAreResolvedFresh covers the other side, because the first test passes just as well against a serializer that has stopped reading the cache at all. It also pins the guarantee from the previous commit: returning the cached $values without recomputing makes the stale draft upload in the stored payload reach a public caller, which is the leak this branch exists to close. It asserts on the payload rather than on how the cache was consulted, so it does not have to be rewritten if that read changes shape again. --- .../Presentation/PresentationSerializer.php | 16 +++-- ...PresentationMediaUploadsVisibilityTest.php | 68 ++++++++++++++++++- 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index 765b6f9ab..b409b6927 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -224,10 +224,18 @@ public function serialize($expand = null, array $fields = [], array $relations = $use_cache = $params['use_cache'] ?? false; - if($use_cache && Cache::has($key)){ - $values = json_decode(Cache::get($key), true); - Log::debug(sprintf("PresentationSerializer::serialize cache hit for presentation %s", $presentation->getId())); - return $this->withMediaUploads($values, $expand, $fields, $relations); + if($use_cache){ + // One read, not Cache::has() followed by Cache::get(): the entry can expire on its + // own TTL, be evicted, or be flushed in the window between the two, and the second + // call would then hand back null for a key the first call vouched for. Anything that + // does not decode to an array - a miss, that race, a truncated write - falls through + // and is rebuilt. + $cached = Cache::get($key); + $values = is_string($cached) ? json_decode($cached, true) : null; + if(is_array($values)){ + Log::debug(sprintf("PresentationSerializer::serialize cache hit for presentation %s", $presentation->getId())); + return $this->withMediaUploads($values, $expand, $fields, $relations); + } } $values = parent::serialize($expand, $fields, $relations, $params); diff --git a/tests/PresentationMediaUploadsVisibilityTest.php b/tests/PresentationMediaUploadsVisibilityTest.php index f937d0762..d479514cf 100644 --- a/tests/PresentationMediaUploadsVisibilityTest.php +++ b/tests/PresentationMediaUploadsVisibilityTest.php @@ -14,6 +14,7 @@ use App\Security\SummitScopes; use Doctrine\Common\Collections\ArrayCollection; +use Illuminate\Support\Facades\Cache; use models\main\Member; use models\oauth2\IResourceServerContext; use models\summit\Presentation; @@ -122,15 +123,20 @@ private function buildServiceContext(bool $with_scope): IResourceServerContext /** * @param Presentation $presentation * @param IResourceServerContext $context + * @param array $params forwarded to serialize(), which is where use_cache is read. * @return array the media upload ids this caller receives */ - private function serializeMediaUploadIds(Presentation $presentation, IResourceServerContext $context): array + private function serializeMediaUploadIds( + Presentation $presentation, + IResourceServerContext $context, + array $params = [] + ): array { $serializer = new PresentationSerializer($presentation, $context); // fields is narrowed to id so the attribute-mapping loop in AbstractSerializer only // reaches Presentation::getId(); every other mapped getter is irrelevant here and would // otherwise have to be stubbed for no gain. - $values = $serializer->serialize(null, ['id'], ['media_uploads']); + $values = $serializer->serialize(null, ['id'], ['media_uploads'], $params); $this->assertArrayHasKey('media_uploads', $values); return $values['media_uploads']; @@ -206,4 +212,62 @@ public function testServiceAccountWithoutScopeSeesOnlyApprovedUploads() $this->assertSame([self::ApprovedUploadId], $ids); } + + /** + * A cached entry that reports as present and then reads back as unavailable must degrade to + * a fresh build, not to an error. The entry can expire on its own TTL, be evicted under + * memory pressure, or be dropped by a cache flush, and none of that is rare enough on the + * voteable-presentation endpoints - the only ones that pass use_cache - to leave unhandled. + * + * The assertion is on the payload rather than on how the cache was consulted, so it holds + * whether the read is one call or two. + */ + public function testUnavailableCachedValueIsTreatedAsAMiss() + { + Cache::shouldReceive('has')->zeroOrMoreTimes()->andReturn(true); + Cache::shouldReceive('get')->zeroOrMoreTimes()->andReturn(null); + Cache::shouldReceive('put')->zeroOrMoreTimes()->andReturn(true); + + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn(null); + + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90107), + $context, + ['use_cache' => true] + ); + + $this->assertSame([self::ApprovedUploadId], $ids); + } + + /** + * The other side of that read: a decodable entry is still served from cache, and the + * media_uploads on it are still resolved for the caller asking now rather than taken from + * whoever populated the key. Without this the previous test would pass just as well against + * a serializer that had stopped reading the cache altogether. + */ + public function testCacheHitIsServedButMediaUploadsAreResolvedFresh() + { + // A payload as it is stored: media_uploads is absent by construction, and the stale value + // is one no unprivileged caller may receive. + Cache::shouldReceive('has')->zeroOrMoreTimes()->andReturn(true); + Cache::shouldReceive('get')->zeroOrMoreTimes()->andReturn( + json_encode(['id' => 90108, 'title' => 'from cache', 'media_uploads' => [self::DraftUploadId]]) + ); + Cache::shouldReceive('put')->zeroOrMoreTimes()->andReturn(true); + + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn(null); + + $serializer = new PresentationSerializer($this->buildPresentation(90108), $context); + $values = $serializer->serialize(null, ['id'], ['media_uploads'], ['use_cache' => true]); + + // Came off the cached payload: parent::serialize() was never reached, and it does not + // produce this field under fields=['id'] anyway. + $this->assertSame('from cache', $values['title']); + // ...but this one did not. + $this->assertSame([self::ApprovedUploadId], $values['media_uploads']); + } } From 6a16770c3a15ace00e2edcd4df87ef0b7e4ab255 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 12:24:01 -0300 Subject: [PATCH 08/10] fix: give the presentation cache key a serializer and an unambiguous encoding Closes the audience gap the media-uploads work left behind. serialize() is inherited, not overridden, by AdminPresentationSerializer and by the track-chair and CSV serializers, so all of them write through this cache, and getAttributeMappings() merges $array_mappings across the hierarchy - the stored payload carries rank, selection_status, streaming_url, etherpad_link, overflow_stream_key, chair scores and vote stats. The key named the presentation and the request shape but never the serializer, so a public caller repeating an admin's query params inside the 1200s TTL read the admin payload back verbatim. GET /summits/{id}/presentations/voteable has no admin gate and resolves the serializer per caller, which is what makes the sequence reachable. The parts are also no longer joined on characters they contain. "_" separated them while appearing inside media_uploads, extra_questions and selection_plan, so distinct requests could render one key: expand=media_uploads&fields=x and expand=media&fields=uploads_x both flattened to "..._media_uploads_x_". The encoding is what fixes this, not the digest - hashing the old concatenation preserves it exactly, which the test asserts. sha256 rather than md5 because these parts come from the query string and a collision here means serving one audience's payload to another. fields and relations are sorted first. Both are consumed with in_array(), so order cannot change the payload and two spellings of one request no longer cost two entries. $expand is left alone on purpose: its relations are dispatched in order, and the speakers and moderator cases both write $values['moderator'] while disagreeing about moderator_speaker_id, so normalising it could merge two payloads that are allowed to differ. A test pins that decision. The id and last_edited stay outside the digest so an update still busts every entry a presentation has, and so an operator can scan or drop them by pattern. No migration: the format change orphans existing entries, which age out on the TTL and rebuild on demand. The comment on the key carries the invariant this rests on - audience-dependent data either appears in the key or stays out of the cache. static::class is sufficient only while the remaining differences are class-determined, which is true today because the mappings are static, getSerializerType() is constant per class, and media_uploads is stripped before Cache::put. Refs ClickUp 86bb6aem0. --- .github/workflows/push.yml | 2 +- .../Presentation/PresentationSerializer.php | 50 ++++- tests/PresentationSerializerCacheKeyTest.php | 187 ++++++++++++++++++ 3 files changed, 232 insertions(+), 7 deletions(-) create mode 100644 tests/PresentationSerializerCacheKeyTest.php diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 1388b4b5c..2e2aec304 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -68,7 +68,7 @@ jobs: - { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" } # Named by path because no job in this matrix runs the tests/ root, only its # subdirectories - a file added there runs nowhere unless it is listed here. - - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php" } + - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php" } env: OTEL_SERVICE_ENABLED: false APP_ENV: testing diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index b409b6927..d7185c074 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -209,17 +209,55 @@ public function serialize($expand = null, array $fields = [], array $relations = $presentation = $this->object; if(!$presentation instanceof Presentation) return []; - // Include last_edited timestamp so a presentation update naturally busts the cache - // without needing an explicit Cache::forget — the old key just ages out via TTL. + // fields and relations are read with in_array() throughout, so their order cannot change + // the payload and sorting lets two spellings of one request share an entry. $expand is + // deliberately NOT sorted: its relations are dispatched in the order given, and the + // speakers and moderator cases both write $values['moderator'] while disagreeing about + // moderator_speaker_id, so the order is capable of changing the result. + $cache_fields = $fields; + $cache_relations = $relations; + sort($cache_fields); + sort($cache_relations); + + // The digest covers everything that shapes the payload and is not already named in the + // readable part of the key. static::class is in it because this method is inherited: + // AdminPresentationSerializer and the track-chair and CSV serializers all cache through + // here, and their merged $array_mappings add fields the public serializer never emits. + // + // INVARIANT: anything the payload depends on either appears here or stays out of the + // cache. static::class covers audience today only because the remaining differences are + // class-determined - the mappings are static, getSerializerType() returns a constant per + // class, and media_uploads, the one per-user field, is stripped before Cache::put. A + // per-user value added to the mappings, or read before parent::serialize() rather than + // after it the way AdminPresentationCSVSerializer and TrackChairPresentationSerializer + // do, would silently break that. + // + // json_encode rather than concatenation: the parts contain "_" and "," themselves + // (media_uploads, extra_questions, selection_plan), so joining them on those characters + // let distinct requests render one key. sha256 rather than md5 because the parts come + // from the query string and a collision here means serving one audience's payload to + // another - the exact failure the class component is here to prevent. + // + // last_edited stays readable so a presentation update naturally busts every entry it has + // without an explicit Cache::forget, and so an operator can still scan or drop one + // presentation's entries by pattern. $key = sprintf ( - "public_presentation_%s_%s_%s_%s_%s", + "presentation_%s_%s_%s", $presentation->getId(), $presentation->getLastEditedUTC()?->getTimestamp() ?? 0, - $expand ?? "", - implode(",",$fields), - implode(",", $relations) + hash + ( + 'sha256', + json_encode + ([ + 'serializer' => static::class, + 'expand' => $expand ?? "", + 'fields' => $cache_fields, + 'relations' => $cache_relations, + ]) + ) ); $use_cache = $params['use_cache'] ?? false; diff --git a/tests/PresentationSerializerCacheKeyTest.php b/tests/PresentationSerializerCacheKeyTest.php new file mode 100644 index 000000000..f08cc74b7 --- /dev/null +++ b/tests/PresentationSerializerCacheKeyTest.php @@ -0,0 +1,187 @@ +store = []; + // A stateful fake rather than the configured driver: it keeps the test off redis/file, + // and the entry count is the whole point of these assertions. + Cache::shouldReceive('put')->andReturnUsing(function ($key, $value, $ttl = null) { + $this->store[$key] = $value; + return true; + }); + Cache::shouldReceive('get')->andReturnUsing(function ($key, $default = null) { + return $this->store[$key] ?? $default; + }); + Cache::shouldReceive('has')->andReturnUsing(function ($key) { + return array_key_exists($key, $this->store); + }); + } + + public function tearDown(): void + { + Mockery::close(); + parent::tearDown(); + } + + /** + * @param int $identifier + * @return Presentation + */ + private function buildPresentation(int $identifier): Presentation + { + $presentation = Mockery::mock(Presentation::class); + $presentation->shouldReceive('getId')->andReturn($identifier); + $presentation->shouldReceive('getLastEditedUTC')->andReturn(null); + $presentation->shouldReceive('getTitle')->andReturn('a presentation'); + $presentation->shouldReceive('getRank')->andReturn(7); + $presentation->shouldReceive('getMediaUploads')->andReturn(new ArrayCollection([])); + $presentation->shouldReceive('getSlides')->andReturn([]); + $presentation->shouldReceive('memberCanEdit')->andReturn(false); + // Reached only by testExpandOrderIsNotNormalised. getType() answering null short-circuits + // the moderator case before it asks for a moderator, which keeps the fixture to the two + // getters the expand dispatch actually needs. + $presentation->shouldReceive('getSpeakers')->andReturn([]); + $presentation->shouldReceive('getType')->andReturn(null); + return $presentation; + } + + /** + * An unauthenticated caller: whatever ends up in the cache, this is who must not receive the + * admin shape of it. + * @return IResourceServerContext + */ + private function buildPublicContext(): IResourceServerContext + { + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn(null); + return $context; + } + + /** + * The reason this ticket exists. AdminPresentationSerializer does not override serialize(), + * so it writes through the inherited caching path, and its attribute mappings are merged on + * top of the public ones - rank, selection_status, streaming_url, etherpad_link, + * overflow_stream_key, chair scores and vote stats all land in the stored payload. With no + * class component in the key, a public caller repeating the same query params inside the TTL + * reads that payload back verbatim. + */ + public function testAdminPayloadIsNotServedToAPublicCaller() + { + $presentation = $this->buildPresentation(90201); + $context = $this->buildPublicContext(); + $arguments = [null, ['id', 'rank'], [], ['use_cache' => true]]; + + $admin = (new AdminPresentationSerializer($presentation, $context))->serialize(...$arguments); + // Sanity: the field really is admin-only, so the assertion below is about the cache and + // not about a field nobody emits. + $this->assertSame(7, $admin['rank']); + + $public = (new PresentationSerializer($presentation, $context))->serialize(...$arguments); + + $this->assertArrayNotHasKey('rank', $public); + $this->assertCount(2, $this->store); + } + + /** + * The key's parts used to be joined with "_", a character that occurs inside the values it + * joins - media_uploads, extra_questions, selection_plan, public_comments. Two different + * requests could therefore render the same key: expand=media_uploads&fields=x and + * expand=media&fields=uploads_x both flattened to "..._media_uploads_x_". + * + * Here the second request asks for a field that matches no mapping, so its correct payload is + * empty; anything it comes back with was somebody else's. + */ + public function testRequestsThatFlattenAlikeDoNotShareAnEntry() + { + $presentation = $this->buildPresentation(90202); + $context = $this->buildPublicContext(); + + $first = (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['id'], ['media_uploads'], ['use_cache' => true]); + $this->assertSame(90202, $first['id']); + + $second = (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['id_media'], ['uploads'], ['use_cache' => true]); + + $this->assertArrayNotHasKey('id', $second); + $this->assertCount(2, $this->store); + } + + /** + * fields and relations are both consumed with in_array(), so their order cannot change the + * payload. Leaving them unsorted just spends a second entry on a request already answered. + */ + public function testFieldAndRelationOrderReuseTheSameEntry() + { + $presentation = $this->buildPresentation(90203); + $context = $this->buildPublicContext(); + + (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['id', 'title'], ['slides', 'media_uploads'], ['use_cache' => true]); + (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['title', 'id'], ['media_uploads', 'slides'], ['use_cache' => true]); + + $this->assertCount(1, $this->store); + } + + /** + * expand is deliberately left alone. Its relations are dispatched in the order given, and the + * speakers and moderator cases both write $values['moderator'] while disagreeing about + * moderator_speaker_id - speakers reads it, moderator unsets it - so the order is capable of + * changing the result. Normalising it would quietly merge two payloads that are allowed to + * differ, which is the failure this whole ticket is about. + */ + public function testExpandOrderIsNotNormalised() + { + $presentation = $this->buildPresentation(90204); + $context = $this->buildPublicContext(); + + (new PresentationSerializer($presentation, $context)) + ->serialize('speakers,moderator', ['id'], [], ['use_cache' => true]); + (new PresentationSerializer($presentation, $context)) + ->serialize('moderator,speakers', ['id'], [], ['use_cache' => true]); + + $this->assertCount(2, $this->store); + } +} From c8a8a7f19587233a6726c0b64c7d6880934adfc6 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 12:40:25 -0300 Subject: [PATCH 09/10] refactor: memoize getMediaUploadsSerializerType per serializer instance withMediaUploads() asks for the type once per media upload, and getVisibleMediaUploads() asks again on top of that, so a presentation with ten uploads resolved it twelve times for one response. The method reads the auth context and then runs memberCanEdit(), which goes through the member's speaker and this presentation's speaker collection - work that cannot change between two calls on the same serializer for the same caller. Measured on a ten-upload presentation serialized with expand and relations both naming media_uploads: 12 executions of the body before, 1 after. The memo is instance-scoped, not request-scoped, and that distinction is the whole point: memberCanEdit() is answered against THIS presentation, so a caller can be a speaker on one and a stranger to the next, and a request-wide memo would hand every presentation in a list the first one's answer. SerializerRegistry builds a fresh serializer per object and none outlive the request, so per-instance is both correct and enough. Private rather than protected because TrackChairPresentationSerializer and AdminPresentationCSVSerializer override the method with a constant and have nothing to memo. Also corrects the docblock, which opened by claiming this method is kept aligned with OAuth2SummitEventsApiController::getSerializerType() and only qualified that three paragraphs later. It is deliberately narrower for service accounts, which need ReadAllPresentationMediaUploads here and nothing beyond the application type there; a reader who stopped at the first sentence would conclude the opposite. Both raised by Copilot on PR 577. --- .../Presentation/PresentationSerializer.php | 37 ++++++++++++++++--- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index d7185c074..a3eac3f55 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -28,6 +28,14 @@ class PresentationSerializer extends SummitEventSerializer { const CacheTTL = 1200; + /** + * Memo for getMediaUploadsSerializerType(). Instance-scoped on purpose - see that method. + * Private rather than protected: the subclasses that override the method answer with a + * constant and have nothing to memo. + * @var string|null + */ + private ?string $media_uploads_serializer_type = null; + protected static $array_mappings = [ 'CreatorId' => 'creator_id:json_int', 'ModeratorId' => 'moderator_speaker_id:json_int', @@ -82,10 +90,16 @@ class PresentationSerializer extends SummitEventSerializer /** * Resolves who is allowed to see every media upload attached to this presentation, approved - * or not. Kept aligned with OAuth2SummitEventsApiController::getSerializerType(), which is - * what decides the serializer type of the presentation itself - when the two disagree the - * presentation is served Private while its uploads are served Public, which is the bug this - * method used to have for summit admins and for service accounts. + * or not. The reference point is OAuth2SummitEventsApiController::getSerializerType(), which + * decides the serializer type of the presentation itself - where this method is narrower than + * that one the presentation is served Private while its uploads are served Public, which is + * the bug it used to have for summit admins and for service accounts. + * + * It is deliberately still narrower in one place. The controller grants Private to any + * ApplicationType_Service caller; here a service account additionally has to hold + * ReadAllPresentationMediaUploads, because these are unpublished files and the application + * type on its own would hand them to every service client. Members are aligned with the + * controller exactly. * * Two distinct privileged callers: * @@ -99,6 +113,17 @@ class PresentationSerializer extends SummitEventSerializer * @return string */ protected function getMediaUploadsSerializerType():string{ + // Memoized per serializer instance, which is the correct scope and not merely the + // convenient one: memberCanEdit() below is answered against THIS presentation, so a + // caller can be a speaker on one and a stranger to the next. A request-wide memo would + // hand every presentation the first one's answer. SerializerRegistry builds a fresh + // serializer per object and none outlive the request, so per-instance already collapses + // the repeated work - this method is called once per media upload plus once per + // getVisibleMediaUploads(), and it reaches the member's speaker and this presentation's + // speaker collection each time. + if (!is_null($this->media_uploads_serializer_type)) + return $this->media_uploads_serializer_type; + // && short-circuits, so getCurrentScope() is only reached for service accounts $isSnapshotClient = $this->resource_server_context->getApplicationType() === IResourceServerContext::ApplicationType_Service @@ -109,7 +134,7 @@ protected function getMediaUploadsSerializerType():string{ ); if ($isSnapshotClient) - return SerializerRegistry::SerializerType_Private; + return $this->media_uploads_serializer_type = SerializerRegistry::SerializerType_Private; $serializerType = SerializerRegistry::SerializerType_Public; $currentUser = $this->resource_server_context->getCurrentUser(); @@ -117,7 +142,7 @@ protected function getMediaUploadsSerializerType():string{ if(!is_null($currentUser) && ( $currentUser->isAdmin() || $currentUser->isSummitAdmin() || $presentation->memberCanEdit($currentUser))){ $serializerType = SerializerRegistry::SerializerType_Private; } - return $serializerType; + return $this->media_uploads_serializer_type = $serializerType; } /** From f03c3fa4cc7a004bdd10bb37719b35929987f22d Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 13:16:51 -0300 Subject: [PATCH 10/10] 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 dea76db68 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. --- .../Presentation/PresentationSerializer.php | 27 ++++++++++-------- tests/PresentationSerializerCacheKeyTest.php | 28 +++++++++++++++++++ 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index a3eac3f55..b94d37f3b 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -266,26 +266,29 @@ public function serialize($expand = null, array $fields = [], array $relations = // last_edited stays readable so a presentation update naturally busts every entry it has // without an explicit Cache::forget, and so an operator can still scan or drop one // presentation's entries by pattern. + // The parts come raw off the query string, and percent-decoding hands them over as + // bytes: a malformed sequence like %FF makes json_encode() return false, which hash() + // would silently coerce to "" - collapsing class, expand, fields and relations onto one + // shared digest per presentation, the exact cross-audience collision the digest exists + // to prevent. A request that cannot be keyed unambiguously bypasses the cache entirely. + $digest_source = json_encode + ([ + 'serializer' => static::class, + 'expand' => $expand ?? "", + 'fields' => $cache_fields, + 'relations' => $cache_relations, + ]); + $key = sprintf ( "presentation_%s_%s_%s", $presentation->getId(), $presentation->getLastEditedUTC()?->getTimestamp() ?? 0, - hash - ( - 'sha256', - json_encode - ([ - 'serializer' => static::class, - 'expand' => $expand ?? "", - 'fields' => $cache_fields, - 'relations' => $cache_relations, - ]) - ) + hash('sha256', (string) $digest_source) ); - $use_cache = $params['use_cache'] ?? false; + $use_cache = ($params['use_cache'] ?? false) && $digest_source !== false; if($use_cache){ // One read, not Cache::has() followed by Cache::get(): the entry can expire on its diff --git a/tests/PresentationSerializerCacheKeyTest.php b/tests/PresentationSerializerCacheKeyTest.php index f08cc74b7..071fd2f26 100644 --- a/tests/PresentationSerializerCacheKeyTest.php +++ b/tests/PresentationSerializerCacheKeyTest.php @@ -184,4 +184,32 @@ public function testExpandOrderIsNotNormalised() $this->assertCount(2, $this->store); } + + /** + * 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. Every request carrying any + * malformed byte then shares one digest per presentation, serializer class included, which + * is exactly the admin-payload-to-public-caller collision the class component exists to + * prevent. A request that cannot be keyed unambiguously must not touch the cache at all. + */ + public function testMalformedUtf8RequestsDoNotCollideOnOneEntry() + { + $presentation = $this->buildPresentation(90205); + $context = $this->buildPublicContext(); + + $admin = (new AdminPresentationSerializer($presentation, $context)) + ->serialize(null, ['id', 'rank', "\xFF"], [], ['use_cache' => true]); + // Sanity, mirroring testAdminPayloadIsNotServedToAPublicCaller: the admin payload + // really carries the field whose leak is asserted below. + $this->assertSame(7, $admin['rank']); + + $public = (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['id', "\xFE"], [], ['use_cache' => true]); + + // Before the guard both digests collapsed to hash("") and this came back with the + // admin-shaped payload, rank included. + $this->assertArrayNotHasKey('rank', $public); + $this->assertSame(90205, $public['id']); + } }