Fix command and argument escaping and path spaces #172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In order to begin fixing the long-standing issue of spaces in paths and escaping, we first need to deal with the unmaintained package `cross-spawn`. Plus only fixer is using cross-spawn anyway. Removed: - Removed `cross-spawn` package and it's types package. Added: - Added proper typing to sniffer's spawn `options` using the `SpawnOptions` interface. - Added Node's `SpawnSync` to fixer and changed it's function call from the cross-spawn variant. - Added double quotes around the arguments values. While it works, this is just for now until we can properly escape them later.
yCodeTech
commented
Oct 1, 2025
Both `encoding` and `tty` options don't exist in `spawn` so they are safe to remove since they don't do anything.
- Fixed all the possibly null TS errors by wrapping each `stdin`, `stdout`, and `stderr` in condition checks to see if they are a truthy value (ie. not `null`). TS errors via GitHub Actions: https://github.com/valeryan/vscode-phpsab/actions/runs/18180905308/job/51756342015?pr=172
* feat: log the exit code, stdout, stderr, and node error to Output. For better debugging and error reporting, we now log all output streams to the Output channel when they are not empty. * fix: ts errors ts errors via github actions: https://github.com/valeryan/vscode-phpsab/actions/runs/18027596186/job/51297731350?pr=171 * fix: fixer logging of `STDOUT` only if it contains the string `ERROR`. - Added a check to see if `STDOUT` starts with the string `ERROR`. We only log it if it does. Also added a comment to document the reason behind it. * refactor: remove duplicate debug message logging in fixer `format` - Removed duplicate `showInformationMessage` calls in the fixer's format switch-case, and added 1 singular call at the bottom of the function. - Removed the conditional debugging for showing the information messages to the user since info like "fixed the errors" etc. should always be shown to the users for better UX. - Since case 0 is only logging the message that is already set before the switch-case, then we can now remove this case, as the message will be logged at the bottom of the function. * fix: fixer general error reporting - Change the multiple lone if statements to if...else if...else for improved readability since errors can't have multiple error codes, as it's one or the other. Also added a catch all else statement to log all other errors that the if statements didn't catch. * refactor: fixer error logging and add message logging. - Removed the logging of the `fixed` variable. - Added logging of the errors at the bottom of the `format` function. - Added logging of the fixer message if there are no errors. This logs the messages such as "fixed all errors", to the Output channel for better debugging. * revert: PR #58 which introduced error thrown when phpcbf returns empty. While the fix was to throw an error when phpcbf returned an empty document (text variable is an empty string), which prevented the file from being emptied. But this thrown error "PHPCBF returned an empty document" isn't necessarily descriptive enough for debugging. It relates to the file not needing formatting changes, which is already a message in the form of "No fixable errors were found". Because PR #160 fixes multiple issues with the promise logic, Copilot recommended "just resolving with no edits rather than throwing an error." This would also promote a better UX. - Reverted back to the just resolving with no edits instead of throwing an error. Note: This doesn't cause the original bug to come back and remove a file's content. * remove: old redundant vscode `editor.formatOnSaveTimeout` setting. The `editor.formatOnSaveTimeout` setting was removed from vscode v1.42 in 2020. PR #15 removed the timeout dependency from the extension. But this one seemed to have been missed. Removing since it doesn't do anything now. * refactor: improve node errors in fixer. - Refactored the `null` switch-case, (which deals with specific Node errors, eg. `ENOENT`) into a new function: `determineNodeError`. This new function determine's the Node error from it's exit code. It uses Node's `getSystemErrorMap` util function to get a Map of all system error codes and their description's (like `ENOENT`), and we can more precisely generate a better error message. This util function only maps the system errors though, and not the internal Node errors that start with `ERR_`. So we still need to map the `ERR_OPERATION_FAILED` code to it's message. - Added new `cause` property to the `ConsoleError` interface because if an error is caught and then a new error is thrown with a different message/code, we still need to be able to see the originating error. So `cause` will be the originating error. It's probably very unlikely this will happen, but on the off chance it does, then debugging would be easier having access to the original error. - Added new `path` property to the `ConsoleError` interface since Node catches system errors in it's `SystemError` class and can have a `path` property in the error object, which contains the file path that caused the error. This is very helpful for `ENOENT` errors. And we can construct a specific user-friendly `ENOENT` error message with it's path. - Added calls to the new `determineNodeError` function in all switch-cases just to fully ensure they are caught when the exit code is not `null`. * remove: logging of the node error before the switch-case. We no longer need to log node errors to the Output channel before the switch-case since we are now logging all errors just before the function rejects the promise and the error is thrown.
If for some reason `stdin` is null, the process just hangs in the background, without ever closing. This can be seen when the vscode statusbar has the `PHP Sniffer: validating…` text displayed indefinitely. To fix, we will manually kill the process when the `stdin` is `null`, which then gracefully resolves the `done` promise callback for the vscode statusbar text. - Added the `kill` function call, using the `SIGKILL` signal to forcefully terminate the process. - Added an error message in the `done` promise callback to show the user and log to the Output channel if we did manually kill the process. This is outputted/logged just before it resolves the callback allowing the vscode statusbar text to disappear.
Changed:
- Move `determineNodeError` function from fixer into utils.
- Refactor `determineNodeError` util function to enable the messages to be for both sniffer and fixer.
- Added new `toolType` param to determine whether we're using it on `sniffer` or `fixer`.
- Changed all hardcoded `fixer` words to use the new `toolType` param for the error messages.
- Changed the `error` variable to `errorMsg` and separated the internal message, stack trace and cause into a new `extraLoggerMsg` variable. This is because the user dialog doesn't need the stack trace, etc. and it isn't very readable if it does. So we remove it by having a separate variable that will be used only in the logger.
- Changed the return type to object.
- Changed the `determineNodeError` function calls in fixer to use the new function structure. ie. passing the `toolType` param, and also getting the returned object using the destructuring syntax.
- Changed the `nodeError` variable typing in sniffer to be of the `ConsoleError` interface or `null`.
Added:
- Added `determineNodeError` function calls to sniffer in appropriate places.
Removed:
- Removed the now redundant `SNIFFER NODE ERROR` logger function call because the new function will take care of that.
- Remove the unused `getSystemErrorMap` import as it was moved to the utils file. Fixes the prettier error in the GitHub Actions: https://github.com/valeryan/vscode-phpsab/actions/runs/18238209264/job/51935645693
valeryan
reviewed
Oct 6, 2025
valeryan
approved these changes
Oct 6, 2025
- Extracted the `process.platform` tests from all the path resolver util functions into a new dedicated `isWin` util function. This makes the code more readable, and we can use the new util function in other files too where needed.
When spawning a command using CMD, and the command is not found, CMD exits with code 1 instead of erroring as ENOENT and then returns the `is not recognized as an internal or external command` message.
*nix systems handle this error correctly by default. So we need to add back the proper ENOENT error in Windows. The cross-spawn package did solve this, but we removed it's usage due to it being unmaintained with issues in newer node versions.
To fix, we can just use the code from cross-spawn under the MIT license, and with major modifications.
- Added new enoent.ts file with functions:
- `addWindowsEnoentError` function to add the ENOENT error handling. This is the parent function that calls the other functions. This also uses the new `isWin` util function to detect Windows systems.
- `hookIntoEmit` to hook into the `emit` method of the spawn ChildProcess instance to handle the error.
- `verifyEnoentError` to verify if we are dealing with an ENOENT error for both spawn and spawnSync.
- `createEnoentError` to create the ENOENT error and return to the parent function.
- Added `addWindowsEnoentError` function calls to both sniffer and fixer using appropriate params.
- Added a `parsed` object to both sniffer and fixer to pass as argument to `addWindowsEnoentError` function.
This is just to get the ENOENT functions working. The object will eventually come from a separate future function that will escape/quote the command paths/values.
- Fixed the ts error by removing the generic typing. - Added return typing. https://github.com/valeryan/vscode-phpsab/actions/runs/18395023897/job/52412587557?pr=172
- Rename `utils` to `helpers`. - Rename `enoent` to `windows-enoent-error`. - Added new `utils` directory and `error-handling` sub-directory. Moved the `helpers` file into the `utils` directory and the `windows-enoent-error` file into the `error-handling` directory for better organisation. - Updated all imports.
- Removed `getArgs` function from both sniffer and fixer. - Added new unified helper function (in helpers file). The arguments are, for the most part, the identical, so using one function would be better. - Remove the now redundant "Important Note".
Forgot to import vscode's TextDocument interface for the `getArgs`. https://github.com/valeryan/vscode-phpsab/actions/runs/18437461323/job/52532902543?pr=172
- Added new `normalizePath` in path-resolver-utils file to normalize the paths and use the OS-specific path separators.
- Refactored the `resolveCBFExecutablePath` and `resolveCSExecutablePath` functions in settings:
- Use the util function `joinPaths`.
- Add an `else` statement for absolute paths to be able normalize the paths.
- Add code comments for better understanding.
- Refactored `getArgs` helper function to pass the `document.FileName` as the new `filePath` arg instead of passing the whole `TextDocument`, as the `fileName` is the only thing used from the `TextDocument`. - Normalized the standards path in the `createStandardsPathResolver` function using the `normalizePath` util function.
- Removed double quotes from the values of `standard` and `stdin-path` args in `getArgs` function because we will double quote the entire argument in a later function. - Added code comments for better understanding.
- Renamed the temporary `parsed` object (introduced in commit 7b62982) to `originalCommand`, and changed it's content to `commandPath` and the original non-parsed `args`. Also changed all references of the old object and keys to the new ones, in the `windows-enoent-error` file. This is because the ENOENT error handling only ever uses the `original.args`, `original.command`, and `file` keys, with the latter 2 being the same values. So we can simplify the object. Because the error creation uses the original args, we should also use the non-parsed/non-escaped args. - Added a proper `existsSync` Node function to the `verifyEnoentError` function to properly check if the path doesn't exist.
- Added `parseArgs` helper function to parse each argument, and surround them in quotes to allow spaces in paths and help prevent some command injections. Note: This may only prevent some simple injections, but more sophisticated injections will still get through. A solution is in the works.
Collaborator
Author
|
Escaping special characters is all well and good to prevent command injection, but we should also be validating the user-supplied arguments to ensure only PHPCS/CBF arguments can be used, and not random commands. This will help considerably when it comes to preventing command injections such as: "phpsab.snifferArguments": [
"\" - > nul 2>&1 & whoami & echo %USERPROFILE% & echo done;"
],We should also only support a limited subset of the arguments since there's only a few that are relevant to our usages. The arguments (other than the internally used
|
- Added `isStandardValid` function to validate a user-supplied standard to ensure it adheres to PHPCS supported formats, and doesn't contain malicious characters (ie. blocks command injection attacks). This also supports multiple standards separated by commas as is supported by PHPCS. If the standard is not valid for any reason, an error is thrown, and the extension will not continue. Which completely prevents malicious code from ever reaching the spawn process and running. - Added `isStandardName` function to check whether the standard is a single standard name. This is used in the `isStandardValid` function, but also in the main standards resolver because if it's just a standard name, we don't want to continue to try and resolve a path. So it'll just return the standard as is. - Added a try-catch block to both sniffer and fixer to catch the standards resolver invalid error and show the error to the user.
…Arguments` function - Refactored the `additionalArgument` filtering code in both sniffer and fixer into a new `validateAdditionalArguments` helper function. For DRYer code, the 2 filters have been merged into one function. This function will also eventually validate the arguments and values. Also added the function call to the `getArgs` function. - Removed the filtering code from both sniffer and fixer. - Changed the `additionalArguments` param value in the `getArgs` function calls in sniffer and fixer to use the respective `resourceConf.[tool]Arguments` value instead.
Removed:
- Removed the `indexOf` checks for the internally-used arguments in the `validateAdditionalArguments` function.
Added:
- Added new arguments file. This has a collection of helper classes, consts, types, and interfaces to help validate and whitelist the arguments.
- `InternalArguments` class to whitelist all the arguments that are internally-used in the extension.
- `AdditionalArguments` class to whitelist relevant additional arguments the user may want to supply to PHPCS/CBF via the extension.
- `validInternalArguments` and `validAdditionalArguments` are arrays of the internally-used and additional arguments respectively, produced from the property keys of the respective classes. These will help validate the arguments.
- Added new `validateArgument` function to validate the argument against the whitelist.
- Added new `validateKeyValueArgument` function to validate a key-value argument and ensure it's value is also valid. This uses the new `validAdditionalArguments` whitelist array to ensure the argument is valid.
- Added new `validateFilterValue` function to validate the possible values of the `--filter` argument.
- Added new `getArgumentKey` function to get the key of the key-value argument.
Changed:
- Refactored `validateAdditionalArguments` function to use the new `getArgumentKey` and `validateArgument` functions, and also provide error logging to warn the user when invalid arguments have been removed.
Also uses the new `validInternalArguments` whitelist array to filter out the internally-used arguments the user may have supplied.
The `-q` and `-` arg types were previously typed as `boolean` but was soon changed to `unknown` to reflect that we don't actually know their types. But their docblocks were left unchanged.
The auto import added `@phpsab` for some reason instead of the relative path. Which stopped GitHub Actions from working. Fixed by changing the paths. See https://github.com/valeryan/vscode-phpsab/actions/runs/19219598359/job/54934945445?pr=172
As of Node v24, spawn will now emit a warning message, when passing arguments as an array with `shell: true`. The fix is to concatenate the command and argument array into one command string instead. - Added new `constructCommandString` helper function to concatenate the command and arg array. - Changed sniffer and fixer to use the new `constructCommandString` and refactored code to use a new variable that contains the executable path.
Owner
|
Hey @yCodeTech do you need anything from me to merge this or are you just planning to keep working on this for a while? |
Collaborator
Author
|
@valeryan I'm just chipping away at this slowly. I've been ill recently, so that's why progress has been slow. But it's nearly done I think. I should be able to merge it soon. Get tests done, and push out a release by end of the month all being well. |
- Refactored the user messages in `validateAdditionalArguments` helper function to be clearer and refactored the conditional statements to remove duplicate code. Also changed how the messages are created making a distinction between logger messages and user warning messages. - Added new `showChannel` logger function to show the Output channel. - Changed the user message popup to have a secondary button which will show the output channel if clicked (using the new logger `showChannel` function). - Changed some code comments and property docblock in `AdditionalArguments` class.
The initial flag `-watch` was incorrect, it's should be `--watch`. The fact that `-watch` still worked is coincidence, npm properly only saw the shortcut `-w` and the rest of the word was scrapped.
Collaborator
Author
|
Now with the new input validation we don't actually need to escape any characters. Escaping characters is very error prone as it is, especially between all the different systems. Input validation is the safer way to go, plus limiting the arguments to a specific whitelist, and validating the values will go a long way to protect against malformed and malicious commands. The notion of escaping characters is now scrapped. |
- Move the xml and path conditionals to the top of the for...of loop, so that the check for standard name is last. - Fixed the test for xml ruleset file. It now checks to see if the supplied standard is in the allowed ruleset array. The regex has also changed to allow xml files ending in `.dist`. - Added new `allowedRulesets` param to pass the config setting to the function.
Collaborator
Author
|
@valeryan @jonathanbossenger I think this PR is ready for testing on macOS and Linux. Please could you test this when you have the time. I've tested on my Windows 10, and both fixer and sniffer are working fine. Please test these things:
This should be a good list to test and covers the major rework and focus points of this PR. Let me know your findings of the tests as soon as possible. I'm looking to merge this and put out a new release by end of November. |
Collaborator
|
I will do my best to give this a test this week. |
When the `validateAdditionalArguments` was refactored in commit d55b7ec, the logger and show user message was moved to the bottom of the function. But an oversight was that this would also warn users even when there were no invalid args. Fixed by surrounding them in a conditional statement that will only run when there are invalid arguments.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #135, and it's sub-issues #158 and #162.
In order to begin fixing the long-standing issue of spaces in paths and general escaping, we first need to deal with the unmaintained package
cross-spawn. There are multiple open issues and pull requests, some from years ago. Some issues are relating to security and other flaws. So it's best to remove the dependency. Plus only fixer is using cross-spawn anyway (sniffer uses Node'sspawndirectly.Another reason to remove the package is because in Node v24.x, passing the arguments array to
spawn(orexecFile) withshelloptiontrueis deprecated (ref: nodejs/node#57199). There is an open issue in cross-spawn from August (moxystudio/node-cross-spawn#176), but probably won't be fixed. So we will need to do our own fix anyway.For the escaping side of things, we could take some of the code from cross-spawn and implement it directly. UsingNo longer applicable since input validation works better.shell: truenegates cross-spawn's escaping anyway, so by taking some of it's code we can escape the command and arguments ourselves.Also because
shell: truealso doesn't allow Node to throw it'sENOENTerror, and cross-spawn seems to re-add this error back, we can take some of that code and implement it directly too.