diff --git a/server/lib/fsutil.js b/server/lib/fsutil.js index 3da44f6..02f0286 100644 --- a/server/lib/fsutil.js +++ b/server/lib/fsutil.js @@ -40,6 +40,27 @@ function copyFileBytes(src, dest) { while (written < read) written += fs.writeSync(outFd, buf, written, read - written); pos += read; } + /* + * Carry the source's permissions across where the filesystem has any. + * + * ⚠️ NOT optional, and the reason this function exists does not excuse skipping it. Dropping + * the chmod entirely - which is what the first version did - creates the copy at the default + * 0666 & ~umask. A database snapshot that was 0600 came out 0664, so the whole database became + * group- and world-readable on every install. That is a worse bug than the one this function + * was written to fix. + * + * Doing it as a SEPARATE, failure-tolerant step is the difference from fs.copyFileSync: there + * the chmod is inseparable from the copy, so a filesystem that refuses modes - exFAT, which is + * what a BrightSign player's storage is - fails the whole operation with EPERM. Here the bytes + * are already written and safe; the mode is applied if it can be, and its refusal is not an + * error because on such a filesystem there were never permissions to preserve. + */ + try { + fs.fchmodSync(outFd, fs.fstatSync(inFd).mode & 0o777); + } catch (e) { + /* no permission bits on this filesystem - nothing to carry across */ + } + // The caller is usually taking a backup it is about to rely on, so make sure the bytes are // actually on the device before it proceeds to modify the original. fs.fsyncSync(outFd); diff --git a/server/test/fsutil.test.js b/server/test/fsutil.test.js index 72c9909..50b96d0 100644 --- a/server/test/fsutil.test.js +++ b/server/test/fsutil.test.js @@ -73,3 +73,43 @@ test('a missing source throws rather than leaving an empty destination behind', assert.strictEqual(fs.existsSync(dest), false); fs.rmSync(dir, { recursive: true, force: true }); }); + +test('the copy carries the source permissions across', () => { + // ⚠️ REGRESSION GUARD. The first version of copyFileBytes dropped the chmod entirely, because + // chmod is exactly what made fs.copyFileSync fail on exFAT. The copy then landed at the default + // 0666 & ~umask: a 0600 database snapshot came out 0664, making the whole database group- and + // world-readable on every install. Removing a permission check to fix a permission error is not + // a fix. + const dir = tmp(); + const src = path.join(dir, 'db.sqlite'); + const dest = path.join(dir, 'snapshot.db'); + fs.writeFileSync(src, 'SQLite format 3\0payload'); + fs.chmodSync(src, 0o600); + + copyFileBytes(src, dest); + + const mode = (p) => fs.statSync(p).mode & 0o777; + assert.strictEqual(mode(dest), 0o600, + `snapshot should be 0600 like its source, was 0${mode(dest).toString(8)}`); + fs.rmSync(dir, { recursive: true, force: true }); +}); + +test('a filesystem that refuses chmod still gets its bytes', () => { + // The exFAT case, which is the whole reason this function exists rather than fs.copyFileSync. + // The bytes are written before the mode is attempted, so a refusal must not fail the copy. + const dir = tmp(); + const src = path.join(dir, 'a.bin'); + const dest = path.join(dir, 'b.bin'); + const data = Buffer.from('bytes that must survive a chmod refusal'); + fs.writeFileSync(src, data); + + const realFchmod = fs.fchmodSync; + fs.fchmodSync = () => { const e = new Error('EPERM: operation not permitted, fchmod'); e.code = 'EPERM'; throw e; }; + try { + assert.doesNotThrow(() => copyFileBytes(src, dest), 'a refused chmod must not fail the copy'); + assert.deepStrictEqual(fs.readFileSync(dest), data); + } finally { + fs.fchmodSync = realFchmod; + } + fs.rmSync(dir, { recursive: true, force: true }); +});