fix(backup): resolve symlinks and add the all-in-one frontend dir to the destination guard

Codex round 3 found two more bypasses of the public-root guard,
both specific to the all-in-one image (Dockerfile.aio):

- /app/frontend/dist (FRONTEND_DIR) ships nodejs-owned and is
  served unauthenticated as the built SPA -- missing from the
  protected-roots list.
- /app/storage is a symlink to /data/storage (the actual
  STORAGE_PATH). A destination given as /app/storage/uploads/logos
  passed the guard's lexical path.resolve() comparison while
  resolving, on disk, to the exact same directory as the protected
  STORAGE_PATH/uploads/logos. isUnderPubliclyServableRoot now
  resolves symlinks in whatever prefix of each path already
  exists (resolveRealish) before comparing, rather than relying on
  path.resolve() alone.

Also restores three fs.mkdir spies in the test file that were
never un-spied, which silently leaked a rejected mock into any
later test doing a real fs.mkdir -- exactly what the new symlink
test needed to set up its fixture.

Found by codex review, round 3.
This commit is contained in:
Paul Nothaft
2026-09-09 22:17:06 +02:00
parent 3b0e213ac2
commit 427f3684c3
2 changed files with 71 additions and 4 deletions
@@ -215,6 +215,7 @@ describe('DatabaseBackupService', () => {
await expect(service.backup({})).rejects.toThrow(stop.message);
expect(mkdirSpy).toHaveBeenCalledWith('/data/db-backups', { recursive: true });
mkdirSpy.mockRestore();
});
it('falls back to /backup/database only when nothing is configured', async () => {
@@ -229,6 +230,7 @@ describe('DatabaseBackupService', () => {
await expect(service.backup({})).rejects.toThrow(stop.message);
expect(mkdirSpy).toHaveBeenCalledWith('/backup/database', { recursive: true });
mkdirSpy.mockRestore();
});
});
@@ -288,6 +290,40 @@ describe('DatabaseBackupService', () => {
await expect(service.backup({})).rejects.toThrow('publicly served directory');
expect(mkdirSpy).not.toHaveBeenCalled();
mkdirSpy.mockRestore();
});
it('flags FRONTEND_DIR — the all-in-one image serves its built SPA unauthenticated', () => {
const originalFrontendDir = process.env.FRONTEND_DIR;
process.env.FRONTEND_DIR = '/app/frontend/dist';
try {
expect(isUnderPubliclyServableRoot('/app/frontend/dist')).toBe(true);
expect(isUnderPubliclyServableRoot(path.join('/app/frontend/dist', 'assets'))).toBe(true);
} finally {
if (originalFrontendDir === undefined) delete process.env.FRONTEND_DIR;
else process.env.FRONTEND_DIR = originalFrontendDir;
}
});
it('resolves a symlinked alias of a public root to the same real directory (all-in-one /app/storage -> /data/storage)', async () => {
const os = require('os');
const realRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-real-'));
const linkRoot = path.join(os.tmpdir(), `picpeak-link-${process.pid}-${Date.now()}`);
await fs.mkdir(path.join(realRoot, 'uploads', 'logos'), { recursive: true });
await fs.symlink(realRoot, linkRoot, 'dir');
try {
// STORAGE_PATH (what the guard's roots are built from) is the real
// path; the attacker-supplied destination goes through the symlink
// — exactly the all-in-one image's /app/storage -> /data/storage.
process.env.STORAGE_PATH = realRoot;
const aliased = path.join(linkRoot, 'uploads', 'logos');
expect(isUnderPubliclyServableRoot(aliased)).toBe(true);
} finally {
await fs.unlink(linkRoot);
await fs.rm(realRoot, { recursive: true, force: true });
}
});
});
+35 -4
View File
@@ -4,7 +4,7 @@ const crypto = require('crypto');
const { spawnAsync, spawnToFile } = require('../utils/safeExec');
const zlib = require('zlib');
const { pipeline } = require('stream/promises');
const { createReadStream, createWriteStream } = require('fs');
const { createReadStream, createWriteStream, realpathSync } = require('fs');
const { db } = require('../database/db');
const knexConfig = require('../../knexfile');
const logger = require('../utils/logger');
@@ -51,18 +51,49 @@ function getPubliclyServableRoots() {
// on overlap but express.static falls through to this one on a miss).
// COPY --chown=nodejs:nodejs in the Dockerfile makes this nodejs-owned
// and therefore writable at runtime, not just a read-only image layer.
path.resolve(__dirname, '../../assets/fonts')
path.resolve(__dirname, '../../assets/fonts'),
// The all-in-one image's built frontend bundle (Dockerfile.aio ships it
// nodejs-owned) — server.js serves it unauthenticated as the SPA itself.
process.env.FRONTEND_DIR || path.resolve(__dirname, '../../../frontend/dist')
];
}
// Resolves symlinks in whatever prefix of candidatePath currently exists,
// then re-appends any not-yet-created remainder literally. A plain
// fs.realpathSync would throw ENOENT for the common case where the backup
// destination doesn't exist yet; a plain path.resolve() would miss the
// all-in-one image's `/app/storage -> /data/storage` symlink (Dockerfile.aio),
// which lets `/app/storage/uploads/logos` alias the real public logos
// directory under a name that never lexically matches it.
function resolveRealish(candidatePath) {
let current = path.resolve(candidatePath);
const remainder = [];
for (;;) {
try {
const real = realpathSync(current);
return remainder.length ? path.join(real, ...remainder) : real;
} catch (error) {
if (error.code !== 'ENOENT') {
return path.resolve(candidatePath);
}
const parent = path.dirname(current);
if (parent === current) {
return path.resolve(candidatePath);
}
remainder.unshift(path.basename(current));
current = parent;
}
}
}
function isUnderPubliclyServableRoot(candidatePath) {
// Lowercased comparison: on a case-insensitive-but-preserving filesystem
// (default macOS APFS, NTFS, and Docker Desktop's bind-mount passthrough
// of either) `STORAGE_PATH/UPLOADS/logos` and `.../uploads/logos` name the
// same directory on disk even though path.resolve() never folds case.
const resolved = path.resolve(candidatePath).toLowerCase();
const resolved = resolveRealish(candidatePath).toLowerCase();
return getPubliclyServableRoots().some((root) => {
const resolvedRoot = path.resolve(root).toLowerCase();
const resolvedRoot = resolveRealish(root).toLowerCase();
return resolved === resolvedRoot || resolved.startsWith(resolvedRoot + path.sep);
});
}