Skip to content

Commit 7bc63c9

Browse files
follow-up: review-squad LOW/MED follow-ups (5 items)
Stacked on top of PR #390 (security-bump-runtime-and-dev). Addresses the LOW/MED findings from the self-review pass that were intentionally deferred from the main PR to keep its scope tight. - F4: Add SECURITY-OVERRIDES.md — document each of the 18 entries in package.json's overrides with provenance (CVE id, dependent chain, exit condition). Reference it from CONTRIBUTING.md. Pure docs. - F2: lz4 test ESM-safe — refactor lib/utils/lz4.ts to expose setLz4LoaderForTest()/resetLz4CacheForTest() seams so the suite can inject stubs without patching Module._load. Drops the Node-22+ conditional skip; suite now runs on every Node version (16/18/20/22/24). 904 → 904 passing on Node 16/18/20 (unchanged), 898→904 on Node 22/24 (lz4 suite re-enabled). - F8: FederationProvider HTTP exchange tests — refactor to expose a setFederationFetchForTest() seam, then add 3 unit tests that exercise the exchange branch where the AbortController + node-fetch shim typing fix lives. Verifies (a) signal reaches fetch as an AbortSignal, (b) signal implements the standard contract, (c) failed exchanges fall back to original token. 904 → 907 passing. - F11: e2e parallel-safety audit — verified all DDL in tests/e2e/ is templated against E2E_TABLE_SUFFIX and staging files use uuid.v4(). Added tests/e2e/README.md documenting the parallelism contract and warehouse capacity considerations. - F14: OAuthCallbackServerStub interface-drift refactor — dropped the pile of no-op shim methods (setTimeout, closeAllConnections, address, ref, unref, Symbol.asyncDispose, etc.) that existed only to satisfy http.Server's structural type. Production code calls only listen() and close() on the stub, which it does implement. The test-site cast in AuthorizationCode.test.ts already carries the "trust me, this is Server-shaped" assertion via `as unknown as ...`. Drive-by lint-script fix: switched the lint script's glob from `tests/e2e/**` (which expanded to .md files and made eslint complain) to `tests/e2e/**/*.{js,ts}`. Same scope, no false positives. Verified locally: prettier, lint, tsc --noEmit, and npm test (907 passing) all clean on Node 16/18/20/22. OSV-Scanner still reports zero findings on the lockfile. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
1 parent bff8d0d commit 7bc63c9

9 files changed

Lines changed: 379 additions & 152 deletions

File tree

CONTRIBUTING.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ npm run type-check
109109

110110
## Dependency Pins
111111

112-
A few entries in `package.json` are pinned more tightly than usual. Don't relax these without understanding why.
112+
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).
113113

114114
- **`typescript: "5.5.4"`** (exact, no caret). This pin has both a floor and a ceiling:
115115

SECURITY-OVERRIDES.md

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
# Security Overrides
2+
3+
The `overrides` block in `package.json` pins 18 transitive dependencies to versions that clear known CVEs. Each entry is debt — when the underlying ecosystem moves on, the corresponding entry should be removed.
4+
5+
This file documents the provenance and exit condition for each override. **When adding or removing an override, update this file in the same commit.**
6+
7+
## Conventions
8+
9+
- **Class**: `runtime` if the package ends up in the published `dist/` runtime path; `dev` if it's used only by tooling (eslint, mocha, nyc, babel, prettier, etc.). The `.npmignore` excludes everything except `dist/`, `thrift/`, `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 scan our lockfile.
10+
- **Exit condition**: the smallest change that would let us drop the override entry. Usually "upstream bump", sometimes "upstream takes the patched version as a dep range".
11+
12+
---
13+
14+
## Entries
15+
16+
### `basic-ftp: ^5.3.1`
17+
18+
- **Class**: runtime
19+
- **Path**: `proxy-agent → pac-proxy-agent → get-uri → basic-ftp`
20+
- **CVEs cleared**: GHSA-5rq4-664w-9x2c (HIGH 9.1), GHSA-6v7q-wjvx-w8wg (HIGH 8.2), GHSA-rp42-5vxx-qpwr (HIGH 7.5), GHSA-rpmf-866q-6p89 (HIGH 7.5)
21+
- **Exit**: `get-uri` bumps its `basic-ftp` dep range to include `^5.3.1`. Currently declares `^5.0.2`.
22+
23+
### `@75lb/deep-merge: ^1.1.2`
24+
25+
- **Class**: dev (transitive of apache-arrow's CLI tooling — not in runtime path)
26+
- **Path**: `apache-arrow → command-line-usage → table-layout → @75lb/deep-merge`
27+
- **CVEs cleared**: GHSA-28mc-g557-92m7 (HIGH 8.7)
28+
- **Exit**: `table-layout` bumps its dep. Note `apache-arrow@13` itself ships unused CLI tooling — bumping arrow to `15.x+` drops this dep entirely.
29+
30+
### `braces: ^3.0.3`
31+
32+
- **Class**: dev (via mocha's chokidar + eslint's micromatch)
33+
- **Path**: `mocha → chokidar → braces` AND `@typescript-eslint/parser → ... → micromatch → braces`
34+
- **CVEs cleared**: GHSA-grv7-fg5c-xmjg (HIGH 7.5)
35+
- **Exit**: `chokidar` bumps its `braces` dep range. Currently declares `~3.0.2`.
36+
37+
### `picomatch: ^2.3.2`
38+
39+
- **Class**: dev (chokidar + micromatch transitive)
40+
- **Path**: `mocha → chokidar → readdirp → picomatch` AND `... → micromatch → picomatch`
41+
- **CVEs cleared**: GHSA-c2c7-rcm5-vvqj (HIGH 7.5)
42+
- **Exit**: `chokidar` and `micromatch` bump their `picomatch` dep ranges.
43+
44+
### `flatted: ^3.4.2`
45+
46+
- **Class**: dev (eslint's file-entry-cache)
47+
- **Path**: `eslint → file-entry-cache → flat-cache → flatted`
48+
- **CVEs cleared**: GHSA-rf6f-7fwh-wjgh (HIGH 8.9), GHSA-25h7-pfq9-p65f (HIGH 7.5)
49+
- **Exit**: `flat-cache` bumps. Or move to eslint 9 (drops file-entry-cache dep tree shape).
50+
51+
### `minimatch: ^3.1.3`
52+
53+
- **Class**: dev (eslint plugins)
54+
- **Path**: `eslint-plugin-import / -jsx-a11y / -react → minimatch`
55+
- **CVEs cleared**: GHSA-3ppc-4f35-3m26 (HIGH 8.7), GHSA-23c5-xmqv-rm74 (HIGH 7.5), GHSA-7r86-cg39-jmmj (HIGH 7.5)
56+
- **Exit**: eslint plugins bump to use minimatch 9.x+ (most have done so on newer majors).
57+
58+
### `ws: ^8.18.0`
59+
60+
- **Class**: runtime (thrift's WebSocket transport)
61+
- **Path**: `thrift → ws` AND `thrift → isomorphic-ws → ws`
62+
- **CVEs cleared**: GHSA-3h5v-q93c-6h6q (HIGH 8.7 — ws@5.x DoS)
63+
- **Exit**: `thrift` bumps its declared `ws: ^5.2.3` to `^8.x`. Without the override, `thrift@0.23.0` would pull the vulnerable `ws@5.x`.
64+
65+
### `cross-spawn: ^7.0.6`
66+
67+
- **Class**: dev (eslint + nyc)
68+
- **Path**: `eslint → cross-spawn`, `nyc → foreground-child → cross-spawn`, `nyc → istanbul-lib-processinfo → cross-spawn`
69+
- **CVEs cleared**: GHSA-3xgq-45jj-v275 (HIGH 7.7 — ReDoS)
70+
- **Exit**: eslint and nyc bump. Currently declare `^7.0.2`.
71+
72+
### `serialize-javascript: ^7.0.5`
73+
74+
- **Class**: dev (mocha)
75+
- **Path**: `mocha → serialize-javascript`
76+
- **CVEs cleared**: GHSA-5c6j-r48x-rmvq (HIGH 8.1 — XSS via prototype pollution)
77+
- **Exit**: mocha bumps. Currently declares `^6.0.2`.
78+
79+
### `follow-redirects: ^1.16.0`
80+
81+
- **Class**: dev (http-proxy testing util)
82+
- **Path**: `http-proxy → follow-redirects`
83+
- **CVEs cleared**: GHSA-r4q5-vmmm-2653 (MED 6.9)
84+
- **Exit**: `http-proxy` bumps. Currently declares `^1.15.0`.
85+
86+
### `brace-expansion: ^1.1.13`
87+
88+
- **Class**: dev (transitive of overridden minimatch)
89+
- **Path**: `eslint-plugin-import → minimatch → brace-expansion`
90+
- **CVEs cleared**: GHSA-v6h2-p8h4-qcjw (LOW)
91+
- **Exit**: same as `minimatch` — when eslint plugins bump to minimatch 9+, this resolves transitively too.
92+
93+
### `@babel/helpers: ^7.26.10`
94+
95+
- **Class**: dev (nyc instrumentation)
96+
- **Path**: `nyc → istanbul-lib-instrument → @babel/core → @babel/helpers`
97+
- **CVEs cleared**: GHSA-968p-4wvh-cqc8 (MED 6.2 — ReDoS)
98+
- **Exit**: `@babel/core` bumps the `@babel/helpers` range. Currently the bundled version was below the patched.
99+
100+
### `@babel/runtime: ^7.26.10`
101+
102+
- **Class**: dev (eslint-config-airbnb-typescript transitive)
103+
- **Path**: eslint plugins (via core-js-pure → @babel/runtime)
104+
- **CVEs cleared**: GHSA-968p-4wvh-cqc8 (MED 6.2 — same as @babel/helpers)
105+
- **Exit**: same as `@babel/helpers`.
106+
107+
### `@babel/runtime-corejs3: ^7.26.10`
108+
109+
- **Class**: dev (eslint-config-airbnb)
110+
- **Path**: same as `@babel/runtime`
111+
- **CVEs cleared**: GHSA-968p-4wvh-cqc8 (MED 6.2)
112+
- **Exit**: same as `@babel/runtime`.
113+
114+
### `ip-address: ^10.1.1`
115+
116+
- **Class**: runtime (proxy-agent → socks → ip-address)
117+
- **Path**: `proxy-agent → socks-proxy-agent → socks → ip-address`
118+
- **CVEs cleared**: GHSA-v2v4-37r5-5v8g (MED 5.3 — IPv6 parsing DoS)
119+
- **Exit**: `socks` bumps to `ip-address@^10.x`. Note: `ip-address@10` is published as CommonJS with conditional exports — verify any future bump retains CJS compat for our `dist/`.
120+
121+
### `js-yaml: ^4.1.1`
122+
123+
- **Class**: dev (eslint / mocha / nyc config loaders)
124+
- **Path**: `eslint → @eslint/eslintrc → js-yaml`, `mocha → js-yaml`, `nyc → @istanbuljs/load-nyc-config → js-yaml`
125+
- **CVEs cleared**: GHSA-mh29-5h37-fv8m (MED 5.3 — DoS on malformed YAML)
126+
- **Exit**: each consumer bumps. All three are already on the 4.x line; the override forces a patch within 4.x.
127+
128+
### `micromatch: ^4.0.8`
129+
130+
- **Class**: dev (typescript-eslint glob)
131+
- **Path**: `@typescript-eslint/parser → @typescript-eslint/typescript-estree → globby → fast-glob → micromatch`
132+
- **CVEs cleared**: GHSA-952p-6rrq-rcjv (MED 5.3 — ReDoS)
133+
- **Exit**: `fast-glob` bumps. Currently declares `^4.0.4`.
134+
135+
### `uuid: ^11.1.1`
136+
137+
- **Class**: **runtime** — this one matters most
138+
- **Path**: declared as a top-level runtime dep AND `thrift → uuid`
139+
- **CVEs cleared**: GHSA-w5hq-g745-h8pq (HIGH 7.5 — buffer-bounds in v3/v5/v6; driver only uses v4 but consumer scanners flag against our lockfile)
140+
- **Why an override is needed**: `thrift@0.23.0` declares `uuid: ^13.0.0`, but `uuid@13` is **ESM-only**. Our driver is compiled 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).
141+
- **Exit**: any of (a) we migrate `dist/` to ESM, (b) `thrift` drops the uuid dep, (c) `thrift` widens its range to `^11 || ^13` and we go through whichever export shape `thrift` decides on. Today, removing this override would cause `require('uuid')` from `dist/` to crash on Node ≤22.11 (which don't support `require(esm)`).
142+
143+
---
144+
145+
## How to audit
146+
147+
```bash
148+
# Show what depends on a specific override target:
149+
npm ls <package-name>
150+
151+
# Re-run the lockfile against OSV-Scanner to verify findings are still cleared:
152+
osv-scanner scan source --lockfile=package-lock.json
153+
```
154+
155+
When all entries' exit conditions are met, this file should be deleted along with the corresponding `overrides` block.

lib/connection/auth/tokenProvider/FederationProvider.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,17 @@
1-
import fetch from 'node-fetch';
1+
import nodeFetch from 'node-fetch';
22
import ITokenProvider from './ITokenProvider';
33
import Token from './Token';
44
import { getJWTIssuer, isSameHost } from './utils';
55

6+
// Indirection so tests can swap the fetch implementation without
7+
// patching the import system. Default is node-fetch.
8+
let fetchImpl: typeof nodeFetch = nodeFetch;
9+
10+
/** Test-only: replace the fetch implementation. Called with no arg, restores node-fetch. */
11+
export function setFederationFetchForTest(fn?: typeof nodeFetch): void {
12+
fetchImpl = fn ?? nodeFetch;
13+
}
14+
615
/**
716
* Token exchange endpoint path for Databricks OIDC.
817
*/
@@ -157,7 +166,7 @@ export default class FederationProvider implements ITokenProvider {
157166
const timeoutId = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS);
158167

159168
try {
160-
const response = await fetch(url, {
169+
const response = await fetchImpl(url, {
161170
method: 'POST',
162171
headers: {
163172
'Content-Type': 'application/x-www-form-urlencoded',

lib/utils/lz4.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,20 @@ import type LZ4Namespace from 'lz4';
22

33
type LZ4Module = typeof LZ4Namespace;
44

5+
// Loader function — extracted so tests can inject a stub without
6+
// having to patch CJS-only Module._load. Default is the real require.
7+
// eslint-disable-next-line global-require
8+
let loader: () => LZ4Module = () => require('lz4');
9+
10+
/** Test-only: replace the loader. Restores the real one when called with no arg. */
11+
export function setLz4LoaderForTest(fn?: () => LZ4Module): void {
12+
// eslint-disable-next-line global-require
13+
loader = fn ?? (() => require('lz4'));
14+
}
15+
516
function tryLoadLZ4Module(): LZ4Module | undefined {
617
try {
7-
return require('lz4'); // eslint-disable-line global-require
18+
return loader();
819
} catch (err) {
920
if (!(err instanceof Error) || !('code' in err)) {
1021
// eslint-disable-next-line no-console
@@ -39,4 +50,9 @@ function getResolvedModule() {
3950
return resolvedModule === null ? undefined : resolvedModule;
4051
}
4152

53+
/** Test-only: reset the memoized resolution so the next call re-invokes the loader. */
54+
export function resetLz4CacheForTest(): void {
55+
resolvedModule = undefined;
56+
}
57+
4258
export default getResolvedModule;

package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@
2121
"type-check": "tsc --noEmit",
2222
"prettier": "prettier . --check",
2323
"prettier:fix": "prettier . --write",
24-
"lint": "eslint lib/** tests/e2e/** --ext .js,.ts",
25-
"lint:fix": "eslint lib/** --ext .js,.ts --fix"
24+
"lint": "eslint 'lib/**/*.{js,ts}' 'tests/e2e/**/*.{js,ts}' --ext .js,.ts",
25+
"lint:fix": "eslint 'lib/**/*.{js,ts}' --ext .js,.ts --fix"
2626
},
2727
"repository": {
2828
"type": "git",

tests/e2e/README.md

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
# End-to-End Tests
2+
3+
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.
4+
5+
## Environment
6+
7+
| Variable | Used for |
8+
| ------------------ | ------------------------------------------------------------------------ |
9+
| `E2E_HOST` | Workspace hostname |
10+
| `E2E_PATH` | Warehouse HTTP path |
11+
| `E2E_ACCESS_TOKEN` | PAT for auth |
12+
| `E2E_TABLE_SUFFIX` | Suffix appended to per-test table names so concurrent runs don't collide |
13+
| `E2E_CATALOG` | Catalog (default: `peco`) |
14+
| `E2E_SCHEMA` | Schema (default: `default`) |
15+
| `E2E_VOLUME` | Volume name (default: `e2etests`) |
16+
17+
## CI parallelism
18+
19+
The `e2e-test` job in `.github/workflows/main.yml` runs as a 5-entry matrix across Node 16/18/20/22/24. All five entries point at the same workspace, catalog, schema, and volume.
20+
21+
Per-test isolation is achieved by:
22+
23+
- **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 `-`.
24+
- **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.
25+
26+
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.
27+
28+
## Local invocation
29+
30+
`npm run e2e` must be run from the repo root. Some specs resolve fixture paths relative to `process.cwd()`.
31+
32+
## Warehouse capacity
33+
34+
Five parallel CI 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

Lines changed: 8 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -100,48 +100,17 @@ export class OAuthCallbackServerStub<
100100
return this;
101101
}
102102

103-
// Dummy methods and properties for compatibility with `http.Server`
104-
105-
public maxHeadersCount: number | null = null;
106-
107-
public maxRequestsPerSocket: number | null = null;
108-
109-
public timeout: number = -1;
110-
111-
public headersTimeout: number = -1;
112-
113-
public keepAliveTimeout: number = -1;
114-
115-
public requestTimeout: number = -1;
116-
117-
public maxConnections: number = -1;
118-
119-
public connections: number = 0;
120-
121-
public setTimeout() {
122-
return this;
123-
}
124-
125-
public closeAllConnections() {}
126-
127-
public closeIdleConnections() {}
128-
103+
// No-op shims for the subset of `http.Server` members production code
104+
// touches. We intentionally do NOT mirror the full http.Server surface;
105+
// the call site uses an `as unknown as ...` cast (see AuthorizationCode
106+
// .test.ts) to satisfy the type checker, and that cast is what carries
107+
// the "trust me, this is Server-shaped" assertion. When @types/node
108+
// adds a new Server member, the test file's cast adapts automatically;
109+
// when the OAuth code starts calling a new Server member, add a shim
110+
// here and the runtime test will exercise it.
129111
public address() {
130112
return null;
131113
}
132-
133-
public getConnections() {}
134-
135-
public ref() {
136-
return this;
137-
}
138-
139-
public unref() {
140-
return this;
141-
}
142-
143-
// Required by @types/node >= 18.19.x (Node 20+ added Symbol.asyncDispose to Server).
144-
public async [Symbol.asyncDispose]() {}
145114
}
146115

147116
export class AuthorizationCodeStub {

0 commit comments

Comments
 (0)