Skip to content

Commit bdc08f1

Browse files
committed
test(queue): verify concurrent cross-tenant isolation through processJob (#4793)
Rent-a-Loop's #4793 asked for a deliberately adversarial test proving one tenant's execution can't reach another tenant's data. Audited every module-level mutable state candidate across src/ (github, queue, review, selfhost, signals, db -- ~96 declarations) plus packages/loopover-miner's execution runtime: everything is either a static read-only allowlist, a correctly tenant/token-keyed cache (verified via existing key-derivation code, e.g. the GitHub response cache's auth-identity+URL key), a deployment-wide singleton by design (SDK clients, health counters, kill-switch cache), or -- for the miner's own execution runtime -- safe by construction since each rented loop run is a fresh CLI process per the architecture #4783 already established, not a shared long-lived instance. Adds a stronger end-to-end proof than #4794's narrower credential-cache test: two different installations' real pull_request webhooks driven through the actual processJob entry point CONCURRENTLY, sharing one Worker isolate's module-level state, with every GitHub write checked against both tenants' identifying data. No code changes were needed -- the isolation already held.
1 parent 6db3683 commit bdc08f1

1 file changed

Lines changed: 104 additions & 0 deletions

File tree

test/unit/queue.test.ts

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7314,4 +7314,108 @@ describe("queue processors", () => {
73147314
// Shared cache-input-fingerprint builder for the #4603 pair below -- mirrors "#1"'s own inline fingerprint,
73157315
// parameterized only by PR title/number/sha so both tests get a genuine cache HIT (aiCalls stays 0) instead of
73167316
// silently falling through to a real (unmocked-defect) AI call on a fingerprint mismatch.
7317+
7318+
it("#4793: two different tenants' concurrent processJob calls never cross-contaminate a shared Worker isolate", async () => {
7319+
// Rent-a-Loop's execution-sandboxing acceptance criterion (#4793): "a deliberately adversarial test run
7320+
// attempting to access another tenant's data ... fails to escape the sandbox." This drives TWO different
7321+
// installations' real pull_request:opened webhooks through the ACTUAL processJob entry point CONCURRENTLY
7322+
// (not just the narrower token-cache layer #4794 already proved) -- the residual risk this exercises is
7323+
// module-level state shared across requests within the SAME Worker isolate, not GitHub's own per-installation
7324+
// API scoping. Every write GitHub receives is checked against BOTH tenants' identifying data.
7325+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
7326+
const TENANT_A = { installationId: 9001, repo: "tenant-a/secret-repo", prNumber: 1, prTitle: "Tenant A's confidential rollout plan", sha: "shaA1" };
7327+
const TENANT_B = { installationId: 9002, repo: "tenant-b/other-repo", prNumber: 2, prTitle: "Tenant B's unrelated bugfix", sha: "shaB1" };
7328+
7329+
for (const t of [TENANT_A, TENANT_B]) {
7330+
await upsertRepositorySettings(env, {
7331+
repoFullName: t.repo,
7332+
commentMode: "all_prs",
7333+
publicSurface: "comment_only",
7334+
autoLabelEnabled: false,
7335+
checkRunMode: "off",
7336+
reviewCheckMode: "disabled",
7337+
aiReviewMode: "off",
7338+
});
7339+
}
7340+
7341+
const postedComments: Record<string, { body: string; authorization: string }[]> = {
7342+
[TENANT_A.repo]: [],
7343+
[TENANT_B.repo]: [],
7344+
};
7345+
const mintedTokensByInstallation: Record<number, string> = {};
7346+
7347+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
7348+
const url = input.toString();
7349+
const method = init?.method ?? "GET";
7350+
const authorization = new Headers(init?.headers).get("authorization") ?? "";
7351+
7352+
const installationMatch = /\/app\/installations\/(\d+)\/access_tokens/.exec(url);
7353+
if (installationMatch) {
7354+
const installationId = Number(installationMatch[1]);
7355+
const token = `installation-${installationId}-token`;
7356+
mintedTokensByInstallation[installationId] = token;
7357+
return Response.json({ token, expires_at: new Date(Date.now() + 60 * 60_000).toISOString() });
7358+
}
7359+
7360+
for (const t of [TENANT_A, TENANT_B]) {
7361+
const [owner, repo] = t.repo.split("/");
7362+
if (url.includes(`/repos/${owner}/${repo}/pulls/${t.prNumber}/files`)) {
7363+
return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]);
7364+
}
7365+
if (url.endsWith(`/repos/${owner}/${repo}/pulls/${t.prNumber}`)) {
7366+
return Response.json({ number: t.prNumber, title: t.prTitle, state: "open", user: { login: "contributor" }, head: { sha: t.sha }, labels: [], body: null, mergeable_state: "clean" });
7367+
}
7368+
if (url.includes(`/repos/${owner}/${repo}/commits/${t.sha}/check-runs`)) return Response.json({ total_count: 0, check_runs: [] });
7369+
if (url.includes(`/repos/${owner}/${repo}/commits/${t.sha}/status`)) return Response.json({ state: "success", statuses: [] });
7370+
if (url.includes(`/repos/${owner}/${repo}/issues/${t.prNumber}/comments`) && method === "GET") return Response.json([]);
7371+
if (url.includes(`/repos/${owner}/${repo}/issues/${t.prNumber}/comments`) && method === "POST") {
7372+
const body = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? "");
7373+
postedComments[t.repo]!.push({ body, authorization });
7374+
return Response.json({ id: 1 }, { status: 201 });
7375+
}
7376+
}
7377+
if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } });
7378+
return Response.json({});
7379+
});
7380+
7381+
const jobFor = (t: typeof TENANT_A) => {
7382+
const [owner, name] = t.repo.split("/") as [string, string];
7383+
return {
7384+
type: "github-webhook" as const,
7385+
deliveryId: `sandbox-test-${t.installationId}`,
7386+
eventName: "pull_request" as const,
7387+
payload: {
7388+
action: "opened",
7389+
installation: { id: t.installationId, account: { login: owner, id: t.installationId, type: "Organization" as const } },
7390+
repository: { name, full_name: t.repo, private: false, owner: { login: owner } },
7391+
pull_request: { number: t.prNumber, title: t.prTitle, state: "open", user: { login: "contributor" }, head: { sha: t.sha }, labels: [], body: null },
7392+
},
7393+
};
7394+
};
7395+
7396+
// Truly concurrent: both installations' jobs race through processJob at once, sharing this one Worker
7397+
// isolate's module-level caches (installation-token cache, response cache, single-flight maps, etc.).
7398+
await Promise.all([processJob(env, jobFor(TENANT_A)), processJob(env, jobFor(TENANT_B))]);
7399+
7400+
// Each installation minted its OWN token -- no cross-tenant credential reuse.
7401+
expect(mintedTokensByInstallation[TENANT_A.installationId]).toBe(`installation-${TENANT_A.installationId}-token`);
7402+
expect(mintedTokensByInstallation[TENANT_B.installationId]).toBe(`installation-${TENANT_B.installationId}-token`);
7403+
7404+
// Each repo received its own reviewing-placeholder POST followed by the final comment (the stub's empty
7405+
// GET-comments response means the code can't find the placeholder to PATCH, so it POSTs the final one
7406+
// fresh too -- both are legitimate writes to check, not test noise). Every write is authenticated with
7407+
// its OWN installation's token, and the final comment's content never leaks the other tenant's PR title.
7408+
expect(postedComments[TENANT_A.repo]!.length).toBeGreaterThanOrEqual(1);
7409+
expect(postedComments[TENANT_B.repo]!.length).toBeGreaterThanOrEqual(1);
7410+
for (const write of postedComments[TENANT_A.repo]!) expect(write.authorization).toBe(`token installation-${TENANT_A.installationId}-token`);
7411+
for (const write of postedComments[TENANT_B.repo]!) expect(write.authorization).toBe(`token installation-${TENANT_B.installationId}-token`);
7412+
const finalA = postedComments[TENANT_A.repo]!.find((c) => !c.body.includes("is reviewing"))!;
7413+
const finalB = postedComments[TENANT_B.repo]!.find((c) => !c.body.includes("is reviewing"))!;
7414+
expect(finalA).toBeDefined();
7415+
expect(finalB).toBeDefined();
7416+
expect(finalA.body).not.toContain(TENANT_B.prTitle);
7417+
expect(finalB.body).not.toContain(TENANT_A.prTitle);
7418+
expect(finalA.body).not.toContain(TENANT_B.repo);
7419+
expect(finalB.body).not.toContain(TENANT_A.repo);
7420+
});
73177421
});

0 commit comments

Comments
 (0)