History

Bind the self-host admin gate to the instance org

Both admin planes authorized via getActiveMember, which resolves the caller's role in session.activeOrganizationId — a caller-controlled field. Any member could create an org of their own, become its owner, set it active, and pass the gate, while the reads stayed scoped to the instance org: the whole user directory on one plane, and an admin invite code on the other. Extract one requireInstanceAdmin that resolves the role against the instance's own organization id, and apply it to both planes. Also close org creation on a single-org instance as defense in depth; the seed creates its org session-lessly and is unaffected.

Rhys Sullivan committed 3 months ago

Commit b79282a1 parent 1f1b2275 files · +361 −30Browse files

Unified · Split

5 files
  1. apps/host-selfhost/src/admin/admin-escalation.node.test.ts +184 −0
  2. apps/host-selfhost/src/admin/admin-users-api.ts +17 −15
  3. apps/host-selfhost/src/admin/handlers.ts +14 −14
  4. apps/host-selfhost/src/admin/require-admin.ts +126 −0
  5. apps/host-selfhost/src/auth/better-auth.ts +20 −1
apps/host-selfhost/src/admin/admin-escalation.node.test.ts+184−0
@@ -0,0 +1,184 @@
1+import { mkdtempSync } from "node:fs";
2+import { tmpdir } from "node:os";
3+import { join } from "node:path";
4+
5+import { afterAll, expect, test } from "@effect/vitest";
6+
7+import { mintInviteCode } from "../testing/mint-invite";
8+
9+// ---------------------------------------------------------------------------
10+// THE ESCALATION, performed rather than asserted about.
11+//
12+// `admin-users.node.test.ts` already asserts a plain member gets a 403, and it
13+// PASSED while the instance was fully bypassable — because it only ever asked
14+// the question one way. A member who simply calls the admin plane is refused;
15+// a member who first arranges to be an owner OF SOMETHING is a different
16+// caller, and the old gate could not tell the difference: it read the role from
17+// `session.activeOrganizationId`, which the caller controls, while the reads
18+// underneath stayed scoped to the instance's boot-seeded org.
19+//
20+// So this file does not test the label ("members are refused"). It runs the
21+// actual attack an invited member can run with nothing but their own session
22+// and the auth routes Better Auth mounts:
23+//
24+// 1. POST /api/auth/organization/create → become `owner` of a new org
25+// 2. POST /api/auth/organization/set-active → point the session at it
26+// 3. call the admin planes → must STILL be refused
27+//
28+// and asserts the refusal survives every step, on BOTH planes. The users plane
29+// leaks the instance's whole user directory and connection inventory; the
30+// invite plane is worse, because a `role: "admin"` invite code is durable
31+// escalation that outlives the session. Step 1 is now refused outright
32+// (`allowUserToCreateOrganization: false`), so the test asserts what it can at
33+// each step and never lets a blocked earlier step silently excuse a later one —
34+// steps 2 and 3 run regardless of whether step 1 succeeded.
35+//
36+// MUTATION-CHECKED, both directions:
37+// - gate reverted to a session-scoped role lookup + org creation re-opened:
38+// FAILS with "/api/admin/users refuses the escalated member: expected 200
39+// to be 403". The escalation really is reachable, and this test really does
40+// catch it.
41+// - gate fixed, org creation re-opened: every admin-plane assertion above
42+// PASSES and only the final defense-in-depth line fails. That is the
43+// evidence that the gate — not the flag — is what closes the hole.
44+// ---------------------------------------------------------------------------
45+
46+process.env.EXECUTOR_DATA_DIR = mkdtempSync(join(tmpdir(), "eh-admin-escalation-"));
47+process.env.BETTER_AUTH_SECRET = "admin-escalation-secret-0123456789-abcdefg";
48+process.env.EXECUTOR_BOOTSTRAP_ADMIN_EMAIL = "admin@escalation.test";
49+process.env.EXECUTOR_BOOTSTRAP_ADMIN_PASSWORD = "admin-pass-123456";
50+
51+const { makeSelfHostApiHandler } = await import("../app");
52+const { handler, dispose } = await makeSelfHostApiHandler();
53+afterAll(() => dispose());
54+
55+const BASE = "http://localhost:4788";
56+
57+const asMember = (token: string) => ({ authorization: `Bearer ${token}` });
58+
59+const post = (path: string, token: string, body: unknown) =>
60+ handler(
61+ new Request(`${BASE}${path}`, {
62+ method: "POST",
63+ headers: { "content-type": "application/json", ...asMember(token) },
64+ body: JSON.stringify(body),
65+ }),
66+ );
67+
68+const get = (path: string, token: string) =>
69+ handler(new Request(`${BASE}${path}`, { headers: asMember(token) }));
70+
71+/** Join the instance for real: mint an invite as the bootstrap admin, redeem it.
72+ * This is a legitimate, fully authenticated member of the instance org — the
73+ * weakest principal that has any standing at all, which is precisely the
74+ * attacker this gate has to hold against. */
75+const joinAsMember = async (email: string): Promise<string> => {
76+ const inviteCode = await mintInviteCode(handler);
77+ const signUp = await handler(
78+ new Request(`${BASE}/api/auth/sign-up/email`, {
79+ method: "POST",
80+ headers: { "content-type": "application/json" },
81+ body: JSON.stringify({ email, password: "password-12345678", name: "Member", inviteCode }),
82+ }),
83+ );
84+ expect(signUp.status, "the member joined through the real invite flow").toBe(200);
85+ const token = signUp.headers.get("set-auth-token") ?? "";
86+ expect(token).not.toBe("");
87+ return token;
88+};
89+
90+test("a member cannot escalate by owning an organization of their own", async () => {
91+ const memberToken = await joinAsMember("climber@escalation.test");
92+
93+ // Baseline: the ordinary refusal the old test already covered. If this were
94+ // ever a 200 the rest of the file would be meaningless, so it is asserted
95+ // first rather than assumed.
96+ expect((await get("/api/admin/users", memberToken)).status, "a plain member is refused").toBe(
97+ 403,
98+ );
99+
100+ // -- Step 1: become an owner. ---------------------------------------------
101+ // With `allowUserToCreateOrganization: false` this is refused outright. Its
102+ // status is RECORDED, not asserted here: the security property this test
103+ // exists for is "the admin plane stays shut", and asserting the block at step
104+ // 1 would abort the run before step 3 ever asserts it. So the attack proceeds
105+ // as far as it can either way, the planes are checked, and only then is the
106+ // defense-in-depth flag asserted at the bottom.
107+ const created = await post("/api/auth/organization/create", memberToken, {
108+ name: "Climber Org",
109+ slug: "climber-org",
110+ });
111+ const createdOrgId = created.ok ? ((await created.json()) as { id?: string }).id : undefined;
112+
113+ // -- Step 2: point the session at an org the caller owns. ------------------
114+ // The cheaper repeat path. Attempted against the created org when step 1
115+ // somehow succeeded, and unconditionally against the slug either way.
116+ if (createdOrgId) {
117+ await post("/api/auth/organization/set-active", memberToken, {
118+ organizationId: createdOrgId,
119+ });
120+ }
121+ await post("/api/auth/organization/set-active", memberToken, {
122+ organizationSlug: "climber-org",
123+ });
124+
125+ // -- Step 3: the planes must still refuse. ---------------------------------
126+ // Every admin users route, not just the list: a gate applied at four call
127+ // sites can be fixed at three.
128+ for (const path of [
129+ "/api/admin/users",
130+ "/api/admin/users/with-connections",
131+ "/api/admin/users/user_anyone/connections",
132+ "/api/admin/users/user_anyone",
133+ `/api/admin/users/${encodeURIComponent("admin@escalation.test")}`,
134+ ]) {
135+ const response = await get(path, memberToken);
136+ expect(response.status, `${path} refuses the escalated member`).toBe(403);
137+ // A refusal that leaked the answer in its body would be no refusal at all.
138+ expect(await response.text(), `${path} discloses nothing`).not.toContain(
139+ "admin@escalation.test",
140+ );
141+ }
142+
143+ // The invite plane shares the gate, and a bypass there is DURABLE: a
144+ // `role: "admin"` code redeems into a real admin membership that outlives
145+ // this session. Read and write are both checked — listing invite codes is
146+ // itself a disclosure (a live code is a credential).
147+ expect((await get("/api/admin/invites", memberToken)).status, "invite list refused").toBe(403);
148+
149+ const minted = await post("/api/admin/invites", memberToken, { role: "admin" });
150+ expect(minted.status, "an admin invite cannot be minted by a member").toBe(403);
151+ expect(await minted.text(), "and no code came back").not.toContain("code");
152+
153+ const revoked = await handler(
154+ new Request(`${BASE}/api/admin/invites/some-invite-id`, {
155+ method: "DELETE",
156+ headers: asMember(memberToken),
157+ }),
158+ );
159+ expect(revoked.status, "revoking someone else's invite is refused too").toBe(403);
160+
161+ // The instance's real admin is unaffected — the gate closed the hole, it did
162+ // not close the door. Without this, every assertion above would also pass if
163+ // the admin plane were simply broken for everyone.
164+ const adminList = await get("/api/admin/users", await signInAdmin());
165+ expect(adminList.status, "the real instance admin still gets in").toBe(200);
166+
167+ // Defense in depth, asserted LAST so it can never mask the assertions above:
168+ // step 1 of the attack is itself closed on a single-org instance.
169+ expect(created.status, "org creation is closed on a single-org instance").not.toBe(200);
170+});
171+
172+const signInAdmin = async (): Promise<string> => {
173+ const response = await handler(
174+ new Request(`${BASE}/api/auth/sign-in/email`, {
175+ method: "POST",
176+ headers: { "content-type": "application/json" },
177+ body: JSON.stringify({
178+ email: process.env.EXECUTOR_BOOTSTRAP_ADMIN_EMAIL,
179+ password: process.env.EXECUTOR_BOOTSTRAP_ADMIN_PASSWORD,
180+ }),
181+ }),
182+ );
183+ return response.headers.get("set-auth-token") ?? "";
184+};
apps/host-selfhost/src/admin/admin-users-api.ts+17−15
@@ -3,9 +3,12 @@ · 2 unmodified lines
33 // backed by a Better-Auth-authorized platform view, mounted at
44 // `/api/admin/users*` beside the existing invite-code admin routes.
55 //
6−// AUTH: an owner/admin member of the single org, resolved through the org
7−// primitive's `getActiveMember` — the SAME gate the invite-code admin API uses
8−// (`admin/handlers.ts`), not a new permission concept. Self-host has no
6+// AUTH: an owner/admin member of the single org, resolved by the shared
7+// `requireInstanceAdmin` — the SAME gate the invite-code admin API uses
8+// (`admin/handlers.ts`), not a new permission concept. Membership is resolved
9+// against the INSTANCE's organization id, never the caller's
10+// `session.activeOrganizationId`: see require-admin.ts for the privilege
11+// escalation that distinction refuses. Self-host has no
912 // organization-OWNED api key (Better Auth keys always belong to the user who
1013 // created one), so there is no machine-credential path here; the operator's own
1114 // admin session is the credential. That asymmetry with cloud is deliberate and
@@ -46,24 +49,23 @@ import { · 34 unmodified lines
4649 import type { Executor } from "@executor-js/sdk";
4750
4851 import { BetterAuth, type BetterAuthHandle } from "../auth/better-auth";
52+import { requireInstanceAdmin } from "./require-admin";
4953 import { SelfHostDb, SelfHostDbProvider, type SelfHostDbHandle } from "../db/self-host-db";
5054 import { SelfHostHostConfig, SelfHostPluginsProvider } from "../execution";
5155
5256 /**
53− * The same owner/admin gate the invite-code admin API applies. A plain member
54− * session is refused: this plane reports every user in the instance.
57+ * The SAME gate the invite-code admin API applies — literally the same
58+ * function, not a second copy of the same idea, which is how the two planes
59+ * came to share a bypass. A plain member session is refused, and so is an owner
60+ * of some OTHER organization: this plane reports every user in the instance, so
61+ * the org it authorizes against is the instance's own (see require-admin.ts).
5562 */
5663 const requireAdmin = (headers: AdminUsersHeaders) =>
57− Effect.gen(function* () {
58− const { auth } = yield* BetterAuth;
59− const member = yield* Effect.tryPromise({
60− try: () => auth.api.getActiveMember({ headers: new Headers(headers) }),
61− catch: () => new AdminUsersError({ message: "Failed to resolve session" }),
62− }).pipe(Effect.orElseSucceed(() => null));
63− if (!member) return yield* new AdminUsersUnauthorized();
64− if (member.role !== "owner" && member.role !== "admin") return yield* new AdminUsersForbidden();
65− return member;
66− });
64+ requireInstanceAdmin(new Headers(headers)).pipe(
65+ Effect.mapError((denial) =>
66+ denial === "unauthorized" ? new AdminUsersUnauthorized() : new AdminUsersForbidden(),
67+ ),
68+ );
6769
6870 /**
6971 * Self-host's member directory: `externalId` → email/name.
apps/host-selfhost/src/admin/handlers.ts+14−14
@@ -10,6 +10,7 @@ import { · 9 unmodified lines
1010 type InviteCode as InviteCodeSchema,
1111 } from "./api";
1212 import { BetterAuth, type BetterAuthHandle } from "../auth/better-auth";
13+import { requireInstanceAdmin } from "./require-admin";
1314 import { SelfHostDb, type SelfHostDbHandle } from "../db/self-host-db";
1415 import {
1516 createInviteCode,
@@ -22,9 +23,11 @@ import { · 6 unmodified lines
2223 // ---------------------------------------------------------------------------
2324 // Handlers for the self-host admin (invite-code) API. Every Promise-returning
2425 // boundary (Better Auth, the libSQL store) is wrapped in Effect.tryPromise with
25−// a typed failure — no raw try/catch, no Promise.catch. Each route is gated:
26−// the caller must be an owner/admin member of the one org (resolved through the
27−// org primitive's getActiveMember).
26+// a typed failure — no raw try/catch, no Promise.catch. Each route is gated by
27+// the SHARED `requireInstanceAdmin`: the caller must be an owner/admin member
28+// of THIS INSTANCE'S organization, named explicitly rather than taken from the
29+// caller's session (see require-admin.ts for the escalation that rule refuses —
30+// this plane mints invite codes, so a bypass here is durable admin).
2831 // ---------------------------------------------------------------------------
2932
3033 const requestHeaders = Effect.map(
@@ -32,18 +35,15 @@ const requestHeaders = Effect.map( · 1 unmodified line
3235 (request): Headers => new Headers({ ...request.headers }),
3336 );
3437
35−// Resolve + authorize the caller, returning their member record (for userId).
38+// Resolve + authorize the caller, rendering the shared gate's denial in THIS
39+// plane's error vocabulary (the HttpApi group declares these, not the users
40+// plane's).
3641 const requireAdmin = (headers: Headers) =>
37− Effect.gen(function* () {
38− const { auth } = yield* BetterAuth;
39− const member = yield* Effect.tryPromise({
40− try: () => auth.api.getActiveMember({ headers }),
41− catch: () => new AdminError({ message: "Failed to resolve session" }),
42− }).pipe(Effect.orElseSucceed(() => null));
43− if (!member) return yield* new AdminUnauthorized();
44− if (member.role !== "owner" && member.role !== "admin") return yield* new AdminForbidden();
45− return member;
46− });
42+ requireInstanceAdmin(headers).pipe(
43+ Effect.mapError((denial) =>
44+ denial === "unauthorized" ? new AdminUnauthorized() : new AdminForbidden(),
45+ ),
46+ );
4747
4848 const narrowRole = (role: string | undefined): InviteRole =>
4949 role === "admin" ? "admin" : "member";
apps/host-selfhost/src/admin/require-admin.ts+126−0
@@ -0,0 +1,126 @@
1+// ---------------------------------------------------------------------------
2+// THE self-host admin gate. One implementation, shared by every admin plane:
3+// the invite-code API (`handlers.ts`) and the admin users API
4+// (`admin-users-api.ts`). Both planes report on, or grant power over, the whole
5+// instance, so they must agree on exactly who counts as an admin — and there is
6+// only one way to be wrong here, so there is only one place to be right.
7+//
8+// WHY THIS IS NOT `getActiveMember`.
9+//
10+// Both planes previously authorized with `auth.api.getActiveMember({ headers
11+// })`. That endpoint resolves the caller's membership in
12+// `session.activeOrganizationId` — a field the CALLER controls. Better Auth's
13+// organization plugin exposes `POST /organization/create` (mounted bare,
14+// `allowUserToCreateOrganization` defaults to true) and `POST
15+// /organization/set-active`, so any authenticated member could create an
16+// organization of their own, become its `owner` (the creator role), have that
17+// org set active, and then present a session whose active org is one they own.
18+// `getActiveMember` would answer `role: "owner"` — for THEIR org — and the gate
19+// would open. Meanwhile the reads underneath are scoped to the instance's
20+// boot-seeded org, so the escalated caller read the real instance: every user's
21+// externalId/createdAt/lastSeenAt and their whole connection inventory on one
22+// plane, and a live `role: "admin"` invite code (durable escalation) on the
23+// other.
24+//
25+// The bug is that the gate and the data disagreed about WHICH organization the
26+// request was about. This gate removes the disagreement by naming the
27+// instance's organization explicitly, so the authorization decision is made
28+// against the same org the reads are scoped to and nothing the caller sends can
29+// redirect it.
30+//
31+// HOW. `getActiveMemberRole` accepts an explicit `organizationId` query, and
32+// when given one it ignores `session.activeOrganizationId` entirely, looks up
33+// the caller's membership in THAT org, and refuses with FORBIDDEN when there
34+// isn't one (better-auth 1.6.12, `plugins/organization/routes/crud-members.mjs`
35+// — read against the installed build, not the docs). So a caller who owns ten
36+// organizations of their own still resolves to "not a member of the instance
37+// org" unless they were actually invited to it. There is no
38+// `member.organizationId !== organizationId` check to write, because the query
39+// makes the mismatch unrepresentable rather than detectable.
40+//
41+// Defense in depth lives in `auth/better-auth.ts`, which now mounts
42+// `organization({ allowUserToCreateOrganization: false })` so the first step of
43+// the escalation is refused too. Either fix alone closes the hole; the gate is
44+// the load-bearing one, because it is what makes the authorization decision
45+// correct rather than merely making one route to it harder.
46+// ---------------------------------------------------------------------------
47+
48+import { Effect } from "effect";
49+
50+import { BetterAuth } from "../auth/better-auth";
51+
52+/**
53+ * Why the caller was refused, in the vocabulary both planes share.
54+ *
55+ * The two planes render refusals through their OWN error classes
56+ * (`AdminUnauthorized` / `AdminUsersUnauthorized`, and so on), because those
57+ * are what their respective HttpApi contracts declare. So the gate decides, and
58+ * each caller translates — rather than the gate importing one plane's errors
59+ * and the other plane mapping between two error vocabularies.
60+ */
61+export type AdminGateDenial = "unauthorized" | "forbidden";
62+
63+/** What a caller who passed the gate is. `userId` is the Better Auth `user.id`
64+ * — the same id `auth/identity.ts` binds as the accountId, which is what the
65+ * invite plane records as `createdBy`. */
66+export interface InstanceAdmin {
67+ readonly userId: string;
68+ readonly role: string;
69+}
70+
71+/**
72+ * Better Auth writes a member's roles as a comma-separated list (its own
73+ * `leaveOrganization` reads them with `role.split(",")`), so a single-role
74+ * string is the common case rather than the contract. Membership in the
75+ * privileged set is therefore tested per role, not by equality on the whole
76+ * field — an `"owner,admin"` value must not read as neither.
77+ */
78+const isPrivileged = (role: string): boolean =>
79+ role
80+ .split(",")
81+ .map((part) => part.trim())
82+ .some((part) => part === "owner" || part === "admin");
83+
84+/**
85+ * Authorize the caller as an owner/admin OF THE INSTANCE'S ORGANIZATION.
86+ *
87+ * Fails with `"unauthorized"` when there is no session at all, and
88+ * `"forbidden"` when there is a session that is not an owner/admin member of
89+ * the instance org — including a session belonging to an owner of some OTHER
90+ * organization, which is the escalation this gate exists to refuse.
91+ *
92+ * Both Better Auth calls FAIL CLOSED, and closed means the less-informative
93+ * answer of the two: a `getSession` that throws is treated as "no session"
94+ * (401) and a `getActiveMemberRole` that throws is treated as "not entitled"
95+ * (403), because the endpoint's own way of saying "you are not a member of this
96+ * organization" IS a thrown FORBIDDEN. An infrastructure fault therefore
97+ * refuses the request rather than surfacing as a 500, which is the correct
98+ * trade on a plane where the wrong answer is a disclosure.
99+ *
100+ * TWO CALLS, NOT ONE. The session read is what distinguishes 401 from 403 — the
101+ * role endpoint cannot, since it refuses the anonymous and the unentitled with
102+ * the same status — and it is also where `userId` comes from, since the role
103+ * endpoint answers with a role and nothing else.
104+ */
105+export const requireInstanceAdmin = (
106+ headers: Headers,
107+): Effect.Effect<InstanceAdmin, AdminGateDenial, BetterAuth> =>
108+ Effect.gen(function* () {
109+ const { auth, organizationId } = yield* BetterAuth;
110+
111+ const session = yield* Effect.tryPromise(() => auth.api.getSession({ headers })).pipe(
112+ Effect.orElseSucceed(() => null),
113+ );
114+ if (!session) return yield* Effect.fail<AdminGateDenial>("unauthorized");
115+
116+ // The whole fix in one argument: the org is named by the INSTANCE, never
117+ // read from the caller's session.
118+ const resolved = yield* Effect.tryPromise(() =>
119+ auth.api.getActiveMemberRole({ headers, query: { organizationId } }),
120+ ).pipe(Effect.orElseSucceed(() => null));
121+ if (!resolved || !isPrivileged(resolved.role)) {
122+ return yield* Effect.fail<AdminGateDenial>("forbidden");
123+ }
124+
125+ return { userId: session.user.id, role: resolved.role };
126+ });
apps/host-selfhost/src/auth/better-auth.ts+20−1
@@ -110,7 +110,26 @@ const makeAuthOptions = (client: Client, getOrganizationId: () => string, gate?: · 109 unmodified lines
110110 // are re-emitted by the shared envelope (MCP clients probe the origin root,
111111 // not the /api/auth basePath).
112112 plugins: [
113− organization(),
113+ // SINGLE-ORG INSTANCE, ENFORCED. `allowUserToCreateOrganization: false`
114+ // closes `POST /api/auth/organization/create` to every session. Left at
115+ // its default (true) it was the first step of a privilege escalation: any
116+ // invited member could create an organization of their own, become its
117+ // `owner` (the creator role), get it set as their active org, and then
118+ // pass an admin gate that resolved the caller's role from
119+ // `session.activeOrganizationId` — reading the whole instance's user
120+ // directory and minting themselves a durable `role: "admin"` invite code.
121+ // The load-bearing fix is `admin/require-admin.ts`, which authorizes
122+ // against THIS instance's org id instead of the session's; this flag is
123+ // defense in depth, and is also just true of the product — self-host
124+ // serves exactly one organization, so a second one is never legitimate.
125+ //
126+ // The bootstrap seed is unaffected: `seed.ts` calls `createOrganization`
127+ // with `userId` and NO session, which better-auth 1.6.12 treats as
128+ // `isSystemAction` and short-circuits past the `canCreateOrg` check
129+ // (`plugins/organization/routes/crud-org.mjs` — verified against the
130+ // installed build, and covered by every node test here, all of which boot
131+ // through that path).
132+ organization({ allowUserToCreateOrganization: false }),
114133 admin(),
115134 apiKey({ enableSessionForAPIKeys: true, rateLimit: { enabled: false } }),
116135 bearer(),