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

[jnsppp]

No description provided.

[bithavoc]

so it's a design flaw, that's the reason for the major version bump?

[jnsppp]

@bithavoc no, but what if someone intentionally used the templating syntax in loggerOptions.msg? The template vars will not be resolved anymore, which is kinda breaking, especially if you have built alerting or monitoring process based on your logs.

However, in the end, I don't have a very strong opinion on the version - could also be a patch or minor, if you say so. Just let me know and I'll update the PR. The important part is that lodash template engine is not invoked so that it cannot be exploited.

[bithavoc]

I think it's a good justification for a major version bump, it's like giving folks too many ways to shoot themselves in the foot.

Can you add to the Readme an example of the migration path and an example user who might be affected by this change?

[jnsppp]

Yes, will do. I will also check the other lines, where the templating is used. Quite sure, this here is a problem, too: https://github.com/bithavoc/express-winston/blob/main/index.js#L134

[jnsppp]

Yes I think, that is actually the case. Where ever the templating engine is used with req.url this vulnerability is there.

Given this, I would propose to replace any occurrences of {{req.url}} with {{req.route.path}}. See https://expressjs.com/en/4x/api.html#req.originalUrl and https://expressjs.com/en/4x/api.html#req.route.

This justifies the major version bump even more. But it also takes more time to prepare the PR, since a lot of tests need to be adapted. I'll let you know, once I have an update for you

[jnsppp]

a28a4cf

[jnsppp]

@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 #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.

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]

Hey @bithavoc any update on this?