Merge pull request 'nextcloud: keep onlyoffice's jwt off argv, and re-check a failed connection every tick (issues 1831, 1832)' (#446) from fix/onlyoffice-followups into main

modules/nextcloud/client.ts

@@ -62,6 +62,10 @@ const ONLYOFFICE_KEYS: ReadonlyArray<readonly [occKey: string, configKey: string
 let saidOnlyOfficeNotGiven = false;
 
 export class NextcloudClient {
+  // Whether the last connection check failed (novox/hq issue 1832): while it did, every tick runs
+  // the check again, even when nothing changed, until it passes.
+  private lastCheckFailed = false;
+
   constructor(
     private readonly container: string,
     private readonly ocsUrl: string,
@@ -92,11 +96,20 @@ export class NextcloudClient {
     return new NextcloudClient(container, ocsUrl.replace(/\/$/, ""), adminUser, adminPassword);
   }
 
-  /** Run occ inside the container as the web user, returning its stdout, throwing its own message. */
-  occ(args: string[]): string {
+  /**
+   * Run occ inside the container as the web user, returning its stdout, throwing its own message.
+   * With `opts.input` given, occ reads it from stdin (`docker exec -i`) instead of taking it on the
+   * command line — the only way a secret value reaches occ without sitting in the docker CLI's argv,
+   * the container's php argv and docker's exec record (novox/hq issue 1831).
+   */
+  occ(args: string[], opts: { input?: string } = {}): string {
+    const dockerArgs = opts.input !== undefined
+      ? ["exec", "-i", "-u", "www-data", this.container, "php", "occ", ...args]
+      : ["exec", "-u", "www-data", this.container, "php", "occ", ...args];
     try {
-      return execFileSync("docker", ["exec", "-u", "www-data", this.container, "php", "occ", ...args], {
+      return execFileSync("docker", dockerArgs, {
         encoding: "utf8", timeout: 60_000,
+        ...(opts.input !== undefined ? { input: opts.input } : {}),
       }).trim();
     } catch (err: any) {
       const detail = String(err?.stderr ?? err?.stdout ?? err?.message ?? "").trim();
@@ -126,11 +139,18 @@ export class NextcloudClient {
     const changed: string[] = [];
     for (const [occKey, value] of desired) {
       if (this.currentAppConfig(occKey) !== value) {
-        this.occ(["config:app:set", "onlyoffice", occKey, `--value=${value}`]);
+        // jwt_secret is a secret: apply it by stdin of config:import, never on an argv (issue 1831).
+        if (occKey === "jwt_secret") {
+          this.occ(["config:import"], { input: JSON.stringify({ apps: { onlyoffice: { jwt_secret: value } } }) });
+        } else {
+          this.occ(["config:app:set", "onlyoffice", occKey, `--value=${value}`]);
+        }
         changed.push(occKey);
       }
     }
-    if (changed.length === 0) return { changed };
+    // Nothing changed, and the last check (if any) passed: there is nothing to check again
+    // (novox/hq issue 1832). Otherwise — something changed, or the last check failed — run it.
+    if (changed.length === 0 && !this.lastCheckFailed) return { changed };
 
     let checkOk = true;
     try {
@@ -138,6 +158,7 @@ export class NextcloudClient {
     } catch {
       checkOk = false;
     }
+    this.lastCheckFailed = !checkOk;
     return { changed, checkOk };
   }
 

modules/nextcloud/index.ts

@@ -45,12 +45,20 @@ async function pollShares(client: NextcloudClient): Promise<void> {
 }
 
 // Keeps Nextcloud's onlyoffice settings matched to the document server the mesh bound (novox/hq
-// issue 1830). The client never logs; this says what it did.
+// issue 1830). The client never logs; this says what it did. A failed connection check is run
+// again on every later tick until it passes (issue 1832): every failing tick is logged, and one
+// line marks it passing again; nothing is logged while the last check passed and nothing changed.
+let onlyOfficeCheckWasFailing = false;
 async function applyOnlyOffice(client: NextcloudClient): Promise<void> {
   const result = client.applyOnlyOffice();
-  if (result.changed.length === 0) return;
-  console.log(`[nextcloud] onlyoffice: set ${result.changed.join(", ")}`);
-  if (result.checkOk === false) console.error("[nextcloud] onlyoffice: connection check failed after applying settings");
+  if (result.changed.length > 0) console.log(`[nextcloud] onlyoffice: set ${result.changed.join(", ")}`);
+  if (result.checkOk === false) {
+    onlyOfficeCheckWasFailing = true;
+    console.error("[nextcloud] onlyoffice: connection check failed" + (result.changed.length > 0 ? " after applying settings" : ""));
+  } else if (result.checkOk === true && onlyOfficeCheckWasFailing) {
+    onlyOfficeCheckWasFailing = false;
+    console.log("[nextcloud] onlyoffice: connection check passes again");
+  }
 }
 
 if (nextcloud) {

modules/nextcloud/test/onlyoffice.test.ts

@@ -33,8 +33,10 @@ function client(): any {
 /** A stand-in occ: config:app:get answers `current`, or throws like the real occ when unset. */
 function standInOcc(current: Record<string, string>, opts: { checkFails?: boolean } = {}) {
   const calls: string[][] = [];
-  const impl = (args: string[]): string => {
+  const inputs: Array<string | undefined> = [];
+  const impl = (args: string[], occOpts: { input?: string } = {}): string => {
     calls.push(args);
+    inputs.push(occOpts.input);
     if (args[0] === "config:app:get") {
       const key = args[2];
       if (!(key in current)) throw new Error("occ produced no output");
@@ -46,13 +48,18 @@ function standInOcc(current: Record<string, string>, opts: { checkFails?: boolea
       current[key] = value;
       return "System config value onlyoffice => " + key + " set to string " + value;
     }
+    if (args[0] === "config:import") {
+      const parsed = JSON.parse(occOpts.input ?? "{}");
+      Object.assign(current, parsed?.apps?.onlyoffice ?? {});
+      return "";
+    }
     if (args[0] === "onlyoffice:documentserver") {
       if (opts.checkFails) throw new Error("occ produced no output");
       return "";
     }
     throw new Error("unexpected occ call: " + args.join(" "));
   };
-  return { calls, impl };
+  return { calls, inputs, impl };
 }
 
 // 1830/AC3a: when the delivered values differ from Nextcloud's settings, it sets exactly the
@@ -77,6 +84,72 @@ test("sets exactly the keys that differ, and runs the connection check", () => {
   assert.equal(current.StorageUrl, FULL_CONFIG.onlyofficeStorageUrl, "the matching key was left alone");
 });
 
+// 1831/AC1: the jwt_secret never reaches a docker/occ argv — it is applied only as stdin of a
+// config:import call, never as a config:app:set --value on the command line (novox/hq issue 1831).
+test("the jwt_secret is set by stdin of config:import, never in an argv", () => {
+  const c = client();
+  const current: Record<string, string> = {
+    DocumentServerUrl: "https://old-office.example/",
+    DocumentServerInternalUrl: FULL_CONFIG.onlyofficeInternalUrl,
+    StorageUrl: FULL_CONFIG.onlyofficeStorageUrl,
+    jwt_secret: "a-stale-secret",
+  };
+  const stand = standInOcc(current);
+  mock.method(c, "occ", stand.impl);
+
+  const result = c.applyOnlyOffice(configFile(FULL_CONFIG));
+
+  assert.ok(result.changed.includes("jwt_secret"), "jwt_secret was changed");
+  assert.ok(!stand.calls.some((a) => a.join(" ").includes(SECRET)), "the secret is in no argv");
+  const importCall = stand.calls.findIndex((a) => a[0] === "config:import");
+  assert.notEqual(importCall, -1, "a config:import call was made");
+  assert.ok(!stand.calls.some((a) => a[0] === "config:app:set" && a[2] === "jwt_secret"),
+    "jwt_secret is never set via config:app:set");
+  assert.equal(stand.inputs[importCall], JSON.stringify({ apps: { onlyoffice: { jwt_secret: SECRET } } }),
+    "the secret is passed only as stdin of config:import");
+  assert.equal(current.jwt_secret, SECRET, "the secret was actually applied");
+});
+
+// 1832/AC1: a failed check keeps running on every later tick, even with nothing changed, reporting
+// checkOk false each time, until it passes — then nothing runs again while nothing changes
+// (novox/hq issue 1832).
+test("a failed check is re-run every tick until it passes, then nothing runs while unchanged", () => {
+  const c = client();
+  const current: Record<string, string> = {}; // every key unset, so tick 1 changes all four
+  const checkOpts: { checkFails?: boolean } = { checkFails: true };
+  const stand = standInOcc(current, checkOpts);
+  mock.method(c, "occ", stand.impl);
+  const checkCalls = () => stand.calls.filter((a) => a[0] === "onlyoffice:documentserver").length;
+  const file = configFile(FULL_CONFIG);
+
+  // Tick 1: everything changes, the check runs and fails (1832/AC1c: reported on a failing tick).
+  const tick1 = c.applyOnlyOffice(file);
+  assert.deepEqual(tick1.changed.sort(), ["DocumentServerInternalUrl", "DocumentServerUrl", "StorageUrl", "jwt_secret"].sort());
+  assert.equal(tick1.checkOk, false);
+  assert.equal(checkCalls(), 1);
+
+  // Tick 2: nothing changed, but the last check failed — it is run again (1832/AC1a), and fails
+  // again, reported again (1832/AC1c).
+  const tick2 = c.applyOnlyOffice(file);
+  assert.deepEqual(tick2.changed, []);
+  assert.equal(tick2.checkOk, false);
+  assert.equal(checkCalls(), 2, "the check ran again though nothing changed");
+
+  // Tick 3: the document server now answers; still nothing changed, but the last check had
+  // failed, so it is run again — and this time it passes.
+  checkOpts.checkFails = false;
+  const tick3 = c.applyOnlyOffice(file);
+  assert.deepEqual(tick3.changed, []);
+  assert.equal(tick3.checkOk, true);
+  assert.equal(checkCalls(), 3);
+
+  // Tick 4: nothing changed, and the last check passed — no check runs (1832/AC1b).
+  const tick4 = c.applyOnlyOffice(file);
+  assert.deepEqual(tick4.changed, []);
+  assert.equal(tick4.checkOk, undefined);
+  assert.equal(checkCalls(), 3, "no further check ran once the last one passed");
+});
+
 // 1830/AC3b: when they match, it changes nothing and runs no check.
 test("changes nothing and runs no check when every value already matches", () => {
   const c = client();