diff --git a/apps/vs-code-designer/src/app/commands/workflows/designer/utils/__test__/fileSystemConnection.test.ts b/apps/vs-code-designer/src/app/commands/workflows/designer/utils/__test__/fileSystemConnection.test.ts new file mode 100644 index 00000000000..39bbc5c6c33 --- /dev/null +++ b/apps/vs-code-designer/src/app/commands/workflows/designer/utils/__test__/fileSystemConnection.test.ts @@ -0,0 +1,206 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ +import { EventEmitter } from 'events'; +import { Writable } from 'stream'; +import type { FileSystemConnectionInfo } from '@microsoft/vscode-extension-logic-apps'; +import * as childProcess from 'child_process'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('child_process', () => ({ + spawn: vi.fn(), +})); + +vi.mock('../../../../../../localize', () => ({ + localize: vi.fn((_key: string, defaultValue: string) => defaultValue), +})); + +import { createFileSystemConnection } from '../fileSystemConnection'; +import { localize } from '../../../../../../localize'; + +class FakeChildProcess extends EventEmitter { + public readonly stdin = new Writable({ + write: (_chunk, _encoding, callback) => callback(), + }); + + public readonly stdinEnd = vi.spyOn(this.stdin, 'end'); +} + +const createConnectionInfo = ( + rootFolder = String.raw`\\server\share`, + username = 'domain\\user', + password = 'password' +): FileSystemConnectionInfo => ({ + displayName: 'File system connection', + connectionParameters: { + rootFolder, + username, + password, + }, +}); + +const spawnFake = (): FakeChildProcess => { + const child = new FakeChildProcess(); + vi.mocked(childProcess.spawn).mockReturnValue(child as unknown as childProcess.ChildProcessWithoutNullStreams); + return child; +}; + +const getSpawnArguments = (): string[] => vi.mocked(childProcess.spawn).mock.calls[0][1] as string[]; + +describe('createFileSystemConnection', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(process.report, 'getReport').mockReturnValue({ + sharedObjects: [String.raw`D:\Windows\System32\KERNEL32.DLL`], + } as ReturnType); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllEnvs(); + }); + + it('passes credentials through stdin instead of the process command line', async () => { + const rootFolder = String.raw`\\server\share & "quoted"`; + const username = String.raw`domain\user|name`; + const password = 'secret & | % ^ ` $(command) "value"'; + const connectionInfo = createConnectionInfo(rootFolder, username, password); + const child = spawnFake(); + + const resultPromise = createFileSystemConnection(connectionInfo); + const spawnArguments = getSpawnArguments(); + const encodedCommand = spawnArguments.at(-1); + const decodedCommand = Buffer.from(encodedCommand ?? '', 'base64').toString('utf16le'); + + expect(childProcess.spawn).toHaveBeenCalledWith( + String.raw`D:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe`, + ['-NoLogo', '-NoProfile', '-NonInteractive', '-EncodedCommand', expect.any(String)], + { + shell: false, + stdio: ['pipe', 'ignore', 'ignore'], + windowsHide: true, + } + ); + expect(JSON.stringify(spawnArguments)).not.toContain(rootFolder); + expect(JSON.stringify(spawnArguments)).not.toContain(username); + expect(JSON.stringify(spawnArguments)).not.toContain(password); + expect(decodedCommand).toContain('[Console]::OpenStandardInput()'); + expect(decodedCommand).toContain('WNetAddConnection2'); + expect(decodedCommand).not.toContain(rootFolder); + expect(decodedCommand).not.toContain(username); + expect(decodedCommand).not.toContain(password); + expect(child.stdinEnd).toHaveBeenCalledWith(JSON.stringify({ rootFolder, username, password }), 'utf8'); + + child.emit('close', 0); + const result = await resultPromise; + expect(result).toEqual({ + connection: { + ...connectionInfo, + connectionParameters: { mountPath: rootFolder }, + }, + }); + expect(JSON.stringify(result)).not.toContain(username); + expect(JSON.stringify(result)).not.toContain(password); + }); + + it('uses identical process arguments for different credentials', async () => { + const firstChild = new FakeChildProcess(); + const secondChild = new FakeChildProcess(); + vi.mocked(childProcess.spawn) + .mockReturnValueOnce(firstChild as unknown as childProcess.ChildProcessWithoutNullStreams) + .mockReturnValueOnce(secondChild as unknown as childProcess.ChildProcessWithoutNullStreams); + + const firstResult = createFileSystemConnection(createConnectionInfo(String.raw`\\first\share`, 'first-user', 'first-password')); + const firstArguments = vi.mocked(childProcess.spawn).mock.calls[0][1]; + firstChild.emit('close', 0); + + const secondResult = createFileSystemConnection(createConnectionInfo(String.raw`\\second\share`, 'second-user', 'second-password')); + const secondArguments = vi.mocked(childProcess.spawn).mock.calls[1][1]; + secondChild.emit('close', 0); + + await Promise.all([firstResult, secondResult]); + expect(firstArguments).toEqual(secondArguments); + }); + + it('returns a stable error without exposing process errors', async () => { + const connectionInfo = createConnectionInfo(); + const { rootFolder, username, password } = connectionInfo.connectionParameters ?? {}; + const child = spawnFake(); + + const resultPromise = createFileSystemConnection(connectionInfo); + child.emit('error', new Error(`Command failed for ${rootFolder} ${username} ${password}`)); + + const result = await resultPromise; + expect(result).toEqual({ + errorMessage: 'Unable to connect to the file system. Verify the connection details and try again.', + }); + expect(JSON.stringify(result)).not.toContain(rootFolder); + expect(JSON.stringify(result)).not.toContain(username); + expect(JSON.stringify(result)).not.toContain(password); + }); + + it('returns a stable error for nonzero exit codes', async () => { + const connectionInfo = createConnectionInfo(); + const { rootFolder, username, password } = connectionInfo.connectionParameters ?? {}; + const child = spawnFake(); + + const resultPromise = createFileSystemConnection(connectionInfo); + child.emit('close', 1); + + const result = await resultPromise; + expect(result).toEqual({ + errorMessage: 'Unable to connect to the file system. Verify the connection details and try again.', + }); + expect(JSON.stringify(result)).not.toContain(rootFolder); + expect(JSON.stringify(result)).not.toContain(username); + expect(JSON.stringify(result)).not.toContain(password); + }); + + it('does not spawn a process when required parameters are missing', async () => { + const result = await createFileSystemConnection({ connectionParameters: {} }); + + expect(result).toEqual({ + errorMessage: 'Unable to connect to the file system. Verify the connection details and try again.', + }); + expect(childProcess.spawn).not.toHaveBeenCalled(); + }); + + it('ignores an overridden system root when selecting PowerShell', async () => { + vi.stubEnv('SystemRoot', String.raw`C:\Users\attacker\..\redirected`); + const child = spawnFake(); + + const resultPromise = createFileSystemConnection(createConnectionInfo()); + child.emit('close', 0); + + await expect(resultPromise).resolves.toHaveProperty('connection'); + expect(childProcess.spawn).toHaveBeenCalledWith( + String.raw`D:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe`, + expect.any(Array), + expect.any(Object) + ); + }); + + it('does not spawn a process when the OS system directory cannot be resolved', async () => { + vi.mocked(process.report.getReport).mockReturnValue({ + sharedObjects: [], + } as ReturnType); + + const result = await createFileSystemConnection(createConnectionInfo()); + + expect(result).toEqual({ + errorMessage: 'Unable to connect to the file system. Verify the connection details and try again.', + }); + expect(childProcess.spawn).not.toHaveBeenCalled(); + }); + + it('localizes sanitized errors', async () => { + const result = await createFileSystemConnection({ connectionParameters: {} }); + + expect(localize).toHaveBeenCalledWith( + 'fileSystemConnectionFailed', + 'Unable to connect to the file system. Verify the connection details and try again.' + ); + expect(result.errorMessage).toBe('Unable to connect to the file system. Verify the connection details and try again.'); + }); +}); diff --git a/apps/vs-code-designer/src/app/commands/workflows/designer/utils/fileSystemConnection.ts b/apps/vs-code-designer/src/app/commands/workflows/designer/utils/fileSystemConnection.ts index 51a8bc9e54b..24d27675621 100644 --- a/apps/vs-code-designer/src/app/commands/workflows/designer/utils/fileSystemConnection.ts +++ b/apps/vs-code-designer/src/app/commands/workflows/designer/utils/fileSystemConnection.ts @@ -3,28 +3,132 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ import type { FileSystemConnectionInfo } from '@microsoft/vscode-extension-logic-apps'; -import { exec } from 'child_process'; +import { spawn } from 'child_process'; +import { win32 as path } from 'path'; +import { localize } from '../../../../../localize'; + +const getFileSystemConnectionError = (): string => + localize('fileSystemConnectionFailed', 'Unable to connect to the file system. Verify the connection details and try again.'); + +const hasSharedObjects = (report: object): report is { sharedObjects: string[] } => + 'sharedObjects' in report && + Array.isArray(report.sharedObjects) && + report.sharedObjects.every((sharedObjectPath) => typeof sharedObjectPath === 'string'); + +const getWindowsPowerShellPath = (): string | undefined => { + const report = process.report?.getReport(); + if (!report || !hasSharedObjects(report)) { + return undefined; + } + + const kernel32Path = report.sharedObjects.find((sharedObjectPath) => path.basename(sharedObjectPath).toLowerCase() === 'kernel32.dll'); + + return kernel32Path ? path.join(path.dirname(kernel32Path), 'WindowsPowerShell', 'v1.0', 'powershell.exe') : undefined; +}; + +const CONNECT_FILE_SYSTEM_SCRIPT = ` +$ErrorActionPreference = 'Stop' + +Add-Type -TypeDefinition @' +using System; +using System.Runtime.InteropServices; + +public static class NetworkConnection +{ + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] + public struct NETRESOURCE + { + public int dwScope; + public int dwType; + public int dwDisplayType; + public int dwUsage; + public string lpLocalName; + public string lpRemoteName; + public string lpComment; + public string lpProvider; + } + + [DllImport("mpr.dll", EntryPoint = "WNetAddConnection2W", CharSet = CharSet.Unicode)] + public static extern int WNetAddConnection2( + ref NETRESOURCE netResource, + string password, + string username, + int flags); +} +'@ + +$inputStream = [Console]::OpenStandardInput() +$reader = [System.IO.StreamReader]::new($inputStream, [System.Text.Encoding]::UTF8) +$connection = $reader.ReadToEnd() | ConvertFrom-Json + +$resource = [NetworkConnection+NETRESOURCE]::new() +$resource.dwType = 1 +$resource.lpRemoteName = [string]$connection.rootFolder + +$result = [NetworkConnection]::WNetAddConnection2( + [ref]$resource, + [string]$connection.password, + [string]$connection.username, + 0) + +if ($result -ne 0) { + exit 1 +} +`; + +const ENCODED_CONNECT_FILE_SYSTEM_SCRIPT = Buffer.from(CONNECT_FILE_SYSTEM_SCRIPT, 'utf16le').toString('base64'); + +interface FileSystemConnectionResult { + connection?: FileSystemConnectionInfo; + errorMessage?: string; +} /** - * Creates a file system connection by mapping a network drive via `net use`. + * Creates a file system connection without exposing credentials in process arguments or errors. */ -export function createFileSystemConnection(connectionInfo: FileSystemConnectionInfo): Promise { +export function createFileSystemConnection(connectionInfo: FileSystemConnectionInfo): Promise { const rootFolder = connectionInfo.connectionParameters?.['rootFolder']; const username = connectionInfo.connectionParameters?.['username']; const password = connectionInfo.connectionParameters?.['password']; + const powershellPath = getWindowsPowerShellPath(); + + if (typeof rootFolder !== 'string' || typeof username !== 'string' || typeof password !== 'string' || powershellPath === undefined) { + return Promise.resolve({ errorMessage: getFileSystemConnectionError() }); + } return new Promise((resolve) => { - exec(`net use ${rootFolder} ${password} /user:${username}`, (error) => { - if (error) { - resolve({ errorMessage: JSON.stringify(error.message) }); - } else { + const childProcess = spawn( + powershellPath, + ['-NoLogo', '-NoProfile', '-NonInteractive', '-EncodedCommand', ENCODED_CONNECT_FILE_SYSTEM_SCRIPT], + { + shell: false, + stdio: ['pipe', 'ignore', 'ignore'], + windowsHide: true, + } + ); + let settled = false; + + const finish = (succeeded: boolean): void => { + if (settled) { + return; + } + + settled = true; + if (succeeded) { resolve({ connection: { ...connectionInfo, connectionParameters: { mountPath: rootFolder }, }, }); + } else { + resolve({ errorMessage: getFileSystemConnectionError() }); } - }); + }; + + childProcess.once('error', () => finish(false)); + childProcess.once('close', (exitCode) => finish(exitCode === 0)); + childProcess.stdin.once('error', () => finish(false)); + childProcess.stdin.end(JSON.stringify({ rootFolder, username, password }), 'utf8'); }); } diff --git a/apps/vs-code-react/src/app/services/Logger.spec.ts b/apps/vs-code-react/src/app/services/Logger.spec.ts index 2edd46a94da..f3b5a45f03d 100644 --- a/apps/vs-code-react/src/app/services/Logger.spec.ts +++ b/apps/vs-code-react/src/app/services/Logger.spec.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { ExtensionCommand } from '@microsoft/vscode-extension-logic-apps'; import { LogEntryLevel } from '@microsoft/logic-apps-shared'; import { LoggerService } from './Logger'; @@ -14,6 +14,7 @@ describe('DataMapperLoggerService', () => { afterEach(() => { vi.clearAllMocks(); + vi.useRealTimers(); }); it('should log telemetry event', () => { @@ -28,17 +29,10 @@ describe('DataMapperLoggerService', () => { command: 'logTelemetry', data: { area: 'testEvent', - args: [ - 'arg1', - 'arg2', - { - dataMapperVersion: 2, - designerVersion: '1.0.0', - }, - ], - level: LogEntryLevel.Verbose, + args: JSON.stringify(['arg1', 'arg2', context]), + level: String(LogEntryLevel.Verbose), message: 'test message', - timestamp: expect.any(Number), + timestamp: expect.any(String), }, }); }); @@ -55,9 +49,9 @@ describe('DataMapperLoggerService', () => { actionModifier: 'start', name: 'testTrace', source: 'testSource', - timestamp: expect.any(Number), - duration: 0, - data: { id: traceId, context }, + timestamp: expect.any(String), + duration: '0', + data: JSON.stringify({ id: traceId, context }), }), }); }); @@ -78,9 +72,9 @@ describe('DataMapperLoggerService', () => { actionModifier: 'end', name: 'testTrace', source: 'testSource', - timestamp: expect.any(Number), - duration: expect.any(Number), - data: { additional: 'info', context, id: traceId }, + timestamp: expect.any(String), + duration: expect.any(String), + data: JSON.stringify({ additional: 'info', context, id: traceId }), }), }); }); @@ -90,4 +84,42 @@ describe('DataMapperLoggerService', () => { expect(mockSendMsgToVsix).not.toHaveBeenCalled(); }); + + it('does not send raw error details as telemetry properties', () => { + loggerService.log({ + level: LogEntryLevel.Error, + area: 'createConnection', + message: 'Unable to connect', + error: new Error('GLASSWING_TEST_ONLY_PASSWORD'), + }); + + expect(mockSendMsgToVsix).toHaveBeenCalledWith({ + command: ExtensionCommand.logTelemetry, + data: expect.objectContaining({ + error: JSON.stringify({ name: 'Error' }), + message: 'Unable to connect', + }), + }); + expect(JSON.stringify(mockSendMsgToVsix.mock.calls)).not.toContain('GLASSWING_TEST_ONLY_PASSWORD'); + }); + + it('does not throw when telemetry data is circular', () => { + const circular: Record = {}; + circular['self'] = circular; + + expect(() => + loggerService.log({ + level: LogEntryLevel.Error, + area: 'createConnection', + message: 'Unable to connect', + args: [circular], + }) + ).not.toThrow(); + expect(mockSendMsgToVsix).toHaveBeenCalledWith({ + command: ExtensionCommand.logTelemetry, + data: expect.objectContaining({ + args: '[Unable to serialize telemetry value]', + }), + }); + }); }); diff --git a/apps/vs-code-react/src/app/services/Logger.ts b/apps/vs-code-react/src/app/services/Logger.ts index 509c822c258..aab747b8061 100644 --- a/apps/vs-code-react/src/app/services/Logger.ts +++ b/apps/vs-code-react/src/app/services/Logger.ts @@ -13,6 +13,35 @@ export interface AdditionalContext { targetType?: string; } +const serializeTelemetryValue = (value: unknown): string | undefined => { + if (value === undefined) { + return undefined; + } + if (typeof value === 'string') { + return value; + } + if (typeof value === 'number' || typeof value === 'boolean' || typeof value === 'bigint') { + return String(value); + } + if (value instanceof Error) { + return JSON.stringify({ name: value.name }); + } + + try { + return JSON.stringify(value) ?? String(value); + } catch { + return '[Unable to serialize telemetry value]'; + } +}; + +const createTelemetryProperties = (data: Record): Record => + Object.fromEntries( + Object.entries(data).flatMap(([key, value]) => { + const serializedValue = serializeTelemetryValue(value); + return serializedValue === undefined ? [] : [[key, serializedValue]]; + }) + ); + /** * Starts measuring the duration of an event and returns its unique identifier. * @param eventName - A string denoting the name of the trace event to start. @@ -49,7 +78,7 @@ export class LoggerService implements ILoggerService { public log = (entry: Omit) => { this.sendMsgToVsix({ command: ExtensionCommand.logTelemetry, - data: { ...entry, timestamp: Date.now(), args: [...(entry.args ?? []), this.context] }, + data: createTelemetryProperties({ ...entry, timestamp: Date.now(), args: [...(entry.args ?? []), this.context] }), }); }; @@ -65,7 +94,13 @@ export class LoggerService implements ILoggerService { const startTimestamp = Date.now(); this.sendMsgToVsix({ command: ExtensionCommand.logTelemetry, - data: { ...eventData, timestamp: startTimestamp, actionModifier: 'start', duration: 0, data: { id, context: this.context } }, + data: createTelemetryProperties({ + ...eventData, + timestamp: startTimestamp, + actionModifier: 'start', + duration: 0, + data: { id, context: this.context }, + }), }); this.inProgressTraces.set(id, { data: eventData, startTimestamp }); @@ -87,13 +122,13 @@ export class LoggerService implements ILoggerService { this.inProgressTraces.delete(id); this.sendMsgToVsix({ command: ExtensionCommand.logTelemetry, - data: { + data: createTelemetryProperties({ ...traceData.data, timestamp: endTimestamp, actionModifier: 'end', duration: endTimestamp - traceData.startTimestamp, data: { ...eventData?.data, context: this.context, id }, - }, + }), }); };