Skip to content

Commit 398ca55

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 f5dabbe commit 398ca55

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
@@ -59,6 +59,47 @@ async function authenticateRequestForApiBuilder(
5959

6060
type AnyZodSchema = z.ZodFirstPartySchemaTypes | z.ZodDiscriminatedUnion<any, any>;
6161

62+
// Most route auth checks pass an array of resources to ability.can() with
63+
// "any-element-passes" semantics — a single record carries multiple
64+
// identifiers (a run is addressable by friendlyId / batch / tags / task) so a
65+
// JWT scoped to *any* of them grants access to the row.
66+
//
67+
// Batch operations are different: each item in the array is a *distinct*
68+
// resource and authorization must hold for every one of them. Wrapping the
69+
// array via `everyResource` flips the auth check from `some` to `every`. The
70+
// marker is a Symbol so it can't collide with arbitrary RbacResource fields.
71+
const EVERY_RESOURCE_MARKER = Symbol.for("@trigger.dev/rbac.everyResource");
72+
73+
type EveryResourceAuth = {
74+
readonly [EVERY_RESOURCE_MARKER]: true;
75+
readonly resources: readonly RbacResource[];
76+
};
77+
78+
export function everyResource(resources: RbacResource[]): EveryResourceAuth {
79+
return { [EVERY_RESOURCE_MARKER]: true, resources };
80+
}
81+
82+
function isEveryResource(value: unknown): value is EveryResourceAuth {
83+
return (
84+
typeof value === "object" &&
85+
value !== null &&
86+
(value as Record<symbol, unknown>)[EVERY_RESOURCE_MARKER] === true
87+
);
88+
}
89+
90+
type AuthResource = RbacResource | RbacResource[] | EveryResourceAuth;
91+
92+
function checkAuth(
93+
ability: RbacAbility,
94+
action: string,
95+
resource: AuthResource
96+
): boolean {
97+
if (isEveryResource(resource)) {
98+
return resource.resources.every((r) => ability.can(action, r));
99+
}
100+
return ability.can(action, resource);
101+
}
102+
62103
type ApiKeyRouteBuilderOptions<
63104
TParamsSchema extends AnyZodSchema | undefined = undefined,
64105
TSearchParamsSchema extends AnyZodSchema | undefined = undefined,
@@ -97,7 +138,7 @@ type ApiKeyRouteBuilderOptions<
97138
headers: THeadersSchema extends z.ZodFirstPartySchemaTypes | z.ZodDiscriminatedUnion<any, any>
98139
? z.infer<THeadersSchema>
99140
: undefined
100-
) => RbacResource | RbacResource[];
141+
) => AuthResource;
101142
};
102143
};
103144

@@ -233,7 +274,7 @@ export function createLoaderApiRoute<
233274
parsedHeaders
234275
);
235276

236-
if (!ability.can(action, $authResource)) {
277+
if (!checkAuth(ability, action, $authResource)) {
237278
return await wrapResponse(
238279
request,
239280
json(
@@ -484,7 +525,7 @@ type ApiKeyActionRouteBuilderOptions<
484525
// externalId for sessions) read it here so a JWT minted for either form
485526
// authorizes both URL forms.
486527
resource: TResource | undefined
487-
) => RbacResource | RbacResource[];
528+
) => AuthResource;
488529
};
489530
maxContentLength?: number;
490531
body?: TBodySchema;
@@ -700,7 +741,7 @@ export function createActionApiRoute<
700741
resource
701742
);
702743

703-
if (!ability.can(action, $resource)) {
744+
if (!checkAuth(ability, action, $resource)) {
704745
return await wrapResponse(
705746
request,
706747
json(
@@ -807,7 +848,7 @@ type MultiMethodApiRouteOptions<
807848
corsStrategy?: "all" | "none";
808849
authorization?: {
809850
action: string;
810-
resource: (params: InferZod<TParamsSchema>) => RbacResource | RbacResource[];
851+
resource: (params: InferZod<TParamsSchema>) => AuthResource;
811852
};
812853
maxContentLength?: number;
813854
methods: Partial<
@@ -938,7 +979,7 @@ export function createMultiMethodApiRoute<
938979
const { action, resource } = authorization;
939980
const $resource = resource(parsedParams);
940981

941-
if (!ability.can(action, $resource)) {
982+
if (!checkAuth(ability, action, $resource)) {
942983
return await wrapResponse(
943984
request,
944985
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)