From 8f2cb994ec71b8c999512e9efb5865ed788724ae Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 21:35:46 +0200 Subject: [PATCH] The code has to choose the template, not the clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sharing a second set of paid accounts with friends means a second template, and the route was not ready for one. It checked the code and then took the most recently updated row: findRecordsByFilter("onboarding_template", "id != ''", "-updated", 1, 0) With a single row that is invisible. With two it hands the family's credentials to friends, or the reverse, decided purely by which row was edited last, silently and with no error. status.html carried a comment warning about exactly this: "never create a second". So the code now selects the row, bound as a filter parameter rather than concatenated. AMBER_INVITE_CODE becomes a fallback that opens the row flagged isDefault, which means nobody halfway through setup broke and Coolify needed no edit; once the family row has its own code the env var stops mattering. A wrong code and a code with no row behind it give the same 403, since telling them apart would confirm which codes exist. Codes live on the rows rather than in more env vars, so a new group is one row in the admin UI instead of a redeploy, and each code is revocable on its own. The unique index is partial because PocketBase text fields default to '' and SQLite calls two empty strings equal. status.html read the template in three places, all by recency. They now go through familyTemplate(), which selects on isDefault, so adding a friends row cannot make the editor wander onto it or make account creation preload the wrong group. Its one-click preload stays family-only; a group picker there is left undone rather than half-built. The website says "rodinný kód" in four places and friends are not family, so that copy widens. The setup flow also offers "Mám kód" up front now: someone Richard shares accounts with has nothing to buy, and walking them through three price lists first would be actively misleading. Verified against PocketBase 0.39.6 with two rows and friends as the most recently updated, the state that used to break: each code resolved to its own credentials, the legacy env code resolved to family via isDefault, wrong and empty codes gave 403, a duplicate code was refused by the index, and familyTemplate() returned family while -updated returned friends. Co-Authored-By: Claude Opus 5 --- docs/onboarding-contract.md | 67 +++++++++++++ pb_hooks/onboarding.pb.js | 97 +++++++++++++------ .../1791000000_onboarding_template_codes.js | 75 ++++++++++++++ pb_public/index.html | 25 ++++- pb_public/status.html | 39 ++++++-- 5 files changed, 259 insertions(+), 44 deletions(-) create mode 100644 pb_migrations/1791000000_onboarding_template_codes.js diff --git a/docs/onboarding-contract.md b/docs/onboarding-contract.md index 70d3832..32e6640 100644 --- a/docs/onboarding-contract.md +++ b/docs/onboarding-contract.md @@ -191,3 +191,70 @@ banner; a reload kept it skipped rather than reopening; resuming came back in. Not yet verified: the whole thing against the live instance, and a real TorBox or webshare signup followed end to end by someone who does not already know the answer. + +## Several shared-credential groups, one code each (2026-08-12) + +The family shares one set of paid accounts. Friends now get a second set that is +deliberately not the family's, so `onboarding_template` holds one row per group. + +`name` labels it, `code` unlocks it, `isDefault` marks the family. The unique index +on `code` is partial (`WHERE code != ''`) because PocketBase text fields default to +`''` and SQLite treats two empty strings as equal, which would otherwise stop a +second row from existing until both had codes. + +### The code chooses the row, which it did not before + +The route used to check the code and then take the most recently updated row: + +```js +findRecordsByFilter("onboarding_template", "id != ''", "-updated", 1, 0) +``` + +With one row that was invisible. With two it would have handed the family's +credentials to friends, or the reverse, depending only on which row was edited +last, silently and with no error. The lookup is now `code = {:code}` bound as a +parameter, and `AMBER_INVITE_CODE` is a fallback that opens the `isDefault` row so +nobody mid-setup broke and Coolify needed no edit. Once the family row carries its +own code the env var is redundant. + +A wrong code and a code with no row behind it both return 403. Distinguishing them +would confirm which codes exist. The per-IP failure limit is unchanged and stays +per IP rather than per code, so guessing at one group cannot earn a fresh budget by +switching to another. + +### status.html is the FAMILY editor, explicitly + +The dashboard read the template in three places, all `sort=-updated`, and its own +comment warned "never create a second, or `-updated` ordering in the hook would +start deciding which config the family gets". All three now go through +`familyTemplate()`, which selects on `isDefault` and falls back to the single row +only on an instance that never had a template. So adding a friends row cannot make +the editor wander onto it, and cannot make account creation preload the wrong +group. + +**The one-click preload on account creation is family-only.** Other groups are +maintained in the PocketBase admin UI and their members type their own code on the +account page. A group picker in the dashboard is a further piece of work, listed as +not done rather than half-built. + +### Verified locally (2026-08-12) + +PocketBase 0.39.6 from a throwaway data dir with this repo's real migrations, hooks +and pages, `AMBER_INVITE_CODE=legacy-family-code`, and two rows carrying +deliberately different credentials. + +With **friends** as the most recently updated row, which is the exact state that +used to break: + +| given | resolves to | +|---|---| +| the friends code | friends credentials | +| the family's own code | family credentials | +| `AMBER_INVITE_CODE` | family credentials, via `isDefault` | +| a wrong code | 403 | +| an empty code | 403 | +| a second row reusing a code | rejected, 400, by the unique index | + +`familyTemplate()` returned `family` while `-updated` returned `friends`, which is +the divergence the change exists to create. Rate limiting still allows ten failures +and answers 429 on the eleventh. diff --git a/pb_hooks/onboarding.pb.js b/pb_hooks/onboarding.pb.js index 48a781c..56cd11e 100644 --- a/pb_hooks/onboarding.pb.js +++ b/pb_hooks/onboarding.pb.js @@ -6,9 +6,11 @@ // 1. Creating a `users` record is refused for everyone except a superuser. The // owner makes accounts in the PocketBase admin UI and hands over a temporary // password. There is no public signup and no invite string. -// 2. GET /api/amber/onboarding-template returns the admin-maintained plaintext -// family config, gated by auth AND the family code — family members use it, -// close friends bring their own credentials instead (BYOC). +// 2. GET /api/amber/onboarding-template returns an admin-maintained plaintext +// shared config, gated by auth AND a group code. There is one row per group +// that the owner shares accounts with (family, friends), each with its own +// credentials and its own code; anyone without a code brings their own +// accounts instead, through the setup flow on the account page. // // **This replaced a single shared invite code** (AMBER_INVITE_CODE) that gated // both doors. That code was a bearer secret: one string, the same for everyone, @@ -19,8 +21,10 @@ // them). For a household this size, "the owner makes the account" is stronger // than either and has nothing to leak. See docs/plan-accounts-and-web.md. // -// AMBER_INVITE_CODE survives with a narrower job: it is the *family* code that -// unlocks the shared template at setup, not a key to the front door. +// AMBER_INVITE_CODE survives with a narrower job again: it is no longer a key to +// the front door, and no longer the only group code either. Codes now live on the +// template rows, and the env var is a fallback that opens the row flagged +// `isDefault` so the move to several groups broke nobody mid-setup. // ── users create gate ───────────────────────────────────────────────────────── // Superuser only. The collection's createRule says the same thing (see the @@ -33,32 +37,36 @@ onRecordCreateRequest((e) => { }, "users") // ── GET /api/amber/onboarding-template ──────────────────────────────────────── -// Query: ?code=. → { addonUrl, czechAddonUrl, tmdbKey }. 403 on a -// bad code, 429 when rate-limited, 404 when no template record exists. +// Query: ?code=. → { addonUrl, czechAddonUrl, tmdbKey }. 403 for a code +// that matches nothing, 429 when rate-limited. There is no 404: "no such code" and +// "no row behind that code" are the same answer, because distinguishing them would +// confirm which codes exist. // -// **Auth AND the family code.** Two gates, because they answer different -// questions. Auth is now guaranteed — accounts are owner-created, so anyone -// reaching setup is signed in — but "has an account" must not mean "may have the -// family's shared credentials": close friends get accounts too, and they bring -// their own (BYOC). The code is what distinguishes *family* from *someone the -// owner also trusts with an account*. +// **One row per group, each with its own code.** The family shares one set of paid +// accounts; friends now get a second set that is deliberately not the family's. So +// the code does not just gate the route, it *selects the row* — which is the part +// that used to be missing (see the migration, and the comment at the lookup below). // -// So AMBER_INVITE_CODE keeps its job and loses its old one. It is no longer a -// registration key — registration is owner-only — it is the family credential -// selector during setup. The env var keeps its name so no Coolify change is -// needed; the name is now a misnomer and can be renamed at leisure. +// **Auth AND a code.** Two gates, because they answer different questions. Auth is +// guaranteed — accounts are owner-created, so anyone reaching setup is signed in — +// but "has an account" must not mean "may have somebody's shared credentials". The +// code is what says which group's credentials, if any, this person is owed. A friend +// with no code brings their own accounts instead, through the setup flow on the +// account page. // -// Rate-limited per IP as before: the code is still a shared secret, and being -// behind auth narrows who can guess at it but does not make guessing free. +// AMBER_INVITE_CODE is now only a fallback, opening the row flagged `isDefault`, so +// that the change broke nobody mid-setup and needed no Coolify edit. Give the family +// row its own `code` and the env var stops mattering. +// +// Rate-limited per IP as before, and per IP rather than per code on purpose: guessing +// at one group's code must not earn a fresh budget by switching to another's. routerAdd("GET", "/api/amber/onboarding-template", (e) => { const MAX_FAILS = 10 const WINDOW_SECONDS = 300 - const code = $os.getenv("AMBER_INVITE_CODE") - if (!code) return e.json(503, { error: "onboarding disabled" }) - // Fixed-window per-IP limit (app store — file scope state does not survive the - // isolated handler JSVMs). + // isolated handler JSVMs). Counted per IP rather than per code, so guessing at + // one group's code does not get a fresh budget by switching to another's. const store = $app.store() const key = "amber.onboardFails." + e.realIP() const now = Math.floor(Date.now() / 1000) @@ -67,23 +75,52 @@ routerAdd("GET", "/api/amber/onboarding-template", (e) => { if (st && st.count >= MAX_FAILS) { return e.json(429, { error: "too many attempts", retryAfter: st.resetAt - now }) } - - const given = (e.request.url.query().get("code") || "").trim() - if (given !== code) { + const fail = () => { store.set(key, { count: (st ? st.count : 0) + 1, resetAt: st ? st.resetAt : now + WINDOW_SECONDS, }) - return e.json(403, { error: "invalid family code" }) + return e.json(403, { error: "invalid code" }) } - store.set(key, null) + const given = (e.request.url.query().get("code") || "").trim() + // An empty code can never match: the filter below requires a non-empty stored + // code, and this returns before it. That is what keeps the route fail-closed on + // an instance where nothing has been configured yet. + if (!given) return fail() + + // **The code chooses the record.** It used to check the code and then take the + // most recently updated row regardless, which was invisible with one row and + // would have handed the family's credentials to friends with two. Bound as a + // filter parameter, never concatenated, so a code cannot smuggle in filter + // syntax of its own. let rec = null try { - const rows = $app.findRecordsByFilter("onboarding_template", "id != ''", "-updated", 1, 0) + const rows = $app.findRecordsByFilter( + "onboarding_template", "code != '' && code = {:code}", "-updated", 1, 0, + { code: given }) if (rows && rows.length) rec = rows[0] } catch (_) { /* collection empty */ } - if (!rec) return e.json(404, { error: "no template configured" }) + + // Fallback for the original single code, which lives in the environment and not + // on any row. It opens the row flagged `isDefault` (the family), so nobody + // halfway through setup is broken by this change and Coolify needs no edit. Once + // that row carries its own `code`, the env var is redundant and can go. + if (!rec) { + const legacy = ($os.getenv("AMBER_INVITE_CODE") || "").trim() + if (legacy && given === legacy) { + try { + const rows = $app.findRecordsByFilter( + "onboarding_template", "isDefault = true", "-updated", 1, 0) + if (rows && rows.length) rec = rows[0] + } catch (_) { /* no default row */ } + } + } + + // A wrong code and a code with no row behind it are the same answer on purpose: + // telling the difference would confirm which codes exist. + if (!rec) return fail() + store.set(key, null) return e.json(200, { addonUrl: rec.getString("addonUrl"), diff --git a/pb_migrations/1791000000_onboarding_template_codes.js b/pb_migrations/1791000000_onboarding_template_codes.js new file mode 100644 index 0000000..d9bf113 --- /dev/null +++ b/pb_migrations/1791000000_onboarding_template_codes.js @@ -0,0 +1,75 @@ +/// + +// More than one shared-credentials template, each behind its own code. +// +// The family shares one set of paid accounts. A few friends are now getting the +// same treatment, but must NOT land on the family's subscriptions, so there has to +// be a second template with its own credentials and its own code. +// +// **The bug this migration exists to make safe.** Before it, the route checked the +// code and then chose a record with `findRecordsByFilter(..., "-updated", 1, 0)` — +// newest wins, with no reference to the code that was given. One record made that +// invisible. A second record would have made the family code hand out whichever row +// was edited last, and the friends code do the same, silently and with no error. +// The hook is rewritten in the same commit; adding a row before that would have been +// the whole failure. +// +// **Why the code lives on the record and not in a new env var.** A second +// `AMBER_FRIENDS_CODE` would mean a Coolify env change and a redeploy for every +// group, and the codes would sit somewhere other than the credentials they unlock. +// Here a new group is one row in the admin UI: no deploy, each code rotatable on its +// own, and the code is visible next to what it hands out. +// +// **The unique index is partial.** PocketBase text fields default to `''`, not NULL, +// and SQLite treats two empty strings as equal — so a plain unique index would stop +// a second row from existing until both had codes. `WHERE code != ''` is the same +// shape PocketBase uses for its own optional-unique columns (see the `email` index on +// `users`). +// +// **No secret is written here.** `AMBER_INVITE_CODE` is deliberately not copied into +// the family row: a migration that bakes in a credential is one that leaks it into +// every future database dump for no gain. The row is flagged `isDefault` instead, and +// the hook keeps honouring the env var for it until the owner pastes a real code in. +// That also means anyone halfway through setup right now is unaffected. + +migrate( + (app) => { + const tpl = app.findCollectionByNameOrId("onboarding_template") + // Which group this is, for the admin list. Never served to clients. + tpl.fields.add(new Field({ type: "text", name: "name", max: 80 })) + // The shared secret that unlocks this row. Still a bearer secret, still + // rate-limited per IP in the hook; being per row is what makes it revocable + // without touching the others. + tpl.fields.add(new Field({ type: "text", name: "code", max: 200 })) + // Which row the legacy AMBER_INVITE_CODE still opens. + tpl.fields.add(new Field({ type: "bool", name: "isDefault" })) + tpl.indexes = (tpl.indexes || []).concat([ + "CREATE UNIQUE INDEX idx_onboarding_template_code " + + "ON onboarding_template (code) WHERE code != ''", + ]) + app.save(tpl) + + // Preserve today's behaviour exactly: the row the route is currently handing + // out is the most recently updated one, so that is the one the old code must + // keep opening. Marking every row would make the fallback ambiguous again. + try { + const rows = app.findRecordsByFilter( + "onboarding_template", "id != ''", "-updated", 1, 0) + if (rows && rows.length) { + const r = rows[0] + r.set("isDefault", true) + if (!r.getString("name")) r.set("name", "family") + app.save(r) + } + } catch (_) { /* no template configured yet: nothing to preserve */ } + }, + (app) => { + const tpl = app.findCollectionByNameOrId("onboarding_template") + tpl.indexes = (tpl.indexes || []).filter( + (i) => !i.includes("idx_onboarding_template_code")) + tpl.fields.removeByName("name") + tpl.fields.removeByName("code") + tpl.fields.removeByName("isDefault") + app.save(tpl) + }, +) diff --git a/pb_public/index.html b/pb_public/index.html index 3f73e85..6aa9cb5 100644 --- a/pb_public/index.html +++ b/pb_public/index.html @@ -267,16 +267,18 @@