Skip to content

Commit

Permalink
fix(router): url encode file names
Browse files Browse the repository at this point in the history 
This is the RR port of remix-run/remix#4473

Bring FormData serialization in line with the spec with respect to File entries
in url-encoded payloads: send the `name` as the value, as is the case with a
vanilla `<form>` submission.

Rework various 400-error tests not to rely on the old behavior, as it's no longer
a bad request :)

References: remix-run/remix#4342
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs
  • Loading branch information
@jenseng
jenseng committed Jan 12, 2023
1 parent 9640d01 commit bf281ed
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 122 deletions.
5 changes: 5 additions & 0 deletions .changeset/sweet-swans-cry.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

Send the name as the value when url-encoding File form data entries
147 changes: 51 additions & 96 deletions packages/router/__tests__/router-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4493,7 +4493,7 @@ describe("a router", () => {
);
});

it("returns a 400 error if binary data is attempted to be submitted using formMethod=GET", async () => {
it("url-encodes File names on GET submissions", async () => {
let t = setup({
routes: TASK_ROUTES,
initialEntries: ["/"],
Expand All @@ -4510,26 +4510,17 @@ describe("a router", () => {
"blob",
new Blob(["<h1>Some html file contents</h1>"], {
type: "text/html",
})
}),
"blob.html"
);

await t.navigate("/tasks", {
let A = await t.navigate("/tasks", {
formMethod: "get",
formData: formData,
});
expect(t.router.state.navigation.state).toBe("idle");
expect(t.router.state.location).toMatchObject({
pathname: "/tasks",
search: "",
});
expect(t.router.state.errors).toEqual({
tasks: new ErrorResponse(
400,
"Bad Request",
new Error("Cannot submit binary form data using GET"),
true
),
});
let params = new URL(A.loaders.tasks.stub.mock.calls[0][0].request.url)
.searchParams;
expect(params.get("blob")).toEqual("blob.html");
});

it("returns a 405 error if attempting to submit with method=HEAD", async () => {
Expand Down Expand Up @@ -4611,61 +4602,6 @@ describe("a router", () => {
),
});
});

it("runs loaders above the boundary for 400 errors if binary data is attempted to be submitted using formMethod=GET", async () => {
let t = setup({
routes: [
{
id: "index",
index: true,
},
{
id: "parent",
path: "parent",
loader: true,
children: [
{
id: "child",
path: "child",
loader: true,
hasErrorBoundary: true,
},
],
},
],
initialEntries: ["/"],
});

let formData = new FormData();
formData.append(
"blob",
new Blob(["<h1>Some html file contents</h1>"], {
type: "text/html",
})
);

let A = await t.navigate("/parent/child", {
formMethod: "get",
formData: formData,
});
expect(t.router.state.navigation.state).toBe("loading");
expect(t.router.state.errors).toEqual(null);

await A.loaders.parent.resolve("PARENT");
expect(A.loaders.child.stub).not.toHaveBeenCalled();
expect(t.router.state.navigation.state).toBe("idle");
expect(t.router.state.loaderData).toEqual({
parent: "PARENT",
});
expect(t.router.state.errors).toEqual({
child: new ErrorResponse(
400,
"Bad Request",
new Error("Cannot submit binary form data using GET"),
true
),
});
});
});

describe("data loading (new)", () => {
Expand Down Expand Up @@ -5710,6 +5646,37 @@ describe("a router", () => {
);
});

it("url-encodes File names on x-www-form-urlencoded submissions", async () => {
let t = setup({
routes: [
{
id: "root",
path: "/",
action: true,
},
],
initialEntries: ["/"],
hydrationData: {
loaderData: {
root: "ROOT_DATA",
},
},
});

let fd = new FormData();
fd.append("key", "value");
fd.append("file", new Blob(["1", "2", "3"]), "file.txt");

let A = await t.navigate("/", {
formMethod: "post",
formEncType: "application/x-www-form-urlencoded",
formData: fd,
});

let req = A.actions.root.stub.mock.calls[0][0].request.clone();
expect((await req.formData()).get("file")).toEqual("file.txt");
});

it("races actions and loaders against abort signals", async () => {
let loaderDfd = createDeferred();
let actionDfd = createDeferred();
Expand Down Expand Up @@ -9909,6 +9876,7 @@ describe("a router", () => {
{
id: "b",
path: "b",
loader: true,
},
],
},
Expand Down Expand Up @@ -9947,12 +9915,11 @@ describe("a router", () => {
// Perform an invalid navigation to /parent/b which will be handled
// using parent's error boundary. Parent's deferred should be left alone
// while A's should be cancelled since they will no longer be rendered
let formData = new FormData();
formData.append("file", new Blob(["1", "2"]), "file.txt");
await t.navigate("/parent/b", {
formMethod: "get",
formData,
});
let B = await t.navigate("/parent/b");
await B.loaders.b.reject(
new Response("broken", { status: 400, statusText: "Bad Request" })
);

// Navigation completes immediately with an error at the boundary
expect(t.router.state.loaderData).toEqual({
parent: {
Expand All @@ -9961,12 +9928,7 @@ describe("a router", () => {
},
});
expect(t.router.state.errors).toEqual({
parent: new ErrorResponse(
400,
"Bad Request",
new Error("Cannot submit binary form data using GET"),
true
),
parent: new ErrorResponse(400, "Bad Request", "broken", false),
});

await parentDfd.resolve("Yep!");
Expand Down Expand Up @@ -10047,12 +10009,11 @@ describe("a router", () => {
// Perform an invalid navigation to /b/child which should cancel all
// pending deferred's since nothing is reused. It should not call bChild's
// loader since it's below the boundary but should call b's loader.
let formData = new FormData();
formData.append("file", new Blob(["1", "2"]), "file.txt");
let B = await t.navigate("/b/child", {
formMethod: "get",
formData,
});
let B = await t.navigate("/b/child");

await B.loaders.bChild.reject(
new Response("broken", { status: 400, statusText: "Bad Request" })
);

// Both should be cancelled
await aDfd.resolve("Nope!");
Expand All @@ -10073,14 +10034,8 @@ describe("a router", () => {
b: "B LOADER",
});
expect(t.router.state.errors).toEqual({
bChild: new ErrorResponse(
400,
"Bad Request",
new Error("Cannot submit binary form data using GET"),
true
),
bChild: new ErrorResponse(400, "Bad Request", "broken", false),
});
expect(B.loaders.bChild.stub).not.toHaveBeenCalled();
});

it("does not cancel pending deferreds on hash change only navigations", async () => {
Expand Down
35 changes: 9 additions & 26 deletions packages/router/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2588,25 +2588,14 @@ function normalizeNavigateOptions(

// Flatten submission onto URLSearchParams for GET submissions
let parsedPath = parsePath(path);
try {
let searchParams = convertFormDataToSearchParams(opts.formData);
// Since fetcher GET submissions only run a single loader (as opposed to
// navigation GET submissions which run all loaders), we need to preserve
// any incoming ?index params
if (
isFetcher &&
parsedPath.search &&
hasNakedIndexQuery(parsedPath.search)
) {
searchParams.append("index", "");
}
parsedPath.search = `?${searchParams}`;
} catch (e) {
return {
path,
error: getInternalRouterError(400),
};
let searchParams = convertFormDataToSearchParams(opts.formData);
// Since fetcher GET submissions only run a single loader (as opposed to
// navigation GET submissions which run all loaders), we need to preserve
// any incoming ?index params
if (isFetcher && parsedPath.search && hasNakedIndexQuery(parsedPath.search)) {
searchParams.append("index", "");
}
parsedPath.search = `?${searchParams}`;

return { path: createPath(parsedPath), submission };
}
Expand Down Expand Up @@ -2955,12 +2944,8 @@ function convertFormDataToSearchParams(formData: FormData): URLSearchParams {
let searchParams = new URLSearchParams();

for (let [key, value] of formData.entries()) {
invariant(
typeof value === "string",
'File inputs are not supported with encType "application/x-www-form-urlencoded", ' +
'please use "multipart/form-data" instead.'
);
searchParams.append(key, value);
// https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#converting-an-entry-list-to-a-list-of-name-value-pairs
searchParams.append(key, value instanceof File ? value.name : value);
}

return searchParams;
Expand Down Expand Up @@ -3223,8 +3208,6 @@ function getInternalRouterError(
`so there is no way to handle the request.`;
} else if (type === "defer-action") {
errorMessage = "defer() is not supported in actions";
} else {
errorMessage = "Cannot submit binary form data using GET";
}
} else if (status === 403) {
statusText = "Forbidden";
Expand Down

0 comments on commit bf281ed

Please sign in to comment.