diff --git a/.changeset/auth-invalid-membership-policy-outcome.md b/.changeset/auth-invalid-membership-policy-outcome.md new file mode 100644 index 0000000000..f2e2a93ef3 --- /dev/null +++ b/.changeset/auth-invalid-membership-policy-outcome.md @@ -0,0 +1,38 @@ +--- +"@objectstack/plugin-auth": minor +--- + +fix(auth): an unrecognised membership policy is refused by both reconcilers, not auto-bound by one of them (#5205) + +**The sign-up path used to bind anyway.** `reconcileMembership` and +`backfillMemberships` — both public exports of `@objectstack/plugin-auth` — read +the same `policy` field and judged it with opposite predicates. Sign-up tested +`policy === 'invite-only'`, so any *other* value fell through to the `auto` +branch and auto-bound the new user; the backfill tested `policy !== 'auto'` and +refused. One input, two opposite postures, and the fail-open half was the one +that runs per sign-up. A caller who wrote `'inviteOnly'` — or any host passing +the policy from JavaScript, past the `MembershipPolicy` type — got auto-binding +while believing they had switched it off, with nothing in the logs to say so. + +Both entry points now check `isMembershipPolicy()` before any policy semantics +and refuse: nothing is bound, and the refusal names the offending value at +`error` level (and on the returned result, so it survives a caller that passed +no logger). This is the posture #5152 took one layer up at the settings +boundary — an unrecognised value is rejected loudly, never coerced to `auto`. + +**Contract change — `ReconcileOutcome` gains `'invalid-policy'`, and +`BackfillMembershipsResult.reason` gains the same member.** Both are exported +types, so a consumer that switches exhaustively over them (a `never`-checked +`default`, or a `Record< ReconcileOutcome, … >`) must handle the new member. +The new verdict is deliberately *not* a reuse of the existing `policy-skip` / +`'policy'`: those mean "a valid policy said no", and reporting them for "this +is not a policy" sends whoever is debugging a missing bind to inspect a +deployment setting that is fine. `BackfillMembershipsResult` also gains an +optional `error?: string`, and the `logger` shape on `ReconcileMembershipDeps` +gains an optional `error?` method (it falls back to `warn`). + +**No behaviour change for the two real policies.** `auto` binds and +`invite-only` skips exactly as before, on both paths — the framework's own +callers resolve the policy through `AuthManager.getMembershipPolicy()`, whose +return type is `MembershipPolicy`, so nothing on a supported path can reach the +new branch. This closes the dormant divergence on the export surface. diff --git a/packages/plugins/plugin-auth/src/reconcile-membership.test.ts b/packages/plugins/plugin-auth/src/reconcile-membership.test.ts index 1c491fde17..fb5212b2d2 100644 --- a/packages/plugins/plugin-auth/src/reconcile-membership.test.ts +++ b/packages/plugins/plugin-auth/src/reconcile-membership.test.ts @@ -1,9 +1,23 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, vi } from 'vitest'; -import { reconcileMembership, backfillMemberships } from './reconcile-membership.js'; +import { + reconcileMembership, + backfillMemberships, + MEMBERSHIP_POLICIES, + type MembershipPolicy, +} from './reconcile-membership.js'; import { runAttributedToUser } from './auth-actor-attribution.js'; +/** + * Off-vocabulary policy values, as a JS caller or a host stack could pass them + * past the `MembershipPolicy` type. `reconcileMembership` and + * `backfillMemberships` are public exports of `@objectstack/plugin-auth`, so + * the cast is the test reproducing a real call, not defeating the type system + * for convenience. + */ +const asPolicy = (value: unknown) => value as MembershipPolicy; + /** * In-memory engine over sys_member (+ optional sys_user) with the find/insert * surface the reconciler uses. `find` honors `user_id` / `organization_id` @@ -160,6 +174,206 @@ describe('backfillMemberships', () => { }); }); +/** + * [#5205] The two entry points read the SAME policy field and used to judge it + * with opposite predicates: `reconcileMembership` tested `=== 'invite-only'` + * (so anything else, including a typo, fell through to the `auto` branch and + * bound — fail-open), `backfillMemberships` tested `!== 'auto'` (refused — + * fail-safe). The dangerous half was the sign-up path: a caller who believed + * they had turned auto-binding off got it anyway, silently. + * + * These pin the fixed contract: one input, one direction (nothing bound), and + * a verdict that says *why* — `'invalid-policy'`, not a `policy-skip` / + * `'policy'` that would send a reader to inspect a setting that is fine. + */ +describe('reconcileMembership / backfillMemberships — off-vocabulary policy (#5205)', () => { + const INVALID: Array<[label: string, value: unknown]> = [ + ['camelCase typo', 'inviteOnly'], + ['snake_case typo', 'invite_only'], + ['wrong case', 'Auto'], + ['empty string', ''], + ['undefined', undefined], + ['null', null], + ['boolean', false], + ['object', { policy: 'invite-only' }], + ]; + + describe.each(INVALID)('policy = %s', (_label, value) => { + it('reconcileMembership refuses — invalid-policy, nothing bound', async () => { + const engine = makeEngine(); + const resolveTargetOrg = vi.fn(async () => 'org_default'); + const res = await reconcileMembership(engine, 'user-1', { + policy: asPolicy(value), + resolveTargetOrg, + }); + expect(res.outcome).toBe('invalid-policy'); + // The whole bug: this used to be `bound`. + expect(engine.insert).not.toHaveBeenCalled(); + expect(engine._members).toHaveLength(0); + // Refused at the entry — no org was even resolved. + expect(resolveTargetOrg).not.toHaveBeenCalled(); + }); + + it('backfillMemberships refuses — invalid-policy, nothing bound', async () => { + const engine = makeEngine({ users: [{ id: 'u1' }, { id: 'u2' }] }); + const resolveTargetOrg = vi.fn(async () => 'org_default'); + const res = await backfillMemberships(engine, { + policy: asPolicy(value), + resolveTargetOrg, + }); + expect(res.reason).toBe('invalid-policy'); + expect(res.bound).toBe(0); + expect(engine.insert).not.toHaveBeenCalled(); + expect(resolveTargetOrg).not.toHaveBeenCalled(); + }); + + it('both paths agree: same input, same direction, neither binds', async () => { + const signupEngine = makeEngine(); + const backfillEngine = makeEngine({ users: [{ id: 'u1' }] }); + const deps = { policy: asPolicy(value), resolveTargetOrg: async () => 'org_default' }; + + const signup = await reconcileMembership(signupEngine, 'user-1', deps); + const backfill = await backfillMemberships(backfillEngine, deps); + + expect(signupEngine._members).toHaveLength(0); + expect(backfillEngine._members).toHaveLength(0); + // …and both name the same cause, so a reader of either one lands on the + // caller's policy value rather than on the deployment's setting. + expect(signup.outcome).toBe('invalid-policy'); + expect(backfill.reason).toBe('invalid-policy'); + expect(signup.outcome).not.toBe('policy-skip'); + expect(backfill.reason).not.toBe('policy'); + }); + }); + + it('names the offending value so it is not swallowed — logged and returned', async () => { + const error = vi.fn(); + const res = await reconcileMembership(makeEngine(), 'user-1', { + policy: asPolicy('inviteOnly'), + resolveTargetOrg: async () => 'org_default', + logger: { error }, + }); + // Returned, so the diagnosis survives a caller that passed no logger. + expect(res.error).toContain(`'inviteOnly'`); + expect(res.error).toContain('invite-only'); // the expected vocabulary + expect(error).toHaveBeenCalledTimes(1); + const [msg, meta] = error.mock.calls[0]; + expect(msg).toContain(`'inviteOnly'`); + expect(meta).toMatchObject({ userId: 'user-1', expected: MEMBERSHIP_POLICIES }); + }); + + it('logs at error, not warn — nothing else looks broken afterwards', async () => { + const error = vi.fn(); + const warn = vi.fn(); + await backfillMemberships(makeEngine({ users: [{ id: 'u1' }] }), { + policy: asPolicy('inviteOnly'), + resolveTargetOrg: async () => 'org_default', + logger: { error, warn }, + }); + expect(error).toHaveBeenCalledTimes(1); + expect(warn).not.toHaveBeenCalled(); + }); + + it('still reaches a logger that only has warn', async () => { + const warn = vi.fn(); + await reconcileMembership(makeEngine(), 'user-1', { + policy: asPolicy('inviteOnly'), + resolveTargetOrg: async () => 'org_default', + logger: { warn }, + }); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain(`'inviteOnly'`); + }); + + it('refuses without a logger at all (the refusal is not a logging side effect)', async () => { + const engine = makeEngine(); + const res = await reconcileMembership(engine, 'user-1', { + policy: asPolicy('inviteOnly'), + resolveTargetOrg: async () => 'org_default', + }); + expect(res.outcome).toBe('invalid-policy'); + expect(engine.insert).not.toHaveBeenCalled(); + }); + + it('does not echo a non-string value into the log — typeof only', async () => { + const error = vi.fn(); + const res = await reconcileMembership(makeEngine(), 'user-1', { + // A caller that passed the whole config object by mistake; it may carry + // fields that have no business in a log line. + policy: asPolicy({ membershipPolicy: 'invite-only', adminEmail: 'ops@example.com' }), + resolveTargetOrg: async () => 'org_default', + logger: { error }, + }); + expect(res.outcome).toBe('invalid-policy'); + expect(res.error).toContain('[object]'); + expect(error.mock.calls[0][0]).not.toContain('ops@example.com'); + expect(JSON.stringify(error.mock.calls[0][1])).not.toContain('ops@example.com'); + }); + + it('bounds a long string value', async () => { + const error = vi.fn(); + const res = await reconcileMembership(makeEngine(), 'user-1', { + policy: asPolicy('x'.repeat(500)), + resolveTargetOrg: async () => 'org_default', + logger: { error }, + }); + expect(res.error).toContain('(truncated)'); + expect(res.error!.length).toBeLessThan(200); + }); + + it('preconditions still win — a missing engine reports skipped, not invalid-policy', async () => { + const res = await reconcileMembership(undefined, 'user-1', { + policy: asPolicy('inviteOnly'), + resolveTargetOrg: async () => 'org_default', + }); + expect(res.outcome).toBe('skipped'); + const bf = await backfillMemberships(undefined, { + policy: asPolicy('inviteOnly'), + resolveTargetOrg: async () => 'org_default', + }); + expect(bf.reason).toBe('engine-unavailable'); + }); +}); + +/** + * [#5205] The guard must not move the ground under the two values that ARE + * policies. Asserted as a table over the declared vocabulary, so the behaviour + * of every legal value is stated here rather than inferred from a green suite. + */ +describe('legal policies are unaffected by the invalid-policy guard (#5205)', () => { + const EXPECTED: Record = { + auto: { signup: 'bound', backfillReason: undefined, binds: true }, + 'invite-only': { signup: 'policy-skip', backfillReason: 'policy', binds: false }, + }; + + it('covers the whole declared vocabulary', () => { + expect(Object.keys(EXPECTED).sort()).toEqual([...MEMBERSHIP_POLICIES].sort()); + }); + + it.each([...MEMBERSHIP_POLICIES])('reconcileMembership(%s) is unchanged', async (policy) => { + const engine = makeEngine(); + const res = await reconcileMembership(engine, 'user-1', { + policy, + resolveTargetOrg: async () => 'org_default', + }); + expect(res.outcome).toBe(EXPECTED[policy].signup); + expect(res.error).toBeUndefined(); + expect(engine._members).toHaveLength(EXPECTED[policy].binds ? 1 : 0); + }); + + it.each([...MEMBERSHIP_POLICIES])('backfillMemberships(%s) is unchanged', async (policy) => { + const engine = makeEngine({ users: [{ id: 'u1' }] }); + const res = await backfillMemberships(engine, { + policy, + resolveTargetOrg: async () => 'org_default', + }); + expect(res.reason).toBe(EXPECTED[policy].backfillReason); + expect(res.error).toBeUndefined(); + expect(res.bound).toBe(EXPECTED[policy].binds ? 1 : 0); + expect(engine._members).toHaveLength(EXPECTED[policy].binds ? 1 : 0); + }); +}); + /** * [#4586] The reconciler bind is the third `sys_member` writer (after the * better-auth adapter and the invite-accept path), and it runs INSIDE diff --git a/packages/plugins/plugin-auth/src/reconcile-membership.ts b/packages/plugins/plugin-auth/src/reconcile-membership.ts index b452843914..af10cc5a84 100644 --- a/packages/plugins/plugin-auth/src/reconcile-membership.ts +++ b/packages/plugins/plugin-auth/src/reconcile-membership.ts @@ -18,12 +18,33 @@ * the user — e.g. the cloud's personal-org provisioning — wins, and there * is never a double membership); * - honors the deployment's `membershipPolicy` (`auto` binds; `invite-only` - * never auto-binds); + * never auto-binds; anything else is REFUSED, see below); * - binds only to an unambiguous target org (single-org's default org; * `multi` mode returns none — invite / JIT own membership there); * - is idempotent (keyed on the `(organization_id, user_id)` unique index) * and never throws (a failed bind must not fail user creation — the * kernel:ready backfill is the self-healing net). + * + * ## Both entry points refuse an off-vocabulary policy (#5205) + * + * `reconcileMembership` and `backfillMemberships` are public exports of + * `@objectstack/plugin-auth`, so a JavaScript caller or a host stack can hand + * either one a policy the `MembershipPolicy` type would have rejected. They + * used to disagree about what that means: the sign-up reconciler tested + * `policy === 'invite-only'`, so a typo'd `'inviteOnly'` fell through to the + * `auto` branch and **auto-bound anyway** (fail-open — a caller who believed + * they had switched auto-binding off got it silently), while the backfill + * tested `policy !== 'auto'` and refused (fail-safe). One input, two opposite + * postures, and the dangerous one was on the per-sign-up path. + * + * Both now check {@link isMembershipPolicy} at the entry and return a distinct + * `'invalid-policy'` outcome/reason naming the offending value. It is a + * separate verdict on purpose rather than a reuse of `policy-skip`: reporting + * "skipped by policy" for "your policy is not a policy" sends whoever is + * debugging the auto-bind to read the deployment's setting, which is fine, and + * find it looking exactly as they left it, which is not. This matches the + * posture #5152 took one layer up at the settings boundary — an unrecognised + * value is rejected loudly, never coerced to `auto`. */ import { authSystemWriteContext } from './auth-actor-attribution.js'; @@ -55,6 +76,12 @@ export type ReconcileOutcome = | 'yielded' /** `membershipPolicy: 'invite-only'` — auto-bind is off by policy. */ | 'policy-skip' + /** + * The policy was not one of {@link MEMBERSHIP_POLICIES} — refused, nothing + * bound (#5205). Distinct from `policy-skip`, which means a *valid* policy + * said no; this one means the caller's policy value is unusable. + */ + | 'invalid-policy' /** No unambiguous target org (multi mode, or single mode not bootstrapped). */ | 'no-target-org' /** An error occurred and was swallowed (never fails user creation). */ @@ -63,7 +90,13 @@ export type ReconcileOutcome = | 'skipped'; export interface ReconcileMembershipDeps { - /** Deployment membership policy. Default `'auto'` at the call site. */ + /** + * Deployment membership policy. Default `'auto'` at the call site. + * + * Checked at runtime as well as declared: these functions are exported, so a + * JS caller can pass anything. Anything outside {@link MEMBERSHIP_POLICIES} + * is refused (`'invalid-policy'`), never coerced to `auto`. + */ policy: MembershipPolicy; /** * Resolve the organization to bind the user to. Single-org → the default org; @@ -71,11 +104,57 @@ export interface ReconcileMembershipDeps { * `tenancy.defaultOrgId`. */ resolveTargetOrg: () => Promise; - logger?: { info?: (msg: string, meta?: any) => void; warn?: (msg: string, meta?: any) => void }; + logger?: { + info?: (msg: string, meta?: any) => void; + warn?: (msg: string, meta?: any) => void; + error?: (msg: string, meta?: any) => void; + }; } const SYSTEM_CTX = { isSystem: true }; +/** + * Describe a rejected policy value for whoever reads the log, without trusting + * it. The declared vocabulary is two short literals, so echoing a *string* + * back is the entire point of the message — it is how the reader sees + * `'inviteOnly'` where they expected `'invite-only'`. Anything else is a value + * that arrived off the type contract, i.e. arbitrary caller data, and a log + * line is the wrong place to find out it was an object carrying user fields: + * those report as their `typeof` only. Strings are bounded for the same + * reason — a policy is never 64 characters long, so anything longer is not a + * typo and does not need to be reproduced in full to be diagnosed. + */ +function describePolicy(value: unknown): string { + if (typeof value === 'string') { + return value.length > 64 ? `'${value.slice(0, 64)}…' (truncated)` : `'${value}'`; + } + if (value === null) return 'null'; + if (value === undefined) return 'undefined'; + return `[${typeof value}]`; +} + +/** + * Refuse an off-vocabulary policy the same way from both entry points (#5205): + * one message, one log level, one returned description. + * + * `error`, not `warn`, for the reason #5152 gives at the settings boundary — + * nothing looks broken afterwards. The bind simply does not happen, and if + * this passed quietly the deployment would either keep auto-binding while an + * operator believed it had stopped (the old sign-up behaviour) or stop binding + * with no stated cause. Falls back to `warn` only because `error` is optional + * on the logger shape and a caller passing a two-method logger should still + * hear about it; the message is identical either way. + */ +function refuseInvalidPolicy(deps: ReconcileMembershipDeps, meta: Record): string { + const described = describePolicy(deps.policy as unknown); + const message = + `[membership] refusing to bind: membership policy ${described} is not a recognized value ` + + `— expected one of: ${MEMBERSHIP_POLICIES.join(', ')}`; + const log = deps.logger?.error ?? deps.logger?.warn; + log?.(message, { ...meta, policy: described, expected: MEMBERSHIP_POLICIES }); + return message; +} + function genMemberId(): string { const rand = Math.random().toString(36).slice(2, 10); const ts = Date.now().toString(36); @@ -121,10 +200,17 @@ export async function reconcileMembership( engine: any, userId: string | undefined, deps: ReconcileMembershipDeps, -): Promise<{ outcome: ReconcileOutcome; organizationId?: string }> { +): Promise<{ outcome: ReconcileOutcome; organizationId?: string; error?: string }> { if (!engine || typeof engine.find !== 'function' || typeof engine.insert !== 'function' || !userId) { return { outcome: 'skipped' }; } + // #5205 — before any policy semantics, is this a policy at all? Testing + // `=== 'invite-only'` alone let every other value fall through to the `auto` + // branch and bind. The check is not dead code: the type says + // `MembershipPolicy`, the export surface says a JS caller can say otherwise. + if (!isMembershipPolicy(deps.policy)) { + return { outcome: 'invalid-policy', error: refuseInvalidPolicy(deps, { userId }) }; + } if (deps.policy === 'invite-only') { return { outcome: 'policy-skip' }; } @@ -170,7 +256,13 @@ export interface BackfillMembershipsResult { scanned: number; bound: number; skipped: number; - reason?: 'policy' | 'no-target-org' | 'engine-unavailable'; + reason?: 'policy' | 'invalid-policy' | 'no-target-org' | 'engine-unavailable'; + /** + * Present with `reason: 'invalid-policy'` — the refusal message, naming the + * offending value. Carried on the result, not only logged, so the diagnosis + * survives a caller that passed no logger (#5205). + */ + error?: string; } /** @@ -190,6 +282,12 @@ export async function backfillMemberships( if (!engine || typeof engine.find !== 'function' || typeof engine.insert !== 'function') { return { ...summary, reason: 'engine-unavailable' }; } + // #5205 — same order, same verdict as the sign-up path. This one already + // refused an off-vocabulary value, but reported it as `'policy'`, which + // reads as "the deployment's policy said no" and hides a caller bug. + if (!isMembershipPolicy(deps.policy)) { + return { ...summary, reason: 'invalid-policy', error: refuseInvalidPolicy(deps, { pass: 'backfill' }) }; + } if (deps.policy !== 'auto') { return { ...summary, reason: 'policy' }; }