Release: 5.2.1

[UlisesGascon]

Important

The prior release (5.2.0) included an erroneous breaking change related to the extended query parser. There is no actual security vulnerability associated with this behavior (CVE-2024-51999 has been rejected). The change has been fully reverted in this release.

[chj957654195]

why revert the security patch?

[RedShift1]

Wow this is sus AF.

[clemens]

@UlisesGascon @jonchurch @ctcpip Could you guys provide a reason why this was reverted? I'm reading in #6931 that the change is a semver major => does that mean you'll release 6.0.0 with that change in place? Or what's the plan here?

[earthlingdavey]

Hi, why revert a security patch, without explanation?

[bogy0]

Reverting that commit brings back allowPrototypes: true, which reintroduces the prototype-pollution vulnerability fixed in CVE-2024-51999.

Even if the original fix was considered semver-major, this rollback effectively restores a known security issue in the v5 line.

Could the @jonchurch, @UlisesGascon clarify:

• how downstream users are expected to mitigate this vulnerability in v5, or
• whether an alternative, non-breaking fix is planned?

This reversion has real security implications, so any guidance would be appreciated.

[sefinek]

Good thing I checked the changelog. Due to the lack of any explanation, I’m sticking with version 5.2.0 for now, the whole situation feels rather suspicious. At this point, I can’t exclude the possibility that something malicious might have been injected into the npm package

[UlisesGascon]

Sorry for the earlier lack of clarity regarding the commit revert and the subsequent release. We decided to release this version with the revert because:

• We have initiated the process to reject the CVE. This may take some time, as it was submitted through the GH CNA instead of the OpenJS CNA.
• The commit is not fully compatible with a semver-minor release and may break certain users who rely on the constructor in specific edge scenarios.

The underlying issue is that I included that commit prematurely in the releases (4.22.0 and 5.2.0) . That CVE was created before we introduced the Incident Response Plan (ref: expressjs/security-wg#56) and was never migrated to follow the updated process. We are considering preparing a brief post-mortem to clarify what happened and to outline some changes we plan to implement to prevent this root cause from occurring again in the future.

[RedShift1]

But this leaves 5.2.1 and other versions except for 5.2.0 still subject to a quite serious vulnerability?

[krzysdz]

But this leaves 5.2.1 and other versions except for 5.2.0 still subject to a quite serious vulnerability?

As far as I understand the low severity "vulnerability" is that properties that exist in object's prototype could be overwritten (e.g. someObj.hasOwnProperty), but this is NOT protoype pollution, which would require overwriting the keys in the prototype (someObj.__proto__.hasOwnProperty which would change the prototype for all objects with the Object prototype).

Let's say you have code like this (maybe one that makes sense):

app.set('query parser', 'extended');
app.use((req, res, next) => {
    console.log(req.query.hasOwnProperty === 'abc');
    next();
});

This worked fine and with the patch still works as the author intended - it expects hasOwnProperty to be a specific string (if not set it would be a function from the prototype, but we know about this and ignore it).

Now imagine that you have instead:

app.set('query parser', 'extended');
app.use((req, res, next) => {
    console.log(req.query.hasOwnProperty('foo'));
    next();
});

Without the patch this will throw an error for ?hasOwnProperty=abc, because req.query.hasOwnProperty will be 'abc' instead of Object.prototype.hasOwnProperty (and that's why you should use Object.prototype.hasOwnProperty.call(req.query, 'foo') - it works with null-prototype objects too).

Now comes the patch and solves the problem - req.query has null prototype, so hasOwnProperty will either be undefined or whatever was parsed from the query. You don't have to worry about your code not working sometimes - it won't work at all! This is the breaking change which makes the change unsuitable for inclusion without changing major version.

Disclaimer: I wrote this on my phone, while doing other things, so there may be mistakes in this response, as I haven't run any code to verify it.

[ctcpip]

Important

The prior release (5.2.0) included an erroneous breaking change related to the extended query parser. There is no actual security vulnerability associated with this behavior (CVE-2024-51999 has been rejected). The change has been fully reverted in this release.

[blakeembrey]

@RedShift1 I just wanted to jump in clarify that there wasn't a vulnerability here. There should be a post-mortem on why it was released at all, but GHSA-pj86-cfqh-vqx6 is invalid because there isn't any prototype pollution or other vulnerability.

@krzysdz's response has a great explanation on why it's not a vulnerability, and I wanted to further add that this is just the behavior of JavaScript and anything that parses user supplied data, such as JSON.parse. Any JavaScript object can have any key name, such as { constructor: 'example' }, and it's a reason to always validate input before operating on it.

The issue that can occur with your code is that you might want to do something like obj.hasOwnProperty('test') but you never checked that hasOwnProperty is a function. That is a problem when using JavaScript, but isn't what was patched with the original PR, all it did was make sure it was always broken.

If you are concerned about object keys that match the names of prototype keys, you can provide your own parser:

app.set('query parser', function (str) {
  return qs.parse(str);
});

But keep in mind that if you expect a key that matches one on the prototype, such as constructor, it will no longer be included in the JavaScript object.