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

Crash after starting using version 4.21.0 #5955

Closed
az-faro opened this issue Sep 12, 2024 · 45 comments
Closed

Crash after starting using version 4.21.0 #5955

az-faro opened this issue Sep 12, 2024 · 45 comments

Comments

@az-faro
Copy link

az-faro commented Sep 12, 2024

I just updated express to fix the latest reported CVEs, but the last version (4.21.0) is now causing crashes when loading express.

Crash happens when I call this:
const express = require("express")

Error is:

...\node_modules\path-to-regexp\index.js:68
  path = path.replace(
              ^

TypeError: path.replace is not a function
    at pathToRegexp (...\node_modules\path-to-regexp\index.js:68:15)
    at new Layer (...\node_modules\express\lib\router\layer.js:45:17)
    at Function.route (...\node_modules\express\lib\router\index.js:505:15)
    at app.<computed> [as put] (...\node_modules\express\lib\application.js:498:30)
    at Object.<anonymous> (...\js\index.js:69:9)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)

I realize this is not the express library, but express is the only user of path-to-regexp in my application, so it can't be any other culprit.

The exact same code on my side runs fine using for example express 4.19.2.

@az-faro az-faro added the bug label Sep 12, 2024
@NewEraCracker
Copy link

NewEraCracker commented Sep 12, 2024

Seems to be caused by this - node_modules\path-to-regexp\index.js:68:15:

pillarjs/path-to-regexp@29b96b4

I would go into that module and change.

path = path.replace(

for:

path = String(path).replace(

My two cents.

PS: If this works good, you can make a patch using patch-package.

@NewEraCracker
Copy link

NewEraCracker commented Sep 12, 2024

Can be reproduced if the argument of routing path is not a string.

For example, this crashes with the same stack trace: app.get(1, (req, res) => res.sendStatus(200));

My workaround works. So my advise is for you to check all routes and make sure they are strings.

The patch:

diff --git a/node_modules/path-to-regexp/index.js b/node_modules/path-to-regexp/index.js
index 1150335..52edd3e 100644
--- a/node_modules/path-to-regexp/index.js
+++ b/node_modules/path-to-regexp/index.js
@@ -65,7 +65,7 @@ function pathToRegexp(path, keys, options) {
     return new RegExp(path.join('|'), flags);
   }
 
-  path = path.replace(
+  path = String(path).replace(
     /\\.|(\/)?(\.)?:(\w+)(\(.*?\))?(\*)?(\?)?|[.*]|\/\(/g,
     function (match, slash, format, key, capture, star, optional, offset) {
       pos = offset + match.length;

Edit after close: These routes/layers were getting ignored. The route was getting garbled to symbols of function. At least it did not crash...

@wesleytodd
Copy link
Member

wesleytodd commented Sep 12, 2024

cc @blakeembrey let me know if the PR looks like the solution you want to go with and I can prep the express release.

@blakeembrey
Copy link
Member

@az-faro what are you using as input if it’s not a string, array, or regex?

This library shouldn’t be trying to coerce non-strings since that will hide important bugs in the users code. For example, I can’t think of any native object that could coerce and still match a real route.

If you have a legitimate use-case for something using this that isn’t a bug, I’d be happy to patch this back, but it wasn’t a part of the original API contract even if it may have worked accidentally.

@az-faro
Copy link
Author

az-faro commented Sep 12, 2024

@blakeembrey I don't even get far enough to use any input, it crashes directly when I call const express = require("express").

@az-faro
Copy link
Author

az-faro commented Sep 12, 2024

Actually I revoke my previous statement, I get the crash here:

app.put(express.raw({ type: '*/*' })); (where app is let app = express();)

@alewitt2
Copy link

alewitt2 commented Sep 12, 2024

we are also running into this, and it appears to be because we are defining routes without a path at all, which should be allowed as defined in the docs Middleware callback function examples, specifically we are doing things like:

app.put(function (req, res, next) { next() })
app.post(function (req, res, next) { next() })
app.patch(function (req, res, next) { next() })
app.delete(function (req, res, next) { next() })
image

@blakeembrey
Copy link
Member

So that's specific to .use, and isn't supported in regular methods. Did you test these ever worked? If they were passed to path-to-regexp and generated routes it was just the route function ..., which doesn't seem like it would have worked before.

@blakeembrey
Copy link
Member

Oh wait, it's in your screenshot, this seems like a regression in Express and not path-to-regexp. Trying to coerce this in path-to-regexp would have hidden this bug.

@alewitt2
Copy link

alewitt2 commented Sep 12, 2024

yes this has worked for us for a long time.. and if you read that last sentence it explicitly says it should work for all app.METHODs

Even though the examples are for app.use(), they are also valid for app.use(), app.METHOD(), and app.all()

i agree this should be handled in express before it gets to path-to-regexp

@blakeembrey
Copy link
Member

@wesleytodd Looks like it may be a regression in Express, I have a short call to jump to now but can investigate it afterward.

@wesleytodd
Copy link
Member

wesleytodd commented Sep 12, 2024

I am very surprised this is not covered in the tests. I also have a few meetings starting in 25min, but I will try before then to get a test (and maybe fix) opened. If we dont have a PR to land in the next hour though, I will be on some work meetings and have trouble releasing this until later today.

EDIT: yep it is wild this was not covered in tests for this long. It was as easy as this to reproduce

      it('should support ' + method.toUpperCase() + ' without a path', function(done){
        if (method === 'query' && shouldSkipQuery(process.versions.node)) {
          this.skip()
        }
        var app = express();

        app[method](function(req, res){
          res.send(method)
        });

        request(app)
        [method]('/foo')
        .expect(200, done)
      })

@wesleytodd
Copy link
Member

wesleytodd commented Sep 12, 2024

The same test in [email protected] and I get a 404 instead of a throw. Were these routes layers actually being matched in the way you were using them in the past?

@wesleytodd
Copy link
Member

I guess what I should ask is, can you provide a functional reproduction of the how you expect this to work?

@az-faro
Copy link
Author

az-faro commented Sep 12, 2024

@wesleytodd who are you asking?

@wesleytodd
Copy link
Member

Anyone who is in this thread who is using this form of the api.

@blakeembrey
Copy link
Member

I wonder if this was a bug in the documentation instead of Express, I suspect it's never actually worked before since the path generated wouldn't have started with / (as it'd be String(function () {})).

That said, it's possible to "fix" this, but that might change the behavior of applications since they weren't actually working before and now they'd start working.

@wesleytodd
Copy link
Member

Yeah I suspect the same. The old behavior looks like it would never match these middleware and would then just pass on to following matched layers. So if we made it now match, that would be a breaking change. I need more information from the folks using this to help us decide what is the right thing to do next.

@az-faro
Copy link
Author

az-faro commented Sep 12, 2024

I'm doing this to be able to parse xml, it now fails on the second to last line (has worked before):

const express = require("express")
const bodyParser = require("body-parser")

require('body-parser-xml')(bodyParser)

// Create the express app
let app = express();

// Setup how to parse data
app.put(express.raw({ type: '*/*' }));  // <--- crash here
app.use(bodyParser.xml());

@wesleytodd
Copy link
Member

wesleytodd commented Sep 12, 2024

I get that we need to fix the new throw, but based on the test and investigating the code here I dont think express.raw would have been called. Can you create a full code example which I can run on [email protected] which shows that the body is populated and that body-parser-xml is not the thing reading the raw body? We dont maintain that library, so I have no idea what it could be doing.

@blakeembrey
Copy link
Member

blakeembrey commented Sep 12, 2024

@az-faro I think we understand that, but if you commented out app.put(express.raw({ type: '*/*' })) the application probably works, in which case I'd be inclined to say either the documentation is wrong and fix that, or Express is wrong and fix that instead. Either way, I think this exposed a bug in your code and isn't a bug in the libraries.

How do you expect app.put(function () {}) to work? Is it just that you want to run that one middleware on PUT only?

@wesleytodd I'm inclined to fix this in Express and make it work, since it was documented to work, and the fact that it may break existing users relying it being broken isn't a breaking change.

@az-faro
Copy link
Author

az-faro commented Sep 12, 2024

@blakeembrey ok, I removed app.put(express.raw({ type: '*/*' })); and indeed the program still works fine without it. It was written a while ago, so I can't say for certain why I thought it was needed, apparently it isn't.

@alewitt2
Copy link

yes i believe the intention is the middleware get applied to all requests to the specified METHOD. So updating express to start matching correctly seems the right approach. This would be a bug fix, not a breaking change since thats what the document says it should be doing.

@blakeembrey
Copy link
Member

@alewitt2 We agree, but your application will start doing something it didn't do before, which could break your application. For example, if you change app.put to app.use and it doesn't work, us fixing this will break your application. Would this be a bad outcome for you?

@NewEraCracker
Copy link

NewEraCracker commented Sep 12, 2024

It does depend.

Did it ever work on v4? If it didn't, I would recommend against fixing, just prevent the crash and emit a warning.

Fix on v5 please, or ensure it works okay, and make a note on release as a breaking change.

Nice work. My two cents.

@alewitt2
Copy link

yes understood. Not sure you want my opinion here 😆 as i am a bit of a purist when it comes to semver.. but since you asked, I don't think it matters if its a bad outcome for me. I think the feature is a good one and it should be fixed to work properly. and bug fixes arent breaking changes.

@wesleytodd
Copy link
Member

bug fixes arent breaking changes

I wish this was the case, but we have many times (like this one) had cases where what we considered a bug fix broke folks, and we always try to follow strict semver unless it is a critical security issue.

@alewitt2
Copy link

do what you think is best, but i would just fix it in 4x

@alewitt2
Copy link

fwiw: to work around this issue we just updated our code to this and all still seems to be working

  app.put('*', _.validate, _.jsonOnly);
  app.post('*', _.validate, _.jsonOnly);
  app.patch('*', _.validate, _.jsonOnly);
  app.delete('*', _.validate, _.jsonOnly);

@alewitt2
Copy link

alewitt2 commented Sep 12, 2024

also the "breaking change" is that people's code would actually be getting called. I don't think thats a bad end result

@ctcpip
Copy link
Member

ctcpip commented Sep 12, 2024

also the "breaking change" is that people's code would actually be getting called. I don't think thats a bad end result

it certainly CAN be. when you put it that way, this seems like a strong argument for not changing behavior until a semver major

@blakeembrey
Copy link
Member

From looking over the various places that resulted in this bug, I believe this was never intended to work and the documentation is wrong. As a result, I'm working through these steps and you should remove these lines of code from your application since they didn't do anything before:

  1. Introduce a clearer error in path-to-regexp
  2. Remove this documentation from expressjs.com (which is under app.use, I believe it was a mistaken clarification)

If you want this code that previously did not run, to run, you can set the route to * as mentioned here: #5955 (comment)

@wesleytodd
Copy link
Member

I will also push my test which proves this should error, and we can merge that after we update to the version with the new error.

@alewitt2
Copy link

sounds good 👍 thanks for digging into this everyone.

@blakeembrey blakeembrey removed the bug label Sep 12, 2024
@wesleytodd
Copy link
Member

wesleytodd commented Sep 12, 2024

Ha, even better is that .get without a path is an alias for getting a setting. These variadic apis are so bad. We will be removing all of these going in some future major. This particular one I even tried to fix back in 2018 but it never landed.

@blakeembrey
Copy link
Member

Yep, this would have errored in early 4.x releases the same way it does today, it looks like I inadvertently introduced coercion in pillarjs/path-to-regexp@943ceb7. It's possible someone saw it not erroring and assumed it worked, and updated the docs as a result.

@wesleytodd
Copy link
Member

In a more positive note, this helped uncover bugs in application code. So if you are landing on this issue with a similar problem, take a look at the behaviors you are expecting out of requests which hit these middleware and spend some time ensuring your application code is working as you intend. Sorry the docs were wrong and that we introduced a throw where it used to silently noop, but IMO this is as good an outcome as we could hope for.

@zmagyar
Copy link

zmagyar commented Sep 19, 2024

As it was documented on this way therefore I assume thousands of developers are using it on this way in their apps. Then a minor release breaks it and most likely thousands of apps. I understand the reasoning behind the change but it seem to be a major breaking change and should have not be in a minor release..

@steveetm
Copy link

steveetm commented Sep 19, 2024

but IMO this is as good an outcome as we could hope for.

No, this is the worst. This is why people start pinning modules and don't get potentially important CVE fixes, because libraries keep breaking with minor upgrades. This is nonsense. Add a warning, keep it as it was, increase major, but do not break with minor, please.

@NewEraCracker
Copy link

Yep, this would have errored in early 4.x releases the same way it does today, it looks like I inadvertently introduced coercion in pillarjs/path-to-regexp@943ceb7. It's possible someone saw it not erroring and assumed it worked, and updated the docs as a result.

« committed 10 years ago » Let that sink in.

but IMO this is as good an outcome as we could hope for.

No, this is the worst. This is why people start pinning modules and don't get potentially important CVE fixes, because libraries keep breaking with minor upgrades. This is nonsense. Add a warning, keep it as it was, increase major, but do not break with minor, please.

I've been doing a variety of things, based on each project working, or going kaboom, or being so big that I cannot check everything.

  1. Patch it (as is) Crash after starting using version 4.21.0 #5955 (comment)
  2. npm 8+ overrides
"overrides": {
  "encodeurl": "~2.0.0",
  "path-to-regexp": "^0.1.11"
},

See: pillarjs/send#240 & #5956 and you may want to add the following patch at your discretion:

diff --git a/node_modules/path-to-regexp/index.js b/node_modules/path-to-regexp/index.js
index 750c2bf..480fd9a 100644
--- a/node_modules/path-to-regexp/index.js
+++ b/node_modules/path-to-regexp/index.js
@@ -65,11 +65,11 @@ function pathToRegexp(path, keys, options) {
     return new RegExp(path.join('|'), flags);
   }
 
-  if (typeof path !== 'string') {
+  if (strict && typeof path !== 'string') {
     throw new TypeError('path must be a string, array of strings, or regular expression');
   }
 
-  path = path.replace(
+  path = String(path).replace(
     /\\.|(\/)?(\.)?:(\w+)(\(.*?\))?(\*)?(\?)?|[.*]|\/\(/g,
     function (match, slash, format, key, capture, star, optional, offset) {
       pos = offset + match.length;

My two cents.

@blakeembrey
Copy link
Member

As it was documented on this way therefore I assume thousands of developers are using it on this way in their apps.

The old behavior meant the code did not run. It literally did not work. So “thousands of developers” never tested their code? You can just delete the lines that have this error and it works exactly the same.

Add a warning, keep it as it was, increase major, but do not break with minor, please.

In this world, what do you see as breaking? If we “fixed” the bug and made it work as documented, it might break much more because your code never ran before. And since it has never run, it probably isn’t being tested, so that could be much worse to run untested code for the first time than to expose an error.

Arguably it could keep silently failing but that isn’t good for anyone either. Code you write should work and should run as expected.

@dsi2dldevel
Copy link

dsi2dldevel commented Sep 23, 2024

Hello,

Rather than patching a lib, #5975 describes a similar issue with a proposed fix.

The way the app binds routes to a controller causes the bug. Small patch in middlewaring resolves the case.

@NewEraCracker
Copy link

NewEraCracker commented Oct 1, 2024

#5955 (comment)

but IMO this is as good an outcome as we could hope for.

No, this is the worst. This is why people start pinning modules and don't get potentially important CVE fixes, because libraries keep breaking with minor upgrades. This is nonsense. Add a warning, keep it as it was, increase major, but do not break with minor, please.

YES. See here for a real life example: wll8/mockm@c06069b#comments

@blakeembrey
Copy link
Member

blakeembrey commented Oct 10, 2024

I'm closing this out as it's resolved as best it can be done for now. This is a good reminder for libraries like Express to avoid generating methods that conflict with native JS methods. It looks like most of the gotchas come from trying to call .bind expecting to be a JS function bind, but it's actually the HTTP method BIND.

In Express 6 I recommend we remove the magical method generation and stick to shorthand for the common methods, allowing a longer .route("BIND", ...) if someone wants to use a BIND method.

@blakeembrey
Copy link
Member

See here for a real life example

For that specific real life example I fixed their dependency with httptoolkit/httpolyglot#3, they just need to update.

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

No branches or pull requests

9 participants