Skip to content

Commit 0385e1d

Browse files
follow-up: review-squad LOW/MED follow-ups (F4, F8, F11, F14) (#401)
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 <vikrant.puppala@databricks.com>
1 parent 11d3f59 commit 0385e1d

8 files changed

Lines changed: 257 additions & 48 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: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
# Security Overrides
2+
3+
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.
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, 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.
10+
- **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".
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, GHSA-6v7q-wjvx-w8wg, GHSA-rp42-5vxx-qpwr, GHSA-rpmf-866q-6p89
21+
- **Exit**: `get-uri` bumps its `basic-ftp` dep range to include `^5.3.1`.
22+
23+
### `@75lb/deep-merge: ^1.1.2`
24+
25+
- **Class**: dev (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
28+
- **Exit**: `table-layout` bumps its dep. Note `apache-arrow@13` ships unused CLI tooling — bumping arrow to `15.x+` drops this dep entirely.
29+
30+
### `ws: ^8.18.0`
31+
32+
- **Class**: runtime (thrift's WebSocket transport)
33+
- **Path**: `thrift → ws` AND `thrift → isomorphic-ws → ws`
34+
- **CVEs cleared**: GHSA-3h5v-q93c-6h6q (ws@5.x DoS)
35+
- **Exit**: `thrift` bumps its declared `ws` range to `^8.x`. Without the override, `thrift` would pull the vulnerable `ws@5.x`.
36+
37+
### `ip-address: ^10.1.1`
38+
39+
- **Class**: runtime
40+
- **Path**: `proxy-agent → socks-proxy-agent → socks → ip-address`
41+
- **CVEs cleared**: GHSA-v2v4-37r5-5v8g (IPv6 parsing DoS)
42+
- **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.
43+
- **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/`.
44+
45+
### `form-data: ^4.0.4`
46+
47+
- **Class**: runtime
48+
- **Path**: `node-fetch → form-data` (multipart bodies)
49+
- **CVEs cleared**: GHSA-fjxv-7rqg-78g4 (unsafe random boundary generation)
50+
- **Exit**: `node-fetch` bumps its `form-data` dep range to include the patched line.
51+
52+
### `serialize-javascript: ^7.0.5`
53+
54+
- **Class**: dev (mocha)
55+
- **Path**: `mocha → serialize-javascript`
56+
- **CVEs cleared**: GHSA-5c6j-r48x-rmvq (XSS via prototype pollution)
57+
- **Note**: the patched line requires Node ≥ 20, which is satisfied by `engines.node >= 20`.
58+
- **Exit**: mocha bumps its declared range to the patched line.
59+
60+
### `uuid: ^11.1.1`
61+
62+
- **Class**: **runtime** — this one matters most
63+
- **Path**: declared as a top-level runtime dep AND `thrift → uuid`
64+
- **CVEs cleared**: GHSA-w5hq-g745-h8pq (buffer-bounds in v3/v5/v6; the driver only uses v4, but consumer scanners flag against our lockfile)
65+
- **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).
66+
- **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)`.
67+
68+
---
69+
70+
## How to audit
71+
72+
```bash
73+
# Show what depends on a specific override target:
74+
npm ls <package-name>
75+
76+
# Re-run the lockfile against OSV-Scanner to verify findings are still cleared:
77+
osv-scanner scan source --lockfile=package-lock.json
78+
```
79+
80+
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',

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
"type-check": "tsc --noEmit",
2424
"prettier": "prettier . --check",
2525
"prettier:fix": "prettier . --write",
26-
"lint": "eslint lib/** tests/e2e/** --ext .js,.ts",
26+
"lint": "eslint 'lib/**/*.{js,ts}' 'tests/e2e/**/*.{js,ts}' --ext .js,.ts",
2727
"lint:fix": "eslint lib/** --ext .js,.ts --fix"
2828
},
2929
"repository": {

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 matrix across Node 20/22/24/26. All 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+
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

Lines changed: 10 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -100,51 +100,19 @@ 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 shim for the subset of `http.Server` members production code
104+
// touches. We intentionally do NOT mirror the full http.Server surface
105+
// (setTimeout, closeAllConnections, ref/unref, Symbol.asyncDispose, the
106+
// maxHeadersCount/timeout/... property pile) -- those existed only to
107+
// satisfy http.Server's structural type and had to grow every time
108+
// @types/node widened the interface. The call site casts the stub via
109+
// `as unknown as ...` (see AuthorizationCode.test.ts), and that cast is
110+
// what carries the "trust me, this is Server-shaped" assertion. When the
111+
// OAuth code starts calling a new Server member, add a shim here and the
112+
// runtime test exercises it; @types/node additions no longer touch this.
129113
public address() {
130114
return null;
131115
}
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-
// Cast through `any`: the project targets ES2018, whose lib predates
145-
// Symbol.asyncDispose, so referencing it directly is a compile error even
146-
// though the runtime (Node 20+) provides it.
147-
public async [(Symbol as any).asyncDispose]() {}
148116
}
149117

150118
export class AuthorizationCodeStub {

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,11 @@ function prepareTestInstances(options: Partial<AuthorizationCodeOptions>) {
9595

9696
const createHttpServer = sinon.spy((requestHandler: (req: IncomingMessage, res: ServerResponse) => void) => {
9797
httpServer.requestHandler = requestHandler;
98-
return httpServer;
98+
// OAuthCallbackServerStub only implements the http.Server members the
99+
// OAuth code actually calls (listen/close/address). This cast carries
100+
// the "trust me, this is Server-shaped" assertion so the stub doesn't
101+
// have to mirror the full (and @types/node-drifting) http.Server surface.
102+
return httpServer as unknown as ReturnType<AuthorizationCode['createHttpServer']>;
99103
});
100104

101105
authCode['createHttpServer'] = createHttpServer;

tests/unit/connection/auth/tokenProvider/FederationProvider.test.ts

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import { expect } from 'chai';
22
import sinon from 'sinon';
3-
import FederationProvider from '../../../../../lib/connection/auth/tokenProvider/FederationProvider';
3+
import type nodeFetch from 'node-fetch';
4+
import FederationProvider, {
5+
setFederationFetchForTest,
6+
} from '../../../../../lib/connection/auth/tokenProvider/FederationProvider';
47
import ITokenProvider from '../../../../../lib/connection/auth/tokenProvider/ITokenProvider';
58
import Token from '../../../../../lib/connection/auth/tokenProvider/Token';
69

@@ -68,6 +71,117 @@ describe('FederationProvider', () => {
6871
});
6972
});
7073

74+
describe('exchange path', () => {
75+
// These tests exercise the federation HTTP exchange — the branch
76+
// taken when the source JWT's issuer doesn't match the Databricks
77+
// host. The branch contains the AbortController + node-fetch shim
78+
// typing fix; without coverage here a regression in those mechanics
79+
// would only surface in production.
80+
81+
afterEach(() => {
82+
setFederationFetchForTest(); // restore real node-fetch
83+
});
84+
85+
// Helper: build a fake node-fetch Response.
86+
function buildFakeResponse(opts: {
87+
ok: boolean;
88+
status?: number;
89+
statusText?: string;
90+
body?: unknown;
91+
text?: string;
92+
}): nodeFetch.Response {
93+
return {
94+
ok: opts.ok,
95+
status: opts.status ?? (opts.ok ? 200 : 500),
96+
statusText: opts.statusText ?? '',
97+
json: async () => opts.body,
98+
text: async () => opts.text ?? '',
99+
} as unknown as nodeFetch.Response;
100+
}
101+
102+
it('should exchange foreign-issued JWT for a Databricks token', async () => {
103+
const foreignJwt = createJWT({ iss: 'https://idp.example.com' });
104+
const baseProvider = new MockTokenProvider(foreignJwt);
105+
const federationProvider = new FederationProvider(baseProvider, 'my-workspace.cloud.databricks.com');
106+
107+
const fetchStub = sinon.stub<Parameters<typeof nodeFetch>, ReturnType<typeof nodeFetch>>().resolves(
108+
buildFakeResponse({
109+
ok: true,
110+
body: { access_token: 'exchanged-databricks-token', token_type: 'Bearer', expires_in: 3600 },
111+
}),
112+
);
113+
setFederationFetchForTest(fetchStub as unknown as typeof nodeFetch);
114+
115+
const token = await federationProvider.getToken();
116+
117+
expect(token.accessToken).to.equal('exchanged-databricks-token');
118+
expect(fetchStub.calledOnce).to.be.true;
119+
120+
// The exchange must POST to the Databricks /oidc/v1/token endpoint.
121+
const [url, init] = fetchStub.firstCall.args;
122+
expect(String(url)).to.include('my-workspace.cloud.databricks.com');
123+
expect(String(url)).to.include('/oidc/v1/token');
124+
expect(init!.method).to.equal('POST');
125+
126+
// Verify the signal propagates an AbortSignal — this is the cast
127+
// site that TS 5 type-strictness caught. Runtime-wise it must
128+
// still be a real AbortSignal-shaped object.
129+
const passedSignal = init!.signal as unknown as AbortSignal;
130+
expect(passedSignal, 'fetch init.signal must be set').to.exist;
131+
expect(typeof passedSignal.aborted, 'signal.aborted must be a boolean').to.equal('boolean');
132+
expect(passedSignal.aborted).to.be.false;
133+
});
134+
135+
it('should propagate abort from the controller to the signal observed by fetch', async () => {
136+
const foreignJwt = createJWT({ iss: 'https://idp.example.com' });
137+
const baseProvider = new MockTokenProvider(foreignJwt);
138+
const federationProvider = new FederationProvider(baseProvider, 'my-workspace.cloud.databricks.com', {
139+
returnOriginalTokenOnFailure: false,
140+
});
141+
142+
// Capture the signal so we can assert it implements the standard
143+
// AbortSignal contract. Resolve immediately with success to avoid
144+
// the 30s real-timeout path; the point is that the signal is wired
145+
// up, not to exercise the abort end-to-end.
146+
let capturedSignal: AbortSignal | undefined;
147+
const fetchStub = sinon
148+
.stub<Parameters<typeof nodeFetch>, ReturnType<typeof nodeFetch>>()
149+
.callsFake(async (_url, init) => {
150+
capturedSignal = init!.signal as unknown as AbortSignal;
151+
return buildFakeResponse({
152+
ok: true,
153+
body: { access_token: 'tok', token_type: 'Bearer', expires_in: 3600 },
154+
});
155+
});
156+
setFederationFetchForTest(fetchStub as unknown as typeof nodeFetch);
157+
158+
await federationProvider.getToken();
159+
160+
expect(capturedSignal, 'signal must reach fetch').to.exist;
161+
// The signal must implement the standard AbortSignal contract.
162+
expect(typeof capturedSignal!.aborted).to.equal('boolean');
163+
expect(typeof capturedSignal!.addEventListener).to.equal('function');
164+
});
165+
166+
it('should fall back to original token when exchange fails (returnOriginalTokenOnFailure default)', async () => {
167+
const foreignJwt = createJWT({ iss: 'https://idp.example.com' });
168+
const baseProvider = new MockTokenProvider(foreignJwt);
169+
const federationProvider = new FederationProvider(baseProvider, 'my-workspace.cloud.databricks.com');
170+
171+
const fetchStub = sinon
172+
.stub<Parameters<typeof nodeFetch>, ReturnType<typeof nodeFetch>>()
173+
.resolves(buildFakeResponse({ ok: false, status: 400, statusText: 'Bad Request', text: 'invalid_grant' }));
174+
setFederationFetchForTest(fetchStub as unknown as typeof nodeFetch);
175+
176+
const token = await federationProvider.getToken();
177+
178+
// Default behavior is to fall back to the original token on failure.
179+
// Retries kick in for 5xx; 400 is non-retryable so this should fail
180+
// fast on the first attempt.
181+
expect(token.accessToken).to.equal(foreignJwt);
182+
});
183+
});
184+
71185
describe('getName', () => {
72186
it('should return wrapped name', () => {
73187
const baseProvider = new MockTokenProvider('token');

0 commit comments

Comments
 (0)