fix(security): enforce media ownership checks on destroy/invalidate endpoints (IDOR fix) - #158
Conversation
|
@teethaking Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds a Media model for ownership, records ownership during upload and event creation, and enforces ownership-based authorization for media invalidation and deletion. Tests cover the new validation and access-control paths. ChangesMedia Ownership and Access Control
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/event-ticket.service.ts (1)
303-329: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid creating events without their ownership mapping.
EventTicket.create()commits before theMedia.findOneAndUpdate(). If the media upsert fails, the method throws “Failed to create event…” even though the event already exists, leaving inconsistent state and potentially blocking later media deletion authorization.Consider wrapping both writes in a MongoDB session transaction, or make the Media write idempotently recoverable without reporting the whole event creation as failed.
🤖 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 `@src/services/event-ticket.service.ts` around lines 303 - 329, The event creation flow in EventTicket.create currently commits the EventTicket before the Media.findOneAndUpdate ownership mapping, so a media upsert failure can leave a partially created event and a misleading “Failed to create event” error. Update the event-creation path in event-ticket.service.ts to make the EventTicket.create and Media.findOneAndUpdate steps atomic, ideally by using a MongoDB session transaction around the create-and-map sequence, or otherwise make the media ownership write safely idempotent and recoverable without failing the whole create operation. Make sure the fix is applied in the same create flow that uses baseEventData and event.cloudinary_public_id.
🧹 Nitpick comments (3)
tests/media.controller.test.ts (3)
35-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the non-existent
publicIdauthorization case.The PR objective says invalid/non-existent media IDs must return
403, but neither suite covers the branch where bothMedia.findOne(...)and theEventTicket.findOne(...)fallback returnnull. Right now that requirement is unguarded by tests.🤖 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 `@tests/media.controller.test.ts` around lines 35 - 192, Add a test for the missing authorization path in destroyMedia/invalidateMedia where the requested publicId does not exist anywhere: mock both Media.findOne and the EventTicket.findOne fallback to return null, then assert the handler returns 403 with the forbidden response instead of proceeding to MediaService.destroy or MediaService.invalidate. Use the existing destroyMedia and invalidateMedia test blocks so the new case covers the unguarded branch.
194-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlease cover the successful upload ownership write.
This file only tests the
400precondition path, but the IDOR fix depends onuploadMediapersisting{ publicId, userId }after a successful upload. Without asserting theMedia.findOneAndUpdate(...)call, the main ownership-tracking contract can regress silently.🤖 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 `@tests/media.controller.test.ts` around lines 194 - 206, Add coverage for the successful `uploadMedia` path so the ownership-tracking contract is verified, not just the 400 precondition. Update the `uploadMedia` test suite to mock a successful upload and assert that `Media.findOneAndUpdate(...)` is called with the uploaded `publicId` and the authenticated `userId`, confirming the `{ publicId, userId }` persistence after success. Use the existing `uploadMedia` handler and `Media.findOneAndUpdate` symbol to place the assertion in the same `describe('uploadMedia')` block.
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
resetAllMocks()here instead ofclearAllMocks().
clearAllMocks()preserves priormockResolvedValue()implementations on the module-mockedMedia,EventTicket, andMediaServiceobjects, so later tests can inherit behavior from earlier ones. That makes this suite order-dependent.Suggested change
beforeEach(() => { - jest.clearAllMocks(); + jest.resetAllMocks(); jest.spyOn(console, 'error').mockImplementation(() => {}); });🤖 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 `@tests/media.controller.test.ts` around lines 26 - 33, The test setup in beforeEach is leaving prior mock implementations intact, which makes the media controller suite order-dependent. Update the reset logic to use resetAllMocks() instead of clearAllMocks() alongside the existing console.error spy in media.controller.test.ts, so module-mocked Media, EventTicket, and MediaService do not carry mockResolvedValue behavior into later tests.
🤖 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 `@src/models/media.ts`:
- Line 10: The Media schema currently allows ownership records without a userId
even though IMedia.userId is required, which can break verifyMediaOwnership when
it assumes mediaRecord.userId exists. Update the userId field in the Media
schema definition to be required so all Media documents always have an owner,
and keep the schema aligned with the IMedia model and verifyMediaOwnership
usage.
---
Outside diff comments:
In `@src/services/event-ticket.service.ts`:
- Around line 303-329: The event creation flow in EventTicket.create currently
commits the EventTicket before the Media.findOneAndUpdate ownership mapping, so
a media upsert failure can leave a partially created event and a misleading
“Failed to create event” error. Update the event-creation path in
event-ticket.service.ts to make the EventTicket.create and
Media.findOneAndUpdate steps atomic, ideally by using a MongoDB session
transaction around the create-and-map sequence, or otherwise make the media
ownership write safely idempotent and recoverable without failing the whole
create operation. Make sure the fix is applied in the same create flow that uses
baseEventData and event.cloudinary_public_id.
---
Nitpick comments:
In `@tests/media.controller.test.ts`:
- Around line 35-192: Add a test for the missing authorization path in
destroyMedia/invalidateMedia where the requested publicId does not exist
anywhere: mock both Media.findOne and the EventTicket.findOne fallback to return
null, then assert the handler returns 403 with the forbidden response instead of
proceeding to MediaService.destroy or MediaService.invalidate. Use the existing
destroyMedia and invalidateMedia test blocks so the new case covers the
unguarded branch.
- Around line 194-206: Add coverage for the successful `uploadMedia` path so the
ownership-tracking contract is verified, not just the 400 precondition. Update
the `uploadMedia` test suite to mock a successful upload and assert that
`Media.findOneAndUpdate(...)` is called with the uploaded `publicId` and the
authenticated `userId`, confirming the `{ publicId, userId }` persistence after
success. Use the existing `uploadMedia` handler and `Media.findOneAndUpdate`
symbol to place the assertion in the same `describe('uploadMedia')` block.
- Around line 26-33: The test setup in beforeEach is leaving prior mock
implementations intact, which makes the media controller suite order-dependent.
Update the reset logic to use resetAllMocks() instead of clearAllMocks()
alongside the existing console.error spy in media.controller.test.ts, so
module-mocked Media, EventTicket, and MediaService do not carry
mockResolvedValue behavior into later tests.
🪄 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: 1cd96d62-4fcf-4a07-8c08-3d8c3779421a
📒 Files selected for processing (4)
src/controllers/media.controller.tssrc/models/media.tssrc/services/event-ticket.service.tstests/media.controller.test.ts
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 `@src/services/event-ticket.service.ts`:
- Around line 331-339: Prevent media ownership from being reassigned during
event creation. In event-ticket.service.ts, the Media.findOneAndUpdate call in
the event creation flow currently upserts by publicId and writes userId from
baseEventData.organizedBy, which can transfer ownership. Change this logic so it
only verifies an existing Media record already belongs to the organizer (using
the existing publicId plus organizer identity) and do not set or overwrite
userId here; if no matching owned media exists, reject the operation instead of
upserting.
🪄 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: 62328fbc-c310-486b-b2be-59e8819f949e
📒 Files selected for processing (3)
src/models/media.tssrc/services/event-ticket.service.tstests/media.controller.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/models/media.ts
- tests/media.controller.test.ts
- Make userId required in Media schema to match IMedia interface - Wrap EventTicket.create and Media ownership check in MongoDB session transaction - Verify existing Media belongs to organizer instead of upserting (prevents ownership transfer) - Reject event creation if media doesn't exist or isn't owned by organizer - Add test for non-existent publicId authorization path in destroyMedia/invalidateMedia - Add test for successful uploadMedia path with Media.findOneAndUpdate assertion - Use resetAllMocks in beforeEach to prevent mock bleed between tests
|
Note We couldn't fetch the incremental changes for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Summary
destroyMediaandinvalidateMediainmedia.controller.tswere protected byauthGuardbut never verified that the authenticated user actually owned the media being targeted. Any authenticated user could pass an arbitrarypublicIdto/destroyor/invalidateand delete any asset in the Cloudinary account — a textbook Insecure Direct Object Reference (IDOR). This PR adds ownership verification so users can only destroy or invalidate media they own.Closes #132
What changed
Database Schema
publicIdis now associated with theuserIdof the uploader at upload timesrc/controllers/media.controller.tsdestroyMediaandinvalidateMedianow look up the media record bypublicIdand verify the requesting user'suserIdmatches the recorded owner before proceedingpublicIddoes not exist or does not belong to the requesting user, the endpoint returns403 Forbiddeninstead of proceeding with destruction/invalidationsrc/routes/media.route.tsauthGuardas before; ownership check is layered on top in the controller rather than replacing existing auth middlewareTests
403 Forbidden, and the underlying Cloudinary asset is untouchedpublicIdis handled gracefully without leaking whether the asset exists for another userHow to verify
/destroywith the resultingpublicId— assert success/destroyor/invalidatewith User A'spublicId— assert403 Forbiddenand that the asset still exists in CloudinaryuserIdownership for newly uploaded mediaChecklist
publicId→userId) tracked in the databasedestroyMediaverifies ownership before calling CloudinaryinvalidateMediaverifies ownership before calling Cloudinary403 ForbiddenSummary by CodeRabbit
403 Forbiddenresponses when access checks fail.