From d14abfffacb813540743f3f9bd5d3717c65f5dc6 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Wed, 20 May 2026 10:44:14 -0300 Subject: [PATCH 1/2] fix(cli): include provider diagnostics in collaboration sync timeouts Before, COLLABORATION_SYNC_TIMEOUT details only contained { timeoutMs }. DNS failures, refused TCP, failed WS upgrades, and auth rejections all collapsed to the same useless payload. The timeout now captures last status/connection-error/connection-close/ authenticationFailed plus event counts, sanitized URL, documentId, tokenEnv state, and paramsKeys. Sensitive query params are redacted in URL and event payload strings. y-websocket's [event, provider] emit pattern is reduced to the first arg so the provider self-ref graph does not bloat the envelope or risk leaking document content. Mock constructors in runtime.test.ts updated: the previous pattern returned undefined instances under bun's mock wrapping, which was harmless when the runtime did not touch the provider synchronously. The new attachProviderDiagnostics binding does. --- .../__tests__/diagnostics.test.ts | 343 ++++++++++++++++++ .../collaboration/__tests__/runtime.test.ts | 12 +- apps/cli/src/lib/collaboration/diagnostics.ts | 249 +++++++++++++ apps/cli/src/lib/collaboration/runtime.ts | 23 +- 4 files changed, 618 insertions(+), 9 deletions(-) create mode 100644 apps/cli/src/lib/collaboration/__tests__/diagnostics.test.ts create mode 100644 apps/cli/src/lib/collaboration/diagnostics.ts diff --git a/apps/cli/src/lib/collaboration/__tests__/diagnostics.test.ts b/apps/cli/src/lib/collaboration/__tests__/diagnostics.test.ts new file mode 100644 index 0000000000..34306863a1 --- /dev/null +++ b/apps/cli/src/lib/collaboration/__tests__/diagnostics.test.ts @@ -0,0 +1,343 @@ +import { describe, expect, test } from 'bun:test'; +import { attachProviderDiagnostics, sanitizeDiagnosticString } from '../diagnostics'; +import type { SyncableProvider } from '../types'; + +// --------------------------------------------------------------------------- +// Test helpers +// --------------------------------------------------------------------------- + +type EmittableProvider = SyncableProvider & { + emit(event: string, ...args: unknown[]): void; +}; + +function makeEmittableProvider(initial: Partial = {}): EmittableProvider { + const handlers = new Map void>>(); + return { + ...initial, + on(event: string, fn: (...args: unknown[]) => void) { + if (!handlers.has(event)) handlers.set(event, []); + handlers.get(event)!.push(fn); + }, + off(event: string, fn: (...args: unknown[]) => void) { + const arr = handlers.get(event); + if (!arr) return; + const idx = arr.indexOf(fn); + if (idx >= 0) arr.splice(idx, 1); + }, + emit(event: string, ...args: unknown[]) { + for (const fn of handlers.get(event) ?? []) fn(...args); + }, + } as EmittableProvider; +} + +function baseContext() { + return { + providerType: 'y-websocket' as const, + url: 'wss://broker.example.com/room', + documentId: 'doc-1', + tokenEnvConfigured: false, + authTokenResolved: false, + paramsKeys: [], + }; +} + +// --------------------------------------------------------------------------- +// sanitizeDiagnosticString +// --------------------------------------------------------------------------- + +describe('sanitizeDiagnosticString', () => { + test('redacts ?token= in URL', () => { + const out = sanitizeDiagnosticString('wss://broker.example.com/room?token=secret-abc-123'); + expect(out).not.toContain('secret-abc-123'); + expect(out).toContain('[REDACTED]'); + }); + + test('redacts ?auth= in URL', () => { + const out = sanitizeDiagnosticString('wss://x/r?auth=open-sesame-99'); + expect(out).not.toContain('open-sesame-99'); + expect(out).toContain('[REDACTED]'); + }); + + test('redacts ?authorization= in URL', () => { + const out = sanitizeDiagnosticString('https://x/r?authorization=BEARER-XYZ'); + expect(out).not.toContain('BEARER-XYZ'); + expect(out).toContain('[REDACTED]'); + }); + + test('redacts ?password= in URL', () => { + const out = sanitizeDiagnosticString('wss://x/r?password=p4ss'); + expect(out).not.toContain('p4ss'); + expect(out).toContain('[REDACTED]'); + }); + + test('redacts ?apiKey= and ?api-key= in URL', () => { + const out1 = sanitizeDiagnosticString('wss://x/r?apiKey=KEY-ONE'); + const out2 = sanitizeDiagnosticString('wss://x/r?api-key=KEY-TWO'); + expect(out1).not.toContain('KEY-ONE'); + expect(out2).not.toContain('KEY-TWO'); + }); + + test('preserves non-sensitive params', () => { + const out = sanitizeDiagnosticString('wss://x/r?token=secret&docId=open-data®ion=us'); + expect(out).not.toContain('secret'); + expect(out).toContain('docId=open-data'); + expect(out).toContain('region=us'); + }); + + test('redacts multiple sensitive keys at once', () => { + const out = sanitizeDiagnosticString('wss://x/r?token=tok-x&auth=auth-y&secret=sec-z'); + expect(out).not.toContain('tok-x'); + expect(out).not.toContain('auth-y'); + expect(out).not.toContain('sec-z'); + }); + + test('redacts tokens in free-form error messages', () => { + const out = sanitizeDiagnosticString( + "WebSocket connection to 'wss://broker.example.com/room?token=leaked-abc-123' failed: Expected 101", + ); + expect(out).not.toContain('leaked-abc-123'); + expect(out).toContain('Expected 101'); + }); + + test('leaves benign strings alone', () => { + const out = sanitizeDiagnosticString('Expected 101 status code'); + expect(out).toBe('Expected 101 status code'); + }); +}); + +// --------------------------------------------------------------------------- +// attachProviderDiagnostics — context fields +// --------------------------------------------------------------------------- + +describe('attachProviderDiagnostics — timeout details shape', () => { + test('includes every required field', () => { + const provider = makeEmittableProvider({ synced: false }); + const diagnostics = attachProviderDiagnostics(provider, { + providerType: 'y-websocket', + url: 'wss://broker.example.com/room', + documentId: 'doc-1', + tokenEnvConfigured: true, + authTokenResolved: true, + paramsKeys: ['region', 'docVersion'], + }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 9_500); + + expect(details).toMatchObject({ + timeoutMs: 10_000, + elapsedMs: 9_500, + providerType: 'y-websocket', + url: 'wss://broker.example.com/room', + documentId: 'doc-1', + tokenEnvConfigured: true, + authTokenResolved: true, + paramsKeys: ['region', 'docVersion'], + }); + expect(details.eventCounts).toBeDefined(); + expect(details.providerState).toBeDefined(); + }); + + test('distinguishes auth-not-configured from auth-configured-and-resolved', () => { + // The tokenEnv-configured-but-empty case throws MISSING_REQUIRED in + // resolveCollaborationToken before runtime setup, so it can't reach + // toTimeoutDetails. The two states we actually see at the timeout path + // are: (a) no tokenEnv configured, no token resolved; (b) tokenEnv + // configured AND env var resolved to a non-empty value. + const provider = makeEmittableProvider(); + + const notConfigured = attachProviderDiagnostics(provider, { + ...baseContext(), + tokenEnvConfigured: false, + authTokenResolved: false, + }).toTimeoutDetails(provider, 10_000, 1); + expect(notConfigured.tokenEnvConfigured).toBe(false); + expect(notConfigured.authTokenResolved).toBe(false); + + const configuredAndResolved = attachProviderDiagnostics(provider, { + ...baseContext(), + tokenEnvConfigured: true, + authTokenResolved: true, + }).toTimeoutDetails(provider, 10_000, 1); + expect(configuredAndResolved.tokenEnvConfigured).toBe(true); + expect(configuredAndResolved.authTokenResolved).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// attachProviderDiagnostics — event capture +// --------------------------------------------------------------------------- + +describe('attachProviderDiagnostics — event capture', () => { + test('captures last connection-close with code/reason/wasClean', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + provider.emit('connection-close', { code: 1006, reason: 'closed', wasClean: false }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + expect(details.lastConnectionClose).toMatchObject({ + code: 1006, + reason: 'closed', + wasClean: false, + }); + }); + + test('captures last connection-error', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + provider.emit('connection-error', { type: 'error', message: 'Expected 101 status code' }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + expect(details.lastConnectionError).toMatchObject({ + type: 'error', + message: 'Expected 101 status code', + }); + }); + + test('captures last status', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + provider.emit('status', { status: 'connecting' }); + provider.emit('status', { status: 'connected' }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + expect(details.lastStatus).toMatchObject({ status: 'connected' }); + }); + + test('skips null-payload connection-close (client-initiated disconnect)', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + provider.emit('connection-close', { code: 1006, reason: 'real', wasClean: false }); + provider.emit('connection-close', null); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + expect((details.lastConnectionClose as { code?: number }).code).toBe(1006); + }); + + test('strips trailing provider self-reference (second emit arg)', () => { + // y-websocket emits `[event, provider]`. The provider self-ref must NOT + // appear in the captured payload - it would bloat the envelope and risk + // leaking document content if the provider holds it. + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + const fakeProviderRef = { + _observers: {}, + doc: { internals: 'should-not-leak' }, + wsconnected: false, + }; + provider.emit('connection-error', { type: 'error', message: 'fail' }, fakeProviderRef); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + const lastErr = details.lastConnectionError as Record; + expect(lastErr.type).toBe('error'); + expect(lastErr.message).toBe('fail'); + expect(JSON.stringify(lastErr)).not.toContain('should-not-leak'); + expect(JSON.stringify(lastErr)).not.toContain('_observers'); + }); + + test('captures Hocuspocus authenticationFailed', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + provider.emit('authenticationFailed', { reason: 'permission-denied' }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + expect(details.lastAuthenticationFailed).toBeDefined(); + }); + + test('eventCounts reflect number of emissions per event', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + provider.emit('status', { status: 'connecting' }); + provider.emit('status', { status: 'connected' }); + provider.emit('connection-error', { type: 'error', message: 'x' }); + provider.emit('connection-close', { code: 1006, reason: '', wasClean: false }); + provider.emit('connection-close', { code: 1006, reason: '', wasClean: false }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + const counts = details.eventCounts as Record; + expect(counts.status).toBe(2); + expect(counts['connection-error']).toBe(1); + expect(counts['connection-close']).toBe(2); + }); + + test('detach stops further capture and is idempotent', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + provider.emit('connection-error', { type: 'error', message: 'before-detach' }); + diagnostics.detach(); + diagnostics.detach(); // idempotent + provider.emit('connection-error', { type: 'error', message: 'after-detach' }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + expect((details.lastConnectionError as { message?: string }).message).toBe('before-detach'); + }); +}); + +// --------------------------------------------------------------------------- +// attachProviderDiagnostics — redaction in captured payloads +// --------------------------------------------------------------------------- + +describe('attachProviderDiagnostics — redaction', () => { + test('redacts token in URL inside event payload reason', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + + provider.emit('connection-error', { + type: 'error', + message: "WebSocket connection to 'wss://broker/room?token=leaked-xyz' failed", + }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + const err = details.lastConnectionError as { message?: string }; + expect(err.message).not.toContain('leaked-xyz'); + expect(err.message).toContain('[REDACTED]'); + }); + + test('redacts sensitive keys in url field of context', () => { + const provider = makeEmittableProvider(); + const diagnostics = attachProviderDiagnostics(provider, { + ...baseContext(), + url: 'wss://broker.example.com/room?token=should-not-leak®ion=us', + }); + + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1) as Record; + expect(details.url as string).not.toContain('should-not-leak'); + expect(details.url as string).toContain('region=us'); + }); +}); + +// --------------------------------------------------------------------------- +// attachProviderDiagnostics — defensive subscription +// --------------------------------------------------------------------------- + +describe('attachProviderDiagnostics — defensive subscription', () => { + test('does not throw when provider.on throws', () => { + const provider: SyncableProvider = { + on() { + throw new Error('subscribe failed'); + }, + off() {}, + }; + expect(() => attachProviderDiagnostics(provider, baseContext())).not.toThrow(); + }); + + test('returns a working diagnostics object even when subscription fails', () => { + const provider: SyncableProvider = { + on() { + throw new Error('subscribe failed'); + }, + off() {}, + }; + const diagnostics = attachProviderDiagnostics(provider, baseContext()); + const details = diagnostics.toTimeoutDetails(provider, 10_000, 1); + expect(details).toBeDefined(); + expect((details as Record).providerType).toBe('y-websocket'); + }); +}); diff --git a/apps/cli/src/lib/collaboration/__tests__/runtime.test.ts b/apps/cli/src/lib/collaboration/__tests__/runtime.test.ts index f0f83757a2..93f1cb8c90 100644 --- a/apps/cli/src/lib/collaboration/__tests__/runtime.test.ts +++ b/apps/cli/src/lib/collaboration/__tests__/runtime.test.ts @@ -13,9 +13,11 @@ const mockWsInstance = { synced: false, }; -const MockWebsocketProvider = mock(function (this: unknown, ..._args: unknown[]) { - Object.assign(this as Record, mockWsInstance); -}); +// Bun's `mock()` wrapper does not preserve `new`-binding of `this` reliably, +// so we return the instance from the mock function directly. JS `new` semantics +// use an explicit object return as the constructor's result, so this still +// looks like `new WebsocketProvider(...)` from callers' perspective. +const MockWebsocketProvider = mock((..._args: unknown[]) => mockWsInstance); const mockHocuspocusInstance = { on: mock(() => {}), @@ -25,9 +27,7 @@ const mockHocuspocusInstance = { synced: false, }; -const MockHocuspocusProvider = mock(function (this: unknown, ..._args: unknown[]) { - Object.assign(this as Record, mockHocuspocusInstance); -}); +const MockHocuspocusProvider = mock((..._args: unknown[]) => mockHocuspocusInstance); mock.module('y-websocket', () => ({ WebsocketProvider: MockWebsocketProvider, diff --git a/apps/cli/src/lib/collaboration/diagnostics.ts b/apps/cli/src/lib/collaboration/diagnostics.ts new file mode 100644 index 0000000000..8b9b941f60 --- /dev/null +++ b/apps/cli/src/lib/collaboration/diagnostics.ts @@ -0,0 +1,249 @@ +import type { SyncableProvider, WebSocketProviderType } from './types'; + +const REDACTED = '[REDACTED]'; +const MAX_OBJECT_DEPTH = 5; + +export type CollaborationDiagnosticContext = { + providerType: WebSocketProviderType; + url: string; + documentId: string; + // tokenEnvConfigured: the profile told us to read auth from an env var. + // authTokenResolved: that env var actually held a non-empty value at runtime. + // Splitting these two lets us tell "no auth configured" from "auth configured + // but env empty" - the customer's reproduction was the first case, not the + // second. + tokenEnvConfigured: boolean; + authTokenResolved: boolean; + paramsKeys: string[]; +}; + +export type ProviderDiagnostics = { + toTimeoutDetails(provider: SyncableProvider, timeoutMs: number, elapsedMs: number): Record; + detach(): void; +}; + +function isSensitiveDiagnosticKey(key: string): boolean { + const normalized = key.toLowerCase().replace(/[^a-z0-9]/g, ''); + if (!normalized) return false; + + return ( + normalized === 'key' || + normalized === 'apikey' || + normalized === 'authorization' || + normalized.includes('token') || + normalized.includes('auth') || + normalized.includes('secret') || + normalized.includes('password') || + normalized.includes('credential') + ); +} + +function sanitizeUrlCandidate(value: string): string { + try { + const parsed = new URL(value); + for (const key of Array.from(parsed.searchParams.keys())) { + if (isSensitiveDiagnosticKey(key)) { + parsed.searchParams.set(key, REDACTED); + } + } + return parsed.toString(); + } catch { + return value; + } +} + +export function sanitizeDiagnosticString(value: string): string { + const withSanitizedUrls = value.replace(/\b(?:wss?|https?):\/\/[^\s"'<>]+/gi, sanitizeUrlCandidate); + + return withSanitizedUrls.replace(/([?&])([^=&#\s"']+)=([^&#\s"']*)/g, (match, prefix: string, key: string) => { + const decodedKey = decodeURIComponent(key.replace(/\+/g, ' ')); + if (!isSensitiveDiagnosticKey(decodedKey)) return match; + return `${prefix}${key}=${REDACTED}`; + }); +} + +function summarizeError(error: Error): Record { + return { + name: sanitizeDiagnosticString(error.name), + message: sanitizeDiagnosticString(error.message), + }; +} + +function isRecord(value: unknown): value is Record { + return !!value && typeof value === 'object'; +} + +function summarizeEventLike(value: Record): Record | null { + if ('code' in value && 'reason' in value && 'wasClean' in value) { + return { + code: typeof value.code === 'number' ? value.code : undefined, + reason: sanitizeDiagnosticString(String(value.reason ?? '')), + wasClean: Boolean(value.wasClean), + }; + } + + if ('message' in value && 'type' in value) { + return { + type: sanitizeDiagnosticString(String(value.type ?? '')), + message: sanitizeDiagnosticString(String(value.message ?? '')), + }; + } + + return null; +} + +export function sanitizeDiagnosticValue(value: unknown, depth = 0): unknown { + if (value == null) return value; + if (typeof value === 'string') return sanitizeDiagnosticString(value); + if (typeof value === 'number' || typeof value === 'boolean') return value; + if (typeof value === 'bigint') return value.toString(); + if (value instanceof Error) return summarizeError(value); + if (depth >= MAX_OBJECT_DEPTH) return '[Truncated]'; + + if (Array.isArray(value)) { + return value.map((entry) => sanitizeDiagnosticValue(entry, depth + 1)); + } + + if (!isRecord(value)) { + return Object.prototype.toString.call(value); + } + + const eventSummary = summarizeEventLike(value); + if (eventSummary) return eventSummary; + + const output: Record = {}; + for (const [key, entry] of Object.entries(value)) { + if (typeof entry === 'function' || typeof entry === 'symbol' || typeof entry === 'undefined') continue; + output[key] = sanitizeDiagnosticValue(entry, depth + 1); + } + return output; +} + +function sanitizeProviderState(provider: SyncableProvider): Record { + const record = provider as Record; + return sanitizeDiagnosticValue({ + synced: record.synced, + isSynced: record.isSynced, + status: record.status, + wsconnected: record.wsconnected, + wsconnecting: record.wsconnecting, + shouldConnect: record.shouldConnect, + }) as Record; +} + +// y-websocket emits events as `[payload, provider]` where the second arg is +// the provider self-reference. Hocuspocus uses `[payload]`. We only want the +// payload - the provider self-ref bloats details with the entire YDoc/state +// graph and risks leaking document content if a future provider includes it. +function firstUsefulArg(args: unknown[]): unknown { + return args[0]; +} + +export function attachProviderDiagnostics( + provider: SyncableProvider, + context: CollaborationDiagnosticContext, +): ProviderDiagnostics { + const eventCounts: Record = {}; + const cleanup: Array<() => void> = []; + let lastStatus: unknown; + let lastConnectionError: unknown; + let lastConnectionClose: unknown; + let lastClose: unknown; + let lastDisconnect: unknown; + let lastAuthenticationFailed: unknown; + let detached = false; + + const capture = (eventName: string, args: unknown[]) => { + eventCounts[eventName] = (eventCounts[eventName] ?? 0) + 1; + + const payload = firstUsefulArg(args); + if (eventName === 'connection-close' && payload == null) return; + + const sanitizedPayload = sanitizeDiagnosticValue(payload); + switch (eventName) { + case 'status': + lastStatus = sanitizedPayload; + break; + case 'connection-error': + lastConnectionError = sanitizedPayload; + break; + case 'connection-close': + lastConnectionClose = sanitizedPayload; + break; + case 'close': + lastClose = sanitizedPayload; + break; + case 'disconnect': + lastDisconnect = sanitizedPayload; + break; + case 'authenticationFailed': + lastAuthenticationFailed = sanitizedPayload; + break; + } + }; + + const subscribe = (eventName: string) => { + if (!provider.on) return; + // Best-effort: a failing subscription must never crash collaboration setup. + try { + const handler = (...args: unknown[]) => capture(eventName, args); + provider.on(eventName, handler); + cleanup.push(() => { + try { + provider.off?.(eventName, handler); + } catch { + // ignore detach failures + } + }); + } catch { + // ignore subscribe failures - diagnostics are advisory + } + }; + + for (const eventName of [ + 'status', + 'sync', + 'synced', + 'connection-error', + 'connection-close', + 'close', + 'disconnect', + 'authenticationFailed', + 'authenticated', + ]) { + subscribe(eventName); + } + + return { + toTimeoutDetails(providerForSnapshot, timeoutMs, elapsedMs) { + const details: Record = { + timeoutMs, + elapsedMs, + providerType: context.providerType, + url: sanitizeDiagnosticString(context.url), + documentId: sanitizeDiagnosticString(context.documentId), + tokenEnvConfigured: context.tokenEnvConfigured, + authTokenResolved: context.authTokenResolved, + paramsKeys: [...context.paramsKeys], + eventCounts: { ...eventCounts }, + providerState: sanitizeProviderState(providerForSnapshot), + }; + + if (lastStatus !== undefined) details.lastStatus = lastStatus; + if (lastConnectionError !== undefined) details.lastConnectionError = lastConnectionError; + if (lastConnectionClose !== undefined) details.lastConnectionClose = lastConnectionClose; + if (lastClose !== undefined) details.lastClose = lastClose; + if (lastDisconnect !== undefined) details.lastDisconnect = lastDisconnect; + if (lastAuthenticationFailed !== undefined) details.lastAuthenticationFailed = lastAuthenticationFailed; + + return details; + }, + detach() { + if (detached) return; + detached = true; + for (const run of cleanup.splice(0)) { + run(); + } + }, + }; +} diff --git a/apps/cli/src/lib/collaboration/runtime.ts b/apps/cli/src/lib/collaboration/runtime.ts index a5e053cc8e..e37277efe6 100644 --- a/apps/cli/src/lib/collaboration/runtime.ts +++ b/apps/cli/src/lib/collaboration/runtime.ts @@ -2,6 +2,7 @@ import { HocuspocusProvider } from '@hocuspocus/provider'; import { WebsocketProvider } from 'y-websocket'; import { Doc as YDoc } from 'yjs'; import { CliError } from '../errors'; +import { attachProviderDiagnostics, type ProviderDiagnostics } from './diagnostics'; import { createLiveblocksRuntime } from './liveblocks'; import { resolveCollaborationToken } from './resolve'; import type { @@ -26,10 +27,15 @@ function isSynced(provider: SyncableProvider): boolean { return provider.synced === true || provider.isSynced === true; } -export function waitForProviderSync(provider: SyncableProvider, timeoutMs: number): Promise { +export function waitForProviderSync( + provider: SyncableProvider, + timeoutMs: number, + diagnostics?: ProviderDiagnostics, +): Promise { if (isSynced(provider)) return Promise.resolve(); return new Promise((resolve, reject) => { + const startedAt = Date.now(); let settled = false; const cleanup: Array<() => void> = []; @@ -60,9 +66,10 @@ export function waitForProviderSync(provider: SyncableProvider, timeoutMs: numbe } const timer = setTimeout(() => { + const elapsedMs = Date.now() - startedAt; finish( new CliError('COLLABORATION_SYNC_TIMEOUT', `Collaboration sync timed out after ${timeoutMs}ms.`, { - timeoutMs, + ...(diagnostics?.toTimeoutDetails(provider, timeoutMs, elapsedMs) ?? { timeoutMs }), }), ); }, timeoutMs); @@ -113,11 +120,21 @@ function createWebSocketRuntime(profile: WebSocketCollaborationProfile): Collabo }) as unknown as SyncableProvider; } + const diagnostics = attachProviderDiagnostics(provider, { + providerType: profile.providerType, + url: profile.url, + documentId: profile.documentId, + tokenEnvConfigured: Boolean(profile.tokenEnv), + authTokenResolved: Boolean(token), + paramsKeys: Object.keys(profile.params ?? {}), + }); + return { ydoc, provider, - waitForSync: () => waitForProviderSync(provider, syncTimeoutMs), + waitForSync: () => waitForProviderSync(provider, syncTimeoutMs, diagnostics), dispose() { + diagnostics?.detach(); provider.disconnect?.(); provider.destroy?.(); ydoc.destroy(); From 71e0fd37c54a1c4503b6be1a6684a2f301a3daea Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Wed, 20 May 2026 11:47:33 -0300 Subject: [PATCH 2/2] fix(cli): guard URI decode in collaboration diagnostics sanitization decodeURIComponent throws URIError on malformed percent-encoding (e.g. a raw `%` in a query key). The diagnostics sanitizer runs inside the sync-timeout callback, so an uncaught throw would suppress the COLLABORATION_SYNC_TIMEOUT envelope entirely - the exact failure mode the rest of the module defends against. Falls back to the raw key on URIError so the sensitivity check still runs. Decoding stays in place so encoded variants like `%74oken` continue to redact correctly. --- .../__tests__/diagnostics.test.ts | 21 +++++++++++++++++++ apps/cli/src/lib/collaboration/diagnostics.ts | 10 ++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/apps/cli/src/lib/collaboration/__tests__/diagnostics.test.ts b/apps/cli/src/lib/collaboration/__tests__/diagnostics.test.ts index 34306863a1..ad083e1cb2 100644 --- a/apps/cli/src/lib/collaboration/__tests__/diagnostics.test.ts +++ b/apps/cli/src/lib/collaboration/__tests__/diagnostics.test.ts @@ -103,6 +103,27 @@ describe('sanitizeDiagnosticString', () => { const out = sanitizeDiagnosticString('Expected 101 status code'); expect(out).toBe('Expected 101 status code'); }); + + test('does not throw on malformed percent-encoded query keys', () => { + // decodeURIComponent('%') throws URIError. The sanitizer runs inside the + // sync-timeout callback, so any throw here would suppress the structured + // COLLABORATION_SYNC_TIMEOUT envelope entirely. + expect(() => sanitizeDiagnosticString('wss://broker/r?%=x')).not.toThrow(); + expect(typeof sanitizeDiagnosticString('wss://broker/r?%=x')).toBe('string'); + }); + + test('still redacts other sensitive keys when one key has a malformed escape', () => { + const out = sanitizeDiagnosticString('wss://broker/r?%=junk&token=secret-leak-123'); + expect(out).not.toContain('secret-leak-123'); + expect(out).toContain('[REDACTED]'); + }); + + test('redacts percent-encoded sensitive key names', () => { + // %74oken decodes to 'token' - should still be redacted so encoded + // variants can't bypass the sensitivity check. + const out = sanitizeDiagnosticString('wss://broker/r?%74oken=secret-leak-xyz'); + expect(out).not.toContain('secret-leak-xyz'); + }); }); // --------------------------------------------------------------------------- diff --git a/apps/cli/src/lib/collaboration/diagnostics.ts b/apps/cli/src/lib/collaboration/diagnostics.ts index 8b9b941f60..0cbec9b026 100644 --- a/apps/cli/src/lib/collaboration/diagnostics.ts +++ b/apps/cli/src/lib/collaboration/diagnostics.ts @@ -56,7 +56,15 @@ export function sanitizeDiagnosticString(value: string): string { const withSanitizedUrls = value.replace(/\b(?:wss?|https?):\/\/[^\s"'<>]+/gi, sanitizeUrlCandidate); return withSanitizedUrls.replace(/([?&])([^=&#\s"']+)=([^&#\s"']*)/g, (match, prefix: string, key: string) => { - const decodedKey = decodeURIComponent(key.replace(/\+/g, ' ')); + let decodedKey: string; + try { + decodedKey = decodeURIComponent(key.replace(/\+/g, ' ')); + } catch { + // Malformed percent-encoding (e.g. a raw `%` in the key). Fall back to + // the raw key for the sensitivity check — diagnostics must never throw + // because they run inside the sync-timeout callback. + decodedKey = key; + } if (!isSensitiveDiagnosticKey(decodedKey)) return match; return `${prefix}${key}=${REDACTED}`; });