Skip to content

fix: require patched Node.js runtimes - #554

Merged
rahuls-db merged 1 commit into
mainfrom
rahul-singhal/require-patched-node-runtimes
Oct 29, 2026
Merged

rahuls-db merged 1 commit into
mainfrom
rahul-singhal/require-patched-node-runtimes

Conversation

@rahuls-db

@rahuls-db rahuls-db commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

What did you change, and why?

Why: Node.js versions affected by CVE-2026-111497 bundle a vulnerable Brotli decoder. The driver should only advertise support for release lines containing the upstream fix.

Change: Restrict engines.node to Node.js 22.23.4+, 24.22.1+, 26.12.1+, and later releases. Update public requirements, security documentation, and all repository-owned Node setup jobs consistently. CI now pins the exact first patched release in each supported line instead of resolving a potentially vulnerable cached major version.

This is a breaking runtime-support change and must ship in the next major release, not a minor or patch release.

How do you know it works?

Testing: Parsed both package files and validated the semver range at every vulnerable/fixed boundary, including acceptance of Node.js 27 and 28. Verified all changed Markdown and YAML with Prettier, searched repository CI and e2e documentation for stale generic/vulnerable Node selections, and ran git diff --check.

@rahuls-db
rahuls-db marked this pull request as ready for review October 29, 2026 00:34
@rahuls-db
rahuls-db requested a review from vuanhphung October 29, 2026 00:36

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Medium · 1 Low

Looks correct — the semver range (>=22.23.4 <23 || >=24.22.1 <25 || >=26.12.1) accurately encodes the intended support matrix (accepts 22.23.4+/24.22.1+/26.12.1+ incl. 27/28, excludes 20/21, early vulnerable 22/24/26 patches, and 23/25), and package.json/package-lock.json stay consistent. One Medium: the change wasn't propagated to README's "Node.js 20 or newer" line or the Node-20 CI matrices in main.yml; plus a Low noting the breaking engines bump warrants a major version release.

Comment thread package.json Resolved
Comment thread CHANGELOG.md Resolved
@rahuls-db
rahuls-db force-pushed the rahul-singhal/require-patched-node-runtimes branch from 5c87d13 to 7944e7e Compare October 29, 2026 01:02

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 2 Low

Looks good — the engines.node semver range is correct at every vulnerable/fixed boundary and package.json, package-lock.json, README, and CHANGELOG are consistent. Two low-severity stale references were left behind by the version bump: a CI matrix comment and a note in SECURITY-OVERRIDES.md, both still citing the old Node-20 floor.

Other findings

  • 🔵 Low — The serialize-javascript entry notes the patched line "requires Node ≥ 20, which is satisfied by engines.node >= 20." After this PR, engines.node is no longer >= 20 — it is >=22.23.4 <23 || >=24.22.1 <25 || >=26.12.1. The substantive claim still holds (the new 22.23.4 floor is ≥ 20), but the literal engines.node >= 20 reference is now incorrect. Worth updating this note so the doc stays accurate alongside the version bump.

Comment thread .github/workflows/main.yml Resolved
@rahuls-db
rahuls-db force-pushed the rahul-singhal/require-patched-node-runtimes branch from 7944e7e to bc57e17 Compare October 29, 2026 01:10

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Medium · 1 Low

Looks solid — the new engines.node range is correct and consistent across package.json, package-lock.json, README, CHANGELOG, and SECURITY-OVERRIDES (correctly excludes odd/EOL lines 23/25 while accepting future majors). Two residual Node 20 references were missed despite the PR's claim of a consistent matrix update: kernel-e2e.yml (medium — still runs the driver e2e suite on the now-unsupported runtime, and is triggered by this PR's package.json change) and tests/e2e/README.md (low — stale matrix doc).

Other findings

  • 🟡 Medium — This PR drops Node 20 from engines.node and from the main.yml unit/e2e matrices, but kernel-e2e.yml still pins node-version: 20 for the job that runs the driver's e2e kernel suite. Two concrete consequences:
  1. It now tests an unsupported runtime. The e2e matrix comment in main.yml says the Node matrix exists specifically to catch "Node-version-specific behaviors." This job runs npm ci + the full tests/e2e/kernel/** suite against the driver on Node 20 — a line this PR declares unsupported (and CVE-exposed).

  2. This very PR triggers that job on Node 20. detect-changes runs the real suite when package.json or package-lock.json change (grep at the detect-changes step), both of which this PR touches — so the merge-queue kernel-e2e run will npm ci a package declaring >=22.23.4 on Node 20 and emit EBADENGINE warnings.

Bump this to 22 (or one of the supported lines) to match main.yml.

  • 🔵 Low — Stale documentation: this line states the e2e job "runs as a matrix across Node 20/22/24/26," but this PR changed the main.yml e2e matrix to ['22', '24', '26']. Update to drop 20 so the doc stays accurate.

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 High · 1 Low

Mostly a clean config/docs change, and the engines.node range (>=22.23.4 <23 || >=24.22.1 <25 || >=26.12.1) correctly admits the patched even lines plus future releases (27/28) while excluding EOL 23/25. One real regression (F1): pinning the e2e CI matrix to dotted full versions injects . into E2E_TABLE_SUFFIX, which is interpolated unquoted into SQL table identifiers and will break the entire e2e matrix. Plus one low-severity stale-doc note (F2).

Other findings

  • 🔵 Low — Stale Node version descriptor. This PR raises the floor to >=22.23.4 but two repo-owned bot prompts still describe the connector as Node >= 20:

  • .bot/prompts/engineer/system.md:13 — "A TypeScript connector (@databricks/sql, Node >= 20)..."

  • .bot/prompts/engineer-followup/system.md:22 — "TypeScript connector (@databricks/sql, Node >= 20)..."

The PR description claims all repository docs were searched for stale Node selections; these two were missed. Low impact (internal agent context only), but worth updating for consistency with the new supported range. Note the historical CHANGELOG.md:40 entry ("engines.node raised to >=20.0.0") is a past-release record and should NOT be changed.

Comment thread .github/workflows/main.yml Resolved
Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
@rahuls-db
rahuls-db force-pushed the rahul-singhal/require-patched-node-runtimes branch from 42548b8 to c0ff18d Compare October 29, 2026 01:27

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

@rahuls-db
rahuls-db enabled auto-merge October 29, 2026 01:31
@rahuls-db
rahuls-db added this pull request to the merge queue Oct 29, 2026
Merged via the queue into main with commit 1f3deb7 Oct 29, 2026
23 checks passed

This branch was successfully deployed

1 active deployment
azure-prod — c0ff18d9 Deployed Oct 29, 2026 by rahuls-db via e2e-test (22.23.4, 22_23_4) #1703
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants