fix: quota bypass when editing a pending request's seasons#3240
fix: quota bypass when editing a pending request's seasons#3240kushiemoon-dev wants to merge 2 commits into
Conversation
TvRequestModal, MovieRequestModal, and CollectionRequestModal all skip fetching the requester's quota whenever requestOverrides.user.id is set and the acting user lacks MANAGE_USERS. Editing an existing request always populates requestOverrides.user with the request's original owner (regardless of the acting user's permissions), so this condition was true for essentially every edit - meaning quota was silently never fetched, and the UI never disabled submission once the user's season limit was reached. A user at their quota could open a pending request and add further seasons past their limit. This also affected the new admin "Bypass User Quota" checkbox added in #2026: its visibility depends on quota.tv.limit / quota.movie.limit being loaded, so it silently failed to appear whenever this same condition suppressed the fetch. The fix removes the permission-gated condition and always fetches quota for the effective target user. This doesn't weaken anything: User.getQuota() already returns an unrestricted quota server-side for users with MANAGE_USERS, so admins editing their own request are unaffected; the change only ensures quota is actually loaded (and enforced) for the common case of a regular user editing their own request. The corresponding server-side gap is fixed as well: PUT /request/:id recomputed which seasons were being added but never checked them against the requester's quota, so the limit could already be bypassed directly via the API regardless of the frontend. It now runs the same quota check used on request creation, including support for the explicit ignoreQuota flag from #2026 (an admin must opt in via ignoreQuota: true - having MANAGE_REQUESTS alone no longer grants a silent bypass), and persists request.ignoreQuota so the seasons added this way are correctly excluded from future quota calculations. Added test coverage in request.test.ts for: a regular user exceeding their quota on edit, an admin edit without the explicit flag (still enforced), a non-admin attempting to set the flag themselves (rejected), and an admin using the flag to legitimately bypass quota. Fixes #633
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTV request season edits now enforce per-user quotas and permission-gated bypasses. Tests cover rejection and persistence behavior, while request modals fetch quota data for the effective request user. ChangesTV request quota flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestRoute
participant UserQuota
Client->>RequestRoute: PUT request with new seasons
RequestRoute->>UserQuota: Get target user quota
UserQuota-->>RequestRoute: Return TV quota limit
RequestRoute-->>Client: Return updated request or 403 quota error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
kushiemoon-dev
changed the title
![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)
server/routes/request.ts (1)
567-599: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccount for seasons removed from the same request before enforcing TV quota.
getQuota()counts persistedSeasonRequestrows, so checkingnewSeasons.lengthbeforerequest.seasonsis pruned can reject a pure season swap even when the final season count stays flat. Compare against the net delta or run the quota check after applying removals.🤖 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 `@server/routes/request.ts` around lines 567 - 599, Update the season quota logic around newSeasons and request.seasons so removed seasons in the same request are included before enforcing TV quota. Calculate the net increase against the seasons retained by the request, or apply the filtered seasons before calling getQuota, ensuring pure season swaps with no final count increase are allowed.
🤖 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/components/RequestModal/TvRequestModal.tsx`:
- Around line 91-93: Update the quota-loading flow near useSWR and the
okDisabled safeguards so users with MANAGE_REQUESTS but without MANAGE_USERS do
not rely on an undefined quota after a 403. Either skip the quota request for
that role or surface the fetch error and keep the modal’s quota safeguards
enforced; preserve normal quota display and season-limit checks for authorized
users.
---
Outside diff comments:
In `@server/routes/request.ts`:
- Around line 567-599: Update the season quota logic around newSeasons and
request.seasons so removed seasons in the same request are included before
enforcing TV quota. Calculate the net increase against the seasons retained by
the request, or apply the filtered seasons before calling getQuota, ensuring
pure season swaps with no final count increase are allowed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bba59c88-dc9f-4c2e-b59a-1312a95df721
📒 Files selected for processing (5)
server/routes/request.test.tsserver/routes/request.tssrc/components/RequestModal/CollectionRequestModal.tsxsrc/components/RequestModal/MovieRequestModal.tsxsrc/components/RequestModal/TvRequestModal.tsx
| const { data: quota } = useSWR<QuotaResponse>( | ||
| user && | ||
| (!requestOverrides?.user?.id || hasPermission(Permission.MANAGE_USERS)) | ||
| ? `/api/v1/user/${requestOverrides?.user?.id ?? user.id}/quota` | ||
| : null | ||
| user ? `/api/v1/user/${requestOverrides?.user?.id ?? user.id}/quota` : null | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the modal and related backend permission checks.
git ls-files | rg 'src/components/RequestModal/TvRequestModal\.tsx|src/components/RequestModal/.*RequestModal\.tsx|api/v1/user/.*/quota|quota|MANAGE_USERS|MANAGE_REQUESTS|permission'
# Show an outline of the target modal before reading focused slices.
ast-grep outline src/components/RequestModal/TvRequestModal.tsx --view expanded || true
# Read the relevant ranges in the modal.
sed -n '1,260p' src/components/RequestModal/TvRequestModal.tsx
# Search for the quota endpoint / permission checks across the repo.
rg -n "GET /.*quota|/quota|MANAGE_USERS|MANAGE_REQUESTS|AND-permission|permission.*quota|quota.*permission|GET.*quota" src .Repository: seerr-team/seerr
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the quota route and nearby permission logic.
sed -n '780,840p' server/routes/user/index.ts
# Inspect the TvRequestModal branches that depend on quota.
sed -n '380,500p' src/components/RequestModal/TvRequestModal.tsx
sed -n '680,740p' src/components/RequestModal/TvRequestModal.tsx
# Compare with the other request modals' quota handling.
sed -n '40,120p' src/components/RequestModal/MovieRequestModal.tsx
sed -n '40,120p' src/components/RequestModal/CollectionRequestModal.tsx
# Inspect the shared quota display component.
sed -n '1,220p' src/components/RequestModal/QuotaDisplay.tsxRepository: seerr-team/seerr
Length of output: 13582
Quota fetch 403 leaves quota safeguards off for some admins src/components/RequestModal/TvRequestModal.tsx:91-93
For users with MANAGE_REQUESTS but not MANAGE_USERS, /api/v1/user/:id/quota returns 403, so quota stays undefined. That hides QuotaDisplay and bypasses the season-limit checks in okDisabled, letting the modal accept selections that the backend rejects on submit. Surface the quota error or skip the fetch for that role.
🤖 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/components/RequestModal/TvRequestModal.tsx` around lines 91 - 93, Update
the quota-loading flow near useSWR and the okDisabled safeguards so users with
MANAGE_REQUESTS but without MANAGE_USERS do not rely on an undefined quota after
a 403. Either skip the quota request for that role or surface the fetch error
and keep the modal’s quota safeguards enforced; preserve normal quota display
and season-limit checks for authorized users.
CodeRabbit flagged that newSeasons.length was compared against quota.tv.remaining without crediting back seasons the same edit drops, since getQuota() counts persisted rows before the save. A same-count season swap at full quota was rejected even though the net season total wouldn't change.
|
Right — |
kushiemoon-dev
changed the title
|
Hey, i thought the quality of the code in plus of explanation was not enought good, i will rework on it and try to make that better 😅 could re open one if you want but i thought the quality itself was not great |
|
Reopened as #3265. |
kushiemoon-dev
changed the title
1 participant
Description
Editing a pending TV request's seasons didn't re-check the requester's quota against the new season list, letting a user (or an admin editing on their behalf) add seasons past the quota limit. The three request modals also skipped fetching quota for non-managers editing an existing request.
Superseded by #3265 (same fix, rebased onto current develop).
Fixes #633