sea-auth-u2m: round-4 fixup — restore M2M-with-bad-secret class, strip envelope sentinel, trim RetryError doc

- NF3-2 (HIGH): when oauthClientId is set and oauthClientSecret is
  blank/reserved, raise AuthenticationError (M2M intent) instead of
  routing to U2M which then raises HiveDriverError. The round-3
  NF-N3 fix over-applied — U2M routing only kicks in when BOTH
  id and secret are blank/absent.
- NF3-3 (MEDIUM): on corrupted-envelope JSON.parse failure, strip
  the internal __databricks_error__: sentinel from the message
  before returning to the caller.
- NF3-6 (LOW): trim RetryError mention from KernelMetadata.errorCode
  doc-comments; no kernel ErrorCode currently maps to RetryError.

Deferred per team-lead disposition: NF3-1 (kernel RequestTokenError
sub-classification — Phase 2 kernel work), NF3-4 (e2e
kernel-error-code assertion — blocked on NF3-1), NF3-5 (path-dep
checksum — resolves when kernel publishes), NF3-7 (looksReal
double-neg — cosmetic), LE3-1..7 (Phase 2 decoder polish).

Co-authored-by: Isaac

Author: msrathore-db

lib/sea/SeaAuth.ts

@@ -182,20 +182,26 @@ export function buildSeaConnectionOptions(options: ConnectionOptions): SeaNative
     }
 
     // Flow selector mirrors thrift's `DBSQLClient.createAuthProvider`
-    // (`DBSQLClient.ts:143`): `oauthClientSecret defined ? M2M : U2M`.
-    // Blank or buggy-shell-export secrets route to U2M (rather than
-    // M2M-with-bad-secret) so the error message correctly points the user
-    // at the right flow. `oauthClientSecret: process.env.MY_SECRET || ''`
-    // is a common shape; routing it to the M2M arm would surface an
-    // M2M-specific error that never mentions U2M.
+    // (`DBSQLClient.ts:143`): presence of `oauthClientId` indicates M2M
+    // intent, otherwise U2M. Routing decision is based on `oauthClientId`
+    // (the "do I have an id?" signal) rather than the secret, so a
+    // user who set an id but typoed/forgot the secret gets the M2M
+    // "secret is required" error instead of a U2M error that hides
+    // their actual intent. The U2M arm still defends against an id
+    // sneaking through (e.g. caller bypasses shape inference).
+    const idIsBlank =
+      oauth.oauthClientId === undefined ||
+      (typeof oauth.oauthClientId === 'string' && isBlankOrReserved(oauth.oauthClientId));
     const secretIsBlank =
-      typeof oauth.oauthClientSecret === 'string' && isBlankOrReserved(oauth.oauthClientSecret);
-    if (oauth.oauthClientSecret === undefined || secretIsBlank) {
-      // U2M.
+      oauth.oauthClientSecret === undefined ||
+      (typeof oauth.oauthClientSecret === 'string' && isBlankOrReserved(oauth.oauthClientSecret));
+
+    if (idIsBlank && secretIsBlank) {
+      // U2M — neither id nor secret supplied.
       if (oauth.oauthClientId !== undefined) {
+        // Defense-in-depth: id was set but blank/reserved literal.
         // The kernel hardcodes `client_id = "databricks-cli"` for U2M;
-        // there's no JS-side override knob. Silently dropping a
-        // user-supplied id would hide that the kernel ignored it.
+        // there's no JS-side override knob.
         throw new HiveDriverError(
           'SEA backend: `oauthClientId` is not supported on the OAuth U2M flow; ' +
             "the kernel uses the built-in 'databricks-cli' client. " +

lib/sea/SeaErrorMapping.ts

@@ -56,12 +56,12 @@ export type KernelErrorCode =
  * `__databricks_error__:` envelope (per `native/sea/src/error.rs:50-89`).
  *
  * `errorCode` is namespaced under `kernelMetadata` rather than placed at
- * the top level because two existing JS-side error classes
- * (`OperationStateError`, `RetryError`) already declare a top-level
+ * the top level because `OperationStateError` already declares a top-level
  * `errorCode: enum` field, and `DBSQLOperation.ts:209` switches on it
  * (`err.errorCode === OperationStateErrorCode.Canceled`). Top-level
  * defineProperty would clobber that enum with a kernel string and break
- * cancel/close detection.
+ * cancel/close detection. (`RetryError.errorCode` is the same shape and
+ * is reserved here for future kernel→`RetryError` mappings.)
  */
 export interface KernelMetadata {
   errorCode?: string;
@@ -76,7 +76,7 @@ export interface KernelMetadata {
  * exposed at the top level (no collision in the existing driver error
  * tree); the remaining envelope fields live under a `kernelMetadata`
  * namespace to avoid clobbering pre-existing `errorCode` semantics on
- * `OperationStateError` / `RetryError`.
+ * `OperationStateError` (and, reserved for future use, `RetryError`).
  */
 export interface ErrorWithSqlState extends Error {
   sqlState?: string;
@@ -210,9 +210,10 @@ function buildKernelMetadata(parsed: Record<string, unknown>): KernelMetadata {
  *    through {@link mapKernelErrorToJsError}, and attach the remaining
  *    envelope fields under a single non-enumerable `kernelMetadata`
  *    namespace. Namespacing avoids the collision with
- *    `OperationStateError.errorCode` (an enum) and `RetryError.errorCode`
- *    (an enum), each of which is already switched on at the JS layer
- *    (see `DBSQLOperation.ts:209`).
+ *    `OperationStateError.errorCode` (an enum already switched on at the
+ *    JS layer — see `DBSQLOperation.ts:209`). `RetryError.errorCode`
+ *    shares the shape and is reserved for future kernel→`RetryError`
+ *    mappings.
  *  - Binding-side error (e.g. `napi::Error::new(InvalidArg, "openSession:
  *    \`token\` is required for the requested auth mode")` produced by
  *    the binding's own validation): returned unchanged. These don't
@@ -237,8 +238,12 @@ export function decodeNapiKernelError(err: unknown): Error {
   try {
     parsed = JSON.parse(jsonStr);
   } catch {
-    // Corrupted envelope — surface the raw message rather than
-    // silently dropping the original error.
+    // Corrupted envelope — surface the raw post-sentinel payload rather
+    // than silently dropping the original error. Strip the internal
+    // `__databricks_error__:` prefix; it's a binding/JS-side framing
+    // marker, not user-actionable, and leaking it makes the message
+    // confusing to operators triaging a malformed kernel response.
+    err.message = jsonStr;
     return err;
   }
 

tests/unit/sea/auth-edge-cases.test.ts

@@ -76,13 +76,13 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => {
       }
     });
 
-    // Post round-3 NF-N3: a blank/reserved-literal `oauthClientSecret`
-    // routes the connection to the U2M arm rather than rejecting on
-    // the M2M arm. When `oauthClientId` is ALSO set, the U2M arm's
-    // dedicated "not supported on U2M" rejection fires — which is more
-    // actionable than the M2M "secret must be non-empty" message
-    // because it tells the user the U2M flow exists and how to use it.
-    it('routes mixed-case reserved-literal oauthClientSecret to U2M; rejects with U2M-id error', () => {
+    // Round-4 NF3-2: presence of `oauthClientId` signals M2M intent.
+    // A blank/reserved-literal `oauthClientSecret` is then a missing-secret
+    // typo, not a request to fall back to U2M. Surface the M2M "secret
+    // required" AuthenticationError so the user fixes the real problem
+    // rather than swap class to a HiveDriverError pointing at a flow
+    // they didn't intend to use.
+    it('rejects mixed-case reserved-literal oauthClientSecret with AuthenticationError when id is set', () => {
       const opts: ConnectionOptions = {
         host: 'example.cloud.databricks.com',
         path: '/sql/1.0/warehouses/abc',
@@ -92,8 +92,8 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => {
       };
 
       expect(() => buildSeaConnectionOptions(opts)).to.throw(
-        HiveDriverError,
-        /oauthClientId.*not supported on the OAuth U2M flow/,
+        AuthenticationError,
+        /oauthClientSecret.*non-empty.*OAuth M2M/,
       );
     });
 
@@ -112,7 +112,7 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => {
       );
     });
 
-    it('routes whitespace-only oauthClientSecret to U2M; with oauthClientId set, rejects U2M+id', () => {
+    it('rejects whitespace-only oauthClientSecret with AuthenticationError when oauthClientId is set (M2M intent)', () => {
       const opts: ConnectionOptions = {
         host: 'example.cloud.databricks.com',
         path: '/sql/1.0/warehouses/abc',
@@ -122,8 +122,8 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => {
       };
 
       expect(() => buildSeaConnectionOptions(opts)).to.throw(
-        HiveDriverError,
-        /oauthClientId.*not supported on the OAuth U2M flow/,
+        AuthenticationError,
+        /oauthClientSecret.*non-empty.*OAuth M2M/,
       );
     });
 
@@ -142,7 +142,7 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => {
       );
     });
 
-    it('routes literal "undefined" as oauthClientSecret to U2M; with oauthClientId set, rejects U2M+id', () => {
+    it('rejects literal "undefined" as oauthClientSecret with AuthenticationError when id is set (M2M intent)', () => {
       const opts: ConnectionOptions = {
         host: 'example.cloud.databricks.com',
         path: '/sql/1.0/warehouses/abc',
@@ -152,10 +152,37 @@ describe('SeaAuth — edge cases (input validation + ambiguity)', () => {
       };
 
       expect(() => buildSeaConnectionOptions(opts)).to.throw(
-        HiveDriverError,
-        /oauthClientId.*not supported on the OAuth U2M flow/,
+        AuthenticationError,
+        /oauthClientSecret.*non-empty.*OAuth M2M/,
       );
     });
+
+    // Round-4 NF3-2: pin the exact class — must be `AuthenticationError`,
+    // not the bare `HiveDriverError` superclass. The round-3 NF-N3 fix
+    // swapped this silently by routing M2M-with-empty-secret through the
+    // U2M arm, which raised a plain `HiveDriverError`. Guard against that
+    // regression by pinning the constructor name (since
+    // `AuthenticationError extends HiveDriverError`, `instanceof` alone
+    // can't distinguish the two).
+    it('M2M-with-empty-secret throws AuthenticationError, not bare HiveDriverError (class pin)', () => {
+      const opts: ConnectionOptions = {
+        host: 'example.cloud.databricks.com',
+        path: '/sql/1.0/warehouses/abc',
+        authType: 'databricks-oauth',
+        oauthClientId: 'x',
+        oauthClientSecret: '',
+      };
+
+      let caught: unknown;
+      try {
+        buildSeaConnectionOptions(opts);
+      } catch (e) {
+        caught = e;
+      }
+      expect(caught).to.be.instanceOf(AuthenticationError);
+      expect((caught as Error).constructor.name).to.equal('AuthenticationError');
+      expect((caught as Error).message).to.match(/oauthClientSecret.*non-empty.*OAuth M2M/);
+    });
   });
 
   describe('ambiguous credentials are rejected', () => {
@@ -399,7 +426,7 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => {
     expect((caught as Error).message).to.match(/`token` is required/);
   });
 
-  it('falls back to original Error for a corrupted envelope', async () => {
+  it('falls back to original Error for a corrupted envelope, stripping the internal sentinel', async () => {
     const binding = bindingRejectingWith('not valid json');
     const backend = new SeaBackend(binding);
     await backend.connect(validConnectArgs);
@@ -414,6 +441,11 @@ describe('SeaBackend — kernel error envelope decoding (DA-F1)', () => {
     // the original Error so the operator sees the raw payload.
     expect(caught).to.be.instanceOf(Error);
     expect((caught as Error).message).to.contain('not valid json');
+    // Round-4 NF3-3: the `__databricks_error__:` prefix is an internal
+    // JS<->binding framing marker; it must not leak to the user-facing
+    // message even on the corrupted-envelope fallback path.
+    expect((caught as Error).message).to.not.match(/^__databricks_error__:/);
+    expect((caught as Error).message).to.equal('not valid json');
   });
 
   // NF-4 / NF-N1: preserve the 5 optional kernel envelope fields on the

tests/unit/sea/auth-m2m.test.ts

@@ -83,7 +83,7 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => {
       );
     });
 
-    it('routes empty oauthClientSecret to the U2M arm (round-3 NF-N3), where oauthClientId being set then rejects', () => {
+    it('rejects empty oauthClientSecret with AuthenticationError when oauthClientId is set (M2M intent)', () => {
       const opts: ConnectionOptions = {
         host: 'example.cloud.databricks.com',
         path: '/sql/1.0/warehouses/abc',
@@ -92,11 +92,13 @@ describe('SeaAuth + SeaBackend — OAuth M2M auth flow', () => {
         oauthClientSecret: '',
       };
 
-      // Blank secret → U2M arm; oauthClientId set on U2M then raises
-      // the dedicated "not supported on U2M" error.
+      // Presence of `oauthClientId` signals M2M intent; an empty secret
+      // is a typo/missing-env, not a request to fall back to U2M.
+      // Surface the M2M "secret required" error so the user knows the
+      // real problem instead of getting routed to a different flow.
       expect(() => buildSeaConnectionOptions(opts)).to.throw(
-        HiveDriverError,
-        /oauthClientId.*not supported on the OAuth U2M flow/,
+        AuthenticationError,
+        /oauthClientSecret.*non-empty.*OAuth M2M/,
       );
     });
 

tests/unit/sea/auth-u2m.test.ts

@@ -16,6 +16,7 @@ import { expect } from 'chai';
 import SeaBackend from '../../../lib/sea/SeaBackend';
 import { buildSeaConnectionOptions } from '../../../lib/sea/SeaAuth';
 import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient';
+import AuthenticationError from '../../../lib/errors/AuthenticationError';
 import HiveDriverError from '../../../lib/errors/HiveDriverError';
 import { makeFakeBinding } from './_helpers/fakeBinding';
 
@@ -37,12 +38,18 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => {
       });
     });
 
-    it('rejects oauthClientId on the U2M path with a clear "not supported" error', () => {
-      // The kernel hardcodes `client_id = "databricks-cli"` for U2M; there's
-      // no JS-side override knob. Silently dropping a user-supplied id would
-      // hide that the kernel ignored it, so we surface the limitation
-      // explicitly. Earlier revisions of this code silently dropped — flipped
-      // to raise based on devils-advocate-auth-u2m-1 round-1 (NF-2).
+    it('rejects oauthClientId without oauthClientSecret as M2M-with-missing-secret', () => {
+      // Round-4 NF3-2: presence of `oauthClientId` signals M2M intent.
+      // Routing now keys off the id (the "do I have an id?" signal),
+      // not the secret. A caller who supplies id but no secret gets the
+      // M2M "secret is required" error — the actionable message for the
+      // real problem (typo'd env var, forgot to export it, etc.).
+      //
+      // The U2M arm still has a defense-in-depth rejection of a stray
+      // `oauthClientId` (the kernel hardcodes `databricks-cli` for U2M);
+      // see [NF-2 / round-1 history]. That defense fires only when
+      // BOTH id and secret are blank — the M2M arm's stricter checks
+      // catch this typical caller-error shape first.
       const opts: ConnectionOptions = {
         host: 'example.cloud.databricks.com',
         path: '/sql/1.0/warehouses/abc',
@@ -51,8 +58,8 @@ describe('SeaAuth + SeaBackend — OAuth U2M auth flow', () => {
       };
 
       expect(() => buildSeaConnectionOptions(opts)).to.throw(
-        HiveDriverError,
-        /oauthClientId.*not supported on the OAuth U2M flow/,
+        AuthenticationError,
+        /oauthClientSecret.*non-empty.*OAuth M2M/,
       );
     });