Address Devin review: 4 RBAC auth path fixes

#1 internal-packages/rbac/src/ability.ts (severity: 🔴 silent privilege
   escalation): buildJwtAbility was treating any scope starting with
   `admin:` as a universal wildcard. Pre-RBAC, the legacy
   checkAuthorization string-matched superScopes — `admin:sessions` only
   granted access to routes that explicitly listed it. After the JWT-
   ability split, the same scope was returning true for every action on
   every resource. Restrict the bypass to bare `admin` (no second
   segment); `admin:<type>` now flows through normal matching as
   action="admin" against resources of that type. Adds 2 regression
   tests in ability.test.ts.

#2 apps/webapp/app/services/routeBuilders/apiBuilder.server.ts (status
   discard): authenticateRequestForApiBuilder hardcoded `status: 401`
   even though BearerAuthResult.status is `401 | 403`. A plugin
   returning 403 (e.g. suspended account, IP block) would silently get
   downgraded to 401 — semantically wrong (401 = "who are you?", 403 =
   "you're not allowed") and confusing for client retry logic. Plumb
   result.status through.

#3 apps/webapp/app/services/routeBuilders/apiBuilder.server.ts
   (everyResource([]) vacuous truth): [].every() returns true, so
   everyResource([]) was passing auth for any token. Not exploitable
   today (Zod rejects empty bodies before auth), but the auth layer
   should never grant on empty input. Same defensive guard added to
   anyResource() for symmetry — only PERMISSIVE_ABILITY would have
   granted there, but the pattern shouldn't depend on the ability's
   choice.

#4 internal-packages/rbac/src/fallback.ts (PREVIEW env regression): the
   fallback's authenticateBearer looked up environments by apiKey only,
   skipping the branch-aware resolution that findEnvironmentByApiKey
   does for PREVIEW envs. Self-hosters using preview/branch envs would
   either fail or operate against the parent env. Mirror the legacy
   path: read x-trigger-branch, include matching child env, and pivot
   the resolved env to the child (apiKey/orgMember/organization/project
   inherited from parent). sanitizeBranchName inlined here because
   internal-packages can't import webapp code; comment notes the
   duplication.

All four flagged by Devin's PR review. Cloud plugin's buildJwtAbility
gets the same #1 fix in a sibling commit on this PR's cloud branch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: matt-aitken

apps/webapp/app/services/routeBuilders/apiBuilder.server.ts

@@ -53,12 +53,15 @@ async function authenticateRequestForApiBuilder(
   request: Request,
   { allowJWT }: { allowJWT: boolean }
 ): Promise<
-  | { ok: false; status: 401; error: string }
+  | { ok: false; status: 401 | 403; error: string }
   | { ok: true; authentication: ApiAuthenticationResultSuccess; ability: RbacAbility }
 > {
   const result = await rbac.authenticateBearer(request, { allowJWT });
   if (!result.ok) {
-    return { ok: false, status: 401, error: result.error };
+    // Plugin auth distinguishes 401 (who are you?) from 403 (you're not
+    // allowed) — e.g. a suspended account or IP block returns 403.
+    // Forwarding the status preserves that semantic for client retry logic.
+    return { ok: false, status: result.status, error: result.error };
   }
 
   // The fallback already filters deleted projects; this is belt-and-braces for
@@ -158,9 +161,19 @@ function checkAuth(
   resource: AuthResource
 ): boolean {
   if (isEveryResource(resource)) {
+    // Empty array via [].every() is vacuously true — would let any token
+    // pass auth. Routes building everyResource() from request bodies
+    // (e.g. batch trigger items) should never produce zero elements
+    // because body validation rejects empty arrays first, but defending
+    // here anyway since the auth layer should never grant on no input.
+    if (resource.resources.length === 0) return false;
     return resource.resources.every((r) => ability.can(action, r));
   }
   if (isAnyResource(resource)) {
+    // Symmetric guard: anyResource([]) is benign for most abilities
+    // (.some() is false on empty), but PERMISSIVE_ABILITY would still
+    // grant. Treat empty as "no resource declared" → deny.
+    if (resource.resources.length === 0) return false;
     return ability.can(action, [...resource.resources]);
   }
   return ability.can(action, resource);

internal-packages/rbac/src/ability.test.ts

@@ -66,6 +66,29 @@ describe("buildJwtAbility", () => {
     expect(ability.can("write", { type: "deployments" })).toBe(true);
   });
 
+  // Pre-RBAC, the legacy checkAuthorization string-matched superScopes;
+  // a scope `admin:sessions` only granted access to routes that
+  // explicitly listed it. After the JWT-ability split we must not let
+  // `admin:<anything>` act as a universal wildcard — it should grant
+  // only the `admin` action against resources of that type.
+  it("admin:<type> is not a universal wildcard", () => {
+    const ability = buildJwtAbility(["admin:sessions"]);
+    expect(ability.can("read", { type: "runs" })).toBe(false);
+    expect(ability.can("write", { type: "tasks" })).toBe(false);
+    expect(ability.can("admin", { type: "runs" })).toBe(false);
+    // But it does grant the admin action on its own type.
+    expect(ability.can("admin", { type: "sessions" })).toBe(true);
+    expect(ability.can("admin", { type: "sessions", id: "ses_abc" })).toBe(true);
+  });
+
+  it("admin:<type>:<id> grants admin action only on that exact resource", () => {
+    const ability = buildJwtAbility(["admin:sessions:ses_abc"]);
+    expect(ability.can("admin", { type: "sessions", id: "ses_abc" })).toBe(true);
+    expect(ability.can("admin", { type: "sessions", id: "ses_xyz" })).toBe(false);
+    expect(ability.can("admin", { type: "runs" })).toBe(false);
+    expect(ability.can("read", { type: "sessions", id: "ses_abc" })).toBe(false);
+  });
+
   it("never grants canSuper", () => {
     expect(buildJwtAbility(["admin"]).canSuper()).toBe(false);
     expect(buildJwtAbility(["read:all"]).canSuper()).toBe(false);

internal-packages/rbac/src/ability.ts

@@ -27,7 +27,13 @@ export function buildJwtAbility(scopes: string[]): RbacAbility {
   const matches = (action: string, r: RbacResource): boolean =>
     scopes.some((scope) => {
       const [scopeAction, scopeType, scopeId] = scope.split(":");
-      if (scopeAction === "admin") return true;
+      // Bare `admin` is the universal wildcard. `admin:<type>` is *not* —
+      // it falls through to normal matching as action="admin" against
+      // resources of that type. Pre-RBAC, the legacy checkAuthorization
+      // string-matched superScopes; `admin:sessions` only granted access
+      // to routes that explicitly listed it. Treating `admin:<anything>`
+      // as universal here would silently broaden any such tokens.
+      if (scopeAction === "admin" && !scopeType) return true;
       if (scopeAction !== action && scopeAction !== "*") return false;
       if (scopeType === "all") return true;
       if (scopeType !== r.type) return false;

internal-packages/rbac/src/fallback.ts

@@ -80,10 +80,19 @@ class RoleBaseAccessFallbackController implements RoleBaseAccessController {
       };
     }
 
+    // PREVIEW envs are parents — operating "on a branch" means routing
+    // to a child env keyed by branchName. The customer authenticates
+    // with the parent's apiKey + an `x-trigger-branch` header. Mirror
+    // findEnvironmentByApiKey: include the matching child env so the
+    // pivot below can adopt its identity.
+    const branchName = sanitizeBranchName(request.headers.get("x-trigger-branch"));
     const include = {
       project: true,
       organization: true,
       orgMember: { select: { userId: true } },
+      childEnvironments: branchName
+        ? { where: { branchName, archivedAt: null } }
+        : undefined,
     } as const;
     let env = await this.prisma.runtimeEnvironment.findFirst({
       where: { apiKey: rawToken },
@@ -108,6 +117,32 @@ class RoleBaseAccessFallbackController implements RoleBaseAccessController {
       return { ok: false, status: 401, error: "Invalid API key" };
     }
 
+    // PREVIEW env requires a branch header; pivot to the child env so
+    // downstream code operates on the branch (its own id, but the
+    // parent's apiKey/orgMember/organization/project — exactly what
+    // findEnvironmentByApiKey does for the legacy auth path).
+    if (env.type === "PREVIEW") {
+      if (!branchName) {
+        return {
+          ok: false,
+          status: 401,
+          error: "x-trigger-branch header required for preview env",
+        };
+      }
+      const child = env.childEnvironments?.[0];
+      if (!child) {
+        return { ok: false, status: 401, error: "No matching branch env" };
+      }
+      env = {
+        ...child,
+        apiKey: env.apiKey,
+        orgMember: env.orgMember,
+        organization: env.organization,
+        project: env.project,
+        childEnvironments: [],
+      };
+    }
+
     const subject: RbacSubject = {
       type: "user",
       userId: env.orgMember?.userId ?? "",
@@ -276,6 +311,22 @@ class RoleBaseAccessFallbackController implements RoleBaseAccessController {
   }
 }
 
+// Mirror of `apps/webapp/app/v3/gitBranch.ts#sanitizeBranchName`.
+// Inlined here because internal-packages can't import webapp code; the
+// two should stay in sync. Strips common refs/* prefixes and rejects
+// unknown ref formats (returns undefined → no branch override).
+function sanitizeBranchName(ref: string | null): string | undefined {
+  if (!ref) return undefined;
+  if (ref.startsWith("refs/heads/")) return ref.substring("refs/heads/".length);
+  if (ref.startsWith("refs/remotes/")) return ref.substring("refs/remotes/".length);
+  if (ref.startsWith("refs/tags/")) return ref.substring("refs/tags/".length);
+  if (ref.startsWith("refs/pull/")) return ref.substring("refs/pull/".length);
+  if (ref.startsWith("refs/merge/")) return ref.substring("refs/merge/".length);
+  if (ref.startsWith("refs/release/")) return ref.substring("refs/release/".length);
+  if (ref.startsWith("refs/")) return undefined;
+  return ref;
+}
+
 function isPublicJWT(token: string): boolean {
   const parts = token.split(".");
   if (parts.length !== 3) return false;