Path to regexp fix #11965

hkbertoson · 2024-09-10T14:52:45Z

Changes

What does this change? Updated path-regexp-fix to latest version
Be short and concise. Bullet points can help!
Before/after screenshots can help as well.
Don't forget a changeset! pnpm exec changeset

Testing

Docs

changeset-bot · 2024-09-10T14:52:50Z

🦋 Changeset detected

Latest commit: a06cfaa

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

uwej711 · 2024-09-10T17:29:31Z

This is not enough, the template building in getRouteGenerator needs to be changed as (.*)? is no longer supported ... the closest to use is the wildcard, but that would require an array for the segments.

hkbertoson · 2024-09-10T17:39:01Z

Let me take a look at that and see!

hkbertoson · 2024-09-10T23:04:13Z

This is not enough, the template building in getRouteGenerator needs to be changed as (.*)? is no longer supported ... the closest to use is the wildcard, but that would require an array for the segments.

I updated the generator function. I tested it in several different scenarios and it seems to be working everywhere.

delucis · 2024-09-11T08:31:41Z

packages/astro/src/core/routing/manifest/generator.ts

@@ -31,7 +33,7 @@ export function getRouteGenerator(
 				segment
 					.map((part) => {
 						if (part.spread) {
-							return `:${part.content.slice(3)}(.*)?`;
+							return `${part.content.slice(3)}`;


This removes this part from being a parameter understood by path-to-regexp to just being a literal, e.g. if the route was [...slug], previously this template literal created :slug(.*) but after this change it just creates slug. That’s probably why a lot of tests are failing.

We’ll need to check how to match the (.*) behaviour in the latest version of path-to-regexp (if possible).

From taking a look at the docs, it seems like this might be the way?

Suggested change

return `${part.content.slice(3)}`;

return `*${part.content.slice(3)}`;

In a quick sandbox I spun up I ran this code:

const { compile } = require('path-to-regexp'); const toPath = compile('/*spread/:slug'); const path = toPath({ spread: ['one','two','three'], slug: 'four' }); console.log(path);

Which logged /one/two/three/four as expected. We’d still need to see if it fully matches all behaviour specified in tests though or if there are edge cases.

I tried this as well. It errors out and says "expected "slug" to be a non-empty array"

Ah, I guess currently we allow [...spread] to match nothing (i.e. [...spread] can match /) and *spread in path-to-regexp doesn’t include support for an optional segment. They do mention {} for optional segments, could try combining that with the wildcard?

Then there is still the issue with the actual parameter value for rest (...) parameters, today it's "somedir/file.ext" and with the new version it should be ["somedir", "file.ext"], i.e. the parameter value needs to be split.

Ah, when I did a quick investigation it looked like path-to-regexp already expected an array in v6, so assumed that hadn’t changed, but I may have misunderstood.

Okay. I made some changes and im testing it all now to see.

Co-authored-by: Chris Swithinbank <[email protected]>

Sanitize parameters, preserve arrays for spread params like `slug`. Update type definitions and handle string-to-array conversion.

uwej711 · 2024-09-11T16:40:10Z

packages/astro/src/core/routing/manifest/generator.ts

@@ -31,7 +37,7 @@ export function getRouteGenerator(
 				segment
 					.map((part) => {
 						if (part.spread) {
-							return `:${part.content.slice(3)}(.*)?`;
+							return `*${part.content.slice(3)}`;


I think it should be

return `{*${part.content.slice(3)}}`;

Or

return `*{${part.content.slice(3)}}`;

Looks like none of those is working ...

uwej711 · 2024-09-11T16:41:41Z

packages/astro/src/core/routing/manifest/generator.ts

+	return (params: Record<string, string[] | number | undefined>): string => {
+
+		// Convert `slug` to an array if it's a string
+		if (typeof params.slug === 'string') {


The array conversion should only happen for parameters which RoutePart has spread set to true. And it should split the string if it contains a /.

blakeembrey · 2024-09-11T23:29:44Z

Couple of things to try and help if someone is attempting this:

but that would require an array for the segments.

Set encode = false, it'll accept only strings again (and assume you've encoded it correctly beforehand).

You might also find generating TokenData directly a bit simpler: https://github.com/pillarjs/path-to-regexp?tab=readme-ov-file#developers. It was added for this use-case, so people don't need to try and generate strings that might not be safe themselves. That would look more like:

const path = new TokenData([
  { type: "text", value: "/" },
 ...segments.map(segment => {
  if (part.spread) return { type: "wildcard", name: part.content }; // Wrap in `type: "group"` if you truly want it to be optional but that seems off to me, since it'd allow `//` with nothing in between?
  if (part.dynamic) return { type: "param", name: part.content };
  return { type: "text", value: part.content.normalize() }
  });
]);

uwej711 · 2024-09-12T21:23:02Z

I tried the update to and in the end think that what we used path-to-regexp for can be simply implemented without, see #11981. Maybe I overlooked some cases but all the tests are green locally.

blakeembrey · 2024-09-12T21:31:04Z

Good call, if you’re never using the template strings you don’t need the library. That’s basically what compile is doing already.

hkbertoson · 2024-09-12T21:37:14Z

That actually makes sense. I was planning on looking to see if we could do that. Glad to see it should be working.

Princesseuh · 2024-09-13T10:51:21Z

Superceded by #11983, thank you for contributing nonetheless!

hkbertoson added 2 commits September 10, 2024 09:46

Updated packages. path-to-regexp to latest version

f4fa4d1

Changeset

02a0340

github-actions bot added pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release labels Sep 10, 2024

github-actions bot requested changes Sep 10, 2024

View reviewed changes

hkbertoson mentioned this pull request Sep 10, 2024

Astro relies on vulnerable path-to-regexp #11956

Closed

1 task

Updated generator function

45b7e2a

delucis reviewed Sep 11, 2024

View reviewed changes

hkbertoson and others added 2 commits September 11, 2024 04:39

Update generator.ts

fd0c086

Co-authored-by: Chris Swithinbank <[email protected]>

Refactor parameter sanitization and array preservation

0e07ae7

Sanitize parameters, preserve arrays for spread params like `slug`. Update type definitions and handle string-to-array conversion.

uwej711 reviewed Sep 11, 2024

View reviewed changes

Alex-Toucan mentioned this pull request Sep 12, 2024

astro-4.15.4.tgz: 1 vulnerabilities (highest severity is: 7.5) Alex-Toucan/atproducts.xyz#274

Closed

Merge branch 'main' into path-to-regexp-fix

a06cfaa

Princesseuh closed this Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path to regexp fix #11965

Path to regexp fix #11965

hkbertoson commented Sep 10, 2024

changeset-bot bot commented Sep 10, 2024 •

edited

Loading

github-actions bot left a comment

uwej711 commented Sep 10, 2024 •

edited

Loading

hkbertoson commented Sep 10, 2024

hkbertoson commented Sep 10, 2024

delucis Sep 11, 2024

delucis Sep 11, 2024 •

edited

Loading

hkbertoson Sep 11, 2024

delucis Sep 11, 2024

uwej711 Sep 11, 2024

delucis Sep 11, 2024

hkbertoson Sep 11, 2024

uwej711 Sep 11, 2024

uwej711 Sep 11, 2024

uwej711 Sep 11, 2024

uwej711 Sep 11, 2024

blakeembrey commented Sep 11, 2024

uwej711 commented Sep 12, 2024

blakeembrey commented Sep 12, 2024

hkbertoson commented Sep 12, 2024

Princesseuh commented Sep 13, 2024

	return `${part.content.slice(3)}`;
	return `*${part.content.slice(3)}`;

Path to regexp fix #11965

Path to regexp fix #11965

Conversation

hkbertoson commented Sep 10, 2024

Changes

Testing

Docs

changeset-bot bot commented Sep 10, 2024 • edited Loading

🦋 Changeset detected

github-actions bot left a comment

Choose a reason for hiding this comment

uwej711 commented Sep 10, 2024 • edited Loading

hkbertoson commented Sep 10, 2024

hkbertoson commented Sep 10, 2024

Choose a reason for hiding this comment

delucis Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeembrey commented Sep 11, 2024

uwej711 commented Sep 12, 2024

blakeembrey commented Sep 12, 2024

hkbertoson commented Sep 12, 2024

Princesseuh commented Sep 13, 2024

changeset-bot bot commented Sep 10, 2024 •

edited

Loading

uwej711 commented Sep 10, 2024 •

edited

Loading

delucis Sep 11, 2024 •

edited

Loading