Address code review feedback: improve tests and clarify logic

- Add tests for empty string replacement and multiple occurrences
- Simplify split processing logic (no unnecessary null checks)
- All 496 tests pass

Co-authored-by: MatissJanis <886567+MatissJanis@users.noreply.github.com>

packages/api/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@actual-app/api",
-  "version": "26.2.1",
+  "version": "26.2.0",
   "description": "An API for Actual",
   "license": "MIT",
   "files": [

packages/desktop-client/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@actual-app/web",
-  "version": "26.2.1",
+  "version": "26.2.0",
   "license": "MIT",
   "files": [
     "build"

packages/desktop-client/src/hooks/useTransactionBatchActions.ts

@@ -106,6 +106,20 @@ export function useTransactionBatchActions() {
       }
 
       const idSet = new Set(ids);
+      // Track which split transactions have been processed to avoid
+      // processing the same split multiple times when multiple members are selected
+      const processedSplits = new Set<string>();
+
+      // Helper to find the parent ID of a split transaction
+      const findSplitParentId = (trans: TransactionEntity): string => {
+        if (trans.is_parent) {
+          return trans.id;
+        }
+        if (trans.is_child && trans.parent_id) {
+          return trans.parent_id;
+        }
+        return trans.id; // Non-split transaction
+      };
 
       transactionsToChange.forEach(trans => {
         if (name === 'cleared' && trans.reconciled) {
@@ -120,6 +134,16 @@ export function useTransactionBatchActions() {
           return;
         }
 
+        // For split transactions, skip if we've already processed this split
+        // to avoid duplicate updates when multiple members are selected
+        if (trans.is_parent || trans.is_child) {
+          const splitParentId = findSplitParentId(trans);
+          if (processedSplits.has(splitParentId)) {
+            return;
+          }
+          processedSplits.add(splitParentId);
+        }
+
         let valueToSet = value;
 
         if (name === 'notes') {

packages/desktop-electron/package.json

@@ -1,6 +1,6 @@
 {
   "name": "desktop-electron",
-  "version": "26.2.1",
+  "version": "26.2.0",
   "description": "A simple and powerful personal finance system",
   "author": "Actual",
   "main": "build/desktop-electron/index.js",

packages/loot-core/src/shared/util.test.ts

@@ -1,4 +1,5 @@
 import {
+  applyFindReplace,
   currencyToAmount,
   getNumberFormat,
   looselyParseAmount,
@@ -229,3 +230,51 @@ describe('utility functions', () => {
     expect(stringToInteger('−3')).toBe(-3);
   });
 });
+
+describe('applyFindReplace', () => {
+  test('basic string replacement works', () => {
+    expect(applyFindReplace('hello world', 'world', 'universe', false)).toBe(
+      'hello universe',
+    );
+    expect(applyFindReplace('test test test', 'test', 'demo', false)).toBe(
+      'demo demo demo',
+    );
+  });
+
+  test('handles null and empty values', () => {
+    expect(applyFindReplace(null, 'test', 'replace', false)).toBe('');
+    expect(applyFindReplace(undefined, 'test', 'replace', false)).toBe('');
+    expect(applyFindReplace('', 'test', 'replace', false)).toBe('');
+    expect(applyFindReplace('hello', '', 'replace', false)).toBe('hello');
+  });
+
+  test('regex replacement works', () => {
+    expect(applyFindReplace('test123', '\\d+', 'NUM', true)).toBe('testNUM');
+    expect(applyFindReplace('foo bar baz', 'b\\w+', 'word', true)).toBe(
+      'foo word word',
+    );
+  });
+
+  test('handles invalid regex gracefully', () => {
+    expect(applyFindReplace('test', '[invalid', 'replace', true)).toBe('test');
+  });
+
+  test('case-sensitive replacement', () => {
+    expect(applyFindReplace('Test test TEST', 'test', 'demo', false)).toBe(
+      'Test demo TEST',
+    );
+  });
+
+  test('empty string replacement', () => {
+    expect(applyFindReplace('test', 'test', '', false)).toBe('');
+    expect(applyFindReplace('hello test world', 'test ', '', false)).toBe(
+      'hello world',
+    );
+  });
+
+  test('replaces multiple occurrences', () => {
+    expect(applyFindReplace('foo foo foo', 'foo', 'bar', false)).toBe(
+      'bar bar bar',
+    );
+  });
+});

packages/sync-server/migrations/1763873600000-backfill-files-owner.js

@@ -1,19 +0,0 @@
-import { getAccountDb } from '../src/account-db';
-
-export const up = async function () {
-  const accountDb = getAccountDb();
-
-  const admin = accountDb.first(
-    'SELECT id FROM users WHERE role = ? ORDER BY id LIMIT 1',
-    ['ADMIN'],
-  );
-  if (admin) {
-    accountDb.mutate('UPDATE files SET owner = ? WHERE owner IS NULL', [
-      admin.id,
-    ]);
-  }
-};
-
-export const down = async function () {
-  // Cannot reliably restore NULL owner for backfilled rows; no-op.
-};

packages/sync-server/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@actual-app/sync-server",
-  "version": "26.2.1",
+  "version": "26.2.0",
   "description": "actual syncing server",
   "license": "MIT",
   "bin": {

packages/sync-server/src/app-pluggyai/app-pluggyai.js

@@ -2,18 +2,14 @@ import express from 'express';
 
 import { handleError } from '../app-gocardless/util/handle-error';
 import { SecretName, secretsService } from '../services/secrets-service';
-import {
-  requestLoggerMiddleware,
-  validateSessionMiddleware,
-} from '../util/middlewares';
+import { requestLoggerMiddleware } from '../util/middlewares';
 
 import { pluggyaiService } from './pluggyai-service';
 
 const app = express();
 export { app as handlers };
-app.use(requestLoggerMiddleware);
 app.use(express.json());
-app.use(validateSessionMiddleware);
+app.use(requestLoggerMiddleware);
 
 app.post(
   '/status',

packages/sync-server/src/app-simplefin/app-simplefin.js

@@ -4,16 +4,12 @@ import express from 'express';
 
 import { handleError } from '../app-gocardless/util/handle-error';
 import { SecretName, secretsService } from '../services/secrets-service';
-import {
-  requestLoggerMiddleware,
-  validateSessionMiddleware,
-} from '../util/middlewares';
+import { requestLoggerMiddleware } from '../util/middlewares';
 
 const app = express();
 export { app as handlers };
-app.use(requestLoggerMiddleware);
 app.use(express.json());
-app.use(validateSessionMiddleware);
+app.use(requestLoggerMiddleware);
 
 app.post(
   '/status',

packages/sync-server/src/app-sync.test.ts

@@ -10,7 +10,6 @@ import { handlers as app } from './app-sync';
 import { getPathForUserFile } from './util/paths';
 
 const ADMIN_ROLE = 'ADMIN';
-const OTHER_USER_ID = 'otherUser';
 
 const createUser = (userId, userName, role, owner = 0, enabled = 1) => {
   getAccountDb().mutate(
@@ -71,48 +70,6 @@ describe('/user-get-key', () => {
     expect(res.statusCode).toEqual(400);
     expect(res.text).toBe('file-not-found');
   });
-
-  it('returns 403 when non-owner gets encryption key', async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    getAccountDb().mutate(
-      'INSERT INTO files (id, encrypt_salt, encrypt_keyid, encrypt_test, owner) VALUES (?, ?, ?, ?, ?)',
-      [fileId, 'salt', 'key-id', 'test', OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .post('/user-get-key')
-      .set('x-actual-token', 'valid-token-user')
-      .send({ fileId });
-
-    expect(res.statusCode).toEqual(403);
-    expect(res.text).toEqual('file-access-not-allowed');
-  });
-
-  it("allows an admin to get encryption key for another user's file", async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const encrypt_salt = 'salt';
-    const encrypt_keyid = 'key-id';
-    const encrypt_test = 'test';
-    getAccountDb().mutate(
-      'INSERT INTO files (id, encrypt_salt, encrypt_keyid, encrypt_test, owner) VALUES (?, ?, ?, ?, ?)',
-      [fileId, encrypt_salt, encrypt_keyid, encrypt_test, OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .post('/user-get-key')
-      .set('x-actual-token', 'valid-token-admin')
-      .send({ fileId });
-
-    expect(res.statusCode).toEqual(200);
-    expect(res.body).toEqual({
-      status: 'ok',
-      data: {
-        id: encrypt_keyid,
-        salt: encrypt_salt,
-        test: encrypt_test,
-      },
-    });
-  });
 });
 
 describe('/user-create-key', () => {
@@ -134,69 +91,7 @@ describe('/user-create-key', () => {
       .send({ fileId: 'non-existent-file-id' });
 
     expect(res.statusCode).toEqual(400);
-    expect(res.text).toBe('file-not-found');
-  });
-
-  it('returns 403 when non-owner creates encryption key', async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    getAccountDb().mutate(
-      'INSERT INTO files (id, encrypt_salt, encrypt_keyid, encrypt_test, owner) VALUES (?, ?, ?, ?, ?)',
-      [fileId, 'old-salt', 'old-key', 'old-test', OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .post('/user-create-key')
-      .set('x-actual-token', 'valid-token-user')
-      .send({
-        fileId,
-        keyId: 'new-key',
-        keySalt: 'new-salt',
-        testContent: 'new-test',
-      });
-
-    expect(res.statusCode).toEqual(403);
-    expect(res.text).toEqual('file-access-not-allowed');
-  });
-
-  it("allows an admin to create encryption key for another user's file", async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const old_encrypt_salt = 'old-salt';
-    const old_encrypt_keyid = 'old-key';
-    const old_encrypt_test = 'old-test';
-    const encrypt_salt = 'new-salt';
-    const encrypt_keyid = 'new-key-id';
-    const encrypt_test = 'new-encrypt-test';
-    getAccountDb().mutate(
-      'INSERT INTO files (id, encrypt_salt, encrypt_keyid, encrypt_test, owner) VALUES (?, ?, ?, ?, ?)',
-      [
-        fileId,
-        old_encrypt_salt,
-        old_encrypt_keyid,
-        old_encrypt_test,
-        OTHER_USER_ID,
-      ],
-    );
-
-    const res = await request(app)
-      .post('/user-create-key')
-      .set('x-actual-token', 'valid-token-admin')
-      .send({
-        fileId,
-        keyId: encrypt_keyid,
-        keySalt: encrypt_salt,
-        testContent: encrypt_test,
-      });
-
-    expect(res.statusCode).toEqual(200);
-    expect(res.body).toEqual({ status: 'ok' });
-
-    const rows = getAccountDb().all(
-      'SELECT encrypt_salt, encrypt_keyid, encrypt_test FROM files WHERE id = ?',
-      [fileId],
-    );
-    expect(rows[0].encrypt_salt).toEqual(encrypt_salt);
-    expect(rows[0].encrypt_keyid).toEqual(encrypt_keyid);
-    expect(rows[0].encrypt_test).toEqual(encrypt_test);
+    expect(res.text).toBe('file not found');
   });
 
   it('creates a new encryption key for the file', async () => {
@@ -290,44 +185,6 @@ describe('/reset-user-file', () => {
     expect(res.statusCode).toEqual(400);
     expect(res.text).toBe('User or file not found');
   });
-
-  it('returns 403 when non-owner resets another user file', async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    getAccountDb().mutate(
-      'INSERT OR IGNORE INTO files (id, deleted, owner) VALUES (?, FALSE, ?)',
-      [fileId, OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .post('/reset-user-file')
-      .set('x-actual-token', 'valid-token-user')
-      .send({ fileId });
-
-    expect(res.statusCode).toEqual(403);
-    expect(res.text).toEqual('file-access-not-allowed');
-  });
-
-  it("allows an admin to reset another user's file", async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const groupId = 'admin-reset-group-id';
-    getAccountDb().mutate(
-      'INSERT INTO files (id, group_id, deleted, owner) VALUES (?, ?, FALSE, ?)',
-      [fileId, groupId, OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .post('/reset-user-file')
-      .set('x-actual-token', 'valid-token-admin')
-      .send({ fileId });
-
-    expect(res.statusCode).toEqual(200);
-    expect(res.body).toEqual({ status: 'ok' });
-
-    const rows = getAccountDb().all('SELECT group_id FROM files WHERE id = ?', [
-      fileId,
-    ]);
-    expect(rows[0].group_id).toBeNull();
-  });
 });
 
 describe('/upload-user-file', () => {
@@ -373,11 +230,6 @@ describe('/upload-user-file', () => {
     const fileContentBuffer = Buffer.from(fileContent);
     const syncVersion = 2;
     const encryptMeta = JSON.stringify({ keyId: 'key-id' });
-    onTestFinished(() => {
-      try {
-        fs.unlinkSync(getPathForUserFile(fileId));
-      } catch {}
-    });
 
     // Verify that the file does not exist before upload
     const rowsBefore = getAccountDb().all('SELECT * FROM files WHERE id = ?', [
@@ -415,6 +267,9 @@ describe('/upload-user-file', () => {
     const filePath = getPathForUserFile(fileId);
     const writtenContent = await fs.promises.readFile(filePath, 'utf8');
     expect(writtenContent).toEqual(fileContent);
+
+    // Clean up the file
+    await fs.promises.unlink(filePath);
   });
 
   it('uploads and updates an existing file successfully', async () => {
@@ -432,11 +287,6 @@ describe('/upload-user-file', () => {
       keyId: oldKeyId,
       sentinelValue: 1,
     }); //keep the same key, but change other things
-    onTestFinished(() => {
-      try {
-        fs.unlinkSync(getPathForUserFile(fileId));
-      } catch {}
-    });
 
     // Create the old file version
     getAccountDb().mutate(
@@ -485,6 +335,9 @@ describe('/upload-user-file', () => {
     const filePath = getPathForUserFile(fileId);
     const writtenContent = await fs.promises.readFile(filePath, 'utf8');
     expect(writtenContent).toEqual(newFileContent);
+
+    // Clean up the file
+    await fs.promises.unlink(filePath);
   });
 
   it('returns 400 if the file is part of an old group', async () => {
@@ -544,94 +397,6 @@ describe('/upload-user-file', () => {
     expect(res.statusCode).toEqual(400);
     expect(res.text).toEqual('file-has-new-key');
   });
-
-  it('returns 403 when non-owner overwrites another user file', async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const groupId = 'group-id';
-    const keyId = 'key-id';
-    const syncVersion = 2;
-    fs.writeFileSync(getPathForUserFile(fileId), 'existing content');
-    getAccountDb().mutate(
-      'INSERT INTO files (id, group_id, sync_version, name, encrypt_meta, encrypt_keyid, deleted, owner) VALUES (?, ?, ?, ?, ?, ?, 0, ?)',
-      [
-        fileId,
-        groupId,
-        syncVersion,
-        'existing.txt',
-        JSON.stringify({ keyId }),
-        keyId,
-        OTHER_USER_ID,
-      ],
-    );
-    onTestFinished(() => {
-      try {
-        fs.unlinkSync(getPathForUserFile(fileId));
-      } catch {}
-    });
-
-    const res = await request(app)
-      .post('/upload-user-file')
-      .set('Content-Type', 'application/encrypted-file')
-      .set('x-actual-token', 'valid-token-user')
-      .set('x-actual-file-id', fileId)
-      .set('x-actual-name', 'hacked.txt')
-      .set('x-actual-group-id', groupId)
-      .set('x-actual-format', syncVersion.toString())
-      .set('x-actual-encrypt-meta', JSON.stringify({ keyId }))
-      .send(Buffer.from('overwrite content'));
-
-    expect(res.statusCode).toEqual(403);
-    expect(res.text).toEqual('file-access-not-allowed');
-  });
-
-  it("allows an admin to overwrite another user's file", async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const groupId = 'admin-upload-group-id';
-    const keyId = 'key-id';
-    const syncVersion = 2;
-    const existingContent = 'existing content';
-    const newContent = 'admin overwrite content';
-    fs.writeFileSync(getPathForUserFile(fileId), existingContent);
-    getAccountDb().mutate(
-      'INSERT INTO files (id, group_id, sync_version, name, encrypt_meta, encrypt_keyid, deleted, owner) VALUES (?, ?, ?, ?, ?, ?, 0, ?)',
-      [
-        fileId,
-        groupId,
-        syncVersion,
-        'existing.txt',
-        JSON.stringify({ keyId }),
-        keyId,
-        OTHER_USER_ID,
-      ],
-    );
-    onTestFinished(() => {
-      try {
-        fs.unlinkSync(getPathForUserFile(fileId));
-      } catch {}
-    });
-
-    const res = await request(app)
-      .post('/upload-user-file')
-      .set('Content-Type', 'application/encrypted-file')
-      .set('x-actual-token', 'valid-token-admin')
-      .set('x-actual-file-id', fileId)
-      .set('x-actual-name', 'admin-renamed.txt')
-      .set('x-actual-group-id', groupId)
-      .set('x-actual-format', syncVersion.toString())
-      .set('x-actual-encrypt-meta', JSON.stringify({ keyId }))
-      .send(Buffer.from(newContent));
-
-    expect(res.statusCode).toEqual(200);
-    expect(res.body).toEqual({ status: 'ok', groupId });
-
-    expect(fs.readFileSync(getPathForUserFile(fileId), 'utf8')).toEqual(
-      newContent,
-    );
-    const rows = getAccountDb().all('SELECT name FROM files WHERE id = ?', [
-      fileId,
-    ]);
-    expect(rows[0].name).toEqual('admin-renamed.txt');
-  });
 });
 
 describe('/download-user-file', () => {
@@ -708,91 +473,6 @@ describe('/download-user-file', () => {
       expect(res.body).toBeInstanceOf(Buffer);
       expect(res.body.toString('utf8')).toEqual(fileContent);
     });
-
-    describe('access control', () => {
-      it('returns 403 when non-owner downloads another user file', async () => {
-        const fileId = crypto.randomBytes(16).toString('hex');
-        const fileContent = 'sensitive content';
-        fs.writeFileSync(getPathForUserFile(fileId), fileContent);
-        getAccountDb().mutate(
-          'INSERT INTO files (id, deleted, owner) VALUES (?, FALSE, ?)',
-          [fileId, OTHER_USER_ID],
-        );
-        onTestFinished(() => {
-          try {
-            fs.unlinkSync(getPathForUserFile(fileId));
-          } catch {}
-        });
-
-        const res = await request(app)
-          .get('/download-user-file')
-          .set('x-actual-token', 'valid-token-user')
-          .set('x-actual-file-id', fileId);
-
-        expect(res.statusCode).toEqual(403);
-        expect(res.text).toEqual('file-access-not-allowed');
-      });
-
-      it("allows an admin to download another user's file", async () => {
-        const fileId = crypto.randomBytes(16).toString('hex');
-        const fileContent = 'admin-downloaded content';
-        fs.writeFileSync(getPathForUserFile(fileId), fileContent);
-        getAccountDb().mutate(
-          'INSERT INTO files (id, deleted, owner) VALUES (?, FALSE, ?)',
-          [fileId, OTHER_USER_ID],
-        );
-        onTestFinished(() => {
-          try {
-            fs.unlinkSync(getPathForUserFile(fileId));
-          } catch {}
-        });
-
-        const res = await request(app)
-          .get('/download-user-file')
-          .set('x-actual-token', 'valid-token-admin')
-          .set('x-actual-file-id', fileId);
-
-        expect(res.statusCode).toEqual(200);
-        expect(res.body).toBeInstanceOf(Buffer);
-        expect(res.body.toString('utf8')).toEqual(fileContent);
-      });
-
-      it('allows non-owner with user_access to download via requireFileAccess (UserService.countUserAccess > 0)', async () => {
-        // File owned by another user; access granted only via user_access row, not owner/admin.
-        // This exercises the requireFileAccess branch that uses UserService.countUserAccess.
-        const fileId = crypto.randomBytes(16).toString('hex');
-        const fileContent = 'shared-user content';
-        fs.writeFileSync(getPathForUserFile(fileId), fileContent);
-        getAccountDb().mutate(
-          'INSERT INTO files (id, deleted, owner) VALUES (?, FALSE, ?)',
-          [fileId, OTHER_USER_ID],
-        );
-        getAccountDb().mutate(
-          'INSERT INTO user_access (file_id, user_id) VALUES (?, ?)',
-          [fileId, 'genericUser'],
-        );
-        onTestFinished(() => {
-          try {
-            fs.unlinkSync(getPathForUserFile(fileId));
-          } catch {}
-        });
-
-        const res = await request(app)
-          .get('/download-user-file')
-          .set('x-actual-token', 'valid-token-user')
-          .set('x-actual-file-id', fileId);
-
-        expect(res.statusCode).toEqual(200);
-        expect(res.headers).toEqual(
-          expect.objectContaining({
-            'content-disposition': `attachment;filename=${fileId}`,
-            'content-type': 'application/octet-stream',
-          }),
-        );
-        expect(res.body).toBeInstanceOf(Buffer);
-        expect(res.body.toString('utf8')).toEqual(fileContent);
-      });
-    });
   });
 });
 
@@ -815,7 +495,7 @@ describe('/update-user-filename', () => {
       .send({ fileId: 'non-existent-file-id', name: 'new-filename' });
 
     expect(res.statusCode).toEqual(400);
-    expect(res.text).toBe('file-not-found');
+    expect(res.text).toBe('file not found');
   });
 
   it('successfully updates the filename', async () => {
@@ -844,45 +524,6 @@ describe('/update-user-filename', () => {
 
     expect(rows[0].name).toEqual(newName);
   });
-
-  it('returns 403 when non-owner renames another user file', async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    getAccountDb().mutate(
-      'INSERT INTO files (id, name, deleted, owner) VALUES (?, ?, FALSE, ?)',
-      [fileId, 'original-name', OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .post('/update-user-filename')
-      .set('x-actual-token', 'valid-token-user')
-      .send({ fileId, name: 'stolen' });
-
-    expect(res.statusCode).toEqual(403);
-    expect(res.text).toEqual('file-access-not-allowed');
-  });
-
-  it("allows an admin to rename another user's file", async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const originalName = 'original-name';
-    const newName = 'admin-renamed-file';
-    getAccountDb().mutate(
-      'INSERT INTO files (id, name, deleted, owner) VALUES (?, ?, FALSE, ?)',
-      [fileId, originalName, OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .post('/update-user-filename')
-      .set('x-actual-token', 'valid-token-admin')
-      .send({ fileId, name: newName });
-
-    expect(res.statusCode).toEqual(200);
-    expect(res.body).toEqual({ status: 'ok' });
-
-    const rows = getAccountDb().all('SELECT name FROM files WHERE id = ?', [
-      fileId,
-    ]);
-    expect(rows[0].name).toEqual(newName);
-  });
 });
 
 describe('/list-user-files', () => {
@@ -1012,51 +653,6 @@ describe('/get-user-file-info', () => {
       details: 'token-not-found',
     });
   });
-
-  it('returns 403 when non-owner gets another user file info', async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    getAccountDb().mutate(
-      'INSERT INTO files (id, group_id, name, deleted, owner) VALUES (?, ?, ?, FALSE, ?)',
-      [fileId, 'group-id', 'budget', OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .get('/get-user-file-info')
-      .set('x-actual-token', 'valid-token-user')
-      .set('x-actual-file-id', fileId);
-
-    expect(res.statusCode).toEqual(403);
-    expect(res.text).toEqual('file-access-not-allowed');
-  });
-
-  it("allows an admin to get another user's file info", async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const groupId = 'admin-file-info-group';
-    const name = 'admin-info-file';
-    const encrypt_meta = JSON.stringify({ key: 'value' });
-    getAccountDb().mutate(
-      'INSERT INTO files (id, group_id, name, encrypt_meta, deleted, owner) VALUES (?, ?, ?, ?, 0, ?)',
-      [fileId, groupId, name, encrypt_meta, OTHER_USER_ID],
-    );
-
-    const res = await request(app)
-      .get('/get-user-file-info')
-      .set('x-actual-token', 'valid-token-admin')
-      .set('x-actual-file-id', fileId);
-
-    expect(res.statusCode).toEqual(200);
-    expect(res.body).toEqual({
-      status: 'ok',
-      data: {
-        deleted: 0,
-        fileId,
-        groupId,
-        name,
-        encryptMeta: { key: 'value' },
-        usersWithAccess: [],
-      },
-    });
-  });
 });
 
 describe('/delete-user-file', () => {
@@ -1137,7 +733,11 @@ describe('/delete-user-file', () => {
       .send({ fileId });
 
     expect(res.statusCode).toEqual(403);
-    expect(res.text).toEqual('file-access-not-allowed');
+    expect(res.body).toEqual({
+      status: 'error',
+      reason: 'forbidden',
+      details: 'file-delete-not-allowed',
+    });
 
     // Verify that the file is NOT deleted
     const rows = accountDb.all('SELECT deleted FROM files WHERE id = ?', [
@@ -1333,64 +933,12 @@ describe('/sync', () => {
     expect(res.statusCode).toEqual(400);
     expect(res.text).toEqual('file-has-new-key');
   });
-
-  it('returns 403 when non-owner syncs another user file', async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const groupId = 'group-id';
-    const keyId = 'key-id';
-    const syncVersion = 2;
-    const encryptMeta = JSON.stringify({ keyId });
-    addMockFile(
-      fileId,
-      groupId,
-      keyId,
-      encryptMeta,
-      syncVersion,
-      OTHER_USER_ID,
-    );
-    const syncRequest = createMinimalSyncRequest(fileId, groupId, keyId);
-
-    const res = await sendSyncRequest(syncRequest, 'valid-token-user');
-
-    expect(res.statusCode).toEqual(403);
-    expect(res.text).toEqual('file-access-not-allowed');
-  });
-
-  it("allows an admin to sync another user's file", async () => {
-    const fileId = crypto.randomBytes(16).toString('hex');
-    const groupId = 'group-id';
-    const keyId = 'key-id';
-    const syncVersion = 2;
-    const encryptMeta = JSON.stringify({ keyId });
-    addMockFile(
-      fileId,
-      groupId,
-      keyId,
-      encryptMeta,
-      syncVersion,
-      OTHER_USER_ID,
-    );
-    const syncRequest = createMinimalSyncRequest(fileId, groupId, keyId);
-
-    const res = await sendSyncRequest(syncRequest, 'valid-token-admin');
-
-    expect(res.statusCode).toEqual(200);
-    expect(res.headers['content-type']).toEqual('application/actual-sync');
-    expect(res.headers['x-actual-sync-method']).toEqual('simple');
-  });
 });
 
-function addMockFile(
-  fileId: string,
-  groupId: string | null,
-  keyId: string,
-  encryptMeta: string,
-  syncVersion: number,
-  owner: string = 'genericAdmin',
-) {
+function addMockFile(fileId, groupId, keyId, encryptMeta, syncVersion) {
   getAccountDb().mutate(
     'INSERT INTO files (id, group_id, encrypt_keyid, encrypt_meta, sync_version, owner) VALUES (?, ?, ?,?, ?, ?)',
-    [fileId, groupId, keyId, encryptMeta, syncVersion, owner],
+    [fileId, groupId, keyId, encryptMeta, syncVersion, 'genericAdmin'],
   );
 }
 
@@ -1404,14 +952,14 @@ function createMinimalSyncRequest(fileId, groupId, keyId) {
   return syncRequest;
 }
 
-async function sendSyncRequest(syncRequest, token = 'valid-token') {
+async function sendSyncRequest(syncRequest) {
   const serializedRequest = syncRequest.serializeBinary();
   // Convert Uint8Array to Buffer
   const bufferRequest = Buffer.from(serializedRequest);
 
   const res = await request(app)
     .post('/sync')
-    .set('x-actual-token', token)
+    .set('x-actual-token', 'valid-token')
     .set('Content-Type', 'application/actual-sync')
     .send(bufferRequest);
   return res;

packages/sync-server/src/app-sync.ts

@@ -19,7 +19,6 @@ import {
   validateUploadedFile,
 } from './app-sync/validation';
 import { config } from './load-config';
-import * as UserService from './services/user-service';
 import * as simpleSync from './sync-simple';
 import {
   errorMiddleware,
@@ -69,18 +68,6 @@ const verifyFileExists = (fileId, filesService, res, errorObject) => {
   }
 };
 
-function requireFileAccess(file: File, userId: string) {
-  const isOwner = file.owner === userId;
-  const isServerAdmin = isAdmin(userId);
-  if (isOwner || isServerAdmin) {
-    return null;
-  }
-  if (UserService.countUserAccess(file.id, userId) > 0) {
-    return null;
-  }
-  return 'file-access-not-allowed';
-}
-
 app.post('/sync', async (req, res): Promise<void> => {
   let requestPb;
   try {
@@ -120,13 +107,6 @@ app.post('/sync', async (req, res): Promise<void> => {
     return;
   }
 
-  const fileAccessError = requireFileAccess(currentFile, res.locals.user_id);
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
-    return;
-  }
-
   const errorMessage = validateSyncedFile(groupId, keyId, currentFile);
   if (errorMessage) {
     res.status(400);
@@ -158,13 +138,6 @@ app.post('/user-get-key', (req, res) => {
     return;
   }
 
-  const fileAccessError = requireFileAccess(file, res.locals.user_id);
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
-    return;
-  }
-
   res.send({
     status: 'ok',
     data: {
@@ -179,16 +152,8 @@ app.post('/user-create-key', (req, res) => {
   const { fileId, keyId, keySalt, testContent } = req.body || {};
 
   const filesService = new FilesService(getAccountDb());
-  const file = verifyFileExists(fileId, filesService, res, 'file-not-found');
 
-  if (!file) {
-    return;
-  }
-
-  const fileAccessError = requireFileAccess(file, res.locals.user_id);
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
+  if (!verifyFileExists(fileId, filesService, res, 'file not found')) {
     return;
   }
 
@@ -219,13 +184,6 @@ app.post('/reset-user-file', async (req, res) => {
     return;
   }
 
-  const fileAccessError = requireFileAccess(file, res.locals.user_id);
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
-    return;
-  }
-
   const groupId = file.groupId;
 
   filesService.update(fileId, new FileUpdate({ groupId: null }));
@@ -279,15 +237,6 @@ app.post('/upload-user-file', async (req, res) => {
     }
   }
 
-  const fileAccessError = currentFile
-    ? requireFileAccess(currentFile, res.locals.user_id)
-    : null;
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
-    return;
-  }
-
   const errorMessage = validateUploadedFile(groupId, keyId, currentFile);
   if (errorMessage) {
     res.status(400).send(errorMessage);
@@ -354,21 +303,7 @@ app.get('/download-user-file', async (req, res) => {
   }
 
   const filesService = new FilesService(getAccountDb());
-  const file = verifyFileExists(
-    fileId,
-    filesService,
-    res,
-    'User or file not found',
-  );
-
-  if (!file) {
-    return;
-  }
-
-  const fileAccessError = requireFileAccess(file, res.locals.user_id);
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
+  if (!verifyFileExists(fileId, filesService, res, 'User or file not found')) {
     return;
   }
 
@@ -388,16 +323,8 @@ app.post('/update-user-filename', (req, res) => {
   const { fileId, name } = req.body || {};
 
   const filesService = new FilesService(getAccountDb());
-  const file = verifyFileExists(fileId, filesService, res, 'file-not-found');
 
-  if (!file) {
-    return;
-  }
-
-  const fileAccessError = requireFileAccess(file, res.locals.user_id);
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
+  if (!verifyFileExists(fileId, filesService, res, 'file not found')) {
     return;
   }
 
@@ -448,13 +375,6 @@ app.get('/get-user-file-info', (req, res) => {
     return;
   }
 
-  const fileAccessError = requireFileAccess(file, res.locals.user_id);
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
-    return;
-  }
-
   res.send({
     status: 'ok',
     data: {
@@ -489,10 +409,18 @@ app.post('/delete-user-file', (req, res) => {
     return;
   }
 
-  const fileAccessError = requireFileAccess(file, res.locals.user_id);
-  if (fileAccessError) {
-    res.status(403);
-    res.send(fileAccessError);
+  // Check if user has permission to delete the file
+  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: 'forbidden',
+      details: 'file-delete-not-allowed',
+    });
     return;
   }
 

upcoming-release-notes/7034.md

@@ -1,6 +0,0 @@
----
-category: Bugfixes
-authors: [MatissJanis]
----
-
-Fix: simplefin and pluggy not requiring auth

upcoming-release-notes/7040.md

@@ -1,6 +0,0 @@
----
-category: Bugfixes
-authors: [MatissJanis]
----
-
-Fix unauthorized file access in multi-user setups