Skip to content

Commit 8bb48c9

Browse files
MariefayTrigger.dev RepoOps
authored andcommitted
fix(webapp): stop offloaded payload and output downloads returning an error file
Fix the dashboard "Download payload" and "Download output" buttons for large payloads and outputs, which downloaded an error file instead of the data. A failed download now shows an error instead of saving a file. Mono-RevId: 1d354a29adfdb23c9fd1ebcd3a52c14b4b5dea55
1 parent b6a4761 commit 8bb48c9

5 files changed

Lines changed: 232 additions & 4 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
area: webapp
3+
type: fix
4+
---
5+
6+
Fix the "Download payload" and "Download output" buttons for large payloads and outputs, which downloaded an error file instead of the data.
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
// @vitest-environment jsdom
2+
import { createElement } from "react";
3+
import { createRoot, type Root } from "react-dom/client";
4+
import { act } from "react-dom/test-utils";
5+
import { MemoryRouter } from "react-router-dom";
6+
import { afterEach, describe, expect, it } from "vitest";
7+
import { OperatingSystemContextProvider } from "~/components/primitives/OperatingSystemProvider";
8+
import { ShortcutsProvider } from "~/components/primitives/ShortcutsProvider";
9+
import { LinkButton, newTabRel } from "./Buttons";
10+
11+
let container: HTMLDivElement | undefined;
12+
let root: Root | undefined;
13+
14+
afterEach(() => {
15+
if (root) {
16+
act(() => root!.unmount());
17+
}
18+
container?.remove();
19+
container = undefined;
20+
root = undefined;
21+
});
22+
23+
function renderLinkButton(props: Parameters<typeof LinkButton>[0]) {
24+
container = document.createElement("div");
25+
document.body.appendChild(container);
26+
root = createRoot(container);
27+
act(() => {
28+
root!.render(
29+
createElement(
30+
MemoryRouter,
31+
null,
32+
createElement(
33+
OperatingSystemContextProvider,
34+
{ platform: "mac" },
35+
createElement(ShortcutsProvider, null, createElement(LinkButton, props))
36+
)
37+
)
38+
);
39+
});
40+
const anchor = container.querySelector("a");
41+
if (!anchor) throw new Error("LinkButton did not render an anchor");
42+
return anchor;
43+
}
44+
45+
describe("LinkButton /resources targets", () => {
46+
it("keeps an offloaded packet href exactly as built", () => {
47+
const to = "/resources/packets/env1/s3://run_abc/output.json";
48+
const anchor = renderLinkButton({ to, variant: "secondary/small", download: true });
49+
50+
expect(anchor.getAttribute("href")).toBe(to);
51+
expect(anchor.getAttribute("rel")).toBe("noopener");
52+
});
53+
54+
it("opens in a new tab, downloads, and keeps the Referer", () => {
55+
const anchor = renderLinkButton({
56+
to: "/resources/runs/run_1/logs/download",
57+
variant: "secondary/small",
58+
download: true,
59+
});
60+
61+
expect(anchor.getAttribute("href")).toBe("/resources/runs/run_1/logs/download");
62+
expect(anchor.getAttribute("target")).toBe("_blank");
63+
expect(anchor.hasAttribute("download")).toBe(true);
64+
expect(anchor.getAttribute("rel")).toBe("noopener");
65+
});
66+
});
67+
68+
describe("newTabRel", () => {
69+
it("drops the Referer for protocol-relative and absolute URLs", () => {
70+
expect(newTabRel("//example.com/x")).toBe("noopener noreferrer");
71+
expect(newTabRel("https://example.com/x")).toBe("noopener noreferrer");
72+
});
73+
});

‎apps/webapp/app/components/primitives/Buttons.tsx‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -504,10 +504,9 @@ export const LinkButton = ({
504504

505505
if (to.toString().startsWith("/resources")) {
506506
return (
507-
<Link
508-
to={to}
507+
<a
508+
href={to.toString()}
509509
ref={innerRef}
510-
reloadDocument
511510
target="_blank"
512511
rel={newTabRel(to.toString())}
513512
className={cn("group/button block focus-custom", props.fullWidth ? "w-full" : "")}
@@ -519,7 +518,7 @@ export const LinkButton = ({
519518
aria-label={ariaLabel}
520519
>
521520
<ButtonContent {...props} />
522-
</Link>
521+
</a>
523522
);
524523
}
525524

‎apps/webapp/app/routes/resources.packets.$environmentId.$.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { LoaderFunctionArgs } from "@remix-run/node";
22
import { basename } from "node:path";
33
import { z } from "zod";
44
import { prisma } from "~/db.server";
5+
import { logger } from "~/services/logger.server";
56
import { requireUserId } from "~/services/session.server";
67
import { generatePresignedRequest } from "~/v3/objectStore.server";
78

@@ -49,6 +50,14 @@ export async function loader({ request, params }: LoaderFunctionArgs) {
4950
headers: signed.request.headers,
5051
});
5152

53+
if (!response.ok) {
54+
response.body?.cancel().catch(() => {});
55+
logger.warn("Packet download failed", { environmentId, status: response.status });
56+
return new Response(response.status === 404 ? "Not found" : "Failed to download packet", {
57+
status: response.status,
58+
});
59+
}
60+
5261
return new Response(response.body, {
5362
status: 200,
5463
headers: {
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
import {
2+
createStandalonePostgresContainer,
3+
MinIOContainer,
4+
type StartedMinIOContainer,
5+
} from "@internal/testcontainers";
6+
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
7+
import { seedTestEnvironment } from "./helpers/seedTestEnvironment";
8+
import { seedTestUser } from "./helpers/seedTestSession";
9+
10+
import type * as Route from "~/routes/resources.packets.$environmentId.$";
11+
import type * as Database from "~/db.server";
12+
import type * as SessionStorage from "~/services/sessionStorage.server";
13+
import type * as ObjectStore from "~/v3/objectStore.server";
14+
15+
vi.setConfig({ testTimeout: 60_000 });
16+
17+
let container: Awaited<ReturnType<typeof createStandalonePostgresContainer>>;
18+
let minio: StartedMinIOContainer;
19+
let loader: typeof Route.loader;
20+
let database: typeof Database;
21+
let sessionStorage: typeof SessionStorage;
22+
let objectStore: typeof ObjectStore;
23+
24+
beforeAll(async () => {
25+
container = await createStandalonePostgresContainer();
26+
minio = await new MinIOContainer().start();
27+
const store = minio.getConnectionConfig();
28+
29+
vi.stubEnv("DATABASE_URL", container.url);
30+
vi.stubEnv("CONTROL_PLANE_DATABASE_URL", container.url);
31+
vi.stubEnv("DATABASE_READ_REPLICA_URL", container.url);
32+
vi.stubEnv("CONTROL_PLANE_DATABASE_READ_REPLICA_URL", container.url);
33+
vi.stubEnv("RBAC_FORCE_FALLBACK", "1");
34+
vi.stubEnv("ENCRYPTION_KEY", "test-encryption-key-for-e2e!!!!!");
35+
for (const prefix of ["OBJECT_STORE_", "OBJECT_STORE_S3_"]) {
36+
vi.stubEnv(`${prefix}BASE_URL`, store.baseUrl);
37+
vi.stubEnv(`${prefix}ACCESS_KEY_ID`, store.accessKeyId);
38+
vi.stubEnv(`${prefix}SECRET_ACCESS_KEY`, store.secretAccessKey);
39+
vi.stubEnv(`${prefix}REGION`, store.region);
40+
vi.stubEnv(`${prefix}SERVICE`, "s3");
41+
}
42+
43+
database = await import("~/db.server");
44+
({ loader } = await import("~/routes/resources.packets.$environmentId.$"));
45+
sessionStorage = await import("~/services/sessionStorage.server");
46+
objectStore = await import("~/v3/objectStore.server");
47+
}, 120_000);
48+
49+
afterAll(async () => {
50+
await database?.$replica.$disconnect();
51+
await database?.prisma.$disconnect();
52+
await container?.container.stop();
53+
await minio?.stop({ timeout: 0 });
54+
vi.unstubAllEnvs();
55+
});
56+
57+
beforeEach(async () => {
58+
await minio.resetBucket();
59+
});
60+
61+
async function seedMember() {
62+
const { organization, project, environment } = await seedTestEnvironment(database.prisma);
63+
const user = await seedTestUser(database.prisma);
64+
await database.prisma.orgMember.create({
65+
data: { userId: user.id, organizationId: organization.id, role: "MEMBER" },
66+
});
67+
const session = await sessionStorage.getSession();
68+
session.set("user", { userId: user.id });
69+
const cookie = (await sessionStorage.commitSession(session)).split(";")[0];
70+
return { project, environment, cookie };
71+
}
72+
73+
async function putPacket(
74+
projectRef: string,
75+
envSlug: string,
76+
filename: string,
77+
body: string,
78+
forceNoPrefix = false
79+
) {
80+
const signed = await objectStore.generatePresignedRequest(projectRef, envSlug, filename, "PUT", {
81+
forceNoPrefix,
82+
});
83+
if (!signed.success) throw new Error(signed.error);
84+
const response = await fetch(signed.request.url, {
85+
method: "PUT",
86+
headers: signed.request.headers,
87+
body,
88+
});
89+
expect(response.status).toBe(200);
90+
}
91+
92+
function download(environmentId: string, packet: string, cookie: string) {
93+
return loader({
94+
request: new Request(`https://app.example.com/resources/packets/${environmentId}/${packet}`, {
95+
headers: { Cookie: cookie },
96+
}),
97+
params: { environmentId, "*": packet },
98+
context: {},
99+
});
100+
}
101+
102+
describe("resources.packets download", () => {
103+
it("returns an offloaded s3:// packet as an attachment", async () => {
104+
const { project, environment, cookie } = await seedMember();
105+
await putPacket(project.externalRef, environment.slug, "s3://run_abc/output.json", '{"ok":1}');
106+
107+
const response = await download(environment.id, "s3://run_abc/output.json", cookie);
108+
109+
expect(response.status).toBe(200);
110+
expect(response.headers.get("Content-Disposition")).toBe('attachment; filename="output.json"');
111+
expect(await response.text()).toBe('{"ok":1}');
112+
});
113+
114+
it("returns a legacy unprefixed packet", async () => {
115+
const { project, environment, cookie } = await seedMember();
116+
await putPacket(
117+
project.externalRef,
118+
environment.slug,
119+
"batch_123/item_0/payload.json",
120+
'{"legacy":true}',
121+
true
122+
);
123+
124+
const response = await download(environment.id, "batch_123/item_0/payload.json", cookie);
125+
126+
expect(response.status).toBe(200);
127+
expect(await response.text()).toBe('{"legacy":true}');
128+
});
129+
130+
it("reports a missing packet as 404 instead of downloading the store's error", async () => {
131+
const { environment, cookie } = await seedMember();
132+
133+
const response = await download(environment.id, "s3://run_missing/output.json", cookie);
134+
const body = await response.text();
135+
136+
expect(response.status).toBe(404);
137+
expect(response.headers.get("Content-Disposition")).toBeNull();
138+
expect(body).not.toContain("<?xml");
139+
expect(body).not.toContain("NoSuchKey");
140+
});
141+
});

0 commit comments

Comments
 (0)