PayU / prometheus-api-metrics

API and process monitoring with Prometheus for Node.js micro-service
Apache License 2.0
130 stars 44 forks source link

TypeError: Cannot read property 'path' of undefined #59

Open ha-akelius opened 4 years ago

ha-akelius commented 4 years ago

we are working on Strapi and we are using strapi-plugin-prometheus-metrics
but we are getting TypeError: Cannot read property 'path' of undefined when the URL is not exist (status code is 404)

node_modules/prometheus-api-metrics/src/koa-middleware.js:98 let route = properRoute.path;

is there a way to fix it?

kobik commented 4 years ago

Hi @ha-akelius, have you checked with the plugin developer?

anyway a stacktrace would be helpful.

ha-akelius commented 4 years ago

@kobik thank you for your reply,

I didn't check the plugin developer but I debug it and did stacktrace with debugging.

my question (before asking the plugin developer) why we don't do a check for undefine in line https://github.com/PayU/prometheus-api-metrics/blob/master/src/koa-middleware.js#L98

wxsms commented 3 years ago

I have meet the same issue. the server just crashed in this case.

wxsms commented 3 years ago

This happends when a (.*) route is defined on a koa app, and visit with a undefined route (404). Then server crashed.

        let route = properRoute.path;
                                ^

TypeError: Cannot read property 'path' of undefined
wirehead commented 3 years ago

Yeah, as the plugin developer in question, @wxsms seems to be correct.

Here's a minimal example case:

const Koa = require('koa');
const Router = require('koa-router');
const apiMetrics = require('prometheus-api-metrics').koaMiddleware;

const app = new Koa();
const router = new Router();

router.get('(.*)', (ctx, next) => {
  // ctx.router available
  ctx.body = 'Hello World!';
  next()
});

app
  .use(router.routes())
  .use(router.allowedMethods())
  .use(apiMetrics());

app.listen(3000);

And here's the associated package.json:

{
  "name": "koa-debug",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "koa": "^2.13.0",
    "koa-router": "^10.0.0",
    "prom-client": "^12.0.0",
    "prometheus-api-metrics": "^3.1.0"
  }
}

And here's the stacktrace:

C:\Users\wirehead\Documents\src\koa-debug\node_modules\prometheus-api-metrics\src\koa-middleware.js:98
        let route = properRoute.path;
                                ^

TypeError: Cannot read property 'path' of undefined
    at KoaMiddleware._findFirstProperRoute (C:\Users\wirehead\Documents\src\koa-debug\node_modules\prometheus-api-metrics\src\koa-middleware.js:98:33)
    at KoaMiddleware._handleSubRoutes (C:\Users\wirehead\Documents\src\koa-debug\node_modules\prometheus-api-metrics\src\koa-middleware.js:79:26)
    at KoaMiddleware._getRoute (C:\Users\wirehead\Documents\src\koa-debug\node_modules\prometheus-api-metrics\src\koa-middleware.js:63:26)
    at KoaMiddleware._handleResponse (C:\Users\wirehead\Documents\src\koa-debug\node_modules\prometheus-api-metrics\src\koa-middleware.js:39:28)
    at ServerResponse.ctx.res.once (C:\Users\wirehead\Documents\src\koa-debug\node_modules\prometheus-api-metrics\src\koa-middleware.js:135:18)
    at Object.onceWrapper (events.js:286:20)
    at ServerResponse.emit (events.js:203:15)
    at onFinish (_http_outgoing.js:671:10)
    at process._tickCallback (internal/process/next_tick.js:61:11)

I think that there are completely valid non-user-error cases where a person would want to create a (.*) route and would want the api-metrics middleware to record that as such. (But, obviously, you don't want to actually record those routes as anything other than (.*) because you don't want a potential attacker being able to bring down your Prom cluster and server processes by spewing tons of invalid URLs)

wxsms commented 3 years ago

I think that a try catch should be applied inside the middleware. As a metrics tool it should not crash the app or block normal flow in any case, which makes user nervous.