feat: rbac by MohammadShehadeh · Pull Request #20 · MohammadShehadeh/nucleus

MohammadShehadeh · 2026-06-18T00:03:31Z

No description provided.

- Updated '@better-auth/expo' from 1.6.2 to 1.6.18 - Updated '@tailwindcss/postcss' from ^4.3.0 to ^4.3.1 - Updated 'better-auth' from 1.6.2 to 1.6.18 - Updated 'next' from ^16.2.7 to ^16.2.9 - Updated 'tailwindcss' from ^4.3.0 to ^4.3.1 - Updated 'vitest' from ^3.2.4 to ^4.1.8 - Changed TypeScript lib target from "ES2022" to "ESNext"

MohammadShehadeh

Automated code review — RBAC feature

Reviewed the diff against main (excluding lockfile + generated migration snapshots). 10 findings posted inline, ranked by severity:

🔴 Critical (1)

users.setRole privilege escalation — no assertCanGrant guard; a user:assign-role holder can self-assign any non-super-admin role.

🟠 High (3)

Rate limiter silently fails open — Redis connection is only established via the RBAC cache path, so anonymous/cold traffic is never limited.
getBaseUrl() builds a double-scheme URL (https://http://…) for server-side tRPC.
superjson is an undeclared dependency of @nucleus/cache.

🟡 Medium (3)

Error.isError runtime regression (Node 22 / older browsers) vs. the previous instanceof Error.
Re-running the seed creates two isDefault roles, breaking the single-default invariant.
A failed/empty signup-role assignment leaves a permanently role-less user.

🟢 Low (3)

Inaccurate comment: wrapWithCache doesn't actually collapse concurrent misses (thundering herd on cold keys).
{...props} double-spread onto an inner SidebarGroup.
roles.create can surface a raw 500 instead of CONFLICT under a concurrent race.

Also verified sound: the requirePermission wiring, the setDefault transaction, roles.update's .set() with undefined, the Can gate logic, and the FK ON DELETE SET NULL. Two recall-pass candidates (superjson decoding legacy JSON, and an unhandled rejection on connect timeout) were investigated and dropped as non-issues.

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:23:43Z

+      return takeFirstOrNull(
+        await ctx.db
+          .update(user)
+          .set({ roleId: input.roleId })


🔴 Critical — privilege escalation: setRole has no assertCanGrant guard.

This mutation only blocks the super_admin slug — it never checks that the caller actually holds the target role's permissions. Role editing (roles.create/roles.update) carefully calls assertCanGrant(ctx.session.user.permissions, input.permissions) to prevent granting permissions you don't have, but assigning a role to a user is an equivalent grant and skips that check.

Exploit: a user whose role grants only ["user:assign-role"] calls setRole({ userId: <self>, roleId: <Admin role id> }) where Admin holds role:delete, user:update, etc. Only super_admin is rejected, so the update succeeds and on their next session they wield every Admin permission.

Fix: load the target role's permissions and gate on them, e.g.

assertCanGrant(ctx.session.user.permissions ?? [], target.permissions);

before applying the update.

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:23:55Z

+   */
+  async wrapWithCache<T>(fn: () => Promise<T>, options: { key: string; ttl?: number }): Promise<T> {
+    const { key, ttl = DEFAULT_TTL_SECONDS } = options;
+    await this.connect().catch(() => undefined);


🟠 High — the rate limiter is silently disabled (fails open) for anonymous/cold traffic.

With disableOfflineQueue: true and the isReady guards on every op, multi() returns null unless the client is connected. This wrapWithCache is the only caller of connect() in the repo, and it runs only via session enrichment (loadUserRbac → customSession).

In proxy.ts, rateLimiter.check() runs before getSession(), and anonymous requests never invoke customSession — so the connection is never established and RedisRateLimiter.check() hits its if (!pipeline) fail-open branch (allowed: true) for every request. Rate limiting stays off until some authenticated request happens to connect Redis; for anonymous-only traffic it never engages (and that's exactly the traffic rate limiting most needs to stop).

Fix: connect eagerly at startup (e.g. in an instrumentation hook or inside getInstance) rather than coupling connection establishment to the RBAC cache path.

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:24:04Z

  if (typeof window !== "undefined") return window.location.origin;
-  if (env.VERCEL_URL) return `https://${env.VERCEL_URL}`;
-  return `http://localhost:${env.PORT ?? 3000}`;
+  return `https://${env.NEXT_PUBLIC_BASE_URL}`;


🟠 High — getBaseUrl() builds a double-scheme URL on the server.

NEXT_PUBLIC_BASE_URL is validated as z.url() — a full URL with scheme (it's passed bare to better-auth's baseUrl in auth/server.ts). Prefixing https:// produces e.g. https://http://localhost:3000; new URL(...) then parses the host as http, so any server-side/SSR tRPC fetch targets the wrong host.

The old code prefixed https:// to VERCEL_URL (host-only), which was correct — the prefix just wasn't removed when the env var was swapped.

Fix:

return env.NEXT_PUBLIC_BASE_URL;

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:24:11Z

  RedisScripts,
 } from "redis";
 import { createClient } from "redis";
+import superjson from "superjson";


🟠 High — superjson is an undeclared (phantom) dependency of @nucleus/cache.

packages/cache/package.json lists only redis under dependencies, but this module now imports superjson. Under pnpm's isolated node_modules, @nucleus/cache can't resolve superjson — tsc/runtime will fail with Cannot find module 'superjson', working only by accident if another package happens to hoist it.

Fix: add superjson to packages/cache/package.json dependencies.

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:24:18Z

          enabled: (op) =>
-            env.NODE_ENV === "development" ||
-            (op.direction === "down" && op.result instanceof Error),
+            env.NODE_ENV === "development" || (op.direction === "down" && Error.isError(op.result)),


🟡 Medium — Error.isError(op.result) is a runtime/compat regression.

This replaced the universally-supported op.result instanceof Error. Error.isError is unavailable on Node 22 (the project's engines allow >=22.20.0) and pre-2025 browsers (Chrome <134 / Safari <18.4 / Firefox <136). In production (NODE_ENV !== "development", so the || no longer short-circuits) every down-direction op evaluates it and throws TypeError: Error.isError is not a function where unsupported, breaking the logger link.

Fix: revert to op.result instanceof Error.

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:24:26Z

+    })
+    .onConflictDoUpdate({
+      target: role.slug,
+      set: { isSystem: true, isDefault: true },


🟡 Medium — re-seeding breaks the "exactly one default role" invariant.

This onConflictDoUpdate force-sets isDefault: true on member without clearing any other default. If an admin moved the default to a custom role via roles.setDefault, re-running pnpm -F @nucleus/db seed leaves two rows with isDefault = true.

resolveSignupRoleId then runs where(eq(role.isDefault, true)).limit(1) with no ORDER BY, so new signups get an arbitrary default role.

Fix: either don't re-assert isDefault on conflict, or clear other defaults in the same transaction (mirror roles.setDefault's "set all others false, then set this true").

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:24:37Z

+  superAdminEmails: string[]
+): Promise<void> {
+  const roleId = await resolveSignupRoleId(email, superAdminEmails);
+  if (roleId) {


🟡 Medium — a failed/empty signup-role assignment leaves a permanently role-less user.

assignSignupRole silently no-ops when resolveSignupRoleId returns null (no isDefault role seeded), and it runs in a databaseHooks.user.create.after hook with no retry/repair. A user can complete signup with roleId = NULL → permissions: [] and no path to recover except a manual DB fix or users.setRole.

Fix: treat a missing default role as a hard error (fail the signup / alert) rather than a silent no-op, and consider wrapping user-create + role-assignment so a failure doesn't leave a half-provisioned user.

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:24:44Z

+ * Loads a user's role + effective permissions for session enrichment, keyed by
+ * `roleId` (passed from better-auth's user object via the `roleId` additional
+ * field, so no extra user query is needed). Cached in Redis and shared across
+ * all users with the same role; `wrapWithCache` collapses concurrent misses.


🟢 Low — this comment is inaccurate: wrapWithCache does not collapse concurrent misses.

wrapWithCache (packages/cache/src/index.ts) has no in-flight/single-flight map — it just does get → miss → run fn → set. N concurrent requests for a cold or expired role key each run loadRoleRbac (N parallel DB queries), i.e. a thundering herd on every cold key.

Fix: either implement single-flight in wrapWithCache (de-dupe by key with an in-flight promise map) or drop the "collapses concurrent misses" claim so future code doesn't rely on coalescing that isn't there.

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:24:54Z

+            ))}
+          </SidebarMenu>
+        </SidebarGroup>
+        <SidebarGroup {...props}>


🟢 Low — {...props} is spread onto both <Sidebar> (line 91) and this inner <SidebarGroup>.

The same rest props (typed as React.ComponentProps<typeof Sidebar>) land on the root Sidebar and the secondary-nav group. Latent today since DashboardLayout renders <AppSidebar user={user} /> with no extra props, but any future className/variant/side/etc. passed to AppSidebar would leak onto this group as invalid DOM attributes.

Fix: drop {...props} here; it belongs only on the root <Sidebar>.

Generated by Claude Code

MohammadShehadeh · 2026-06-18T00:25:01Z

+      // fall back to a generated slug — only system roles need a meaningful one.
+      const slug = slugify(input.name) || `role-${randomUUID().slice(0, 8)}`;
+
+      const existing = takeFirstOrNull(


🟢 Low — create can surface a raw 500 instead of a clean CONFLICT under a race.

This existence check is a separate, non-transactional SELECT before the insert. Two concurrent create calls with the same name both pass the check; one insert wins and the other violates the role_name_unique / role_slug_unique constraint, throwing a raw DB error to the client instead of the intended CONFLICT TRPCError.

(The sequential "two distinct names → same slug" case is already handled by the or(name, slug) check — only the concurrent race remains.)

Fix: catch the unique-violation from .insert(...) and translate it to a CONFLICT, making the existence check a fast-path rather than the integrity guarantee.

Generated by Claude Code

MohammadShehadeh added 2 commits June 12, 2026 18:23

feat: role based access control

b6818ab

MohammadShehadeh commented Jun 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rbac#20

feat: rbac#20
MohammadShehadeh wants to merge 2 commits into
mainfrom
feat/rbac

MohammadShehadeh commented Jun 18, 2026

Uh oh!

MohammadShehadeh left a comment

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

MohammadShehadeh Jun 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MohammadShehadeh commented Jun 18, 2026

Uh oh!

MohammadShehadeh left a comment

Choose a reason for hiding this comment

Automated code review — RBAC feature

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant