fix: block SQL Server pass-through data sources in read-only mode

src/connectors/sqlserver/index.ts

@@ -20,6 +20,8 @@ import { stripCommentsAndStrings } from "../../utils/sql-parser.js";
 import {
   sqlServerDynamicSqlKeywords,
   sqlServerDynamicSqlPattern,
+  sqlServerPassThroughKeywords,
+  sqlServerPassThroughPattern,
 } from "../../utils/allowed-keywords.js";
 import { closeQuietly } from "../../utils/resource-cleanup.js";
 
@@ -634,7 +636,7 @@ export class SQLServerConnector implements Connector {
       sqlQuery.match(SQLServerConnector.LEADING_NOISE)![0].length
     );
     if (/^explain\b/i.test(afterNoise)) {
-      return this.explainQuery(afterNoise.slice("explain".length).trim());
+      return this.explainQuery(afterNoise.slice("explain".length).trim(), options.readonly);
     }
 
     try {
@@ -718,33 +720,33 @@ export class SQLServerConnector implements Connector {
   }
 
   /**
-   * Execute a query inside a transaction that always rolls back, preventing
-   * any modifications from persisting. SQL Server has no native READ ONLY
-   * transaction mode, so this is the defense-in-depth backstop behind the
-   * keyword classifier.
+   * Reject the constructs that escape SQL Server's read-only guards, for use by
+   * both read-only execution paths.
    *
-   * Because the rollback guard is application-level (not engine-enforced),
-   * we reject dangerous keywords in the stripped SQL before opening the
-   * transaction:
-   * - COMMIT/ROLLBACK: would end the outer transaction, letting writes persist
    * - Dynamic SQL (sqlServerDynamicSqlKeywords): can carry hidden COMMIT/ROLLBACK
    *   inside string literals that stripCommentsAndStrings removes
+   * - Pass-through data sources (sqlServerPassThroughKeywords): execute on a
+   *   remote or ad-hoc source, so a local rollback never reaches them
+   * - COMMIT/ROLLBACK, when `transactionControl` is set: would end the wrapping
+   *   transaction, letting writes persist. Only meaningful for the transaction
+   *   path; the EXPLAIN path opens no transaction of its own.
    *
-   * The dynamic SQL keywords are imported from the read-only classifier rather
-   * than redeclared, so the classifier and this backstop cannot drift apart.
+   * Both keyword lists are imported from the read-only classifier rather than
+   * redeclared, so the classifier and these backstops cannot drift apart.
    *
    * Note the COMMIT/ROLLBACK check is SQL Server-only by design. MySQL/MariaDB
    * wrap batches in a transaction too, but there `commit`, `prepare` and
    * `execute` are absent from their allow-lists in allowedKeywords, and
    * execute-sql.ts requires every split statement to pass the classifier — so a
    * transaction-control statement can never reach their backstop.
    */
-  private async executeReadOnly(
-    processedSQL: string,
-    parameters?: any[],
-  ): Promise<SQLResult> {
-    const cleaned = stripCommentsAndStrings(processedSQL, "sqlserver").toLowerCase();
-    if (/\b(?:commit|rollback)\b/.test(cleaned)) {
+  private assertNoReadOnlyEscapes(
+    sqlText: string,
+    { transactionControl = false }: { transactionControl?: boolean } = {},
+  ): void {
+    const cleaned = stripCommentsAndStrings(sqlText, "sqlserver").toLowerCase();
+
+    if (transactionControl && /\b(?:commit|rollback)\b/.test(cleaned)) {
       throw new Error(
         "Read-only mode: transaction control statements (COMMIT, ROLLBACK) are not allowed",
       );
@@ -756,6 +758,29 @@ export class SQLServerConnector implements Connector {
           .join(", ")}) is not allowed`,
       );
     }
+    if (sqlServerPassThroughPattern.test(cleaned)) {
+      throw new Error(
+        `Read-only mode: pass-through data sources (${sqlServerPassThroughKeywords
+          .map((k) => k.toUpperCase())
+          .join(", ")}) are not allowed`,
+      );
+    }
+  }
+
+  /**
+   * Execute a query inside a transaction that always rolls back, preventing
+   * any modifications from persisting. SQL Server has no native READ ONLY
+   * transaction mode, so this is the defense-in-depth backstop behind the
+   * keyword classifier.
+   *
+   * Dangerous constructs are rejected before the transaction opens; see
+   * assertNoReadOnlyEscapes.
+   */
+  private async executeReadOnly(
+    processedSQL: string,
+    parameters?: any[],
+  ): Promise<SQLResult> {
+    this.assertNoReadOnlyEscapes(processedSQL, { transactionControl: true });
 
     const transaction = new sql.Transaction(this.connection!);
     await transaction.begin();
@@ -841,14 +866,23 @@ export class SQLServerConnector implements Connector {
    * off the shared pool, so a concurrent query can never land on a connection
    * with SHOWPLAN enabled (which would return a plan instead of its results).
    */
-  private async explainQuery(innerQuery: string): Promise<SQLResult> {
+  private async explainQuery(innerQuery: string, readonly?: boolean): Promise<SQLResult> {
     // Validate against comment/string-stripped SQL so comment-only input counts
     // as empty and a SET SHOWPLAN can't hide behind comments.
     const cleaned = stripCommentsAndStrings(innerQuery, "sqlserver").trim();
     if (!cleaned) {
       throw new Error("EXPLAIN requires a statement to analyze");
     }
 
+    // EXPLAIN is routed here before the read-only branch in executeSQL, so it
+    // opens no rolling-back transaction. SHOWPLAN_XML compiles without
+    // executing, but that single session toggle would otherwise be the whole
+    // guarantee — apply the same escape checks as executeReadOnly. Skipped
+    // outside read-only mode, where explaining an EXEC is legitimate.
+    if (readonly) {
+      this.assertNoReadOnlyEscapes(innerQuery);
+    }
+
     // Defense in depth: the SET SHOWPLAN session toggle is what makes EXPLAIN
     // non-executing, so the explained statement must not disable it. SQL Server
     // already rejects `SET SHOWPLAN_* OFF` alongside other statements in a

src/utils/__tests__/allowed-keywords.test.ts

@@ -284,6 +284,64 @@ describe("isReadOnlySQL", () => {
     });
   });
 
+  describe("SQL Server pass-through data source bypass prevention", () => {
+    it("should reject OPENQUERY, whose payload runs on the remote server", () => {
+      expect(
+        isReadOnlySQL("SELECT * FROM OPENQUERY(linked_srv, 'DELETE FROM customers')", "sqlserver")
+      ).toBe(false);
+    });
+
+    it("should reject OPENROWSET bulk file reads", () => {
+      expect(
+        isReadOnlySQL("SELECT * FROM OPENROWSET(BULK N'C:\\secrets\\config.ini', SINGLE_CLOB) AS x", "sqlserver")
+      ).toBe(false);
+    });
+
+    it("should reject OPENDATASOURCE ad-hoc connections", () => {
+      expect(
+        isReadOnlySQL(
+          "SELECT * FROM OPENDATASOURCE('SQLNCLI', 'Server=evil;Trusted_Connection=yes').db.dbo.t",
+          "sqlserver"
+        )
+      ).toBe(false);
+    });
+
+    it("should reject OPENQUERY nested inside a CTE", () => {
+      expect(
+        isReadOnlySQL(
+          "WITH cte AS (SELECT * FROM OPENQUERY(srv, 'DROP TABLE t')) SELECT * FROM cte",
+          "sqlserver"
+        )
+      ).toBe(false);
+    });
+
+    it("should reject OPENQUERY after multi-statement split", () => {
+      expect(
+        areAllStatementsReadOnly("SELECT 1; SELECT * FROM OPENQUERY(srv, 'DELETE FROM t')", "sqlserver")
+      ).toBe(false);
+    });
+
+    it("should not reject the word openquery inside a string literal", () => {
+      expect(isReadOnlySQL("SELECT * FROM logs WHERE note = 'openquery(x)'", "sqlserver")).toBe(true);
+    });
+
+    it("should not reject the word openrowset in a comment", () => {
+      expect(isReadOnlySQL("/* OPENROWSET(BULK 'x') */ SELECT 1", "sqlserver")).toBe(true);
+    });
+
+    it("should not reject identifiers that merely start with a pass-through name", () => {
+      expect(isReadOnlySQL("SELECT openquery_audit FROM logs", "sqlserver")).toBe(true);
+    });
+
+    it("should not reject a bare column named openquery (no call syntax)", () => {
+      expect(isReadOnlySQL("SELECT openquery FROM logs", "sqlserver")).toBe(true);
+    });
+
+    it("should leave other dialects unaffected", () => {
+      expect(isReadOnlySQL("SELECT * FROM openquery(a, b)", "postgres")).toBe(true);
+    });
+  });
+
   describe("edge cases", () => {
     it("should treat empty SQL after comment stripping as not read-only", () => {
       expect(isReadOnlySQL("-- just a comment", "postgres")).toBe(false);

src/utils/allowed-keywords.ts

@@ -77,6 +77,38 @@ export const sqlServerDynamicSqlPattern = new RegExp(
   "i",
 );
 
+/**
+ * T-SQL pass-through / ad-hoc data source table functions. Unlike the dynamic
+ * SQL primitives above, these appear as ordinary table sources inside a plain
+ * SELECT, so both read-only layers miss them:
+ * - The classifier cannot see the payload, because OPENQUERY carries it as a
+ *   string literal that stripCommentsAndStrings deliberately removes.
+ * - The rollback backstop cannot contain it, because the work executes on the
+ *   remote/ad-hoc source rather than in the local transaction.
+ *
+ * OPENROWSET additionally reads server-side files in its BULK form
+ * (`OPENROWSET(BULK N'C:\...', SINGLE_CLOB)`), which exfiltrates data without
+ * writing anything.
+ *
+ * Shared with executeReadOnly in src/connectors/sqlserver/index.ts on the same
+ * single-source-of-truth basis as sqlServerDynamicSqlKeywords.
+ */
+export const sqlServerPassThroughKeywords = [
+  "openquery",
+  "openrowset",
+  "opendatasource",
+] as const;
+
+/**
+ * Matches a pass-through data source in call position. The trailing `\s*\(` is
+ * required so an ordinary column named `openquery` still classifies as
+ * read-only — only the invocation form is a bypass.
+ */
+export const sqlServerPassThroughPattern = new RegExp(
+  `\\b(?:${sqlServerPassThroughKeywords.join("|")})\\s*\\(`,
+  "i",
+);
+
 /**
  * Extended pattern for SQL Server: base mutating keywords plus the dynamic SQL
  * primitives above.
@@ -147,6 +179,13 @@ function checkReadOnly(cleanedSQL: string, connectorType: ConnectorType | string
     return false;
   }
 
+  // SQL Server pass-through data sources escape both read-only layers, so they
+  // are rejected wherever they appear — not just under WITH, since the common
+  // form is a plain `SELECT ... FROM OPENQUERY(...)`.
+  if (connectorType === "sqlserver" && sqlServerPassThroughPattern.test(cleanedSQL)) {
+    return false;
+  }
+
   // WITH statements can embed DML in CTEs (e.g. WITH cte AS (UPDATE ...))
   if (firstWord === "with") {
     const pattern = mutatingPatterns[connectorType as ConnectorType] ?? mutatingPattern;