Skip to content

Commit 9f987ae

Browse files
matt-aitkenclaude
andcommitted
fix(webapp): backwards-compat for type-level scope on runs list + tighten team action authz
Two related findings reviewing the RBAC route migration. 1. api.v1.runs / realtime.v1.runs: type-level scope no longer granted unfiltered access Pre-RBAC the resource was a plain object (`{ tasks: searchParams[...] }` for api.v1.runs, the searchParams object itself for realtime.v1.runs) and the legacy `checkAuthorization` iterated `Object.keys`. So a JWT with type-level `read:tasks` (api.v1.runs) or `read:tags` (realtime.v1.runs) — no id — granted access to the unfiltered list. The new `anyResource([…])` model only matches resources we list, and the post-migration list dropped the type-level alternative when no filter was set, so those JWTs started 403'ing on unfiltered listings. Add `{ type: "tasks" }` and `{ type: "tags" }` (no id) alongside `{ type: "runs" }` so the legacy semantic is preserved verbatim — per-id `read:tasks:foo` / `read:tags:bar` still grants only when the filter contains the matching id. 2. team action: explicit per-intent authz gate The team route's action handler accepts three intents (set-role, purchase-seats, remove-member). set-role already gates on `manage:members`, but purchase-seats and remove-member relied on model-layer enforcement (SetSeatsAddOnService, removeTeamMember). The loader gates on `read:members` (broader audience than pre-RBAC's owner/admin-only loader), so any user reaching the page could submit those forms — banking on defense in depth at the model layer. Fix: - purchase-seats → require `manage:billing` - remove-member → require `manage:members`, with self-leave (target.userId === actor.userId) carved out as always allowed - removeTeamMember model fn previously only verified the actor was a member of the org — any org member could remove any other member by id. The new route gate closes that hole. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2674417 commit 9f987ae

3 files changed

Lines changed: 49 additions & 3 deletions

File tree

apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.team/route.tsx

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,13 @@ export const action = dashboardAction(
151151
return orgId ? { organizationId: orgId } : {};
152152
},
153153
// No top-level authorization — different intents have different
154-
// requirements (set-role needs manage:members; remove/leave is
155-
// gated by the existing model layer; purchase-seats by the
156-
// SetSeatsAddOnService). Per-intent ability checks happen inside.
154+
// requirements. Each branch inside checks the right ability:
155+
// set-role → manage:members
156+
// purchase-seats → manage:billing
157+
// remove-member → manage:members (skipped for self-leave)
158+
// Don't rely on the model-layer (removeTeamMember /
159+
// SetSeatsAddOnService) for enforcement — those are defense in
160+
// depth; the route layer is where the ability gate belongs.
157161
},
158162
async ({ user, ability, request, params, context }) => {
159163
const userId = user.id;
@@ -187,6 +191,15 @@ export const action = dashboardAction(
187191
}
188192

189193
if (formType === "purchase-seats") {
194+
// Adjusting seat count is a billing operation. Pre-RBAC the team
195+
// page's loader gated the entire route on Owner/Admin, so reaching
196+
// this action implied authority. Post-RBAC the loader requires
197+
// `read:members` (broader audience), so gate the seat purchase
198+
// explicitly here against the right ability rather than relying
199+
// on the SetSeatsAddOnService for enforcement at the model layer.
200+
if (!ability.can("manage", { type: "billing" })) {
201+
return json({ ok: false, error: "Unauthorized" } as const, { status: 403 });
202+
}
190203
const org = await $replica.organization.findFirst({
191204
where: { slug: organizationSlug },
192205
select: { id: true },
@@ -231,6 +244,21 @@ export const action = dashboardAction(
231244
return json(submission);
232245
}
233246

247+
// Default intent: remove a member or leave the org. Self-leave (the
248+
// actor removing their own membership) is always allowed. Removing
249+
// another member requires `manage:members` — pre-RBAC the
250+
// `removeTeamMember` model fn only verified the actor was a member
251+
// of the target org, so any org member could remove any other
252+
// member by id; this gate fixes that latent permissions hole.
253+
const targetMember = await $replica.orgMember.findFirst({
254+
where: { id: submission.value.memberId },
255+
select: { userId: true },
256+
});
257+
const isSelfLeave = targetMember?.userId === userId;
258+
if (!isSelfLeave && !ability.can("manage", { type: "members" })) {
259+
return json({ ok: false, error: "Unauthorized" } as const, { status: 403 });
260+
}
261+
234262
try {
235263
const deletedMember = await removeTeamMember({
236264
userId,

apps/webapp/app/routes/api.v1.runs.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,19 @@ export const loader = createLoaderApiRoute(
1818
action: "read",
1919
resource: (_, __, searchParams) => {
2020
const taskFilter = searchParams["filter[taskIdentifier]"] ?? [];
21+
// Pre-RBAC, the resource was `{ tasks: searchParams["filter[taskIdentifier]"] }`
22+
// and the legacy `checkAuthorization` iterated `Object.keys` — so a
23+
// JWT with type-level `read:tasks` (no id) granted access to the
24+
// unfiltered runs list. The new ability model only matches against
25+
// resources we list, so the type-level `{ type: "tasks" }` element
26+
// (alongside `{ type: "runs" }` and the per-id task elements)
27+
// preserves that semantic — `read:tasks` JWTs in the wild still
28+
// list unfiltered runs without needing a separate `read:runs`
29+
// scope. Per-id `read:tasks:foo` still grants only when the
30+
// filter includes `foo`.
2131
return anyResource([
2232
{ type: "runs" },
33+
{ type: "tasks" },
2334
...taskFilter.map((id) => ({ type: "tasks", id })),
2435
]);
2536
},

apps/webapp/app/routes/realtime.v1.runs.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,15 @@ export const loader = createLoaderApiRoute(
2525
authorization: {
2626
action: "read",
2727
resource: (_, __, searchParams) =>
28+
// Pre-RBAC, the resource was the searchParams object itself and
29+
// the legacy `checkAuthorization` iterated `Object.keys`, so a
30+
// JWT with type-level `read:tags` (no id) granted access to the
31+
// unfiltered runs stream. Including `{ type: "tags" }` here
32+
// preserves that — per-id `read:tags:<tag>` still grants only
33+
// when the filter includes that tag.
2834
anyResource([
2935
{ type: "runs" },
36+
{ type: "tags" },
3037
...(searchParams.tags ?? []).map((tag) => ({ type: "tags", id: tag })),
3138
]),
3239
},

0 commit comments

Comments
 (0)