Merge branch 'main' into fix/hardcoded-snowflake-dialect

Author: anandgupta42

packages/drivers/src/index.ts

@@ -2,7 +2,7 @@
 export type { Connector, ConnectorResult, SchemaColumn, ConnectionConfig } from "./types"
 
 // Re-export config normalization
-export { normalizeConfig } from "./normalize"
+export { normalizeConfig, sanitizeConnectionString } from "./normalize"
 
 // Re-export driver connect functions
 export { connect as connectPostgres } from "./postgres"

packages/drivers/src/normalize.ts

@@ -111,6 +111,95 @@ const DRIVER_ALIASES: Record<string, AliasMap> = {
   // duckdb and sqlite have simple configs — no aliases needed
 }
 
+// ---------------------------------------------------------------------------
+// Connection string password encoding
+// ---------------------------------------------------------------------------
+
+/**
+ * URI-style connection strings (postgres://, mongodb://, etc.) embed
+ * credentials in the userinfo section: `scheme://user:password@host/db`.
+ * If the password contains special characters (@, #, :, /, ?, etc.) and
+ * they are NOT percent-encoded, drivers will mis-parse the URI and fail
+ * with cryptic auth errors.
+ *
+ * This function detects an unencoded password in the userinfo portion and
+ * re-encodes it so the connection string is valid.  Already-encoded
+ * passwords (containing %XX sequences) are left untouched.
+ */
+export function sanitizeConnectionString(connectionString: string): string {
+  // Only touch scheme://... URIs.
+  const schemeMatch = connectionString.match(/^[a-zA-Z][a-zA-Z0-9+.-]*:\/\//)
+  if (!schemeMatch) return connectionString
+
+  const scheme = schemeMatch[0]
+  const afterScheme = connectionString.slice(scheme.length)
+
+  // Find the userinfo/host separator. A password can legitimately contain
+  // '@', '#', '/', '?', or ':' characters, so the URI spec is ambiguous
+  // when those are unencoded. We use the LAST '@' as the separator because
+  // that correctly handles the common case of passwords with special
+  // characters (the stated purpose of this function — see issue #589).
+  //
+  // The known trade-off: if the query string or path also contains an
+  // unencoded '@' (e.g. `?email=alice@example.com`), the rightmost '@'
+  // is NOT the userinfo separator. We detect this ambiguous case below
+  // with a guard and bail to avoid corrupting the URI.
+  const lastAt = afterScheme.lastIndexOf("@")
+  if (lastAt < 0) return connectionString // No userinfo — nothing to fix
+
+  const beforeAt = afterScheme.slice(0, lastAt)
+  const afterAt = afterScheme.slice(lastAt + 1)
+
+  // Ambiguity guard: if the content AFTER the '@' has no path/query/
+  // fragment delimiter ('/', '?', or '#') but the content BEFORE the
+  // '@' does, then the '@' is almost certainly inside a path, query,
+  // or fragment — not the userinfo separator. Bail and leave the URI
+  // untouched so the caller can pre-encode explicitly.
+  //
+  // Examples that trigger the guard (correctly left alone):
+  //   postgresql://u:p@host/db?email=alice@example.com   ('@' in query)
+  //   postgresql://u:p@host:5432/db@archive              ('@' in path)
+  //   postgresql://u:p@host/db#at@frag                   ('@' in fragment)
+  //
+  // Examples that pass the guard (correctly encoded):
+  //   postgresql://u:p@ss@host/db            (last '@' has '/' after)
+  //   postgresql://u:f@ke#PLACEHOLDER@host/db (last '@' has '/' after)
+  const afterAtHasDelim = /[/?#]/.test(afterAt)
+  const beforeAtHasDelim = /[/?#]/.test(beforeAt)
+  if (!afterAtHasDelim && beforeAtHasDelim) {
+    return connectionString
+  }
+
+  // Idempotent re-encoding: decode any existing percent-escapes and
+  // re-encode. Already-encoded values round-trip unchanged; raw values
+  // with special characters get encoded. Malformed percent sequences
+  // fall back to encoding the raw input.
+  const encodeIdempotent = (v: string): string => {
+    if (v.length === 0) return v
+    try {
+      return encodeURIComponent(decodeURIComponent(v))
+    } catch {
+      return encodeURIComponent(v)
+    }
+  }
+
+  // Split userinfo on the FIRST ':' only (password may contain ':').
+  const colonIdx = beforeAt.indexOf(":")
+  let encodedUserinfo: string
+  if (colonIdx < 0) {
+    // Username-only userinfo: still encode if it contains special chars
+    // (e.g. email addresses used as usernames).
+    encodedUserinfo = encodeIdempotent(beforeAt)
+  } else {
+    const user = beforeAt.slice(0, colonIdx)
+    const password = beforeAt.slice(colonIdx + 1)
+    encodedUserinfo = `${encodeIdempotent(user)}:${encodeIdempotent(password)}`
+  }
+
+  const rebuilt = `${scheme}${encodedUserinfo}@${afterAt}`
+  return rebuilt === connectionString ? connectionString : rebuilt
+}
+
 // ---------------------------------------------------------------------------
 // Core logic
 // ---------------------------------------------------------------------------
@@ -178,6 +267,15 @@ export function normalizeConfig(config: ConnectionConfig): ConnectionConfig {
   const aliases = DRIVER_ALIASES[type]
   let result = aliases ? applyAliases(config, aliases) : { ...config }
 
+  // Sanitize connection_string: if the password contains special characters
+  // (@, #, :, /, etc.) that are not percent-encoded, URI-based drivers will
+  // mis-parse the string and fail with cryptic auth errors.  This is the
+  // single integration point — every caller of normalizeConfig() gets the
+  // fix automatically.
+  if (typeof result.connection_string === "string") {
+    result.connection_string = sanitizeConnectionString(result.connection_string)
+  }
+
   // Type-specific post-processing
   // Note: MySQL SSL fields (ssl_ca, ssl_cert, ssl_key) are NOT constructed
   // into an ssl object here. They stay as top-level fields so the credential

packages/drivers/test/clickhouse-unit.test.ts

@@ -203,6 +203,38 @@ describe("ClickHouse driver unit tests", () => {
       await connector.execute("SELECT 'it''s -- LIMIT 5' FROM t", 10)
       expect(mockQueryCalls[0].query).toContain("LIMIT 11")
     })
+
+    test("leading block comment does NOT bypass LIMIT injection", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("/* analytics query */ SELECT * FROM t", 10)
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("multi-line block comment before SELECT does NOT bypass LIMIT", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("/*\n * Report query\n * Author: test\n */\nSELECT * FROM t", 10)
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("backslash-escaped string does NOT break LIMIT check", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("SELECT 'path\\\\to\\'s -- LIMIT 5' FROM t", 10)
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("leading comment does NOT misroute SELECT as DDL", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("-- INSERT note\nSELECT * FROM t", 10)
+      expect(mockQueryCalls.length).toBe(1)
+      expect(mockCommandCalls.length).toBe(0)
+    })
+
+    test("block comment containing DDL keyword does NOT misroute SELECT", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("/* DROP TABLE note */ SELECT * FROM t", 10)
+      expect(mockQueryCalls.length).toBe(1)
+      expect(mockCommandCalls.length).toBe(0)
+    })
   })
 
   // --- Truncation detection ---
@@ -285,6 +317,31 @@ describe("ClickHouse driver unit tests", () => {
       const cols = await connector.describeTable("default", "t")
       expect(cols[0].nullable).toBe(false)
     })
+
+    test("LowCardinality(Nullable(FixedString(32))) IS nullable — nested inner type", async () => {
+      mockQueryResult = [{ name: "col1", type: "LowCardinality(Nullable(FixedString(32)))" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(true)
+    })
+
+    test("Map(String, Nullable(UInt32)) is NOT nullable at column level", async () => {
+      // The map values are nullable, but the column itself is not
+      mockQueryResult = [{ name: "col1", type: "Map(String, Nullable(UInt32))" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
+
+    test("Tuple(Nullable(String), UInt32) is NOT nullable at column level", async () => {
+      mockQueryResult = [{ name: "col1", type: "Tuple(Nullable(String), UInt32)" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
+
+    test("handles empty/missing type gracefully", async () => {
+      mockQueryResult = [{ name: "col1", type: undefined }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
   })
 
   // --- Connection guard ---