fix(websocket): omit credentials from URL logs

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()
   }
 
   /**

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

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<NotificationEvent>()
+    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

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)