Skip to content

2.0.0: breaking security bump — Node >=20, thrift 0.23, uuid 11, lz4→lz4-napi (0 OSV findings)#390

Merged
vikrantpuppala merged 16 commits into
mainfrom
vp/security-bump-runtime-and-dev
Jul 14, 2026
Merged

2.0.0: breaking security bump — Node >=20, thrift 0.23, uuid 11, lz4→lz4-napi (0 OSV findings)#390
vikrantpuppala merged 16 commits into
mainfrom
vp/security-bump-runtime-and-dev

Conversation

@vikrantpuppala

@vikrantpuppala vikrantpuppala commented May 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Breaking security release (2.0.0) — completes the vulnerability cleanup, taking the lockfile to 0 OSV-Scanner findings. The non-breaking #446 (1.17.0) shipped first and cleared the criticals without breaking changes; this release rebases onto main and finishes the job with the breaking version-floor bumps.

Breaking changes

Change From → To Why
engines.node >=14 → >=20 Node 14/16/18 dropped — all past upstream EOL (18 EOL'd 2025-04). >=20 is also the floor for the patched serialize-javascript (dev) and clears the kernel backend's Node 18 requirement.
thrift ^0.16.0 → ^0.23.0 Clears GHSA-r67j-r569-jrwp (HIGH) + GHSA-526f-jxpj-jmg2. Direct runtime dep; fix is a major bump.
uuid ^9.0.0 → ^11.1.1 Clears GHSA-w5hq-g745-h8pq (HIGH 7.5). uuid 11 dual-publishes ESM+CJS; the driver's CJS dist/ still require('uuid') unchanged. An overrides pin holds thrift's transitive uuid at 11 (uuid 13 is ESM-only → would break require()).
lz4 → lz4-napi dropped lz4@^0.6.5 → lz4-napi@^2.9.0 The old lz4 package (unmaintained since 2021) is a node-gyp native addon with no prebuilt binaries — it fails to compile on Node 22+, so it was silently skipped as an optional dep and LZ4-compressed CloudFetch/Arrow results could not be decoded. lz4-napi is maintained, ships napi-rs prebuilds for every supported platform/Node version, and its compressFrameSync/decompressFrameSync are byte-compatible with the LZ4 frame format (magic 0x184D2204) the server sends — verified by round-trip and cross-decode both directions, and confirmed green by the e2e matrix below.

Supporting changes

  • typescript pinned to exact 5.5.4 — uuid@11's .d.ts uses export type (needs TS 5+); <5.6 avoids @types/node Buffer-generic emit changing our published .d.ts.
  • serialize-javascript override ^7.0.5 (its only patched line requires Node ≥20 — now satisfied by the floor bump).
  • @types/node-fetch ^2.6.13; dropped @types/lz4 (lz4-napi ships its own types).
  • OAuth.ts test stub: cast Symbol.asyncDispose through any (ES2018 target predates the symbol; Node 20+ runtime provides it).
  • mocha (tests/unit + tests/e2e): require: ts-node/register + loader: ts-node/esm so Node 24+'s native --experimental-strip-types doesn't intercept .ts specs ahead of ts-node (it can't handle TS parameter properties).
  • CI matrix → [20, 22, 24, 26] for both unit and e2e (fail-fast: false so a version-specific regression can't mask the others); setup-jfrog rewrites lockfile resolved: URLs to the JFrog mirror so protected runners can npm ci.

Net security result

OSV-Scanner v2.3.8 against the regenerated lockfile: No issues found (0 CRITICAL / 0 HIGH / 0 MED / 0 LOW).

Verification

  • npm run build clean (TS 5.5.4)
  • eslint + prettier --check clean
  • unit tests passing on Node 20 / 22 / 24 / 26
  • e2e tests passing on Node 20 / 22 / 24 / 26 (real LZ4-frame decode against the warehouse)
  • osv-scanner = No issues found
  • CI green on the [20, 22, 24, 26] matrix (lint / unit-test / e2e-test / coverage / Kernel E2E / DCO)

Closes #370
Closes #293

This pull request was AI-assisted by Isaac.

This was referenced May 27, 2026
Closed
Closed
Open
@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@vikrantpuppala vikrantpuppala force-pushed the vp/security-bump-runtime-and-dev branch from da4c50c to 439de47 Compare May 29, 2026 06:21
@vikrantpuppala vikrantpuppala force-pushed the vp/security-bump-runtime-and-dev branch from b5b46b2 to f4b11d3 Compare May 29, 2026 10:18
@vikrantpuppala
test(ci): drop invalid node-option flag; ts-node/register alone fixes…
22b49cc 
… Node 24+

The previous commit added `node-option: [no-experimental-strip-types]`,
but that flag doesn't exist before Node 22.6 — it's a "bad option" on
Node 20, crashing the test process at startup and breaking a version that
previously passed.

`require: ts-node/register` alone is sufficient and version-agnostic: it
registers ts-node's CommonJS require-hook so `.ts` resolves through ts-node
on every supported Node, bypassing Node 24+'s built-in ESM strip-types path
(whose strip-only mode rejects TS parameter properties). Verified locally:
34-test OAuth spec (uses parameter properties) passes on both Node 20 and
Node 24 with require-only.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
test(ci): add ts-node/esm loader so Node 24+ handles .ts on the impor…
02994a4 
…t() path

require:ts-node/register alone was insufficient: mocha loads some specs
(e.g. tests/unit/kernel/execution.test.ts) via import() (ESM), not require
(CJS). On Node 24+ those ESM-loaded .ts files are grabbed by Node's built-in
--experimental-strip-types, whose strip-only mode rejects TS parameter
properties. Adding `loader: ts-node/esm` covers the import() path; the
existing `require: ts-node/register` covers the require() path.

Verified locally on BOTH Node 20 and Node 24: the kernel spec (ESM-loaded,
uses parameter properties) and the OAuth spec (CJS-loaded) both pass —
102 passing on each. No version-specific node flags (which would crash
Node < 22.6).

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
fix(deps): trim redundant overrides to minimal 3; fail-fast:false for…
20dbce1 
… diagnosis

Overrides review (per "why override instead of bumping the parent?"):
main's dependency tree already resolves basic-ftp (5.3.1), @75lb/deep-merge
(1.1.4), ip-address (10.2.0), and form-data (4.0.6) to PATCHED versions via
their parents (get-uri, table-layout, socks, node-fetch types) — verified
main's lock has 0 OSV findings with no overrides at all. So those four
overrides were redundant. Removed them.

The minimal necessary override set is 3, each with a specific reason:
- uuid ^11.1.1  — thrift@0.23 declares uuid ^13 (ESM-only, breaks require())
- ws ^8.18.0    — thrift@0.23 pulls ws ^5.2.3 (vulnerable; <8.17.1)
- serialize-javascript ^7.0.5 — dev; only patched line, needs Node >=20

Lock unchanged by the trim (the 4 removed overrides matched default
resolution), OSV still 0 findings.

Also set fail-fast:false on the unit-test matrix so each Node version
reports independently — the lz4 install failure needs per-version signal
(lz4@0.6.5 is a 2021 native addon with no prebuilds; suspect it fails to
build on Node 22+, which main never tested — main only ran [14,16,18,20]).

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
Revert override trim — restore 7-override set consistent with the loc…
2c194ad 
…kfile

The previous commit trimmed overrides to 3 but couldn't regenerate the
lockfile to match (local npm proxy was failing tarball fetches), leaving
package.json out of sync with package-lock.json → CI `npm ci` failed with
EUSAGE "Missing: ip-address@9.0.5 from lock file".

Also, the trim was partly wrong on the merits: ip-address's CVE fix
(GHSA-v2v4-37r5-5v8g) is only in 10.1.1, but its parent `socks` caps it at
`^9.0.5` — so without the override npm resolves the vulnerable 9.x. That
override is genuinely required (parent won't allow the patched major),
which is exactly when an override is the right tool.

Restores the OSV-verified 7-override set matching the committed lock.
`npm ci` clean, osv-scanner 0 findings. The fail-fast:false and ts-node
strip-types fixes from earlier commits remain.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
lz4: replace unmaintained lz4 with maintained lz4-napi (frame-com…
6593cb5 
…patible)

Root cause of the Node 22+ CI failures: `lz4@0.6.5` (2021, unmaintained) is
a node-gyp native addon with NO prebuilt binaries. It fails to compile on
Node 22+, gets silently skipped as an optional dep, then tests that
require('lz4') crash with "Cannot find module 'lz4'". Node 20 passed; 22/24/26
failed — confirmed via fail-fast:false per-version run.

Replaces it with `lz4-napi@2.9.0` (maintained, napi-rs, ships prebuilds ->
no local toolchain build, works on all Node versions). Verified locally that
lz4-napi's compressFrameSync/decompressFrameSync are byte-compatible with the
LZ4 *frame* format (magic 0x184D2204) the old package used and the Databricks
server sends — round-trip and cross-decode both directions, incl. large
buffers. lib/utils/lz4.ts now wraps lz4-napi behind the same { encode, decode }
codec, so call sites are unchanged and the load-failure-tolerant contract is
kept. Also fixes #293 (deprecated Buffer() warning + xxhash build-resolution
error, both from the old lz4 package).

DIAGNOSTIC (this commit): CI temporarily uses `npm install` instead of
`npm ci` so the JFrog registry resolves lz4-napi + its @lz4-napi/* platform
packages (the dev npm proxy doesn't mirror those scoped packages, so the lock
couldn't be generated locally). If CI resolves them, I'll pin a proper lock
and restore `npm ci`.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
ci: complete lz4-napi lockfile, restore npm ci, fix prettier
c619b2f 
The lz4-napi swap CI run proved green on all Node versions (unit + e2e
20/22/24/26), confirming the maintained fork resolves and decodes LZ4
frames correctly where the old node-gyp `lz4` addon failed to build on
Node 22+.

Finalize #390:
- package-lock.json: regenerate against the dev proxy so the lz4-napi
  dependency tree is fully pinned (lz4-napi + 13 @antoniomuso/* platform
  packages + @node-rs/helper + @napi-rs/triples), with resolved: URLs the
  setup-jfrog action rewrites to JFrog. lockfileVersion stays 2. This makes
  the lock npm-ci-valid (the earlier stub was incomplete because @antoniomuso/*
  wasn't resolved).
- main.yml: revert the diagnostic `npm install` back to `npm ci` in all
  three jobs (lint / unit-test / e2e-test) now that the lock is complete.
- package.json: prettier — use the literal em-dash in the prepack message.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
review: address code-review findings on the lz4-napi swap
c357037 
From the Code Review Squad pass on #390:

- F1 (HIGH): package-lock.json — rewrite the 16 lz4-napi tree entries from
  the internal npm-proxy.dev.databricks.com back to registry.npmjs.org, matching
  the ~555 other lock entries so external forks/contributors can `npm ci`. CI's
  setup-jfrog step still rewrites npmjs.org → JFrog at install time. Integrity
  hashes are content-based, so the host swap is safe.
- F2 (test): add a real, unmocked frame-compat suite (tests/unit/utils/lz4.test.ts)
  exercising the driver's codec against the actual lz4-napi — decodes a golden LZ4
  frame produced by the independent `lz4` CLI (proving cross-encoder frame compat,
  not just self round-trip), plus binary round-trips incl. >single-block payloads.
  Runs on every Node version (import-based, no CJS-only skip).
- F4 (HIGH): ArrowResultHandler / CloudFetchResultHandler error text said module
  `lz4` not installed — updated to `lz4-napi` so it no longer misdirects users to
  the removed package.
- F6: README requirement Node.js 14 → 20, matching engines.node >=20.
- F7 (perf): lz4-napi frame APIs already return owned Buffers; drop the redundant
  Buffer.from copy on the CloudFetch decode hot path.
- F10: remove stale devDependency @types/uuid (uuid 11 is self-typed, unreferenced).

F5 (matrix Node 26) is not applicable — Node 26 has GA'd (26.5.0 resolved and both
unit+e2e legs passed), so the leg is real, not a permanently-red phantom.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
review: gate real-codec lz4 tests on availability, not the environment
3318065 
Follow-up to N1 (devils-advocate, conf 85): the `is available` case asserted
getLZ4() is always defined, which tests the environment rather than the code and
breaks the module's deliberately-optional contract. lz4-napi ships prebuilds for
mainstream platforms but not every arch (linux-ppc64le/s390x/…), and
`npm ci --omit=optional` also yields undefined — on any of those the whole unit
run would fail.

Move the availability check into a `before()` that `this.skip()`s (with a visible
console.warn) when the binding can't load, and drop the unconditional assertion.
The frame-compat cases now test the codec when present and degrade gracefully
when absent. CI's linux-x64 matrix always has a prebuild, so the golden-frame and
round-trip coverage still runs there on every push.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@vikrantpuppala
Merge branch 'main' into vp/security-bump-runtime-and-dev
7aa00b7 
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
This was referenced Jul 14, 2026
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gopalldb gopalldb gopalldb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bump thrift dependency past GHSA-r67j-r569-jrwp Deprecated Buffer() Usage Warning in @databricks/sql v1.7.1

2 participants

@vikrantpuppala @gopalldb