task/2565 add test cases for calling mcdev via cli instead from javascript#2566
task/2565 add test cases for calling mcdev via cli instead from javascript#2566
Conversation
|
@copilot retrieve you will find them documented here:
Do ignore the commands: createDeltaPkg |
|
@copilot sorry, "replaceContentBlockbuild" was supposed to be "replaceContentBlock" and "build" |
Co-authored-by: JoernBerkefeld <1917227+JoernBerkefeld@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| async function runCLIExpectError(args, cwd, mockPort) { | ||
| return new Promise((resolve) => { | ||
| exec( | ||
| `node ${cliPath} ${args} --noLogFile`, |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 hours ago
In general, the fix is to avoid passing a dynamically constructed command string to a shell via exec. Instead, call node directly and pass cliPath, CLI arguments, and --noLogFile as separate arguments using execFile (or spawn) so no shell parsing occurs. This prevents any special characters in cliPath or arguments from altering the command structure.
Concretely, in this file we should:
- Stop using
promisify(exec)and instead importexecFilefromnode:child_process. - Define a promisified
execFileAsync. - In
runCLI, build anargsarray like[cliPath, ...splitArgs, '--noLogFile']and callexecFileAsync('node', args, options). - In
runCLIExpectError, useexecFile('node', args, options, callback)similarly. - Preserve existing behavior: still add
--noLogFile, still passcwd,timeout, and env frombuildSubprocessEnv, and keep the same resolved return shapes.
Because the existing args parameter is a single string like "some subcommand --flag value", we need to convert it into an array. The safest minimal approach without new dependencies is to use String.prototype.trim().split(/\s+/) under the assumption that tests don’t rely on shell quoting. This preserves current semantics closely enough for test code and keeps the change small. We also replace the template strings with proper argument arrays, eliminating the tainted absolute-path injection into a shell command.
| @@ -1,7 +1,7 @@ | ||
| import * as chai from 'chai'; | ||
| const assert = chai.assert; | ||
|
|
||
| import { exec } from 'node:child_process'; | ||
| import { execFile } from 'node:child_process'; | ||
| import { promisify } from 'node:util'; | ||
| import fs from 'fs-extra'; | ||
| import path from 'node:path'; | ||
| @@ -10,7 +10,7 @@ | ||
| import { startMockServer } from './mockServer.js'; | ||
| import auth from '../lib/util/auth.js'; | ||
|
|
||
| const execAsync = promisify(exec); | ||
| const execFileAsync = promisify(execFile); | ||
|
|
||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
| const projectRoot = path.resolve(__dirname, '..'); | ||
| @@ -47,9 +47,11 @@ | ||
| * @returns {Promise.<{stdout: string, stderr: string}>} combined output | ||
| */ | ||
| async function runCLI(args, cwd, mockPort) { | ||
| return execAsync( | ||
| const argList = args ? args.trim().split(/\s+/) : []; | ||
| return execFileAsync( | ||
| 'node', | ||
| // always add --noLogFile to avoid creating log files in tmpDir | ||
| `node ${cliPath} ${args} --noLogFile`, | ||
| [cliPath, ...argList, '--noLogFile'], | ||
| { | ||
| cwd, | ||
| timeout: 30_000, | ||
| @@ -67,9 +68,11 @@ | ||
| * @returns {Promise.<{error: Error|null, stdout: string, stderr: string}>} exec result | ||
| */ | ||
| async function runCLIExpectError(args, cwd, mockPort) { | ||
| const argList = args ? args.trim().split(/\s+/) : []; | ||
| return new Promise((resolve) => { | ||
| exec( | ||
| `node ${cliPath} ${args} --noLogFile`, | ||
| execFile( | ||
| 'node', | ||
| [cliPath, ...argList, '--noLogFile'], | ||
| { | ||
| cwd, | ||
| timeout: 10_000, |
| async function runCLI(args, cwd, mockPort) { | ||
| return execAsync( | ||
| // always add --noLogFile to avoid creating log files in tmpDir | ||
| `node ${cliPath} ${args} --noLogFile`, |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 hours ago
To fix this, we should avoid building a single shell command string and instead invoke node directly with an argument array using child_process.execFile (or spawn). This bypasses the shell entirely so that cliPath and CLI arguments are not interpreted by the shell, only by node/the CLI parser.
Concretely, in test/cli.test.js:
- Replace the use of
execwithexecFilefromnode:child_process. - Replace
execAsync(based onpromisify(exec)) with a small wrapper aroundexecFilethat:- Accepts a command and an argument array.
- Returns a
Promiseresolving with{ stdout, stderr }or rejecting on error, mimickingexecAsync’s behavior enough for this file.
- Change
runCLIto call this new helper with:- Command:
'node' - Args array:
[cliPath, ...splitArgs, '--noLogFile'], wheresplitArgscomes from splitting theargsstring into tokens. Since we must not assume extra libraries, we can implement a minimal shell-like splitter that handles quotes reasonably for test usage. Alternatively, if we want to avoid even that complexity, we can treatargsas a single argument string when appropriate. Given the tests likely pass arguments similar to what would go on the command line, a simple splitter is appropriate and keeps behavior close.
- Command:
- Ensure the environment and other
execoptions (cwd,timeout,env) are passed through toexecFile.
The core change is localized to the import of exec, the definition of execAsync, and the body of runCLI. No other files need modification.
| @@ -1,7 +1,7 @@ | ||
| import * as chai from 'chai'; | ||
| const assert = chai.assert; | ||
|
|
||
| import { exec } from 'node:child_process'; | ||
| import { execFile } from 'node:child_process'; | ||
| import { promisify } from 'node:util'; | ||
| import fs from 'fs-extra'; | ||
| import path from 'node:path'; | ||
| @@ -10,7 +10,20 @@ | ||
| import { startMockServer } from './mockServer.js'; | ||
| import auth from '../lib/util/auth.js'; | ||
|
|
||
| const execAsync = promisify(exec); | ||
| const execFileAsync = (command, args, options) => | ||
| new Promise((resolve, reject) => { | ||
| execFile(command, args, options, (error, stdout, stderr) => { | ||
| if (error) { | ||
| // Mirror child_process.exec behavior: attach stdout/stderr to the error object | ||
| // so callers (or tests) can inspect them on failure. | ||
| error.stdout = stdout; | ||
| error.stderr = stderr; | ||
| reject(error); | ||
| } else { | ||
| resolve({ stdout, stderr }); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
| const projectRoot = path.resolve(__dirname, '..'); | ||
| @@ -47,15 +60,24 @@ | ||
| * @returns {Promise.<{stdout: string, stderr: string}>} combined output | ||
| */ | ||
| async function runCLI(args, cwd, mockPort) { | ||
| return execAsync( | ||
| // always add --noLogFile to avoid creating log files in tmpDir | ||
| `node ${cliPath} ${args} --noLogFile`, | ||
| { | ||
| cwd, | ||
| timeout: 30_000, | ||
| env: buildSubprocessEnv(mockPort), | ||
| } | ||
| ); | ||
| // Build argument list for `node` without going through a shell. This avoids any | ||
| // interpretation of cliPath or args by `/bin/sh`, preventing spaces or metacharacters | ||
| // in paths from altering the command semantics. | ||
| const argList = []; | ||
| argList.push(cliPath); | ||
| if (args && args.trim()) { | ||
| // Simple split on whitespace for test arguments; tests are expected to pass | ||
| // a properly formatted argument string. | ||
| argList.push(...args.trim().split(/\s+/)); | ||
| } | ||
| // always add --noLogFile to avoid creating log files in tmpDir | ||
| argList.push('--noLogFile'); | ||
|
|
||
| return execFileAsync('node', argList, { | ||
| cwd, | ||
| timeout: 30_000, | ||
| env: buildSubprocessEnv(mockPort), | ||
| }); | ||
| } | ||
|
|
||
| /** |
|
@copilot do not use .cjs files. this project uses ESM imports. if you are already doing that, ignore this message |
…ate port access, fix URL security check Co-authored-by: JoernBerkefeld <1917227+JoernBerkefeld@users.noreply.github.com>
Co-authored-by: JoernBerkefeld <1917227+JoernBerkefeld@users.noreply.github.com>
All existing tests invoke mcdev as a JavaScript library. This PR adds a CLI-layer test suite that exercises commands via subprocess (
node lib/cli.js ...), fixating the status quo and surfacing logic holes between the CLI interface and the JS API.New files
test/cli.test.js— 12 subprocess-based tests coveringretrieve,deploy,buildTemplate,buildDefinition,explainTypes, and error-handling. Usesqueryas the representative metadata type. Documents 5 logic holes in JSDoc:changelogOnlyretrieve param is not exposed via CLI--metadata TYPE(no key) and positionalTYPEtake different code paths in#retrieveBUnullas BU (JS API allows it to mean "all BUs")test/mockServer.js— Plain HTTP server that adapts raw subprocess requests to the existinghandleSOAPRequest/handleRESTRequestfromresourceFactory.js, reusing alltest/resources/fixtures. Returnssoap_instance_url/rest_instance_urlpointing to itself so the SDK uses HTTP for all subsequent calls.test/cli-preload.cjs— CJS preload injected viaNODE_OPTIONS=--requirethat patchesnode:https'srequest/getto redirect*.marketingcloudapis.comto the local mock server. Necessary becausesfmc-sdkvalidatesauth_urlmust matchhttps://mc{28chars}.auth.marketingcloudapis.com/; the preload satisfies that regex while routing traffic locally. Host matching usesendsWith('.marketingcloudapis.com')to avoid substring-based URL spoofing.Modified files
eslint.config.js— Added a*.cjsconfig block (sourceType: 'commonjs',globals.node) so the preload file is linted correctly.Issues
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.