bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
797 stars 186 forks source link

Security Fix | https://github.com/bithavoc/express-winston/issues/266 #284

Open jnsppp opened 8 months ago

jnsppp commented 8 months ago

@bithavoc

I just pushed a commit that replaces the {{req.url}} with {{req.route.path}} all over the place, because {{req.url}} might open a Pandora's box, as explained in https://github.com/bithavoc/express-winston/issues/266.

Therefore, no user provided input is logged anymore by default, but only the route itself. This is safe as it's a string and a client cannot exploit it. However, there's one edge case, where this might break, but I got it covered. Please see the following example:

// Middleware not associated with a specific route
app.use((req, res, next) => {
    // req.route will be undefined here
    try {
      console.log(req.route.path); // ! This would throw if not handled in the package
    } catch (error) {
      console.error('Error:', error.message);
    }
    next();
});

To prevent it from failing, I introduced a ternary in the template:

'{{req.method}} {{req.route?.path ? req.route.path : "invalid route"}}'

This is not only handling this, but also highlighting if express could not handle a specific route.

image

Additionally, I consolidated the regex that was used for the template engine. There were two, but I went for the more inclusive one, since there's no reason two use different ones.

The section in the Readme.md is still pending, will come back to this later. - Update: done ✅

jnsppp commented 8 months ago

Hey @bithavoc any update on this?