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

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

Open
ha-akelius opened this issue Oct 15, 2020 · 6 comments
Open

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

ha-akelius opened this issue Oct 15, 2020 · 6 comments

Comments

@ha-akelius
Copy link

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
Copy link
Collaborator

kobik commented Oct 18, 2020

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

anyway a stacktrace would be helpful.

@ha-akelius
Copy link
Author

ha-akelius commented Oct 19, 2020

@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
Copy link

wxsms commented Dec 9, 2020

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

@wxsms
Copy link

wxsms commented Dec 9, 2020

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
Copy link

wirehead commented Dec 29, 2020

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
Copy link

wxsms commented Dec 29, 2020

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.

kdhira added a commit to kdhira/prometheus-api-metrics that referenced this issue Feb 21, 2022
In the cases where the request route is not matched to any Koa route,
bail out before trying to format the path string.

Issue: PayU#59
kdhira added a commit to kdhira/prometheus-api-metrics that referenced this issue Mar 1, 2022
kdhira added a commit to kdhira/prometheus-api-metrics that referenced this issue Mar 8, 2022
kobik pushed a commit that referenced this issue Mar 8, 2022
* Check if properRoute is found before acting on it

In the cases where the request route is not matched to any Koa route,
bail out before trying to format the path string.

Issue: #59

* Add test for koa middleware unmatched route scenario

Issue: #59

* Add integration test for wildcard path scenario

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

No branches or pull requests

4 participants