diff --git a/.changeset/operator-provisioning-ticket-binding.md b/.changeset/operator-provisioning-ticket-binding.md new file mode 100644 index 0000000000..fe60d882d7 --- /dev/null +++ b/.changeset/operator-provisioning-ticket-binding.md @@ -0,0 +1,53 @@ +--- +"@objectstack/plugin-auth": minor +--- + +fix(plugin-auth): bind the dev-admin seed's operator-provisioning ticket to more than the seed address, so a concurrent stranger posting that address cannot ride the window (#14373) + +`AuthManager.stageOperatorProvisioning` staged its ticket keyed on +`email.trim().toLowerCase()` alone. The address is **not a secret** — +`admin@objectos.ai` is the documented default and the boot banner prints it +(with the password) once the seed completes — so "the address is the +operator's own" did not narrow the attacker set the way it would for an +unguessable value. A stranger's own concurrent `POST /sign-up/email` for that +same address, arriving while the ticket was staged, would satisfy an +email-only peek at both admission seams (the `disableSignUp` before-hook and +`validateAudienceAdmission`'s `creationClass` computation) and be admitted as +the `operator` class too — and since a unique-email constraint lets only one +of the two concurrent `signUpEmail` calls actually land, a stranger who won +that race would not merely read as the operator, their row would BECOME the +account at that address. The safety this rested on — "milliseconds, and +`NODE_ENV==='development'` only" — was true today, but both are properties of +the *caller*, not of what the ticket asserted. + +`stageOperatorProvisioning(email)` now also generates a random, unguessable +ticket value (128 bits from WebCrypto's `getRandomValues`, matching the +existing `resolvePasswordHasher` salt convention) and returns it. +`AuthPlugin.maybeSeedDevAdmin` threads that value into the SAME `signUpEmail` +call's body, under the new `AuthManager.OPERATOR_PROVISIONING_TICKET_FIELD` +key — an unrecognized key that better-auth's `signUpEmailBodySchema` (`.and( +z.record(z.string(), z.any()))`) lets ride through untouched, so it never +becomes a declared `sys_user` field. `isOperatorProvisioning(email, ticket)` +now requires an exact match on both; a missing, wrong-typed, or mismatched +ticket reads as "not provisioning" — the same as no ticket staged at all, +with no email-only fallback. This converts what was a timing argument +("the window is short and dev-only") into a structural one: admission now +asks "did THIS process's own boot command make THIS exact call", and a +stranger's request carries no value that was ever transmitted anywhere for +them to replay, however precisely they time the window. + +**Public surface**: `stageOperatorProvisioning`'s return type widens from +`void` to `string` (additive — any existing caller ignoring the return value +is unaffected), `isOperatorProvisioning` gains an optional second parameter +(omitting it now always reads as "not provisioning", which is the safe +default), and the class gains one new static member, +`AuthManager.OPERATOR_PROVISIONING_TICKET_FIELD`. `AuthManager` is +barrel-public (`@objectstack/plugin-auth`'s `index.ts` re-exports it in +full), so this is graded `minor` rather than `patch`. + +**Not touched**: the JSDoc documenting the in-process trust assumption on +`stageOperatorProvisioning` (already accurate on landed `main`), and the +method's barrel-public location (a published-surface removal is a decision +card, not a dev-agent edit) — both dispositions triage already closed out on +this card. Whether the bootstrap window should count humans or logins is +`#14349`'s question, not touched here. diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index dd8dffd335..66ba6e5cd1 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -2066,8 +2066,17 @@ export class AuthManager { // carve-out: an app that seeds people makes the bootstrap probe // answer "populated" before the seed ever runs. Cheap synchronous // check first, so the ticket path costs no I/O. + // [#14373] The email alone no longer admits — see + // `stageOperatorProvisioning`'s doc. `ctx.body` is the vendor's own + // zod-validated body, which keeps any key outside the declared + // schema (`signUpEmailBodySchema.and(z.record(...))`), so the + // in-process seed call's ticket field survives here untouched. const signUpEmail = typeof ctx?.body?.email === 'string' ? ctx.body.email : undefined; - if (this.isOperatorProvisioning(signUpEmail) || (await this.isBootstrapCreation())) { + const provisioningTicket = ctx?.body?.[AuthManager.OPERATOR_PROVISIONING_TICKET_FIELD]; + if ( + this.isOperatorProvisioning(signUpEmail, provisioningTicket) || + (await this.isBootstrapCreation()) + ) { ctx.context.__osDisableSignUpOrig = ep.disableSignUp; ep.disableSignUp = false; } @@ -3809,8 +3818,12 @@ export class AuthManager { * [#14157] Addresses the deployment's OWN boot command is provisioning right * now — see {@link stageOperatorProvisioning} for why this exists and why it * is not the bootstrap probe. + * + * [#14373] Keyed by email, but admission also requires the per-stage + * `ticket` below — see {@link stageOperatorProvisioning} for why the + * address alone stopped being enough. */ - private pendingOperatorProvisioning = new Map(); + private pendingOperatorProvisioning = new Map(); /** * Deliberately short. The window this covers is a single in-process @@ -3819,6 +3832,19 @@ export class AuthManager { */ private static readonly OPERATOR_PROVISIONING_STAGE_TTL_MS = 60 * 1000; + /** + * [#14373] Body field the ticket's random value rides on for the single + * in-process `signUpEmail` call it admits. `signUpEmailBodySchema` in + * better-auth is `z.object({...}).and(z.record(z.string(), z.any()))`, so + * an unrecognized key survives validation on `ctx.body` untouched — this + * rides that catch-all rather than a declared `additionalFields` column, + * so it never becomes a `sys_user` field. The name is deliberately + * internal-looking; nothing public ever sends this key, so its presence in + * a request is itself the credential this seam demands (see + * {@link isOperatorProvisioning}). + */ + static readonly OPERATOR_PROVISIONING_TICKET_FIELD = '__osOperatorProvisioningTicket'; + /** * Page size of the bootstrap population probe ({@link isBootstrapCreation}). * Matches the bound the dev-admin seed reads with, so the two ask the same @@ -3887,17 +3913,23 @@ export class AuthManager { * OAuth flows redirect to the error URL carrying the same code). * * [#11767] The vendor's second argument — the endpoint context — is - * deliberately UNREAD. Both probes below take their own ctx-independent data - * path, because sourcing this gate's I/O from the request context is exactly - * what made the bootstrap bypass inert (see {@link isBootstrapCreation}). - * The parameter is kept so the signature still reads as the vendor's. + * deliberately UNREAD for the population probes below: both take their own + * ctx-independent data path, because sourcing THEIR I/O from the request + * context is exactly what made the bootstrap bypass inert (see {@link + * isBootstrapCreation}). [#14373] carves one narrow exception: the operator + * ticket's random value (see `stageOperatorProvisioning`), which travels + * only as a `ctx.body` field and has no other channel to reach this gate — + * `data.user` here is `internalAdapter.createUser`'s own input, not the raw + * request body, so it never carries an undeclared key. That is a plain read + * of an already-parsed value, not a re-derived I/O probe, so it does not + * reopen what #11767 closed. */ private async validateAudienceAdmission( data: { user?: Record; source?: { action?: string; method?: string; oauth?: { providerId?: string } }; }, - _ctx?: unknown, + ctx?: unknown, ): Promise<{ error: string; errorDescription?: string } | undefined> { try { // link-account / provider sign-in concern an EXISTING user's identity, @@ -3911,7 +3943,15 @@ export class AuthManager { // because the seed reaches better-auth through the same `signUpEmail` // API a person's sign-up does. See `stageOperatorProvisioning` for why // this is a declared ticket and not a wider bootstrap probe. - const creationClass: AudienceCreationClass = this.isOperatorProvisioning(email) + // [#14373] …and why email alone no longer decides it: the ticket's + // random value must also match, read off the SAME endpoint context the + // `disableSignUp` before-hook already reads `ctx.body.email` from + // (`getCurrentAuthEndpointContext()` — one context per request, shared + // across the before/after hooks and this validateUserInfo callback). + const provisioningTicket = (ctx as { body?: Record } | undefined)?.body?.[ + AuthManager.OPERATOR_PROVISIONING_TICKET_FIELD + ]; + const creationClass: AudienceCreationClass = this.isOperatorProvisioning(email, provisioningTicket) ? 'operator' : classifyCreationMethod(data?.source, { enterpriseOAuthProviderIds: this.enterpriseOAuthProviderIds(), @@ -4229,10 +4269,39 @@ export class AuthManager { * `NODE_ENV==='development'`, the ticket names ONE address, the caller * clears it in a `finally`, and anything that outlives that is pruned by * {@link OPERATOR_PROVISIONING_STAGE_TTL_MS}. - */ - stageOperatorProvisioning(email: string): void { + * + * ## [#14373] Why the address alone stopped being the whole ticket + * + * The address is **not a secret**: `admin@objectos.ai` is the documented + * default and the boot banner prints it (with the password) once the seed + * completes. "The address is the operator's own" is true but does not + * narrow the attacker set the way it would for an unguessable value — a + * stranger's OWN concurrent `POST /sign-up/email` for that same address, + * arriving while this ticket is staged, would satisfy an email-only peek at + * BOTH admission seams and get admitted as `operator` class: past the + * `disableSignUp` bypass, and (since the stamp only cares about the + * creation class) potentially email-verified at creation too — a stranger + * would not merely read as the operator, their row would BECOME the + * account at that address, since a unique-email constraint lets only one of + * the two concurrent `signUpEmail` calls actually land. Milliseconds and + * `NODE_ENV==='development'` are true today, but both are properties of the + * *caller*, not of what the ticket asserts. + * + * So the ticket now also carries a random, unguessable `ticket` string, + * returned here and threaded by the caller into the SAME `signUpEmail` + * call's body under {@link OPERATOR_PROVISIONING_TICKET_FIELD} — see + * `AuthPlugin.maybeSeedDevAdmin`. {@link isOperatorProvisioning} requires an + * exact match on both email AND this ticket, so admission now asks "did + * THIS process's own boot command make THIS exact call" rather than "does + * the address match" — a stranger's request, however precisely it times + * the window, carries no value that was ever transmitted anywhere for them + * to replay. Not a stronger *guess* — a different question. + */ + stageOperatorProvisioning(email: string): string { this.prunePendingOperatorProvisioning(); - this.pendingOperatorProvisioning.set(email.trim().toLowerCase(), { stagedAtMs: Date.now() }); + const ticket = this.generateOperatorProvisioningTicket(); + this.pendingOperatorProvisioning.set(email.trim().toLowerCase(), { stagedAtMs: Date.now(), ticket }); + return ticket; } /** [#14157] Drop the ticket staged by {@link stageOperatorProvisioning}. */ @@ -4240,6 +4309,20 @@ export class AuthManager { this.pendingOperatorProvisioning.delete(email.trim().toLowerCase()); } + /** + * [#14373] 128 bits from WebCrypto, hex-encoded — same `getRandomValues` + * baseline `resolvePasswordHasher`'s WebContainer salt uses, so this stays + * portable to hosts without `node:crypto`. Never persisted, never logged, + * never sent anywhere but the one in-process `signUpEmail` body call that + * consumes it. + */ + private generateOperatorProvisioningTicket(): string { + const bytes = (globalThis as any).crypto.getRandomValues(new Uint8Array(16)); + let hex = ''; + for (let i = 0; i < bytes.length; i++) hex += bytes[i].toString(16).padStart(2, '0'); + return hex; + } + /** * [#14157] Is this address being provisioned by the deployment's own boot * command? A PEEK, not a consume: both admission seams (the `disableSignUp` @@ -4247,11 +4330,19 @@ export class AuthManager { * creation, so a one-shot read here would admit at the first seam and refuse * at the second. The ticket's lifetime is bounded by its owner's `finally` * and by the TTL instead. + * + * [#14373] `ticket` is REQUIRED to match the value {@link + * stageOperatorProvisioning} returned — see that method's doc for why the + * address alone is no longer sufficient. A missing, wrong-typed, or + * mismatched ticket reads as "not provisioning", the same as no ticket at + * all; there is no email-only fallback path. */ - isOperatorProvisioning(email: unknown): boolean { + isOperatorProvisioning(email: unknown, ticket?: unknown): boolean { if (typeof email !== 'string' || email.trim() === '') return false; this.prunePendingOperatorProvisioning(); - return this.pendingOperatorProvisioning.has(email.trim().toLowerCase()); + const entry = this.pendingOperatorProvisioning.get(email.trim().toLowerCase()); + if (!entry) return false; + return typeof ticket === 'string' && ticket === entry.ticket; } private prunePendingOperatorProvisioning(): void { diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 6664bcb228..fcd095b0ca 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -1871,9 +1871,25 @@ export class AuthPlugin implements Plugin { // accounts. The ticket says what this creation IS (the deployment's own // boot command provisioning its admin — the operator class) for exactly // this address, and is cleared whatever happens. - this.authManager.stageOperatorProvisioning(email); + // + // [#14373] The address alone is not the ticket — it is the documented + // default and the boot banner prints it, so it does not narrow a + // concurrent stranger's guess. `stageOperatorProvisioning` also returns + // a random value that exists only in this process's memory; threading + // it into THIS SAME call's body (under `AuthManager + // .OPERATOR_PROVISIONING_TICKET_FIELD`) is what a stranger's own + // concurrent sign-up for this address cannot replay, however it times + // the window — see that method's doc for the full argument. + const provisioningTicket = this.authManager.stageOperatorProvisioning(email); try { - await api.signUpEmail({ body: { email, password, name } }); + await api.signUpEmail({ + body: { + email, + password, + name, + [AuthManager.OPERATOR_PROVISIONING_TICKET_FIELD]: provisioningTicket, + }, + }); } finally { this.authManager.clearOperatorProvisioning(email); } diff --git a/packages/plugins/plugin-auth/src/dev-admin-seed-credential-gate.test.ts b/packages/plugins/plugin-auth/src/dev-admin-seed-credential-gate.test.ts index 15774a1b22..59fdb2d720 100644 --- a/packages/plugins/plugin-auth/src/dev-admin-seed-credential-gate.test.ts +++ b/packages/plugins/plugin-auth/src/dev-admin-seed-credential-gate.test.ts @@ -484,4 +484,112 @@ describe('[#14157] the dev-admin seed gates on a LOGIN, not on user rows', () => expect(await bootstrapStatus(mountBootstrapStatus(manager))).toEqual({ hasOwner: true }); }); + + it('⑨ [#14373] binding beats timing: a stranger posting the seed address WHILE the ticket is open is still refused', async () => { + const engine = await bootEngine(); + await seedPeople(engine, 13); + const manager = makeManager(engine); + + // Open the exact window the seed's own `signUpEmail` call sits inside — + // staged directly, not via `maybeSeedDevAdmin`, so the window's OPEN for + // as long as this test needs it rather than for one async call's + // duration. This is the precondition the vulnerability needed: the + // ticket for SEED_EMAIL exists, unconsumed, right now. + const ticket = manager.stageOperatorProvisioning(SEED_EMAIL); + try { + // A stranger's own concurrent request for the SAME address, carrying + // NONE of the ticket — the only shape available to an outside caller, + // since the ticket value is generated in-process and never + // transmitted anywhere a stranger could observe or replay it. Before + // #14373 this peeked `isOperatorProvisioning(SEED_EMAIL)` as `true` on + // email alone and was admitted as the `operator` class — same as the + // seed itself. + const strangerRes = await manager.handleRequest( + new Request(`${BASE}${AUTH_BASE}/sign-up/email`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE }, + body: JSON.stringify({ + email: SEED_EMAIL, + password: 'Attacker!Passw0rd-14373', + name: 'Attacker', + }), + }), + ); + expect(strangerRes.status, `stranger sign-up: ${await strangerRes.clone().text()}`).toBe(403); + expect(((await strangerRes.json()) as { code?: string }).code).toBe(SELF_REGISTRATION_CLOSED); + expect( + await readRows(engine, 'sys_account'), + 'the ticket window must not have let the stranger create ANY account', + ).toEqual([]); + + // POSITIVE CONTROL, same still-open window: the correctly-bound call + // IS admitted — the fix narrows admission to the ticket holder, it + // does not also break the seed's own path. This calls `signUpEmail` + // directly (as `maybeSeedDevAdmin` does), not the seed's OWN separate + // post-creation `email_verified` write (#11343, a step in + // `auth-plugin.ts` outside the admission gate this card touches), so + // admission is what this proves — creation, and that the credential + // actually authenticates. + const api = (await manager.getApi()) as unknown as { + signUpEmail(input: { body: Record }): Promise; + }; + await api.signUpEmail({ + body: { + email: SEED_EMAIL, + password: SEED_PASSWORD, + name: 'Dev Admin', + [AuthManager.OPERATOR_PROVISIONING_TICKET_FIELD]: ticket, + }, + }); + const accounts = await readRows(engine, 'sys_account'); + expect(accounts.map((a) => a.provider_id)).toEqual(['credential']); + const seeded = (await readRows(engine, 'sys_user')).find( + (u) => String(u.email).toLowerCase() === SEED_EMAIL, + ); + expect(seeded, 'the correctly-bound call must have created the seed address').toBeTruthy(); + const signInRes = await manager.handleRequest( + new Request(`${BASE}${AUTH_BASE}/sign-in/email`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE }, + body: JSON.stringify({ email: SEED_EMAIL, password: SEED_PASSWORD }), + }), + ); + expect( + signInRes.status, + `the correctly-bound credential must actually authenticate: ${await signInRes.clone().text()}`, + ).toBeLessThan(300); + } finally { + manager.clearOperatorProvisioning(SEED_EMAIL); + } + }); + + it('⑨ (b) [#14373] a wrong ticket value is refused exactly like no ticket at all', async () => { + const engine = await bootEngine(); + await seedPeople(engine, 13); + const manager = makeManager(engine); + + manager.stageOperatorProvisioning(SEED_EMAIL); + try { + const res = await manager.handleRequest( + new Request(`${BASE}${AUTH_BASE}/sign-up/email`, { + method: 'POST', + headers: { 'content-type': 'application/json', origin: BASE }, + body: JSON.stringify({ + email: SEED_EMAIL, + password: 'Attacker!Passw0rd-14373', + name: 'Attacker', + // A guess at the field's shape — the wrong VALUE, not a missing + // field. The credential is the exact random value, not merely + // knowledge of the field's name (which is visible in this very + // source file). + [AuthManager.OPERATOR_PROVISIONING_TICKET_FIELD]: 'guessed-ticket-value', + }), + }), + ); + expect(res.status).toBe(403); + expect(((await res.json()) as { code?: string }).code).toBe(SELF_REGISTRATION_CLOSED); + } finally { + manager.clearOperatorProvisioning(SEED_EMAIL); + } + }); });