diff --git a/.changeset/validate-member-roles.md b/.changeset/validate-member-roles.md new file mode 100644 index 0000000000..a593234af2 --- /dev/null +++ b/.changeset/validate-member-roles.md @@ -0,0 +1,5 @@ +--- +"better-auth": patch +--- + +Validate roles when updating an organization member. Roles are now normalized into individual tokens and checked against the configured static and dynamic roles, so unknown or malformed role values are rejected instead of being persisted. diff --git a/.gitignore b/.gitignore index cc478b4614..a3825ce363 100644 --- a/.gitignore +++ b/.gitignore @@ -212,4 +212,4 @@ state.txt .cursor/ .claude/worktrees -.deepsec \ No newline at end of file +.deepsec diff --git a/packages/better-auth/src/plugins/organization/organization.test.ts b/packages/better-auth/src/plugins/organization/organization.test.ts index 10d5b9ea86..e5a98915bd 100644 --- a/packages/better-auth/src/plugins/organization/organization.test.ts +++ b/packages/better-auth/src/plugins/organization/organization.test.ts @@ -2385,14 +2385,15 @@ describe("owner can update roles", async () => { }); it("allows an org owner to remove their own creator role if not sole owner", async () => { - await auth.api.updateMemberRole({ + const updated = await auth.api.updateMemberRole({ headers: { cookie: adminCookie }, body: { organizationId: org.id, memberId: ownerId, - role: [], + role: ["custom"], }, }); + expect(updated.role).toBe("custom"); }); it("should throw error if sole org owner tries to remove creator role"), diff --git a/packages/better-auth/src/plugins/organization/routes/crud-members.test.ts b/packages/better-auth/src/plugins/organization/routes/crud-members.test.ts index 799b937a75..7bd80b9f93 100644 --- a/packages/better-auth/src/plugins/organization/routes/crud-members.test.ts +++ b/packages/better-auth/src/plugins/organization/routes/crud-members.test.ts @@ -407,6 +407,166 @@ describe("updateMemberRole", async () => { .message, ); }); + + it("should not allow a comma-delimited role string", async () => { + const { headers } = await signInWithTestUser(); + const client = createAuthClient({ + plugins: [organizationClient()], + baseURL: "http://localhost:3000/api/auth", + fetchOptions: { + customFetchImpl, + }, + }); + + const org = await client.organization.create({ + name: "escalation", + slug: "escalation", + fetchOptions: { + headers, + }, + }); + + const adminUser = await auth.api.signUpEmail({ + body: { + email: "admin-escalation@test.com", + name: "admin", + password: "password", + }, + }); + + const adminMember = await auth.api.addMember({ + body: { + organizationId: org.data?.id as string, + userId: adminUser.user.id, + role: "admin", + }, + }); + + const adminHeaders = new Headers({ + authorization: `Bearer ${adminUser.token}`, + }); + + const escalated = await client.organization.updateMemberRole( + { + organizationId: org.data?.id as string, + memberId: adminMember?.id as string, + role: "admin,owner" as "admin", + }, + { + headers: adminHeaders, + }, + ); + + expect(escalated.error?.status).toBe(403); + expect(escalated.data).toBeNull(); + + const ctx = await auth.$context; + const persisted = await ctx.adapter.findOne<{ role: string }>({ + model: "member", + where: [{ field: "id", value: adminMember?.id as string }], + }); + expect(persisted?.role).toBe("admin"); + }); + + it("should reject updating a member to an unknown role", async () => { + const { headers } = await signInWithTestUser(); + const client = createAuthClient({ + plugins: [organizationClient()], + baseURL: "http://localhost:3000/api/auth", + fetchOptions: { + customFetchImpl, + }, + }); + + const org = await client.organization.create({ + name: "unknown-role", + slug: "unknown-role", + fetchOptions: { + headers, + }, + }); + + const newUser = await auth.api.signUpEmail({ + body: { + email: "unknown-role@test.com", + name: "test", + password: "password", + }, + }); + + const member = await auth.api.addMember({ + body: { + organizationId: org.data?.id as string, + userId: newUser.user.id, + role: "member", + }, + }); + + const updated = await client.organization.updateMemberRole( + { + organizationId: org.data?.id as string, + memberId: member?.id as string, + role: "superadmin" as "admin", + }, + { + headers, + }, + ); + + expect(updated.error?.status).toBe(400); + expect(updated.error?.message).toContain( + ORGANIZATION_ERROR_CODES.ROLE_NOT_FOUND, + ); + }); + + it("should reject updating a member to an empty role list", async () => { + const { headers } = await signInWithTestUser(); + const client = createAuthClient({ + plugins: [organizationClient()], + baseURL: "http://localhost:3000/api/auth", + fetchOptions: { + customFetchImpl, + }, + }); + + const org = await client.organization.create({ + name: "empty-role", + slug: "empty-role", + fetchOptions: { + headers, + }, + }); + + const newUser = await auth.api.signUpEmail({ + body: { + email: "empty-role@test.com", + name: "test", + password: "password", + }, + }); + + const member = await auth.api.addMember({ + body: { + organizationId: org.data?.id as string, + userId: newUser.user.id, + role: "member", + }, + }); + + for (const role of [[], ","] as ("admin" | "admin"[])[]) { + const updated = await client.organization.updateMemberRole( + { + organizationId: org.data?.id as string, + memberId: member?.id as string, + role, + }, + { + headers, + }, + ); + expect(updated.error?.status).toBe(400); + } + }); }); describe("activeMemberRole", async () => { diff --git a/packages/better-auth/src/plugins/organization/routes/crud-members.ts b/packages/better-auth/src/plugins/organization/routes/crud-members.ts index 10ceba572c..4a1df8ca48 100644 --- a/packages/better-auth/src/plugins/organization/routes/crud-members.ts +++ b/packages/better-auth/src/plugins/organization/routes/crud-members.ts @@ -6,6 +6,7 @@ import * as z from "zod"; import { getSessionFromCtx, sessionMiddleware } from "../../../api"; import type { InferAdditionalFieldsFromPluginOptions } from "../../../db"; import { toZodSchema } from "../../../db/to-zod"; +import { defaultRoles } from "../access/statement"; import { getOrgAdapter } from "../adapter"; import { orgMiddleware, orgSessionMiddleware } from "../call"; import { ORGANIZATION_ERROR_CODES } from "../error-codes"; @@ -525,11 +526,52 @@ export const updateMemberRole = (option: O) => } const adapter = getOrgAdapter(ctx.context, ctx.context.orgOptions); - const roleToSet: string[] = Array.isArray(ctx.body.role) - ? ctx.body.role - : ctx.body.role - ? [ctx.body.role] - : []; + const roleToSet: string[] = ( + Array.isArray(ctx.body.role) ? ctx.body.role : [ctx.body.role] + ) + .flatMap((role) => role.split(",")) + .map((role) => role.trim()) + .filter(Boolean); + + if (roleToSet.length === 0) { + throw APIError.fromStatus("BAD_REQUEST"); + } + + const validStaticRoles = new Set([ + ...Object.keys(defaultRoles), + ...Object.keys(ctx.context.orgOptions.roles || {}), + ]); + const unknownRoles = roleToSet.filter( + (role) => !validStaticRoles.has(role), + ); + if (unknownRoles.length > 0) { + if (ctx.context.orgOptions.dynamicAccessControl?.enabled) { + const foundRoles = await ctx.context.adapter.findMany<{ + role: string; + }>({ + model: "organizationRole", + where: [ + { field: "organizationId", value: organizationId }, + { field: "role", value: unknownRoles, operator: "in" }, + ], + }); + const foundRoleNames = foundRoles.map((r) => r.role); + const stillInvalid = unknownRoles.filter( + (r) => !foundRoleNames.includes(r), + ); + if (stillInvalid.length > 0) { + throw new APIError("BAD_REQUEST", { + code: ORGANIZATION_ERROR_CODES.ROLE_NOT_FOUND.code, + message: `${ORGANIZATION_ERROR_CODES.ROLE_NOT_FOUND.code}: ${stillInvalid.join(", ")}`, + }); + } + } else { + throw new APIError("BAD_REQUEST", { + code: ORGANIZATION_ERROR_CODES.ROLE_NOT_FOUND.code, + message: `${ORGANIZATION_ERROR_CODES.ROLE_NOT_FOUND.code}: ${unknownRoles.join(", ")}`, + }); + } + } const member = await adapter.findMemberByOrgId({ userId: session.user.id, @@ -647,7 +689,7 @@ export const updateMemberRole = (option: O) => } const previousRole = toBeUpdatedMember.role; - const newRole = parseRoles(ctx.body.role as string | string[]); + const newRole = parseRoles(roleToSet); // Run beforeUpdateMemberRole hook if (option?.organizationHooks?.beforeUpdateMemberRole) {