diff --git a/foldersync/package.json b/foldersync/package.json index f5c8a0f..38e5d33 100644 --- a/foldersync/package.json +++ b/foldersync/package.json @@ -8,7 +8,7 @@ "scripts": { "app": "npm run build && electron .", "build": "tsc -p tsconfig.json && node scripts/css-types.mjs && tsc -p tsconfig.renderer.json --noEmit && node scripts/build-renderer.mjs && node scripts/copy-static.mjs", - "build:tokenserver": "cd tokenserver && go build -o ../out/tokenserver .", + "build:tokenserver": "cd tokenserver && go build -o ../out/ .", "format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\" \"scripts/**/*.mjs\" \"*.mjs\" \"*.mts\"", "format:check": "prettier --check \"src/**/*.ts\" \"test/**/*.ts\" \"scripts/**/*.mjs\" \"*.mjs\" \"*.mts\"", "lint": "eslint . && stylelint \"src/**/*.css\"", diff --git a/foldersync/scripts/start.mjs b/foldersync/scripts/start.mjs index b5250b9..2e6d27e 100644 --- a/foldersync/scripts/start.mjs +++ b/foldersync/scripts/start.mjs @@ -18,7 +18,9 @@ if (!/^\d+$/.test(port) || Number(port) < 1 || Number(port) > 65535) { const tokenServerUrl = `http://127.0.0.1:${port}`; const home = process.env.FOLDERSYNC_HOME ?? join(homedir(), '.foldersync'); const appKeyPath = join(home, 'tokenserver', 'app_key'); -const tokenServerBin = process.env.FOLDERSYNC_TOKENSERVER ?? join(root, 'out', 'tokenserver'); +const tokenServerBin = + process.env.FOLDERSYNC_TOKENSERVER ?? + join(root, 'out', process.platform === 'win32' ? 'tokenserver.exe' : 'tokenserver'); async function tokenServerRunning() { try { diff --git a/foldersync/src/engine/contentHash.ts b/foldersync/src/engine/contentHash.ts index 6ded00f..3c22752 100644 --- a/foldersync/src/engine/contentHash.ts +++ b/foldersync/src/engine/contentHash.ts @@ -1,8 +1,8 @@ // Dropbox content_hash: SHA-256 of each 4 MiB block, then SHA-256 of the // concatenated block hashes, as hex. import {createHash, type Hash} from 'node:crypto'; -import {constants} from 'node:fs'; -import {open} from 'node:fs/promises'; + +import {openNoFollow} from './openFile'; export const BLOCK_SIZE = 4 * 1024 * 1024; @@ -46,7 +46,7 @@ export function hashBuffer(data: Uint8Array): string { // Hashes a file. Fails if the path is a symlink, so it never reads outside the project. export async function hashFile(path: string): Promise { - const file = await open(path, constants.O_RDONLY | constants.O_NOFOLLOW); + const file = await openNoFollow(path); try { const hasher = new ContentHasher(); for await (const chunk of file.createReadStream({autoClose: false})) { diff --git a/foldersync/src/engine/dropboxApi.ts b/foldersync/src/engine/dropboxApi.ts index 48e80ac..01c4158 100644 --- a/foldersync/src/engine/dropboxApi.ts +++ b/foldersync/src/engine/dropboxApi.ts @@ -2,10 +2,10 @@ // rpc() for other routes. Uploads send their data in parallel and commit in // batches, one batch at a time, since Dropbox limits how many writes run at // once in an account. -import {constants} from 'node:fs'; import {open, rm, stat} from 'node:fs/promises'; import {ContentHasher, hashBuffer} from './contentHash'; +import {openNoFollow} from './openFile'; export interface FileMeta { tag: 'file'; @@ -252,8 +252,8 @@ export class DropboxClient implements DropboxFiles { this.sendingData++; try { size = (await stat(req.localPath)).size; - // O_NOFOLLOW: if the file was swapped for a symlink, fail instead of uploading its target. - const file = await open(req.localPath, constants.O_RDONLY | constants.O_NOFOLLOW); + // If the file was swapped for a symlink, fail instead of uploading its target. + const file = await openNoFollow(req.localPath); try { sessionId = await this.uploadData(file, size, req); } finally { diff --git a/foldersync/src/engine/openFile.ts b/foldersync/src/engine/openFile.ts new file mode 100644 index 0000000..31494bb --- /dev/null +++ b/foldersync/src/engine/openFile.ts @@ -0,0 +1,31 @@ +// Opens a local file to read. Fails if the path itself is a symlink, or is +// swapped for another file or a symlink while it's being opened. Callers check +// that the path is inside the project. +import {constants} from 'node:fs'; +import {lstat, open, type FileHandle} from 'node:fs/promises'; + +export async function openNoFollow(path: string): Promise { + const before = await lstat(path, {bigint: true}); + if (before.isSymbolicLink()) { + throw new Error(`${path} is a symbolic link.`); + } + // Windows has no O_NOFOLLOW, so the checks below also cover a swap there. + const file = await open(path, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0)); + try { + const opened = await file.stat({bigint: true}); + const now = await lstat(path, {bigint: true}); + if ( + now.isSymbolicLink() || + opened.ino !== before.ino || + opened.dev !== before.dev || + now.ino !== before.ino || + now.dev !== before.dev + ) { + throw new Error(`${path} changed while it was being opened.`); + } + return file; + } catch (err) { + await file.close(); + throw err; + } +} diff --git a/foldersync/test/account.test.ts b/foldersync/test/account.test.ts index cc1971c..c164abd 100644 --- a/foldersync/test/account.test.ts +++ b/foldersync/test/account.test.ts @@ -8,6 +8,7 @@ import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; import {Account, serverUrl} from '../src/main/account'; import {Settings} from '../src/main/config'; import type {TokenState} from '../src/shared/types'; +import {NO_CHMOD, chmodBlocksAccess} from './fsSupport'; let dir: string; let credentialsPath: string; @@ -153,7 +154,8 @@ describe('Account', () => { expect(redirects.every((r) => r === 'error')).toBe(true); }); - it('keeps the previous account when settings can’t be saved', async () => { + it('keeps the previous account when settings can’t be saved', async ({skip}) => { + skip(!chmodBlocksAccess, NO_CHMOD); const server = twoUserServer(); server.releaseA(); const a = await account(server.fetchImpl); @@ -171,7 +173,10 @@ describe('Account', () => { expect(() => a.userClient()).not.toThrow(); }); - it('stays signed out of a deleted account when its credentials can’t be removed', async () => { + it('stays signed out of a deleted account when its credentials can’t be removed', async ({ + skip, + }) => { + skip(!chmodBlocksAccess, NO_CHMOD); const server = twoUserServer(); server.releaseA(); const a = await account(server.fetchImpl); @@ -250,7 +255,8 @@ describe('Account', () => { expect(a.setupInfo().signedIn).toBe(false); }); - it('stays signed in when the credentials file can’t be deleted', async () => { + it('stays signed in when the credentials file can’t be deleted', async ({skip}) => { + skip(!chmodBlocksAccess, NO_CHMOD); const server = tokenServer(); server.release(); const a = await account(server.fetchImpl); diff --git a/foldersync/test/credentials.test.ts b/foldersync/test/credentials.test.ts index aa8259f..0186648 100644 --- a/foldersync/test/credentials.test.ts +++ b/foldersync/test/credentials.test.ts @@ -5,6 +5,7 @@ import {join} from 'node:path'; import {afterEach, beforeEach, describe, expect, it} from 'vitest'; import {loadCredentials, saveCredentials, type Credentials} from '../src/engine/credentials'; +import {isWindows} from './fsSupport'; let dir: string; let path: string; @@ -35,7 +36,8 @@ describe('credentials file', () => { expect(await loadCredentials(path)).toEqual(creds); }); - it('writes the file 0600 in a 0700 folder, even if they were looser', async () => { + it('writes the file 0600 in a 0700 folder, even if they were looser', async ({skip}) => { + skip(isWindows, 'Windows has no POSIX file modes'); await chmod(join(dir, 'home'), 0o755); await writeFile(path, '{}', {mode: 0o644}); await writeFile(`${path}.tmp`, 'old', {mode: 0o644}); diff --git a/foldersync/test/filesPanel.test.ts b/foldersync/test/filesPanel.test.ts index 8dcf94d..c12ac83 100644 --- a/foldersync/test/filesPanel.test.ts +++ b/foldersync/test/filesPanel.test.ts @@ -1,6 +1,6 @@ import {mkdir, mkdtemp, readFile, realpath, rm, symlink, writeFile} from 'node:fs/promises'; import {tmpdir} from 'node:os'; -import {join} from 'node:path'; +import {basename, isAbsolute, join} from 'node:path'; import {afterEach, beforeEach, describe, expect, it} from 'vitest'; @@ -167,7 +167,8 @@ describe('FilesPanel', () => { name: 'payload.exe', }); - expect(opened.endsWith(`${'/'}a.txt`)).toBe(true); + expect(isAbsolute(opened)).toBe(true); + expect(basename(opened)).toBe('a.txt'); panel.removeOpenCopies(); }); diff --git a/foldersync/test/fsSupport.ts b/foldersync/test/fsSupport.ts new file mode 100644 index 0000000..a542aeb --- /dev/null +++ b/foldersync/test/fsSupport.ts @@ -0,0 +1,26 @@ +// What this computer's file system supports, so tests that need it can skip +// where it's missing. +import {chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync} from 'node:fs'; +import {tmpdir} from 'node:os'; +import {join} from 'node:path'; + +// Whether chmod can stop this user reading a file. It can't on Windows, or as root. +export const chmodBlocksAccess = ((): boolean => { + const dir = mkdtempSync(join(tmpdir(), 'foldersync-chmod-')); + const file = join(dir, 'probe'); + writeFileSync(file, 'x'); + chmodSync(file, 0o000); + try { + readFileSync(file); + return false; + } catch { + return true; + } finally { + chmodSync(file, 0o600); + rmSync(dir, {recursive: true, force: true}); + } +})(); + +export const NO_CHMOD = 'chmod can’t block access on this computer'; + +export const isWindows = process.platform === 'win32'; diff --git a/foldersync/test/openFile.test.ts b/foldersync/test/openFile.test.ts new file mode 100644 index 0000000..e3def86 --- /dev/null +++ b/foldersync/test/openFile.test.ts @@ -0,0 +1,89 @@ +import type {FileHandle} from 'node:fs/promises'; +import {lstat, mkdtemp, open, rename, rm, symlink, writeFile} from 'node:fs/promises'; +import {tmpdir} from 'node:os'; +import {join} from 'node:path'; + +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; + +import {openNoFollow} from '../src/engine/openFile'; + +// The real functions, wrapped so a test can swap what lstat sees and look at +// the handle open returned. +vi.mock('node:fs/promises', async (importOriginal) => { + const real = await importOriginal(); + return {...real, lstat: vi.fn(real.lstat), open: vi.fn(real.open)}; +}); + +let dir: string; + +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'foldersync-open-')); + await writeFile(join(dir, 'a.txt'), 'A'); + await writeFile(join(dir, 'b.txt'), 'B'); +}); + +afterEach(async () => { + vi.mocked(lstat).mockClear(); + vi.mocked(open).mockClear(); + await rm(dir, {recursive: true, force: true}); +}); + +describe('openNoFollow', () => { + it('opens a regular file', async () => { + const file = await openNoFollow(join(dir, 'a.txt')); + try { + expect((await file.readFile()).toString()).toBe('A'); + } finally { + await file.close(); + } + }); + + it('refuses a symlink without opening it', async () => { + await symlink(join(dir, 'a.txt'), join(dir, 'link.txt')); + + await expect(openNoFollow(join(dir, 'link.txt'))).rejects.toThrow(/symbolic link/); + expect(open).not.toHaveBeenCalled(); + }); + + it('refuses and closes a file that isn’t the one lstat saw', async () => { + // As if a.txt were swapped for b.txt between lstat and open. + vi.mocked(lstat).mockResolvedValueOnce(await lstat(join(dir, 'b.txt'), {bigint: true})); + + await expect(openNoFollow(join(dir, 'a.txt'))).rejects.toThrow( + /changed while it was being opened/ + ); + const handle = (await vi.mocked(open).mock.results[0]?.value) as FileHandle; + expect(handle.fd).toBe(-1); + }); + + it('refuses and closes a file whose path is replaced right after it opens', async () => { + // lstat sees a.txt before the open, then b.txt, as if a.txt were replaced. + const a = await lstat(join(dir, 'a.txt'), {bigint: true}); + const b = await lstat(join(dir, 'b.txt'), {bigint: true}); + vi.mocked(lstat).mockResolvedValueOnce(a).mockResolvedValueOnce(b); + + await expect(openNoFollow(join(dir, 'a.txt'))).rejects.toThrow( + /changed while it was being opened/ + ); + const handle = (await vi.mocked(open).mock.results[0]?.value) as FileHandle; + expect(handle.fd).toBe(-1); + }); + + it('refuses a file swapped for a symlink to itself while it opens', async () => { + const real = await vi.importActual('node:fs/promises'); + let handle: FileHandle | undefined; + // Moves a.txt and links its old name to it, right before the open. + vi.mocked(open).mockImplementationOnce(async (...args) => { + await rename(join(dir, 'a.txt'), join(dir, 'moved.txt')); + await symlink(join(dir, 'moved.txt'), join(dir, 'a.txt')); + handle = await real.open(...args); + return handle; + }); + + // macOS and Linux refuse the open itself (ELOOP); Windows opens it, then checks. + await expect(openNoFollow(join(dir, 'a.txt'))).rejects.toThrow( + /changed while it was being opened|ELOOP/ + ); + expect(handle?.fd ?? -1).toBe(-1); + }); +}); diff --git a/foldersync/test/projects.test.ts b/foldersync/test/projects.test.ts index b3ff3a9..5dca567 100644 --- a/foldersync/test/projects.test.ts +++ b/foldersync/test/projects.test.ts @@ -10,6 +10,7 @@ import {ProjectClients, type UserApi} from '../src/main/projectClients'; import {Projects} from '../src/main/projects'; import type {SyncStatus} from '../src/shared/types'; import type {DropboxFiles} from '../src/engine/dropboxApi'; +import {NO_CHMOD, chmodBlocksAccess} from './fsSupport'; import {FakeRpc, agent, memoryDirectory} from './fakeRpc'; const WAIT_MS = 10 * 60 * 1000; @@ -428,7 +429,8 @@ describe('Projects.remove', () => { expect(existsSync(join(local, '.foldersync', 'state.db'))).toBe(true); }); - it('says so when the project is removed but its sync state can’t be deleted', async () => { + it('says so when the project is removed but its sync state can’t be deleted', async ({skip}) => { + skip(!chmodBlocksAccess, NO_CHMOD); const {projects} = await setup(() => 'complete'); await projects.setLocalDir('storyboard', join(dir, 'work', 'storyboard')); const state = join(dir, 'work', 'storyboard', '.foldersync'); diff --git a/foldersync/test/syncPass.test.ts b/foldersync/test/syncPass.test.ts index d0cd22a..ce68463 100644 --- a/foldersync/test/syncPass.test.ts +++ b/foldersync/test/syncPass.test.ts @@ -25,6 +25,7 @@ import {Pool} from '../src/engine/pool'; import {StateStore} from '../src/engine/stateStore'; import {runSyncPass, type PassOptions} from '../src/engine/syncPass'; import {FakeDropbox} from './fakeDropbox'; +import {NO_CHMOD, chmodBlocksAccess, isWindows} from './fsSupport'; const ROOT = '/project'; const ACCOUNT = 'pid_user:a'; @@ -320,7 +321,8 @@ describe('sync pass', () => { expect(dropbox.files.size).toBe(0); }); - it('refuses to sync when the project folder is missing', async () => { + it('refuses to sync when the project folder is missing', async ({skip}) => { + skip(isWindows, 'Windows can’t delete the folder while the sync database in it is open'); dropbox.put('/project/a.txt', 'A'); await sync(); await rm(dir, {recursive: true, force: true}); @@ -511,7 +513,8 @@ describe('sync pass', () => { expect((await sync()).issues).toEqual([]); }); - it('syncs the other files when one can’t be read', async () => { + it('syncs the other files when one can’t be read', async ({skip}) => { + skip(!chmodBlocksAccess, NO_CHMOD); await writeLocal('locked.txt', 'L'); await writeLocal('a.txt', 'A'); await chmod(join(dir, 'locked.txt'), 0o000); @@ -527,7 +530,8 @@ describe('sync pass', () => { expect(dropbox.content('/project/locked.txt')).toBe('L'); }); - it('syncs the rest of the project when a folder can’t be read', async () => { + it('syncs the rest of the project when a folder can’t be read', async ({skip}) => { + skip(!chmodBlocksAccess, NO_CHMOD); dropbox.put('/project/locked/keep.txt', 'K'); await sync(); await writeLocal('a.txt', 'A'); @@ -542,7 +546,8 @@ describe('sync pass', () => { expect((await sync()).issues).toEqual([]); }); - it('syncs the rest of the project when a folder’s items can’t be read', async () => { + it('syncs the rest of the project when a folder’s items can’t be read', async ({skip}) => { + skip(!chmodBlocksAccess, NO_CHMOD); dropbox.put('/project/listed/keep.txt', 'K'); await sync(); await writeLocal('a.txt', 'A'); @@ -680,7 +685,8 @@ describe('sync pass', () => { expect(backups.some((n) => n.endsWith('漢.txt'))).toBe(true); }); - it('never uploads the state folder when it is spelled .FolderSync', async () => { + it('never uploads the state folder when it is spelled .FolderSync', async ({skip}) => { + skip(isWindows, 'Windows can’t rename the state folder while the sync database in it is open'); await rename(join(dir, STATE_DIR), join(dir, '.FolderSync')); await writeLocal('.FolderSync/notes.txt', 'private'); await writeLocal('a.txt', 'A'); @@ -848,7 +854,13 @@ describe('sync pass', () => { await rm(outside, {recursive: true, force: true}); }); - it('stops writing when the project folder is swapped for a link during the pass', async () => { + it('stops writing when the project folder is swapped for a link during the pass', async ({ + skip, + }) => { + skip( + isWindows, + 'Windows can’t rename the project folder while the sync database in it is open' + ); dropbox.put('/project/a.txt', 'A'); await sync(); const moved = `${dir}-moved`; @@ -870,7 +882,13 @@ describe('sync pass', () => { await rename(moved, dir); }); - it('deletes nothing in Dropbox when the project folder is replaced before the scan', async () => { + it('deletes nothing in Dropbox when the project folder is replaced before the scan', async ({ + skip, + }) => { + skip( + isWindows, + 'Windows can’t rename the project folder while the sync database in it is open' + ); dropbox.put('/project/a.txt', 'A'); dropbox.put('/project/b.txt', 'B'); await sync(); @@ -1092,7 +1110,8 @@ describe('sync pass', () => { expect(await local('shots/a.txt')).toBe('A'); }); - it('keeps a synced file in Dropbox after it is replaced by a FIFO', async () => { + it('keeps a synced file in Dropbox after it is replaced by a FIFO', async ({skip}) => { + skip(isWindows, 'Windows has no FIFOs'); dropbox.put('/project/a.txt', 'A'); await sync(); await rm(join(dir, 'a.txt')); diff --git a/foldersync/tokenserver/tokenmint/users_test.go b/foldersync/tokenserver/tokenmint/users_test.go index 08093d9..39a5e1e 100644 --- a/foldersync/tokenserver/tokenmint/users_test.go +++ b/foldersync/tokenserver/tokenmint/users_test.go @@ -3,6 +3,7 @@ package tokenmint import ( "os" "path/filepath" + "runtime" "strings" "testing" @@ -32,7 +33,10 @@ func TestUsers_AddSaveAndLoad(t *testing.T) { info, err := os.Stat(path) require.NoError(t, err) - require.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + // Windows has no POSIX file modes. + if runtime.GOOS != "windows" { + require.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + } leftovers, err := filepath.Glob(path + ".tmp*") require.NoError(t, err) require.Empty(t, leftovers) @@ -102,7 +106,7 @@ func TestDefaultUsersPath(t *testing.T) { return "" }) require.NoError(t, err) - require.Equal(t, "/tmp/fs/tokenserver/users.json", path) + require.Equal(t, filepath.Join("/tmp/fs", "tokenserver", "users.json"), path) } func TestUsers_RejectsADuplicateName(t *testing.T) {