Skip to content

Conversation

@s1efhan
Copy link

@s1efhan s1efhan commented Dec 2, 2025

Description

This PR addresses two critical issues regarding quota enforcement:

  1. 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 getQuota check to the PUT /request/:id route to prevent this.

  2. Calculation Logic Fix: The getQuota method in User.ts previously calculated usage based on the createdAt date 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.createdAt date 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.ts regarding 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

  1. Created a test user with the "Advanced Request" permission (but NO admin rights).
  2. Set a strict quota for TV Shows (e.g., limit of 2 seasons per 7 days).
  3. Logged in as the test user and requested 2 seasons (quota full).
  4. Edited the pending request via the UI.
  5. Attempted to select a 3rd season.
  6. Result: The checkboxes for additional seasons were correctly disabled/blocked, enforcing the quota.

Test Scenario 2: Verify Admin override

  1. Logged in as an Admin user (with MANAGE_REQUESTS permission).
  2. Edited the request of the user from Scenario 1.
  3. Attempted to select additional seasons beyond the user's quota.
  4. Result: The quota check was bypassed as expected for admins, allowing me to add more seasons regardless of the user's limit.

Screenshots / Logs (if applicable)

Checklist:

  • I have read and followed the contribution guidelines.
  • Disclosed any use of AI (see our policy)
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • Successful build pnpm build
  • Translation keys pnpm i18n:extract
  • Database migration (if required)

@s1efhan s1efhan requested a review from a team as a code owner December 2, 2025 20:18
@s1efhan s1efhan force-pushed the fix/bypassing-request-limit branch 2 times, most recently from 32515a9 to 79ab2e8 Compare December 5, 2025 20:20
Copy link
Collaborator

@fallenbagel fallenbagel left a 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?

@s1efhan s1efhan changed the title fix(quota): enforce limits when editing requests with overrides fix(api): enforce quota limits when editing requests & fix calculation logic Dec 12, 2025
@s1efhan s1efhan force-pushed the fix/bypassing-request-limit branch from 79ab2e8 to 29f9880 Compare December 12, 2025 21:41
@s1efhan
Copy link
Author

s1efhan commented Dec 12, 2025

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?

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.

Copy link
Contributor

Copilot AI left a 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.createdAt instead of MediaRequest.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)
Copy link

Copilot AI Dec 12, 2025

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:

  1. Regular users see their own quota
  2. Admins with MANAGE_USERS permission see the quota when overriding the user
  3. 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.

Suggested change
user && !hasPermission(Permission.MANAGE_REQUESTS)
user && (!requestOverrides?.user?.id || hasPermission(Permission.MANAGE_USERS))

Copilot uses AI. Check for mistakes.
Comment on lines 320 to 322
.andWhere('request.status != :declinedStatus', {
declinedStatus: MediaRequestStatus.DECLINED,
})
Copy link

Copilot AI Dec 12, 2025

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 > :bufferDate where 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.

Copilot uses AI. Check for mistakes.
Comment on lines +565 to +575
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.',
});
}
}
Copy link

Copilot AI Dec 12, 2025

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:

  1. Non-admin users cannot exceed quota limits when editing requests to add new seasons
  2. Admin users with MANAGE_REQUESTS permission can bypass quota limits when editing
  3. The quota calculation correctly counts seasons added within the quota window
  4. 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.

Copilot uses AI. Check for mistakes.
Comment on lines +328 to +331
.where('parentRequest.id = request.id')
.andWhere('season.createdAt > :date', {
date: tvQuotaStartDate,
});
Copy link

Copilot AI Dec 12, 2025

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:

  1. Seasons added to old requests (created before the quota window) are correctly counted in the current quota usage
  2. Seasons created before the quota window (on old requests) are not counted
  3. The calculation handles edge cases like requests with mixed old/new seasons
  4. 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.

Copilot uses AI. Check for mistakes.
);

if (newSeasons.length > 0) {
if (!req.user?.hasPermission(Permission.MANAGE_REQUESTS)) {
Copy link

Copilot AI Dec 12, 2025

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:

  1. Regular users are subject to their own quota limits
  2. Admins can bypass their own quota limits
  3. When an admin requests on behalf of a regular user, that user's quota limits apply
Suggested change
if (!req.user?.hasPermission(Permission.MANAGE_REQUESTS)) {
if (!requestUser.hasPermission(Permission.MANAGE_REQUESTS)) {

Copilot uses AI. Check for mistakes.
Copy link
Contributor

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.

@s1efhan s1efhan marked this pull request as draft December 13, 2025 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

series request limit can be bypassed by users

3 participants