jochen-schweizer / express-prom-bundle

express middleware with standard prometheus metrics in one bundle
MIT License
303 stars 67 forks source link

Allow a path whitelist option #90

Closed RyanBard closed 3 years ago

RyanBard commented 3 years ago

Thanks for the extremely useful middlware!

I've run into a potential issue when using it, however.

When turning on the includePath option, I don't see a convenient way to allow only the paths I want. This is important to have for security reasons. A malicious caller could flood your endpoint with randomly generated urls that would 404, but also create metrics for each of those paths (which could get really expensive in your cloud provider).

I see a way to exclude paths via excludeRoutes and that I can specify anything I want with bypass, however, I don't see something more convenient like a whitelistRoutes, allowRoutes, includeRoutes, or onlyIncludeRoutes.

I see that a whitelist and blacklist used to be in the code, but was replaced with only excludeRoutes: https://github.com/jochen-schweizer/express-prom-bundle/commit/c6d59647686c4271a2d620925ebb0ea1eb52890f

How to workaround with bypass:

app.use(promBundle({
    bypass: req => {
        const onlyIncludeRoutes = ['/foo', '/foo/', /^\/bar\/?$/];
        return !onlyIncludeRoutes.some(route => {
            if (route instanceof RegExp) {
                return route.test(req.url);
            } else {
                return route === req.url;
            }
        })
    }
}));

Better workaround:

app.use(['/foo', '/bar'], promBundle({}));

Proposed feature: just add this in-between bypass and excludeRoutes: https://github.com/jochen-schweizer/express-prom-bundle/blob/1ec800cde7867ed3e4e7966f6de8fc7251dcf7fd/src/index.js#L175

if (opts.onlyIncludeRoutes && !matchVsRegExps(path, opts.onlyIncludeRoutes)) {
    return next();
}

Thanks again for awesome middleware!

RyanBard commented 3 years ago

Here is a PR to add this feature (and update the tests and types):

Let me know if you'd like the feature, but want it a different way (or if I should add something to the PR like bumping the version).

disjunction commented 3 years ago

@RyanBard thanks for contributing. I believe it was removed because whitelisting duplicates core express functionality. And i try to keep the library as simple as possible (it already has too many options as to my taste).

You can achieve the same result with the code like this:

app.use(['/metrics', '/foo*'], promBundle())
app.get('/foo/123', handler1) // will be counted by prom bundle
app.get('/bar/567', handler2) // will be ignored by prom bundle

Have you considered this approach?

RyanBard commented 3 years ago

Good point. I didn't think about handling this in express. Thanks! I'll close this issue and the PR.