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 @@