Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch-all dynamic route doesn't catch a path with undefined path parameter #13011

Closed
kweiberth opened this issue May 17, 2020 · 9 comments · Fixed by #27738
Closed

Catch-all dynamic route doesn't catch a path with undefined path parameter #13011

kweiberth opened this issue May 17, 2020 · 9 comments · Fixed by #27738
Assignees
Milestone

Comments

@kweiberth
Copy link

kweiberth commented May 17, 2020

Bug report

Describe the bug

Hello! I'm experiencing an issue with a dynamic catch-all route and empty path parameters. For example:

/pages/[...slug]/index.tsx

Is not catching the route: http://localhost:3000/empty//parameter

Instead, it is loading default 404 page.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

Create a simple catch-all page:

// /pages/[...slug]/index.js
export default function () {
  return <div>{'Hello!'}</div>;
}

Then try to load URL with empty path parameter:

http://localhost:3000/empty//parameter

Expected behavior

It should load the catch-all page, not a 404.

System information

  • OS: macOS
  • Browser (if applies): Chrome
  • Version of Next.js: 9.4.0
  • Version of Node.js: 14.0.0
@Timer Timer added this to the 9.4.x milestone May 19, 2020
@tomdohnal
Copy link
Contributor

This comes down to the way Next.js handles matching routes.

It uses the path-to-regexp library to create Regexes which are then matched to the routes. For the catchall route, '/:path(.*)' is passed as an argument to the pathToRegexp function.

And it behaves like this:

const regex = path.pathToRegexp("/:path*");

console.log(regex.test("/catch/all/route")); // true
console.log(regex.test("/catch//route")); // false

If you pass '/:path(.*)' instead, it can handle empty path parameters:

const regex = path.pathToRegexp("/:path(.*)");

console.log(regex.test("/catch/all/route")); // true
console.log(regex.test("/catch//route")); // true

Using '/:path*' (instead of '/:path(.*)') to match the catch-all routes is all over the Next.js codebase and I'm not sure if it's intentional or not.

If somebody from the Next.js team could tell if it's intentional to NOT match routes with empty parameters it'd be awesome :)

@Timer
Copy link
Member

Timer commented Jun 3, 2020

It sounds like Next.js should disallow multi-slash and auto-redirect.

@kweiberth
Copy link
Author

kweiberth commented Jun 3, 2020

@tomdohnal thanks for looking into this.

@Timer can you describe more what you mean by this? I'm unsure what you mean by multi-slash and auto-redirect, and also unsure if you want to disallow both of them, or rather disallow multi-slash but instead auto-redirect

@Timer
Copy link
Member

Timer commented Jun 5, 2020

To clarify what I meant, Next.js should intercept any request with double forward slashes and normalize it to one with a redirect.

@kweiberth
Copy link
Author

Oh okay, cool, I think that could make sense. Do you think it's necessary to normalize and redirect? Would it be okay to leave it as an undefined path parameter, especially if we're using [...slug] catch-all? It might be valuable for the app to know if a certain parameter was undefined (as opposed to changing the number of parameters in the scenario of normalizing)

@Timer Timer added the kind: bug label Jun 8, 2020
@Timer Timer modified the milestones: 9.x.x, iteration 3 Jun 16, 2020
@Timer Timer modified the milestones: iteration 3, iteration 4 Jun 30, 2020
@Timer Timer assigned Timer and ijjk and unassigned Timer Jun 30, 2020
@Timer Timer modified the milestones: iteration 4, iteration 5 Jul 13, 2020
@Timer Timer assigned Janpot and unassigned ijjk Jul 14, 2020
@Janpot Janpot mentioned this issue Jul 14, 2020
1 task
@Timer Timer modified the milestones: iteration 5, iteration 6 Jul 27, 2020
@Timer Timer modified the milestones: iteration 6, iteration 7 Aug 6, 2020
@kweiberth
Copy link
Author

Hello team 👋 It looks like you are going to be normalizing double slashes to a single slash. Will there be a way to opt out of this? I would like to be able to conserve the data of the empty param. So for my original example, with page:

/pages/[...slug]/index.tsx

and URL:

http://localhost:3000/empty//parameter

I'd like to have query.slug to be

['empty', undefined, 'parameter']

I have legacy link structure that I need to be able to handle with catch-all route. My legacy link structure looks like:

https://song.link/https://open.spotify.com/track/69o00fGvsei250jH7bF781

Do you know how I can handle this URL?

@kweiberth
Copy link
Author

Ah, I think I figured it out. I can define a rewrite:

{
    source: `/(https?\\:)/(.*)*`,
    destination: '/api/redirect',
},

Then in the API route I can grab req.url, which would be /https://open.spotify.com/track/69o00fGvsei250jH7bF781 in my example above, and then I can parse it and redirect to the correct canonical URL.

Hopefully the work in #15171 won't conflict with this solution, but I don't think it will bc my source definition only checks for that first rewrite. If Next.js removes the extra double slash before this, I'll just have to modify my logic in my API route when handling the redirect

@Timer Timer modified the milestones: iteration 7, 9.x.x Aug 21, 2020
@Timer Timer modified the milestones: 10.x.x, backlog Jan 6, 2021
@Timer Timer modified the milestones: backlog, iteration 16 Jan 25, 2021
@dalfriedrice
Copy link

@Timer @kweiberth
Can someone give any insight to my issue
Its like, I am using [...catch_all_urls] routing to consume my routes

So, the issue is,
When I am entering a url such as promo/cc///cd///dcscc (url with multiple slashes) 404 page gets rendered by nextjs, but it don't reads the runtime variable process.env.ECS_Cluster in next.config.js but if I give a proper url like promo/cc/dscsdcs then in next.config.js it is properly reading runtime variable process.env.ECS_Cluster.

Can one of you or someone from next team give an explanation around this.

@Timer Timer modified the milestones: iteration 17, 10.x.x Mar 15, 2021
@kodiakhq kodiakhq bot closed this as completed in #27738 Aug 3, 2021
kodiakhq bot pushed a commit that referenced this issue Aug 3, 2021
This adds handling for repeated forward/back slashes in Next.js, when these slashes are detected in a request to Next.js we will automatically remove the additional slashes redirecting with a 308 status code which prevents duplicate content when being crawled by search engines. 

Fixes: #13011
Fixes: #23772
Closes: #15171
Closes: #25745
flybayer pushed a commit to blitz-js/next.js that referenced this issue Aug 19, 2021
This adds handling for repeated forward/back slashes in Next.js, when these slashes are detected in a request to Next.js we will automatically remove the additional slashes redirecting with a 308 status code which prevents duplicate content when being crawled by search engines. 

Fixes: vercel#13011
Fixes: vercel#23772
Closes: vercel#15171
Closes: vercel#25745
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants