Skip to content

Conversation

jpw
Copy link
Contributor

@jpw jpw commented Jul 30, 2025

$ nvm use 20
Now using node v20.17.0 (npm v10.8.2)
$ npm ci
npm warn EBADENGINE Unsupported engine {
npm warn EBADENGINE   package: '[email protected]',
npm warn EBADENGINE   required: { node: '>=20.18.1' },
npm warn EBADENGINE   current: { node: 'v20.17.0', npm: '10.8.2' }
npm warn EBADENGINE }
npm warn EBADENGINE Unsupported engine {
npm warn EBADENGINE   package: '[email protected]',
npm warn EBADENGINE   required: { node: '>=20.18.1' },
npm warn EBADENGINE   current: { node: 'v20.17.0', npm: '10.8.2' }
npm warn EBADENGINE }
...

@jpw
Copy link
Contributor Author

jpw commented Jul 30, 2025

Thinking some more, it is implied/expected that using e.g. 20 means the latest version of 20...
Some CI/CD systems do use the value of the engines field.

If we want to be explicit, then I'll update the docs to reflect the patch level compatibility required.

@aarongoldenthal
Copy link
Contributor

Thinking some more, it is implied/expected that using e.g. 20 means the latest version of 20... Some CI/CD systems do use the value of the engines field.

If we want to be explicit, then I'll update the docs to reflect the patch level compatibility required.

It does seem implied (to me at least) that we assume the latest of each major version. If not, it seems like we'd need to dramatically increase the test matrix. It makes sense to me to be explicit in the package for this type of case, and in the docs so people know the expectation.

@josebolos
Copy link
Member

josebolos commented Jul 31, 2025

Hello! @jpw Is the plan to release v5.0.0 after this?

I'm not necessarily against this change but, to be honest, that's not how I've interpreted it in the past. To me, requiring version 20 means... any version of node JS equal or greater than 20.0.0.

We acknowledge that people can run software on platforms that they don't always control, and not everyone is as lucky as we are in that we have a choice of well supported platforms where to run their stuff. There might be reasons why they are not running the very latest (as @jpw's snippet clearly shows).

It has always been our policy that we consider upgrading the requirements a breaking change (so in the past, releasing this would require the release of pa11y-ci 5.0.0), precisely to avoid causing breakage on dependants as it has just happened to us.

I was curious why these packages would request 20.18.1 or greater as it's a very odd and specific version to request. Apparently there are several critical vulnerabilities that were being actively exploited in the wild that were patched in that version. Apparently when undici dropped support for Node 18 for the release of version 7 they decided to require 20.18.1 or greater, which is absolutely fine.

Cheerio must have upgraded their copy of undici without checking the requirements, which caused the same breakage that we're now experiencing, so cheerio decided to upgrade their requirements accordingly without making it a breaking change (they are still at v1 so they don't seem to consider this "breaking").

I think it's unfortunate that we didn't catch this in time before releasing 4.0.0, but if we're not planning to release v5.0.0 immediately this goes against our established policy. I also personally think that it would not be fair on users of pa11y.

If we want to establish this as a new policy, fair enough, but it should probably be documented somewhere.

@jpw jpw marked this pull request as draft July 31, 2025 11:22
@jpw
Copy link
Contributor Author

jpw commented Jul 31, 2025

Thanks @josebolos and @aarongoldenthal - good points!

I've updated the PR to add a top limit to the semver range, to explicitly include what we currently test against: 20 - 24.

I've also converted the PR to a draft, while we discuss further.

My thoughts:

  • Specifying a version of something as X not X.Y.Z is not valid semver so we are in the realms of opinion and convention here.
  • I tend to think it's better to be explicit with versioning issues like this; plus, it's easy to do.
  • Agree that X does/may reasonably imply "any version of X".
  • Agree that strictly speaking this would be a breaking change.

Do we think this is important enough to release soon, or should we wait until there are more changes worthy of a major version release?

@aarongoldenthal
Copy link
Contributor

@josebolos @jpw

I took a look at the Node release docs to see if they were clear on what they supported under the banner of LTS, but they really aren't and I can see why people have different interpretations. One thing I've noted before is that they do transition a version from Current to LTS with a specific release, so you can see v20.9.0 was the start of LTS for Node 20, v22.11.0 was the start for Node 22, etc. That's probably why I think of LTS really being the latest for that major version, but I can certainly also see the other side.

It does seems like in either case we should be specific in engine compatibility to avoid confusion. With that, I wonder if we need to go even further than "node": ">=20.18.1 <25" to match the LTS (even) versions that we call out in the README, something like "node": "^20 || ^22 || ^24" (with more version specificity if needed). Then people would get EBADENGINE warnings if still using 21/23 to match the docs.

I wonder if one option as a short-term fix without another major release, and to give time to sort this out, is to revert back to [email protected], the last on undici@^6. Undici did resolve CVE-2025-22150 in v7.2.3, but they also backported the fix to v6.21.1, and v6 supports "node": ">=18.17" so we could fix it with a v5.0.1. I didn't read all of the undici and cheerio release notes, but that was the only issue that stood out to me when I scanned through, and ultimately we're only using cheerio to parse sitemaps, so it seems like that limits the risk of unknown/untrusted inputs.

@josebolos
Copy link
Member

Yeah, the way the Node.js project managed LTS versions is bizarre because I don't know any other projects that work in that way. If a project decides that version X is going to be LTS, it's usually from the get go so every version including x.0.0 will be considered LTS.

To be honest we decided to support only LTS versions of node on this project because it became quite clear very early that there was no capacity, and no benefit on having to update supported versions every time a new one is released every 6 months. We made this decision based on how every other open source project manages LTS versions but the fact that Node manages this in a slightly different and, to be honest, unnecessarily confusing way hasn't been much of a problem until now.

I'm a fan of keeping things as simple as possible so I think that downgrading undici is probably the simplest and therefore best option.

@jpw
Copy link
Contributor Author

jpw commented Aug 15, 2025

undici is an HTTP client, and the CVE refers to potential issues using undici to post multipart form data to a malicious website.

As @aarongoldenthal says, we use cheerio (which has undici as a dependency) to parse sitemap XML so I can't think how users of pa11y-ci could be exposed to the vulnerability.

False positives from security tools are unwelcome noise (npm audit isn't flagging this, I suppose other tools might). But, I don't see a release of cheerio that uses any of the patched versions of undici so I'm not sure what we can do about that.

I wonder if one option as a short-term fix without another major release, and to give time to sort this out, is to revert back to [email protected]

I think so too, looks like we are all agreed! I will prepare a release.

@jpw jpw mentioned this pull request Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants