Add table names for enforcedWhereClause columns

Author: matt-aitken

internal-packages/tsql/src/query/printer.ts

@@ -1664,12 +1664,21 @@ export class ClickHousePrinter {
    *
    * @param column - The column name
    * @param condition - The condition to apply
+   * @param tableAlias - Optional table alias to qualify the column reference.
+   *                     When provided, constructs the field chain as [tableAlias, column]
+   *                     so resolveFieldChain will resolve to the correct table in multi-join queries.
    * @returns The AST expression for the condition
    */
-  private createConditionExpression(column: string, condition: WhereClauseCondition): Expression {
+  private createConditionExpression(
+    column: string,
+    condition: WhereClauseCondition,
+    tableAlias?: string
+  ): Expression {
+    // When tableAlias is provided, qualify the field chain to ensure it binds
+    // to the correct table in multi-join queries
     const fieldExpr: Field = {
       expression_type: "field",
-      chain: [column],
+      chain: tableAlias ? [tableAlias, column] : [column],
     };
 
     if (condition.op === "between") {
@@ -1705,10 +1714,11 @@ export class ClickHousePrinter {
    *
    * This ensures the same enforcedWhereClause can be used across different tables.
    *
-   * Note: We use just the column name without table prefix since ClickHouse
-   * requires the actual table name (task_runs_v2), not the TSQL alias (task_runs)
+   * All guard expressions are qualified with the table alias to ensure they bind
+   * to the correct table in multi-join queries, preventing potential security
+   * issues where an unqualified column reference could bind to the wrong table.
    */
-  private createEnforcedGuard(tableSchema: TableSchema, _tableAlias: string): Expression | null {
+  private createEnforcedGuard(tableSchema: TableSchema, tableAlias: string): Expression | null {
     const { requiredFilters, tenantColumns } = tableSchema;
     const guards: Expression[] = [];
 
@@ -1721,24 +1731,26 @@ export class ClickHousePrinter {
     }
 
     // Apply all enforced conditions for columns that exist in this table
+    // Pass tableAlias to ensure guards are qualified and bind to the correct table
     for (const [column, condition] of Object.entries(this.context.enforcedWhereClause)) {
       // Skip undefined/null conditions (allows conditional inclusion like project_id?: condition)
       if (condition === undefined || condition === null) {
         continue;
       }
       // Only apply if column exists in this table's schema or is a tenant column
       if (validColumns.has(column)) {
-        guards.push(this.createConditionExpression(column, condition));
+        guards.push(this.createConditionExpression(column, condition, tableAlias));
       }
     }
 
     // Add required filters from the table schema (e.g., engine = 'V2')
+    // Also qualified with table alias to ensure correct binding in multi-join queries
     if (requiredFilters && requiredFilters.length > 0) {
       for (const filter of requiredFilters) {
         const filterGuard: CompareOperation = {
           expression_type: "compare_operation",
           op: CompareOperationOp.Eq,
-          left: { expression_type: "field", chain: [filter.column] } as Field,
+          left: { expression_type: "field", chain: [tableAlias, filter.column] } as Field,
           right: { expression_type: "constant", value: filter.value } as Constant,
         };
         guards.push(filterGuard);
@@ -2692,6 +2704,31 @@ export class ClickHousePrinter {
       return columnSchema.clickhouseName || columnSchema.name;
     }
 
+    // Check if this is a tenant column that's not exposed in the schema's columns
+    // These are internal columns used for tenant isolation guards
+    const { tenantColumns, requiredFilters } = tableSchema;
+    if (tenantColumns) {
+      if (
+        columnName === tenantColumns.organizationId ||
+        columnName === tenantColumns.projectId ||
+        columnName === tenantColumns.environmentId
+      ) {
+        // Tenant columns are already ClickHouse column names, return as-is
+        return columnName;
+      }
+    }
+
+    // Check if this is a required filter column (e.g., engine = 'V2')
+    // These are internal columns used for enforced filters
+    if (requiredFilters) {
+      for (const filter of requiredFilters) {
+        if (columnName === filter.column) {
+          // Required filter columns are already ClickHouse column names, return as-is
+          return columnName;
+        }
+      }
+    }
+
     // Column not in schema - this is a security issue, block access
     // Check if the user typed a ClickHouse column name instead of the TSQL name
     for (const [tsqlName, colSchema] of Object.entries(tableSchema.columns)) {

internal-packages/tsql/src/query/security.test.ts

@@ -590,6 +590,95 @@ describe("Optional Tenant Filters", () => {
   });
 });
 
+describe("Multi-join Tenant Guard Qualification", () => {
+  /**
+   * Security Test: Verifies that tenant guards are properly table-qualified in multi-join queries.
+   *
+   * The bug: createEnforcedGuard was building unqualified guard expressions like:
+   *   organization_id = 'org_tenant1'
+   *
+   * In a multi-table join where both tables have the same column (organization_id),
+   * an unqualified reference could potentially bind to the wrong table during resolution,
+   * or be ambiguous. The guards should be qualified like:
+   *   r.organization_id = 'org_tenant1' AND e.organization_id = 'org_tenant1'
+   *
+   * This ensures each table's guard binds to the correct table, not just any matching column.
+   */
+  it("should qualify tenant guards with table alias in JOIN queries", () => {
+    const { sql } = compile(`
+      SELECT r.id, e.event_type 
+      FROM task_runs r 
+      JOIN task_events e ON r.id = e.run_id
+    `);
+
+    // The guards should be table-qualified to prevent binding to the wrong table
+    // Look for pattern like: r.organization_id and e.organization_id (with table alias prefix)
+    // The exact format in ClickHouse SQL is just "alias.column" after resolution
+    
+    // Count qualified organization_id references (should have table prefixes)
+    // In the WHERE clause, we should see both r.organization_id and e.organization_id
+    const whereClause = sql.substring(sql.indexOf("WHERE"));
+    
+    // Both tables should have their own qualified tenant guards
+    // The pattern should be: table_alias.organization_id for each table
+    expect(whereClause).toMatch(/\br\b[^,]*organization_id/);
+    expect(whereClause).toMatch(/\be\b[^,]*organization_id/);
+  });
+
+  it("should qualify tenant guards with table alias in LEFT JOIN queries", () => {
+    const { sql } = compile(`
+      SELECT r.id, e.event_type 
+      FROM task_runs r 
+      LEFT JOIN task_events e ON r.id = e.run_id
+    `);
+
+    const whereClause = sql.substring(sql.indexOf("WHERE"));
+    
+    // Both tables should have qualified guards
+    expect(whereClause).toMatch(/\br\b[^,]*organization_id/);
+    expect(whereClause).toMatch(/\be\b[^,]*organization_id/);
+  });
+
+  it("should qualify tenant guards in multi-way JOIN queries", () => {
+    const { sql } = compile(`
+      SELECT r.id, e1.event_type, e2.event_type
+      FROM task_runs r 
+      JOIN task_events e1 ON r.id = e1.run_id
+      JOIN task_events e2 ON r.id = e2.run_id
+    `);
+
+    const whereClause = sql.substring(sql.indexOf("WHERE"));
+    
+    // All three table aliases should have qualified guards
+    expect(whereClause).toMatch(/\br\b[^,]*organization_id/);
+    expect(whereClause).toMatch(/\be1\b[^,]*organization_id/);
+    expect(whereClause).toMatch(/\be2\b[^,]*organization_id/);
+  });
+
+  it("should ensure guards cannot bind to wrong table by verifying separate qualifications", () => {
+    const { sql, params } = compile(`
+      SELECT r.id, e.event_type 
+      FROM task_runs r 
+      JOIN task_events e ON r.id = e.run_id
+      WHERE r.status = 'completed'
+    `);
+
+    // Count organization_id occurrences with different table prefixes
+    // This ensures each table gets its own guard, not shared/ambiguous references
+    const orgIdPattern = /(\w+)\.organization_id/g;
+    const matches = [...sql.matchAll(orgIdPattern)];
+    const tableAliases = matches.map(m => m[1]);
+    
+    // Should have at least 2 different table aliases for organization_id
+    // (one for task_runs alias 'r' and one for task_events alias 'e')
+    expect(tableAliases).toContain("r");
+    expect(tableAliases).toContain("e");
+    
+    // Both should use the same tenant value (parameterized)
+    expect(Object.values(params)).toContain("org_tenant1");
+  });
+});
+
 describe("Edge Cases", () => {
   it("should handle empty string values", () => {
     const { params } = compile("SELECT * FROM task_runs WHERE status = ''");