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

Case sensitivity of path matching #3330

Closed
jeffreythomasprice opened this issue Nov 9, 2023 · 2 comments · Fixed by #3353
Closed

Case sensitivity of path matching #3330

jeffreythomasprice opened this issue Nov 9, 2023 · 2 comments · Fixed by #3353

Comments

@jeffreythomasprice
Copy link

Steps to reproduce

  • Launch a feathers app with at least one rest path, e.g. /hello
  • Hit that path with some letters capitalized differently, e.g. /HELLO

Expected behavior

Any capitalization of the path should hit the same feathers service.

It appears that paths were matched in a case-insensitive manner prior to feathers 5.

Actual behavior

Exact capitalization is required. It does not appear to be configurable.

System configuration

  • We recently upgraded from feathers 4.5.11 to 5.0.8.
  • We found relevant code in packages/transport-commons/src/routing/router.ts, which is on 5.0.8.
  • Node 16.14.0, although I can't imagine that's relevant.

Code Sample

We're using express, and we tried playing with express properties to control case-sensitivity, e.g.

// app is the express app in this snippet, not a feathers app
app.set('case sensitive routing', true);

We played with several variants of constructing an express app and configuring it's path matching, then associating it with a feathers app. After we found packages/transport-commons/src/routing/router.ts it became clear that the path matching isn't using express' built-in behavior at all.

We found some text in this document https://feathersjs.com/guides/whats-new.html that references new path matching behavior, but no mention of case sensitivity. That links to https://feathersjs.com/api/application.html#lookup-path, but ditto.

We've worked around this by changing the path matching behavior in packages/transport-commons/src/routing/router.ts. Our snippet is slightly more complicated, but the relevant change is:

$ git diff
diff --git a/packages/transport-commons/src/routing/router.ts b/packages/transport-commons/src/routing/router.ts
index ad76319b..c46fe0b7 100644
--- a/packages/transport-commons/src/routing/router.ts
+++ b/packages/transport-commons/src/routing/router.ts
@@ -90,7 +90,10 @@ export class RouteNode<T = any> {
     }
 
     const current = path[this.depth]
-    const child = this.children[current]
+    const currentLowerCase = current.toLowerCase()
+    const child = Object.entries(this.children).find(
+      ([child]) => child.toLowerCase() === currentLowerCase
+    )?.[1]
 
     if (child) {
       const lookup = child.lookup(path, info)

@daffl
Copy link
Member

daffl commented Nov 27, 2023

That is definitely an issue that slipped through. #3353 has a backwards compatible fix that allows case insensitive route lookups by setting

app.routes.caseSensitive = false

@jeffreythomasprice
Copy link
Author

Appreciate it! I'll look out for the next release.

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

Successfully merging a pull request may close this issue.

2 participants