From d112b5e71609a1b59325b16bf6884d7473d13018 Mon Sep 17 00:00:00 2001 From: Vikrant Puppala Date: Tue, 14 Jul 2026 07:03:20 +0000 Subject: [PATCH] follow-up: review-squad LOW/MED follow-ups (F4, F8, F11, F14) Rebuilt cleanly on top of merged main (post-#390). The original #401 branch had diverged badly (branched off 1.15.0, carried a stale duplicate of the entire #390 security-bump, missing ~20k lines of since-merged kernel/telemetry work). This is only the genuinely-new follow-up content. F2 (lz4 test ESM-safety) is intentionally DROPPED: #390 already made the lz4 tests ESM-safe a different way (golden-frame real-codec suite + an availability-gated before()), so #401's setLz4LoaderForTest seam would revert merged work. - F4: SECURITY-OVERRIDES.md documents each of the 7 package.json `overrides` entries (class, dependent chain, CVE ids, exit condition), matching the actual merged override set (not the stale 18-entry set the original branch documented). Referenced from CONTRIBUTING.md. - F8: FederationProvider gains a setFederationFetchForTest() seam so tests can stub node-fetch without patching the import system; adds 3 unit tests exercising the previously-uncovered HTTP exchange branch (exchange success + endpoint/method/AbortSignal assertions, signal contract, non-retryable-failure fallback to the original token). - F11: tests/e2e/README.md documents the parallelism contract (per-job E2E_TABLE_SUFFIX, uuid.v4() staging file names) and warehouse-capacity considerations for the Node 20/22/24/26 matrix. Drive-by: fix the lint script glob (`tests/e2e/**` -> `tests/e2e/**/*.{js,ts}`) so eslint no longer tries to parse the new README.md. - F14: OAuthCallbackServerStub drops the pile of no-op http.Server shim methods (setTimeout/closeAllConnections/ref/unref/Symbol.asyncDispose/ the maxHeadersCount... property pile). Production OAuth code only calls listen/close/address; the AuthorizationCode.test.ts call site now carries the structural-type assertion via an `as unknown as ...` cast, so @types/node widening no longer forces stub churn. Verified on Node 20: tsc --noEmit clean, eslint clean, prettier clean, 1253 unit tests passing. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala --- CONTRIBUTING.md | 2 +- SECURITY-OVERRIDES.md | 80 ++++++++++++ .../auth/tokenProvider/FederationProvider.ts | 13 +- package.json | 2 +- tests/e2e/README.md | 34 +++++ tests/unit/.stubs/OAuth.ts | 52 ++------ .../DatabricksOAuth/AuthorizationCode.test.ts | 6 +- .../tokenProvider/FederationProvider.test.ts | 116 +++++++++++++++++- 8 files changed, 257 insertions(+), 48 deletions(-) create mode 100644 SECURITY-OVERRIDES.md create mode 100644 tests/e2e/README.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 01968699..19429136 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -109,7 +109,7 @@ npm run type-check ## Dependency Pins -A few entries in `package.json` are pinned more tightly than usual. Don't relax these without understanding why. +A few entries in `package.json` are pinned more tightly than usual. Don't relax these without understanding why. For the full list of CVE-driven `overrides` entries, see [`SECURITY-OVERRIDES.md`](./SECURITY-OVERRIDES.md). - **`typescript: "5.5.4"`** (exact, no caret). This pin has both a floor and a ceiling: diff --git a/SECURITY-OVERRIDES.md b/SECURITY-OVERRIDES.md new file mode 100644 index 00000000..15870cd1 --- /dev/null +++ b/SECURITY-OVERRIDES.md @@ -0,0 +1,80 @@ +# Security Overrides + +The `overrides` block in `package.json` pins transitive (and one direct) dependencies to versions that clear known CVEs. Each entry is debt — when the underlying ecosystem moves on, the corresponding entry should be removed. + +This file documents the provenance and exit condition for each override. **When adding or removing an override, update this file in the same commit.** + +## Conventions + +- **Class**: `runtime` if the package ends up in the published `dist/` runtime path; `dev` if it's used only by tooling (eslint, mocha, nyc, prettier, etc.). The published tarball excludes everything except `dist/`, `thrift/`, `native/`, `LICENSE`, `NOTICE`, `package.json`, `README.md` — so dev-tooling overrides do not ship to consumers but DO surface in customer-side scanners (Dependabot, Snyk, OSV) that read our lockfile. +- **Exit condition**: the smallest change that would let us drop the override entry. Usually "upstream bump", sometimes "upstream widens the patched version into its dep range". + +--- + +## Entries + +### `basic-ftp: ^5.3.1` + +- **Class**: runtime +- **Path**: `proxy-agent → pac-proxy-agent → get-uri → basic-ftp` +- **CVEs cleared**: GHSA-5rq4-664w-9x2c, GHSA-6v7q-wjvx-w8wg, GHSA-rp42-5vxx-qpwr, GHSA-rpmf-866q-6p89 +- **Exit**: `get-uri` bumps its `basic-ftp` dep range to include `^5.3.1`. + +### `@75lb/deep-merge: ^1.1.2` + +- **Class**: dev (apache-arrow's CLI tooling — not in runtime path) +- **Path**: `apache-arrow → command-line-usage → table-layout → @75lb/deep-merge` +- **CVEs cleared**: GHSA-28mc-g557-92m7 +- **Exit**: `table-layout` bumps its dep. Note `apache-arrow@13` ships unused CLI tooling — bumping arrow to `15.x+` drops this dep entirely. + +### `ws: ^8.18.0` + +- **Class**: runtime (thrift's WebSocket transport) +- **Path**: `thrift → ws` AND `thrift → isomorphic-ws → ws` +- **CVEs cleared**: GHSA-3h5v-q93c-6h6q (ws@5.x DoS) +- **Exit**: `thrift` bumps its declared `ws` range to `^8.x`. Without the override, `thrift` would pull the vulnerable `ws@5.x`. + +### `ip-address: ^10.1.1` + +- **Class**: runtime +- **Path**: `proxy-agent → socks-proxy-agent → socks → ip-address` +- **CVEs cleared**: GHSA-v2v4-37r5-5v8g (IPv6 parsing DoS) +- **Why an override is needed**: `socks` caps its `ip-address` dependency below the patched `^10.1.1`, so a plain bump of the parent can't reach the fix — the override is required to force the patched version. +- **Exit**: `socks` widens its `ip-address` range to include `^10.x`. Note: `ip-address@10` is CommonJS with conditional exports — verify any future bump retains CJS compat for our `dist/`. + +### `form-data: ^4.0.4` + +- **Class**: runtime +- **Path**: `node-fetch → form-data` (multipart bodies) +- **CVEs cleared**: GHSA-fjxv-7rqg-78g4 (unsafe random boundary generation) +- **Exit**: `node-fetch` bumps its `form-data` dep range to include the patched line. + +### `serialize-javascript: ^7.0.5` + +- **Class**: dev (mocha) +- **Path**: `mocha → serialize-javascript` +- **CVEs cleared**: GHSA-5c6j-r48x-rmvq (XSS via prototype pollution) +- **Note**: the patched line requires Node ≥ 20, which is satisfied by `engines.node >= 20`. +- **Exit**: mocha bumps its declared range to the patched line. + +### `uuid: ^11.1.1` + +- **Class**: **runtime** — this one matters most +- **Path**: declared as a top-level runtime dep AND `thrift → uuid` +- **CVEs cleared**: GHSA-w5hq-g745-h8pq (buffer-bounds in v3/v5/v6; the driver only uses v4, but consumer scanners flag against our lockfile) +- **Why an override is needed**: `thrift` declares `uuid: ^13.0.0`, but `uuid@13` is **ESM-only**. The driver compiles to CJS (`dist/*.js`), so a top-level `uuid: ^11.1.1` plus this matching override forces `thrift`'s transitive uuid down to v11 (which dual-publishes ESM + CJS via conditional exports). +- **Exit**: any of (a) we migrate `dist/` to ESM, (b) `thrift` drops the uuid dep, or (c) `thrift` widens its range to `^11 || ^13` in a CJS-compatible export shape. Today, removing this override would cause `require('uuid')` from `dist/` to crash on Node runtimes that don't support `require(esm)`. + +--- + +## How to audit + +```bash +# Show what depends on a specific override target: +npm ls + +# Re-run the lockfile against OSV-Scanner to verify findings are still cleared: +osv-scanner scan source --lockfile=package-lock.json +``` + +When all entries' exit conditions are met, this file should be deleted along with the corresponding `overrides` block. diff --git a/lib/connection/auth/tokenProvider/FederationProvider.ts b/lib/connection/auth/tokenProvider/FederationProvider.ts index 417851ff..c680c35c 100644 --- a/lib/connection/auth/tokenProvider/FederationProvider.ts +++ b/lib/connection/auth/tokenProvider/FederationProvider.ts @@ -1,8 +1,17 @@ -import fetch from 'node-fetch'; +import nodeFetch from 'node-fetch'; import ITokenProvider from './ITokenProvider'; import Token from './Token'; import { getJWTIssuer, isSameHost } from './utils'; +// Indirection so tests can swap the fetch implementation without +// patching the import system. Default is node-fetch. +let fetchImpl: typeof nodeFetch = nodeFetch; + +/** Test-only: replace the fetch implementation. Called with no arg, restores node-fetch. */ +export function setFederationFetchForTest(fn?: typeof nodeFetch): void { + fetchImpl = fn ?? nodeFetch; +} + /** * Token exchange endpoint path for Databricks OIDC. */ @@ -157,7 +166,7 @@ export default class FederationProvider implements ITokenProvider { const timeoutId = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS); try { - const response = await fetch(url, { + const response = await fetchImpl(url, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', diff --git a/package.json b/package.json index e963ac74..2ec01635 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,7 @@ "type-check": "tsc --noEmit", "prettier": "prettier . --check", "prettier:fix": "prettier . --write", - "lint": "eslint lib/** tests/e2e/** --ext .js,.ts", + "lint": "eslint 'lib/**/*.{js,ts}' 'tests/e2e/**/*.{js,ts}' --ext .js,.ts", "lint:fix": "eslint lib/** --ext .js,.ts --fix" }, "repository": { diff --git a/tests/e2e/README.md b/tests/e2e/README.md new file mode 100644 index 00000000..2e759d59 --- /dev/null +++ b/tests/e2e/README.md @@ -0,0 +1,34 @@ +# End-to-End Tests + +These tests run against a real Databricks SQL warehouse. They're invoked by `npm run e2e` and exercise the driver's HTTP/Thrift/Arrow path against live infrastructure. + +## Environment + +| Variable | Used for | +| ------------------ | ------------------------------------------------------------------------ | +| `E2E_HOST` | Workspace hostname | +| `E2E_PATH` | Warehouse HTTP path | +| `E2E_ACCESS_TOKEN` | PAT for auth | +| `E2E_TABLE_SUFFIX` | Suffix appended to per-test table names so concurrent runs don't collide | +| `E2E_CATALOG` | Catalog (default: `peco`) | +| `E2E_SCHEMA` | Schema (default: `default`) | +| `E2E_VOLUME` | Volume name (default: `e2etests`) | + +## CI parallelism + +The `e2e-test` job in `.github/workflows/main.yml` runs as a matrix across Node 20/22/24/26. All entries point at the same workspace, catalog, schema, and volume. + +Per-test isolation is achieved by: + +- **Tables**: all DDL in tests is templated against `${E2E_TABLE_SUFFIX}`, which in CI is `${{ github.sha }}_node${{ matrix.node-version }}`. Underscores not hyphens — SQL unquoted identifiers don't allow `-`. +- **Volume files**: `tests/e2e/staging_ingestion.test.ts` generates per-file `uuid.v4()` names. Multiple matrix entries can read/write the volume concurrently without collisions. + +No test creates or drops the shared catalog/schema/volume. If you add a test that does, you'll need to suffix-unique the resource name too — verify before merging. + +## Local invocation + +`npm run e2e` must be run from the repo root. Some specs resolve fixture paths relative to `process.cwd()`. + +## Warehouse capacity + +The parallel CI matrix entries against one warehouse plus any concurrent PR runs can saturate the warehouse's session limit. If you see queue-related flakes (`session start` timeouts, request queueing delays), check the warehouse's `max_num_concurrent_runs` setting. diff --git a/tests/unit/.stubs/OAuth.ts b/tests/unit/.stubs/OAuth.ts index 1f892d46..caeecd4a 100644 --- a/tests/unit/.stubs/OAuth.ts +++ b/tests/unit/.stubs/OAuth.ts @@ -100,51 +100,19 @@ export class OAuthCallbackServerStub< return this; } - // Dummy methods and properties for compatibility with `http.Server` - - public maxHeadersCount: number | null = null; - - public maxRequestsPerSocket: number | null = null; - - public timeout: number = -1; - - public headersTimeout: number = -1; - - public keepAliveTimeout: number = -1; - - public requestTimeout: number = -1; - - public maxConnections: number = -1; - - public connections: number = 0; - - public setTimeout() { - return this; - } - - public closeAllConnections() {} - - public closeIdleConnections() {} - + // No-op shim for the subset of `http.Server` members production code + // touches. We intentionally do NOT mirror the full http.Server surface + // (setTimeout, closeAllConnections, ref/unref, Symbol.asyncDispose, the + // maxHeadersCount/timeout/... property pile) -- those existed only to + // satisfy http.Server's structural type and had to grow every time + // @types/node widened the interface. The call site casts the stub via + // `as unknown as ...` (see AuthorizationCode.test.ts), and that cast is + // what carries the "trust me, this is Server-shaped" assertion. When the + // OAuth code starts calling a new Server member, add a shim here and the + // runtime test exercises it; @types/node additions no longer touch this. public address() { return null; } - - public getConnections() {} - - public ref() { - return this; - } - - public unref() { - return this; - } - - // Required by @types/node >= 18.19.x (Node 20+ added Symbol.asyncDispose to Server). - // Cast through `any`: the project targets ES2018, whose lib predates - // Symbol.asyncDispose, so referencing it directly is a compile error even - // though the runtime (Node 20+) provides it. - public async [(Symbol as any).asyncDispose]() {} } export class AuthorizationCodeStub { diff --git a/tests/unit/connection/auth/DatabricksOAuth/AuthorizationCode.test.ts b/tests/unit/connection/auth/DatabricksOAuth/AuthorizationCode.test.ts index 9b109f1c..50f01395 100644 --- a/tests/unit/connection/auth/DatabricksOAuth/AuthorizationCode.test.ts +++ b/tests/unit/connection/auth/DatabricksOAuth/AuthorizationCode.test.ts @@ -95,7 +95,11 @@ function prepareTestInstances(options: Partial) { const createHttpServer = sinon.spy((requestHandler: (req: IncomingMessage, res: ServerResponse) => void) => { httpServer.requestHandler = requestHandler; - return httpServer; + // OAuthCallbackServerStub only implements the http.Server members the + // OAuth code actually calls (listen/close/address). This cast carries + // the "trust me, this is Server-shaped" assertion so the stub doesn't + // have to mirror the full (and @types/node-drifting) http.Server surface. + return httpServer as unknown as ReturnType; }); authCode['createHttpServer'] = createHttpServer; diff --git a/tests/unit/connection/auth/tokenProvider/FederationProvider.test.ts b/tests/unit/connection/auth/tokenProvider/FederationProvider.test.ts index 4a7c5465..fbf6a817 100644 --- a/tests/unit/connection/auth/tokenProvider/FederationProvider.test.ts +++ b/tests/unit/connection/auth/tokenProvider/FederationProvider.test.ts @@ -1,6 +1,9 @@ import { expect } from 'chai'; import sinon from 'sinon'; -import FederationProvider from '../../../../../lib/connection/auth/tokenProvider/FederationProvider'; +import type nodeFetch from 'node-fetch'; +import FederationProvider, { + setFederationFetchForTest, +} from '../../../../../lib/connection/auth/tokenProvider/FederationProvider'; import ITokenProvider from '../../../../../lib/connection/auth/tokenProvider/ITokenProvider'; import Token from '../../../../../lib/connection/auth/tokenProvider/Token'; @@ -68,6 +71,117 @@ describe('FederationProvider', () => { }); }); + describe('exchange path', () => { + // These tests exercise the federation HTTP exchange — the branch + // taken when the source JWT's issuer doesn't match the Databricks + // host. The branch contains the AbortController + node-fetch shim + // typing fix; without coverage here a regression in those mechanics + // would only surface in production. + + afterEach(() => { + setFederationFetchForTest(); // restore real node-fetch + }); + + // Helper: build a fake node-fetch Response. + function buildFakeResponse(opts: { + ok: boolean; + status?: number; + statusText?: string; + body?: unknown; + text?: string; + }): nodeFetch.Response { + return { + ok: opts.ok, + status: opts.status ?? (opts.ok ? 200 : 500), + statusText: opts.statusText ?? '', + json: async () => opts.body, + text: async () => opts.text ?? '', + } as unknown as nodeFetch.Response; + } + + it('should exchange foreign-issued JWT for a Databricks token', async () => { + const foreignJwt = createJWT({ iss: 'https://idp.example.com' }); + const baseProvider = new MockTokenProvider(foreignJwt); + const federationProvider = new FederationProvider(baseProvider, 'my-workspace.cloud.databricks.com'); + + const fetchStub = sinon.stub, ReturnType>().resolves( + buildFakeResponse({ + ok: true, + body: { access_token: 'exchanged-databricks-token', token_type: 'Bearer', expires_in: 3600 }, + }), + ); + setFederationFetchForTest(fetchStub as unknown as typeof nodeFetch); + + const token = await federationProvider.getToken(); + + expect(token.accessToken).to.equal('exchanged-databricks-token'); + expect(fetchStub.calledOnce).to.be.true; + + // The exchange must POST to the Databricks /oidc/v1/token endpoint. + const [url, init] = fetchStub.firstCall.args; + expect(String(url)).to.include('my-workspace.cloud.databricks.com'); + expect(String(url)).to.include('/oidc/v1/token'); + expect(init!.method).to.equal('POST'); + + // Verify the signal propagates an AbortSignal — this is the cast + // site that TS 5 type-strictness caught. Runtime-wise it must + // still be a real AbortSignal-shaped object. + const passedSignal = init!.signal as unknown as AbortSignal; + expect(passedSignal, 'fetch init.signal must be set').to.exist; + expect(typeof passedSignal.aborted, 'signal.aborted must be a boolean').to.equal('boolean'); + expect(passedSignal.aborted).to.be.false; + }); + + it('should propagate abort from the controller to the signal observed by fetch', async () => { + const foreignJwt = createJWT({ iss: 'https://idp.example.com' }); + const baseProvider = new MockTokenProvider(foreignJwt); + const federationProvider = new FederationProvider(baseProvider, 'my-workspace.cloud.databricks.com', { + returnOriginalTokenOnFailure: false, + }); + + // Capture the signal so we can assert it implements the standard + // AbortSignal contract. Resolve immediately with success to avoid + // the 30s real-timeout path; the point is that the signal is wired + // up, not to exercise the abort end-to-end. + let capturedSignal: AbortSignal | undefined; + const fetchStub = sinon + .stub, ReturnType>() + .callsFake(async (_url, init) => { + capturedSignal = init!.signal as unknown as AbortSignal; + return buildFakeResponse({ + ok: true, + body: { access_token: 'tok', token_type: 'Bearer', expires_in: 3600 }, + }); + }); + setFederationFetchForTest(fetchStub as unknown as typeof nodeFetch); + + await federationProvider.getToken(); + + expect(capturedSignal, 'signal must reach fetch').to.exist; + // The signal must implement the standard AbortSignal contract. + expect(typeof capturedSignal!.aborted).to.equal('boolean'); + expect(typeof capturedSignal!.addEventListener).to.equal('function'); + }); + + it('should fall back to original token when exchange fails (returnOriginalTokenOnFailure default)', async () => { + const foreignJwt = createJWT({ iss: 'https://idp.example.com' }); + const baseProvider = new MockTokenProvider(foreignJwt); + const federationProvider = new FederationProvider(baseProvider, 'my-workspace.cloud.databricks.com'); + + const fetchStub = sinon + .stub, ReturnType>() + .resolves(buildFakeResponse({ ok: false, status: 400, statusText: 'Bad Request', text: 'invalid_grant' })); + setFederationFetchForTest(fetchStub as unknown as typeof nodeFetch); + + const token = await federationProvider.getToken(); + + // Default behavior is to fall back to the original token on failure. + // Retries kick in for 5xx; 400 is non-retryable so this should fail + // fast on the first attempt. + expect(token.accessToken).to.equal(foreignJwt); + }); + }); + describe('getName', () => { it('should return wrapped name', () => { const baseProvider = new MockTokenProvider('token');