Fix code scanning issues by Morgy93 · Pull Request #56 · OpenForgeProject/vscode-ddev-phpstan

Morgy93 · 2026-01-12T13:06:50Z

This pull request refactors how commands are executed within the DDEV environment by replacing usage of execSync with spawnSync and by consistently passing command arguments as arrays instead of strings. This improves security, reliability, and testability. The changes also update related tests and command-building logic to match the new approach, and adjust GitHub workflow permissions for better security.

DDEV command execution and utility improvements:

Refactored DdevUtils.execDdev to use spawnSync and accept command arguments as arrays, improving security and preventing shell injection. Enhanced error handling and environment variable management for tool execution. (src/shared/utils/ddev-utils.ts) [1] [2]
Updated related utility methods (hasDdevProject, isDdevRunning, isToolInstalled, validateDdevTool) to use the new argument array approach and spawnSync. (src/shared/utils/ddev-utils.ts) [1] [2] [3] [4]

PHPStan service and base service changes:

Updated BasePhpToolService and PhpstanService to build tool commands as string arrays instead of strings, aligning with the new execution model. (src/shared/services/base-php-tool-service.ts, src/services/phpstan-service.ts) [1] [2]
Adjusted logging to print the joined command array for clarity. (src/shared/services/base-php-tool-service.ts)

Test suite modernization:

Refactored tests in ddev-utils.test.ts to mock spawnSync and fs.existsSync instead of execSync, and updated all relevant test cases to use the new command format and improved assertions. (src/test/ddev-utils.test.ts) [1] [2] [3] [4] [5]

GitHub workflow security:

Added explicit permissions blocks to workflow YAML files to restrict token permissions, improving CI/CD security. (.github/workflows/ci.yml, .github/workflows/code-quality.yml, .github/workflows/dependabot.yml) [1] [2] [3]

src/test/ddev-utils.test.ts

…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

src/test/ddev-utils.test.ts

Copilot

Pull request overview

This pull request enhances security and code quality by refactoring command execution from execSync with shell strings to spawnSync with argument arrays, preventing potential shell injection vulnerabilities. The changes also add explicit GitHub workflow permissions following the principle of least privilege.

Changes:

Replaced execSync with spawnSync for safer command execution without shell interpretation
Converted command building from string concatenation to array-based arguments
Updated test suite to mock spawnSync and fs.existsSync instead of execSync

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/shared/utils/ddev-utils.ts	Refactored command execution to use `spawnSync` with array arguments and improved error handling
src/shared/services/base-php-tool-service.ts	Updated `buildToolCommand` signature to return string array instead of string
src/services/phpstan-service.ts	Converted command building from string concatenation to array construction
src/test/ddev-utils.test.ts	Updated tests to mock `spawnSync` and `fs.existsSync` with comprehensive test cases
.github/workflows/dependabot.yml	Added explicit permissions for pull requests and contents
.github/workflows/code-quality.yml	Added explicit read-only permissions for contents
.github/workflows/ci.yml	Added explicit read-only permissions for contents

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-12T13:11:33Z

src/shared/utils/ddev-utils.ts

+                throw enhancedError;
+            }
+
+            return result.stdout;


This line is unreachable code. If result.status === 0 (which is required to reach this line), and allowedExitCodes contains 0 (which it does by default), the function would have already returned at line 203. This line can only theoretically be reached if someone passes custom allowedExitCodes that doesn't include 0, which would be an unusual edge case. Consider removing this line or restructuring the logic for clarity.

Copilot · 2026-01-12T13:11:33Z

src/test/ddev-utils.test.ts

 import * as sinon from 'sinon';
 import { afterEach, beforeEach } from 'mocha';
 import { DdevUtils } from '../shared/utils/ddev-utils';
+import * as fs from 'fs';


These import statements are unused. The code uses require at lines 35-36 to import the same modules, which shadows these imports. Consider removing these unused import statements or using them directly instead of calling require.

Suggested change

import * as fs from 'fs';

Copilot · 2026-01-12T13:11:34Z

src/shared/utils/ddev-utils.ts

+            }
+
+            if (result.status !== 0) {
+                 // Enhance error message with more details


Inconsistent indentation: this comment line has an extra leading space. The comment should align with the code at the same indentation level.

Copilot · 2026-01-12T13:11:34Z

src/shared/utils/ddev-utils.ts

+
+            return result.stdout;
+        } catch (error: any) {
+             // If error was already thrown above, rethrow it


Inconsistent indentation: this comment line has an extra leading space. The comment should align with the code at the same indentation level.

…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

Morgy93 · 2026-01-12T13:14:40Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-01-12T13:14:49Z

@Morgy93 I've opened a new pull request, #57, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-12T13:16:47Z

src/shared/utils/ddev-utils.ts

                cwd: workspacePath,
-                stdio: 'ignore'
+                encoding: 'utf-8'
            });


The isDdevRunning method doesn't handle the case where result.status is null, which can occur when the spawn operation fails (e.g., if 'ddev' command is not found). This will incorrectly return false when it should be distinguishing between "ddev not running" and "ddev command not found". Consider explicitly checking for result.status === null or adding a check for result.error before the status check.

Suggested change

});

});

if (result.error) {

vscode.window.showErrorMessage(

'Failed to execute "ddev". Please ensure DDEV is installed and available in your PATH.',

);

return false;

}

if (result.status === null) {

vscode.window.showErrorMessage(

'The "ddev" command did not exit normally. Please check your DDEV installation.',

);

return false;

}

Copilot · 2026-01-12T13:16:47Z

src/services/phpstan-service.ts

                if (this.config.excludePaths && this.config.excludePaths.length > 0) {
                    for (const excludePath of this.config.excludePaths) {
-                        command += ` --exclude="${excludePath}"`;
+                        command.push(`--exclude=${excludePath}`);


The excludePath values should be properly quoted or escaped when pushed to the command array. The current implementation uses command.push(\--exclude=${excludePath}`)which concatenates the path into the option value. If excludePath contains special characters or spaces, this could cause issues. Consider using separate arguments:command.push('--exclude', excludePath)` to let the shell handle quoting properly.

Suggested change

command.push(`--exclude=${excludePath}`);

command.push('--exclude', excludePath);

Copilot · 2026-01-12T13:16:48Z

src/services/phpstan-service.ts

            } else {
                // Use individual settings when no config file is found
-                command += ` --level=${this.config.level}`;
+                command.push(`--level=${this.config.level}`);


Similar to the exclude path issue, the level value is concatenated into the option using command.push(\--level=${this.config.level}`). While this is likely safe for numeric level values, for consistency and safety with spawnSync's array-based approach, consider using separate arguments: command.push('--level', String(this.config.level))`.

Suggested change

command.push(`--level=${this.config.level}`);

command.push('--level', String(this.config.level));

…nd clean up imports Co-authored-by: Morgy93 <7961978+Morgy93@users.noreply.github.com>

Address code review findings: remove unreachable code and fix style issues

Morgy93 · 2026-01-12T13:24:38Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-01-12T13:24:45Z

@Morgy93 I've opened a new pull request, #58, to work on those changes. Once the pull request is ready, I'll request review from you.

…n options Co-authored-by: Morgy93 <7961978+Morgy93@users.noreply.github.com>

Handle null status in isDdevRunning and use separate args for PHPStan options

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-12T13:44:34Z

src/test/ddev-utils.test.ts

+        // Try to stub, but handle if it fails (basic check)
+        try {
+            spawnSyncStub = sandbox.stub(cp, 'spawnSync');
+            existsSyncStub = sandbox.stub(fs, 'existsSync');
+        } catch (e) {
+            console.error('Failed to stub modules:', e);
+            throw e;
+        }


The try-catch block in the test setup (lines 36-42) is unnecessary and adds complexity. If stubbing fails, it's a test setup issue that should fail immediately. The explicit error logging and re-throwing doesn't add value since the test framework will handle the error appropriately.

Suggested change

// Try to stub, but handle if it fails (basic check)

try {

spawnSyncStub = sandbox.stub(cp, 'spawnSync');

existsSyncStub = sandbox.stub(fs, 'existsSync');

} catch (e) {

console.error('Failed to stub modules:', e);

throw e;

}

spawnSyncStub = sandbox.stub(cp, 'spawnSync');

existsSyncStub = sandbox.stub(fs, 'existsSync');

Copilot · 2026-01-12T13:44:34Z

src/test/ddev-utils.test.ts

    test('isDdevRunning returns true when DDEV container is running', () => {
-        execSyncStub.returns('');
+        spawnSyncStub.returns({ status: 0, stdout: 'test\n' });

        const result = DdevUtils.isDdevRunning('/test/workspace');

        assert.strictEqual(result, true);
    });

    test('isDdevRunning returns false when DDEV container is not running', () => {
-        execSyncStub.throws(new Error('Container not running'));
+        spawnSyncStub.returns({ status: 1, stderr: 'Container not running' });

        const result = DdevUtils.isDdevRunning('/test/workspace');

        assert.strictEqual(result, false);
    });


The tests for isDdevRunning don't cover the error handling paths where result.error is truthy (line 70-74 in implementation) or where result.status is null (line 76-80 in implementation). These error conditions should be tested to ensure the error messages are shown correctly and the method returns false as expected.

Copilot · 2026-01-12T13:44:35Z

src/test/ddev-utils.test.ts

+    test('execDdev passes args array correctly', () => {
+        spawnSyncStub.returns({ status: 0, stdout: 'output' });

-        const result = DdevUtils.execDdev('phpstan analyze', '/test/workspace');
+        const result = DdevUtils.execDdev(['phpstan', 'analyze'], '/test/workspace');

        assert.strictEqual(result, 'output');
-        assert.strictEqual(execSyncStub.calledOnce, true);
-        const callArgs = execSyncStub.firstCall.args;
-        assert.ok(callArgs[0].includes("XDEBUG_MODE=off"));
-        assert.ok(callArgs[0].includes("bash -c"));
-        assert.ok(callArgs[0].includes("'XDEBUG_MODE=off phpstan analyze'"));
+        assert.strictEqual(spawnSyncStub.calledOnce, true);
+        const callArgs = spawnSyncStub.firstCall.args;
+        assert.strictEqual(callArgs[0], 'ddev');
+        assert.deepStrictEqual(callArgs[1], ['exec', 'env', 'XDEBUG_MODE=off', 'phpstan', 'analyze']);
+        assert.strictEqual(callArgs[2].cwd, '/test/workspace');
    });

-    test('execDdev escapes single quotes in command', () => {
-        execSyncStub.returns('output');
+    test('execDdev throws on non-zero exit code', () => {
+        spawnSyncStub.returns({ status: 1, stderr: 'error', stdout: '' });

-        const result = DdevUtils.execDdev("echo 'hello'", '/test/workspace');
+        assert.throws(() => {
+            DdevUtils.execDdev(['ls'], '/test/workspace');
+        }, (err: any) => {
+            return err.status === 1 && err.stderr === 'error';
+        });
+    });

-        assert.strictEqual(result, 'output');
-        const callArgs = execSyncStub.firstCall.args;
-        // Should be: ddev exec bash -c 'XDEBUG_MODE=off echo '\''hello'\'''
-        assert.ok(callArgs[0].includes("'XDEBUG_MODE=off echo '\\''hello'\\'''"));
+    test('execDdev returns stdout on allowed non-zero exit code', () => {
+        spawnSyncStub.returns({ status: 1, stdout: 'partial output' });
+
+        const result = DdevUtils.execDdev(['ls'], '/test/workspace', [0, 1]);
+
+        assert.strictEqual(result, 'partial output');
    });


Test coverage is missing for execDdev error handling when result.error is present (line 205-206 in implementation) and when result.status is null (which would cause the function to exit without returning). These edge cases should be tested to ensure proper error handling.

Copilot · 2026-01-12T13:44:35Z

src/shared/utils/ddev-utils.ts

+            // Check if this is an acceptable exit code
+            if (result.status !== null && allowedExitCodes.includes(result.status)) {
                // Return stdout even if exit code is non-zero but allowed
-                console.log(`Command exited with allowed code ${error.status}: ${command}`);
-                return error.stdout || '';
+                if (result.status !== 0) {
+                    console.log(`Command exited with allowed code ${result.status}: ${command.join(' ')}`);
+                }
+                return result.stdout || '';
+            }
+
+            if (result.status !== 0) {
+                // Enhance error message with more details
+                const enhancedError = new Error(result.stderr || 'Command execution failed');
+                enhancedError.name = 'CommandError';
+                (enhancedError as any).status = result.status;
+                (enhancedError as any).stderr = result.stderr;
+                (enhancedError as any).stdout = result.stdout;
+                (enhancedError as any).command = `ddev exec ${command.join(' ')}`;
+                (enhancedError as any).workspacePath = workspacePath;
+                throw enhancedError;
+            }


The execDdev method has a logical gap in its exit code handling. If result.status === 0 but 0 is not in the allowedExitCodes array (line 210 condition fails), and the check at line 218 (result.status !== 0) also fails, the function exits the try block without returning a value. While this is unlikely with the default allowedExitCodes = [0], it could occur if a caller explicitly sets allowedExitCodes to exclude 0 (e.g., [1, 2]). The function should either throw an error or return stdout in this case.

Copilot · 2026-01-12T13:44:35Z

src/shared/utils/ddev-utils.ts

+                vscode.window.showErrorMessage(
+                    'Failed to execute "ddev". Please ensure DDEV is installed and available in your PATH.',
+                );
+                return false;
+            }
+            if (result.status === null) {
+                vscode.window.showErrorMessage(
+                    'The "ddev" command did not exit normally. Please check your DDEV installation.',
+                );
+                return false;
+            }
+            return result.status === 0;
        } catch (error) {


The isDdevRunning method shows error messages to the user via vscode.window.showErrorMessage even though it's meant to be a simple boolean check. This creates unexpected UI popups during validation flows. The error messages should be logged instead, or the method should return more detailed information that callers can use to show appropriate messages.

Suggested change

vscode.window.showErrorMessage(

'Failed to execute "ddev". Please ensure DDEV is installed and available in your PATH.',

);

return false;

}

if (result.status === null) {

vscode.window.showErrorMessage(

'The "ddev" command did not exit normally. Please check your DDEV installation.',

);

return false;

}

return result.status === 0;

} catch (error) {

console.error(

'Failed to execute "ddev". Please ensure DDEV is installed and available in your PATH.',

result.error

);

return false;

}

if (result.status === null) {

console.error(

'The "ddev" command did not exit normally. Please check your DDEV installation.',

result.stderr

);

return false;

}

return result.status === 0;

} catch (error) {

console.error('Unexpected error while checking DDEV status.', error);

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-12T14:01:42Z

src/shared/utils/ddev-utils.ts

+                (enhancedError as any).command = `ddev exec ${command.join(' ')}`;
+                (enhancedError as any).workspacePath = workspacePath;
+                throw enhancedError;
+            }


The function has a logic gap: if result.status is 0 but 0 is not in allowedExitCodes, the function will not return or throw, resulting in an implicit undefined return. After line 223 returns for allowed exit codes, add an explicit return statement for the case where result.status === 0, or restructure the logic to ensure all code paths return a value or throw an error. Consider checking if result.status === 0 first and returning the stdout, then checking allowedExitCodes for non-zero statuses.

Suggested change

}

}

// If we reach this point, the command exited successfully with status 0.

// Return stdout to avoid an implicit undefined return value.

return result.stdout || '';

Copilot · 2026-01-12T14:01:43Z

src/shared/utils/ddev-utils.ts

+                (enhancedError as any).status = result.status;
+                (enhancedError as any).stderr = result.stderr;
+                (enhancedError as any).stdout = result.stdout;
+                (enhancedError as any).command = `ddev exec ${command.join(' ')}`;


The reconstructed command string does not include the 'env' and 'XDEBUG_MODE=off' arguments that are actually part of the executed command (see line 206). For better debugging and error messages, consider including these in the command string, such as: ddev exec env XDEBUG_MODE=off ${command.join(' ')}

src/shared/utils/ddev-utils.ts

Morgy93 added 2 commits January 12, 2026 14:01

♻️ refactor: update command building to use array for PHPStan service

32a8962

♻️ chore: add permissions to CI and code quality workflows

175ac46

Copilot AI review requested due to automatic review settings January 12, 2026 13:06

Copilot started reviewing on behalf of Morgy93 January 12, 2026 13:07 View session

github-code-quality bot found potential problems Jan 12, 2026

View reviewed changes

src/test/ddev-utils.test.ts Fixed Show fixed Hide fixed

src/test/ddev-utils.test.ts Fixed Show fixed Hide fixed

Potential fix for pull request finding 'Unused variable, import, func…

1ebe291

…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

github-code-quality bot found potential problems Jan 12, 2026

View reviewed changes

src/test/ddev-utils.test.ts Fixed Show fixed Hide fixed

Copilot AI reviewed Jan 12, 2026

View reviewed changes

Potential fix for pull request finding 'Unused variable, import, func…

10672e9

…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

Copilot AI review requested due to automatic review settings January 12, 2026 13:12

Copilot started reviewing on behalf of Morgy93 January 12, 2026 13:13 View session

Initial plan

cdd7261

Copilot AI mentioned this pull request Jan 12, 2026

Address code review findings: remove unreachable code and fix style issues #57

Merged

Copilot AI reviewed Jan 12, 2026

View reviewed changes

Copilot AI and others added 2 commits January 12, 2026 13:18

Fix code review comments: remove unreachable code, fix indentation, a…

2fed5e9

…nd clean up imports Co-authored-by: Morgy93 <7961978+Morgy93@users.noreply.github.com>

Merge pull request #57 from OpenForgeProject/copilot/sub-pr-56

f7f59d2

Address code review findings: remove unreachable code and fix style issues

Initial plan

7ee6ecd

Copilot AI mentioned this pull request Jan 12, 2026

Handle null status in isDdevRunning and use separate args for PHPStan options #58

Merged

Copilot AI and others added 2 commits January 12, 2026 13:27

Fix review comments: handle null status, use separate args for PHPSta…

ec73220

…n options Co-authored-by: Morgy93 <7961978+Morgy93@users.noreply.github.com>

Merge pull request #58 from OpenForgeProject/copilot/sub-pr-56

6eb6794

Handle null status in isDdevRunning and use separate args for PHPStan options

Copilot AI review requested due to automatic review settings January 12, 2026 13:39

Copilot started reviewing on behalf of Morgy93 January 12, 2026 13:40 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

Morgy93 added 2 commits January 12, 2026 14:48

♻️ refactor: replace direct fs and cp calls with sys wrapper

2c63888

♻️ chore: update permissions for pull requests in CI workflows

0692e5e

Copilot AI review requested due to automatic review settings January 12, 2026 13:58

Copilot started reviewing on behalf of Morgy93 January 12, 2026 13:58 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

♻️ chore: update vscode engine version to ^1.108.0

e54c64a

Morgy93 merged commit 35f692a into main Jan 12, 2026
15 checks passed

Morgy93 deleted the fix-code-scanning branch January 12, 2026 14:11

	command.push(`--exclude=${excludePath}`);
	command.push('--exclude', excludePath);

	command.push(`--level=${this.config.level}`);
	command.push('--level', String(this.config.level));

-            }
+            }
+            // If we reach this point, the command exited successfully with status 0.
+            // Return stdout to avoid an implicit undefined return value.
+            return result.stdout || '';

Conversation

Morgy93 commented Jan 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Morgy93 commented Jan 12, 2026

Uh oh!

Copilot AI commented Jan 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Morgy93 commented Jan 12, 2026

Uh oh!

Copilot AI commented Jan 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone