Skip to content

Commit f58a9dc

Browse files
committed
fix(webapp): only let the browser set the agent's page context
The chat turn's metadata is now a whitelist: the page the user is on is all the browser can set, and every identity, tenancy and token field is filled in by the server.
1 parent d51cb9a commit f58a9dc

3 files changed

Lines changed: 176 additions & 8 deletions

apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.in.$.ts

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,23 @@ const FORWARDED_HEADERS = [
3333
"x-trigger-branch",
3434
];
3535

36+
// The only turn metadata a browser may set. Everything else the agent reads — identity, tenancy,
37+
// the delegated token, the eval opt-out's inputs — is injected server-side, so a client-sent copy
38+
// is dropped rather than merged. A whitelist, not a deny-list: a field added to the agent's
39+
// clientData is server-owned until it is listed here on purpose.
40+
const CLIENT_METADATA_KEYS = ["currentPage", "pageContext"] as const;
41+
42+
export function pickAgentClientMetadata(
43+
metadata: Record<string, unknown> | undefined
44+
): Record<string, unknown> {
45+
const picked: Record<string, unknown> = {};
46+
if (!metadata) return picked;
47+
for (const key of CLIENT_METADATA_KEYS) {
48+
if (metadata[key] !== undefined) picked[key] = metadata[key];
49+
}
50+
return picked;
51+
}
52+
3653
function tooLarge() {
3754
return json({ error: MESSAGE_TOO_LARGE_ERROR, code: MESSAGE_TOO_LARGE_CODE }, { status: 413 });
3855
}
@@ -104,16 +121,18 @@ export async function action({ request, params }: ActionFunctionArgs) {
104121
return tooLarge();
105122
}
106123
parsed.payload.metadata = {
107-
...(parsed.payload.metadata ?? {}),
124+
// Whitelisted: only the page context the browser is allowed to set survives, so it can
125+
// neither overwrite nor smuggle in any of the server-owned fields below.
126+
...pickAgentClientMetadata(parsed.payload.metadata),
108127
userActorToken: await mintDashboardAgentUserActorToken(user.id, {
109128
environmentId: runtimeEnv.id,
110129
}),
111130
apiOrigin,
112131
projectRef: project.externalRef,
113-
// Server-owned: the browser sends these too, and the eval opt-out and every tenancy
114-
// check key on them, so the client's copy must never win.
132+
// Server-owned: the eval opt-out and every tenancy check key on these.
115133
organizationId: project.organizationId,
116134
userId: user.id,
135+
projectId: project.id,
117136
// `(projectId, slug)` isn't unique (dev is per-member), so anything addressing
118137
// one environment row uses this id. `environmentName` is for name-addressed tools.
119138
environmentId: runtimeEnv.id,

apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.ts

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ import { resolveTriggerUri } from "~/services/resolveTriggerUri.server";
5757
import { requireUser } from "~/services/session.server";
5858
import { EnvironmentParamSchema } from "~/utils/pathBuilder";
5959
import { canAccessDashboardAgent } from "~/v3/canAccessDashboardAgent.server";
60+
// The client-metadata whitelist lives with the `in` proxy, the other mint site, so the two cannot
61+
// drift apart.
62+
import { pickAgentClientMetadata } from "./resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.in.$";
6063

6164
// The agent's tools address the canonical env name, not the dashboard URL slug.
6265
const ENV_NAME_BY_TYPE: Record<string, string> = {
@@ -284,14 +287,16 @@ export const action = async ({ request, params }: ActionFunctionArgs) => {
284287
} catch {
285288
/* invalid JSON — create without context metadata */
286289
}
290+
// Only the whitelisted page context survives; the rest is injected below.
291+
const clientContext = pickAgentClientMetadata(clientData);
287292

288293
const chatId = generateFriendlyId("chat");
289294
try {
290295
await createChat(dashboardAgentDb, {
291296
id: chatId,
292297
organizationId: project.organizationId,
293298
userId,
294-
...(clientData ? { metadata: { context: clientData } } : {}),
299+
...(clientData ? { metadata: { context: clientContext } } : {}),
295300
});
296301

297302
// Membership-scoped: dev rows are per-developer, so a token must never be minted for
@@ -310,17 +315,18 @@ export const action = async ({ request, params }: ActionFunctionArgs) => {
310315
mode: repoSnapshot ? "code" : "assistant",
311316
metadata: {
312317
// The agent validates run metadata against its clientDataSchema, so the
313-
// per-turn clientData must accompany the injected auth and context fields.
314-
...(clientData ?? {}),
318+
// per-turn client context must accompany the injected auth and context fields.
319+
...clientContext,
315320
userActorToken: await mintDashboardAgentUserActorToken(userId, {
316321
environmentId: runtimeEnv.id,
317322
}),
318323
apiOrigin: dashboardAgentApiOrigin(),
319324
projectRef: project.externalRef,
320325
// Server-owned, like the `in` proxy: the eval opt-out and every tenancy check
321-
// key on these, so a client-sent copy must not win.
326+
// key on these, so the client can't set them at all.
322327
organizationId: project.organizationId,
323328
userId,
329+
projectId: project.id,
324330
// Same environment identity the `in` proxy injects.
325331
environmentId: runtimeEnv.id,
326332
environmentName,
@@ -330,7 +336,19 @@ export const action = async ({ request, params }: ActionFunctionArgs) => {
330336
} else {
331337
// Cold start: the client sends the first message through the `in` proxy, which
332338
// injects the token.
333-
await startDashboardAgentSession({ chatId, clientData });
339+
// Same server-owned identity the head-start path injects; the `in` proxy adds the
340+
// delegated token on the first turn.
341+
await startDashboardAgentSession({
342+
chatId,
343+
clientData: {
344+
...clientContext,
345+
organizationId: project.organizationId,
346+
userId,
347+
projectId: project.id,
348+
environmentId: runtimeEnv.id,
349+
environmentName,
350+
},
351+
});
334352
}
335353

336354
const publicAccessToken = await mintDashboardAgentToken(chatId);
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
3+
// The `in` proxy is the one path a browser reaches the agent through, and it injects the turn's
4+
// identity, tenancy and delegated token. Whatever the browser sends must not be able to set any of
5+
// those fields — not by overwriting them, and not by smuggling in a field the server doesn't own.
6+
7+
const mocks = vi.hoisted(() => ({
8+
fetch: vi.fn(),
9+
}));
10+
11+
vi.mock("~/db.server", () => ({ $replica: {} }));
12+
vi.mock("~/env.server", () => ({ env: { SESSION_SECRET: "test-session-secret" } }));
13+
vi.mock("~/services/session.server", () => ({
14+
requireUser: async () => ({ id: "usr_real", admin: false, isImpersonating: false }),
15+
}));
16+
vi.mock("~/v3/canAccessDashboardAgent.server", () => ({
17+
canAccessDashboardAgent: async () => true,
18+
}));
19+
vi.mock("~/models/project.server", () => ({
20+
findProjectBySlug: async () => ({
21+
id: "proj_real",
22+
organizationId: "org_real",
23+
externalRef: "proj_ref_real",
24+
}),
25+
}));
26+
vi.mock("~/models/runtimeEnvironment.server", () => ({
27+
findEnvironmentBySlug: async () => ({ id: "env_real", type: "DEVELOPMENT" }),
28+
}));
29+
vi.mock("~/services/dashboardAgent.server", () => ({
30+
dashboardAgentApiOrigin: () => "https://api.trigger.dev",
31+
dashboardAgentEnvironmentName: () => "dev",
32+
mintDashboardAgentUserActorToken: async () => "tr_uat_real",
33+
resolveDashboardAgentRepoSnapshot: async () => null,
34+
}));
35+
vi.mock("~/services/logger.server", () => ({
36+
logger: { debug: vi.fn(), error: vi.fn(), warn: vi.fn(), info: vi.fn() },
37+
}));
38+
39+
import { action } from "~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.in.$";
40+
41+
async function appendTurn(metadata: Record<string, unknown>): Promise<Record<string, unknown>> {
42+
const request = new Request(
43+
"https://app.trigger.dev/resources/orgs/acme/projects/api/env/dev/dashboard-agent/in/realtime/v1/sessions/chat_1/in/append",
44+
{
45+
method: "POST",
46+
headers: { "content-type": "application/json" },
47+
body: JSON.stringify({
48+
kind: "message",
49+
payload: { metadata, message: { parts: [{ type: "text", text: "hi" }] } },
50+
}),
51+
}
52+
);
53+
54+
const response = await action({
55+
request,
56+
params: {
57+
organizationSlug: "acme",
58+
projectParam: "api",
59+
envParam: "dev",
60+
"*": "realtime/v1/sessions/chat_1/in/append",
61+
},
62+
context: {},
63+
} as any);
64+
65+
expect(response.status).toBe(200);
66+
expect(mocks.fetch).toHaveBeenCalledTimes(1);
67+
const forwarded = JSON.parse(mocks.fetch.mock.calls[0][1].body as string);
68+
return forwarded.payload.metadata as Record<string, unknown>;
69+
}
70+
71+
describe("dashboard agent `in` proxy — client metadata", () => {
72+
beforeEach(() => {
73+
mocks.fetch.mockReset();
74+
mocks.fetch.mockResolvedValue(
75+
new Response(JSON.stringify({ ok: true }), {
76+
status: 200,
77+
headers: { "content-type": "application/json" },
78+
})
79+
);
80+
vi.stubGlobal("fetch", mocks.fetch);
81+
});
82+
83+
it("keeps the whitelisted page context", async () => {
84+
const metadata = await appendTurn({
85+
currentPage: "/orgs/acme/projects/api/env/dev/runs",
86+
pageContext: { kind: "runs" },
87+
});
88+
89+
expect(metadata.currentPage).toBe("/orgs/acme/projects/api/env/dev/runs");
90+
expect(metadata.pageContext).toEqual({ kind: "runs" });
91+
});
92+
93+
it("ignores a client-sent copy of every server-owned field", async () => {
94+
const metadata = await appendTurn({
95+
currentPage: "/runs",
96+
organizationId: "org_evil",
97+
userId: "usr_evil",
98+
projectId: "proj_evil",
99+
projectRef: "proj_ref_evil",
100+
environmentId: "env_evil",
101+
environmentName: "prod",
102+
apiOrigin: "https://evil.example.com",
103+
userActorToken: "tr_uat_evil",
104+
repoSnapshot: { tarballUrl: "https://evil.example.com/x.tar.gz" },
105+
});
106+
107+
expect(metadata.organizationId).toBe("org_real");
108+
expect(metadata.userId).toBe("usr_real");
109+
expect(metadata.projectId).toBe("proj_real");
110+
expect(metadata.projectRef).toBe("proj_ref_real");
111+
expect(metadata.environmentId).toBe("env_real");
112+
expect(metadata.environmentName).toBe("dev");
113+
expect(metadata.apiOrigin).toBe("https://api.trigger.dev");
114+
expect(metadata.userActorToken).toBe("tr_uat_real");
115+
// Not resolved for this project, so the client's pointer must not stand in for it.
116+
expect(metadata.repoSnapshot).toBeUndefined();
117+
});
118+
119+
it("drops any field the server doesn't own", async () => {
120+
const metadata = await appendTurn({
121+
currentPage: "/runs",
122+
evalOptOut: false,
123+
cap: ["admin"],
124+
somethingNew: "smuggled",
125+
});
126+
127+
expect(metadata).not.toHaveProperty("evalOptOut");
128+
expect(metadata).not.toHaveProperty("cap");
129+
expect(metadata).not.toHaveProperty("somethingNew");
130+
});
131+
});

0 commit comments

Comments
 (0)