fix(request): correct delete permission check and await movie save

[dougrathbone]

Description

Two bugs in server/routes/request.ts:

1. Anyone can delete any pending request

The DELETE /request/:id handler used three top-level && conditions to gate deletion:

// Before
if (
  !req.user?.hasPermission(Permission.MANAGE_REQUESTS) &&
  request.requestedBy.id !== req.user?.id &&
  request.status !== 1
) {

Because all three must be true to deny, the third condition short-circuits the check whenever a request is PENDING -- meaning any authenticated user can delete any pending request they did not create. The intended behaviour is that non-admins may only retract their own requests while they are still pending.

// After
if (
  !req.user?.hasPermission(Permission.MANAGE_REQUESTS) &&
  (request.requestedBy.id !== req.user?.id ||
    request.status !== MediaRequestStatus.PENDING)
) {

Also replaces the magic number 1 with MediaRequestStatus.PENDING.

2. Movie request update is not awaited

The movie branch of PUT /request/:id called requestRepository.save() without await, returning the 200 response before the write completed. The TV branch in the same handler correctly awaits the save.

AI Disclosure: I used Claude Code to help write the tests. I reviewed and understood all generated code before submitting.

How Has This Been Tested?

New suite at server/routes/request.test.ts using the real SQLite test database via setupTestDb():

• allows the owner to delete their own pending request
• allows an admin to delete any pending request
• prevents a non-owner non-admin from deleting a pending request -- directly tests the authorization bug
• prevents the owner from deleting an approved request -- asserts the status check is enforced
• returns 404 for a non-existent request
• persists server and root folder changes to the database -- regression guard for the unawaited movie save

To reproduce bug 1 manually:

1. Log in as a non-admin user (User A) and submit a request.
2. Log in as a different non-admin user (User B) who did not create the request.
3. Send DELETE /api/v1/request/:id as User B.
4. Before this fix: the request is deleted. After this fix: 403 Forbidden.

This was independently confirmed by @fallenbagel via API before review.

Screenshots / Logs (if applicable)

N/A -- backend-only change with no UI impact.

Checklist:

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

Summary by CodeRabbit

• Bug Fixes

  • Tightened deletion authorization so requests can only be removed when allowed by status and ownership.
  • Ensured updates to movie requests are reliably persisted.

• Tests

  • Added end-to-end tests covering request deletion and update endpoints, including authorization and edge cases.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0e1b84aa-e5ab-4b9a-a18a-c60cb5d136a5

📥 Commits

Reviewing files that changed from the base of the PR and between e33c770 and 13ab307.

📒 Files selected for processing (1)

• server/routes/request.test.ts

---

📝 Walkthrough

Walkthrough

Added request route tests and tightened request route logic: new test suite for DELETE and PUT /request/:requestId, fixed an awaited save in the movie PUT handler, and refined DELETE authorization to explicitly use MediaRequestStatus.PENDING.

Changes

Cohort / File(s)	Summary
Request Route Tests server/routes/request.test.ts	New test file mounting /auth and /request, configures session, initializes test DB, provides auth/seed helpers, tests DELETE success/failure cases and PUT persistence for movie requests.
Request Route Handler server/routes/request.ts	Await requestRepository.save(request) in movie PUT handler; tighten DELETE authorization by comparing request.status to MediaRequestStatus.PENDING and adjust conditional logic to reject unauthorized deletions.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant Server as API Server
    participant Auth as Auth Middleware
    participant DB as Database

    Client->>Server: DELETE /request/:requestId
    Server->>Auth: checkUser (session)
    Auth-->>Server: user info
    Server->>DB: load MediaRequest by id
    DB-->>Server: request (owner, status)
    alt request not found
        Server-->>Client: 404 Not Found
    else user is owner OR admin AND status == PENDING
        Server->>DB: delete request
        DB-->>Server: deleted
        Server-->>Client: 204 No Content
    else
        Server-->>Client: 401 Unauthorized
    end

Loading

sequenceDiagram
    participant Client as Client
    participant Server as API Server
    participant Auth as Auth Middleware
    participant DB as Database

    Client->>Server: PUT /request/:requestId (movie with server/profile/rootFolder)
    Server->>Auth: checkUser (session)
    Auth-->>Server: user info
    Server->>DB: load MediaRequest by id
    DB-->>Server: request
    Server->>DB: apply updates
    Server->>DB: await save(request)
    DB-->>Server: saved request
    Server-->>Client: 200 OK (updated request)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix: await missing repository saves #2760 — Makes the same change to await requestRepository.save(request) in the movie PUT handler.

Suggested reviewers

• fallenbagel
• gauthier-th
• 0xSysR3ll

Poem

🐰 I hop through routes with eager cheer,
Tests arrive to make things clear,
Awaited saves and status guards,
Deletions checked by rightful cards,
A tiny rabbit's testing cheer!

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the two main changes: fixing the DELETE permission check and adding await to the movie save operation.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[dougrathbone]

@fallenbagel any chance i could get a review?

[fallenbagel]

Verified the bug via API as a non-admin user with a valid session cookie successfully deletes another user's pending request. Confirmed the permission check is broken as described.

Will review later.

[Copilot]

Pull request overview

Fixes two request-route bugs in the server API: (1) corrects authorization logic so only admins can delete any request while non-admins can only retract their own pending requests, and (2) ensures movie request updates are persisted before responding.

Changes:

• Fix DELETE /request/:id permission gating for pending vs non-pending requests (and replaces a status magic number with MediaRequestStatus.PENDING).
• Await requestRepository.save() in the movie branch of PUT /request/:id to prevent responding before the write completes.
• Add a new server/routes/request.test.ts suite covering both regressions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
server/routes/request.ts	Corrects delete authorization logic and awaits movie request persistence.
server/routes/request.test.ts	Adds regression tests for delete authorization and awaited movie update persistence.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dougrathbone]

Thanks @fallenbagel ! Sorry for the nudge, but just happy to contribute to my favourite app for the first time. Keep on rocking! 🙇

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@server/routes/request.test.ts`:
- Around line 57-61: The test mutates global settings.main.localLogin in the
before hook (via getSettings()) and never restores it, causing state leakage;
fix by capturing the original value of settings.main.localLogin when you call
getSettings() in the before hook and restore that value in an after or afterEach
hook (or use your existing settings helper to scope changes), referencing the
same symbols: createApp(), getSettings(), settings.main.localLogin, before ->
after/afterEach to ensure the global flag is returned to its prior value.

🪄 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: 5ad916d6-3d72-4ae5-bb81-2e6147124c61

📥 Commits

Reviewing files that changed from the base of the PR and between 5315b28 and e33c770.

📒 Files selected for processing (1)

• server/routes/request.test.ts