From 4b9dec6dab52ca826371405f72b6175c2505830e Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 19 May 2026 14:59:06 +0100 Subject: [PATCH 1/3] test(docker): admin save persists across container restart (#7819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OP reports the symptom on the official Docker image specifically. Adds two layers of coverage to docker.yml's build-test job, driven from inside a container started against the same TEST_TAG the existing test-container step uses: 1. New mocha spec adminSettings_7819.ts under tests/container/specs/api — authenticates against /admin, opens the /settings socket, saves an augmented JSON with an ep_oauth-shaped top-level block, and asserts the next load reply contains the marker. Intentionally leaves the marker on disk so the workflow can inspect it. 2. docker.yml now `docker exec test grep`s for the marker after test-container, then `docker restart`s the container, waits for the health probe, and re-greps. Both checks must pass — the first proves the socket-driven save actually touched the file inside the container layer; the second proves an in-place restart doesn't reset it. A recreate (docker rm + docker run) would wipe the file, but that's expected (image layer) and out of scope. Container is started with `-e ADMIN_PASSWORD=changeme1` so the existing settings.json.docker provisions the admin user; pad.js doesn't touch /admin so the existing API specs are unaffected. test-container timeout bumped 5s → 30s to cover socket connect + save round-trip, and the mocha discovery extension list now includes `ts` so the new spec is picked up. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/docker.yml | 36 +++++- src/package.json | 2 +- .../container/specs/api/adminSettings_7819.ts | 114 ++++++++++++++++++ 3 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 src/tests/container/specs/api/adminSettings_7819.ts diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index d709d3a508a..0d1c51a7cd8 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -66,7 +66,13 @@ jobs: name: Test working-directory: etherpad run: | - docker run --rm -d -p 9001:9001 --name test ${{ env.TEST_TAG }} + # ADMIN_PASSWORD provisions settings.json.docker's admin user so + # the adminSettings_7819 container spec can authenticate against + # /admin and the /settings socket. pad.js doesn't touch /admin + # so the existing API specs are unaffected. + docker run --rm -d -p 9001:9001 \ + -e ADMIN_PASSWORD=changeme1 \ + --name test ${{ env.TEST_TAG }} ./bin/installDeps.sh docker logs -f test & while true; do @@ -79,6 +85,34 @@ jobs: esac done (cd src && pnpm run test-container) + # Regression for #7819. adminSettings_7819.ts saves a marker via + # the admin /settings socket and intentionally leaves it on + # disk. We assert here that the save actually hit the file + # (mocha only sees the next `load` reply — this catches a + # scenario where the load is served from cache and the file + # never actually changed). + docker exec test grep -q persist-marker-7819 /opt/etherpad-lite/settings.json || { + echo "[#7819] socket-driven save did NOT reach /opt/etherpad-lite/settings.json on disk" + docker exec test cat /opt/etherpad-lite/settings.json | head -50 + exit 1 + } + # Now prove the on-disk file survives an in-place container + # restart. This is the scenario a docker-compose user with + # `restart: always` hits on every host reboot. + docker restart test >/dev/null + for i in $(seq 1 60); do + status=$(docker container inspect -f '{{.State.Health.Status}}' test 2>/dev/null) || { docker logs test; exit 1; } + case ${status} in + healthy) break;; + starting) sleep 2;; + *) docker logs test; exit 1;; + esac + done + docker exec test grep -q persist-marker-7819 /opt/etherpad-lite/settings.json || { + echo "[#7819 REGRESSION] settings.json was reset on docker restart — ep_oauth block vanished" + docker logs test + exit 1 + } docker rm -f test git clean -dxf . - diff --git a/src/package.json b/src/package.json index 126ac0e54b8..fa2cf5ad44d 100644 --- a/src/package.json +++ b/src/package.json @@ -151,7 +151,7 @@ "lint": "eslint .", "test": "cross-env NODE_ENV=production mocha --import=tsx --require ./tests/backend/diagnostics.ts --timeout 120000 --extension ts --recursive tests/backend/specs ../node_modules/ep_*/static/tests/backend/specs", "test-utils": "cross-env NODE_ENV=production mocha --import=tsx --timeout 5000 --recursive tests/backend/specs/*utils.ts", - "test-container": "mocha --import=tsx --timeout 5000 tests/container/specs/api", + "test-container": "mocha --import=tsx --timeout 30000 --extension ts,js tests/container/specs/api", "dev": "cross-env NODE_ENV=development node --require tsx/cjs node/server.ts", "prod": "cross-env NODE_ENV=production node --require tsx/cjs node/server.ts", "ts-check": "tsc --noEmit", diff --git a/src/tests/container/specs/api/adminSettings_7819.ts b/src/tests/container/specs/api/adminSettings_7819.ts new file mode 100644 index 00000000000..020179de9ad --- /dev/null +++ b/src/tests/container/specs/api/adminSettings_7819.ts @@ -0,0 +1,114 @@ +'use strict'; + +// Regression coverage for https://github.com/ether/etherpad/issues/7819. +// Drives the admin /settings socket against the running Docker container +// (test-container target) to prove the save flow actually writes a new +// top-level plugin block and the next `load` reads it back. +// +// Requires the container to be started with `-e ADMIN_PASSWORD=changeme1` +// so settings.json.docker provisions the admin user used here. Run via +// `pnpm run test-container` from the docker.yml workflow. + +import {strict as assert} from 'assert'; +import setCookieParser from 'set-cookie-parser'; + +const supertest = require('supertest'); +const io = require('socket.io-client'); + +const BASE_URL = 'http://localhost:9001'; +const ADMIN_USER = 'admin'; +const ADMIN_PASSWORD = 'changeme1'; +const MARKER = 'persist-marker-7819'; + +const adminCookieHeader = async (): Promise => { + const res: any = await supertest(BASE_URL) + .get('/admin/') + .auth(ADMIN_USER, ADMIN_PASSWORD); + const cookies = setCookieParser.parse(res, {map: true}) as Record; + return Object.entries(cookies) + .map(([name, cookie]) => `${name}=${encodeURIComponent(cookie.value)}`) + .join('; '); +}; + +const settingsSocket = async (cookieHdr: string) => { + const socket = io(`${BASE_URL}/settings`, { + forceNew: true, + query: {cookie: cookieHdr}, + transports: ['websocket'], + }); + await new Promise((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 load = (socket: any): Promise<{results: string; resolved?: any; flags?: any}> => + new Promise((res) => { + socket.once('settings', res); + socket.emit('load', null); + }); + +const save = (socket: any, payload: string): Promise<{status: string; detail?: any}> => + new Promise((res, rej) => { + const t = setTimeout( + () => rej(new Error('saveSettings: no saveprogress within 8s')), 8000); + socket.once('saveprogress', (status: string, detail: any) => { + clearTimeout(t); + res({status, detail}); + }); + socket.emit('saveSettings', payload); + }); + +describe('admin /settings socket (Docker container) — #7819', function () { + this.timeout(20000); + let socket: any; + let originalRaw: string; + + before(async function () { + const cookieHdr = await adminCookieHeader(); + socket = await settingsSocket(cookieHdr); + const reply = await load(socket); + assert.equal(typeof reply.results, 'string', + 'settings.results must be a string — container started without ADMIN_PASSWORD?'); + originalRaw = reply.results; + }); + + after(function () { + if (socket) socket.disconnect(); + // INTENTIONAL: do NOT restore baseline. docker.yml greps for MARKER + // via `docker exec` after this suite, then runs `docker restart`, + // then greps again — that whole chain proves the on-disk file + // survives container restart, which is the actual #7819 ask. The + // container is `docker rm -f`'d at the end of the workflow step, so + // leftover state doesn't poison anything. + }); + + it('save → load round-trip preserves a new top-level plugin block', async function () { + // Inject `"ep_oauth": {...},` right after the opening brace. Pure + // textual splice — keeps every existing key/comment intact, which + // is exactly what a user adding a plugin section would do. + const augmented = originalRaw.replace( + /^(\s*\{)/, + `$1"ep_oauth":{"clientID":"${MARKER}","clientSecret":"x",` + + '"callbackURL":"http://x/cb"},', + ); + assert.notEqual(augmented, originalRaw, 'splice should have changed the string'); + assert.ok(augmented.includes(MARKER), 'sanity: marker is in payload'); + + const ack = await save(socket, augmented); + assert.equal(ack.status, 'saved', + `saveSettings did not ack 'saved' — got ${JSON.stringify(ack)}`); + + // Re-load over the same socket. The server re-reads + // settings.settingsFilename on every `load`, so this reflects the + // actual file on disk — not a client-side echo. + const reply = await load(socket); + assert.ok(reply.results.includes('"ep_oauth"'), + 'ep_oauth block missing from next load — file on disk does not match payload'); + assert.ok(reply.results.includes(MARKER), + `marker '${MARKER}' missing from next load`); + }); +}); From 2f55bb9f918527b4cd181ead33958689eb91182a Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 19 May 2026 15:43:43 +0100 Subject: [PATCH 2/3] test(docker): authenticate via /admin-auth/ POST, surface auth/load failures fast (#7819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed on #7821 with a generic 20s mocha timeout because the spec hit GET /admin/ to grab a session cookie. webaccess.ts only treats paths starting with /admin-auth as requireAdmin — and the container runs with REQUIRE_AUTHENTICATION=false (default), so GET /admin/ never issued a Basic challenge and Set-Cookie was empty. The socket then connected unauthenticated, adminsettings.ts's connection handler returned early without binding any listeners, and the load() promise hung until mocha killed the test with no useful diagnostic. Switch to POST /admin-auth/ (always-requireAdmin, regardless of settings.requireAuthentication). Assert a 2xx with at least one Set-Cookie before proceeding. Add an 8s timeout + meaningful error message to load() so the "session was not admin" failure mode reports immediately instead of burning the suite budget. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../container/specs/api/adminSettings_7819.ts | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/tests/container/specs/api/adminSettings_7819.ts b/src/tests/container/specs/api/adminSettings_7819.ts index 020179de9ad..bf2256e5b2a 100644 --- a/src/tests/container/specs/api/adminSettings_7819.ts +++ b/src/tests/container/specs/api/adminSettings_7819.ts @@ -20,11 +20,25 @@ const ADMIN_USER = 'admin'; const ADMIN_PASSWORD = 'changeme1'; const MARKER = 'persist-marker-7819'; +// /admin-auth/ is the path webaccess.ts always treats as requireAdmin, +// regardless of settings.requireAuthentication. The container runs with +// REQUIRE_AUTHENTICATION=false (default), so GET /admin/ would NOT issue +// a Basic challenge and we'd never get a session cookie. POSTing to +// /admin-auth/ does. const adminCookieHeader = async (): Promise => { const res: any = await supertest(BASE_URL) - .get('/admin/') + .post('/admin-auth/') .auth(ADMIN_USER, ADMIN_PASSWORD); + if (res.status !== 200) { + throw new Error( + `/admin-auth/ POST returned ${res.status} (expected 200) — ` + + 'is the container started with ADMIN_PASSWORD=changeme1? ' + + `Body: ${String(res.text).slice(0, 200)}`); + } const cookies = setCookieParser.parse(res, {map: true}) as Record; + if (Object.keys(cookies).length === 0) { + throw new Error('/admin-auth/ returned 200 but set no cookies — session middleware not wired?'); + } return Object.entries(cookies) .map(([name, cookie]) => `${name}=${encodeURIComponent(cookie.value)}`) .join('; '); @@ -46,8 +60,16 @@ const settingsSocket = async (cookieHdr: string) => { }; const load = (socket: any): Promise<{results: string; resolved?: any; flags?: any}> => - new Promise((res) => { - socket.once('settings', res); + new Promise((res, rej) => { + // No reply == handler never registered, which means our session + // wasn't admin. Surface that fast rather than burning the mocha + // timeout — the adminsettings.ts connection handler silently + // returns without binding any listeners when is_admin is false. + const t = setTimeout( + () => rej(new Error( + 'load: no `settings` reply within 8s — likely not authenticated as admin')), + 8000); + socket.once('settings', (s: any) => { clearTimeout(t); res(s); }); socket.emit('load', null); }); From 7fa7e50dbc83a83b4b7dec2c2e9bcdaf10022579 Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 19 May 2026 15:52:34 +0100 Subject: [PATCH 3/3] test(docker): replace splice with hand-built payload (#7819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last CI failed because the splice-after-last-} approach landed a comma between an existing trailing-comma-before-comment and the close brace of settings.json.docker, producing `, /* … */, "ep_oauth"` — invalid JSON. settings.json.docker uses jsonc `/* */` and `//` comments and a trailing-comma-before-comment-before-close shape that's annoying to patch from the test side, and the existing isJSONClean has zero backend coverage so the splice is going through Etherpad's lenient write path anyway. Switch to a hand-built minimal-but-viable settings document containing the ep_oauth block. Three properties hold: - We're testing the WRITE path, not the synthesis path. Whatever bytes we send, the next `load` must return verbatim. - The post-save document must survive `docker restart` (the next step in docker.yml) — minimal-but-viable means port/users/dbType are present so Etherpad boots back up and HEALTHCHECK passes. - The next `load` reply must equal the bytes we saved (`reply.results === augmented`) — stronger than `.includes()`. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../container/specs/api/adminSettings_7819.ts | 49 +++++++++++++------ 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/src/tests/container/specs/api/adminSettings_7819.ts b/src/tests/container/specs/api/adminSettings_7819.ts index bf2256e5b2a..5c58e7551e0 100644 --- a/src/tests/container/specs/api/adminSettings_7819.ts +++ b/src/tests/container/specs/api/adminSettings_7819.ts @@ -87,15 +87,15 @@ const save = (socket: any, payload: string): Promise<{status: string; detail?: a describe('admin /settings socket (Docker container) — #7819', function () { this.timeout(20000); let socket: any; - let originalRaw: string; before(async function () { const cookieHdr = await adminCookieHeader(); socket = await settingsSocket(cookieHdr); + // Sanity: load works as admin. We don't keep the result — the file + // we're about to save replaces settings.json entirely. const reply = await load(socket); assert.equal(typeof reply.results, 'string', 'settings.results must be a string — container started without ADMIN_PASSWORD?'); - originalRaw = reply.results; }); after(function () { @@ -108,26 +108,47 @@ describe('admin /settings socket (Docker container) — #7819', function () { // leftover state doesn't poison anything. }); - it('save → load round-trip preserves a new top-level plugin block', async function () { - // Inject `"ep_oauth": {...},` right after the opening brace. Pure - // textual splice — keeps every existing key/comment intact, which - // is exactly what a user adding a plugin section would do. - const augmented = originalRaw.replace( - /^(\s*\{)/, - `$1"ep_oauth":{"clientID":"${MARKER}","clientSecret":"x",` + - '"callbackURL":"http://x/cb"},', - ); - assert.notEqual(augmented, originalRaw, 'splice should have changed the string'); - assert.ok(augmented.includes(MARKER), 'sanity: marker is in payload'); + it('save → load round-trip preserves a top-level plugin block', async function () { + // Hand-built minimal-but-viable settings document. Three reasons we + // don't splice into the original: + // 1. settings.json.docker uses jsonc `/* */` and `//` comments and + // keeps a trailing-comma-before-comment-before-close pattern + // that's annoying to patch correctly from the test side. + // 2. The backend `saveSettings` handler writes bytes verbatim with + // zero validation — so what we save IS what should come back. + // Whether the payload is "realistic" is orthogonal to whether + // the file persists. + // 3. After this save the container will be `docker restart`ed by + // the workflow. Minimal-but-viable means Etherpad starts back + // up: `port` is required by the HTTP server, `users.admin` + // keeps admin auth working post-restart, dbType=dirty keeps DB + // init happy. + const augmented = JSON.stringify({ + title: 'Etherpad', + ip: '0.0.0.0', + port: 9001, + dbType: 'dirty', + dbSettings: {filename: 'var/dirty.db'}, + showSettingsInAdminPage: true, + enableAdminUITests: true, + users: {admin: {password: ADMIN_PASSWORD, is_admin: true}}, + ep_oauth: { + clientID: MARKER, + clientSecret: 'x', + callbackURL: 'http://x/cb', + }, + }, null, 2); const ack = await save(socket, augmented); assert.equal(ack.status, 'saved', `saveSettings did not ack 'saved' — got ${JSON.stringify(ack)}`); - // Re-load over the same socket. The server re-reads + // Re-load over the same socket. adminsettings.ts re-reads // settings.settingsFilename on every `load`, so this reflects the // actual file on disk — not a client-side echo. const reply = await load(socket); + assert.equal(reply.results, augmented, + 'load.results must equal the bytes we just saved'); assert.ok(reply.results.includes('"ep_oauth"'), 'ep_oauth block missing from next load — file on disk does not match payload'); assert.ok(reply.results.includes(MARKER),