Skip to content

Commit

Permalink
fix: generatePath incorrectly applying parameters remix-run#9051
Browse files Browse the repository at this point in the history
generatePath was doing multiple passes on the `path` using string replace, the first two passes were applying parameters, the third pass was doing a cleanup and the fourth path was applying the `splat`.
It was possible to get incorrect results while applying `splat` when the last parameter value ended with `*`:

```ts
const path = generatePath("/route/:name", {
    name: encodeURIComponent("includes *asterisk at the end*"),
})
```
```
    Expected: "/route/includes *asterisk at the end*"
    Received: "/route/includes *asterisk at the end"
```
results of the first two passes return the value of `/route/*asterisk at the end*` which was later treated as path with the splat resulting in the last asterisk removed.

it was also possible to inject the splat value unintentionally
```ts
generatePath("/courses/:name", { name: "foo*", "*": "splat_should_not_be_added" })
```
```
    Expected: "/courses/foo*"
    Received: "/courses/foosplat_should_not_be_added"
```

A safer option, instead of mutating a global path multiple times, is to split the path into segments, process each segment in isolation and then join them back together.

fixes remix-run#9051
  • Loading branch information
Obi-Dann committed Feb 9, 2023
1 parent 918b158 commit 4440090
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 40 deletions.
5 changes: 5 additions & 0 deletions .changeset/old-camels-accept.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-router": patch
---

Fix `generatePath` incorrectly applying parameters in some cases
1 change: 1 addition & 0 deletions contributors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
- ms10596
- ned-park
- noisypigeon
- Obi-Dann
- omar-moquete
- p13i
- parched
Expand Down
14 changes: 14 additions & 0 deletions packages/react-router/__tests__/generatePath-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@ describe("generatePath", () => {
);
expect(generatePath("/*", {})).toBe("/");
});
it("handles * in parameter values", () => {
expect(generatePath("/courses/:name", { name: "foo*" })).toBe(
"/courses/foo*"
);
expect(generatePath("/courses/:name", { name: "*foo" })).toBe(
"/courses/*foo"
);
expect(generatePath("/courses/:name", { name: "*f*oo*" })).toBe(
"/courses/*f*oo*"
);
expect(generatePath("/courses/:name", { name: "foo*", "*": "splat_should_not_be_added" })).toBe(
"/courses/foo*"
);
});
});

describe("with extraneous params", () => {
Expand Down
8 changes: 8 additions & 0 deletions packages/react-router/__tests__/matchPath-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,14 @@ describe("matchPath *", () => {
pathnameBase: "/",
});
});

it("resolves params containing '*' correctly", () => {
expect(matchPath("/users/:name/*", "/users/foo*/splat")).toMatchObject({
params: { name: "foo*", "*": "splat" },
pathname: "/users/foo*/splat",
pathnameBase: "/users/foo*",
});
});
});

describe("matchPath warnings", () => {
Expand Down
77 changes: 37 additions & 40 deletions packages/router/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ export function generatePath<Path extends string>(
[key in PathParam<Path>]: string | null;
} = {} as any
): string {
let path = originalPath;
let path: string = originalPath;
if (path.endsWith("*") && path !== "*" && !path.endsWith("/*")) {
warning(
false,
Expand All @@ -633,49 +633,46 @@ export function generatePath<Path extends string>(
path = path.replace(/\*$/, "/*") as Path;
}

return (
path
.replace(
/^:(\w+)(\??)/g,
(_, key: PathParam<Path>, optional: string | undefined) => {
let param = params[key];
if (optional === "?") {
return param == null ? "" : param;
}
if (param == null) {
invariant(false, `Missing ":${key}" param`);
}
return param;
}
)
.replace(
/\/:(\w+)(\??)/g,
(_, key: PathParam<Path>, optional: string | undefined) => {
let param = params[key];
if (optional === "?") {
return param == null ? "" : `/${param}`;
}
if (param == null) {
invariant(false, `Missing ":${key}" param`);
}
return `/${param}`;
}
)
// Remove any optional markers from optional static segments
.replace(/\?/g, "")
.replace(/(\/?)\*/, (_, prefix, __, str) => {
// ensure `/` is added at the beginning if the path is absolute
const prefix = path.startsWith("/") ? "/" : "";

const segments = path
.split(/\/+/)
.map((segment, index, array) => {
const isLastSegment = index === array.length - 1;

// only apply the splat if it's the last segment
if (isLastSegment && segment === "*") {
const star = "*" as PathParam<Path>;
const starParam = params[star];

if (params[star] == null) {
// If no splat was provided, trim the trailing slash _unless_ it's
// the entire path
return str === "/*" ? "/" : "";
// Apply the splat
return starParam;
}

const keyMatch = segment.match(/^:(\w+)(\??)$/);
if (keyMatch) {
const [, key, optional] = keyMatch;
let param = params[key as PathParam<Path>];

if (optional === "?") {
return param == null ? "" : param;
}

// Apply the splat
return `${prefix}${params[star]}`;
})
);
if (param == null) {
invariant(false, `Missing ":${key}" param`);
}

return param;
}

// Remove any optional markers from optional static segments
return segment.replace(/\?$/g, "");
})
// Remove empty segments
.filter((segment) => !!segment);

return prefix + segments.join("/");
}

/**
Expand Down

0 comments on commit 4440090

Please sign in to comment.