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

[Bug]: Match route does not work if there is a minus sign at the beginning #8525

Closed
kulakowka opened this issue Dec 22, 2021 · 21 comments
Closed
Labels

Comments

@kulakowka
Copy link

What version of React Router are you using?

6.2.1

Steps to Reproduce

<Routes>
      <Route path="/users/:id" element={<UserApp />} />
</Routes>

Expected Behavior

/users/-wYVsb1KDtT9YJJFUiSTq0dqUyvc5vK51nx7QW29CPJc - This route is matched

Actual Behavior

/users/-wYVsb1KDtT9YJJFUiSTq0dqUyvc5vK51nx7QW29CPJc - This route is NOT matched

/users/wYVsb1KDtT9YJJFUiSTq0dqUyvc5vK51nx7QW29CPJc - This route is matched

I think the problem is in the mechanism that is looking for a suitable route.

Anything that starts with a symbol - is not matched.

@kulakowka kulakowka added the bug label Dec 22, 2021
@ivansvlv
Copy link

ivansvlv commented Dec 22, 2021

I've just noticed the same bug, but I have a "." in the route.
e.g. this wouldn't match to the path - login/reset/:resetToken:
login/reset/Djdfsd32afasdg.kdaf324asdf.adsgGsd
however this path - login/reset/:resetToken/success will be matched:
login/reset/Djdfsd32afasdg.kdaf324asdf.adsgGsd/success

Update
Since my error was actually 404, it's actually a problem with Vite's local dev server, and I've found Issue and PR in their repo for similar behaviour with "." in the URL.

@kulakowka
Copy link
Author

@mjackson Sorry to bother you. This bug is critical for us. We have already uploaded the new version of our application to production. Please pay attention to this problem. Probably there is an error somewhere in the regular expressions. Surely this can be fixed very quickly.

@benkraus
Copy link

benkraus commented Jan 3, 2022

remix-run/remix#819 relates here. We had to hack that patch in (probably breaking something else wrt routing) in order to get .well-known paths to match correctly.

@edbella
Copy link

edbella commented Jan 12, 2022

Update: This also breaks when there is a - at the end as well, not just at the beginning

@seeden
Copy link

seeden commented Jan 20, 2022

Same problem for @ character at the beginning

@zwily
Copy link
Contributor

zwily commented Jan 20, 2022

In Remix, I can't use a splat path on any path with a hyphen anywhere in it. So, app/routes/cdn-cgi.image.$.js does NOT work for matching /cdn-cgi/image/foo, but app/routes/cdncgi.image.$.js DOES match /cdncgi/image/foo. Sounds like it's possibly related.

@joacub
Copy link

joacub commented Feb 4, 2022

same here, urls with @ in the beginning does not work, this was working before, what happened ?

thanks in advance

@joacub
Copy link

joacub commented Feb 7, 2022

Is there any advance on this, I can’t update my application becouse of this.

@egerrek
Copy link

egerrek commented Feb 11, 2022

I have problem with cyrillic symbols. The route beginning with the Cyrillic character is not found.

Steps to Reproduce

<Route path="/users/:id" element={} />

Expected Behavior
/users/михаил - This route is matched

Actual Behavior
/users/михаил - This route is NOT matched

/users/michael - This route is matched

@dartandrevinsky
Copy link

I have problem with cyrillic symbols. The route beginning with the Cyrillic character is not found.

Steps to Reproduce Expected Behavior /users/михаил - This route is matched

Actual Behavior /users/михаил - This route is NOT matched

/users/michael - This route is matched

Try and analyze the regexes in play yourself - maybe you will be able to spot the error, file a PR, or - fork the repo and use it as a dependency directly:

function compilePath(
path: string,
caseSensitive = false,
end = true
): [RegExp, string[]] {
warning(
path === "*" || !path.endsWith("*") || path.endsWith("/*"),
`Route path "${path}" will be treated as if it were ` +
`"${path.replace(/\*$/, "/*")}" because the \`*\` character must ` +
`always follow a \`/\` in the pattern. To get rid of this warning, ` +
`please change the route path to "${path.replace(/\*$/, "/*")}".`
);
let paramNames: string[] = [];
let regexpSource =
"^" +
path
.replace(/\/*\*?$/, "") // Ignore trailing / and /*, we'll handle it below
.replace(/^\/*/, "/") // Make sure it has a leading /
.replace(/[\\.*+^$?{}|()[\]]/g, "\\$&") // Escape special regex chars
.replace(/:(\w+)/g, (_: string, paramName: string) => {
paramNames.push(paramName);
return "([^\\/]+)";
});
if (path.endsWith("*")) {
paramNames.push("*");
regexpSource +=
path === "*" || path === "/*"
? "(.*)$" // Already matched the initial /, just match the rest
: "(?:\\/(.+)|\\/*)$"; // Don't include the / in params["*"]
} else {
regexpSource += end
? "\\/*$" // When matching to the end, ignore trailing slashes
: // Otherwise, match a word boundary or a proceeding /. The word boundary restricts
// parent routes to matching only their own words and nothing more, e.g. parent
// route "/home" should not match "/home2".
"(?:\\b|\\/|$)";
}
let matcher = new RegExp(regexpSource, caseSensitive ? undefined : "i");
return [matcher, paramNames];
}

@knpwrs
Copy link
Contributor

knpwrs commented Feb 20, 2022

This bug is what causes this: remix-run/remix#2012

@troyschneringer
Copy link

troyschneringer commented Feb 25, 2022

@dartandrevinsky and @egerrek

I think the issue can be traced to line 1225:

if (path.endsWith("*")) {
paramNames.push("*");
regexpSource +=
path === "*" || path === "/*"
? "(.*)$" // Already matched the initial /, just match the rest
: "(?:\\/(.+)|\\/*)$"; // Don't include the / in params["*"]
} else {
regexpSource += end
? "\\/*$" // When matching to the end, ignore trailing slashes
: // Otherwise, match a word boundary or a proceeding /. The word boundary restricts
// parent routes to matching only their own words and nothing more, e.g. parent
// route "/home" should not match "/home2".
"(?:\\b|\\/|$)";
}

The problem with using \b in the regex is that its only matching the equivalent of [a-zA-Z0-9_].

I'm not a regex expert and am not finding an equivalent of \b to use that handles unicode characters.

@brophdawg11
Copy link
Contributor

Fixed in #8563

@troyschneringer
Copy link

@brophdawg11 the fix is #8563 doesn't appear to address the issue with routes that start with @ not matching. Is this intentional?

@knpwrs
Copy link
Contributor

knpwrs commented Feb 28, 2022

@troyschneringer it appears that of @, ., -, and ~, only @ gets urlencoded by encodeURIComponent:

$ echo "@.-~" | node -e 'process.stdout.write(encodeURIComponent(require("fs").readFileSync(0, "utf-8")))'
%40.-~

The fix does support url-encoded characters, though I'm not sure how browsers handle @ when they are sent in the URL.

@brophdawg11
Copy link
Contributor

@troyschneringer Yeah that's intentional per the Unreserved Characters section of RFC 3986 referenced in the PR. Preceding that is the section on Reserved Characters which includes @ and indicates that URLs should be encoding @ ahead of time:

2.2. Reserved Characters

URIs include components and subcomponents that are delimited by
characters in the "reserved" set. These characters are called
"reserved" because they may (or may not) be defined as delimiters by
the generic syntax, by each scheme-specific syntax, or by the
implementation-specific syntax of a URI's dereferencing algorithm.
If data for a URI component would conflict with a reserved
character's purpose as a delimiter, then the conflicting data must be
percent-encoded before the URI is formed.

 reserved    = gen-delims / sub-delims

 gen-delims  = ":" / "/" / "?" / "#" / "[" / "]" / "@"

 sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
             / "*" / "+" / "," / ";" / "="

@TrevorSayre
Copy link

TrevorSayre commented Feb 28, 2022

@brophdawg11 So are routes like https://www.npmjs.com/package/@types/node in violation of RFC 3986 and thus react-router won't support it, despite this existing in the wild? Or am I misunderstanding?

@brophdawg11
Copy link
Contributor

I wouldn't consider it a hard "won't support it" based on #8563 alone - @ in particular hasn't been given a lot of thought since it was not the original issue in this bug or #8561. The RFC section I mentioned above spoke more to why the PR author chose to support ~/-/. as they are explicitly Unreserved Characters.

I would vote that support for Reserved Characters such as @ be discussed separately in a new discussion/issue so they can be properly evaluated.

@kiliman
Copy link
Contributor

kiliman commented Mar 17, 2022

2.2. Reserved Characters

URIs include components and subcomponents that are delimited by
characters in the "reserved" set. These characters are called
"reserved" because they may (or may not) be defined as delimiters by
the generic syntax, by each scheme-specific syntax, or by the
implementation-specific syntax of a URI's dereferencing algorithm.

I think the operative phrase here is "... they may (or may not) be defined as delimiters by
the generic syntax, by each scheme-specific syntax..."

I believe standard URLs (via http: and https: schemes) treat @ as a regular character. Same with mailto: (otherwise you'd have to encode the @ in email addresses.

@shamsup
Copy link
Contributor

shamsup commented Mar 31, 2022

#8699 (comment)
cc

In #8563 I was specifically only including the unreserved characters with the idea that the whitelist could be expanded if needed. Of the reserved characters in the RFC, only !'()* are not touched by encodeURIComponent, but the RFC also says in section 3.3 that @ (as well as : and any sub-delims from 2.2) can be included in paths:

pchar = unreserved / pct-encoded / sub-delims / ":" / "@"

@knpwrs
Copy link
Contributor

knpwrs commented Mar 31, 2022

It would be great to support @ in paths. I made a remix site that was intended to support @ in URLs but it doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests