Skip to content

fix(router-core): Backslash in path parameters are incorrectly restored from URL#3987

Closed
leesb971204 wants to merge 4 commits into
TanStack:mainfrom
leesb971204:fix/backshlash-path-parameters
Closed

fix(router-core): Backslash in path parameters are incorrectly restored from URL#3987
leesb971204 wants to merge 4 commits into
TanStack:mainfrom
leesb971204:fix/backshlash-path-parameters

Conversation

@leesb971204

@leesb971204 leesb971204 commented Apr 14, 2025

Copy link
Copy Markdown
Contributor

fixes: #3962

I believe the bug is caused by an unnecessary decodeURI in path.ts.
It works well, but I think we need to reconsider the test case we wrote. I think the test case is incorrectly written.

If my understanding is incorrect, I apologize.

@nx-cloud

nx-cloud Bot commented Apr 14, 2025

Copy link
Copy Markdown
Contributor

View your CI Pipeline Execution ↗ for commit f219344.

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ❌ Failed 6m 8s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 2m 4s View ↗

☁️ Nx Cloud last updated this comment at 2025-06-05 01:58:28 UTC

@pkg-pr-new

pkg-pr-new Bot commented Apr 14, 2025

Copy link
Copy Markdown
More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@3987

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@3987

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@3987

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@3987

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@3987

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@3987

@tanstack/react-router-with-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-with-query@3987

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@3987

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@3987

@tanstack/react-start-config

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-config@3987

@tanstack/react-start-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-plugin@3987

@tanstack/react-start-router-manifest

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-router-manifest@3987

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@3987

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@3987

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@3987

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@3987

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@3987

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@3987

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@3987

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@3987

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@3987

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@3987

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@3987

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@3987

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@3987

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@3987

@tanstack/solid-start-config

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-config@3987

@tanstack/solid-start-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-plugin@3987

@tanstack/solid-start-router-manifest

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-router-manifest@3987

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@3987

@tanstack/start

npm i https://pkg.pr.new/TanStack/router/@tanstack/start@3987

@tanstack/start-api-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-api-routes@3987

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@3987

@tanstack/start-config

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-config@3987

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@3987

@tanstack/start-server-functions-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-client@3987

@tanstack/start-server-functions-fetcher

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-fetcher@3987

@tanstack/start-server-functions-handler

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-handler@3987

@tanstack/start-server-functions-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-server@3987

@tanstack/start-server-functions-ssr

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-functions-ssr@3987

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@3987

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@3987

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@3987

commit: f219344

@schiller-manuel

Copy link
Copy Markdown
Collaborator

can you please remove the irrelevant commits from this PR?

…ecoding logic

Signed-off-by: leesb971204 <leesb971204@gmail.com>
…URI encoded characters.

Signed-off-by: leesb971204 <leesb971204@gmail.com>
@leesb971204

leesb971204 commented Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

@schiller-manuel

I think we need to write separate tests for paths containing special characters, depending on whether we use $ with params or define the path directly.
For example, when accessing a path like %C3%A9 using $ and params props in <Link>, the path remains %C3%A9, but when it’s defined directly, it gets decoded and becomes é.
This behavior was the same before and after my changes in my local, but I don’t understand why the tests are failing only after my modification...

@SeanCassiere SeanCassiere left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside of the test regressions present in the unit testing, I'm not sure if this is the right approach.

I'm wondering whether we should be doing any decoding of the named path parameters (not the splat parameter) at all. Perhaps, we shouldn't be encoding the path parameters at all? 🤔

Whilst, URI encoding works for the URL Search Parameters, perhaps we shouldn't be doing this for the path parameters. Essentially we'd leave them "as-is".

@leesb971204

leesb971204 commented Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

Aside of the test regressions present in the unit testing, I'm not sure if this is the right approach.

I'm wondering whether we should be doing any decoding of the named path parameters (not the splat parameter) at all. Perhaps, we shouldn't be encoding the path parameters at all? 🤔

Whilst, URI encoding works for the URL Search Parameters, perhaps we shouldn't be doing this for the path parameters. Essentially we'd leave them "as-is".

// path.ts
value: part.includes('%25')
          ? part
              .split('%25')
              .map((segment) => decodeURI(segment))
              .join('%25')
          : hasUriEncodedChars(part)
            ? part
            : decodeURI(part),

👇

value: part

So do you think we should make the change as described above?
And I agree that we shouldn’t decode the path parameters, but isn’t encoding necessary to prevent browser errors?

@SeanCassiere

Copy link
Copy Markdown
Member

Yea, I'm wondering if that'd be the trick. Also, whether or not we should be doing the encodeUri... for the path parameters.

Nothing concrete. Just thinking of ideas to go about this without breaking everything for everyone 😅

@leesb971204

Copy link
Copy Markdown
Contributor Author

Yea, I'm wondering if that'd be the trick. Also, whether or not we should be doing the encodeUri... for the path parameters.

Nothing concrete. Just thinking of ideas to go about this without breaking everything for everyone 😅

Exactly. The decoding logic does seem pretty complex. For encoding, I think it might be reasonable to delegate that responsibility to the user when it comes to path parameters. But if we go that route, it would likely introduce a lot of changes to the existing codebase.

@DreierF

DreierF commented May 10, 2025

Copy link
Copy Markdown

What is the current state of this PR?
Is the PR currently just blocked by the test failure or do you consider to rework the whole parameter encoding approach? Is there anything I could contribute to move this forward?

As we are currently waiting for a fix for the issue, I would favor if the current fix would be released and a potential bigger rework could be implemented as a follow-up PR.

@SeanCassiere

Copy link
Copy Markdown
Member

We are currently locked in a bit of code-freeze so we can minimize the number of conflicts while we complete the large rewrites happening in the devinxi branch.

'As-is', after the code freeze, even disregarding the lack of tests, I think I'd still have reservations as to the flow and logic in how this fix is being implemented in this PR and stand by comment from above - #3987 (review).


What is the current state of this PR? Is the PR currently just blocked by the test failure or do you consider to rework the whole parameter encoding approach? Is there anything I could contribute to move this forward?

As we are currently waiting for a fix for the issue, I would favor if the current fix would be released and a potential bigger rework could be implemented as a follow-up PR.

That said, @DreierF if you'd be willing to look at going about this using the "no encoding at all approach" and have some unit tests in place, then I can look at having your changes pushed through and I'd resolve the conflicts later on.

… parsing function

Signed-off-by: leesb971204 <leesb971204@gmail.com>
@nlynzaad

Copy link
Copy Markdown
Contributor

closing in favour of #5663

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Backslash in path parameters are incorrectly restored from URL

5 participants