fix: gate public presentation serialization on display_on_site for media uploads - #577
fix: gate public presentation serialization on display_on_site for media uploads#577JpMaxMan wants to merge 1 commit into
Conversation
…dia 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.
📝 WalkthroughWalkthrough
ChangesPresentation media visibility
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-577/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/ModelSerializers/Summit/Presentation/PresentationSerializer.php`:
- Line 153: Update PresentationSerializer cache-key generation to distinguish
public output from private/admin output resolved through
AdminPresentationSerializer, using a private serializer-specific key for private
data. Ensure cached relation IDs still pass through the appropriate visibility
rules before being returned, including the logic around getVisibleMediaUploads
and the related cache handling at the referenced later section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d0d53ee-95bd-4518-867f-495fd7dd1e9b
📒 Files selected for processing (1)
app/ModelSerializers/Summit/Presentation/PresentationSerializer.php
| $media_uploads = []; | ||
|
|
||
| foreach ($presentation->getMediaUploads() as $mediaUpload) { | ||
| foreach ($this->getVisibleMediaUploads() as $mediaUpload) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Expect private/editor callers to bypass this cache, or cache keys to include serializer type.
rg -n -C 12 'PresentationSerializer|public_presentation_|use_cache' .Repository: OpenStackweb/summit-api
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the relevant PresentationSerializer paths and all passing of use_cache.
printf '--- PresentationSerializer relevant lines ---\n'
sed -n '1,240p;400,430p' app/ModelSerializers/Summit/Presentation/PresentationSerializer.php | cat -n
printf '\n--- use_cache usages (concise) ---\n'
rg -n --glob '!adr/**' 'use_cache|SerializerRegistry::getInstance\(\)->getSerializer\(|serialize\(' app tests tests-api || trueRepository: OpenStackweb/summit-api
Length of output: 252
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the relevant PresentationSerializer paths and all passing of use_cache.
printf '%s\n' '--- PresentationSerializer relevant lines ---'
sed -n '1,240p;400,430p' app/ModelSerializers/Summit/Presentation/PresentationSerializer.php | cat -n
printf '%s\n' ''
printf '%s\n' '--- use_cache usages (concise) ---'
rg -n --glob '!adr/**' 'use_cache|SerializerRegistry::getInstance\(\)->getSerializer\(|serialize\(' app tests tests-api || trueRepository: OpenStackweb/summit-api
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- exact use_cache references outside tests ---'
rg -n --glob '!adr/**' --glob '!tests/**' 'use_cache' app
printf '%s\n' ''
printf '%s\n' '--- exact use_cache references in tests ---'
rg -n 'use_cache' tests || true
printf '%s\n' ''
printf '%s\n' '--- parent cache key helpers/usages ---'
rg -n --glob '!adr/**' 'build.*[Cc]ache[Kk]ey|get.*Cache.*Key|Cache::put|Cache::has' app/ModelSerializers | head -n 120Repository: OpenStackweb/summit-api
Length of output: 1247
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- OAuth2SummitEventsApiController cache calls context ---'
sed -n '520,700p' app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php | cat -n
printf '%s\n' ''
printf '%s\n' '--- route/controller declarations around protected events ---'
rg -n --glob '!adr/**' --glob '!tests/**' 'OAuth2SummitEventsApiController|Protected/Summit|summit/events|/presentations' app/routes app/Http/Controllers/Apis app/Http/Controllers -g '*.php' | head -n 200Repository: OpenStackweb/summit-api
Length of output: 40762
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- SerializerRegistry resolved serializer and toArray ---'
sed -n '750,835p' app/ModelSerializers/SerializerRegistry.php | cat -n
printf '%s\n' ''
printf '%s\n' '--- OAuth2SummitEventsApiController serializer type logic ---'
sed -n '86,150p' app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php | cat -nRepository: OpenStackweb/summit-api
Length of output: 7021
Keep private presentation cache keys isolated.
Private/admin endpoints pass use_cache => true and are resolved to AdminPresentationSerializer, while cached PresentationSerializer output key is only "public_presentation_...". Store private data under a private-serializer cache key and apply visibility rules to cached relation IDs.
Also applies to: 215-219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ModelSerializers/Summit/Presentation/PresentationSerializer.php` at line
153, Update PresentationSerializer cache-key generation to distinguish public
output from private/admin output resolved through AdminPresentationSerializer,
using a private serializer-specific key for private data. Ensure cached relation
IDs still pass through the appropriate visibility rules before being returned,
including the logic around getVisibleMediaUploads and the related cache handling
at the referenced later section.
| * public events/published endpoints. | ||
| * @return \Doctrine\Common\Collections\Collection|PresentationMediaUpload[] | ||
| */ | ||
| protected function getVisibleMediaUploads() |
There was a problem hiding this comment.
@JpMaxMan This filter is an off switch rather than a draft gate, because display_on_site is false on essentially every existing row.
Nothing in the submission flow ever sets it true. PresentationMaterial::__construct (PresentationMaterial.php:190) defaults it false, PresentationMediaUploadFactory::populate (:39-40) only calls setDisplayOnSite when the key is present in the payload, and call-for-presentations - the app speakers actually upload through - has zero occurrences of display_on_site in src/. The only writer is summit-admin's material form checkbox, which itself starts unchecked (event-material-reducer.js:30).
Concrete failure: pub-api's snapshot job requests expand=media_uploads with a client_credentials token, which resolves Public here, so every media_uploads array in events.json and presentations.json comes back empty. pub-api does not validate response shape - it only raises on transport errors - so the snapshot publishes normally and SnapshotCompleted fires. dropbox-materializer then iterates sess["media_uploads"] with no filter of its own and stages nothing, for every session, silently.
Suggested: get a production count first -
SELECT DisplayOnSite, COUNT(*) FROM PresentationMaterial
WHERE ClassName = 'PresentationMediaUpload' GROUP BY DisplayOnSite;and gate this behind a backfill plus a defined write path for the flag. Also worth a test pinning both branches - the PR's own test plan is still unchecked, and this is exactly the case where behaviour depends entirely on data.
| { | ||
| $presentation = $this->object; | ||
| $mediaUploads = $presentation->getMediaUploads(); | ||
| if ($this->getMediaUploadsSerializerType() === SerializerRegistry::SerializerType_Private) { |
There was a problem hiding this comment.
@JpMaxMan Delegating to getMediaUploadsSerializerType() inherits a privilege gap this PR does not close: that method checks only isAdmin() || memberCanEdit(), so summit admins and service accounts both land on Public.
Compare OAuth2SummitEventsApiController::getSerializerType() (:127), which already resolves Private for isSummitAdmin() and for ApplicationType_Service. The presentation is served Private to those callers while its media uploads are served Public - the two decisions disagree.
Concrete failure, and it is circular: an operator in summit-front-end-administrators loads the summit-admin event grid, which requests media_uploads.display_on_site among its fields (event-actions.js:288) against GET /api/v1/summits/{id}/events. With this filter in place those rows disappear, so the operator cannot see the upload in order to tick the very checkbox that would make it visible again. The same gap is why pub-api's snapshot client gets Public output despite that controller granting service accounts Private.
Suggested: align getMediaUploadsSerializerType() with the check the controller already performs - Private for isSummitAdmin() as well as isAdmin(), and for service accounts. The application-type pattern has serializer-layer precedent at PresentationSpeakerBaseSerializer.php:104-109. For the service-account half I would require a dedicated snapshot scope rather than ApplicationType_Service alone, so this does not hand drafts to every service client.
| { | ||
| $media_uploads = []; | ||
| foreach ($presentation->getMediaUploads() as $mediaUpload) { | ||
| foreach ($this->getVisibleMediaUploads() as $mediaUpload) { |
There was a problem hiding this comment.
@JpMaxMan This call site never executes when caching is on, so the relations id-list path stays unfiltered.
The cached branch above returns at line 170, and on a cache hit it only recomputes media_uploads for the expand path. A request passing relations=media_uploads without expand=media_uploads gets the cached id array verbatim - built by whichever caller populated the key, filtered or not.
use_cache => true is set on the voteable-presentations endpoints (OAuth2SummitEventsApiController.php:554, :616, :685), which is where pub-api's presentations.json feed goes, so this is a live path rather than a theoretical one.
Suggested: handle the relations case inside the cache-hit branch the same way expand is handled, or drop media_uploads from the cached payload entirely and always recompute it after the cache lookup.
| $media_uploads = []; | ||
|
|
||
| foreach ($presentation->getMediaUploads() as $mediaUpload) { | ||
| foreach ($this->getVisibleMediaUploads() as $mediaUpload) { |
There was a problem hiding this comment.
@JpMaxMan Pre-existing rather than introduced here, but it undercuts this PR's premise, so flagging it on the branch you are touching: the cache key is not privilege-scoped.
It is built from presentation id, last_edited, expand, fields and relations with no serializer-type or user component, while getAllVoteablePresentations passes $this->getSerializerType(), which resolves Private or Public per caller. For one combination of params, a payload built by a Private caller can be served to a Public one, and vice versa.
Recomputing media_uploads here covers this PR's field, but every other field the Admin serializer contributes still comes straight off the shared key. So "Public callers only ever see approved uploads" is not guaranteed by construction while this stands.
Suggested: add the resolved serializer type to the key. Worth its own ticket rather than expanding this PR's scope.
2 participants
Summary
Public/anonymous callers to the events/published endpoints could pull full
PresentationMediaUploaddata — including live public S3 URLs — for draft (display_on_site=false) uploads via?expand=media_uploads, becausePresentationSerializerread the unfiltered media uploads collection with no visibility check.Reported externally: a third party's calendar/speaker-scraping AI agent recovered pre-event draft slide decks this way, with no malicious intent — it just added
expand=media_uploadsto a normal public schedule call.ClickUp: https://app.clickup.com/t/86bb7zx8t
Root cause
GET /summits/{id}/events/publishedandGET /summits/{id}/events/{event_id}/publishedare fully public/unauthenticated and forward a caller-suppliedexpandwith no allowlist.PresentationSerializer::serialize()handledexpand=media_uploads(and therelationsid-list path) by callingPresentation::getMediaUploads()— unfiltered — and serializing every attached upload'spublic_urlregardless of draft state.display_on_siteexists onPresentationMaterial/PresentationMediaUploadfor exactly this distinction (defaultsfalse), andSummitMediaUploadType::getMediaUploadsToDisplayOnSite()was even built to filter on it — but had zero callers in the API/serializer stack.Fix
PresentationSerializernow resolves media uploads through a newgetVisibleMediaUploads()helper: when the resolved serializer type is Public (the same admin/editor privilege checkgetMediaUploadsSerializerType()already performs), it filters to onlydisplay_on_site=trueuploads before serializing. Applied at all three call sites in this file that readgetMediaUploads()(cached-expand branch, plainrelationsid-list branch, main expand branch).AdminPresentationCSVSerializer(admin-only CSV export) is intentionally untouched.Out of scope / follow-up (tracked in the ClickUp ticket)
use_temporary_links_on_public_storage+ the existingpresentations-regenerate-media-uploads-temporal-public-urlscommand, or a longer-term private-storage migration) is a separate ops decision for @smarcet.Test plan
?expand=media_uploadsonGET /summits/{id}/events/{id}/published(unauthenticated) no longer returns uploads withdisplay_on_site=falseAdminPresentationCSVSerializerexport still includes all uploads regardless ofdisplay_on_siteSummary by CodeRabbit