why revert the security patch?

Wow this is sus AF.

@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?

Hi, why revert a security patch, without explanation?

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.

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

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.

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

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.

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

Hi all, I’m following up because we did the post-mortem for how we released the invalid advisory and patch before reverting them in this PR. First I want to share more context.

The patch was intended to fix a concern around keys with the same name as prototype properties, and was raised in 2018. This pre-dated current processes and the project’s revival, as well as many of the current maintainers involvement. As part of the V5 release in 2024, the issue was revived, discussed, and a draft advisory with a fix was created. A few days later it was determined that this is not a vulnerability, but all the artifacts were left open (GitHub advisory, private security PR with approval, etc). Last month, when a new release was planned, the advisories were reviewed for things to include, and the lingering advisory from 2024 looked “good” (it was approved and had a clear description) and was included in the release when it shouldn’t have. It was quickly noticed after the initial release, reverted, and released (here) without the change.

This PR should have included a description, and in retrospect the fact it was missing does look alarming. This release was done quickly after noticing the release could be a breaking change, and in sync over a video call for consensus, but it still should have included as much detail as possible for everyone else not involved.

Why did we revert at all? The “fix” in 5.2.0 is a breaking change. Although unlikely, it’s possible a user previously expected to access these prototype properties (e.g. obj.hasOwnProperty) and the change removed the prototype altogether. Having a new TypeError appear in someone’s application that previously worked fine is clearly not acceptable from us as a team.

Side note: We do not recommend using Object.prototype properties on user defined objects without validation. A user can submit anything, e.g. { hasOwnProperty: true }, and this will cause a TypeError in your application if you aren’t expecting it.

Finally, we have a few action items as a team:

• Review our private security triage process to be a better source of truth
• Close any lingering advisories from before this year that are invalid
• Ensure all PRs always have a clear title and description

Because security fixes happen in private, separate from the main repo, context is sometimes hard to follow. We’re confident our current process would not let this happen, but this specific issue pre-dated our current process and should have been closed or migrated long ago.