Clear OSV-Scanner HIGH/MED/LOW findings via dep bumps + overrides#390
Open
vikrantpuppala wants to merge 9 commits into
Open
Clear OSV-Scanner HIGH/MED/LOW findings via dep bumps + overrides#390vikrantpuppala wants to merge 9 commits into
vikrantpuppala wants to merge 9 commits into
Conversation
vikrantpuppala
had a problem deploying
to
azure-prod
GitHub Actions
Failure
vikrantpuppala
had a problem deploying
to
azure-prod
GitHub Actions
Failure
This was referenced May 27, 2026
vikrantpuppala
had a problem deploying
to
azure-prod
GitHub Actions
Failure
vikrantpuppala
had a problem deploying
to
azure-prod
GitHub Actions
Failure
|
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 ( |
vikrantpuppala
force-pushed
the
vp/security-bump-runtime-and-dev
branch
from
da4c50c to
439de47
Compare
vikrantpuppala
had a problem deploying
to
azure-prod
GitHub Actions
Failure
vikrantpuppala
had a problem deploying
to
azure-prod
GitHub Actions
Failure
Surfaced by OSV-Scanner against package-lock.json. The pre-change scan reported 22 HIGH / 15 MED / 5 LOW (42 total). After this PR a clean scan against the new lockfile reports 0 HIGH / 0 MED / 1 LOW (the single remaining LOW is GHSA-73rr-hh4g-fpgx on diff@7.0.0, pinned by sinon@19.0.5 — not overridable without breaking sinon's peer ranges, and is reachable only via assertion-error rendering in test code). Top-level bumps (runtime): thrift 0.16.0 -> 0.23.0 GHSA-r67j-r569-jrwp, GHSA-526f-jxpj-jmg2 (both HIGH) Top-level bumps (devDependencies): mocha 10.2.0 -> 10.8.2 eslint 8.22.0 -> 8.57.1 eslint-plugin-import 2.26.0 -> 2.32.0 sinon 17.0.1 -> 19.0.5 @types/node-fetch 2.6.4 -> 2.6.13 `overrides` block added for deep transitives that can't be reached by top-level bumps (basic-ftp via proxy-agent chain; @75lb/deep-merge via apache-arrow chain; ws pinned inside thrift; cross-spawn pinned inside eslint; etc.). Each override is set to the lowest version that clears its CVEs to minimize unintended behavior changes. Test-stub follow-ups (required by the dev-dep bumps' newer types): - OAuthCallbackServerStub: add Symbol.asyncDispose stub method (newer @types/node added it to http.Server). - Issuer stub in OAuthManager.test: add FAPI2Client property (openid-client >= 5.5 widened the interface). - AuthorizationCode.test: cast sinon.spy result to `as any` for the private-field assignment (the stub intentionally doesn't fully mirror http.Server; runtime is identical). Net OSV-Scanner result after this PR: HIGH: 22 -> 0 MED: 15 -> 0 LOW: 5 -> 1 (sinon-pinned, documented in PR description) Verified locally: npm run build -- clean npm run type-check -- clean (no errors in lib/ or tests/) npm run lint -- 3 pre-existing warnings, no errors Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
npm ci honors the lockfile's resolved: URL over .npmrc's registry setting. Protected runners cannot reach registry.npmjs.org directly, so any package not already in the ~/.npm cache hangs ~8 minutes before being killed with "Exit handler never called", which silently skips bin-linking and breaks downstream `nyc`/`prettier` calls. Rewrite the lockfile in-place after configuring the JFrog registry so npm fetches via the proxy. Committed lockfile stays in standard public-npm form so contributors can `npm ci` locally without JFrog auth. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
`npm ci` is hanging silently for ~8 minutes in protected-runner CI
and dying with "Exit handler never called", which skips bin-linking
and breaks downstream `prettier`/`nyc` calls. Previous fix attempts
have not surfaced the root cause; this commit adds enough signal
that the next run will reveal exactly what's going wrong.
Three diagnostic blocks added to lint / unit-test / e2e-test jobs:
1. pre-npm-ci: captures effective npm config, ~/.npmrc contents,
cache state, lockfile resolved-URL distribution, and reachability
probes to both registries plus a sample fetch of a known new
package (basic-ftp). Runs always; cheap.
2. npm ci flags: --loglevel=http --no-progress --foreground-scripts
so the CI log itself shows every HTTP request npm makes and any
lifecycle-script output that would otherwise be buffered.
3. post-npm-ci on failure: bundles ~/.npm/_logs (npm's own debug
log — definitive proof of what hung), cacache state, node_modules
bin state, process tree, dmesg tail, and the in-CI lockfile.
Uploaded as artifact npm-diag-{job}{-node<v>}.
To be reverted in one commit once the hang is fixed.
Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Diagnostic run revealed the package-lock.json contains 415 URLs pointing to `npm-proxy.dev.databricks.com` (in addition to 774 pointing to `registry.npmjs.org`). The internal dev-proxy URLs are written by `npm install` when the lockfile is regenerated on a dev workstation whose .npmrc points to that proxy. Protected runners cannot reach npm-proxy.dev.databricks.com — every fetch fails with ECONNRESET, npm retries (default 2 attempts), then gives up with "Exit handler never called", skipping bin-linking and breaking downstream `prettier`/`nyc` calls. Extend the sed rewrite to cover both registry hosts so all `resolved:` URLs route through JFrog regardless of where the lockfile was generated. Also print the post-rewrite URL distribution so future debugging is one log scroll away. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Mocha 10.5+ takes an ESM import() path for .ts spec files on some Node versions (observed failing on Node 16 with mocha 10.8.2, ERR_UNKNOWN_FILE_EXTENSION). The ESM path bypasses ts-node/register which was previously only loaded via nyc.config.js — that only patches require(), not import(). Adding `require: ['ts-node/register']` to both mocharcs forces mocha to require the ts-node hook before resolving any specs, which keeps the CJS loader path active for .ts files regardless of which Node version is in use. No impact on published package — tests/ is excluded by .npmignore. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
thrift@0.23.0 (the CVE-patched thrift required by GHSA-r67j-r569-jrwp and GHSA-526f-jxpj-jmg2) declared `uuid: ^13.0.0`. uuid@13 is ESM-only ("type": "module") with no engines.node restriction, but thrift's compact_protocol.js does require('uuid') — which fails on Node < 22.12 with ERR_REQUIRE_ESM. The failure cascades through the test harness: - mocha 10.x tries import() first, falls back to require() on ERR_UNKNOWN_FILE_EXTENSION - ts-node compiles the test, which transitively requires thrift - thrift requires uuid — fails with ERR_REQUIRE_ESM - mocha catches it as 'cannot use import statement outside a module', re-throws the original ERR_UNKNOWN_FILE_EXTENSION End-user impact would have been identical: anyone using the driver on Node 14/16/18/20 would crash with ERR_REQUIRE_ESM on first thrift use. Fix: pin transitive uuid to ^9.0.0 (CommonJS-compatible) via package.json overrides. uuid@9 is what the driver already uses directly. The public uuid API used by thrift (v4 generation) is identical across v9 and v13. Lockfile regenerated from scratch with npm 10 to ensure the override is applied to fresh resolution. The format also upgraded from lockfileVersion 2 to 3 — only the legacy `dependencies` mirror block was dropped, no semantic changes. package-lock.json is not in the published tarball (see .npmignore) so end users are unaffected. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
vikrantpuppala
force-pushed
the
vp/security-bump-runtime-and-dev
branch
from
b5b46b2 to
f4b11d3
Compare
Node 14 ships with npm 6.14.18, which cannot read lockfileVersion 3 and crashes with the cryptic error: npm ERR! Cannot read property 'apache-arrow' of undefined To preserve Node 14 support in CI, regenerate the lockfile with --lockfile-version=2. Same package set (585), same override behavior (transitive uuid pinned to ^9.0.0), but readable by all npm versions from 6 through 10. Contributors running `npm install` on modern npm (10+) will get v3 written back to the lockfile by default. To prevent silent drift, always regenerate with `npm install --lockfile-version=2` until the project drops Node 14. Verified locally: `npm ci` succeeds on Node 14.21.3 (npm 6.14.18) and Node 16.20.2 (npm 8.19.4), both with the uuid override applied (no nested node_modules/thrift/node_modules/uuid). Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Node 14 has been EOL upstream since April 2023. The npm ecosystem
now ships ES2021 syntax in widely-used transitive deps that Node 14
cannot parse — concretely, @dabh/diagnostics@2.0.7+ (a transitive
of winston) switched its colorspace dep to @so-ric/colorspace,
which uses the `||=` (logical-nullish-assignment) operator in its
distributed CJS bundle. Node 14's V8 can't parse it:
SyntaxError: Unexpected token '||='
at .../node_modules/@so-ric/colorspace/dist/index.cjs.js:1976
This is not fixable by overriding individual packages: every dep
bump or lockfile regeneration pulls in newer transitives that
target newer Node. Keeping Node 14 means perpetual whack-a-mole.
Drop Node 14 from the CI matrix and bump engines.node to >= 16.0.0
to align the published support claim with what we actually test.
Node 16, 18, 20 remain in the matrix.
Customer impact: nominal. Node 14 has had no upstream security
patches in 3+ years; anyone still on it is already unsupported by
their runtime. Users on Node 16/18/20 are unaffected.
Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Three coupled changes to clear the remaining OSV finding cleanly: 1. uuid: ^9.0.0 -> ^11.1.1 (both top-level dep and the override that constrains thrift's transitive uuid). Clears GHSA-w5hq-g745-h8pq (CVSS 7.5, buffer-bounds bug in v3/v5/v6). Driver uses only v4, stringify, NIL — not the vulnerable functions — but the CVE surfaces in consumers' own OSV/Snyk scans against our lockfile, so we need it actually patched, not just suppressed. 2. typescript: ^4.9.3 -> 5.5.4 (exact pin). uuid@11's d.ts uses `export type *` (TS 5.0+). TypeScript is pinned to <5.6 because TS 5.6+ tells @types/node to use its newer generic Buffer declaration (Buffer<TArrayBuffer extends ArrayBufferLike>), which would leak into our published d.ts as `Buffer<ArrayBufferLike>[]` and break consumers on stale @types/node. TS 5.5 uses the ts5.6/ fallback shim that keeps the emit as plain `Buffer[]` — byte-equivalent to today's d.ts. 3. sinon: ^19.0.5 -> ^17.0.2. The sinon@19 bump in this PR was redundant for security (all CVEs in sinon's transitive tree are covered by our existing overrides on flatted, serialize-javascript, etc.) and broke the FeatureFlagCache.test.ts upstream test on Node 16 due to sinon 19's broader fake-timer faking (@sinonjs/fake-timers 13 vs 11). Reverting to ^17.0.2 restores the test pass with no CVE re-introduction. Source change in FederationProvider.ts: TS 5 caught a real but benign type incompatibility between node-fetch's AbortSignal shim and the native AbortSignal that TS 4 missed. Cast the controller signal through `any` — the two are runtime-compatible; this is a typings-only mismatch. Verified locally: - npm ci + npm test pass on Node 16.20.2 and 18.20.8 (904 passing) - OSV-Scanner reports zero findings against the regenerated lockfile - Emitted dist/*.d.ts and dist/*.js diff against current main only in cosmetic ways (removed obsolete /// <reference> directives, preserved source comments, TS 5's improved enum/export emit) — no behavior changes for consumers Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bumps deps and adds
package.jsonoverridesto clear all HIGH and MED OSV-Scanner findings against the currentpackage-lock.json. Companion to #388 (the security workflow PR).Top-level bumps
thriftmochaeslinteslint-plugin-importsinon@types/node-fetchoverridesfor deep transitivesPackages reachable only via deep dependency chains where no top-level bump applies. Each is pinned to the lowest version that clears its CVEs:
Net OSV-Scanner result
The single remaining LOW is
GHSA-73rr-hh4g-fpgxondiff@7.0.0, pinned bysinon@19.0.5. Can't be overridden without breaking sinon's peer ranges (sinon requiresdiff@^7and the fix is indiff@8.0.3+which sinon hasn't accepted yet). Reachable only via sinon's assertion-error rendering in test code — never runtime. Recommended to add a documented[[IgnoredVulns]]entry for it inosv-scanner.toml(this happens in #388's scope, not here).Test-stub follow-ups required by the bumps
A few test stubs needed updates because
@types/nodeandopenid-clientinterfaces grew in the newer versions transitively pulled in by the dev-dep bumps:tests/unit/.stubs/OAuth.ts: added[Symbol.asyncDispose]()stub onOAuthCallbackServerStub—@types/node ≥ 18.19added it tohttp.Server.OAuthManager.test.ts: addedFAPI2Clientto the issuer stub — openid-client ≥ 5.5 widened theIssuerinterface.AuthorizationCode.test.ts: cast thesinon.spyassignment toas anyfor the private-field write. The stub intentionally doesn't fully mirrorhttp.Server; runtime is identical.No Node.js engine bump
Despite eslint/typescript-eslint declaring
>=16ranges, all of them ship versions that accept Node 14.17+. The repo's CI matrix[14, 16, 18, 20]continues to work without changes.package.jsonengines.node: ">=14.0.0"is also unchanged.Test plan
npm run buildcleannpm run type-checkclean (no errors inlib/ortests/)npm run lint— 3 pre-existing warnings, no errorsThis pull request was AI-assisted by Isaac.