github-starred/actual
2
0
Fork 0
mirror of https://github.com/actualbudget/actual.git synced 2026-05-08 04:49:45 -05:00
Code Issues 1.6k Packages Projects Releases 55 Wiki Activity

[PR #6338] Fix authorization bypass allowing non-owners to delete shared budgets #20815

New Issue
Closed
opened 2026-04-14 21:45:54 -05:00 by GiteaMirror · 0 comments
GiteaMirror commented 2026-04-14 21:45:54 -05:00
Owner
Copy Link

Original Pull Request: https://github.com/actualbudget/actual/pull/6338

State: closed
Merged: Yes

Description

Non-owner users could delete shared budgets from the sync server, causing the owner to lose access with unrecoverable "This file is not a cloud file" errors.

Changes

Server-side authorization (packages/sync-server/src/app-sync.ts)

  • Added permission check to /delete-user-file endpoint
  • Only file owners or server admins can delete from server
  • Returns 403 Forbidden for unauthorized attempts
const { user_id: userId } = res.locals;
const isOwner = file.owner === userId;
const isServerAdmin = isAdmin(userId);

if (!isOwner && !isServerAdmin) {
  res.status(403).send({
    status: 'error',
    reason: 'unauthorized',
    details: 'file-delete-not-allowed',
  });
  return;
}

Client-side protection (packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx)

  • Hides "Delete file from all devices" button for non-owners
  • Displays: "This is a hosted file shared with you. Only the file owner can delete it from the server."
  • Local deletion remains available to all users

Test coverage (packages/sync-server/src/app-sync.test.ts)

  • Non-owner receives 403
  • Owner can delete own files
  • Admin can delete any file
Original prompt

This section details on the original issue you should resolve

<issue_title>[Bug]: Non owner users able to delete shared budgets. And owners unable to register the budget on the sync error produced.</issue_title>
<issue_description>### Verified issue does not already exist?

  • I have searched and found no existing issue

What happened?

A bug happened!

Only Budget owner or server owner must have permission to delete a budget. All other shared budget users should not have delete access to the budget they don't own.

Image

How can we reproduce the issue?

How can we reproduce the issue?
Enabled multiuser support with OpenID with google.com

Server or Budget Owner creates a budget and shares it with another user.
The user deletes the budget from the switch files menu and thereby removing it from the server.
Now, for Server/budget owner, it shows a local only file.
Server/budget owner gets a sync error in their budget file and cannot recover the budget by trying to register (clicking the register button) from the UI.
Message:
"This file is not a cloud file
You need to register it to take advantage of syncing which allows you to use it across devices and never worry about losing your data. "

Where are you hosting Actual?

Pikapods

What browsers are you seeing the problem on?

Firefox, Chrome, Safari

Operating System

Windows 11</issue_description>

Comments on the Issue (you are @copilot in this section)

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Note

Enforces authorization on POST /delete-user-file (owner/admin only), updates desktop modal to hide server delete for non-owners, adds tests, and notes in release docs.

  • Backend (sync-server):
    • Auth check: POST /delete-user-file now verifies requester is file owner or isAdmin(userId); otherwise returns 403 with reason: 'forbidden' and details: 'file-delete-not-allowed' in packages/sync-server/src/app-sync.ts.
    • Refactors file existence check to use returned file from verifyFileExists.
  • Frontend (desktop-client):
    • Ownership gating: DeleteFileModal reads state.user.data and shows server delete action only if file.owner === currentUserId or user has ADMIN permission; non-owners see explanatory text in packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx.
  • Tests:
    • Adds coverage for non-owner 403, owner success, and admin success in packages/sync-server/src/app-sync.test.ts.
  • Docs/Release notes:
    • Adds upcoming-release-notes/6338.md describing the bugfix.

Written by Cursor Bugbot for commit 034defddbe. This will update automatically on new commits. Configure here.

**Original Pull Request:** https://github.com/actualbudget/actual/pull/6338 **State:** closed **Merged:** Yes --- ## Description Non-owner users could delete shared budgets from the sync server, causing the owner to lose access with unrecoverable "This file is not a cloud file" errors. ### Changes **Server-side authorization** (`packages/sync-server/src/app-sync.ts`) - Added permission check to `/delete-user-file` endpoint - Only file owners or server admins can delete from server - Returns 403 Forbidden for unauthorized attempts ```typescript const { user_id: userId } = res.locals; const isOwner = file.owner === userId; const isServerAdmin = isAdmin(userId); if (!isOwner && !isServerAdmin) { res.status(403).send({ status: 'error', reason: 'unauthorized', details: 'file-delete-not-allowed', }); return; } ``` **Client-side protection** (`packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx`) - Hides "Delete file from all devices" button for non-owners - Displays: "This is a hosted file shared with you. Only the file owner can delete it from the server." - Local deletion remains available to all users **Test coverage** (`packages/sync-server/src/app-sync.test.ts`) - Non-owner receives 403 - Owner can delete own files - Admin can delete any file <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[Bug]: Non owner users able to delete shared budgets. And owners unable to register the budget on the sync error produced.</issue_title> > <issue_description>### Verified issue does not already exist? > > - [x] I have searched and found no existing issue > > ### What happened? > > A bug happened! > > Only Budget owner or server owner must have permission to delete a budget. All other shared budget users should not have delete access to the budget they don't own. > > <img width="1920" height="914" alt="Image" src="https://github.com/user-attachments/assets/e810ad93-5366-4b2e-b149-8cd264c2c2f8" /> > > > ### How can we reproduce the issue? > > How can we reproduce the issue? > Enabled multiuser support with OpenID with google.com > > Server or Budget Owner creates a budget and shares it with another user. > The user deletes the budget from the switch files menu and thereby removing it from the server. > Now, for Server/budget owner, it shows a local only file. > Server/budget owner gets a sync error in their budget file and cannot recover the budget by trying to register (clicking the register button) from the UI. > Message: > "This file is not a cloud file > You need to register it to take advantage of syncing which allows you to use it across devices and never worry about losing your data. <Register button>" > > ### Where are you hosting Actual? > > Pikapods > > ### What browsers are you seeing the problem on? > > Firefox, Chrome, Safari > > ### Operating System > > Windows 11</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes actualbudget/actual#6334 <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Enforces authorization on `POST /delete-user-file` (owner/admin only), updates desktop modal to hide server delete for non-owners, adds tests, and notes in release docs. > > - **Backend (sync-server)**: > - **Auth check**: `POST /delete-user-file` now verifies requester is file owner or `isAdmin(userId)`; otherwise returns `403` with `reason: 'forbidden'` and `details: 'file-delete-not-allowed'` in `packages/sync-server/src/app-sync.ts`. > - Refactors file existence check to use returned `file` from `verifyFileExists`. > - **Frontend (desktop-client)**: > - **Ownership gating**: `DeleteFileModal` reads `state.user.data` and shows server delete action only if `file.owner === currentUserId` or user has `ADMIN` permission; non-owners see explanatory text in `packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx`. > - **Tests**: > - Adds coverage for non-owner `403`, owner success, and admin success in `packages/sync-server/src/app-sync.test.ts`. > - **Docs/Release notes**: > - Adds `upcoming-release-notes/6338.md` describing the bugfix. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 034defddbe52bbbcec87a842eab32912b1becde9. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
GiteaMirror added the pull-request label 2026-04-14 21:45:54 -05:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
AI generated
API
bank sync
budgeting
bug
can’t replicate
dependencies
docker
documentation
electron
experimental feature
feature
feedback
goal templates
good first issue
help wanted
importers
maintenance
needs info
needs testing
needs triage
needs votes
openid
payees
pull-request
Mirrored from GitHub Pull Request
 regression
reports
responsive
rules
schedules
server
✨ merged
split transactions
tech debt
theme
transaction import
transaction reconciliation
transactions
translations
upstream
user interface
✅ approved
wontfix
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/actual#20815
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?