Skip to content

Commit 977089c

Browse files
committed
RBAC: address PR review — batch trigger AND, fallback resilience, picker tightening
#1 Batch trigger AND semantics (security): `api.v[12].tasks.batch` now uses `everyResource(...)` so a JWT scoped to taskA can no longer submit a batch that also includes taskB / taskC. Added an `everyResource` helper to `apiBuilder` (Symbol-marked wrapper that flips `ability.can` to `every`). Multi-key OR semantics still apply for single-resource arrays (a run carries multiple identifiers). Updated the e2e test to assert AND behaviour. #3 Realtime stream resource (correctness): `findResource` for `realtime.v1.streams.$runId.$streamId` now selects `taskIdentifier`, `runTags`, and `realtimeStreamsVersion` — fields the auth resource builder + handler read but findResource was returning undefined for. #4 projectCreated optional chaining (crash bug): added the missing `?.` between v3Subscription and plan so a missing subscription no longer throws and aborts project creation. #5 RBAC plugin loader logging: distinguish "plugin itself missing" from "plugin found but a transitive dep failed to resolve" by inspecting the ERR_MODULE_NOT_FOUND error message for the plugin's own module specifier. The transitive-dep case now logs at error level (matches the comment's stated behaviour). Removed the orphan log line that contradicted it. #6 account.tokens picker source mismatch: the picker now sources roles from the same plan-tier-filtered list (`systemRoles().filter(available)`) as the default-role calculation. Added server-side roleId revalidation in the create action so a hand-crafted POST can't bind a PAT to an unavailable role.
1 parent 59ee4c8 commit 977089c

8 files changed

Lines changed: 176 additions & 47 deletions

File tree

apps/webapp/app/routes/account.tokens/route.tsx

Lines changed: 45 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -58,33 +58,45 @@ export const meta: MetaFunction = () => {
5858

5959
// PATs aren't org-scoped, but the RBAC plugin's allRoles is org-keyed
6060
// (a plugin may also expose org-defined custom roles alongside the
61-
// global system roles). To get the canonical system roles (Owner /
62-
// Admin / Member / Viewer), we hand allRoles any orgId the user
63-
// belongs to and filter down to the rows the plugin marks as system
64-
// roles. This is a UI-only convenience — the chosen role becomes a
65-
// global TokenRole that applies wherever the PAT is used. Custom
66-
// (org-defined) roles are out of scope for v1: their org-binding
67-
// semantics for a multi-org user's PAT need a separate design pass.
61+
// global system roles). The picker shows the assignable system role
62+
// catalogue for the user's primary org — joining `allRoles` (for the
63+
// full Role with permissions) against `systemRoles` (for the per-org
64+
// `available` flag, which gates roles by plan tier). This is a UI-only
65+
// convenience — the chosen role becomes a global TokenRole that
66+
// applies wherever the PAT is used. Custom (org-defined) roles are
67+
// out of scope for v1: their org-binding semantics for a multi-org
68+
// user's PAT need a separate design pass.
6869
async function loadSystemRolesForUser(userId: string) {
6970
const orgMember = await prisma.orgMember.findFirst({
7071
where: { userId },
7172
select: { organizationId: true },
7273
orderBy: { createdAt: "asc" },
7374
});
7475
if (!orgMember) {
75-
return { roles: [], userRoleId: null as string | null, orgId: null as string | null };
76+
return {
77+
roles: [],
78+
userRoleId: null as string | null,
79+
orgId: null as string | null,
80+
};
7681
}
7782

78-
const allRoles = await rbac.allRoles(orgMember.organizationId);
79-
const systemRoles = allRoles.filter((r) => r.isSystem);
83+
const [allRoles, systemRoles, userRole] = await Promise.all([
84+
rbac.allRoles(orgMember.organizationId),
85+
rbac.systemRoles(orgMember.organizationId),
86+
rbac.getUserRole({ userId, organizationId: orgMember.organizationId }),
87+
]);
8088

81-
const userRole = await rbac.getUserRole({
82-
userId,
83-
organizationId: orgMember.organizationId,
84-
});
89+
// Restrict the picker to system roles the plan permits assigning —
90+
// anything else would be a noisy create-time failure (or, with a
91+
// permissive fallback, a token bound to a role this org isn't
92+
// allowed to issue).
93+
const availableIds = new Set(
94+
(systemRoles ?? []).filter((r) => r.available).map((r) => r.id)
95+
);
96+
const roles = allRoles.filter((r) => r.isSystem && availableIds.has(r.id));
8597

8698
return {
87-
roles: systemRoles,
99+
roles,
88100
userRoleId: userRole?.id ?? null,
89101
orgId: orgMember.organizationId,
90102
};
@@ -158,10 +170,27 @@ export const action: ActionFunction = async ({ request }) => {
158170
switch (submission.value.action) {
159171
case "create": {
160172
try {
173+
// Revalidate the submitted roleId against the plan-allowed set
174+
// — the loader filters the picker, but a hand-crafted POST can
175+
// still submit any string. Empty / undefined is fine: that
176+
// means "no role" and createPersonalAccessToken just doesn't
177+
// write a TokenRole.
178+
const submittedRoleId = submission.value.roleId;
179+
if (submittedRoleId) {
180+
const { roles } = await loadSystemRolesForUser(userId);
181+
const allowed = new Set(roles.map((r) => r.id));
182+
if (!allowed.has(submittedRoleId)) {
183+
return json(
184+
{ errors: { body: "Selected role isn't available on this plan" } },
185+
{ status: 400 }
186+
);
187+
}
188+
}
189+
161190
const tokenResult = await createPersonalAccessToken({
162191
name: submission.value.tokenName,
163192
userId,
164-
roleId: submission.value.roleId,
193+
roleId: submittedRoleId,
165194
});
166195

167196
return json({ ...submission, payload: { token: tokenResult } });

apps/webapp/app/routes/api.v1.tasks.batch.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,10 @@ import {
77
import { env } from "~/env.server";
88
import { AuthenticatedEnvironment, getOneTimeUseToken } from "~/services/apiAuth.server";
99
import { logger } from "~/services/logger.server";
10-
import { createActionApiRoute } from "~/services/routeBuilders/apiBuilder.server";
10+
import {
11+
createActionApiRoute,
12+
everyResource,
13+
} from "~/services/routeBuilders/apiBuilder.server";
1114
import { resolveIdempotencyKeyTTL } from "~/utils/idempotencyKeys.server";
1215
import { ServiceValidationError } from "~/v3/services/baseService.server";
1316
import {
@@ -30,11 +33,17 @@ const { action, loader } = createActionApiRoute(
3033
maxContentLength: env.BATCH_TASK_PAYLOAD_MAXIMUM_SIZE,
3134
authorization: {
3235
action: "batchTrigger",
36+
// Each item in the batch is a distinct task — every one must be
37+
// authorized, not just any one of them. `everyResource` flips
38+
// the auth check to AND semantics so a JWT scoped to taskA can't
39+
// submit a batch that also includes taskB / taskC.
3340
resource: (_, __, ___, body) =>
34-
Array.from(new Set(body.items.map((i) => i.task))).map((id) => ({
35-
type: "tasks",
36-
id,
37-
})),
41+
everyResource(
42+
Array.from(new Set(body.items.map((i) => i.task))).map((id) => ({
43+
type: "tasks",
44+
id,
45+
}))
46+
),
3847
},
3948
corsStrategy: "all",
4049
},

apps/webapp/app/routes/api.v2.tasks.batch.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,10 @@ import { env } from "~/env.server";
99
import { RunEngineBatchTriggerService } from "~/runEngine/services/batchTrigger.server";
1010
import { AuthenticatedEnvironment, getOneTimeUseToken } from "~/services/apiAuth.server";
1111
import { logger } from "~/services/logger.server";
12-
import { createActionApiRoute } from "~/services/routeBuilders/apiBuilder.server";
12+
import {
13+
createActionApiRoute,
14+
everyResource,
15+
} from "~/services/routeBuilders/apiBuilder.server";
1316
import {
1417
handleRequestIdempotency,
1518
saveRequestIdempotency,
@@ -32,11 +35,17 @@ const { action, loader } = createActionApiRoute(
3235
maxContentLength: env.BATCH_TASK_PAYLOAD_MAXIMUM_SIZE,
3336
authorization: {
3437
action: "batchTrigger",
38+
// Each item in the batch is a distinct task — every one must be
39+
// authorized, not just any one of them. `everyResource` flips
40+
// the auth check to AND semantics so a JWT scoped to taskA can't
41+
// submit a batch that also includes taskB / taskC.
3542
resource: (_, __, ___, body) =>
36-
Array.from(new Set(body.items.map((i) => i.task))).map((id) => ({
37-
type: "tasks",
38-
id,
39-
})),
43+
everyResource(
44+
Array.from(new Set(body.items.map((i) => i.task))).map((id) => ({
45+
type: "tasks",
46+
id,
47+
}))
48+
),
4049
},
4150
corsStrategy: "all",
4251
},

apps/webapp/app/routes/realtime.v1.streams.$runId.$streamId.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,12 @@ export const loader = createLoaderApiRoute(
8686
friendlyId: params.runId,
8787
runtimeEnvironmentId: auth.environment.id,
8888
},
89-
include: {
89+
select: {
90+
id: true,
91+
friendlyId: true,
92+
taskIdentifier: true,
93+
runTags: true,
94+
realtimeStreamsVersion: true,
9095
batch: {
9196
select: {
9297
friendlyId: true,

apps/webapp/app/services/projectCreated.server.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ export async function projectCreated(
2222
});
2323
} else {
2424
const plan = await getCurrentPlan(organization.id);
25-
if (plan?.v3Subscription.plan?.limits.hasStagingEnvironment) {
25+
if (plan?.v3Subscription?.plan?.limits?.hasStagingEnvironment) {
2626
await createEnvironment({ organization, project, type: "STAGING" });
2727
await createEnvironment({
2828
organization,

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

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,47 @@ async function authenticateRequestForApiBuilder(
8383

8484
type AnyZodSchema = z.ZodFirstPartySchemaTypes | z.ZodDiscriminatedUnion<any, any>;
8585

86+
// Most route auth checks pass an array of resources to ability.can() with
87+
// "any-element-passes" semantics — a single record carries multiple
88+
// identifiers (a run is addressable by friendlyId / batch / tags / task) so a
89+
// JWT scoped to *any* of them grants access to the row.
90+
//
91+
// Batch operations are different: each item in the array is a *distinct*
92+
// resource and authorization must hold for every one of them. Wrapping the
93+
// array via `everyResource` flips the auth check from `some` to `every`. The
94+
// marker is a Symbol so it can't collide with arbitrary RbacResource fields.
95+
const EVERY_RESOURCE_MARKER = Symbol.for("@trigger.dev/rbac.everyResource");
96+
97+
type EveryResourceAuth = {
98+
readonly [EVERY_RESOURCE_MARKER]: true;
99+
readonly resources: readonly RbacResource[];
100+
};
101+
102+
export function everyResource(resources: RbacResource[]): EveryResourceAuth {
103+
return { [EVERY_RESOURCE_MARKER]: true, resources };
104+
}
105+
106+
function isEveryResource(value: unknown): value is EveryResourceAuth {
107+
return (
108+
typeof value === "object" &&
109+
value !== null &&
110+
(value as Record<symbol, unknown>)[EVERY_RESOURCE_MARKER] === true
111+
);
112+
}
113+
114+
type AuthResource = RbacResource | RbacResource[] | EveryResourceAuth;
115+
116+
function checkAuth(
117+
ability: RbacAbility,
118+
action: string,
119+
resource: AuthResource
120+
): boolean {
121+
if (isEveryResource(resource)) {
122+
return resource.resources.every((r) => ability.can(action, r));
123+
}
124+
return ability.can(action, resource);
125+
}
126+
86127
type ApiKeyRouteBuilderOptions<
87128
TParamsSchema extends AnyZodSchema | undefined = undefined,
88129
TSearchParamsSchema extends AnyZodSchema | undefined = undefined,
@@ -121,7 +162,7 @@ type ApiKeyRouteBuilderOptions<
121162
headers: THeadersSchema extends z.ZodFirstPartySchemaTypes | z.ZodDiscriminatedUnion<any, any>
122163
? z.infer<THeadersSchema>
123164
: undefined
124-
) => RbacResource | RbacResource[];
165+
) => AuthResource;
125166
};
126167
};
127168

@@ -257,7 +298,7 @@ export function createLoaderApiRoute<
257298
parsedHeaders
258299
);
259300

260-
if (!ability.can(action, $authResource)) {
301+
if (!checkAuth(ability, action, $authResource)) {
261302
return await wrapResponse(
262303
request,
263304
json(
@@ -498,7 +539,7 @@ type ApiKeyActionRouteBuilderOptions<
498539
// externalId for sessions) read it here so a JWT minted for either form
499540
// authorizes both URL forms.
500541
resource: TResource | undefined
501-
) => RbacResource | RbacResource[];
542+
) => AuthResource;
502543
};
503544
maxContentLength?: number;
504545
body?: TBodySchema;
@@ -714,7 +755,7 @@ export function createActionApiRoute<
714755
resource
715756
);
716757

717-
if (!ability.can(action, $resource)) {
758+
if (!checkAuth(ability, action, $resource)) {
718759
return await wrapResponse(
719760
request,
720761
json(
@@ -811,7 +852,7 @@ type MultiMethodApiRouteOptions<
811852
corsStrategy?: "all" | "none";
812853
authorization?: {
813854
action: string;
814-
resource: (params: InferZod<TParamsSchema>) => RbacResource | RbacResource[];
855+
resource: (params: InferZod<TParamsSchema>) => AuthResource;
815856
};
816857
maxContentLength?: number;
817858
methods: Partial<
@@ -942,7 +983,7 @@ export function createMultiMethodApiRoute<
942983
const { action, resource } = authorization;
943984
const $resource = resource(parsedParams);
944985

945-
if (!ability.can(action, $resource)) {
986+
if (!checkAuth(ability, action, $resource)) {
946987
return await wrapResponse(
947988
request,
948989
json(

apps/webapp/test/auth-api.e2e.full.test.ts

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -387,9 +387,11 @@ describe("API", () => {
387387
// Trigger task routes (TRI-8733). The single-task route uses
388388
// action: "trigger" with a single resource { type: "tasks", id };
389389
// batch v1/v2 use action: "batchTrigger" with a body-derived array
390-
// [{type:"tasks", id}, ...]; v3 batches use a collection-level
391-
// resource { type: "tasks" } (no id — items are validated per-row
392-
// when streamed).
390+
// [{type:"tasks", id}, ...] under AND semantics — every task in the
391+
// batch must be authorized, not just any one (otherwise a JWT scoped
392+
// to one task could submit a batch with arbitrary other tasks).
393+
// v3 batches use a collection-level resource { type: "tasks" }
394+
// (no id — items are validated per-row when streamed).
393395
//
394396
// ACTION_ALIASES (from packages/core/src/v3/jwt.ts) maps write→trigger
395397
// and write→batchTrigger so write:tasks scopes also satisfy these
@@ -619,10 +621,11 @@ describe("API", () => {
619621
expect(res.status).not.toBe(403);
620622
});
621623

622-
it("JWT with batchTrigger:tasks:taskA + body has [taskA, taskB]: auth passes (any-match)", async () => {
623-
// Multi-key resource semantics: when the route's resource is an
624-
// array, ANY scope matching ANY array element grants access.
625-
// Locks in the legacy contract from TRI-8719.
624+
it("JWT with batchTrigger:tasks:taskA + body has [taskA, taskB]: 403 (every-task semantics)", async () => {
625+
// Batch trigger uses AND semantics — every task in the body must
626+
// be authorized, not just any one of them. A JWT scoped to only
627+
// taskA cannot submit a batch that also includes taskB, otherwise
628+
// the caller would be triggering tasks they have no scope for.
626629
const server = getTestServer();
627630
const seed = await seedTestEnvironment(server.prisma);
628631
const jwt = await generateJWT({
@@ -639,6 +642,28 @@ describe("API", () => {
639642
headers: { Authorization: `Bearer ${jwt}`, "Content-Type": "application/json" },
640643
body: JSON.stringify(buildBody(["taskA", "taskB"])),
641644
});
645+
expect(res.status).toBe(403);
646+
});
647+
648+
it("JWT with batchTrigger:tasks:taskA + body has [taskA] only: auth passes", async () => {
649+
// Per-task scope grants per-task access — a batch containing
650+
// only the authorized task is allowed.
651+
const server = getTestServer();
652+
const seed = await seedTestEnvironment(server.prisma);
653+
const jwt = await generateJWT({
654+
secretKey: seed.apiKey,
655+
payload: {
656+
pub: true,
657+
sub: seed.environment.id,
658+
scopes: ["batchTrigger:tasks:taskA"],
659+
},
660+
expirationTime: "15m",
661+
});
662+
const res = await server.webapp.fetch(path, {
663+
method: "POST",
664+
headers: { Authorization: `Bearer ${jwt}`, "Content-Type": "application/json" },
665+
body: JSON.stringify(buildBody(["taskA"])),
666+
});
642667
expect(res.status).not.toBe(401);
643668
expect(res.status).not.toBe(403);
644669
});

internal-packages/rbac/src/index.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ class LazyController implements RoleBaseAccessController {
5959
if (options?.forceFallback) {
6060
return new RoleBaseAccessFallback(prisma).create(helpers);
6161
}
62+
const moduleName = "@triggerdotdev/plugins/rbac";
6263
try {
63-
const moduleName = "@triggerdotdev/plugins/rbac";
6464
const module = await import(moduleName);
6565
const plugin: RoleBasedAccessControlPlugin = module.default;
6666
console.log("RBAC: using plugin implementation");
@@ -71,15 +71,28 @@ class LazyController implements RoleBaseAccessController {
7171
// — silently swallowing the error here is what produced "why is
7272
// the fallback being used?" mysteries before.
7373
//
74-
// 1. Module-not-found — expected when no plugin is installed.
74+
// 1. The plugin itself is absent (no install) — expected.
7575
// Logged at info level only when RBAC_LOG_FALLBACK=1 so
7676
// production logs stay quiet.
7777
// 2. Anything else (transitive dep missing, init error, syntax
7878
// error in the plugin's dist, etc.) — a real bug. Always
7979
// logged loudly so it surfaces in CI / production logs.
80+
//
81+
// Node throws ERR_MODULE_NOT_FOUND for both cases — the *plugin*
82+
// module being absent and a *transitive* dep of the plugin
83+
// being absent. Disambiguate by checking whether the missing
84+
// specifier in the error message is the plugin's own moduleName.
8085
const code = (err as NodeJS.ErrnoException | undefined)?.code;
81-
const isModuleNotFound = code === "ERR_MODULE_NOT_FOUND" || code === "MODULE_NOT_FOUND";
82-
if (!isModuleNotFound) {
86+
const message = err instanceof Error ? err.message : String(err);
87+
const isModuleNotFound =
88+
code === "ERR_MODULE_NOT_FOUND" || code === "MODULE_NOT_FOUND";
89+
const isPluginItselfMissing =
90+
isModuleNotFound && message.includes(moduleName);
91+
92+
if (!isPluginItselfMissing) {
93+
// Either the error wasn't a missing-module error at all, or the
94+
// plugin was found but a transitive dep failed to resolve.
95+
// Either way: a real problem worth surfacing.
8396
console.error(
8497
"RBAC: plugin found but failed to load; falling back to default implementation",
8598
err
@@ -88,8 +101,6 @@ class LazyController implements RoleBaseAccessController {
88101
console.log(
89102
"RBAC: no plugin installed (ERR_MODULE_NOT_FOUND); using fallback"
90103
);
91-
} else {
92-
console.log(`RBAC: using fallback implementation. ${err}`);
93104
}
94105
return new RoleBaseAccessFallback(prisma).create(helpers);
95106
}

0 commit comments

Comments
 (0)