Skip to content

Commit 3a7d215

Browse files
carderneTrigger.dev RepoOps
authored andcommitted
fix(webapp): harden redirect and rendered output handling
Mono-RevId: 94d1f8a40178cfe84399d958596a96bcb83cfb70
1 parent 13e2bf5 commit 3a7d215

18 files changed

Lines changed: 412 additions & 209 deletions
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import { createElement } from "react";
2+
import { renderToStaticMarkup } from "react-dom/server";
3+
import { describe, expect, it } from "vitest";
4+
import { OperatingSystemContextProvider } from "~/components/primitives/OperatingSystemProvider";
5+
import { ShortcutsProvider } from "~/components/primitives/ShortcutsProvider";
6+
import { DiagnosisDocsAction } from "./RunDiagnosisCard";
7+
8+
function markup(label: string) {
9+
return renderToStaticMarkup(
10+
createElement(
11+
OperatingSystemContextProvider,
12+
{ platform: "mac" },
13+
createElement(
14+
ShortcutsProvider,
15+
null,
16+
createElement(DiagnosisDocsAction, {
17+
action: {
18+
kind: "docs",
19+
label,
20+
to: "https://example.com/help",
21+
destinationHost: "example.com",
22+
},
23+
})
24+
)
25+
)
26+
);
27+
}
28+
29+
describe("diagnosis documentation actions", () => {
30+
it("bounds and isolates an untrusted label without hiding the destination host", () => {
31+
const label = `${"Read documentation ".repeat(20)}\u202eevil.test`;
32+
const html = markup(label);
33+
const labelStart = html.indexOf('<bdi dir="auto"');
34+
const hostStart = html.indexOf('<bdi dir="ltr" class="shrink-0 text-text-dimmed">');
35+
36+
expect(html).toContain('href="https://example.com/help"');
37+
expect(html).toContain('target="_blank"');
38+
expect(html).toContain('rel="noopener noreferrer"');
39+
expect(html).toContain("max-w-[24ch] truncate");
40+
expect(html).toContain(label);
41+
expect(labelStart).toBeGreaterThan(-1);
42+
expect(hostStart).toBeGreaterThan(labelStart);
43+
expect(html.slice(hostStart)).toContain("(example.com)</bdi>");
44+
});
45+
});

‎apps/webapp/app/components/dashboard-agent/RunDiagnosisCard.tsx‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,9 @@ import { useOptionalOrganization } from "~/hooks/useOrganizations";
1212
import { useOptionalProject } from "~/hooks/useProject";
1313
import { cn } from "~/utils/cn";
1414
import { v3RunPath } from "~/utils/pathBuilder";
15-
import { planDiagnosisActions } from "./diagnosis-actions";
15+
import { planDiagnosisActions, type PlannedDiagnosisAction } from "./diagnosis-actions";
1616
import { isRunFriendlyId } from "./run-id";
1717

18-
// No markup comes from the model, so only outbound URLs need checking.
19-
2018
function Em({ children }: { children: React.ReactNode }) {
2119
return <span className="font-semibold text-text-bright">{children}</span>;
2220
}
@@ -129,6 +127,23 @@ function EvidenceReference({ reference }: { reference: string }) {
129127
return <span className="font-mono text-xs text-text-dimmed">{reference}</span>;
130128
}
131129

130+
export function DiagnosisDocsAction({
131+
action,
132+
}: {
133+
action: Extract<PlannedDiagnosisAction, { kind: "docs" }>;
134+
}) {
135+
return (
136+
<LinkButton to={action.to} variant="docs/small" LeadingIcon={BookOpenIcon}>
137+
<bdi dir="auto" className="block min-w-0 max-w-[24ch] truncate">
138+
{action.label}
139+
</bdi>
140+
<bdi dir="ltr" className="shrink-0 text-text-dimmed">
141+
({action.destinationHost})
142+
</bdi>
143+
</LinkButton>
144+
);
145+
}
146+
132147
function DiagnosisActions({ actions }: { actions: NonNullable<DiagnosisBlock["actions"]> }) {
133148
const runPath = useRunPathResolver();
134149
const planned = planDiagnosisActions(actions, {
@@ -141,9 +156,7 @@ function DiagnosisActions({ actions }: { actions: NonNullable<DiagnosisBlock["ac
141156
<div className="flex flex-wrap gap-2 pt-2">
142157
{planned.map((action, i) =>
143158
action.kind === "docs" ? (
144-
<LinkButton key={i} to={action.to} variant="docs/small" LeadingIcon={BookOpenIcon}>
145-
{action.label}
146-
</LinkButton>
159+
<DiagnosisDocsAction key={i} action={action} />
147160
) : (
148161
<LinkButton key={i} to={action.to} variant="primary/small">
149162
{action.label}

‎apps/webapp/app/components/dashboard-agent/diagnosis-actions.test.ts‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,12 @@ describe("planDiagnosisActions", () => {
2727

2828
it("drops only the unresolvable action, keeping the rest", () => {
2929
expect(planDiagnosisActions([viewRun, docs], withoutContext)).toEqual([
30-
{ kind: "docs", label: "Read the docs", to: docs.target },
30+
{
31+
kind: "docs",
32+
label: "Read the docs",
33+
to: docs.target,
34+
destinationHost: "trigger.dev",
35+
},
3136
]);
3237
});
3338

@@ -37,10 +42,35 @@ describe("planDiagnosisActions", () => {
3742
);
3843
});
3944

45+
it("derives the displayed host from the resolved destination", () => {
46+
expect(
47+
planDiagnosisActions(
48+
[{ ...docs, target: "https://example.com/help", label: "trigger.dev documentation" }],
49+
resolve
50+
)
51+
).toEqual([
52+
{
53+
kind: "docs",
54+
label: "trigger.dev documentation",
55+
to: "https://example.com/help",
56+
destinationHost: "example.com",
57+
},
58+
]);
59+
});
60+
4061
it("drops a docs action with an unsafe target", () => {
4162
expect(planDiagnosisActions([{ ...docs, target: "javascript:alert(1)" }], resolve)).toEqual([]);
4263
});
4364

65+
it("drops a docs action when its resolved destination has no hostname", () => {
66+
expect(
67+
planDiagnosisActions([docs], {
68+
...resolve,
69+
docsUrl: () => "blob:https://trigger.dev/id",
70+
})
71+
).toEqual([]);
72+
});
73+
4474
it("drops an action kind it does not know", () => {
4575
expect(planDiagnosisActions([{ ...viewRun, kind: "replay_run" }], resolve)).toEqual([]);
4676
});

‎apps/webapp/app/components/dashboard-agent/diagnosis-actions.ts‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,18 @@ import { isRunFriendlyId } from "./run-id";
22

33
export type DiagnosisActionInput = { kind: string; target: string; label: string };
44

5-
export type PlannedDiagnosisAction = {
6-
kind: "view_run" | "docs";
7-
label: string;
8-
to: string;
9-
};
5+
export type PlannedDiagnosisAction =
6+
| {
7+
kind: "view_run";
8+
label: string;
9+
to: string;
10+
}
11+
| {
12+
kind: "docs";
13+
label: string;
14+
to: string;
15+
destinationHost: string;
16+
};
1017

1118
/**
1219
* An action whose destination can't be resolved is dropped, never rendered as a
@@ -29,7 +36,14 @@ export function planDiagnosisActions(
2936
}
3037
if (action.kind === "docs") {
3138
const to = resolve.docsUrl(action.target);
32-
if (to) planned.push({ kind: "docs", label: action.label, to });
39+
if (!to) continue;
40+
41+
try {
42+
const destinationHost = new URL(to).hostname;
43+
if (destinationHost) {
44+
planned.push({ kind: "docs", label: action.label, to, destinationHost });
45+
}
46+
} catch {}
3347
}
3448
}
3549

‎apps/webapp/app/routes/resources.$projectId.deployments.$deploymentShortCode.promote.ts‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { $replica, prisma } from "~/db.server";
55
import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/message.server";
66
import { logger } from "~/services/logger.server";
77
import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder";
8+
import { sanitizeRedirectPath } from "~/utils";
89
import { ChangeCurrentDeploymentService } from "~/v3/services/changeCurrentDeployment.server";
910

1011
export const promoteSchema = z.object({
@@ -43,6 +44,8 @@ export const action = dashboardAction(
4344
return json(submission.reply());
4445
}
4546

47+
const redirectUrl = sanitizeRedirectPath(submission.value.redirectUrl);
48+
4649
try {
4750
const project = await prisma.project.findFirst({
4851
where: {
@@ -58,7 +61,7 @@ export const action = dashboardAction(
5861
});
5962

6063
if (!project) {
61-
return redirectWithErrorMessage(submission.value.redirectUrl, request, "Project not found");
64+
return redirectWithErrorMessage(redirectUrl, request, "Project not found");
6265
}
6366

6467
const deployment = await prisma.workerDeployment.findFirst({
@@ -69,18 +72,14 @@ export const action = dashboardAction(
6972
});
7073

7174
if (!deployment) {
72-
return redirectWithErrorMessage(
73-
submission.value.redirectUrl,
74-
request,
75-
"Deployment not found"
76-
);
75+
return redirectWithErrorMessage(redirectUrl, request, "Deployment not found");
7776
}
7877

7978
const promoteService = new ChangeCurrentDeploymentService();
8079
await promoteService.call(deployment, "promote");
8180

8281
return redirectWithSuccessMessage(
83-
submission.value.redirectUrl,
82+
redirectUrl,
8483
request,
8584
`Promoted deployment version ${deployment.version} to current.`
8685
);

‎apps/webapp/app/routes/resources.$projectId.deployments.$deploymentShortCode.rollback.ts‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { $replica, prisma } from "~/db.server";
55
import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/message.server";
66
import { logger } from "~/services/logger.server";
77
import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder";
8+
import { sanitizeRedirectPath } from "~/utils";
89
import { ChangeCurrentDeploymentService } from "~/v3/services/changeCurrentDeployment.server";
910

1011
export const rollbackSchema = z.object({
@@ -43,6 +44,8 @@ export const action = dashboardAction(
4344
return json(submission.reply());
4445
}
4546

47+
const redirectUrl = sanitizeRedirectPath(submission.value.redirectUrl);
48+
4649
try {
4750
const project = await prisma.project.findFirst({
4851
where: {
@@ -58,7 +61,7 @@ export const action = dashboardAction(
5861
});
5962

6063
if (!project) {
61-
return redirectWithErrorMessage(submission.value.redirectUrl, request, "Project not found");
64+
return redirectWithErrorMessage(redirectUrl, request, "Project not found");
6265
}
6366

6467
const deployment = await prisma.workerDeployment.findFirst({
@@ -69,21 +72,13 @@ export const action = dashboardAction(
6972
});
7073

7174
if (!deployment) {
72-
return redirectWithErrorMessage(
73-
submission.value.redirectUrl,
74-
request,
75-
"Deployment not found"
76-
);
75+
return redirectWithErrorMessage(redirectUrl, request, "Deployment not found");
7776
}
7877

7978
const rollbackService = new ChangeCurrentDeploymentService();
8079
await rollbackService.call(deployment, "rollback");
8180

82-
return redirectWithSuccessMessage(
83-
submission.value.redirectUrl,
84-
request,
85-
"Rolled back deployment"
86-
);
81+
return redirectWithSuccessMessage(redirectUrl, request, "Rolled back deployment");
8782
} catch (error) {
8883
if (error instanceof Error) {
8984
logger.error("Failed to roll back deployment", {

‎apps/webapp/app/routes/resources.feedback.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { uiComponent } from "@team-plain/ui-components";
44
import { z } from "zod";
55
import { redirectWithSuccessMessage } from "~/models/message.server";
66
import { requireUser } from "~/services/session.server";
7+
import { sanitizeRedirectPath } from "~/utils";
78
import { sendToPlain } from "~/utils/plain.server";
89

910
export const feedbackTypes = {
@@ -77,6 +78,7 @@ export async function action({ request }: ActionFunctionArgs) {
7778
}
7879

7980
const inquiry = feedbackTypes[submission.value.feedbackType as FeedbackType];
81+
const redirectPath = sanitizeRedirectPath(submission.value.path);
8082
try {
8183
await sendToPlain({
8284
userId: user.id,
@@ -110,7 +112,7 @@ export async function action({ request }: ActionFunctionArgs) {
110112
});
111113

112114
return redirectWithSuccessMessage(
113-
submission.value.path,
115+
redirectPath,
114116
request,
115117
"Thanks for your feedback! We'll get back to you soon."
116118
);

‎apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.ai-filter.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ export async function action({ request, params }: ActionFunctionArgs) {
154154

155155
const [error, result] = await tryCatch(service.call(text, environment.id));
156156
if (error) {
157-
return json({ success: false, error: error.message }, { status: 400 });
157+
return json({ success: false, error: "Unable to create filters" }, { status: 400 });
158158
}
159159

160160
return json(result);

‎apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ import { RUNS_BULK_INSPECTOR_UI_SEARCH_PARAMS } from "~/routes/_app.orgs.$organi
5858
import { logger } from "~/services/logger.server";
5959
import { dashboardAction, dashboardLoader } from "~/services/routeBuilders/dashboardBuilder";
6060
import { checkPermissions } from "~/services/routeBuilders/permissions.server";
61+
import { sanitizeRedirectPath } from "~/utils";
6162
import { cn } from "~/utils/cn";
6263
import { EnvironmentParamSchema, v3BulkActionPath } from "~/utils/pathBuilder";
6364
import { BulkActionService } from "~/v3/services/bulk/BulkActionV2.server";
@@ -187,6 +188,8 @@ export const action = dashboardAction(
187188
return redirectWithErrorMessage("/", request, "Invalid bulk action");
188189
}
189190

191+
const failedRedirect = sanitizeRedirectPath(submission.value.failedRedirect);
192+
190193
// "Don't override" keeps each run's original region — drop it so it isn't
191194
// stored as a real override.
192195
if (submission.value.region === REPLAY_REGION_NO_OVERRIDE_VALUE) {
@@ -222,7 +225,7 @@ export const action = dashboardAction(
222225
});
223226

224227
return redirectWithErrorMessage(
225-
submission.value.failedRedirect,
228+
failedRedirect,
226229
request,
227230
`Failed to create bulk action: ${error.message}`
228231
);

‎apps/webapp/app/routes/resources.sessions.$sessionParam.close.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/m
66
import { resolveSessionByIdOrExternalId } from "~/services/realtime/sessions.server";
77
import { logger } from "~/services/logger.server";
88
import { requireUserId } from "~/services/session.server";
9+
import { sanitizeRedirectPath } from "~/utils";
910

1011
export const closeSessionSchema = z.object({
1112
redirectUrl: z.string(),
@@ -28,7 +29,8 @@ export const action: ActionFunction = async ({ request, params }) => {
2829
return json(submission.reply());
2930
}
3031

31-
const { redirectUrl, environmentId, reason } = submission.value;
32+
const { environmentId, reason } = submission.value;
33+
const redirectUrl = sanitizeRedirectPath(submission.value.redirectUrl);
3234
const trimmedReason = reason?.trim();
3335
const closedReason =
3436
trimmedReason && trimmedReason.length > 0 ? trimmedReason : "closed-from-dashboard";

0 commit comments

Comments
 (0)