security(oidc): stop shipping a hardcoded cookie key and reflecting all CORS origins

settings.json.template

@@ -966,6 +966,14 @@
 
   "sso": {
     "issuer": "${SSO_ISSUER:http://localhost:9001}",
+      /*
+       * Optional signing keys for the embedded OIDC provider's cookies. When
+       * omitted, Etherpad derives a secret key from the persisted session
+       * secret (SESSIONKEY.txt), which is stable across restarts and shared
+       * across horizontally-scaled pods. Set an explicit ordered array to
+       * rotate: the first key signs, the rest are still accepted for verify.
+       * "cookieKeys": ["${OIDC_COOKIE_KEY:}"],
+       */
       "clients": [
         {
           "client_id": "${ADMIN_CLIENT:admin_client}",

src/node/security/OAuth2Provider.ts

@@ -10,6 +10,12 @@ import {format} from 'url'
 import {ParsedUrlQuery} from "node:querystring";
 import {MapArrayType} from "../types/MapType";
 import crypto from "node:crypto";
+import {resolveOidcCookieKeys, isOriginAllowedForOidcClient} from "./OidcProviderSecurity";
+import SecretRotator from "./SecretRotator";
+
+// Held at module scope so the rotator (and its refresh timer) is not garbage
+// collected for the lifetime of the process, mirroring express.ts.
+let oidcCookieSecretRotator: SecretRotator | null = null;
 
 // Small fixed delay applied to every failed interactive login, mirroring
 // webaccess.authnFailureDelayMs, so failures take a consistent amount of time.
@@ -68,9 +74,13 @@ const configuration: Configuration = {
         profile: ['name'],
         admin: ['admin']
     },
-    cookies: {
-      keys: ['oidc'],
-    },
+    // NOTE: cookies.keys is deliberately NOT set here. A committed literal key
+    // (historically ['oidc']) lets anyone with the public source forge valid
+    // OIDC provider `.sig` cookies. The real key material is resolved at
+    // provider-construction time in expressCreateServer() via
+    // resolveOidcCookieKeys(), which prefers an operator-supplied
+    // settings.sso.cookieKeys and otherwise derives a secret, stable key from
+    // the persisted session secret. See OidcProviderSecurity.ts.
     features:{
       devInteractions: {enabled: false},
     },
@@ -92,8 +102,39 @@ export const expressCreateServer = async (hookName: string, args: ArgsExpressTyp
     publicKeyExported = publicKey
     privateKeyExported = privateKey
 
+    // Resolve the OIDC provider's cookie-signing keys. Never a committed literal.
+    // When the operator hasn't pinned settings.sso.cookieKeys and cookie key
+    // rotation is enabled (the default), reuse the same DB-backed SecretRotator
+    // mechanism as the Express session cookies so the key is stable across
+    // restarts and shared across horizontally-scaled pods. `settings.sessionKey`
+    // is commonly null under default rotation settings, so relying on it alone
+    // would hand the provider an unstable per-process random key.
+    const operatorCookieKeys = (settings.sso as {cookieKeys?: unknown}).cookieKeys;
+    const hasOperatorKeys = Array.isArray(operatorCookieKeys) &&
+        operatorCookieKeys.some((k) => typeof k === 'string' && k.length > 0);
+    const {keyRotationInterval, sessionLifetime} = settings.cookie;
+    let rotatedSecrets: string[] | null = null;
+    if (!hasOperatorKeys && keyRotationInterval && sessionLifetime) {
+        if (oidcCookieSecretRotator == null) {
+            oidcCookieSecretRotator = new SecretRotator(
+                'oidcCookieSecrets', keyRotationInterval, sessionLifetime, settings.sessionKey);
+            await oidcCookieSecretRotator.start();
+        }
+        rotatedSecrets = oidcCookieSecretRotator.secrets;
+    }
+
     const oidc = new Provider(settings.sso.issuer, {
-        ...configuration, jwks: {
+        ...configuration,
+        cookies: {
+            // Secret, deployment-stable signing keys — never a committed literal.
+            // See resolveOidcCookieKeys().
+            keys: resolveOidcCookieKeys({
+                cookieKeys: operatorCookieKeys,
+                rotatedSecrets,
+                sessionKey: settings.sessionKey,
+            }),
+        },
+        jwks: {
             keys: [
                 privateKeyJWK
             ],
@@ -127,9 +168,12 @@ export const expressCreateServer = async (hookName: string, args: ArgsExpressTyp
           },
             jwtResponseModes: {enabled: true},
         },
-        clientBasedCORS: (ctx, origin, client) => {
-          return true
-        },
+        clientBasedCORS: (ctx, origin, client) =>
+          // Only allow cross-origin reads from an origin registered as one of
+          // the client's redirect URIs. Returning `true` unconditionally
+          // reflected any Origin into Access-Control-Allow-Origin. See
+          // isOriginAllowedForOidcClient().
+          isOriginAllowedForOidcClient(origin, client as {redirectUris?: unknown}),
         extraParams: [],
         extraTokenClaims: async (ctx, token) => {
             if(token.kind === 'AccessToken') {

src/node/security/OidcProviderSecurity.ts

@@ -0,0 +1,96 @@
+import crypto from 'node:crypto';
+
+/**
+ * Security helpers for the embedded OIDC provider (`OAuth2Provider.ts`).
+ *
+ * Kept in a dependency-light module (only `node:crypto`) so the pure decisions
+ * can be unit-tested without constructing an `oidc-provider` instance.
+ */
+
+// Domain-separation label so the derived cookie key is cryptographically
+// unrelated to any other use of the session secret. The trailing NUL keeps the
+// label from ambiguously running into the appended secret.
+export const COOKIE_KEY_DERIVATION_LABEL = 'etherpad-oidc-provider-cookie-signing-key\0';
+
+/**
+ * Resolve the cookie-signing keys for the embedded OIDC provider.
+ *
+ * oidc-provider signs its short-lived interaction/session/grant cookies
+ * (`_interaction`, `_interaction_resume`, `_grant`, `_session`) with an HMAC
+ * keyed by these values. Shipping a hardcoded key (historically `['oidc']`) let
+ * anyone with the public source forge valid `.sig` cookies, defeating the
+ * provider's cookie-integrity guarantee. Reported by `meifukun`.
+ *
+ * Resolution order:
+ *   1. An explicit operator-supplied `settings.sso.cookieKeys` array — use this
+ *      for controlled rotation: `[newKey, ...oldKeys]`.
+ *   2. The live secrets array of a DB-backed `SecretRotator` (the same mechanism
+ *      the Express session-cookie stack uses). This is the default path: it is
+ *      stable across restarts, shared across horizontally-scaled pods via the
+ *      database, and rotates automatically. The array is returned BY REFERENCE
+ *      so a later in-place rotation propagates to oidc-provider/keygrip without
+ *      reconstructing the provider.
+ *   3. A value derived from the persisted Etherpad session secret
+ *      (`SESSIONKEY.txt`) via a domain-separated SHA-256 — used when rotation is
+ *      disabled but a static session key exists. Stable across restarts/pods,
+ *      never committed to source.
+ *   4. As a last resort (no key material at all), an ephemeral per-process
+ *      random key. The integrity boundary holds, but interactions won't survive
+ *      a restart or span multiple pods.
+ */
+export const resolveOidcCookieKeys = (
+  opts: {cookieKeys?: unknown, rotatedSecrets?: string[] | null, sessionKey?: string | null},
+): string[] => {
+  const {cookieKeys, rotatedSecrets, sessionKey} = opts;
+
+  if (Array.isArray(cookieKeys)) {
+    const usable = cookieKeys.filter((k): k is string => typeof k === 'string' && k.length > 0);
+    if (usable.length > 0) return usable;
+  }
+
+  // Return the rotator's array by reference (do not copy/filter) so in-place
+  // rotation is observed live by keygrip.
+  if (Array.isArray(rotatedSecrets) &&
+      rotatedSecrets.some((k) => typeof k === 'string' && k.length > 0)) {
+    return rotatedSecrets;
+  }
+
+  if (typeof sessionKey === 'string' && sessionKey.length > 0) {
+    const derived = crypto.createHash('sha256')
+        .update(COOKIE_KEY_DERIVATION_LABEL)
+        .update(sessionKey)
+        .digest('hex');
+    return [derived];
+  }
+
+  return [crypto.randomBytes(32).toString('hex')];
+};
+
+/**
+ * Origin allow-list decision for the embedded OIDC provider's CORS-enabled
+ * endpoints (`/oidc/token`, `/oidc/me`, ...). oidc-provider invokes
+ * `clientBasedCORS(ctx, origin, client)` for every cross-origin request;
+ * returning `true` unconditionally (the historical behavior) reflected ANY
+ * `Origin` into `Access-Control-Allow-Origin`, letting unregistered origins read
+ * token/userinfo responses that use non-cookie credentials (Authorization
+ * headers, POST-body client credentials). Reported by `meifukun`.
+ *
+ * An origin is allowed only when it exactly matches the origin (scheme + host +
+ * port) of one of the client's registered redirect URIs.
+ */
+export const isOriginAllowedForOidcClient = (
+  origin: string | undefined | null,
+  client: {redirectUris?: unknown} | undefined | null,
+): boolean => {
+  if (!origin || !client) return false;
+  const uris = (client as {redirectUris?: unknown}).redirectUris;
+  if (!Array.isArray(uris)) return false;
+  return uris.some((uri: unknown) => {
+    if (typeof uri !== 'string') return false;
+    try {
+      return new URL(uri).origin === origin;
+    } catch {
+      return false;
+    }
+  });
+};

src/node/utils/Settings.ts

@@ -304,6 +304,10 @@ export type SettingsType = {
   sso: {
     issuer: string,
     clients?: {client_id: string}[]
+    // Optional operator-supplied signing keys for the embedded OIDC provider's
+    // cookies. When unset, a secret key is derived from the session secret.
+    // Provide an ordered array `[newKey, ...oldKeys]` to rotate.
+    cookieKeys?: string[]
   },
   showSettingsInAdminPage: boolean,
   cleanup: {

src/tests/backend/specs/OidcProviderSecurity.ts

@@ -0,0 +1,128 @@
+'use strict';
+
+/**
+ * Unit coverage for the embedded OIDC provider's cookie-signing key derivation
+ * and CORS origin allow-list. Both were reported by `meifukun`:
+ *   - the provider historically signed its cookies with the hardcoded key
+ *     `['oidc']`, so anyone with the public source could forge valid `.sig`
+ *     cookies;
+ *   - `clientBasedCORS` returned `true` for every origin, reflecting arbitrary
+ *     `Origin` values into `Access-Control-Allow-Origin`.
+ */
+
+const assert = require('assert').strict;
+import {
+  resolveOidcCookieKeys,
+  isOriginAllowedForOidcClient,
+} from '../../../node/security/OidcProviderSecurity';
+
+describe(__filename, function () {
+  describe('resolveOidcCookieKeys', function () {
+    it('never returns the historical hardcoded key', function () {
+      const keys = resolveOidcCookieKeys({sessionKey: 'a-persisted-session-secret'});
+      assert.ok(!keys.includes('oidc'));
+    });
+
+    it('uses operator-supplied cookieKeys when provided', function () {
+      const keys = resolveOidcCookieKeys({cookieKeys: ['k1', 'k2'], sessionKey: 'x'});
+      assert.deepEqual(keys, ['k1', 'k2']);
+    });
+
+    it('ignores empty/invalid entries in cookieKeys and falls through', function () {
+      const keys = resolveOidcCookieKeys({cookieKeys: ['', null as any], sessionKey: 'secret'});
+      assert.equal(keys.length, 1);
+      assert.notEqual(keys[0], 'oidc');
+      assert.ok(keys[0].length >= 32);
+    });
+
+    it('prefers rotated DB-backed secrets over the session-key derivation', function () {
+      const rotated = ['rot-new', 'rot-old'];
+      const keys = resolveOidcCookieKeys({rotatedSecrets: rotated, sessionKey: 'secret'});
+      assert.deepEqual(keys, ['rot-new', 'rot-old']);
+    });
+
+    it('returns the live rotatedSecrets array by reference (so rotation propagates)', function () {
+      // oidc-provider/keygrip holds the array by reference and reads it live on
+      // each sign/verify, so returning the same object means a rotation that
+      // mutates the array in place is picked up without reconstructing the provider.
+      const rotated = ['rot-new'];
+      const keys = resolveOidcCookieKeys({rotatedSecrets: rotated, sessionKey: 'secret'});
+      assert.strictEqual(keys, rotated);
+    });
+
+    it('ignores an empty rotatedSecrets array and falls through', function () {
+      const keys = resolveOidcCookieKeys({rotatedSecrets: [], sessionKey: 'secret'});
+      assert.equal(keys.length, 1);
+      assert.notEqual(keys[0], 'oidc');
+      // fell through to the session-key derivation (stable, deterministic)
+      assert.deepEqual(keys, resolveOidcCookieKeys({sessionKey: 'secret'}));
+    });
+
+    it('lets operator cookieKeys win over rotated secrets', function () {
+      const keys = resolveOidcCookieKeys({
+        cookieKeys: ['operator'], rotatedSecrets: ['rot'], sessionKey: 'secret',
+      });
+      assert.deepEqual(keys, ['operator']);
+    });
+
+    it('derives a stable key from the session secret (survives restart/multi-pod)', function () {
+      const a = resolveOidcCookieKeys({sessionKey: 'secret'});
+      const b = resolveOidcCookieKeys({sessionKey: 'secret'});
+      assert.deepEqual(a, b);
+    });
+
+    it('derives different keys for different session secrets', function () {
+      const a = resolveOidcCookieKeys({sessionKey: 'secret-a'});
+      const b = resolveOidcCookieKeys({sessionKey: 'secret-b'});
+      assert.notDeepEqual(a, b);
+    });
+
+    it('does not reuse the raw session secret as the cookie key', function () {
+      const keys = resolveOidcCookieKeys({sessionKey: 'secret'});
+      assert.ok(!keys.includes('secret'));
+    });
+
+    it('falls back to a fresh random key when no session secret exists', function () {
+      const a = resolveOidcCookieKeys({sessionKey: null});
+      const b = resolveOidcCookieKeys({sessionKey: null});
+      assert.equal(a.length, 1);
+      assert.notEqual(a[0], 'oidc');
+      assert.ok(a[0].length >= 32);
+      assert.notDeepEqual(a, b); // random => different each call
+    });
+  });
+
+  describe('isOriginAllowedForOidcClient', function () {
+    const client = {
+      redirectUris: ['https://app.example.com/admin/', 'https://app.example.com/'],
+    };
+
+    it('allows an origin matching a registered redirect URI', function () {
+      assert.equal(isOriginAllowedForOidcClient('https://app.example.com', client), true);
+    });
+
+    it('rejects an unregistered attacker origin', function () {
+      assert.equal(isOriginAllowedForOidcClient('https://evil.attacker.com', client), false);
+    });
+
+    it('rejects a look-alike suffix origin (no substring matching)', function () {
+      assert.equal(isOriginAllowedForOidcClient('https://app.example.com.evil.com', client), false);
+    });
+
+    it('rejects a scheme mismatch (http vs https)', function () {
+      assert.equal(isOriginAllowedForOidcClient('http://app.example.com', client), false);
+    });
+
+    it('rejects when origin is missing', function () {
+      assert.equal(isOriginAllowedForOidcClient(undefined, client), false);
+    });
+
+    it('rejects when client is missing', function () {
+      assert.equal(isOriginAllowedForOidcClient('https://app.example.com', null), false);
+    });
+
+    it('rejects when client has no redirect URIs', function () {
+      assert.equal(isOriginAllowedForOidcClient('https://app.example.com', {}), false);
+    });
+  });
+});

src/tests/backend/specs/oidcCookieRotator.ts

@@ -0,0 +1,54 @@
+'use strict';
+
+/**
+ * Integration coverage for the OIDC provider cookie-key source under the
+ * DEFAULT deployment config (cookie key rotation enabled, no SESSIONKEY.txt so
+ * `settings.sessionKey` is null). Without the DB-backed SecretRotator path the
+ * provider would fall back to a per-process random key, breaking OIDC cookies on
+ * restart and across horizontally-scaled pods. This test proves the default
+ * path yields a stable, non-random, DB-backed key.
+ */
+
+const assert = require('assert').strict;
+const common = require('../common');
+const SecretRotator = require('../../../node/security/SecretRotator').SecretRotator;
+import {resolveOidcCookieKeys} from '../../../node/security/OidcProviderSecurity';
+import settings from '../../../node/utils/Settings';
+
+describe(__filename, function () {
+  this.timeout(30000);
+
+  before(async function () {
+    // Boots the server, which initialises the database the rotator writes to.
+    await common.init();
+  });
+
+  it('default config yields DB-backed rotator secrets, not a random key', async function () {
+    const {keyRotationInterval, sessionLifetime} = settings.cookie;
+    assert.ok(keyRotationInterval && sessionLifetime,
+        'precondition: cookie key rotation is enabled by default');
+
+    const rotator = new SecretRotator(
+        'oidcCookieSecretsTest', keyRotationInterval, sessionLifetime, settings.sessionKey);
+    await rotator.start();
+    try {
+      assert.ok(Array.isArray(rotator.secrets) && rotator.secrets.length > 0,
+          'rotator produced at least one secret');
+      assert.ok(rotator.secrets.every((s: any) => typeof s === 'string' && s.length > 0),
+          'all rotator secrets are non-empty strings');
+
+      // The provider would select these rotated secrets (by reference) over any
+      // session-key derivation or random fallback.
+      const keys = resolveOidcCookieKeys({
+        cookieKeys: (settings.sso as any).cookieKeys,
+        rotatedSecrets: rotator.secrets,
+        sessionKey: settings.sessionKey,
+      });
+      assert.strictEqual(keys, rotator.secrets, 'resolver returns the live rotator array');
+      assert.ok(!keys.includes('oidc'), 'never the historical hardcoded key');
+    } finally {
+      // stop the refresh timer so it does not keep the event loop alive
+      if (typeof rotator.stop === 'function') await rotator.stop();
+    }
+  });
+});