Skip to content

Commit d36ccac

Browse files
committed
sea-auth-u2m: address round-1 review (HIGH error-mapping wiring + 7 mediums)
Ports the M2M-side devils-advocate round-1 fixes (commit eef9d30 on sea-auth-m2m) onto the U2M worktree. Same shape, with the U2M-specific adjustments noted below. Added `decodeNapiKernelError(err: unknown): Error` to `SeaErrorMapping.ts` and wrapped `SeaBackend.openSession`'s napi call in `try`/`catch` + the decoder. The wiring step was missing on both branches; now M2M and U2M users see typed errors (`AuthenticationError` on Unauthenticated, `HiveDriverError` on NetworkError, etc.) instead of raw `Error` with sentinel-prefixed message bodies. `buildSeaConnectionOptions` rejects: - PAT path + stray OAuth fields → HiveDriverError "cannot supply both `token` and `oauthClientId`/`...Secret`". - OAuth path (M2M or U2M) + stray `token` → HiveDriverError "cannot supply `token` alongside `authType: 'databricks-oauth'`". The OAuth-side check fires BEFORE the M2M/U2M split, so the U2M arm gets the protection too. Rewrote three M2M-arm error messages plus the U2M persistence message to be time-bound free: - "U2M lands in sea-auth-u2m" → "OAuth M2M requires `oauthClientSecret`. For interactive OAuth (U2M), see the driver OAuth U2M docs." - "Azure-direct OAuth ... is a later M1 task" → "Azure-direct OAuth ... is not supported. The workspace-OIDC discovery path handles Azure workspaces today without these options." - "M1+ follow-ups" → "Supported modes on the SEA backend today: ..." - U2M persistence: dropped "M1 Phase 2" — kept the technical explanation citing kernel-side `AuthConfig::External` plumbing (durable; describes the kernel gap, not the feature roadmap). Zero hits for `sea-auth-u2m|sea-auth-m2m|later M1|M1\+ follow|M1 Phase` in `lib/sea/`. Updated regex assertions in lockstep. `isBlankOrReserved(s)` helper trims + rejects empty-after-trim and literal `'undefined'` / `'null'` strings. Applied to `token`, `oauthClientId`, `oauthClientSecret`. E2e env-gate hardened the same way. Added `tests/unit/sea/auth-edge-cases.test.ts` with 18 cases: - Whitespace + reserved-literal PAT (3) - Same for `oauthClientId` / `oauthClientSecret` on M2M (4) - Ambiguous-creds: PAT+id, PAT+secret, M2M+token, U2M+token (4) - Explicit-undefined Azure-direct discriminants on M2M + U2M (3) - `decodeNapiKernelError` for Unauthenticated, NetworkError, SQLSTATE preservation, plain napi pass-through, corrupted envelope fallback (5) Added a bad-secret `it(...)` block to `auth-m2m-e2e.test.ts` that asserts `AuthenticationError` + `invalid_client`. Closes the loop on DA-F1 by proving the kernel-side error path surfaces correctly. The U2M e2e remains `it.skip` pending the Playwright/Puppeteer harness; once it lands, the same negative-path pattern can be added there. L-F3, L-F4, L-F5 — deferred per the previous fixup's reasoning. Tests: - Unit: 74/74 pass (was 55 before this commit: +18 from edge-cases + 1 from new pending placeholder count). - TypeScript build: clean. - Native build: unchanged (no Rust changes this commit). Co-authored-by: Isaac
1 parent e9db5dc commit d36ccac

6 files changed

Lines changed: 513 additions & 38 deletions

File tree

lib/sea/SeaAuth.ts

Lines changed: 62 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,19 @@ function prependSlash(str: string): string {
7777
return str;
7878
}
7979

80+
/**
81+
* Reject inputs that pass `typeof === 'string' && length > 0` but are
82+
* structurally useless as credentials: whitespace-only strings, and the
83+
* literal strings `'undefined'` / `'null'` that buggy shell exports
84+
* (e.g. `export FOO="$UNSET_VAR"`) produce. Surfacing these here means
85+
* an OAuth flow's `invalid_client` from the workspace is always a real
86+
* credential mismatch, never a malformed-input passthrough.
87+
*/
88+
function isBlankOrReserved(s: string): boolean {
89+
const trimmed = s.trim();
90+
return trimmed.length === 0 || trimmed === 'undefined' || trimmed === 'null';
91+
}
92+
8093
/**
8194
* Validate the user-supplied `ConnectionOptions` and build the
8295
* napi-binding's connection-options shape.
@@ -87,9 +100,7 @@ function prependSlash(str: string): string {
87100
* `DBSQLClient.createAuthProvider`).
88101
* - OAuth M2M: `authType: 'databricks-oauth'` + `oauthClientId` +
89102
* `oauthClientSecret`. Kernel handles OIDC discovery, client_credentials
90-
* exchange, and re-auth on expiry internally (no caching needed — M2M
91-
* never has a refresh token; see `auth/oauth/m2m.rs` and the thrift
92-
* parity note at `OAuthManager.ts:178-181`).
103+
* exchange, and re-auth on expiry internally.
93104
* - OAuth U2M: `authType: 'databricks-oauth'` + NO `oauthClientSecret`.
94105
* Kernel runs the PKCE auth-code dance (opens a browser, listens on
95106
* localhost:8030, exchanges the code, persists to
@@ -99,20 +110,24 @@ function prependSlash(str: string): string {
99110
*
100111
* Out of scope on the OAuth paths (rejected with a clear error):
101112
* - `azureTenantId` / `useDatabricksOAuthInAzure` → Microsoft Entra
102-
* direct flow with `<tenantId>/.default` scope rewrite. The kernel
103-
* uses workspace-OIDC discovery (which works against Azure workspaces
104-
* too — they serve `/oidc/.well-known/...`); Entra-direct is a
105-
* follow-on M1 Phase 2 task.
106-
* - `persistence` on either flavor — for M2M the kernel doesn't cache
107-
* (re-issuing is cheap; M2M has no refresh token). For U2M, custom
108-
* persistence requires the kernel to expose `AuthConfig::External`
109-
* (M1 Phase 2 task). The kernel-internal disk cache works for the
110-
* standard flow today.
113+
* direct flow. The kernel uses workspace-OIDC discovery (which works
114+
* against Azure workspaces too — they serve `/oidc/.well-known/...`)
115+
* and does not implement the Entra-direct scope-rewrite path.
116+
* - `persistence` on M2M → M2M tokens are not cached (re-issuing is
117+
* cheap; no refresh token).
118+
* - `persistence` on U2M → custom token store is a parity gap;
119+
* requires kernel-side `AuthConfig::External` plumbing. The kernel's
120+
* auto-disk-cache works for the standard flow today.
121+
*
122+
* Ambiguity:
123+
* - PAT path: rejects when OAuth fields (`oauthClientId` /
124+
* `oauthClientSecret`) are simultaneously set.
125+
* - OAuth path: rejects when `token` is set alongside OAuth fields.
111126
*
112127
* Throws:
113-
* - `AuthenticationError` for missing required credentials.
128+
* - `AuthenticationError` for missing/blank required credentials.
114129
* - `HiveDriverError` for unsupported auth modes / Azure-direct /
115-
* custom persistence.
130+
* custom persistence / ambiguous combinations.
116131
*/
117132
export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNativeConnectionOptions {
118133
const { authType } = options as { authType?: string };
@@ -122,30 +137,44 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative
122137
httpPath: prependSlash(options.path),
123138
};
124139

140+
const oauth = options as {
141+
oauthClientId?: string;
142+
oauthClientSecret?: string;
143+
azureTenantId?: string;
144+
useDatabricksOAuthInAzure?: boolean;
145+
persistence?: unknown;
146+
};
147+
125148
if (authType === undefined || authType === 'access-token') {
126149
const { token } = options as { token?: string };
127-
if (typeof token !== 'string' || token.length === 0) {
150+
if (typeof token !== 'string' || isBlankOrReserved(token)) {
128151
throw new AuthenticationError(
129152
'SEA backend: a non-empty PAT must be supplied via `token` when using `authType: \'access-token\'`.',
130153
);
131154
}
155+
if (oauth.oauthClientId !== undefined || oauth.oauthClientSecret !== undefined) {
156+
throw new HiveDriverError(
157+
'SEA backend: cannot supply both `token` and `oauthClientId`/`oauthClientSecret` ' +
158+
"on the same connection. Pick one: 'access-token' (PAT) uses `token`; " +
159+
"'databricks-oauth' uses the OAuth fields.",
160+
);
161+
}
132162
return { ...base, authMode: 'Pat', token };
133163
}
134164

135165
if (authType === 'databricks-oauth') {
136-
const oauth = options as {
137-
oauthClientId?: string;
138-
oauthClientSecret?: string;
139-
azureTenantId?: string;
140-
useDatabricksOAuthInAzure?: boolean;
141-
persistence?: unknown;
142-
};
166+
if ((options as { token?: string }).token !== undefined) {
167+
throw new HiveDriverError(
168+
"SEA backend: cannot supply `token` alongside `authType: 'databricks-oauth'`. " +
169+
"Use `authType: 'access-token'` for PAT, or omit `token` to use OAuth.",
170+
);
171+
}
143172

144173
if (oauth.azureTenantId !== undefined || oauth.useDatabricksOAuthInAzure === true) {
145174
throw new HiveDriverError(
146175
'SEA backend: Azure-direct OAuth (azureTenantId / useDatabricksOAuthInAzure) ' +
147-
'is a later M1 task; the kernel uses workspace-OIDC discovery today, ' +
148-
'which works against Azure workspaces with no extra options.',
176+
'is not supported. The workspace-OIDC discovery path handles Azure workspaces ' +
177+
'today without these options.',
149178
);
150179
}
151180

@@ -156,7 +185,7 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative
156185
if (oauth.persistence !== undefined) {
157186
throw new HiveDriverError(
158187
'SEA backend: `persistence` (custom OAuth token store) is not yet wired through ' +
159-
'to the kernel — requires `AuthConfig::External` plumbing planned for M1 Phase 2. ' +
188+
'to the kernel — requires `AuthConfig::External` plumbing. ' +
160189
'Today the kernel auto-persists U2M tokens to ' +
161190
'`~/.config/databricks-sql-kernel/oauth/` which works for the standard flow; ' +
162191
"the JS-supplied hook (matching thrift's `OAuthPersistence` interface) lands " +
@@ -171,12 +200,14 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative
171200
}
172201

173202
// M2M.
174-
if (typeof oauth.oauthClientId !== 'string' || oauth.oauthClientId.length === 0) {
175-
throw new AuthenticationError('SEA backend: `oauthClientId` is required for OAuth M2M.');
203+
if (typeof oauth.oauthClientId !== 'string' || isBlankOrReserved(oauth.oauthClientId)) {
204+
throw new AuthenticationError(
205+
'SEA backend: `oauthClientId` is required (non-empty, non-whitespace) for OAuth M2M.',
206+
);
176207
}
177-
if (typeof oauth.oauthClientSecret !== 'string' || oauth.oauthClientSecret.length === 0) {
208+
if (typeof oauth.oauthClientSecret !== 'string' || isBlankOrReserved(oauth.oauthClientSecret)) {
178209
throw new AuthenticationError(
179-
'SEA backend: `oauthClientSecret` must be a non-empty string for OAuth M2M.',
210+
'SEA backend: `oauthClientSecret` must be a non-empty non-whitespace string for OAuth M2M.',
180211
);
181212
}
182213
if (oauth.persistence !== undefined) {
@@ -195,7 +226,7 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative
195226

196227
throw new HiveDriverError(
197228
`SEA backend: unsupported auth mode '${authType}'. ` +
198-
`Supported modes today: 'access-token' (PAT), 'databricks-oauth' (M2M + U2M). ` +
199-
`Other modes (token-provider, external-token, static-token, custom) are M1+ follow-ups.`,
229+
"Supported modes on the SEA backend today: 'access-token' (PAT) and 'databricks-oauth' " +
230+
'(M2M with oauthClientId+oauthClientSecret, or U2M with neither).',
200231
);
201232
}

lib/sea/SeaErrorMapping.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,15 @@ import AuthenticationError from '../errors/AuthenticationError';
33
import OperationStateError, { OperationStateErrorCode } from '../errors/OperationStateError';
44
import ParameterError from '../errors/ParameterError';
55

6+
/**
7+
* Sentinel prefix the napi binding's `napi_err_from_kernel` puts on
8+
* `Error.message` when the underlying failure was a structured kernel
9+
* `Error` rather than a plain napi `InvalidArg` from binding-side
10+
* validation. Defined here (and in `native/sea/src/error.rs:44`) — the
11+
* two MUST stay in lockstep.
12+
*/
13+
const ERROR_SENTINEL = '__databricks_error__:';
14+
615
/**
716
* Shape of the kernel error surfaced by the napi-binding's `napi_err_from_kernel`.
817
*
@@ -139,3 +148,57 @@ export function mapKernelErrorToJsError(kErr: KernelErrorShape): ErrorWithSqlSta
139148

140149
return attachSqlState(error, sqlstate);
141150
}
151+
152+
/**
153+
* Decode a napi-binding error into the typed JS error class.
154+
*
155+
* Two paths:
156+
* - Structured kernel error: `Error.message` starts with
157+
* {@link ERROR_SENTINEL} followed by a JSON envelope. We strip the
158+
* sentinel, parse the JSON, and route through
159+
* {@link mapKernelErrorToJsError}.
160+
* - Binding-side error (e.g. `napi::Error::new(InvalidArg, "openSession:
161+
* \`token\` is required for the requested auth mode")` produced by
162+
* the binding's own validation): returned unchanged. These don't
163+
* carry kernel `code` info, so we surface them as-is.
164+
*
165+
* Non-`Error` values (e.g. a `Promise.reject('string')`) pass through
166+
* wrapped in `HiveDriverError` so callers always see an `Error`
167+
* subclass.
168+
*/
169+
export function decodeNapiKernelError(err: unknown): Error {
170+
if (!(err instanceof Error)) {
171+
return new HiveDriverError(typeof err === 'string' ? err : 'SEA backend: unknown error');
172+
}
173+
174+
const { message } = err;
175+
if (typeof message !== 'string' || !message.startsWith(ERROR_SENTINEL)) {
176+
return err;
177+
}
178+
179+
const jsonStr = message.slice(ERROR_SENTINEL.length);
180+
let parsed: unknown;
181+
try {
182+
parsed = JSON.parse(jsonStr);
183+
} catch {
184+
// Corrupted envelope — surface the raw message rather than
185+
// silently dropping the original error.
186+
return err;
187+
}
188+
189+
if (
190+
typeof parsed !== 'object' ||
191+
parsed === null ||
192+
typeof (parsed as { code?: unknown }).code !== 'string' ||
193+
typeof (parsed as { message?: unknown }).message !== 'string'
194+
) {
195+
return err;
196+
}
197+
198+
const kErr = parsed as { code: string; message: string; sqlState?: string };
199+
return mapKernelErrorToJsError({
200+
code: kErr.code,
201+
message: kErr.message,
202+
sqlstate: kErr.sqlState,
203+
});
204+
}

tests/integration/sea/auth-m2m-e2e.test.ts

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
import { expect } from 'chai';
1616
import { DBSQLClient } from '../../../lib';
17+
import AuthenticationError from '../../../lib/errors/AuthenticationError';
1718

1819
/**
1920
* sea-auth M1 OAuth M2M end-to-end:
@@ -49,7 +50,17 @@ describe('sea-auth e2e — OAuth M2M through DBSQLClient ↔ SeaBackend ↔ napi
4950
this.timeout(120_000);
5051

5152
before(function gate() {
52-
if (!host || !path || !oauthClientId || !oauthClientSecret) {
53+
// Reject not just absent env vars but also literal `'undefined'` /
54+
// `'null'` / whitespace-only values from buggy shell exports — these
55+
// would otherwise reach the workspace as bogus creds and yield an
56+
// `invalid_client` indistinguishable from a real SP-not-registered
57+
// issue.
58+
const looksReal = (s: string | undefined): s is string => {
59+
if (typeof s !== 'string') return false;
60+
const t = s.trim();
61+
return t.length > 0 && t !== 'undefined' && t !== 'null';
62+
};
63+
if (!looksReal(host) || !looksReal(path) || !looksReal(oauthClientId) || !looksReal(oauthClientSecret)) {
5364
// eslint-disable-next-line no-invalid-this
5465
this.skip();
5566
}
@@ -77,4 +88,32 @@ describe('sea-auth e2e — OAuth M2M through DBSQLClient ↔ SeaBackend ↔ napi
7788

7889
await client.close();
7990
});
91+
92+
// Negative path — proves the kernel-side OAuth error path is intact
93+
// and surfaces as the typed `AuthenticationError` (DA-F1 + DA-F6).
94+
// Distinguishes "creds wrong" (this test passes with bogus secret)
95+
// from "all code broken" (this test fails with a non-AuthenticationError).
96+
it('rejects with AuthenticationError when oauthClientSecret is deliberately wrong', async () => {
97+
const client = new DBSQLClient();
98+
99+
await client.connect({
100+
host: host as string,
101+
path: path as string,
102+
authType: 'databricks-oauth',
103+
oauthClientId: oauthClientId as string,
104+
oauthClientSecret: 'definitely-not-the-real-secret-deadbeef',
105+
useSEA: true,
106+
});
107+
108+
let caught: unknown;
109+
try {
110+
await client.openSession();
111+
} catch (e) {
112+
caught = e;
113+
}
114+
expect(caught).to.be.instanceOf(AuthenticationError);
115+
expect((caught as Error).message).to.match(/invalid_client/i);
116+
117+
await client.close();
118+
});
80119
});

0 commit comments

Comments
 (0)