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

Middleware NextResponse.rewrite() crashes on colons in paths #31523

Closed
klarstrup opened this issue Nov 17, 2021 · 4 comments · Fixed by #32543
Closed

Middleware NextResponse.rewrite() crashes on colons in paths #31523

klarstrup opened this issue Nov 17, 2021 · 4 comments · Fixed by #32543
Labels
Middleware Related to Next.js Middleware.

Comments

@klarstrup
Copy link

klarstrup commented Nov 17, 2021

What version of Next.js are you using?

12.0.4

What version of Node.js are you using?

16.10.0

What browser are you using?

Chrome 95

What operating system are you using?

macOS

How are you deploying your application?

next start

Describe the Bug

Using NextResponse.rewrite("/:blahblah") in a middleware to navigate to a path with a colon in it causes an obtuse TypeError: Expected "blahblah" to be a string style error(ultimately coming from path-to-regexp).

Expected Behavior

Rewriting to a path with a colon in it to simply work without any unexpected replacement being attempted internally.

To Reproduce

something like this in any middleware

import { NextResponse } from "next/server";

export default function middleware() {
  return NextResponse.rewrite("/not-a:param");
}

repro repo here: https://github.com/klarstrup/next-middleware-rewrite-bug

@klarstrup klarstrup added the bug Issue was opened via the bug report template. label Nov 17, 2021
@klarstrup
Copy link
Author

Oh and this was working in 12.0.3

@klarstrup klarstrup changed the title Middleware NextResponse.rewrite() crashes on colons in paths Middleware NextResponse.rewrite() crashes on colons in paths :( Dec 15, 2021
@klarstrup klarstrup changed the title Middleware NextResponse.rewrite() crashes on colons in paths :( 😭 Middleware NextResponse.rewrite() crashes on colons in paths Dec 15, 2021
klarstrup pushed a commit to klarstrup/next.js that referenced this issue Dec 15, 2021
this addresses the symptom but the real systemic issue is that prepareDestination is called on rewrite/redirect URLs, which have no defined special behavior for colons and they should not be compiled at all
@balazsorban44 balazsorban44 reopened this Dec 22, 2021
@balazsorban44 balazsorban44 added Middleware Related to Next.js Middleware. kind: bug and removed bug Issue was opened via the bug report template. labels Dec 22, 2021
@balazsorban44 balazsorban44 changed the title 😭 Middleware NextResponse.rewrite() crashes on colons in paths Middleware NextResponse.rewrite() crashes on colons in paths Jan 26, 2022
@javivelasco javivelasco added this to the Next.js Middleware GA milestone Feb 18, 2022
@Schniz
Copy link
Contributor

Schniz commented Mar 2, 2022

checking this out 🙏

ijjk pushed a commit that referenced this issue Mar 3, 2022
* Add failing colon rewrite test

* add test fixture

* better colon rewrite tests

* middleware rewrite colon tests with query parameters

* fix #31523

this addresses the symptom but the real systemic issue is that prepareDestination is called on rewrite/redirect URLs, which have no defined special behavior for colons and they should not be compiled at all

* hack around prepareDestination to skip compiling x-middleware-rewrite

this is a bit nicer than just escaping colons, but ideally we find a way to obviate prepareDestination

* obviate prepareDestination for x-middleware-rewrite handling

* don't clobber rewrite query data

* omit redundant type

* catch up to main

* It looks like newUrl should contain only pathname

Co-authored-by: Naoyuki Kanezawa <naoyuki.kanezawa@gmail.com>
@Schniz
Copy link
Contributor

Schniz commented Mar 3, 2022

🤩

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2022

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Middleware Related to Next.js Middleware.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants