From 1cc54959afcf4d2fe658ed47531829e45c6aee89 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 25 Oct 2026 16:57:46 +0100 Subject: [PATCH] nextcloud: repair the account only on a 401, never reset the throttle (issue 1253, review 2) Resetting Nextcloud's brute-force record for the docker gateway cleared it for every client behind that address: someone guessing passwords could drive the throttle to 429 and have the module's poll wipe it. A throttle is no evidence the password is wrong, so a 429 is now backed off only, and the back-off outlasts the throttle. The account's existence is read from user:info's exit status, since user:list pages. --- modules/nextcloud/client.ts | 67 ++++++++++------------ modules/nextcloud/test/admin-login.test.ts | 64 ++++++++++++--------- 2 files changed, 68 insertions(+), 63 deletions(-) diff --git a/modules/nextcloud/client.ts b/modules/nextcloud/client.ts index 95c94977..1dfaaaa6 100644 --- a/modules/nextcloud/client.ts +++ b/modules/nextcloud/client.ts @@ -185,7 +185,9 @@ export class NextcloudClient { // occ writes many refusals ("user not found") to stdout with an empty stderr: say both. const detail = [err?.stderr, err?.stdout].map((x) => String(x ?? "").trim()).filter(Boolean).join("\n") || String(err?.message ?? "").trim(); - throw new Error(detail || `occ produced no output — is the ${this.container} container running?`); + // The exit status rides along, so a caller can tell occ's own answer from a failure. + throw Object.assign(new Error(detail || `occ produced no output — is the ${this.container} container running?`), + { status: typeof err?.status === "number" ? err.status : undefined }); } } @@ -265,49 +267,40 @@ export class NextcloudClient { * The image creates NEXTCLOUD_ADMIN_USER only when it installs a fresh instance, so an instance * the mesh adopted never has it, and a secret made anew never reaches it. Only the module's own * account (MESH_ACCOUNT) is touched: under any other name it writes nothing and throws - * LoginRefusedError. Through occ: whether the account exists is read from user:list's - * membership; a missing one is added, an existing one gets its password set to the secret. It - * gets no admin group: the shares poll needs no admin rights (least privilege). The password - * goes by OC_PASS (`--password-from-env`), never on an argv. Then the brute-force record of the - * addresses the module's calls arrive from is reset, so the first good login is not held behind - * the throttle. A failing occ write is thrown as LoginRefusedError carrying occ's own message. + * LoginRefusedError. Through occ: whether the account exists is read from `user:info`, whose + * exit status 1 with "not found" means missing (user:list pages, so it is not asked); a missing + * one is added, an existing one gets its password set to the secret. It gets no admin group: the + * shares poll needs no admin rights (least privilege). The password goes by OC_PASS + * (`--password-from-env`), never on an argv. Nextcloud's brute-force record is left alone: it is + * shared by every client behind the same address, and the caller's back-off outlasts it. A + * failing occ is thrown as LoginRefusedError carrying occ's own message. */ - ensureAdminAccount(): { account: "created" | "password-set"; throttleReset: boolean } { + ensureAdminAccount(): "created" | "password-set" { if (this.adminUser !== MESH_ACCOUNT) { throw new LoginRefusedError(401, this.adminUser, `not repaired: only ${MESH_ACCOUNT}, the module's own account, is`); } const pass = { env: { OC_PASS: this.adminPassword } }; - let account: "created" | "password-set"; try { - if (!this.listUsers().some((u) => u.uid === this.adminUser)) { + if (!this.accountExists()) { this.occ(["user:add", "--password-from-env", "--display-name=Mesh administration", this.adminUser], pass); - account = "created"; - } else { - this.occ(["user:resetpassword", "--password-from-env", this.adminUser], pass); - account = "password-set"; + return "created"; } + this.occ(["user:resetpassword", "--password-from-env", this.adminUser], pass); + return "password-set"; } catch (err) { throw new LoginRefusedError(401, this.adminUser, `repair failed: ${String((err as Error)?.message ?? err).split("\n")[0]}`); } - let throttleReset = true; - try { - for (const address of this.callerAddresses()) this.occ(["security:bruteforce:reset", address]); - } catch { - throttleReset = false; - } - return { account, throttleReset }; } - /** - * The addresses the module's calls reach Nextcloud from, as Nextcloud sees them: the gateway of - * each network the container is on (a published port arrives from it; the access log shows - * 172.17.0.1 on the default bridge). - */ - callerAddresses(): string[] { - const out = execFileSync("docker", ["inspect", "-f", "{{range .NetworkSettings.Networks}}{{.Gateway}} {{end}}", this.container], { - encoding: "utf8", timeout: 30_000, - }); - return out.split(/\s+/).filter(Boolean); + /** Whether the account exists: occ user:info answers it, exit 1 and "not found" when missing. */ + private accountExists(): boolean { + try { + this.occ(["user:info", this.adminUser, "--output=json"]); + return true; + } catch (err: any) { + if (err?.status === 1 && /not found/i.test(String(err?.message ?? ""))) return false; + throw err; + } } private fetchShares(): Promise { @@ -319,15 +312,15 @@ export class NextcloudClient { } /** - * List every share, over the OCS Sharing API as the mesh's own account. A refused login (401) or - * a throttle (429) makes the account right once (ensureAdminAccount) and asks again; a refusal - * or a throttle after that throws LoginRefusedError, so the caller backs off (novox/hq issue 1253). + * List every share, over the OCS Sharing API as the mesh's own account. A refused login (401) + * makes the account right once (ensureAdminAccount) and asks again; a refusal after that, or + * any throttle (429), throws LoginRefusedError, so the caller backs off (novox/hq issue 1253). */ async listShares(): Promise { let res = await this.fetchShares(); - // A throttle (429) follows refusals, so it gets the same one repair: the repair goes by occ, - // never by a login, and the throttle then wears off under the caller's back-off. - if ((res.status === 401 || res.status === 429) && !this.accountMadeRight) { + // Only a refused login (401) is repaired. A throttle (429) is no evidence the password is + // wrong (anyone guessing passwords from the same address causes it): it is backed off only. + if (res.status === 401 && !this.accountMadeRight) { await res.text(); this.accountMadeRight = true; this.ensureAdminAccount(); diff --git a/modules/nextcloud/test/admin-login.test.ts b/modules/nextcloud/test/admin-login.test.ts index 61496606..218c9030 100644 --- a/modules/nextcloud/test/admin-login.test.ts +++ b/modules/nextcloud/test/admin-login.test.ts @@ -16,10 +16,7 @@ const REFUSED_XML = `\n\n \n failure ["172.17.0.1"]); - return c; + return new (NextcloudClient as any)("nextcloud", "http://127.0.0.1:20003", user, PASSWORD); } /** A stand-in Nextcloud: answers `statuses` in turn (the last one repeating) and counts the asks. */ @@ -41,25 +38,26 @@ function standInOcc(accountExists: boolean, opts: { failWrite?: string } = {}) { const impl = (args: string[], occOpts: { env?: Record } = {}): string => { calls.push(args); envs.push(occOpts.env); - if (args[0] === "user:list") { - return JSON.stringify(accountExists ? { jochens: "Jochen", "mesh-admin": "Mesh administration" } : { jochens: "Jochen" }); + if (args[0] === "user:info") { + // As the real occ() throws it: occ's stdout as the message, its exit status alongside. + if (!accountExists) throw Object.assign(new Error("user not found"), { status: 1 }); + return "{}"; } if (["user:add", "user:resetpassword"].includes(args[0])) { if (opts.failWrite) throw new Error(opts.failWrite); return ""; } - if (args[0] === "security:bruteforce:reset") return ""; throw new Error("unexpected occ call: " + args.join(" ")); }; return { calls, envs, impl }; } -/** The occ calls that write: anything but a read of the user list. */ -const writes = (calls: string[][]) => calls.filter((a) => a[0] !== "user:list"); +/** The occ calls that write: anything but a read of the account. */ +const writes = (calls: string[][]) => calls.filter((a) => a[0] !== "user:info"); // 1253/AC1: given an adopted instance without the mesh's account, when Nextcloud refuses the // login, then the account is added (no admin group) with the module's own secret, by OC_PASS and -// never on an argv, the throttle of the caller's address is reset, and the shares are listed. +// never on an argv, and the shares are listed; Nextcloud's brute-force record is left alone. test("a refused login adds the missing account, password by environment, and lists the shares", async () => { const c = client(); const occ = standInOcc(false); @@ -74,7 +72,7 @@ test("a refused login adds the missing account, password by environment, and lis assert.ok(add, "user:add ran"); assert.ok(add.includes("--password-from-env") && add.includes("mesh-admin")); assert.ok(!add.some((a) => a.includes("group")), "no admin group: least privilege"); - assert.deepEqual(occ.calls.at(-1), ["security:bruteforce:reset", "172.17.0.1"]); + assert.deepEqual(occ.calls.map((a) => a[0]), ["user:info", "user:add"]); assert.equal(occ.envs[occ.calls.indexOf(add)]?.OC_PASS, PASSWORD); assert.ok(!occ.calls.flat().some((a) => a.includes(PASSWORD)), "the password is on no argv"); } finally { @@ -83,7 +81,7 @@ test("a refused login adds the missing account, password by environment, and lis }); // 1253/AC2: given the account exists with another password, when Nextcloud refuses the login, -// then its password is set to the module's own secret, in no group, and the throttle is reset. +// then its password is set to the module's own secret, in no group. test("a refused login resets an existing account's password, in no group", async () => { const c = client(); const occ = standInOcc(true); @@ -91,7 +89,7 @@ test("a refused login resets an existing account's password, in no group", async const f = mock.method(globalThis, "fetch", standInFetch([401, 200]).impl); try { await c.listShares(); - assert.deepEqual(occ.calls.map((a) => a[0]), ["user:list", "user:resetpassword", "security:bruteforce:reset"]); + assert.deepEqual(occ.calls.map((a) => a[0]), ["user:info", "user:resetpassword"]); assert.equal(occ.envs[1]?.OC_PASS, PASSWORD); assert.ok(!occ.calls.flat().some((a) => a.includes(PASSWORD))); } finally { @@ -124,17 +122,33 @@ test("a refusal after the repair throws LoginRefusedError once-repaired, short a } }); -// 1253/AC3b: a throttle (429) follows refusals: it gets the same one repair, and a throttle -// that stays throws LoginRefusedError. -test("a throttled login (429) is repaired once and then throws LoginRefusedError", async () => { +// 1253/AC3b: a throttle (429) is no evidence the password is wrong: two cycles of 429 then 200 +// run no occ write, and the 429s are LoginRefusedError for the back-off. +test("a throttle (429) is backed off, never repaired", async () => { const c = client(); const occ = standInOcc(true); mock.method(c, "occ", occ.impl); - const f = mock.method(globalThis, "fetch", standInFetch([429]).impl); + const f = mock.method(globalThis, "fetch", standInFetch([429, 200, 429, 200]).impl); try { - await assert.rejects(c.listShares(), (err: any) => err instanceof LoginRefusedError && err.status === 429); - await assert.rejects(c.listShares(), LoginRefusedError); - assert.equal(occ.calls.filter((a) => a[0] === "user:resetpassword").length, 1); + for (let cycle = 0; cycle < 2; cycle++) { + await assert.rejects(c.listShares(), (err: any) => err instanceof LoginRefusedError && err.status === 429); + assert.equal((await c.listShares()).length, 1); + } + assert.deepEqual(occ.calls, [], "no occ call at all, so no write"); + } finally { + f.mock.restore(); + } +}); + +// 1253/AC3c: a 401 runs exactly one repair. +test("a 401 runs exactly one repair", async () => { + const c = client(); + const occ = standInOcc(true); + mock.method(c, "occ", occ.impl); + const f = mock.method(globalThis, "fetch", standInFetch([401, 200]).impl); + try { + await c.listShares(); + assert.equal(writes(occ.calls).length, 1); } finally { f.mock.restore(); } @@ -252,7 +266,7 @@ test("a failing repair carries occ's message into LoginRefusedError", async () = }); /** A fake docker on PATH: answers occ like Nextcloud (user:info's refusal on stdout, exit 1). */ -function fakeDocker(users: Record): { dir: string; record: string } { +function fakeDocker(): { dir: string; record: string } { const dir = mkdtempSync(join(tmpdir(), "nextcloud-1253-docker-")); const record = join(dir, "record"); writeFileSync(join(dir, "docker"), [ @@ -260,8 +274,6 @@ function fakeDocker(users: Record): { dir: string; record: strin `printf '%s\\n' "$*" >> ${record}`, 'case "$*" in', " *' occ user:info '*) echo 'user not found'; exit 1 ;;", - ` *' occ user:list '*) printf '%s' '${JSON.stringify(users)}' ;;`, - " inspect*) echo '172.17.0.1 ' ;;", "esac", "exit 0", "", @@ -279,15 +291,15 @@ function withPath(dir: string, fn: () => T): T { // 1253/AC9: occ's own refusal on stdout with an empty stderr reaches the error, and the real // occ() through a docker answering like Nextcloud makes the missing account. test("occ says a refusal printed on stdout, and the real occ makes the missing account", () => { - const docker = fakeDocker({ jochens: "Jochen" }); + const docker = fakeDocker(); const c = new (NextcloudClient as any)("nextcloud", "http://127.0.0.1:20003", "mesh-admin", PASSWORD); withPath(docker.dir, () => { assert.throws(() => c.occ(["user:info", "mesh-admin", "--output=json"]), /user not found/); const did = c.ensureAdminAccount(); - assert.deepEqual(did, { account: "created", throttleReset: true }); + assert.equal(did, "created"); }); const lines = readFileSync(docker.record, "utf8").trim().split("\n"); assert.ok(lines.some((l) => l.endsWith("php occ user:add --password-from-env --display-name=Mesh administration mesh-admin")), lines.join("\n")); - assert.ok(lines.some((l) => l.endsWith("php occ security:bruteforce:reset 172.17.0.1")), lines.join("\n")); + assert.ok(!lines.some((l) => l.includes("bruteforce")), "the brute-force record is left alone"); assert.ok(!lines.join("\n").includes(PASSWORD)); });