sea-auth: address PR #379 review (F1–F10)

msrathore-db · msrathore-db · commit 50f436e93dff · 2026-05-31T00:14:31.000Z
- F1: fix the fake-binding casts in auth-pat.test.ts so the unit suite
  type-checks (nyc registers ts-node/register with full type-checking —
  the bad casts were failing the whole unit-test CI job). Cast through
  the binding's own member types.
- F2: the PAT e2e test couldn't compile (useSEA excess-property) and was
  orphaned under tests/integration (run by no CI script, outside the lint
  glob). Cast useSEA as ConnectionOptions &amp; InternalConnectionOptions and
  move it to tests/e2e/sea/ (wired by `npm run e2e` + linted); drop the
  orphaned tests/integration/.mocharc.js.
- F3: surface the real server-issued session id from the kernel
  Connection.sessionId getter instead of a process-local synthetic
  counter, so DBSQLSession's logged id correlates with kernel/server logs.
- F4: only build the Thrift auth/connection providers on the Thrift path;
  the SEA path never reads them, so this stops validating the PAT twice
  and constructing a throwaway OAuth provider for an OAuth+useSEA call.
- F5: derive SeaNativeConnectionOptions via Pick&lt;SeaConnectionOptions,…&gt;
  instead of re-declaring it, so a kernel field rename fails to compile
  rather than silently drifting.
- F6: reject whitespace/control chars in the PAT (parity with the Python
  driver; the kernel HeaderValue already blocks CR/LF/NUL).
- F7: extract the duplicated prependSlash into lib/utils/prependSlash.ts.
- F8: lazy-load the native binding (resolve on first use, not in the
  constructor) so constructing SeaBackend never throws on a platform
  without the optional .node — connect()'s clearer validation runs first.
- F9: thread the client logger into SeaBackend and log backend selection
  + session open/close (token excluded).
- F10: document that SEA connect() does no network round-trip.

Verified: tsc clean (0 errors, was 4), 13/13 unit tests, lib lint clean.

Co-authored-by: Isaac
diff --git a/lib/DBSQLClient.ts b/lib/DBSQLClient.ts
@@ -32,13 +32,7 @@ import IDBSQLLogger, { LogLevel } from './contracts/IDBSQLLogger';
 import DBSQLLogger from './DBSQLLogger';
 import CloseableCollection from './utils/CloseableCollection';
 import IConnectionProvider from './connection/contracts/IConnectionProvider';
-
-function prependSlash(str: string): string {
-  if (str.length > 0 && str.charAt(0) !== '/') {
-    return `/${str}`;
-  }
-  return str;
-}
+import prependSlash from './utils/prependSlash';
 
 export type ThriftLibrary = Pick<typeof thrift, 'createClient'>;
 
@@ -234,20 +228,26 @@ export default class DBSQLClient extends EventEmitter implements IDBSQLClient, I
       this.config.userAgentEntry = options.userAgentEntry;
     }
 
-    this.authProvider = this.createAuthProvider(options, authProvider);
-
-    this.connectionProvider = this.createConnectionProvider(options);
-
     // M0: `useSEA` is consumed via a non-exported internal-options cast so it
     // doesn't ship in the public `.d.ts`. Mirrors Python's `kwargs.get("use_sea")`
     // pattern (see databricks-sql-python/src/databricks/sql/session.py).
     const internalOptions = options as ConnectionOptions & InternalConnectionOptions;
-    this.backend = internalOptions.useSEA
-      ? new SeaBackend()
-      : new ThriftBackend({
-          context: this,
-          onConnectionEvent: (event, payload) => this.forwardConnectionEvent(event, payload),
-        });
+
+    if (internalOptions.useSEA) {
+      // The SEA backend authenticates inside the native binding; the
+      // Thrift auth/connection providers are never read on this path, so
+      // we don't build them (avoids validating the PAT twice and
+      // constructing a throwaway OAuth provider for an OAuth+useSEA call).
+      this.logger.log(LogLevel.info, 'Connecting via the SEA (native) backend');
+      this.backend = new SeaBackend(undefined, this.logger);
+    } else {
+      this.authProvider = this.createAuthProvider(options, authProvider);
+      this.connectionProvider = this.createConnectionProvider(options);
+      this.backend = new ThriftBackend({
+        context: this,
+        onConnectionEvent: (event, payload) => this.forwardConnectionEvent(event, payload),
+      });
+    }
 
     await this.backend.connect(options);
 
diff --git a/lib/sea/SeaAuth.ts b/lib/sea/SeaAuth.ts
@@ -15,26 +15,17 @@
 import { ConnectionOptions } from '../contracts/IDBSQLClient';
 import AuthenticationError from '../errors/AuthenticationError';
 import HiveDriverError from '../errors/HiveDriverError';
+import prependSlash from '../utils/prependSlash';
+import { SeaConnectionOptions } from './SeaNativeLoader';
 
 /**
- * Shape consumed by the napi-binding's `openSession()` (see
- * `native/sea/index.d.ts`). M0 supports PAT only — `token` is required.
- *
- * Mirrors `ConnectionOptions` in the binding's `.d.ts`; declared locally
- * to avoid coupling the JS-side adapter to the auto-generated TS file.
+ * Shape consumed by the napi-binding's `openSession()`. M0 sends only the
+ * PAT triple, so we `Pick` those fields off the binding's generated
+ * `ConnectionOptions` (re-exported as `SeaConnectionOptions`) rather than
+ * re-declaring them — if the kernel renames `hostName`/`httpPath`/`token`
+ * this stops compiling instead of silently drifting.
  */
-export interface SeaNativeConnectionOptions {
-  hostName: string;
-  httpPath: string;
-  token: string;
-}
-
-function prependSlash(str: string): string {
-  if (str.length > 0 && str.charAt(0) !== '/') {
-    return `/${str}`;
-  }
-  return str;
-}
+export type SeaNativeConnectionOptions = Pick<SeaConnectionOptions, 'hostName' | 'httpPath' | 'token'>;
 
 /**
  * Validate that the user-supplied `ConnectionOptions` describe a PAT auth
@@ -74,6 +65,17 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative
       'SEA backend: a non-empty PAT must be supplied via `token` when using `authType: \'access-token\'`.',
     );
   }
+  // Reject whitespace / control characters in the PAT. The kernel's
+  // reqwest `HeaderValue` already hard-rejects CR/LF/NUL at build time so
+  // this isn't a header-injection fix — it's parity with the Python
+  // driver (auth_bridge.py rejects `[\x00-\x20\x7f]`) and catches
+  // copy-paste whitespace before a confusing downstream failure.
+  // eslint-disable-next-line no-control-regex
+  if (/[\x00-\x20\x7f]/.test(token)) {
+    throw new AuthenticationError(
+      'SEA backend: the PAT supplied via `token` must not contain whitespace or control characters.',
+    );
+  }
 
   return {
     hostName: options.host,
diff --git a/lib/sea/SeaBackend.ts b/lib/sea/SeaBackend.ts
@@ -31,6 +31,7 @@ import {
 import Status from '../dto/Status';
 import InfoValue from '../dto/InfoValue';
 import HiveDriverError from '../errors/HiveDriverError';
+import IDBSQLLogger, { LogLevel } from '../contracts/IDBSQLLogger';
 import { getSeaNative, SeaNativeBinding } from './SeaNativeLoader';
 import { buildSeaConnectionOptions, SeaNativeConnectionOptions } from './SeaAuth';
 
@@ -44,6 +45,8 @@ const NOT_IMPLEMENTED_SESSION =
  * leak into every call site.
  */
 interface NativeConnection {
+  /** Server-issued session id (kernel `Connection.sessionId` getter). */
+  readonly sessionId: string;
   close(): Promise<void>;
 }
 
@@ -54,21 +57,22 @@ interface NativeConnection {
  * subset required to round-trip a connect-open-close cycle. Every other
  * method throws a clear "not implemented in M0" `HiveDriverError`.
  *
- * The `id` field is currently a synthetic counter-based string; the kernel
- * exposes a real session-id through a follow-on getter that
- * `sea-execution` will wire through.
+ * `id` is the server-issued session id read straight off the kernel
+ * `Connection` (its `sessionId` getter, readable even after close()), so
+ * the value logged by `DBSQLSession` correlates with kernel / server logs
+ * rather than being a process-local synthetic counter.
  */
 export class SeaSessionBackend implements ISessionBackend {
-  private static seq = 0;
-
   public readonly id: string;
 
   private readonly connection: NativeConnection;
 
-  constructor(connection: NativeConnection) {
+  private readonly logger?: IDBSQLLogger;
+
+  constructor(connection: NativeConnection, logger?: IDBSQLLogger) {
     this.connection = connection;
-    SeaSessionBackend.seq += 1;
-    this.id = `sea-session-${SeaSessionBackend.seq}`;
+    this.logger = logger;
+    this.id = connection.sessionId;
   }
 
   /* eslint-disable @typescript-eslint/no-unused-vars */
@@ -121,6 +125,7 @@ export class SeaSessionBackend implements ISessionBackend {
   /* eslint-enable @typescript-eslint/no-unused-vars */
 
   public async close(): Promise<Status> {
+    this.logger?.log(LogLevel.debug, `SEA session closing with id: ${this.id}`);
     await this.connection.close();
     return Status.success();
   }
@@ -140,16 +145,36 @@ export class SeaSessionBackend implements ISessionBackend {
 export default class SeaBackend implements IBackend {
   private nativeOptions?: SeaNativeConnectionOptions;
 
-  private readonly native: SeaNativeBinding;
+  private readonly injectedNative?: SeaNativeBinding;
+
+  private cachedNative?: SeaNativeBinding;
+
+  private readonly logger?: IDBSQLLogger;
 
-  constructor(native: SeaNativeBinding = getSeaNative()) {
-    this.native = native;
+  // `native` is injectable (tests pass a fake); production leaves it
+  // undefined and the binding is resolved lazily on first use so that
+  // constructing a SeaBackend never throws on a platform without the
+  // optional `.node` — the clearer auth/option validation in connect()
+  // runs first.
+  constructor(native?: SeaNativeBinding, logger?: IDBSQLLogger) {
+    this.injectedNative = native;
+    this.logger = logger;
+  }
+
+  private get native(): SeaNativeBinding {
+    if (!this.cachedNative) {
+      this.cachedNative = this.injectedNative ?? getSeaNative();
+    }
+    return this.cachedNative;
   }
 
   public async connect(options: ConnectionOptions): Promise<void> {
-    // Validate PAT auth + capture the napi-binding option shape.
-    // Any non-PAT mode (or a missing token) throws here, before we ever
-    // touch the native binding.
+    // Validate PAT auth + capture the napi-binding option shape. Any
+    // non-PAT mode (or a missing token) throws here, before we ever touch
+    // the native binding. NOTE: unlike Thrift, this performs no network
+    // round-trip — the session is opened lazily in openSession(), so a
+    // resolved connect() does not by itself prove the endpoint is
+    // reachable or the credential is valid.
     this.nativeOptions = buildSeaConnectionOptions(options);
   }
 
@@ -159,7 +184,9 @@ export default class SeaBackend implements IBackend {
       throw new HiveDriverError('SeaBackend: connect() must be called before openSession().');
     }
     const connection = (await this.native.openSession(this.nativeOptions)) as NativeConnection;
-    return new SeaSessionBackend(connection);
+    const session = new SeaSessionBackend(connection, this.logger);
+    this.logger?.log(LogLevel.info, `SEA session opened with id: ${session.id}`);
+    return session;
   }
 
   public async close(): Promise<void> {
diff --git a/lib/utils/prependSlash.ts b/lib/utils/prependSlash.ts
@@ -0,0 +1,25 @@
+// Copyright (c) 2026 Databricks, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/**
+ * Normalise an HTTP path to a leading-slash form. Empty strings are left
+ * untouched. Shared by the Thrift connect path (`DBSQLClient`) and the
+ * SEA auth adapter (`SeaAuth`) so the two can't drift.
+ */
+export default function prependSlash(str: string): string {
+  if (str.length > 0 && str.charAt(0) !== '/') {
+    return `/${str}`;
+  }
+  return str;
+}
diff --git a/tests/e2e/sea/auth-pat-e2e.test.ts b/tests/e2e/sea/auth-pat-e2e.test.ts
@@ -14,6 +14,8 @@
 
 import { expect } from 'chai';
 import { DBSQLClient } from '../../../lib';
+import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient';
+import { InternalConnectionOptions } from '../../../lib/contracts/InternalConnectionOptions';
 
 /**
  * sea-auth M0 end-to-end:
@@ -58,8 +60,11 @@ describe('sea-auth e2e — PAT through DBSQLClient ↔ SeaBackend ↔ napi bindi
       host: host as string,
       path: path as string,
       token: token as string,
+      // `useSEA` is an internal opt-in (InternalConnectionOptions), not a
+      // public ConnectionOptions field — cast exactly as DBSQLClient.connect
+      // does internally so the literal passes excess-property checking.
       useSEA: true,
-    });
+    } as ConnectionOptions & InternalConnectionOptions);
     expect(connected).to.equal(client);
 
     const session = await client.openSession();
diff --git a/tests/integration/.mocharc.js b/tests/integration/.mocharc.js
diff --git a/tests/unit/sea/auth-pat.test.ts b/tests/unit/sea/auth-pat.test.ts
@@ -29,6 +29,9 @@ function makeFakeBinding() {
   const calls: Array<{ method: string; args: unknown[] }> = [];
 
   const fakeConnection = {
+    // Mirrors the kernel `Connection.sessionId` getter; SeaSessionBackend
+    // surfaces this as its `id`.
+    sessionId: '01ef-fake-session-id',
     async executeStatement() {
       throw new Error('not used in this test');
     },
@@ -43,10 +46,17 @@ function makeFakeBinding() {
     },
     async openSession(opts: { hostName: string; httpPath: string; token: string }) {
       calls.push({ method: 'openSession', args: [opts] });
-      return fakeConnection as unknown;
+      // Cast through the binding's own member types: `SeaNativeBinding` is
+      // `typeof import('../../native/sea')`, so `openSession`'s resolved
+      // return type is the napi `Connection`. A bare `as unknown` stops
+      // short of that and fails to satisfy the annotation.
+      return fakeConnection as unknown as Awaited<ReturnType<SeaNativeBinding['openSession']>>;
     },
-    Connection: function FakeConnection() {} as unknown as Function,
-    Statement: function FakeStatement() {} as unknown as Function,
+    // `Connection`/`Statement` are exported as type aliases in
+    // SeaNativeLoader, so `typeof Connection` is illegal (TS2693); index
+    // the binding type instead to get the napi class constructor type.
+    Connection: function FakeConnection() {} as unknown as SeaNativeBinding['Connection'],
+    Statement: function FakeStatement() {} as unknown as SeaNativeBinding['Statement'],
   };
 
   return { binding, calls };
@@ -167,7 +177,8 @@ describe('SeaAuth + SeaBackend — PAT auth flow', () => {
 
       const session = await backend.openSession({});
       expect(session).to.exist;
-      expect(session.id).to.match(/^sea-session-\d+$/);
+      // id is the real server-issued session id (kernel `sessionId`).
+      expect(session.id).to.equal('01ef-fake-session-id');
 
       expect(calls).to.have.lengthOf(1);
       expect(calls[0].method).to.equal('openSession');
