diff --git a/api/src/config.js b/api/src/config.js index 19c08d7b..5dd65ca2 100644 --- a/api/src/config.js +++ b/api/src/config.js @@ -146,11 +146,16 @@ function loadDemoConfig() { return _demoCfg; } +const isPlainObject = v => !!v && typeof v === 'object' && !Array.isArray(v); + +/* A wrong-typed settings must count as damage. Replacing it with {} turns + sign-in off and the next save erases the password. */ function _normalizeShape(parsed) { - if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return null; + if (!isPlainObject(parsed)) return null; if (parsed.items !== undefined && !Array.isArray(parsed.items)) return null; + if (parsed.settings !== undefined && !isPlainObject(parsed.settings)) return null; if (!Array.isArray(parsed.items)) parsed.items = []; - if (!parsed.settings || typeof parsed.settings !== 'object' || Array.isArray(parsed.settings)) parsed.settings = {}; + if (parsed.settings === undefined) parsed.settings = {}; return parsed; } @@ -238,6 +243,17 @@ function loadConfig() { } _clearDamage(); + /* Every reader dereferences items without a guard. */ + if (!shaped.items.every(isPlainObject)) { + const first = raw !== _lastCorruptRaw; + const backup = _backupCorrupt(raw); + shaped.items = shaped.items.filter(isPlainObject); + if (first) + log.warn('config entries that are not objects were skipped', { + path: CONFIG_PATH, + ...(backup ? { backup } : {}), + }); + } const before = shaped._schemaVersion; migrate(shaped); _cfgCache = shaped; diff --git a/api/src/routes/config.js b/api/src/routes/config.js index 8f9d7fbc..7eecdf70 100644 --- a/api/src/routes/config.js +++ b/api/src/routes/config.js @@ -60,6 +60,9 @@ on('POST', '/api/config', async (req, res) => { try { const data = JSON.parse(await readBody(req)); if (!Array.isArray(data.items)) return json(res, 400, { error: 'items must be an array', kind: KIND.INVALID }); + /* An array or a primitive here drops the stored password on write. */ + if (data.settings != null && (typeof data.settings !== 'object' || Array.isArray(data.settings))) + return json(res, 400, { error: 'settings must be an object', kind: KIND.INVALID }); const bad = data.items.find(i => !i || typeof i.id !== 'string' || !i.id || typeof i.type !== 'string' || !i.type); if (bad) return json(res, 400, { error: 'every item needs a non-empty id and type', kind: KIND.INVALID }); /* Lookups take the first match, so a second item sharing an id is diff --git a/api/test/auth-hardening.test.js b/api/test/auth-hardening.test.js index 020ff711..4e8c8c5d 100644 --- a/api/test/auth-hardening.test.js +++ b/api/test/auth-hardening.test.js @@ -201,6 +201,23 @@ test('a config write keeps the stored password while protection is on', async () assert.equal(await verifyPassword('correct-horse', auth.passwordHash), true); }); +for (const [name, settings] of [ + ['an array', []], + ['a string', 'x'], +]) { + test(`a config write with settings as ${name} is refused and keeps the password`, async () => { + const cfg = loadConfig(); + cfg.settings.auth = { enabled: true, passwordHash: await hashPassword('correct-horse'), secret: 'real-secret' }; + saveConfig(cfg); + + const r = await post('/api/config', { items: [], settings }, { cookie: sessionCookie() }); + assert.equal(r.status, 400); + const auth = loadConfig().settings.auth; + assert.equal(auth.enabled, true); + assert.equal(await verifyPassword('correct-horse', auth.passwordHash), true); + }); +} + test('an ordinary config write is unaffected', async () => { const r = await post('/api/config', { items: [{ id: 'a1', type: 'app', name: 'App' }], diff --git a/api/test/config-corrupt.test.js b/api/test/config-corrupt.test.js index a18e519c..3d8d1de6 100644 --- a/api/test/config-corrupt.test.js +++ b/api/test/config-corrupt.test.js @@ -117,3 +117,37 @@ test('a second, different corruption is preserved alongside the first', async () const contents = b.map(f => fs.readFileSync(path.join(DIR, f), 'utf8')).sort(); assert.deepEqual(contents, ['{ broken one', '{ broken two']); }); + +for (const [name, settings] of [ + ['an array', [{ auth: { enabled: true, passwordHash: 'h', secret: 's' } }]], + ['a string', 'x'], + ['null', null], +]) { + test(`a settings block that is ${name} is treated as corrupt, not reset`, () => { + reset(); + const raw = JSON.stringify({ items: [], settings }); + fs.writeFileSync(CFG, raw); + const cfg = fresh(); + cfg.loadConfig(); + assert.equal(cfg.configDamage()?.reason, 'corrupt'); + assert.equal(backups().length, 1); + assert.throws(() => cfg.saveConfig({ items: [], settings: {} })); + assert.equal(fs.readFileSync(CFG, 'utf8'), raw); + }); +} + +test('item entries that are not objects are skipped and the file is backed up', () => { + reset(); + const raw = JSON.stringify({ items: [{ id: 'a', type: 'app' }, null, 'x', []], settings: {} }); + fs.writeFileSync(CFG, raw); + const cfg = fresh(); + const loaded = cfg.loadConfig(); + assert.deepEqual( + loaded.items.map(i => i.id), + ['a'], + ); + assert.equal(cfg.configDamage(), null); + const b = backups(); + assert.equal(b.length, 1); + assert.equal(fs.readFileSync(path.join(DIR, b[0]), 'utf8'), raw); +});