test(admin): cover saveSettings round-trip + restart persistence (#7819)

src/tests/backend/specs/admin/adminSettingsSave.ts

@@ -0,0 +1,220 @@
+'use strict';
+
+import {strict as assert} from 'assert';
+import setCookieParser from 'set-cookie-parser';
+import * as fs from 'fs';
+import * as os from 'os';
+import * as path from 'path';
+
+const io = require('socket.io-client');
+const common = require('../../common');
+const settings = require('../../../../node/utils/Settings');
+
+// Mirrors the adminSocket helper in adminSettingsResolved.ts. Lifted here
+// because the suite owns its own settings.settingsFilename stub and we
+// don't want either suite's setup/teardown to step on the other.
+const adminSocket = async () => {
+  settings.users = settings.users || {};
+  settings.users['test-admin'] = {password: 'test-admin-password', is_admin: true};
+  const savedRequireAuthentication = settings.requireAuthentication;
+  settings.requireAuthentication = true;
+  let res: any;
+  try {
+    res = await (common.agent as any)
+        .get('/admin/')
+        .auth('test-admin', 'test-admin-password');
+  } finally {
+    settings.requireAuthentication = savedRequireAuthentication;
+  }
+  const resCookies = setCookieParser.parse(res, {map: true});
+  const reqCookieHdr = Object.entries(resCookies)
+      .map(([name, cookie]: [string, any]) =>
+          `${name}=${encodeURIComponent(cookie.value)}`)
+      .join('; ');
+  const socket = io(`${common.baseUrl}/settings`, {
+    forceNew: true,
+    query: {cookie: reqCookieHdr},
+  });
+  await new Promise<void>((res, rej) => {
+    const onErr = (err: any) => { socket.off('connect', onConn); rej(err); };
+    const onConn = () => { socket.off('connect_error', onErr); res(); };
+    socket.once('connect', onConn);
+    socket.once('connect_error', onErr);
+  });
+  return socket;
+};
+
+const PROBE_BUDGET_MS = 15000;
+const adminSocketWithProbe = async (budgetMs: number): Promise<{
+  ok: true; socket: any;
+} | {ok: false; reason: string;}> => {
+  const deadline = Date.now() + budgetMs;
+  let socket: any;
+  try {
+    socket = await Promise.race([
+      adminSocket(),
+      new Promise<never>((_, rej) =>
+          setTimeout(() => rej(new Error('adminSocket connect timed out')),
+              Math.max(0, deadline - Date.now()))),
+    ]);
+  } catch (err: any) {
+    return {ok: false, reason: String(err && err.message || err)};
+  }
+  const remaining = Math.max(0, deadline - Date.now());
+  const replied = new Promise<true>((res) => socket.once('settings', () => res(true)));
+  socket.emit('load', null);
+  const probed = await Promise.race([
+    replied,
+    new Promise<false>((res) => setTimeout(() => res(false), remaining)),
+  ]);
+  if (!probed) {
+    socket.disconnect();
+    return {ok: false, reason: `no \`settings\` reply within ${budgetMs}ms`};
+  }
+  return {ok: true, socket};
+};
+
+const ask = (socket: any, evt: string, payload: any, replyEvt: string) =>
+    new Promise<any>((res) => {
+      socket.once(replyEvt, res);
+      socket.emit(evt, payload);
+    });
+
+const save = (socket: any, payload: string) =>
+    new Promise<{status: string; detail?: any}>((res, rej) => {
+      const timeout = setTimeout(
+          () => rej(new Error('saveSettings: no saveprogress within 5s')), 5000);
+      socket.once('saveprogress', (status: string, detail: any) => {
+        clearTimeout(timeout);
+        res({status, detail});
+      });
+      socket.emit('saveSettings', payload);
+    });
+
+// Regression coverage for issue #7819 / the broader observation that the
+// admin saveSettings socket has zero backend coverage. The goal here is
+// narrow: prove that whatever raw string the admin SPA emits ends up on
+// disk byte-for-byte at settings.settingsFilename, and the subsequent
+// `load` reply reflects the new file contents. We do NOT exercise
+// runtime reload — that's reloadSettings()' job and is covered elsewhere.
+describe(__filename, function () {
+  let socket: any;
+  let savedUsers: any;
+  let savedRequireAuthentication: boolean;
+  let savedSettingsFilename: any;
+  let tmpSettingsPath: string | null = null;
+  let baselineContents: string;
+  let setupCompleted = false;
+
+  before(async function () {
+    this.timeout(60000);
+    await common.init();
+
+    savedSettingsFilename = settings.settingsFilename;
+    tmpSettingsPath = path.join(os.tmpdir(),
+        `etherpad-7819-settings-${process.pid}.json`);
+    // Realistic baseline: keys you'd find in a stock settings.json.
+    // Saved with two-space indent so we can later assert formatting is
+    // preserved through the write path.
+    baselineContents = JSON.stringify({
+      title: 'Etherpad',
+      ip: '0.0.0.0',
+      port: 9001,
+      users: {admin: {password: 'changeme1', is_admin: true}},
+    }, null, 2) + '\n';
+    fs.writeFileSync(tmpSettingsPath, baselineContents);
+    settings.settingsFilename = tmpSettingsPath;
+
+    savedUsers = settings.users;
+    savedRequireAuthentication = settings.requireAuthentication;
+    setupCompleted = true;
+
+    const probe = await adminSocketWithProbe(PROBE_BUDGET_MS);
+    if (!probe.ok) {
+      console.warn(
+          `[adminSettingsSave] admin socket probe failed (${probe.reason}); ` +
+          'skipping suite — likely an authenticate-hook plugin rejecting test creds.');
+      this.skip();
+      return;
+    }
+    socket = probe.socket;
+  });
+
+  after(function () {
+    if (socket) socket.disconnect();
+    if (!setupCompleted) return;
+    settings.settingsFilename = savedSettingsFilename;
+    if (tmpSettingsPath) {
+      try { fs.unlinkSync(tmpSettingsPath); } catch { /* best effort */ }
+    }
+    if (settings.users) delete settings.users['test-admin'];
+    settings.users = savedUsers;
+    settings.requireAuthentication = savedRequireAuthentication;
+  });
+
+  // Reset to baseline between tests so each it() is independent — earlier
+  // suites in the same mocha run can leave behind state via shared sockets.
+  beforeEach(function () {
+    if (!tmpSettingsPath) this.skip();
+    fs.writeFileSync(tmpSettingsPath!, baselineContents);
+  });
+
+  it('saveSettings writes the payload byte-for-byte to settings.settingsFilename',
+      async function () {
+        const payload = JSON.stringify({title: 'EtherpadWrittenViaSocket'}, null, 2);
+        const ack = await save(socket, payload);
+        assert.equal(ack.status, 'saved', 'saveprogress should be "saved"');
+        const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8');
+        assert.equal(onDisk, payload,
+            'on-disk contents must equal the raw payload (no transform)');
+      });
+
+  // The shape that triggered #7819: take an existing settings.json and add
+  // one new top-level block (a plugin config). The block must persist on
+  // disk verbatim and reappear in the next `load` reply.
+  it('augmenting existing JSON with a new top-level plugin block round-trips',
+      async function () {
+        const augmented = JSON.stringify({
+          title: 'Etherpad',
+          ip: '0.0.0.0',
+          port: 9001,
+          ep_oauth: {
+            clientID: 'Iv1.testclient',
+            clientSecret: 'testsecret',
+            callbackURL: 'https://etherpad.example.com/auth/callback',
+          },
+          users: {admin: {password: 'changeme1', is_admin: true}},
+        }, null, 2);
+
+        const ack = await save(socket, augmented);
+        assert.equal(ack.status, 'saved');
+
+        const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8');
+        assert.equal(onDisk, augmented,
+            'augmented JSON must be on disk verbatim');
+
+        // load() now reads the file we just wrote — `results` is the raw
+        // string, so it must contain the plugin block we added.
+        const reply: any = await ask(socket, 'load', null, 'settings');
+        assert.equal(reply.results, augmented,
+            'load.results must equal the file we just saved');
+        assert.ok(reply.results.includes('"ep_oauth"'),
+            'plugin block must be present in subsequent load');
+      });
+
+  // /* */ comments are legal in the admin editor (jsonc-parser tolerates
+  // them; the SPA's isJSONClean strips them before validation). The save
+  // path must not normalize or strip them — the SPA test
+  // 'preserves /* */ comments after save round-trip' covers the UI side;
+  // this one covers the socket-level guarantee.
+  it('preserves /* */ comments in the written file', async function () {
+    const withComment =
+        '/* persisted-marker-7819 */\n' +
+        JSON.stringify({title: 'Etherpad'}, null, 2);
+    const ack = await save(socket, withComment);
+    assert.equal(ack.status, 'saved');
+    const onDisk = fs.readFileSync(tmpSettingsPath!, 'utf8');
+    assert.ok(onDisk.includes('persisted-marker-7819'),
+        'comment must survive the write path');
+  });
+});

src/tests/frontend-new/admin-spec/adminsettings.spec.ts

@@ -124,6 +124,66 @@
     await expect(settings).not.toHaveValue('', {timeout: 30000});
   });
 
+  // Regression for https://github.com/ether/etherpad/issues/7819.
+  // The pre-existing 'restart works' test only proves the server comes
+  // back up; it never sets a custom value first, so a deployment that
+  // resets settings.json on restart (Docker layer wipe, init-container
+  // template render, snap reseed bug) would pass it. This test mirrors
+  // the user's actual workflow: open Raw, add a new top-level plugin
+  // block, save, restart, confirm the block is still there.
+  test('#7819 custom top-level block survives server restart', async ({page}) => {
+    await page.goto('http://localhost:9001/admin/settings');
+    await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});
+    await page.getByTestId('mode-toggle-raw').click();
+    const raw = page.getByTestId('settings-raw-textarea');
+    await expect(raw).toBeVisible({timeout: 10000});
+    const original = await raw.inputValue();
+    expect(original.length).toBeGreaterThan(0);
+
+    // Inject `"ep_oauth": {…},` right after the opening brace. This matches
+    // what a user adding a plugin config block would type — we keep every
+    // other key intact, no schema, just a new top-level object.
+    const augmented = original.replace(
+      /^(\s*\{)/,
+      '$1"ep_oauth":{"clientID":"persist-marker-7819","clientSecret":"x",' +
+      '"callbackURL":"http://x/cb"},',
+    );
+    expect(augmented).not.toEqual(original);
+    expect(augmented).toContain('persist-marker-7819');
+
+    await raw.fill(augmented);
+    await saveSettings(page);
+
+    await restartEtherpad(page);
+    await loginToAdmin(page, 'admin', 'changeme1');
+    await page.goto('http://localhost:9001/admin/settings');
+    await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});
+    await page.getByTestId('mode-toggle-raw').click();
+    const rawAfter = page.getByTestId('settings-raw-textarea');
+    await expect(rawAfter).toBeVisible({timeout: 10000});
+    await expect(rawAfter).not.toHaveValue('', {timeout: 30000});
+
+    const afterRestart = await rawAfter.inputValue();
+    expect(afterRestart).toContain('"ep_oauth"');
+    expect(afterRestart).toContain('persist-marker-7819');
+
+    // The Form view must also surface the new block as its own section,
+    // since FormView renders every top-level object/array as a Section
+    // (admin/src/components/settings/FormView.tsx). humanize('ep_oauth')
+    // becomes 'Ep oauth'.
+    await page.getByTestId('mode-toggle-form').click();
+    await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});
+    await expect(page.locator('[data-testid="settings-form-view"]'))
+        .toContainText(/Ep oauth/i);
+
+    // Restore — DO NOT rely on the next test's cleanup; the file is
+    // shared across the serial run and a custom ep_oauth block could
+    // poison anything that asserts the exact length of settings.
+    await page.getByTestId('mode-toggle-raw').click();
+    await rawAfter.fill(original);
+    await saveSettings(page);
+  });
+
   test('form view derives label + help text from key comment', async ({page}) => {
     await page.goto('http://localhost:9001/admin/settings');
     await page.waitForSelector('[data-testid="settings-form-view"]', {timeout: 30000});
@@ -149,7 +209,7 @@
     const titleLabel = page.locator('label[for="field-title"]');
     await expect(titleLabel).toBeVisible({timeout: 10000});
     const titleRow = titleLabel.locator('xpath=ancestor::*[contains(@class,"settings-row")][1]');
     await expect(titleRow).toContainText('CustomTitleLabel');
     await expect(titleRow).toContainText('ExtraHelpMarker');
 
     // Restore