diff --git a/packages/appkit/src/connectors/lakebase-v1/client.ts b/packages/appkit/src/connectors/lakebase-v1/client.ts index ed3e2fea..b9aa3f31 100644 --- a/packages/appkit/src/connectors/lakebase-v1/client.ts +++ b/packages/appkit/src/connectors/lakebase-v1/client.ts @@ -158,6 +158,12 @@ export class LakebaseV1Connector { span.recordException(error as Error); span.setStatus({ code: SpanStatusCode.ERROR }); + logger.error( + "Query execution failed: %s (code=%s)", + error instanceof Error ? error.message : String(error), + (error as any)?.code, + ); + if (error instanceof AppKitError) { throw error; } @@ -245,6 +251,12 @@ export class LakebaseV1Connector { span.recordException(error as Error); span.setStatus({ code: SpanStatusCode.ERROR }); + logger.error( + "Transaction execution failed: %s (code=%s)", + error instanceof Error ? error.message : String(error), + (error as any)?.code, + ); + if (error instanceof AppKitError) { throw error; } diff --git a/packages/appkit/src/connectors/sql-warehouse/client.ts b/packages/appkit/src/connectors/sql-warehouse/client.ts index 4ab9344e..26bbe560 100644 --- a/packages/appkit/src/connectors/sql-warehouse/client.ts +++ b/packages/appkit/src/connectors/sql-warehouse/client.ts @@ -236,6 +236,11 @@ export class SQLWarehouseConnector { code: SpanStatusCode.ERROR, message: error instanceof Error ? error.message : String(error), }); + + logger.error( + "Statement execution failed: %s", + error instanceof Error ? error.message : String(error), + ); } if (error instanceof AppKitError) { @@ -377,6 +382,12 @@ export class SQLWarehouseConnector { message: error instanceof Error ? error.message : String(error), }); + logger.error( + "Statement polling failed for %s: %s", + statementId, + error instanceof Error ? error.message : String(error), + ); + if (error instanceof AppKitError) { throw error; } diff --git a/packages/appkit/src/connectors/tests/lakebase-v1.test.ts b/packages/appkit/src/connectors/tests/lakebase-v1.test.ts index b1b5992c..7bf89f6b 100644 --- a/packages/appkit/src/connectors/tests/lakebase-v1.test.ts +++ b/packages/appkit/src/connectors/tests/lakebase-v1.test.ts @@ -221,6 +221,37 @@ describe("LakebaseV1Connector", () => { await expect(connector.query("SELEC 1")).rejects.toThrow("Query failed"); expect(mockQuery).toHaveBeenCalledTimes(1); }); + + test("should not log the SQL query string on error", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const pgError = new Error('relation "users" does not exist') as any; + pgError.code = "42P01"; + pgError.query = "SELECT secret_token FROM users WHERE id = $1"; + pgError.parameters = ["sensitive-user-id-123"]; + + mockQuery.mockRejectedValue(pgError); + + await expect( + connector.query("SELECT secret_token FROM users WHERE id = $1", [ + "sensitive-user-id-123", + ]), + ).rejects.toThrow("Query failed"); + + const loggedOutput = errorSpy.mock.calls + .map((call) => call.join(" ")) + .join(" "); + + // Should log the error message and code (useful for debugging) + expect(loggedOutput).toContain('relation "users" does not exist'); + expect(loggedOutput).toContain("42P01"); + + // Should NOT log the raw query or parameter values + expect(loggedOutput).not.toContain("secret_token"); + expect(loggedOutput).not.toContain("sensitive-user-id-123"); + + errorSpy.mockRestore(); + }); }); describe("transaction", () => { @@ -282,6 +313,48 @@ describe("LakebaseV1Connector", () => { expect(mockClient.release).toHaveBeenCalled(); }); + + test("should not log the SQL query string on transaction error", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const pgError = new Error("duplicate key value") as any; + pgError.code = "23505"; + pgError.query = "INSERT INTO users (email, secret) VALUES ($1, $2)"; + pgError.parameters = ["user@test.com", "super-secret-value"]; + + const failingClient = { + query: vi.fn().mockImplementation((sql: string) => { + if (sql === "BEGIN" || sql === "ROLLBACK") { + return Promise.resolve({ rows: [] }); + } + return Promise.reject(pgError); + }), + release: vi.fn(), + }; + mockConnect.mockResolvedValue(failingClient); + + await expect( + connector.transaction(async (client) => { + await client.query( + "INSERT INTO users (email, secret) VALUES ($1, $2)", + ); + }), + ).rejects.toThrow(); + + const loggedOutput = errorSpy.mock.calls + .map((call) => call.join(" ")) + .join(" "); + + // Should log the error message and code + expect(loggedOutput).toContain("duplicate key value"); + expect(loggedOutput).toContain("23505"); + + // Should NOT log the raw query or parameter values + expect(loggedOutput).not.toContain("super-secret-value"); + expect(loggedOutput).not.toContain("INSERT INTO users"); + + errorSpy.mockRestore(); + }); }); describe("healthCheck", () => { diff --git a/packages/appkit/src/connectors/tests/sql-warehouse.test.ts b/packages/appkit/src/connectors/tests/sql-warehouse.test.ts new file mode 100644 index 00000000..9084ad6a --- /dev/null +++ b/packages/appkit/src/connectors/tests/sql-warehouse.test.ts @@ -0,0 +1,118 @@ +import { beforeEach, describe, expect, test, vi } from "vitest"; +import { SQLWarehouseConnector } from "../sql-warehouse"; + +// Mock telemetry to pass through span callbacks +vi.mock("../../telemetry", () => { + const mockSpan = { + end: vi.fn(), + setAttribute: vi.fn(), + setAttributes: vi.fn(), + setStatus: vi.fn(), + recordException: vi.fn(), + addEvent: vi.fn(), + isRecording: vi.fn().mockReturnValue(true), + spanContext: vi.fn(), + }; + + return { + TelemetryManager: { + getProvider: vi.fn(() => ({ + startActiveSpan: vi + .fn() + .mockImplementation(async (_name, _options, fn) => { + return await fn(mockSpan); + }), + getMeter: vi.fn().mockReturnValue({ + createCounter: vi.fn().mockReturnValue({ add: vi.fn() }), + createHistogram: vi.fn().mockReturnValue({ record: vi.fn() }), + }), + })), + }, + SpanKind: { CLIENT: 2 }, + SpanStatusCode: { OK: 1, ERROR: 2 }, + }; +}); + +describe("SQLWarehouseConnector", () => { + describe("error log redaction", () => { + let connector: SQLWarehouseConnector; + + beforeEach(() => { + vi.clearAllMocks(); + connector = new SQLWarehouseConnector({ timeout: 5000 }); + }); + + test("should not log the SQL statement on executeStatement error", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const sensitiveStatement = + "SELECT password, ssn FROM users WHERE email = 'admin@test.com'"; + + const mockWorkspaceClient = { + statementExecution: { + executeStatement: vi + .fn() + .mockRejectedValue(new Error("warehouse unavailable")), + }, + config: { host: "https://test.databricks.com" }, + }; + + await expect( + connector.executeStatement(mockWorkspaceClient as any, { + statement: sensitiveStatement, + warehouse_id: "test-warehouse", + }), + ).rejects.toThrow(); + + const loggedOutput = errorSpy.mock.calls + .map((call) => call.join(" ")) + .join(" "); + + // Should log the error message + expect(loggedOutput).toContain("warehouse unavailable"); + + // Should NOT log the SQL statement + expect(loggedOutput).not.toContain("password"); + expect(loggedOutput).not.toContain("ssn"); + expect(loggedOutput).not.toContain("admin@test.com"); + + errorSpy.mockRestore(); + }); + + test("should not log the SQL statement on polling error", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const mockWorkspaceClient = { + statementExecution: { + executeStatement: vi.fn().mockResolvedValue({ + statement_id: "stmt-123", + status: { state: "RUNNING" }, + }), + getStatement: vi.fn().mockRejectedValue(new Error("polling timeout")), + }, + config: { host: "https://test.databricks.com" }, + }; + + await expect( + connector.executeStatement(mockWorkspaceClient as any, { + statement: "SELECT secret_data FROM vault", + warehouse_id: "test-warehouse", + }), + ).rejects.toThrow(); + + const loggedOutput = errorSpy.mock.calls + .map((call) => call.join(" ")) + .join(" "); + + // Should log the error message and statement ID + expect(loggedOutput).toContain("polling timeout"); + expect(loggedOutput).toContain("stmt-123"); + + // Should NOT log the SQL statement + expect(loggedOutput).not.toContain("secret_data"); + expect(loggedOutput).not.toContain("vault"); + + errorSpy.mockRestore(); + }); + }); +}); diff --git a/packages/appkit/src/stream/stream-manager.ts b/packages/appkit/src/stream/stream-manager.ts index 41764772..942de11d 100644 --- a/packages/appkit/src/stream/stream-manager.ts +++ b/packages/appkit/src/stream/stream-manager.ts @@ -3,11 +3,14 @@ import { context } from "@opentelemetry/api"; import type { IAppResponse, StreamConfig } from "shared"; import { EventRingBuffer } from "./buffers"; import { streamDefaults } from "./defaults"; +import { createLogger } from "../logging/logger"; import { SSEWriter } from "./sse-writer"; import { StreamRegistry } from "./stream-registry"; import { SSEErrorCode, type StreamEntry, type StreamOperation } from "./types"; import { StreamValidator } from "./validator"; +const logger = createLogger("stream"); + // main entry point for Server-Sent events streaming export class StreamManager { private activeOperations: Set; @@ -260,6 +263,8 @@ export class StreamManager { const errorEventId = randomUUID(); const errorCode = this._categorizeError(error); + logger.error("Stream execution failed: %s (code=%s)", errorMsg, errorCode); + // buffer error event streamEntry.eventBuffer.add({ id: errorEventId, diff --git a/packages/appkit/src/stream/tests/stream.test.ts b/packages/appkit/src/stream/tests/stream.test.ts index fae54289..53f75995 100644 --- a/packages/appkit/src/stream/tests/stream.test.ts +++ b/packages/appkit/src/stream/tests/stream.test.ts @@ -277,6 +277,36 @@ describe("StreamManager", () => { 0, ); }); + + test("should not log the full error object (query redaction)", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const { mockRes } = createMockResponse(); + + const dbError = new Error('column "email" does not exist') as any; + dbError.query = "SELECT email, secret_token FROM users WHERE id = $1"; + dbError.parameters = ["sensitive-id"]; + + async function* generator() { + yield { type: "start" }; + throw dbError; + } + + await streamManager.stream(mockRes as any, generator); + + const loggedOutput = errorSpy.mock.calls + .map((call) => call.join(" ")) + .join(" "); + + // Should log the error message (contains column name, which is OK) + expect(loggedOutput).toContain('column "email" does not exist'); + + // Should NOT log the raw query or parameters from the error object + expect(loggedOutput).not.toContain("secret_token"); + expect(loggedOutput).not.toContain("sensitive-id"); + + errorSpy.mockRestore(); + }); }); describe("heartbeat", () => {