fix(security): enforce studio ownership on movie lookups (Closes #251) - #445
Merged
Merged
Conversation
|
@SakethSumanBathini is attempting to deploy a commit to the srv30's projects Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #251
Six sites, not four
The issue names
releaseMovie,getMovieDetails,getMovieTrackingandaddMarketingCampaign. Auditing everyMovielookup in the controller turned up two more with the identical flaw:releaseMoviescheduleReleasegetMovieDetailsgetMovieTrackingaddMarketingCampaignreReleaseMovieAll six used
Movie.findById(id)with no ownership filter, so any authenticated user could read or act on any movie by ID.addMarketingCampaignwas the worst of themIt fetched the movie with no check, then fetched the caller's studio and deducted funds from it:
So you could pay to promote a rival studio's film — boosting their box office with your money — or drain your own balance by repeating the call against any ID. The order is now inverted: the studio is resolved first, inside the transaction, and the movie query filters on it.
The fix follows the pattern already in this file
getReleasedMoviesat line 327 already did this correctly:Ownership is enforced in the query, not after it. Two consequences worth naming:
if (movie.studioId !== studio._id) return 403would leak existence.releaseMovieandreReleaseMovieuseownerStudiofor the check because both already declare a separatestudiolater — a full mutable document they need for.save(). Keeping the names distinct avoids shadowing.Verification
node --checkclean on the modified file.Movie.findByIdcalls in the controller; all six lookups now carrystudioId.I couldn't run the backend suite:
tests/testScreening.test.jsneedsMongoMemoryServer, which downloads a MongoDB binary my environment can't reach. It fails identically on unmodifiedelusoc, so it's an environment limit rather than a regression — but a live run against a real database is worth doing before merge.Note on PR #329
@Sairaj2033's #329 targets this issue and is still open. I raised the overlap with @SRV30 before starting and was asked to proceed. That PR also carries three unrelated fixes (route shadowing, notification error handling, spy transactions) which this PR does not touch, so those remain worth taking from it independently.