troch / route-node

A package to create a tree of named routes
MIT License
20 stars 15 forks source link

Firefox: matchChildren() issue when pathSegment contains | (pipe) char #19

Closed todorpr closed 6 years ago

todorpr commented 6 years ago

Basically Firefox doesn't encode | (pipe) char, actually even if it is already encoded to %7c it decodes it. This way in matchChildren():

remainingPath = segment.replace(consumedPath, '')

replace doesn't work properly as segment is decoded, whereas consumedPath is encoded.

For example if route is /one|two it becomes: "/one|two".replace("/one%7ctwo", '') and remainingPath is wrong.

This issue causes problems to router5, since it can't identify current route properly.

This is the stack trace (router-liefecycle.js belongs to router5): image

One solution in matchChildren() to change:

var segment = pathSegment;

to

var segment = encodeURI(decodeURI(pathSegment));
troch commented 6 years ago

OK, thanks for the feedback. I'm in the process of refactoring route-node and fixing another issue. How urgent is it for you to have it fixed?

todorpr commented 6 years ago

Hey, thanks for your help. Well, it's not super urgent, but it's good to be fixed soon.

todorpr commented 6 years ago

Hi @troch, I saw you have released route-node v3.0.2, but this issue is not handled yet. I don't want to push you for sure, but have you planned to fix this soon, since the issues became a blocker on my side? At least maybe you can apply my suggested solution.

troch commented 6 years ago

Will fix soon, but don't have a timeline yet

troch commented 6 years ago

@todorpr is it when a | is in a parameter? or when it is in the path definition?

chrisdoc commented 6 years ago

@troch hello from my side, I'm a colleague of @todorpr and the issue occurs when it is part of a path.

Our path regex looks something like /:query<(\\w{1,}(-|_|%7C|\\|)?)+> where we have multiple tokens ( words ) that can be combined by ['-', '_', '|', '%7'].

www.mydomain.com/cars-red|blue www.mydomain.com/cars-red|blue-automatic_transmission

troch commented 6 years ago

I struggle to replicate, I find it works OK on Firefox and elsewhere. Segments are decoded, so it seems to work whether pipe characters are encoded or not. Could you provide code to replicate?

troch commented 6 years ago

Hmmm I see now what the issue is, creating a fix.

troch commented 6 years ago

Fixed: https://github.com/troch/path-parser/commit/d6e6d27657600954947f22c039f1218f954ef6c7

Will be included in router5@6

chrisdoc commented 6 years ago

@troch i updated our application to router5@6.1.3 and I can still observe the issue with not encoded | pipe characters in Firefox.

I extended router5's react example ( https://github.com/chrisdoc/router5/commit/a5703c16776af3278d5dd7176bdb1ebb921613d4) to show a POC for when this is happening. I deployed the example http://ten-page.surge.sh/ , when you go to http://ten-page.surge.sh/#/firefox/red|blue via a link it opens the correct site but when you open a new tap and paste the URL you will end up on the not found page

image
troch commented 6 years ago

I don't have the issue reproduced in Firefox

image

todorpr commented 6 years ago

@troch it's a bit tricky to reproduce it, but here is a summary:

if URL is already encoded with %7c (like so http://ten-page.surge.sh/#/firefox/aaa%7cddd) it's parsed properly, but if you change last letter, press Enter and then refresh the page it fails too.

if URL contains decoded pipe | it fails http://ten-page.surge.sh/#/firefox/aaa|bbb

troch commented 6 years ago

OK, I now see what the problem is: when a pipe is displayed in the URL, it might be encoded or not, until you manually interact with it:

chrisdoc commented 6 years ago

@troch exactly that is the issue we have observed

troch commented 6 years ago

It was fixed with the latest release of router5 (6.2.0)