Skip to content

Commit 6bcd369

Browse files
matt-aitkenclaude
andauthored
feat(webapp,rbac): REQUIRE_PLUGINS=1 fail-fast for required plugin loads [TRI-9852] (#3734)
## Summary - `internal-packages/rbac/src/index.ts` — in `LazyController.load()`'s catch block, throw an Error when `process.env.REQUIRE_PLUGINS === "1"` instead of silently falling back. The throw is captured into the lazy controller's init promise, so it surfaces on the first method call. - `apps/webapp/app/routes/healthcheck.tsx` — `await rbac.isUsingPlugin()` after the DB ping. With `REQUIRE_PLUGINS=1` and a failed plugin load, the throw surfaces here and the healthcheck returns 500 → readiness probe fails → rollout is rolled back. Noop for self-hosters. - `.server-changes/require-plugins-fail-fast.md` — server-changes entry. - `internal-packages/rbac/src/require-plugins.test.ts` — 4 unit tests covering loader branching: unset → fallback, `=1` → throw, `forceFallback: true` wins, only exactly `"1"` enforces. - `internal-packages/testcontainers/src/webapp.ts` — adds `requirePlugins?: boolean` to `StartWebappOptions`. Implies `forceRbacFallback: false`. - `apps/webapp/test/healthcheck-require-plugins.e2e.test.ts` — e2e closes the loop: spawns a real webapp, hits `/healthcheck` via HTTP, asserts 500 with `REQUIRE_PLUGINS=1` and 200 without. ## Motivation Today the RBAC plugin loader catches any plugin-load failure (missing module, broken transitive dep, init throw) and silently returns the default fallback implementation. This is the correct behaviour for self-hosters who don't ship the plugin — but it's dangerous in deployments where the plugin is expected to load: an accidentally-missing or broken plugin would silently disable enforcement. `REQUIRE_PLUGINS=1` makes the loader fail loudly in those deployments. The variable name is intentionally plural and generic — future plugin contracts (audit logs, SSO) can read the same flag without renaming. Closes [TRI-9852](https://linear.app/triggerdotdev/issue/TRI-9852/require-plugins1-fail-fast-for-required-plugin-loads). ## Test plan - [x] `pnpm run test --filter @trigger.dev/rbac` — 38/38 tests pass, including the 4 new loader tests - [x] `pnpm run typecheck --filter webapp` — passes - [x] `pnpm run typecheck --filter @trigger.dev/rbac --filter @internal/testcontainers` — passes - [x] e2e test added (`healthcheck-require-plugins.e2e.test.ts`) — CI runs it via `e2e-webapp.yml`. Couldn't run locally (no Docker daemon up); CI has Docker provisioned. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 0ee1461 commit 6bcd369

7 files changed

Lines changed: 219 additions & 8 deletions

File tree

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
area: webapp
3+
type: feature
4+
---
5+
6+
Add `REQUIRE_PLUGINS=1` env var. When set, the RBAC plugin loader throws instead of silently falling back to the default implementation if the plugin module fails to load (missing, broken transitive dep, etc.). The webapp's `/healthcheck` route now resolves the lazy plugin controller so the throw surfaces during readiness probes — a deploy where the plugin didn't load fails the probe and is rolled back.
7+
8+
Self-hosters leave `REQUIRE_PLUGINS` unset and continue to use the fallback when no plugin is installed.

apps/webapp/app/routes/healthcheck.tsx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,24 @@
11
import { prisma } from "~/db.server";
22
import type { LoaderFunction } from "@remix-run/node";
33
import { env } from "~/env.server";
4+
import { rbac } from "~/services/rbac.server";
45

56
export const loader: LoaderFunction = async ({ request }) => {
67
try {
8+
// Resolve the lazy plugin controller so plugin-load failures surface
9+
// during readiness probes. With REQUIRE_PLUGINS=1, a failed plugin
10+
// load throws here and the rollout's readiness probe fails. The
11+
// fallback path doesn't touch the DB, so this runs even when
12+
// HEALTHCHECK_DATABASE_DISABLED=1 — REQUIRE_PLUGINS protection must
13+
// not be silently bypassed by the DB-disabled flag.
14+
await rbac.isUsingPlugin();
15+
716
if (env.HEALTHCHECK_DATABASE_DISABLED === "1") {
817
return new Response("OK");
918
}
1019

1120
await prisma.$queryRaw`SELECT 1`;
21+
1222
return new Response("OK");
1323
} catch (error: unknown) {
1424
console.log("healthcheck ❌", { error });

apps/webapp/server.ts

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -197,10 +197,18 @@ if (ENABLE_CLUSTER && cluster.isPrimary) {
197197
})
198198
);
199199
} else {
200-
// we need to do the health check here at /healthcheck
201-
app.get("/healthcheck", (req, res) => {
202-
res.status(200).send("OK");
203-
});
200+
// we need to do the health check here at /healthcheck — forward
201+
// to the Remix handler so the loader's readiness checks (DB ping,
202+
// REQUIRE_PLUGINS-gated plugin load) run in this mode too. A
203+
// static 200 here would silently mask a failed plugin load.
204+
app.get(
205+
"/healthcheck",
206+
// @ts-ignore
207+
createRequestHandler({
208+
build,
209+
mode: MODE,
210+
})
211+
);
204212
}
205213

206214
const server = app.listen(port, () => {
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/**
2+
* E2E verification that REQUIRE_PLUGINS=1 fails the rollout via /healthcheck.
3+
*
4+
* The unit tests in @trigger.dev/rbac cover the loader throw. This file
5+
* closes the loop end-to-end: spawn a real webapp, hit /healthcheck via
6+
* HTTP, and verify the route's catch turns the throw into a 500 — the
7+
* status the ECS/k8s readiness probe rolls back on.
8+
*
9+
* Each case spawns its own webapp + Postgres + Redis container (~30s) so
10+
* env can differ per case. Slow but isolated, matching api-auth.e2e.test.ts.
11+
*
12+
* Requires a pre-built webapp: pnpm run build --filter webapp
13+
*
14+
* The REQUIRE_PLUGINS=1 case relies on the plugin NOT being resolvable
15+
* from the spawned webapp. CI satisfies this because the plugin isn't in
16+
* pnpm-lock.yaml. Local devs who ran `pnpm dev:link-webapp` have the
17+
* plugin symlinked into apps/webapp/node_modules — that case is detected
18+
* and skipped below.
19+
*/
20+
import { existsSync } from "node:fs";
21+
import { resolve } from "node:path";
22+
import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
23+
import type { TestServer } from "@internal/testcontainers/webapp";
24+
import { startTestServer } from "@internal/testcontainers/webapp";
25+
26+
const LINKED_PLUGIN_PATH = resolve(
27+
__dirname,
28+
"..",
29+
"node_modules",
30+
"@triggerdotdev",
31+
"plugins"
32+
);
33+
const pluginLocallyLinked = existsSync(LINKED_PLUGIN_PATH);
34+
35+
vi.setConfig({ testTimeout: 180_000 });
36+
37+
describe("/healthcheck with REQUIRE_PLUGINS", () => {
38+
describe.skipIf(pluginLocallyLinked)("REQUIRE_PLUGINS=1 + plugin missing", () => {
39+
let server: TestServer;
40+
41+
beforeAll(async () => {
42+
// requirePlugins: true implies forceRbacFallback: false, so the
43+
// loader actually tries to dynamic-import the plugin. The plugin
44+
// is not installed in this OSS repo, so the import fails and the
45+
// loader throws (instead of falling back) because REQUIRE_PLUGINS=1.
46+
// The throw surfaces on the first .isUsingPlugin() call from the
47+
// /healthcheck route, which catches it and returns 500.
48+
server = await startTestServer({ requirePlugins: true });
49+
}, 180_000);
50+
51+
afterAll(async () => {
52+
await server?.stop();
53+
}, 120_000);
54+
55+
it("returns 500 so the readiness probe fails and the rollout is rolled back", async () => {
56+
const res = await server.webapp.fetch("/healthcheck");
57+
expect(res.status).toBe(500);
58+
expect(await res.text()).toBe("ERROR");
59+
});
60+
});
61+
62+
// Surface the skip in dev so it doesn't go unnoticed. CI hits the real test above.
63+
describe.runIf(pluginLocallyLinked)(
64+
"REQUIRE_PLUGINS=1 + plugin LOCALLY LINKED (cross-repo dev setup)",
65+
() => {
66+
it.skip(
67+
`skipped because ${LINKED_PLUGIN_PATH} exists — plugin would load successfully. Run \`pnpm dev:unlink-webapp\` to exercise this case locally; CI runs it without the link.`,
68+
() => {}
69+
);
70+
}
71+
);
72+
73+
describe("REQUIRE_PLUGINS unset + plugin missing", () => {
74+
let server: TestServer;
75+
76+
beforeAll(async () => {
77+
// Default: forceRbacFallback=true so the loader short-circuits to
78+
// the fallback without trying to import. /healthcheck succeeds.
79+
server = await startTestServer();
80+
}, 180_000);
81+
82+
afterAll(async () => {
83+
await server?.stop();
84+
}, 120_000);
85+
86+
it("returns 200 (baseline — unchanged self-hoster behaviour)", async () => {
87+
const res = await server.webapp.fetch("/healthcheck");
88+
expect(res.status).toBe(200);
89+
expect(await res.text()).toBe("OK");
90+
});
91+
});
92+
});

internal-packages/rbac/src/index.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,13 @@ class LazyController implements RoleBaseAccessController {
5353

5454
constructor(prisma: RbacPrismaInput, options?: RbacCreateOptions) {
5555
this._init = this.load(prisma, options);
56+
// load() runs eagerly but the result is awaited lazily on first method
57+
// call. If load() rejects (e.g. REQUIRE_PLUGINS=1 + plugin missing) and
58+
// nothing awaits _init before Node ticks past, the rejection surfaces
59+
// as unhandledRejection and kills the process. Attach a no-op .catch
60+
// so Node sees the rejection as handled; the error is re-thrown when
61+
// any consumer awaits this._init via c().
62+
this._init.catch(() => {});
5663
}
5764

5865
private async load(
@@ -105,6 +112,20 @@ class LazyController implements RoleBaseAccessController {
105112
"RBAC: no plugin installed (ERR_MODULE_NOT_FOUND); using fallback"
106113
);
107114
}
115+
116+
// Fail-fast for deployments that require plugins to be present. Set
117+
// REQUIRE_PLUGINS=1 in environments where the fallback is not an
118+
// acceptable degraded state — the throw surfaces on the first method
119+
// call on the lazy controller (e.g. via the webapp's /healthcheck
120+
// route), so the rollout's readiness probe fails and the deploy is
121+
// rolled back. Self-hosters leave REQUIRE_PLUGINS unset and continue
122+
// to use the fallback when no plugin is installed.
123+
if (process.env.REQUIRE_PLUGINS === "1") {
124+
throw new Error(
125+
`REQUIRE_PLUGINS=1 but plugin "${moduleName}" did not load: ${message}`
126+
);
127+
}
128+
108129
return new RoleBaseAccessFallback(prisma).create();
109130
}
110131
}
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import type { PrismaClient } from "@trigger.dev/database";
2+
import { afterEach, describe, expect, it, vi } from "vitest";
3+
import loader from "./index.js";
4+
5+
// The plugin module `@triggerdotdev/plugins/rbac` is not installed in this
6+
// repo (it lives in the cloud monorepo), so a real dynamic import inside
7+
// the loader will reliably fail with ERR_MODULE_NOT_FOUND. These tests
8+
// exercise the loader's branching on that natural failure — no module
9+
// mocking required.
10+
11+
// The fallback's isUsingPlugin() returns false synchronously without
12+
// touching prisma, so a placeholder client is fine for tests that only
13+
// drive the loader path.
14+
const prismaPlaceholder = {} as unknown as PrismaClient;
15+
16+
describe("LazyController plugin loading", () => {
17+
afterEach(() => {
18+
vi.unstubAllEnvs();
19+
});
20+
21+
it("falls back silently when REQUIRE_PLUGINS is unset and the plugin is missing", async () => {
22+
vi.stubEnv("REQUIRE_PLUGINS", "");
23+
const controller = loader.create(prismaPlaceholder);
24+
await expect(controller.isUsingPlugin()).resolves.toBe(false);
25+
});
26+
27+
it("throws when REQUIRE_PLUGINS=1 and the plugin is missing", async () => {
28+
vi.stubEnv("REQUIRE_PLUGINS", "1");
29+
const controller = loader.create(prismaPlaceholder);
30+
await expect(controller.isUsingPlugin()).rejects.toThrow(/REQUIRE_PLUGINS=1/);
31+
});
32+
33+
it("forceFallback wins over REQUIRE_PLUGINS=1 (so tests inheriting the env aren't broken)", async () => {
34+
vi.stubEnv("REQUIRE_PLUGINS", "1");
35+
const controller = loader.create(prismaPlaceholder, { forceFallback: true });
36+
await expect(controller.isUsingPlugin()).resolves.toBe(false);
37+
});
38+
39+
it("treats any non-'1' REQUIRE_PLUGINS value as unset (must be exactly '1' to enforce)", async () => {
40+
vi.stubEnv("REQUIRE_PLUGINS", "true");
41+
const controller = loader.create(prismaPlaceholder);
42+
await expect(controller.isUsingPlugin()).resolves.toBe(false);
43+
});
44+
});

internal-packages/testcontainers/src/webapp.ts

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,16 @@ async function findFreePort(): Promise<number> {
2020
});
2121
}
2222

23-
async function waitForHealthcheck(url: string, timeoutMs = 60000): Promise<void> {
23+
async function waitForHealthcheck(
24+
url: string,
25+
opts: { timeoutMs?: number; acceptAnyResponse?: boolean } = {}
26+
): Promise<void> {
27+
const { timeoutMs = 60000, acceptAnyResponse = false } = opts;
2428
const deadline = Date.now() + timeoutMs;
2529
while (Date.now() < deadline) {
2630
try {
2731
const res = await fetch(url);
28-
if (res.ok) return;
32+
if (acceptAnyResponse || res.ok) return;
2933
} catch {}
3034
await new Promise((r) => setTimeout(r, 500));
3135
}
@@ -49,6 +53,18 @@ export interface StartWebappOptions {
4953
* plugin instead, for testing the plugin path.
5054
*/
5155
forceRbacFallback?: boolean;
56+
57+
/**
58+
* When true, spawns the webapp with `REQUIRE_PLUGINS=1` so the plugin
59+
* loader throws instead of silently falling back when the plugin
60+
* module fails to load. Used by the healthcheck rollback e2e test —
61+
* with this set and the plugin not installed, `/healthcheck` should
62+
* return 500.
63+
*
64+
* Implies `forceRbacFallback: false` (you can't observe REQUIRE_PLUGINS
65+
* behaviour when the loader is short-circuited).
66+
*/
67+
requirePlugins?: boolean;
5268
}
5369

5470
export async function startWebapp(
@@ -59,7 +75,10 @@ export async function startWebapp(
5975
instance: WebappInstance;
6076
stop: () => Promise<void>;
6177
}> {
62-
const forceRbacFallback = options.forceRbacFallback ?? true;
78+
// requirePlugins implies forceRbacFallback=false — you can't observe the
79+
// REQUIRE_PLUGINS=1 throw if the loader short-circuits before reaching it.
80+
const requirePlugins = options.requirePlugins ?? false;
81+
const forceRbacFallback = options.forceRbacFallback ?? !requirePlugins;
6382
const port = await findFreePort();
6483

6584
// Merge NODE_PATH so transitive pnpm deps (hoisted to .pnpm/node_modules) are resolvable
@@ -107,6 +126,10 @@ export async function startWebapp(
107126
// plugin is installed in the local node_modules. Set to "0" /
108127
// undefined to spawn a webapp that loads any installed plugin.
109128
...(forceRbacFallback ? { RBAC_FORCE_FALLBACK: "1" } : {}),
129+
// When requirePlugins is set, explicitly override RBAC_FORCE_FALLBACK
130+
// to "0" so a local apps/webapp/.env that sets it to "1" doesn't
131+
// short-circuit the loader past the REQUIRE_PLUGINS check.
132+
...(requirePlugins ? { REQUIRE_PLUGINS: "1", RBAC_FORCE_FALLBACK: "0" } : {}),
110133
NODE_PATH: nodePath,
111134
},
112135
stdio: ["ignore", "pipe", "pipe"],
@@ -142,7 +165,12 @@ export async function startWebapp(
142165

143166
try {
144167
if (spawnError) throw spawnError;
145-
await waitForHealthcheck(`${baseUrl}/healthcheck`);
168+
// When requirePlugins is set, /healthcheck is expected to respond with
169+
// 500 (the whole point of those tests is to verify that). Accept any
170+
// response as "the webapp is up" — the test then asserts the status.
171+
await waitForHealthcheck(`${baseUrl}/healthcheck`, {
172+
acceptAnyResponse: requirePlugins,
173+
});
146174
if (spawnError) throw spawnError;
147175
} catch (err) {
148176
proc.kill("SIGTERM");

0 commit comments

Comments
 (0)