Security: refuse a non-object settings block on save and on read

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;

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

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' }],

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);
+});