follow-up: review-squad LOW/MED follow-ups (F4, F8, F11, F14)

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:
 

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 <package-name>
+
+# 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.

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',

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": {

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.

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 {

tests/unit/connection/auth/DatabricksOAuth/AuthorizationCode.test.ts

@@ -95,7 +95,11 @@ function prepareTestInstances(options: Partial<AuthorizationCodeOptions>) {
 
   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<AuthorizationCode['createHttpServer']>;
   });
 
   authCode['createHttpServer'] = createHttpServer;

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<Parameters<typeof nodeFetch>, ReturnType<typeof nodeFetch>>().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<Parameters<typeof nodeFetch>, ReturnType<typeof nodeFetch>>()
+        .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<Parameters<typeof nodeFetch>, ReturnType<typeof nodeFetch>>()
+        .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');