From 377c6025183d565b5c29644b6c880e68c252ebb6 Mon Sep 17 00:00:00 2001 From: olavurellefsen Date: Mon, 28 Sep 2026 22:17:30 +0100 Subject: [PATCH 1/2] fix(websocket): omit credentials from URL logs --- src/common/websocket-client.ts | 41 +++++++---- test/tests/common/websocket-client.test.ts | 82 ++++++++++++++++++++++ 2 files changed, 111 insertions(+), 12 deletions(-) diff --git a/src/common/websocket-client.ts b/src/common/websocket-client.ts index 732b824..83e4448 100644 --- a/src/common/websocket-client.ts +++ b/src/common/websocket-client.ts @@ -32,6 +32,23 @@ const MAX_RECONNECT_INTERVAL = 30_000 const WEBSOCKET_CLOSING = 2 const WEBSOCKET_CLOSED = 3 +function safeWebSocketLogUrl(url: string): string { + try { + const parsed = new URL(url) + // Keep endpoint context while excluding query parameters, fragments, and user info. + return `${parsed.origin}${parsed.pathname}` + } catch { + // Malformed input may still contain credentials outside a parseable query. + return "(invalid WebSocket URL)" + } +} + +function safeWebSocketLogEndpoint(baseUrl: string, pathSegment: string): string { + const cleanedBaseUrl = baseUrl.endsWith("/") ? baseUrl.slice(0, -1) : baseUrl + const cleanedPathSegment = pathSegment.startsWith("/") ? pathSegment.slice(1) : pathSegment + return safeWebSocketLogUrl(`${cleanedBaseUrl}/${cleanedPathSegment}`) +} + // Define the expected WebSocket interface subset used by the client interface MinimalWebSocket { readyState: number @@ -96,7 +113,7 @@ export class WebSocketClient { * @param baseUrl - The new base URL to use (e.g., "wss://staging-server.api.flowcore.io"). */ setBaseUrl(baseUrl: string): void { - this.logger.info(`WebSocket base URL overridden to: ${baseUrl}`) + this.logger.info(`WebSocket base URL overridden to: ${safeWebSocketLogUrl(baseUrl)}`) this.overrideBaseUrl = baseUrl } @@ -139,7 +156,7 @@ export class WebSocketClient { // Use override URL if set, otherwise use command's base URL const baseUrl = this.overrideBaseUrl ?? command.getWebSocketBaseUrl() const pathSegment = command.getWebSocketPathSegment(config) // Get path segment from command - this.logger.debug(`Attempting to connect stream: ${baseUrl}${pathSegment}`) + this.logger.debug(`Attempting to connect stream: ${safeWebSocketLogEndpoint(baseUrl, pathSegment)}`) try { const urlParams = new URLSearchParams() @@ -159,7 +176,7 @@ export class WebSocketClient { const cleanedPathSegment = pathSegment.startsWith("/") ? pathSegment.slice(1) : pathSegment const streamUrl = `${cleanedBaseUrl}/${cleanedPathSegment}?${urlParams.toString()}` // Construct URL from command parts - this.logger.debug(`Connecting to WebSocket URL: ${streamUrl}`) + this.logger.debug(`Connecting to WebSocket URL: ${safeWebSocketLogUrl(streamUrl)}`) this.webSocket = this.webSocketFactory(streamUrl) this.setupEventHandlers() // Sets up handlers that push to internalSubject @@ -171,7 +188,7 @@ export class WebSocketClient { disconnect: (): void => this.disconnect(), } } catch (error) { - this.logger.error(`Failed to initiate connection: ${error}`) + this.logger.error("Failed to initiate WebSocket connection.") this._isConnecting = false this.currentCommand = null // Clear command/config on failure this.currentConfig = null @@ -191,7 +208,7 @@ export class WebSocketClient { const baseUrl = this.overrideBaseUrl ?? this.currentCommand?.getWebSocketBaseUrl() const pathSegment = this.currentCommand?.getWebSocketPathSegment(this.currentConfig) const logUrl = baseUrl && pathSegment - ? `${baseUrl.replace(/\/?$/, "/")}${pathSegment.replace(/^\/?/, "")}` + ? safeWebSocketLogUrl(`${baseUrl.replace(/\/?$/, "/")}${pathSegment.replace(/^\/?/, "")}`) : "(unknown URL)" this.logger.debug(`WebSocket connection opened: ${logUrl}`) this.reconnectInterval = this.options.reconnectInterval @@ -235,10 +252,10 @@ export class WebSocketClient { const baseUrl = this.overrideBaseUrl ?? this.currentCommand?.getWebSocketBaseUrl() const pathSegment = this.currentCommand?.getWebSocketPathSegment(this.currentConfig) const logUrl = baseUrl && pathSegment - ? `${baseUrl.replace(/\/?$/, "/")}${pathSegment.replace(/^\/?/, "")}` + ? safeWebSocketLogUrl(`${baseUrl.replace(/\/?$/, "/")}${pathSegment.replace(/^\/?/, "")}`) : "(unknown URL)" this.logger.debug( - `WebSocket connection closed: ${logUrl} Code [${event.code}], Reason: ${event.reason || "No reason given"} + `WebSocket connection closed: ${logUrl} Code [${event.code}] . Was open: ${wasOpen}`, ) if (wasOpen && event.code !== 1000 && this.currentCommand) { // Only reconnect if command is still set @@ -256,7 +273,7 @@ export class WebSocketClient { const baseUrl = this.overrideBaseUrl ?? this.currentCommand?.getWebSocketBaseUrl() const pathSegment = this.currentCommand?.getWebSocketPathSegment(this.currentConfig) const logUrl = baseUrl && pathSegment - ? `${baseUrl.replace(/\/?$/, "/")}${pathSegment.replace(/^\/?/, "")}` + ? safeWebSocketLogUrl(`${baseUrl.replace(/\/?$/, "/")}${pathSegment.replace(/^\/?/, "")}`) : "(unknown URL)" this.logger.error(`WebSocket encountered an error for stream: ${logUrl}.`) if ( @@ -289,7 +306,7 @@ export class WebSocketClient { const baseUrl = this.overrideBaseUrl ?? this.currentCommand.getWebSocketBaseUrl() const pathSegment = this.currentCommand.getWebSocketPathSegment(this.currentConfig) const logUrl = baseUrl && pathSegment - ? `${baseUrl.replace(/\/?$/, "/")}${pathSegment.replace(/^\/?/, "")}` + ? safeWebSocketLogUrl(`${baseUrl.replace(/\/?$/, "/")}${pathSegment.replace(/^\/?/, "")}`) : "(unknown URL)" if (this.options.maxReconnects && this.reconnectAttempts >= this.options.maxReconnects) { this.logger.error( @@ -338,12 +355,12 @@ export class WebSocketClient { // Use potentially overridden base URL for reconnect URL const streamUrl = `${baseUrl}${pathSegment}?${urlParams.toString()}` - this.logger.debug(`Reconnecting to WebSocket URL: ${streamUrl}`) + this.logger.debug(`Reconnecting to WebSocket URL: ${safeWebSocketLogUrl(streamUrl)}`) this.webSocket = this.webSocketFactory(streamUrl) // Connecting flag will be reset in onopen/onerror/onclose this.setupEventHandlers() - } catch (error) { - this.logger.error(`Reconnect attempt connection failed: ${error}`) + } catch { + this.logger.error("Reconnect attempt connection failed.") this._isConnecting = false // Reset connection flag on immediate error // Error during reconnect setup, trigger another attempt after backoff // We might get stuck here if auth always fails, consider adding specific error handling diff --git a/test/tests/common/websocket-client.test.ts b/test/tests/common/websocket-client.test.ts index 71a0e48..eab85b0 100644 --- a/test/tests/common/websocket-client.test.ts +++ b/test/tests/common/websocket-client.test.ts @@ -169,6 +169,28 @@ describe("WebSocketClient", () => { // Updated describe block assertEquals(instance.url, expectedUrl) }) + it("does not log API key credentials in the initial WebSocket URL", async () => { + const logger = createRecordingLogger() + client = createClient(authOptionsApiKey, { logger }) + activeStream = await client.connect(testCommand) + + assertEquals(mockWebSocketInstances.length, 1) + assertEquals(mockWebSocketInstances[0].url.includes("test-api-key"), true) + const allLogs = JSON.stringify(logger.calls) + assertEquals(allLogs.includes("test-api-key"), false) + assertEquals(allLogs.includes("test-api-key-id"), false) + assertEquals(allLogs.includes("ai-coordinator.api.flowcore.io/api/v1/stream/conv_test_123"), true) + + activeStream.disconnect() + await delay(10) + + const bearerLogger = createRecordingLogger() + client = createClient(authOptionsBearer, { logger: bearerLogger }) + activeStream = await client.connect(testCommand) + assertEquals(mockWebSocketInstances[1].url.includes("test-bearer-token"), true) + assertEquals(JSON.stringify(bearerLogger.calls).includes("test-bearer-token"), false) + }) + it("should set client state correctly during connection", async () => { client = createClient(authOptionsBearer) const connectPromise = client.connect(testCommand) @@ -321,6 +343,66 @@ describe("WebSocketClient", () => { // Updated describe block assertEquals(client.isConnecting, false) }) + it("does not log API key credentials in reconnect URLs", async () => { + const logger = createRecordingLogger() + client = createClient(authOptionsApiKey, { maxReconnects: 1, logger }) + activeStream = await client.connect(testCommand) + const firstSocket = mockWebSocketInstances[0] + firstSocket._triggerOpen() + await delay(0) + + firstSocket._triggerClose(1006, "Synthetic close") + await delay(clientOptions.reconnectInterval + 50) + + assertEquals(mockWebSocketInstances.length, 2) + assertEquals(mockWebSocketInstances[1].url.includes("test-api-key"), true) + const allLogs = JSON.stringify(logger.calls) + assertEquals(allLogs.includes("test-api-key"), false) + assertEquals(allLogs.includes("test-api-key-id"), false) + assertEquals(allLogs.includes("ai-coordinator.api.flowcore.io/api/v1/stream/conv_test_123"), true) + }) + + it("does not log URL-bearing close reasons", async () => { + const logger = createRecordingLogger() + client = createClient(authOptionsApiKey, { logger }) + activeStream = await client.connect(testCommand) + const socket = mockWebSocketInstances[0] + socket._triggerOpen() + await delay(0) + + socket._triggerClose(1000, "wss://service.example/stream?token=fake-close-secret") + + const allLogs = JSON.stringify(logger.calls) + assertEquals(allLogs.includes("fake-close-secret"), false) + assertEquals(allLogs.includes("Code [1000]"), true) + }) + + it("does not log malformed URLs or credential-bearing setup errors", async () => { + const logger = createRecordingLogger() + client = createClient(authOptionsApiKey, { logger }) + client.setBaseUrl("wss://fake-user:fake-password@bad host?token=fake-url-secret") + + assertEquals(JSON.stringify(logger.calls).includes("fake-password"), false) + assertEquals(JSON.stringify(logger.calls).includes("fake-url-secret"), false) + assertEquals(logger.calls.info.some((message) => message.includes("(invalid WebSocket URL)")), true) + + const failingClient = new WebSocketClient( + authOptionsApiKey, + { ...clientOptions, logger }, + () => { + const error = new Error("wss://service.example/stream?api_key=fake-error-secret") + error.name = "fake-name-secret" + throw error + }, + ) + await failingClient.connect(testCommand).catch(() => {}) + + const allLogs = JSON.stringify(logger.calls) + assertEquals(allLogs.includes("fake-error-secret"), false) + assertEquals(allLogs.includes("fake-name-secret"), false) + assertEquals(allLogs.includes("Failed to initiate WebSocket connection."), true) + }) + it("output$ should complete after max reconnect attempts failed", async () => { client = createClient(authOptionsBearer, { maxReconnects: 1, reconnectInterval: 5 }) activeStream = await client.connect(testCommand) From 83d820d9e1d6133773c99167538cbd92c3c81f0c Mon Sep 17 00:00:00 2001 From: olavurellefsen Date: Mon, 28 Sep 2026 22:24:51 +0100 Subject: [PATCH 2/2] fix(notifications): omit websocket error event from logs --- src/common/notification-client.ts | 25 ++++++++------- test/tests/common/notification-client.test.ts | 32 +++++++++++++++++++ 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/src/common/notification-client.ts b/src/common/notification-client.ts index 251711b..305c535 100644 --- a/src/common/notification-client.ts +++ b/src/common/notification-client.ts @@ -266,19 +266,22 @@ export class NotificationClient { this.observer.complete() } - this.webSocket.onerror = (error) => { - // A server without subprotocol support refuses the handshake, which - // surfaces here BEFORE `onopen`. That is a probe result, not a fault, so - // it must not terminate the observable — `onclose` performs the retry. - if (this.shouldFallBackToQueryTransport()) { - this.logger.debug("Subprotocol handshake refused; retrying on the legacy query transport") - this.webSocket.close() - return - } - this.logger.error(`WebSocket encountered error: ${error}`) - this.observer.error(error) + this.webSocket.onerror = (error) => this.handleWebSocketError(error) + } + + private handleWebSocketError(error: Event) { + // A server without subprotocol support refuses the handshake, which + // surfaces here BEFORE `onopen`. That is a probe result, not a fault, so + // it must not terminate the observable — `onclose` performs the retry. + if (this.shouldFallBackToQueryTransport()) { + this.logger.debug("Subprotocol handshake refused; retrying on the legacy query transport") this.webSocket.close() + return } + // ErrorEvent stringification can include the request URL and its query. + this.logger.error("WebSocket encountered an error.") + this.observer.error(error) + this.webSocket.close() } /** diff --git a/test/tests/common/notification-client.test.ts b/test/tests/common/notification-client.test.ts index 7f718f9..1b70491 100644 --- a/test/tests/common/notification-client.test.ts +++ b/test/tests/common/notification-client.test.ts @@ -93,6 +93,38 @@ describe("NotificationClient.handleMessage", () => { }) }) +describe("NotificationClient WebSocket errors", () => { + it("does not log a credential-bearing ErrorEvent string", () => { + const observer = new Subject() + let observedError: Event | undefined + observer.subscribe({ error: (error) => observedError = error }) + const logger = recordingLogger() + const client = new NotificationClient( + observer, + { apiKey: "fake-api-key", apiKeyId: "fake-key-id" }, + { tenant: "t", dataCore: "dc" }, + { logger }, + ) + const event = Object.assign(new Event("error"), { + toString: () => "wss://service.example/notifications?api_key=fake-api-key&api_key_id=fake-key-id", + }) + const internal = client as unknown as { + transportSettled: boolean + webSocket: { readyState: number; close: () => void } + handleWebSocketError: (error: Event) => void + } + internal.transportSettled = true + internal.webSocket = { readyState: 1, close: () => {} } + + internal.handleWebSocketError(event) + + assertEquals(logger.calls.error, ["WebSocket encountered an error."]) + assertEquals(JSON.stringify(logger.calls).includes("fake-api-key"), false) + assertEquals(JSON.stringify(logger.calls).includes("fake-key-id"), false) + assertEquals(observedError, event) + }) +}) + // The security property of this client, asserted directly: on the default // transport the credential is NOT in the URL. An ingress access log records the // request line, so a credential in the query string is written to disk in