[AI] Enforce file access authorization on sync API endpoints

[MatissJanis]

No description provided.

[netlify]

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	f687d17
🔍 Latest deploy log	https://app.netlify.com/projects/actualbudget/deploys/6998daa36081cf00082341fd
😎 Deploy Preview	https://deploy-preview-7040.demo.actualbudget.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[MatissJanis]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a centralized file-access authorization helper used across sync-server routes, updates tests to validate admin vs non-owner access and simplified error payloads, introduces a migration to backfill NULL file owners, and adds release notes describing the bugfix.

Changes

Cohort / File(s)	Summary
Authorization Implementation packages/sync-server/src/app-sync.ts	Adds requireFileAccess(file, userId) (uses UserService) and applies it across file-related routes; fetches files via verifyFileExists then enforces authorization, replacing inline permission checks and standardizing 403 responses.
Authorization Tests packages/sync-server/src/app-sync.test.ts	Adds OTHER_USER_ID, expands tests for admin vs non-owner 403/200 flows across endpoints (user-get-key, user-create-key, reset-user-file, upload-user-file, download-user-file, update-user-filename, get-user-file-info, sync); simplifies expected error payloads; extends helpers (addMockFile(owner), sendSyncRequest(token)); adds per-test filesystem cleanup.
DB Migration packages/sync-server/migrations/1763873600000-backfill-files-owner.js	New migration up backfills NULL owner_id in files to an admin user's id; down is a no-op.
Release Notes upcoming-release-notes/7040.md	Adds a Bugfixes release note: "Fix unauthorized file access in multi-user setups."

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client as Client
  participant App as SyncServer (app-sync)
  participant DB as Database
  participant FS as FileStore

  Client->>App: Request (route with fileId, token)
  App->>DB: verifyFileExists(fileId)
  DB-->>App: file
  App->>DB: UserService.countUserAccess / requireFileAccess(file, userId)
  alt access allowed (owner or admin or granted access)
    App->>FS: read/write file as needed
    FS-->>App: file data / ack
    App-->>Client: 200 + payload
  else access denied
    App-->>Client: 403 "file-access-not-allowed"
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through tests to guard each file's door,
Admins may override, others stopped at four-oh-three,
Backfilled the blanks where owners were lost,
Routes now check first, then act on the file,
A carrot cheer for safe access—hop, patch, behold!

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No description was provided by the author; the field is completely empty.	Add a pull request description explaining the changes, context, and rationale for enforcing file access authorization.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately describes the main objective of the pull request: enforcing file access authorization on sync API endpoints.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch matiss/sync-api-file-access-auth

---

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.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/sync-server/src/app-sync.ts (1)

278-291: ⚠️ Potential issue | 🟡 Minor

Auth check should precede validateUploadedFile to prevent file-state disclosure.

For an existing file, validateUploadedFile can return 'file-has-reset' or 'file-has-new-key' to the caller before the 403 is emitted. An attacker who knows a valid fileId can probe these responses to learn that the file's groupId or encryptKeyId has changed — metadata leakage without having access to the file. Moving the auth check above the validation call closes this.

🛡️ Suggested re-ordering

-  const errorMessage = validateUploadedFile(groupId, keyId, currentFile);
-  if (errorMessage) {
-    res.status(400).send(errorMessage);
-    return;
-  }
-
   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);
+    return;
+  }

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.ts` around lines 278 - 291, The validation
call validateUploadedFile(currentFile, ...) is leaking file-state details before
the authorization check—move the requireFileAccess(currentFile,
res.locals.user_id) block to run before calling validateUploadedFile so that for
existing files you first verify access and return 403 if unauthorized; only
after requireFileAccess succeeds (and currentFile is confirmed accessible) call
validateUploadedFile and then return 400 on its errors. Ensure you reference
currentFile, requireFileAccess, validateUploadedFile, and res.locals.user_id
when making the change.

🧹 Nitpick comments (2)

packages/sync-server/src/app-sync.ts (1)

488-501: /delete-user-file duplicates the requireFileAccess predicate instead of reusing it.

The ownership+admin check logic on lines 491–494 is identical to what requireFileAccess does. The error format differs (a JSON body vs the plain-text string), but the predicate can still delegate to requireFileAccess:

♻️ Proposed refactor

-  const { user_id: userId } = res.locals;
-
-  const isOwner = file.owner === userId;
-  const isServerAdmin = isAdmin(userId);
-
-  if (!isOwner && !isServerAdmin) {
+  if (requireFileAccess(file, res.locals.user_id)) {
     res.status(403).send({
       status: 'error',
       reason: 'forbidden',
       details: 'file-delete-not-allowed',
     });
     return;
   }

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.ts` around lines 488 - 501, The file-delete
handler duplicates the ownership/admin check; replace the inline
isOwner/isServerAdmin logic with a call to the existing requireFileAccess
predicate so we reuse authorization logic: invoke requireFileAccess (the same
function/middleware used elsewhere) to validate access for the current user and
file, and if it denies access return the same 403 JSON response currently used
in this handler; remove the duplicated isOwner/isServerAdmin branch and any
direct calls to isAdmin in this handler so authorization is centralized via
requireFileAccess.

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

75-89: Missing admin-bypass coverage for the new access-controlled endpoints.

All eight new 403 tests verify that a non-owner is rejected, but none verify the complement: that an admin can access files owned by someone else. The existing /delete-user-file test suite has this pattern (lines 917–941). Adding a matching admin-bypass test for at least one representative endpoint (e.g., /user-get-key or /sync) would close the coverage gap symmetrically.

Also applies to: 114-133, 227-241, 455-489, 568-586, 638-652, 783-797, 1079-1099

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.test.ts` around lines 75 - 89, Add a
complementary admin-bypass test: generate a fileId and insert a files row via
getAccountDb().mutate(...) with owner = OTHER_USER_ID (same as the failing
test), then call request(app).post('/user-get-key') with header 'x-actual-token'
set to 'valid-token-admin' (or whatever admin token is used in the suite) and
the same body { fileId }; assert the response is allowed (status 200) and
contains the expected encryption info (e.g., the encrypt_test/encrypt_keyid
value you injected). This mirrors the non-owner 403 test but uses the admin
token to verify admins can bypass ownership checks for the '/user-get-key'
endpoint.

🤖 Prompt for all review comments with AI agents

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

Outside diff comments:
In `@packages/sync-server/src/app-sync.ts`:
- Around line 278-291: The validation call validateUploadedFile(currentFile,
...) is leaking file-state details before the authorization check—move the
requireFileAccess(currentFile, res.locals.user_id) block to run before calling
validateUploadedFile so that for existing files you first verify access and
return 403 if unauthorized; only after requireFileAccess succeeds (and
currentFile is confirmed accessible) call validateUploadedFile and then return
400 on its errors. Ensure you reference currentFile, requireFileAccess,
validateUploadedFile, and res.locals.user_id when making the change.

---

Nitpick comments:
In `@packages/sync-server/src/app-sync.test.ts`:
- Around line 75-89: Add a complementary admin-bypass test: generate a fileId
and insert a files row via getAccountDb().mutate(...) with owner = OTHER_USER_ID
(same as the failing test), then call request(app).post('/user-get-key') with
header 'x-actual-token' set to 'valid-token-admin' (or whatever admin token is
used in the suite) and the same body { fileId }; assert the response is allowed
(status 200) and contains the expected encryption info (e.g., the
encrypt_test/encrypt_keyid value you injected). This mirrors the non-owner 403
test but uses the admin token to verify admins can bypass ownership checks for
the '/user-get-key' endpoint.

In `@packages/sync-server/src/app-sync.ts`:
- Around line 488-501: The file-delete handler duplicates the ownership/admin
check; replace the inline isOwner/isServerAdmin logic with a call to the
existing requireFileAccess predicate so we reuse authorization logic: invoke
requireFileAccess (the same function/middleware used elsewhere) to validate
access for the current user and file, and if it denies access return the same
403 JSON response currently used in this handler; remove the duplicated
isOwner/isServerAdmin branch and any direct calls to isAdmin in this handler so
authorization is centralized via requireFileAccess.

[coderabbitai]

🧹 Nitpick comments (5)

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

1147-1159: groupId parameter type should be string | null

addMockFile is called with null for groupId at line 1073. The @ts-strict-ignore banner suppresses the diagnostic, but tightening the type makes the intent explicit and avoids silent bugs if strict mode is ever re-enabled.

♻️ Proposed fix

 function addMockFile(
   fileId: string,
-  groupId: string,
+  groupId: string | null,
   keyId: string,
   encryptMeta: string,
   syncVersion: number,
   owner: string = 'genericAdmin',
 ) {

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.test.ts` around lines 1147 - 1159, The
addMockFile helper currently types groupId as string but callers sometimes pass
null; update the function signature for addMockFile to accept groupId: string |
null and ensure the SQL bind uses that value directly (allowing null to be
inserted) so callers no longer need `@ts-strict-ignore`; locate the addMockFile
function and change its parameter type and any related test-call sites if
necessary to match the new union type.

---

481-515: Use onTestFinished to guarantee file cleanup on assertion failures

Both new tests call fs.unlinkSync after the expect assertions. If any assertion throws, the temp file is left on disk and can pollute subsequent test runs.

♻️ Proposed fix for both tests

  it('returns 403 when non-owner overwrites another user file', async () => {
    const fileId = crypto.randomBytes(16).toString('hex');
    // ...
    fs.writeFileSync(getPathForUserFile(fileId), 'existing content');
+   onTestFinished(() => {
+     try { fs.unlinkSync(getPathForUserFile(fileId)); } catch { /* already removed */ }
+   });
    // ...
-   fs.unlinkSync(getPathForUserFile(fileId));
  });

Apply the same pattern to the 'returns 403 when non-owner downloads another user file' test.

Also applies to: 594-612

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.test.ts` around lines 481 - 515, The tests
'returns 403 when non-owner overwrites another user file' and 'returns 403 when
non-owner downloads another user file' currently call
fs.unlinkSync(getPathForUserFile(fileId)) after assertions, which leaves temp
files if an assertion fails; modify both tests to register cleanup with
onTestFinished(() => fs.unlinkSync(getPathForUserFile(fileId))) immediately
after creating the temp file (before any assertions) so the file is always
removed, referencing the existing getPathForUserFile and fileId variables and
removing the trailing direct fs.unlinkSync calls at the end of each test.

---

75-115: 403/admin coverage LGTM; consider adding admin-access tests for remaining endpoints

All new 403 tests correctly assert both the status code and the 'file-access-not-allowed' text body. Admin bypass tests exist for /user-get-key and /sync, but are absent for /user-create-key, /reset-user-file, /upload-user-file, /download-user-file, /update-user-filename, and /get-user-file-info. Not a blocker, but parity would strengthen the test suite.

Also applies to: 1101-1144

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.test.ts` around lines 75 - 115, Add parity
admin-access tests for the remaining endpoints listed in the review so admins
can bypass file-owner restrictions: create tests in the same test suite that
insert a file row (like the existing tests that set
encrypt_salt/encrypt_keyid/encrypt_test and owner = OTHER_USER_ID) and then call
each endpoint '/user-create-key', '/reset-user-file', '/upload-user-file',
'/download-user-file', '/update-user-filename', and '/get-user-file-info' using
the admin token header ('x-actual-token' = 'valid-token-admin'), asserting a 200
response (or the expected success payload) and confirming the returned data
matches the inserted values; mirror the non-owner 403 tests by also keeping a
non-admin request for each endpoint that asserts 403 and
'file-access-not-allowed' to ensure coverage of both branches.

packages/sync-server/src/app-sync.ts (2)

178-178: Inconsistent error string: 'file not found' vs 'file-not-found'

Both lines pass 'file not found' (human-readable, space-separated) to verifyFileExists, while every other call in this file uses kebab-case ('file-not-found' or a structured object). Standardizing to 'file-not-found' would require updating the corresponding pre-existing test assertions at lines 137 and 634 of the test file, but the inconsistency is worth cleaning up.

♻️ Proposed change (requires paired test updates)

-  const file = verifyFileExists(fileId, filesService, res, 'file not found');
+  const file = verifyFileExists(fileId, filesService, res, 'file-not-found');

Also applies to: 387-387

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.ts` at line 178, The error string passed to
verifyFileExists is inconsistent: replace the human-readable 'file not found'
with the kebab-case 'file-not-found' wherever verifyFileExists(fileId,
filesService, res, 'file not found') is used (e.g., the call in app-sync.ts that
uses variables fileId, filesService, res and the other similar call), and update
the corresponding test assertions that expect the old string so they assert
'file-not-found' instead; ensure you only change the string literals passed to
verifyFileExists and keep the function signature and surrounding logic
unchanged.

---

71-78: Operational concern: legacy files without an owner will be inaccessible to non-admin users

file.owner === userId evaluates to false when file.owner is null or '' (i.e., files created before owner-tracking was introduced). Such files will return 403 for any non-admin user, even the original uploader.

Before deploying, confirm that all pre-existing rows in the files table have a populated owner, or add a data migration to back-fill the owner from upload history. Admin users are unaffected.

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.ts` around lines 71 - 78, requireFileAccess
currently denies access when file.owner is null/empty causing legacy uploads to
be inaccessible; fix this by either adding a data migration to backfill
file.owner from upload history before deploying OR change requireFileAccess to
treat missing owners specially: if file.owner is falsy, look up the original
uploader (e.g., via the upload record or a field like file.uploaderId) and
compare that uploaderId to userId, and only deny when neither the uploader nor
isAdmin(userId) matches; update the requireFileAccess function and any call
sites to use this lookup so pre-existing files are accessible to their original
uploader.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@packages/sync-server/src/app-sync.test.ts`:
- Around line 1147-1159: The addMockFile helper currently types groupId as
string but callers sometimes pass null; update the function signature for
addMockFile to accept groupId: string | null and ensure the SQL bind uses that
value directly (allowing null to be inserted) so callers no longer need
`@ts-strict-ignore`; locate the addMockFile function and change its parameter type
and any related test-call sites if necessary to match the new union type.
- Around line 481-515: The tests 'returns 403 when non-owner overwrites another
user file' and 'returns 403 when non-owner downloads another user file'
currently call fs.unlinkSync(getPathForUserFile(fileId)) after assertions, which
leaves temp files if an assertion fails; modify both tests to register cleanup
with onTestFinished(() => fs.unlinkSync(getPathForUserFile(fileId))) immediately
after creating the temp file (before any assertions) so the file is always
removed, referencing the existing getPathForUserFile and fileId variables and
removing the trailing direct fs.unlinkSync calls at the end of each test.
- Around line 75-115: Add parity admin-access tests for the remaining endpoints
listed in the review so admins can bypass file-owner restrictions: create tests
in the same test suite that insert a file row (like the existing tests that set
encrypt_salt/encrypt_keyid/encrypt_test and owner = OTHER_USER_ID) and then call
each endpoint '/user-create-key', '/reset-user-file', '/upload-user-file',
'/download-user-file', '/update-user-filename', and '/get-user-file-info' using
the admin token header ('x-actual-token' = 'valid-token-admin'), asserting a 200
response (or the expected success payload) and confirming the returned data
matches the inserted values; mirror the non-owner 403 tests by also keeping a
non-admin request for each endpoint that asserts 403 and
'file-access-not-allowed' to ensure coverage of both branches.

In `@packages/sync-server/src/app-sync.ts`:
- Line 178: The error string passed to verifyFileExists is inconsistent: replace
the human-readable 'file not found' with the kebab-case 'file-not-found'
wherever verifyFileExists(fileId, filesService, res, 'file not found') is used
(e.g., the call in app-sync.ts that uses variables fileId, filesService, res and
the other similar call), and update the corresponding test assertions that
expect the old string so they assert 'file-not-found' instead; ensure you only
change the string literals passed to verifyFileExists and keep the function
signature and surrounding logic unchanged.
- Around line 71-78: requireFileAccess currently denies access when file.owner
is null/empty causing legacy uploads to be inaccessible; fix this by either
adding a data migration to backfill file.owner from upload history before
deploying OR change requireFileAccess to treat missing owners specially: if
file.owner is falsy, look up the original uploader (e.g., via the upload record
or a field like file.uploaderId) and compare that uploaderId to userId, and only
deny when neither the uploader nor isAdmin(userId) matches; update the
requireFileAccess function and any call sites to use this lookup so pre-existing
files are accessible to their original uploader.

[MatissJanis]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[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 `@packages/sync-server/src/app-sync.test.ts`:
- Around line 75-115: The test suite never triggers the requireFileAccess branch
that grants access via UserService.countUserAccess because existing tests either
use the owner or an admin; add a new test in app-sync.test.ts that inserts a
files row with owner != TEST_USER and then inserts a user_access row for a
non-owner/non-admin user (not the file owner or admin) so that
UserService.countUserAccess > 0 is true, then call the /download-user-file
endpoint (or another endpoint that uses requireFileAccess) with that non-owner
user's token and assert a 200 response (not 403) and expected body, ensuring the
test references requireFileAccess and UserService.countUserAccess behavior
rather than owner/admin shortcuts.

[coderabbitai]

🧹 Nitpick comments (1)

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

415-419: Move onTestFinished cleanup to the top of each test, before any assertions.

onTestFinished is only registered if execution reaches that line. When an assertion throws first, the cleanup is never scheduled and the temp file leaks. This same pattern is repeated at lines 487-491, 585-589, 635-639, 736-740, 761-765, and 798-802.

♻️ Proposed fix (same pattern applies to all affected tests)

 it('uploads a new file successfully', async () => {
   const fileId = crypto.randomBytes(16).toString('hex');
   const filePath = getPathForUserFile(fileId);
+  onTestFinished(() => {
+    try {
+      fs.unlinkSync(filePath);
+    } catch {}
+  });
+
   // ... test body and assertions ...
-
-  onTestFinished(() => {
-    try {
-      fs.unlinkSync(filePath);
-    } catch {}
-  });
 });

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.test.ts` around lines 415 - 419, The
cleanup registration using onTestFinished(() => { try { fs.unlinkSync(filePath);
} catch {} }) must be moved to the very start of each test (immediately after
the temp file is created) so it is always registered even if an assertion
throws; locate each test that currently registers onTestFinished right
before/after assertions (look for onTestFinished, fs.unlinkSync and filePath in
the test bodies) and hoist that registration to the top of the test block,
preserving the try/catch and filePath reference so the temp file is reliably
unlinked; apply the same change to all repeated occurrences found in the file.

🤖 Prompt for all review comments with AI agents

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

Duplicate comments:
In `@packages/sync-server/src/app-sync.test.ts`:
- Around line 768-803: The review comment is a duplicate meta-tag
([duplicate_comment]) and not a code issue; no changes to the test are
required—leave the test "allows non-owner with user_access to download via
requireFileAccess (UserService.countUserAccess > 0)" as written, remove the
duplicate meta-tag from the review/PR comment, and mark the change approved.

---

Nitpick comments:
In `@packages/sync-server/src/app-sync.test.ts`:
- Around line 415-419: The cleanup registration using onTestFinished(() => { try
{ fs.unlinkSync(filePath); } catch {} }) must be moved to the very start of each
test (immediately after the temp file is created) so it is always registered
even if an assertion throws; locate each test that currently registers
onTestFinished right before/after assertions (look for onTestFinished,
fs.unlinkSync and filePath in the test bodies) and hoist that registration to
the top of the test block, preserving the try/catch and filePath reference so
the temp file is reliably unlinked; apply the same change to all repeated
occurrences found in the file.

[MatissJanis]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

🧹 Nitpick comments (4)

packages/sync-server/src/app-sync.ts (2)

72-82: isAdmin is always eagerly evaluated, even when the user is already the file owner.

Lines 73–74 compute both isOwner and isServerAdmin up-front. Since isAdmin performs a DB query (hasPermission), it fires on every access-checked request — including those from the file's owner — adding an unnecessary round-trip.

⚡ Proposed fix (short-circuit evaluation)

 function requireFileAccess(file: File, userId: string) {
-  const isOwner = file.owner === userId;
-  const isServerAdmin = isAdmin(userId);
-  if (isOwner || isServerAdmin) {
+  if (file.owner === userId || isAdmin(userId)) {
     return null;
   }
   if (UserService.countUserAccess(file.id, userId) > 0) {
     return null;
   }
   return 'file-access-not-allowed';
 }

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.ts` around lines 72 - 82, The
requireFileAccess function eagerly calls isAdmin even when the caller is the
file owner; change it so isAdmin(userId) is only invoked if the ownership check
fails: compute isOwner first, return null immediately if true, then call
isAdmin(userId) and proceed with the existing logic (fall back to
UserService.countUserAccess(file.id, userId) if not admin). This keeps the
short-circuit behavior in requireFileAccess and avoids unnecessary DB calls
inside isAdmin/hasPermission.

---

72-82: isAdmin is always eagerly evaluated, even when the user is already the file owner.

Both isOwner (Line 73) and isServerAdmin (Line 74) are computed before the if check, so isAdmin(userId) — which hits the DB — fires on every request even when file.owner === userId.

⚡ Proposed fix (lazy short-circuit)

 function requireFileAccess(file: File, userId: string) {
-  const isOwner = file.owner === userId;
-  const isServerAdmin = isAdmin(userId);
-  if (isOwner || isServerAdmin) {
+  if (file.owner === userId || isAdmin(userId)) {
     return null;
   }
   if (UserService.countUserAccess(file.id, userId) > 0) {
     return null;
   }
   return 'file-access-not-allowed';
 }

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.ts` around lines 72 - 82, In
requireFileAccess, avoid eagerly calling isAdmin(userId): first check isOwner
(file.owner === userId) and return null immediately if true, and only then call
isAdmin(userId) to set isServerAdmin; keep the existing fallback to
UserService.countUserAccess(file.id, userId) as before. If isAdmin is async,
await it when called; otherwise call it synchronously — the key change is to
compute isAdmin only when isOwner is false.

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

760-794: Missing DB-row cleanup for the user_access entry.

onTestFinished at line 774 cleans up the on-disk file, but the user_access row inserted at lines 770–773 is never deleted. Since fileId is random there is no collision risk, but orphaned rows accumulate across test runs.

🧹 Suggested fix

         getAccountDb().mutate(
           'INSERT INTO user_access (file_id, user_id) VALUES (?, ?)',
           [fileId, 'genericUser'],
         );
+        onTestFinished(() => {
+          getAccountDb().mutate(
+            'DELETE FROM user_access WHERE file_id = ?',
+            [fileId],
+          );
+        });
         onTestFinished(() => {

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.test.ts` around lines 760 - 794, The test
inserts a user_access row for fileId but never cleans it up; update the
onTestFinished callback used for file cleanup to also delete the DB rows you
created (at least the user_access row for that fileId, and optionally the files
row) by calling getAccountDb().mutate('DELETE FROM user_access WHERE file_id =
?', [fileId]) (and/or getAccountDb().mutate('DELETE FROM files WHERE id = ?',
[fileId])); place these deletes inside the existing onTestFinished block so all
created state (disk file and DB rows) is removed after the test.

---

760-794: Missing DB cleanup for the user_access row inserted in the countUserAccess test.

The onTestFinished callback at line 774 correctly removes the filesystem file, but the user_access row at lines 770–773 is never removed. While the random fileId prevents primary-key collisions, orphaned rows accumulate across runs in shared test databases.

🧹 Suggested cleanup addition

+        onTestFinished(() => {
+          getAccountDb().mutate(
+            'DELETE FROM user_access WHERE file_id = ?',
+            [fileId],
+          );
+        });
         onTestFinished(() => {
           try {
             fs.unlinkSync(getPathForUserFile(fileId));
           } catch {}
         });

🤖 Prompt for AI Agents

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

In `@packages/sync-server/src/app-sync.test.ts` around lines 760 - 794, The test
inserts a user_access row but never removes it; update the onTestFinished
cleanup for the test that defines fileId to also delete the inserted user_access
row by calling getAccountDb().mutate to delete from user_access where file_id =
fileId (use the same fileId variable) so the test removes the DB row in addition
to unlinking the file.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@packages/sync-server/src/app-sync.test.ts`:
- Around line 760-794: The test inserts a user_access row for fileId but never
cleans it up; update the onTestFinished callback used for file cleanup to also
delete the DB rows you created (at least the user_access row for that fileId,
and optionally the files row) by calling getAccountDb().mutate('DELETE FROM
user_access WHERE file_id = ?', [fileId]) (and/or getAccountDb().mutate('DELETE
FROM files WHERE id = ?', [fileId])); place these deletes inside the existing
onTestFinished block so all created state (disk file and DB rows) is removed
after the test.
- Around line 760-794: The test inserts a user_access row but never removes it;
update the onTestFinished cleanup for the test that defines fileId to also
delete the inserted user_access row by calling getAccountDb().mutate to delete
from user_access where file_id = fileId (use the same fileId variable) so the
test removes the DB row in addition to unlinking the file.

In `@packages/sync-server/src/app-sync.ts`:
- Around line 72-82: The requireFileAccess function eagerly calls isAdmin even
when the caller is the file owner; change it so isAdmin(userId) is only invoked
if the ownership check fails: compute isOwner first, return null immediately if
true, then call isAdmin(userId) and proceed with the existing logic (fall back
to UserService.countUserAccess(file.id, userId) if not admin). This keeps the
short-circuit behavior in requireFileAccess and avoids unnecessary DB calls
inside isAdmin/hasPermission.
- Around line 72-82: In requireFileAccess, avoid eagerly calling
isAdmin(userId): first check isOwner (file.owner === userId) and return null
immediately if true, and only then call isAdmin(userId) to set isServerAdmin;
keep the existing fallback to UserService.countUserAccess(file.id, userId) as
before. If isAdmin is async, await it when called; otherwise call it
synchronously — the key change is to compute isAdmin only when isOwner is false.