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

slashes should not be encoded for catch all regexes #1638

Open
keith0305 opened this issue Dec 20, 2022 · 9 comments · May be fixed by #2291
Open

slashes should not be encoded for catch all regexes #1638

keith0305 opened this issue Dec 20, 2022 · 9 comments · May be fixed by #2291
Labels
bug Something isn't working

Comments

@keith0305
Copy link

keith0305 commented Dec 20, 2022

Version

4.1.6

Reproduction link

codesandbox.io

Steps to reproduce

  1. Load the codesandbox.io project using the link provided.
  2. Click on About page.
  3. Click on the "Add hash" button, this will call router.replace({ hash: '#something' }).
  4. Notice that vue-router correctly appends the # in the url. (https://domain.com/about.html#yippy).
  5. Click on Deep About page.
  6. Click on the "Add hash" button.
  7. Notice that the url becomes https://domain.com/test%2Fdeep%2Fabout.html#yaya.

What is expected?

Url to become https://domain.com/test/deep/about.html#yaya

What is actually happening?

Url became https://domain.com/test%2Fdeep%2Fabout.html#yaya

image


This is for the front-end of a CMS-supported project, where the front part of the URL is controlled by the CMS and it varies depending on the environment. Hence the need to first match it using :pathMatch() then match the actual page to render using :pageName().

Also, all pages has to be suffixed with .html, hence I am using a (kind of) complex regex to match the page. It works, however, I stumble upon this issue.

@posva
Copy link
Member

posva commented Dec 27, 2022

This behavior is intended for regular params like :param because the / is a delimiter of params, but it should probably not happen for star params (:param(.*))

Note to myself in edit comment

@posva posva added the bug Something isn't working label Dec 27, 2022
@keith0305
Copy link
Author

Hey, thanks for the reply. Hopefully one day I can contribute a bug fix PR myself. For now, is there any workaround that you can suggest while I wait for the official fix?

@keith0305
Copy link
Author

Hi, is there any update to this?
Perhaps point me to the correct place where this bug occurs so I can attempt to tackle the issue?

@dschmidt
Copy link

Here's a workaround that we/I've come up with - it's not perfect as it's applied to the whole path, so we enable it only for routes with meta.patchCleanPath === true but it works well enough for us for now (although we obviously would like to see an upstream fix, so we can get rid of this):

import { RouteLocation, RouteLocationNormalizedLoaded, RouteLocationRaw, Router } from 'vue-router'

export const patchRouter = (router: Router) => {
  const cleanPath = (route) =>
    [
      ['%2F', '/'],
      ['//', '/']
    ].reduce((path, rule) => path.replaceAll(rule[0], rule[1]), route || '')

  const bindResolve = router.resolve.bind(router)
  router.resolve = (
    raw: RouteLocationRaw,
    currentLocation?: RouteLocationNormalizedLoaded
  ): RouteLocation & {
    href: string
  } => {
    const resolved = bindResolve(raw, currentLocation)
    if (resolved.meta?.patchCleanPath !== true) {
      return resolved
    }

    return {
      ...resolved,
      href: cleanPath(resolved.href),
      path: cleanPath(resolved.path),
      fullPath: cleanPath(resolved.fullPath)
    }
  }

  const routerMethodFactory = (method) => (to) => {
    const resolved = router.resolve(to)
    if (resolved.meta?.patchCleanPath !== true) {
      return method(to)
    }

    return method({
      path: cleanPath(resolved.fullPath),
      query: resolved.query
    })
  }

  router.push = routerMethodFactory(router.push.bind(router))
  router.replace = routerMethodFactory(router.replace.bind(router))

  return router
}

@keith0305
Copy link
Author

Hey, thanks for your patch. Will try it out. It is very appreciated.

@dschmidt
Copy link

If you find any edge cases that don't work, I'm happy to hear.

@keith0305
Copy link
Author

Your patch works. Good enough for us to ship the site for now. Thank you so much @dschmidt.

@memcorrupt
Copy link

@posva Is there any plans to fix this behavior? I think that bef97d6 is what caused the regression, since I used to be able to do things like redirecting to error pages without mangling the URL.

@posva posva changed the title Using router.push or .replace will cause forward slashes in url to be wrongly escaped when using regex to match path slashes should not be encoded for catch all regexes Oct 22, 2023
@posva posva moved this to Planned in Vue Router Roadmap Apr 17, 2024
ll3006 added a commit to ll3006/vue-router that referenced this issue Jul 4, 2024
This applies to all custom regex routes which match a slash or
contain one in their literal part

fixes vuejs#1638
@ll3006 ll3006 linked a pull request Jul 4, 2024 that will close this issue
@Tofandel
Copy link

Tofandel commented Aug 12, 2024

Same issue, it seems it doesn't happen when you use something like :category(.*)* but the problem is the catch all is greedy and causes issue because then the following /:category(.*?)*/(page/:page(\d+))? will never have page matched, it will go into category (which I think is also a behavior that should be changed or allow a second modifier token in the parser (like :category(.*?)*?to indicate non greediness)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: 📆 Planned
Development

Successfully merging a pull request may close this issue.

5 participants