-
-
Notifications
You must be signed in to change notification settings - Fork 545
fix(api): enforce quota limits when editing requests & fix calculation logic #2190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
32515a9 to
79ab2e8
Compare
fallenbagel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only fixes it in the frontend. Vulnerability still exists at the API level.
The PUT /:requestId endpoint still doesn’t validate the quotas when adding new seasons. That means anyone with REQUEST_ADVANCED perm can skip the frontend entirely and hit the API directly to get around the limits.
Could you add the same quota validation on the backend so the API can’t be used to bypass it as well?
79ab2e8 to
29f9880
Compare
Yes! You're right. I fixed it in the API Code now. But while testing found another Bug I had to fix to get a satisfying result. I discovered a flaw in the underlying quota calculation: It was checking the createdAt date of the parent Request rather than the Seasons. Which meant that adding new seasons to an old request wouldn't count towards the current limit allowing a bypass. I’ve updated the query in User.ts to filter by SeasonRequest.createdAt instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses critical quota enforcement vulnerabilities by adding quota checks when editing TV requests and fixing the quota calculation logic to correctly count seasons based on their creation date rather than the parent request's creation date.
Key Changes:
- Added quota enforcement to the edit request API endpoint to prevent users from bypassing limits
- Fixed quota calculation to filter by
SeasonRequest.createdAtinstead ofMediaRequest.createdAt - Updated frontend quota fetching logic in TvRequestModal
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/components/RequestModal/TvRequestModal.tsx | Modified quota fetching condition to skip quota for users with MANAGE_REQUESTS permission |
| server/routes/request.ts | Added relations loading, quota fetching, and enforcement logic to prevent quota bypass when editing requests |
| server/entity/User.ts | Changed quota calculation to filter seasons by their creation date instead of parent request creation date |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { data: quota } = useSWR<QuotaResponse>( | ||
| user && | ||
| (!requestOverrides?.user?.id || hasPermission(Permission.MANAGE_USERS)) | ||
| user && !hasPermission(Permission.MANAGE_REQUESTS) |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quota fetching logic is inconsistent with other request modals (MovieRequestModal and CollectionRequestModal). When requestOverrides.user.id is set (i.e., when an admin is changing the request user), the quota for that overridden user should still be fetched, not skipped entirely.
The correct condition should be: user && (!requestOverrides?.user?.id || hasPermission(Permission.MANAGE_USERS))
This ensures that:
- Regular users see their own quota
- Admins with MANAGE_USERS permission see the quota when overriding the user
- Admins with MANAGE_REQUESTS bypass quota entirely only when NOT overriding the user
The current logic skips quota fetching for all users with MANAGE_REQUESTS permission, even when they're overriding to request on behalf of another user who should be subject to quota limits.
| user && !hasPermission(Permission.MANAGE_REQUESTS) | |
| user && (!requestOverrides?.user?.id || hasPermission(Permission.MANAGE_USERS)) |
| .andWhere('request.status != :declinedStatus', { | ||
| declinedStatus: MediaRequestStatus.DECLINED, | ||
| }) |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the request.createdAt filter may cause performance issues for users with many historical TV requests. The query now retrieves ALL TV requests for a user (potentially hundreds or thousands), then filters seasons by date in a subquery.
Consider adding back a broader date filter on the request level (e.g., including a buffer period) to limit the initial result set, while still keeping the precise season.createdAt filter in the subquery. For example:
- Add a request-level filter with a reasonable buffer:
request.createdAt > :bufferDatewhere bufferDate is (tvQuotaDays + reasonable buffer like 90 days) - Keep the precise season.createdAt filter in the subquery
This would maintain correctness while improving query performance for users with extensive request histories.
| if (!req.user?.hasPermission(Permission.MANAGE_REQUESTS)) { | ||
| if ( | ||
| (quota?.tv?.limit ?? 0) > 0 && | ||
| newSeasons.length > (quota?.tv?.remaining ?? 0) | ||
| ) { | ||
| return next({ | ||
| status: 403, | ||
| message: 'Request exceeds series quota limit.', | ||
| }); | ||
| } | ||
| } |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new quota enforcement logic in the edit request flow lacks test coverage. Consider adding tests to verify:
- Non-admin users cannot exceed quota limits when editing requests to add new seasons
- Admin users with MANAGE_REQUESTS permission can bypass quota limits when editing
- The quota calculation correctly counts seasons added within the quota window
- Edge cases like editing old requests to add new seasons
This is particularly important given the security-sensitive nature of quota enforcement and the bypass vulnerability that this PR addresses.
| .where('parentRequest.id = request.id') | ||
| .andWhere('season.createdAt > :date', { | ||
| date: tvQuotaStartDate, | ||
| }); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The modified quota calculation logic lacks test coverage to verify the fix works correctly. Consider adding unit tests to verify:
- Seasons added to old requests (created before the quota window) are correctly counted in the current quota usage
- Seasons created before the quota window (on old requests) are not counted
- The calculation handles edge cases like requests with mixed old/new seasons
- Performance is acceptable with varying numbers of historical requests
This is particularly critical since the logic change addresses a security vulnerability where users could bypass quotas.
| ); | ||
|
|
||
| if (newSeasons.length > 0) { | ||
| if (!req.user?.hasPermission(Permission.MANAGE_REQUESTS)) { |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quota enforcement logic should check the permissions of requestUser (the user for whom the request is being made), not req.user (the user making the API call). When an admin changes the userId via requestOverrides, they may be requesting on behalf of another user who is subject to quota limits.
The current logic allows an admin to bypass quota limits even when requesting on behalf of a non-admin user. The check should be:
if (!requestUser.hasPermission(Permission.MANAGE_REQUESTS))
This ensures that:
- Regular users are subject to their own quota limits
- Admins can bypass their own quota limits
- When an admin requests on behalf of a regular user, that user's quota limits apply
| if (!req.user?.hasPermission(Permission.MANAGE_REQUESTS)) { | |
| if (!requestUser.hasPermission(Permission.MANAGE_REQUESTS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, because unless this PR #2026 is merged, admins should not be able to bypass user's quota.
Description
This PR addresses two critical issues regarding quota enforcement:
Enforcement on Edit (API): previously, the quota check was only performed when creating a new request. Users could bypass their limits by creating a request for 1 season and then editing it to add more seasons. I added a
getQuotacheck to thePUT /request/:idroute to prevent this.Calculation Logic Fix: The
getQuotamethod inUser.tspreviously calculated usage based on thecreatedAtdate of the parent Request. This meant if a user added seasons (via edit) to an old request (created before the quota window), those new seasons were not counted towards the current quota.I updated the query to filter based on the
SeasonRequest.createdAtdate instead. This ensures that any season added within the quota window counts, regardless of when the original request was created.AI Disclosure
I discussed the logic flaw in
User.tsregarding the "Request date vs. Season date" with an AI (Gemini). The implementation and testing were done by me.How Has This Been Tested?
I tested these changes locally on macOS using Safari and Node v22.
Test Scenario 1: Verify the fix
Test Scenario 2: Verify Admin override
MANAGE_REQUESTSpermission).Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extract