Skip to content

1.17.0: clear CRITICAL + transitive OSV findings via overrides (non-breaking)#446

Merged
vikrantpuppala merged 3 commits into
mainfrom
vp/security-overrides-1.17.0
Jul 10, 2026
Merged

1.17.0: clear CRITICAL + transitive OSV findings via overrides (non-breaking)#446
vikrantpuppala merged 3 commits into
mainfrom
vp/security-overrides-1.17.0

Conversation

@vikrantpuppala

@vikrantpuppala vikrantpuppala commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Non-breaking security patch on the 1.x line. Adds a package.json overrides block pinning transitive/dev dependencies to patched versions, so consumers can clear their critical/high scanner alerts without waiting for or adopting the breaking 2.0.0 bump (#390).

This directly answers the request on #386 for a patch on the current major.

What this clears

  • 2 CRITICAL: basic-ftp, form-data
  • HIGH / MED / LOW across the dev + transitive tree: ws, braces, micromatch, cross-spawn, serialize-javascript, @babel/*, js-yaml, ip-address, path-to-regexp, diff, ajv, nanoid, minimatch, follow-redirects, @75lb/deep-merge, flatted, picomatch, brace-expansion.

What this intentionally does NOT change (no breaking changes)

  • engines.node unchanged (>=14)
  • No declared-dependency version changes, no runtime-API changes
  • Drop-in patch: npm i @databricks/sql@1.17.0 clears the alerts with zero migration

Residual (deferred to 2.0.0)

Two HIGH findings remain — on direct runtime deps whose fixes are SemVer-major and therefore breaking:

Package Advisory Fixed in Ships in
thrift GHSA-r67j-r569-jrwp, GHSA-526f-jxpj-jmg2 0.23.0 2.0.0 (#390)
uuid GHSA-w5hq-g745-h8pq 11.1.1 2.0.0 (#390)

npm audit confirms both are "breaking change" fixes, which is why they're not in this patch.

Verification

  • OSV-Scanner v2.3.8 against the regenerated lockfile: only thrift + uuid remain (the two 2.0.0 items); all CRITICAL + transitive/dev findings cleared.

Test plan

  • CI green on the Node 18/20/22/24 matrix
  • e2e green

Closes #386
Closes #263

This pull request was AI-assisted by Isaac.

@vikrantpuppala
chore(sec): 1.17.0 — clear CRITICAL + transitive findings via overrid…
43a91fd 
…es (non-breaking)

Non-breaking security patch on the 1.x line so consumers can clear
critical/high scanner alerts without waiting for or adopting the breaking
2.0.0 bump (requested on #386).

Adds a package.json `overrides` block pinning transitive/dev deps to
patched versions. Clears:
- 2 CRITICAL: basic-ftp, form-data
- HIGH/MED/LOW across the dev + transitive tree (ws, braces, micromatch,
  cross-spawn, serialize-javascript, @babel/*, js-yaml, ip-address,
  path-to-regexp, diff, ajv, nanoid, minimatch, follow-redirects, ...)

No engine, runtime-API, or declared-dependency changes -> drop-in patch.

The two remaining HIGH findings are on direct runtime deps whose fixes are
SemVer-major (thrift 0.16->0.23, uuid 9->11); those, plus the engines
>=18 bump, ship in 2.0.0 (#390). Verified with osv-scanner v2.3.8: only
thrift + uuid remain (both "breaking change" per npm audit).

Closes #386
Closes #263

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala vikrantpuppala mentioned this pull request Jul 10, 2026
Merged
6 tasks
@vikrantpuppala
fix: coherent dev-toolchain for 1.17.0 overrides (mocha/eslint bump)
208527f 
Replaces the earlier hand-tuned transitive override list (which forced
minimatch@5 globally and broke glob@7 with a "Cannot convert Symbol to
string" crash) with a coherent approach: bump mocha 10.2->10.8.2 +
eslint ->8.57.1 so the dev tree resolves patched transitive versions
without version-split conflicts, and keep only the runtime/transitive
overrides that don't fight the tree (basic-ftp, @75lb/deep-merge, ws,
ip-address, form-data).

Residual (deferred to 2.0.0, all require breaking changes):
- thrift, uuid (HIGH) -> SemVer-major runtime bumps
- serialize-javascript (dev-only) -> patched versions need Node >=20,
  which the non-breaking >=14 floor can't adopt; dev-only, not shipped.

OSV: 0 CRITICAL, 0 transitive-fixable findings; only the above residual.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala
style: prettier --write on CHANGELOG.md + package.json
320ed33 
The lint job runs `prettier . --check`; the hand-edited 1.17.0 CHANGELOG
entry and package.json overrides block needed reformatting to pass.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@vikrantpuppala vikrantpuppala enabled auto-merge July 10, 2026 10:35
@vikrantpuppala vikrantpuppala added this pull request to the merge queue Jul 10, 2026
Merged via the queue into main with commit d55b056 Jul 10, 2026
25 of 26 checks passed
pull Bot pushed a commit to rebeccalarner/databricks-sql-nodejs that referenced this pull request Jul 14, 2026
@vikrantpuppala
2.0.0: breaking security bump — Node >=20, thrift 0.23, uuid 11, lz4→…
c1a9d4b 
…lz4-napi (0 OSV findings) (databricks#390)

* chore(sec): 1.17.0 — clear CRITICAL + transitive findings via overrides (non-breaking)

Non-breaking security patch on the 1.x line so consumers can clear
critical/high scanner alerts without waiting for or adopting the breaking
2.0.0 bump (requested on databricks#386).

Adds a package.json `overrides` block pinning transitive/dev deps to
patched versions. Clears:
- 2 CRITICAL: basic-ftp, form-data
- HIGH/MED/LOW across the dev + transitive tree (ws, braces, micromatch,
  cross-spawn, serialize-javascript, @babel/*, js-yaml, ip-address,
  path-to-regexp, diff, ajv, nanoid, minimatch, follow-redirects, ...)

No engine, runtime-API, or declared-dependency changes -> drop-in patch.

The two remaining HIGH findings are on direct runtime deps whose fixes are
SemVer-major (thrift 0.16->0.23, uuid 9->11); those, plus the engines
>=18 bump, ship in 2.0.0 (databricks#390). Verified with osv-scanner v2.3.8: only
thrift + uuid remain (both "breaking change" per npm audit).

Closes databricks#386
Closes databricks#263

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

* fix: coherent dev-toolchain for 1.17.0 overrides (mocha/eslint bump)

Replaces the earlier hand-tuned transitive override list (which forced
minimatch@5 globally and broke glob@7 with a "Cannot convert Symbol to
string" crash) with a coherent approach: bump mocha 10.2->10.8.2 +
eslint ->8.57.1 so the dev tree resolves patched transitive versions
without version-split conflicts, and keep only the runtime/transitive
overrides that don't fight the tree (basic-ftp, @75lb/deep-merge, ws,
ip-address, form-data).

Residual (deferred to 2.0.0, all require breaking changes):
- thrift, uuid (HIGH) -> SemVer-major runtime bumps
- serialize-javascript (dev-only) -> patched versions need Node >=20,
  which the non-breaking >=14 floor can't adopt; dev-only, not shipped.

OSV: 0 CRITICAL, 0 transitive-fixable findings; only the above residual.

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

* style: prettier --write on CHANGELOG.md + package.json

The lint job runs `prettier . --check`; the hand-edited 1.17.0 CHANGELOG
entry and package.json overrides block needed reformatting to pass.

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

* 2.0.0: breaking security bump — Node >=20, thrift 0.23, uuid 11 (0 OSV findings)

Completes the security cleanup that 1.17.0 (databricks#446) could not do without
breaking changes. Stacks on the 1.17.0 overrides.

Breaking:
- engines.node >=14 -> >=20. Node 14/16/18 dropped (all past upstream EOL;
  18 EOL 2025-04). >=20 is also the floor for the patched serialize-javascript
  (dev) and clears the kernel backend's own Node 18 requirement.
- thrift ^0.16.0 -> ^0.23.0 (clears GHSA-r67j-r569-jrwp HIGH + GHSA-526f-jxpj-jmg2)
- uuid ^9.0.0 -> ^11.1.1 (clears GHSA-w5hq-g745-h8pq HIGH 7.5); overrides pin
  thrift's transitive uuid at 11 (uuid 13 is ESM-only, breaks require()).

Supporting:
- typescript pinned 5.5.4 (uuid@11's .d.ts uses `export type` -> needs TS 5+;
  <5.6 to avoid @types/node Buffer-generic emit changing published .d.ts).
- serialize-javascript override ^7.0.5 (needs Node >=20; now satisfied).
- @types/node-fetch ^2.6.13.
- OAuth.ts stub: cast Symbol.asyncDispose through any (ES2018 target predates
  the symbol; runtime Node 20+ provides it).
- CI matrix -> [20, 22, 24]; e2e expanded to matrix; setup-jfrog lockfile
  URL rewrite (carried from the prior PR).

Verified locally under Node 20: build clean, eslint clean, 1249 unit tests
passing, osv-scanner v2.3.8 = No issues found.

Closes databricks#370

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

* ci: trigger checks after clean force-update

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

* test(ci): fix ts-node on Node 24+ (strip-types) and add Node 26 to matrix

Two related changes for the widened 2.0.0 CI matrix [20, 22, 24, 26]:

1. Node >= 22.6 enables --experimental-strip-types, which on Node 24+ is the
   default loader for .ts and intercepts before ts-node. Its strip-only mode
   can't handle TypeScript features that emit code (constructor parameter
   properties), throwing "parameter property is not supported in strip-only
   mode" — even on files ts-node compiles fine. Both mocharc files now force
   `require: ts-node/register` and `node-option: [no-experimental-strip-types]`
   so ts-node owns .ts compilation on every supported Node. Verified locally:
   unit specs using parameter properties pass under Node 24.

2. Add Node 26 (current) to the unit + e2e matrices alongside the 20/22/24
   LTS+active lines, for forward-compat coverage.

Note: main previously only tested [14,16,18,20], so Node 22/24/26 were never
exercised — this surfaced the strip-types issue as a latent, not a regression.

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

* test(ci): drop invalid node-option flag; ts-node/register alone fixes 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>

* test(ci): add ts-node/esm loader so Node 24+ handles .ts on the import() 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>

* fix(deps): trim redundant overrides to minimal 3; fail-fast:false for 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>

* Revert override trim — restore 7-override set consistent with the lockfile

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>

* lz4: replace unmaintained `lz4` with maintained `lz4-napi` (frame-compatible)

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 databricks#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>

* ci: complete lz4-napi lockfile, restore npm ci, fix prettier

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 databricks#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>

* review: address code-review findings on the lz4-napi swap

From the Code Review Squad pass on databricks#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>

* review: gate real-codec lz4 tests on availability, not the environment

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>

---------

Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Vulnerability Issues Upgrade version of apache-arrow for security

2 participants

@vikrantpuppala @sreekanth-db