Skip to content

Commit 9484784

Browse files
rllyy97Copilot
andauthored
fix(vscode): prevent SMB credential leakage into telemetry (#9648)
* fix(vscode): keep SMB credentials out of commands and telemetry Use WNetAddConnection2W through a bundled PowerShell helper with private stdin credential transport and fixed localized failure messages. Add credential-safety regressions and document Windows requirements. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * test(vscode): compare SMB transport fixtures byte for byte Replace the digest of synthetic credential fixtures with exact UTF-8 byte comparison. This makes the transport assertion stronger and removes the CodeQL password-hashing false positive without suppressing analysis. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(vscode): retain net use with credential-safe failures Keep SMB authentication on net.exe, discard its output, and contain raw process diagnostics before webview logging and telemetry. Remove the PowerShell adapter and document the remaining process-argument exposure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(vscode): align SMB completion error contract Always return a string errorMessage, including an empty string on success, to match both designer completion-message handlers. Add a typed regression against CompleteFileSystemConnectionData. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 04df5b5 commit 9484784

3 files changed

Lines changed: 304 additions & 16 deletions

File tree

‎apps/vs-code-designer/src/README.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,14 @@ Please note that this feature is currently in private preview and is only availa
6060

6161
For known issues, see [GitHub issues - Azure Logic Apps](https://github.com/Azure/LogicAppsUX/issues).
6262

63+
## Local SMB file system connections
64+
65+
Creating a local file system connection to an SMB share requires Windows. The extension runs `net.exe use` directly, without a shell, and returns the existing UNC `mountPath` on success. No PowerShell adapter or additional runtime dependency is required.
66+
67+
Process output is discarded, and failures return a fixed message. Raw process errors, command arguments, and diagnostic output are not forwarded to the designer, logs, or telemetry. If a connection fails, check the share path, credentials, network access, and any existing connections to the same server.
68+
69+
Passwords remain in the `net.exe` process arguments and may be visible to local process inspection or process-creation auditing. This protects the extension's error-reporting path, not command-line credential exposure. Standard `net use` password parsing still applies, including the special `*` password-prompt syntax; the extension does not provide interactive input.
70+
6371
## Providing feedback
6472

6573
- To open product bugs, go to [GitHub issues for Azure Logic Apps](https://github.com/Azure/LogicAppsUX/issues).
Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
import type { CompleteFileSystemConnectionData } from '@microsoft/vscode-extension-logic-apps';
2+
import { ChildProcess, type SpawnOptions } from 'child_process';
3+
import { platform } from 'os';
4+
import * as path from 'path';
5+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
6+
import { createFileSystemConnection } from '../fileSystemConnection';
7+
8+
const { mockSpawn } = vi.hoisted(() => ({ mockSpawn: vi.fn() }));
9+
10+
vi.mock('child_process', async () => ({
11+
...(await vi.importActual<typeof import('child_process')>('child_process')),
12+
spawn: mockSpawn,
13+
}));
14+
15+
vi.mock('os', () => ({ platform: vi.fn() }));
16+
vi.mock('../../../../../../localize', () => ({
17+
localize: (_key: string, message: string) => message,
18+
}));
19+
20+
const credentials = {
21+
rootFolder: '\\\\CWE532-ROOT\\share folder\\caf\u00e9',
22+
username: 'CWE532-USER\\user name',
23+
password: 'CWE532-PASSWORD \u00e9 "&|<>^%!\r\n$()',
24+
};
25+
const connectionInfo = { displayName: 'SMB connection', connectionParameters: credentials };
26+
const genericFailure = {
27+
errorMessage: 'Unable to create the file system connection. Check the SMB share, credentials, network access, and existing connections.',
28+
};
29+
30+
describe('createFileSystemConnection', () => {
31+
let child: ChildProcess;
32+
33+
beforeEach(() => {
34+
vi.clearAllMocks();
35+
vi.stubEnv('SystemRoot', 'C:\\Windows');
36+
vi.mocked(platform).mockReturnValue('win32');
37+
child = new ChildProcess();
38+
mockSpawn.mockReturnValue(child);
39+
});
40+
41+
afterEach(() => {
42+
child.removeAllListeners();
43+
vi.unstubAllEnvs();
44+
});
45+
46+
it('runs net.exe without a shell or diagnostic pipes and preserves the credential-free success contract', async () => {
47+
const result = createFileSystemConnection(connectionInfo);
48+
49+
expect(mockSpawn).toHaveBeenCalledWith(
50+
path.join('C:\\Windows', 'System32', 'net.exe'),
51+
['use', credentials.rootFolder, credentials.password, `/user:${credentials.username}`],
52+
{ shell: false, windowsHide: true, stdio: 'ignore' }
53+
);
54+
child.emit('close', 0, null);
55+
expect(await result).toEqual({
56+
errorMessage: '',
57+
connection: { displayName: connectionInfo.displayName, connectionParameters: { mountPath: credentials.rootFolder } },
58+
});
59+
expect(JSON.stringify(await result)).not.toContain('CWE532-PASSWORD');
60+
expect(JSON.stringify(await result)).not.toContain('CWE532-USER');
61+
expect(connectionInfo.connectionParameters).toEqual(credentials);
62+
});
63+
64+
it.each(['', '*', '/persistent:no', '"quoted"\\'])(
65+
'passes password %j as a separate argument without shell interpolation',
66+
async (password) => {
67+
const result = createFileSystemConnection({ connectionParameters: { ...credentials, password } });
68+
expect(mockSpawn.mock.lastCall?.[1]).toEqual(['use', credentials.rootFolder, password, `/user:${credentials.username}`]);
69+
child.emit('close', 0, null);
70+
expect(await result).toHaveProperty('connection.connectionParameters.mountPath', credentials.rootFolder);
71+
}
72+
);
73+
74+
it('returns an error string compatible with the completion message contract on success', async () => {
75+
const result = createFileSystemConnection(connectionInfo);
76+
child.emit('close', 0, null);
77+
const { connection, errorMessage } = await result;
78+
const completion: CompleteFileSystemConnectionData = {
79+
connectionName: 'smb-connection',
80+
connection,
81+
error: errorMessage,
82+
};
83+
expect(completion.error).toBe('');
84+
expect(completion.connection).toEqual({
85+
displayName: connectionInfo.displayName,
86+
connectionParameters: { mountPath: credentials.rootFolder },
87+
});
88+
});
89+
90+
it.each([1, 2, 5, 53, 1219, null])('returns only the fixed safe failure for exit code %s', async (code) => {
91+
const result = createFileSystemConnection(connectionInfo);
92+
child.emit('close', code, null);
93+
expect(await result).toEqual(genericFailure);
94+
});
95+
96+
it('fails on signal termination, even if a zero exit code is reported', async () => {
97+
const result = createFileSystemConnection(connectionInfo);
98+
child.emit('close', 0, 'SIGTERM');
99+
expect(await result).toEqual(genericFailure);
100+
});
101+
102+
it.each(['ENOENT', 'EACCES', 'EINVAL'])('contains all diagnostics for process error %s', async (code) => {
103+
const result = createFileSystemConnection(connectionInfo);
104+
const error = Object.assign(new Error(`Command failed: net use ${JSON.stringify(credentials)}`), {
105+
code,
106+
path: credentials.rootFolder,
107+
spawnargs: [credentials.password, credentials.username],
108+
stdout: credentials.password,
109+
stderr: credentials.password,
110+
cause: new Error(credentials.password),
111+
});
112+
child.emit('error', error);
113+
child.emit('close', null, null);
114+
expect(await result).toEqual(genericFailure);
115+
});
116+
117+
it('does not inspect or serialize raw errors', async () => {
118+
const error = new Error();
119+
Object.defineProperty(error, 'message', {
120+
get: () => {
121+
throw new Error(credentials.password);
122+
},
123+
});
124+
const result = createFileSystemConnection(connectionInfo);
125+
child.emit('error', error);
126+
child.emit('close', 1, null);
127+
expect(await result).toEqual(genericFailure);
128+
});
129+
130+
it('does not turn a process error into success or expose subsequent errors', async () => {
131+
const result = createFileSystemConnection(connectionInfo);
132+
child.emit('error', new Error(credentials.password));
133+
child.emit('close', 0, null);
134+
child.emit('error', new Error(credentials.username));
135+
expect(await result).toEqual(genericFailure);
136+
});
137+
138+
it('contains synchronous launch exceptions', async () => {
139+
mockSpawn.mockImplementation(() => {
140+
throw new Error(credentials.password);
141+
});
142+
expect(await createFileSystemConnection(connectionInfo)).toEqual(genericFailure);
143+
});
144+
145+
it.each([undefined, ''])('reports a safe failure when the Windows system directory is %j', async (systemRoot) => {
146+
vi.stubEnv('SystemRoot', systemRoot);
147+
expect(await createFileSystemConnection(connectionInfo)).toEqual(genericFailure);
148+
expect(mockSpawn).not.toHaveBeenCalled();
149+
});
150+
151+
it.each(['linux', 'darwin'] as const)('does not launch net.exe on %s', async (os) => {
152+
vi.mocked(platform).mockReturnValue(os);
153+
expect(await createFileSystemConnection(connectionInfo)).toEqual({
154+
errorMessage: 'File system connections to SMB shares are supported only on Windows.',
155+
});
156+
expect(mockSpawn).not.toHaveBeenCalled();
157+
});
158+
159+
it.each([
160+
undefined,
161+
{},
162+
{ ...credentials, rootFolder: undefined },
163+
{ ...credentials, rootFolder: 123 },
164+
{ ...credentials, rootFolder: 'C:\\local' },
165+
{ ...credentials, rootFolder: `${credentials.rootFolder}\0` },
166+
{ ...credentials, username: undefined },
167+
{ ...credentials, username: '' },
168+
{ ...credentials, username: 123 },
169+
{ ...credentials, username: `${credentials.username}\0` },
170+
{ ...credentials, password: undefined },
171+
{ ...credentials, password: 123 },
172+
{ ...credentials, password: `${credentials.password}\0` },
173+
])('rejects invalid parameters without echoing them or starting a process: %j', async (connectionParameters) => {
174+
const result = await createFileSystemConnection({ connectionParameters });
175+
expect(result).toEqual({
176+
errorMessage: 'Provide a valid UNC root folder, username, and password for the SMB share.',
177+
});
178+
expect(mockSpawn).not.toHaveBeenCalled();
179+
});
180+
181+
it.each([credentials.password, ''])(
182+
'preserves arguments and discards sensitive output through a real child process: %j',
183+
async (password) => {
184+
const cp = await vi.importActual<typeof import('child_process')>('child_process');
185+
const expectedArgs = ['use', credentials.rootFolder, password, `/user:${credentials.username}`];
186+
const script = `const expected = ${JSON.stringify(expectedArgs)};
187+
console.log(expected);
188+
console.error(expected);
189+
process.exit(JSON.stringify(process.argv.slice(1)) === JSON.stringify(expected) ? 0 : 1);`;
190+
mockSpawn.mockImplementation((_file: string, args: string[], options: SpawnOptions) => {
191+
child = cp.spawn(process.execPath, ['-e', script, '--', ...args], options);
192+
return child;
193+
});
194+
195+
expect(await createFileSystemConnection({ connectionParameters: { ...credentials, password } })).toEqual({
196+
errorMessage: '',
197+
connection: { connectionParameters: { mountPath: credentials.rootFolder } },
198+
});
199+
expect(child.stdin).toBeNull();
200+
expect(child.stdout).toBeNull();
201+
expect(child.stderr).toBeNull();
202+
},
203+
10000
204+
);
205+
206+
it('contains a real child-process launch error with sensitive spawnargs', async () => {
207+
const cp = await vi.importActual<typeof import('child_process')>('child_process');
208+
mockSpawn.mockImplementation((_file: string, args: string[], options: SpawnOptions) => {
209+
child = cp.spawn(path.join(__dirname, 'CWE532-missing-net.exe'), args, options);
210+
return child;
211+
});
212+
expect(await createFileSystemConnection(connectionInfo)).toEqual(genericFailure);
213+
}, 10000);
214+
215+
it.skipIf(process.platform !== 'win32')(
216+
'contains a real net.exe local-option failure without contacting a share',
217+
async () => {
218+
const cp = await vi.importActual<typeof import('child_process')>('child_process');
219+
mockSpawn.mockImplementation((file: string, _args: string[], options: SpawnOptions) => {
220+
child = cp.spawn(file, ['use', '/__logicapps_invalid_option__'], options);
221+
return child;
222+
});
223+
expect(await createFileSystemConnection(connectionInfo)).toEqual(genericFailure);
224+
},
225+
10000
226+
);
227+
});

‎apps/vs-code-designer/src/app/commands/workflows/designer/utils/fileSystemConnection.ts‎

Lines changed: 69 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,28 +3,81 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55
import type { FileSystemConnectionInfo } from '@microsoft/vscode-extension-logic-apps';
6-
import { exec } from 'child_process';
6+
import { spawn } from 'child_process';
7+
import { platform } from 'os';
8+
import * as path from 'path';
9+
import { localize } from '../../../../../localize';
710

8-
/**
9-
* Creates a file system connection by mapping a network drive via `net use`.
10-
*/
11-
export function createFileSystemConnection(connectionInfo: FileSystemConnectionInfo): Promise<any> {
11+
interface FileSystemConnectionResult {
12+
connection?: FileSystemConnectionInfo;
13+
errorMessage: string;
14+
}
15+
16+
export function createFileSystemConnection(connectionInfo: FileSystemConnectionInfo): Promise<FileSystemConnectionResult> {
1217
const rootFolder = connectionInfo.connectionParameters?.['rootFolder'];
1318
const username = connectionInfo.connectionParameters?.['username'];
1419
const password = connectionInfo.connectionParameters?.['password'];
1520

21+
if (platform() !== 'win32') {
22+
return Promise.resolve({
23+
errorMessage: localize('fileSystemConnectionWindowsOnly', 'File system connections to SMB shares are supported only on Windows.'),
24+
});
25+
}
26+
27+
if (
28+
typeof rootFolder !== 'string' ||
29+
!rootFolder.startsWith('\\\\') ||
30+
typeof username !== 'string' ||
31+
username.length === 0 ||
32+
typeof password !== 'string' ||
33+
[rootFolder, username, password].some((value) => value.includes('\0'))
34+
) {
35+
return Promise.resolve({
36+
errorMessage: localize(
37+
'fileSystemConnectionInvalidParameters',
38+
'Provide a valid UNC root folder, username, and password for the SMB share.'
39+
),
40+
});
41+
}
42+
1643
return new Promise((resolve) => {
17-
exec(`net use ${rootFolder} ${password} /user:${username}`, (error) => {
18-
if (error) {
19-
resolve({ errorMessage: JSON.stringify(error.message) });
20-
} else {
21-
resolve({
22-
connection: {
23-
...connectionInfo,
24-
connectionParameters: { mountPath: rootFolder },
25-
},
26-
});
44+
// Process errors can contain credentials in spawnargs; never forward them to webview telemetry.
45+
const fail = () =>
46+
resolve({
47+
errorMessage: localize(
48+
'fileSystemConnectionFailed',
49+
'Unable to create the file system connection. Check the SMB share, credentials, network access, and existing connections.'
50+
),
51+
});
52+
53+
try {
54+
const systemRoot = process.env.SystemRoot;
55+
if (!systemRoot) {
56+
fail();
57+
return;
2758
}
28-
});
59+
60+
const child = spawn(path.join(systemRoot, 'System32', 'net.exe'), ['use', rootFolder, password, `/user:${username}`], {
61+
shell: false,
62+
windowsHide: true,
63+
stdio: 'ignore',
64+
});
65+
child.on('error', fail);
66+
child.once('close', (code: number | null, signal: NodeJS.Signals | null) => {
67+
if (code !== 0 || signal) {
68+
fail();
69+
} else {
70+
resolve({
71+
errorMessage: '',
72+
connection: {
73+
...connectionInfo,
74+
connectionParameters: { mountPath: rootFolder },
75+
},
76+
});
77+
}
78+
});
79+
} catch {
80+
fail();
81+
}
2982
});
3083
}

0 commit comments

Comments
 (0)