From 8928e6f97268e09cb28b9d12ede0cc1e51b85dbe Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 16:27:15 +0200 Subject: [PATCH 1/4] nextcloud: keep onlyoffice's jwt_secret off every argv (issue 1831) occ's config:app:set put the secret on the docker exec command line, so it sat in the docker CLI's argv, the container's php argv and docker's exec record. Apply jwt_secret through config:import on occ's stdin instead; the other three onlyoffice keys still go through config:app:set. --- modules/nextcloud/client.ts | 22 +++++++++++--- modules/nextcloud/test/onlyoffice.test.ts | 37 +++++++++++++++++++++-- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/modules/nextcloud/client.ts b/modules/nextcloud/client.ts index e2310704..a3d4bc3b 100644 --- a/modules/nextcloud/client.ts +++ b/modules/nextcloud/client.ts @@ -92,11 +92,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,7 +135,12 @@ 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); } } diff --git a/modules/nextcloud/test/onlyoffice.test.ts b/modules/nextcloud/test/onlyoffice.test.ts index 09241597..52d4896d 100644 --- a/modules/nextcloud/test/onlyoffice.test.ts +++ b/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, opts: { checkFails?: boolean } = {}) { const calls: string[][] = []; - const impl = (args: string[]): string => { + const inputs: Array = []; + 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, 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,32 @@ 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 = { + 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"); +}); + // 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(); From edb6d3b7d3e187e2540ef7c75064afab3d7040f6 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 16:28:35 +0200 Subject: [PATCH 2/4] nextcloud: keep checking onlyoffice's connection while the last check failed (issue 1832) applyOnlyOffice only ran the connection check when a setting changed, so once it failed while settings matched, nothing checked again and the failure was logged once and forgotten. The client now remembers whether the last check failed and re-runs it on every tick until it passes; index.ts logs every failing tick and one line when it recovers. --- modules/nextcloud/client.ts | 9 ++++- modules/nextcloud/index.ts | 16 ++++++--- modules/nextcloud/test/onlyoffice.test.ts | 40 +++++++++++++++++++++++ 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/modules/nextcloud/client.ts b/modules/nextcloud/client.ts index a3d4bc3b..f0461db8 100644 --- a/modules/nextcloud/client.ts +++ b/modules/nextcloud/client.ts @@ -62,6 +62,10 @@ const ONLYOFFICE_KEYS: ReadonlyArray { } // 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 { 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) { diff --git a/modules/nextcloud/test/onlyoffice.test.ts b/modules/nextcloud/test/onlyoffice.test.ts index 52d4896d..5c90b9d8 100644 --- a/modules/nextcloud/test/onlyoffice.test.ts +++ b/modules/nextcloud/test/onlyoffice.test.ts @@ -110,6 +110,46 @@ test("the jwt_secret is set by stdin of config:import, never in an argv", () => 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 = {}; // 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(); From d0a3784be28f401445a2871941ffb5512dc3d503 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 16:34:06 +0200 Subject: [PATCH 3/4] only-office: bar every account but the converter from the state root (issue 1843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lib (bind-mounted into the container at /var/lib/onlyoffice) was 0711 so nginx inside the container could reach App_Data/cache, and the module root above it stayed 0711 too, so any account on the machine could walk through the root into lib's 0755 directories and 0644 files to read converted documents. The container never passes through the root — only the converter process does, to read jwt.secret and broker-converter.secret — so the root is now 0710, owned by the converter account: only that account and root pass through it. lib stays 0711 (1837/AC3). --- modules/only-office/manifest_test.go | 52 ++++++++++++++++++++++++++++ modules/only-office/module.json | 3 +- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/modules/only-office/manifest_test.go b/modules/only-office/manifest_test.go index 88cbddfe..7d28056f 100644 --- a/modules/only-office/manifest_test.go +++ b/modules/only-office/manifest_test.go @@ -125,3 +125,55 @@ func TestLibDirectoryLetsNginxThrough(t *testing.T) { } t.Fatal("manifest declares no lib directory") } + +// 1843/AC1 (unit: manifest check) +// Given the catalogue, when only-office's manifest is read, then its state directory (the module +// root, bind-mounted nowhere, so the container never passes through it) is mode 0710 and belongs +// to the converter account — the only host process that reaches through the root to read +// ${dir:state}/jwt.secret and broker-converter.secret — so no other account on the machine can +// pass through it to the converted documents under lib (which stays 0711, 1837/AC3). +func TestStateDirectoryBarsOtherAccounts(t *testing.T) { + raw, err := os.ReadFile("module.json") + if err != nil { + t.Fatalf("reading module.json: %v", err) + } + var m struct { + Resources []struct { + ID string `json:"id"` + Type string `json:"type"` + Mode string `json:"mode"` + Owner string `json:"owner"` + Name string `json:"name"` + } `json:"resources"` + } + if err := json.Unmarshal(raw, &m); err != nil { + t.Fatalf("parsing module.json: %v", err) + } + + var converterAccount string + for _, r := range m.Resources { + if r.Type == "user" { + converterAccount = r.Name + } + } + if converterAccount == "" { + t.Fatal("manifest declares no user resource for the converter account") + } + + var sawState bool + for _, r := range m.Resources { + if r.ID == "state" && r.Type == "directory" { + sawState = true + if r.Mode != "0710" { + t.Errorf("state directory mode = %q, want 0710", r.Mode) + } + if r.Owner != converterAccount { + t.Errorf("state directory owner = %q, want %q, the converter account (whose own group "+ + "the mode's group bit then names)", r.Owner, converterAccount) + } + } + } + if !sawState { + t.Fatal("manifest declares no state directory") + } +} diff --git a/modules/only-office/module.json b/modules/only-office/module.json index e31fefc0..35987242 100644 --- a/modules/only-office/module.json +++ b/modules/only-office/module.json @@ -144,7 +144,8 @@ { "id": "state", "type": "directory", - "mode": "0711", + "mode": "0710", + "owner": "only-office-converter", "place": ".", "unknown": "program" }, From 6bf054b9826199f7759b3ee07051be8c083bdcd2 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 16:37:35 +0200 Subject: [PATCH 4/4] Take issue 1843's owner change back out: giving the converter the module root opens a link swap to root (review), so 1843 needs its own design --- modules/only-office/manifest_test.go | 52 ---------------------------- modules/only-office/module.json | 3 +- 2 files changed, 1 insertion(+), 54 deletions(-) diff --git a/modules/only-office/manifest_test.go b/modules/only-office/manifest_test.go index 7d28056f..88cbddfe 100644 --- a/modules/only-office/manifest_test.go +++ b/modules/only-office/manifest_test.go @@ -125,55 +125,3 @@ func TestLibDirectoryLetsNginxThrough(t *testing.T) { } t.Fatal("manifest declares no lib directory") } - -// 1843/AC1 (unit: manifest check) -// Given the catalogue, when only-office's manifest is read, then its state directory (the module -// root, bind-mounted nowhere, so the container never passes through it) is mode 0710 and belongs -// to the converter account — the only host process that reaches through the root to read -// ${dir:state}/jwt.secret and broker-converter.secret — so no other account on the machine can -// pass through it to the converted documents under lib (which stays 0711, 1837/AC3). -func TestStateDirectoryBarsOtherAccounts(t *testing.T) { - raw, err := os.ReadFile("module.json") - if err != nil { - t.Fatalf("reading module.json: %v", err) - } - var m struct { - Resources []struct { - ID string `json:"id"` - Type string `json:"type"` - Mode string `json:"mode"` - Owner string `json:"owner"` - Name string `json:"name"` - } `json:"resources"` - } - if err := json.Unmarshal(raw, &m); err != nil { - t.Fatalf("parsing module.json: %v", err) - } - - var converterAccount string - for _, r := range m.Resources { - if r.Type == "user" { - converterAccount = r.Name - } - } - if converterAccount == "" { - t.Fatal("manifest declares no user resource for the converter account") - } - - var sawState bool - for _, r := range m.Resources { - if r.ID == "state" && r.Type == "directory" { - sawState = true - if r.Mode != "0710" { - t.Errorf("state directory mode = %q, want 0710", r.Mode) - } - if r.Owner != converterAccount { - t.Errorf("state directory owner = %q, want %q, the converter account (whose own group "+ - "the mode's group bit then names)", r.Owner, converterAccount) - } - } - } - if !sawState { - t.Fatal("manifest declares no state directory") - } -} diff --git a/modules/only-office/module.json b/modules/only-office/module.json index 35987242..e31fefc0 100644 --- a/modules/only-office/module.json +++ b/modules/only-office/module.json @@ -144,8 +144,7 @@ { "id": "state", "type": "directory", - "mode": "0710", - "owner": "only-office-converter", + "mode": "0711", "place": ".", "unknown": "program" },