diff --git a/.env.example b/.env.example index 544f455a..ac4cac87 100644 --- a/.env.example +++ b/.env.example @@ -67,6 +67,9 @@ DB_NAME=picpeak_prod # written to data/SETUP_TOKEN with mode 0600 — read it with # `docker compose exec backend cat /app/data/SETUP_TOKEN`. It is NOT logged # unless that write fails, so it never sits in `docker logs`. +# The all-in-one image keeps it at /data/db/SETUP_TOKEN — inside the volume, +# in the db/ subdirectory (#1218). On a NAS with no shell, set ADMIN_PASSWORD +# below instead: it needs no file at all. # Set ADMIN_PASSWORD to auto-create the admin on first boot instead (legacy; # credentials written to data/ADMIN_CREDENTIALS.txt). #ADMIN_USERNAME=admin diff --git a/backend/__tests__/integration/setupService.test.js b/backend/__tests__/integration/setupService.test.js index 7e4f234a..664a83ee 100644 --- a/backend/__tests__/integration/setupService.test.js +++ b/backend/__tests__/integration/setupService.test.js @@ -105,6 +105,133 @@ describe('setupService (first-run bootstrap)', () => { expect(fs.existsSync(tokenFile)).toBe(false); // burned in DB + file removed }); + it('restores 0600 on a token file that already existed with looser permissions (#1218)', async () => { + // fs.writeFileSync's `mode` applies only when the file is created, so + // writing over a 0644 file left the first-admin credential group- and + // world-readable while the code claimed otherwise. On a NAS the volume is + // often a shared mount, which is exactly where that matters. + const canonical = path.join(tmpDir, 'SETUP_TOKEN'); + fs.writeFileSync(canonical, 'stale\n', { mode: 0o644 }); + fs.chmodSync(canonical, 0o644); + expect(fs.statSync(canonical).mode & 0o777).toBe(0o644); + + const token = await setupService.ensureSetupToken(); + + expect(fs.readFileSync(canonical, 'utf8').trim()).toBe(token); + expect(fs.statSync(canonical).mode & 0o777).toBe(0o600); + }); + + it('never publishes a token it cannot make private (#1218)', async () => { + // The CIFS/SMB case this targets: the mount carries no Unix modes, so + // chmod is a silent no-op. The check runs on the temporary file, before + // the rename, so a credential that cannot be protected never reaches the + // published path at all. + const canonical = path.join(tmpDir, 'SETUP_TOKEN'); + try { fs.unlinkSync(canonical); } catch (_) { /* start clean */ } + const chmodSpy = jest.spyOn(fs, 'chmodSync').mockImplementation(() => {}); + const realLstat = fs.lstatSync; + const lstatSpy = jest.spyOn(fs, 'lstatSync').mockImplementation((target, ...rest) => { + const st = realLstat(target, ...rest); + return String(target).includes('SETUP_TOKEN') + ? { ...st, mode: (st.mode & ~0o777) | 0o644 } + : st; + }); + + try { + const token = await setupService.ensureSetupToken(); + // Setup stays completable — server.js prints the token on stdout when no + // file was written — but nothing readable was left on the volume. + expect(token).toBeTruthy(); + expect(fs.existsSync(canonical)).toBe(false); + expect(fs.readdirSync(tmpDir).filter((f) => f.includes('.tmp'))).toEqual([]); + } finally { + chmodSpy.mockRestore(); + lstatSpy.mockRestore(); + } + }); + + it('keeps the token out of the log files when no private copy is possible (#1218)', async () => { + // LOG_DIR is on the same mount as the token in the all-in-one image, so + // logging the credential would put it in combined.log — as readable as the + // file we just refused to leave, and it outlives setup. stdout is the + // fallback instead, which server.js prints. + const logger = require('../../src/utils/logger'); + try { fs.unlinkSync(path.join(tmpDir, 'SETUP_TOKEN')); } catch (_) { /* start clean */ } + const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {}); + const chmodSpy = jest.spyOn(fs, 'chmodSync').mockImplementation(() => {}); + const realStat = fs.lstatSync; + const statSpy = jest.spyOn(fs, 'lstatSync').mockImplementation((target, ...rest) => { + const st = realStat(target, ...rest); + // The mode check runs on the temporary file, so match the prefix. + return String(target).includes('SETUP_TOKEN') + ? { ...st, mode: (st.mode & ~0o777) | 0o644 } + : st; + }); + + try { + const token = await setupService.ensureSetupToken(); + const logged = warnSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(logged).toMatch(/could not write a private setup token file/i); + expect(logged).not.toContain(token); + } finally { + warnSpy.mockRestore(); + chmodSpy.mockRestore(); + statSpy.mockRestore(); + } + }); + + it('tells the startup banner where the token went, so it is never printed (#1218)', async () => { + // server.js prints the token itself only when no file was written. If this + // reports nothing after a successful write, the banner takes that failure + // branch and puts the live credential into stdout and `docker logs` beside + // a perfectly good 0600 file. + const token = await setupService.ensureSetupToken(); + expect(token).toBeTruthy(); + + expect(setupService.writtenSetupTokenFile()).toBe(path.join(tmpDir, 'SETUP_TOKEN')); + }); + + it('lets a second worker publish without disturbing the first (#1218)', async () => { + // The shipped PM2 cluster config runs several workers against one DATA_DIR. + // Publishing through rename means they simply overwrite the same value in + // turn — no shared inode to race, and neither worker can end up reporting + // nothing written and printing the live token to its own log. + const canonical = path.join(tmpDir, 'SETUP_TOKEN'); + const first = await setupService.ensureSetupToken(); + expect(setupService.writtenSetupTokenFile()).toBe(canonical); + + const second = await setupService.ensureSetupToken(); + + expect(second).toBe(first); + expect(setupService.writtenSetupTokenFile()).toBe(canonical); + expect(fs.readFileSync(canonical, 'utf8').trim()).toBe(first); + expect(fs.statSync(canonical).mode & 0o777).toBe(0o600); + // No temporary files left lying about. + expect(fs.readdirSync(tmpDir).filter((f) => f.includes('.tmp'))).toEqual([]); + }); + + it('revokes the token when it cannot replace an exposed file (#1218)', async () => { + // A restart reuses the token from the database, so a file left at the + // token path may hold the live value. If it cannot be replaced — an + // ACL-backed or read-only directory — that credential is out of our + // control, and /setup/admin would go on accepting it. + const canonical = path.join(tmpDir, 'SETUP_TOKEN'); + await setupService.ensureSetupToken(); + + const renameSpy = jest.spyOn(fs, 'renameSync').mockImplementation(() => { + const err = new Error('EACCES'); err.code = 'EACCES'; throw err; + }); + try { + expect(await setupService.ensureSetupToken()).toBeNull(); + expect(await getAppSetting('setup_token')).toBeFalsy(); + // And the temporary file did not survive the failure. + expect(fs.readdirSync(tmpDir).filter((f) => f.includes('.tmp'))).toEqual([]); + } finally { + renameSpy.mockRestore(); + try { fs.unlinkSync(canonical); } catch (_) { /* may be gone */ } + } + }); + it('refuses to create a second admin (setup already complete)', async () => { const token = await setupService.ensureSetupToken(); await setupService.createInitialAdmin({ token, email: 'first@example.com', password: VALID_PW }); diff --git a/backend/src/services/setupService.js b/backend/src/services/setupService.js index 467ee2aa..17c9ce6c 100644 --- a/backend/src/services/setupService.js +++ b/backend/src/services/setupService.js @@ -105,28 +105,124 @@ async function ensureSetupToken() { // existsSync() guess: printing the token there lands it in `docker logs` / // journald, which is the very leak this closes, and suppressing it when the // file is NOT actually current strands the operator with no token at all. - let file = null; + // Every copy is attempted independently. The canonical one failing while the + // volume-root copy succeeds still leaves the operator a readable token, and + // the reverse is the compose case where there is only ever one — so success + // is "at least one file exists", not "the first one did". + // One file, in DATA_DIR (#1218). A second copy at the volume root was tried + // for discoverability and dropped: it carried almost the whole security + // surface of this function — a second inode to race, to verify, and to + // revoke — for a convenience the documentation covers better, by pointing + // NAS users at ADMIN_PASSWORD, which needs no file at all. + const candidate = setupTokenFilePath(); + let written = null; + // Set when the file is readable by others AND cannot be deleted: a live + // credential we do not control. Kept separate from writeError because it + // must not be cleared by anything else succeeding. + let leftReadable = null; let writeError = null; + // Written to a private temporary file and published with rename(2) + // (#1218 review). Every earlier shape raced: unlink-then-create left a + // window for a symlink, and exclusive-create left two PM2 workers fighting + // over one inode — the loser could see the winner's file after creation but + // before its content landed, judge it wrong, and delete it, after which both + // workers reported nothing written and both printed the live token. + // + // rename is atomic and replaces the path entry itself, so: the name is + // unique to this process and cannot be raced, the file never appears at the + // final path with the wrong mode or half its content, a symlink sitting + // there is replaced rather than followed, and concurrent workers simply + // publish the same value one after another. + const tmp = `${candidate}.${process.pid}.tmp`; + let createdTmp = false; try { - file = setupTokenFilePath(); - fs.mkdirSync(path.dirname(file), { recursive: true }); - fs.writeFileSync(file, `${token}\n`, { mode: 0o600 }); - writtenTokenFile = file; + fs.mkdirSync(path.dirname(candidate), { recursive: true }); + + fs.writeFileSync(tmp, `${token}\n`, { mode: 0o600, flag: 'wx' }); + createdTmp = true; + try { fs.chmodSync(tmp, 0o600); } catch (_) { /* verified next */ } + + // Checked before publishing, not after. Asking is not the same as + // succeeding: a CIFS/SMB mount — what a NAS commonly offers — carries no + // Unix modes, so chmod is a silent no-op and the file keeps whatever + // file_mode= the mount forces, typically 0644. Verifying here means a + // credential that cannot be made private never reaches the published path + // at all. lstat, not stat: it must describe the file, not a link target. + const mode = fs.lstatSync(tmp).mode & 0o777; + if (mode & 0o077) { + throw new Error( + `refusing to write a group/world-readable setup token (mode ${mode.toString(8)})` + ); + } + + // rename consumes tmp, so the catch below has nothing left to clean up. + fs.renameSync(tmp, candidate); + written = candidate; } catch (err) { + if (createdTmp) { + try { fs.unlinkSync(tmp); } catch (_) { /* best-effort */ } + } + // Publishing failed and something is still sitting at the token path. On a + // restart the token is reused from the database, so that file may hold the + // live value — and we could not replace it. Treat it as exposed: the + // revocation below turns what is there into a dead string rather than + // leaving a credential we do not control. + if (fs.existsSync(candidate)) { + leftReadable = candidate; + } writeError = err; - file = null; + } + + // Publish what landed, for server.js's banner. Dropping these assignments + // is not a cosmetic bug: writtenSetupTokenFile() reading null makes the + // banner take its failure branch and print the live token to stdout, so a + // perfectly good 0600 file coexists with the credential in `docker logs` — + // the exact leak this whole path exists to close. + writtenTokenFile = written; + if (written) writeError = null; + + if (leftReadable) { + // Fail closed (#1218 review). A readable copy that cannot be deleted is a + // live first-admin credential sitting where anyone on the mount can read + // it, and /setup/admin would go on accepting it — so the token is revoked + // instead of merely reported. What is left on disk becomes a dead string. + // + // Copies that DID land privately are removed too: they hold the same value, + // which is about to stop working. The next boot mints a fresh token, and + // the undeletable file is skipped rather than rewritten because its unlink + // still fails — so this converges instead of looping on the same exposure. + if (written) { + try { fs.unlinkSync(written); } catch (_) { /* best-effort */ } + } + await upsertAppSetting(SETUP_TOKEN_KEY, null, 'string'); + writtenTokenFile = null; + + logger.error( + `[setup] The setup token file at ${leftReadable} is readable by other ` + + 'users and could not be removed, so the token has been revoked and no admin ' + + 'can be created with it. Delete that file, then restart to issue a new one.' + ); + return null; } if (writeError) { + // Deliberately WITHOUT the token (#1218 review). This branch fires when no + // copy could be written privately — on the all-in-one image that is + // typically a mount with no Unix modes, and LOG_DIR sits on that same + // mount, so logger.warn would write the credential into combined.log: + // exactly as readable as the file we just refused to leave, and it + // outlives setup. server.js prints the token on stdout instead when no + // file was written, which reaches `docker logs` without landing on the + // shared volume. logger.warn( - `[setup] Could not write the setup token file (${writeError.message}) — ` - + 'falling back to the log. No admin account yet; open /admin to finish setup. ' - + `One-time setup token: ${token}` + `[setup] Could not write a private setup token file (${writeError.message}). ` + + 'No admin account yet; open /admin to finish setup. The token is printed ' + + 'on stdout at startup — it is deliberately not written to the log files.' ); } else { logger.warn( '[setup] No admin account yet — open /admin to finish setup. ' - + `The one-time setup token is in ${file} (not logged).` + + `The one-time setup token is in ${written} (not logged).` ); } return token; @@ -213,7 +309,11 @@ async function createInitialAdmin({ token, email, password, ip }) { return inserted[0]?.id || inserted[0]; }); - // DB token cleared inside the tx; remove the on-disk file too (best-effort). + // DB token cleared inside the tx; remove the on-disk files too + // (best-effort). Every copy, not just the canonical one (#1218) — the + // all-in-one image also keeps one at the volume root, and a burned token + // left lying there is a live-looking credential that no longer works: an + // operator would paste it, be rejected, and have nothing to fall back on. try { fs.unlinkSync(setupTokenFilePath()); } catch (_) { /* best-effort */ } logger.info(`[setup] Initial super_admin created (id=${id}, email=${cleanEmail})`);