fix: use shell when running child_process.spawn #1804
Conversation
🦋 Changeset detectedLatest commit: 2381283 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Coverage report
Show new covered files 🐣
Test suite run success832 tests passing in 121 suites. Report generated by 🧪jest coverage report action from 2381283 |
|
Converting back to draft as I'd like to add some input sanitization |
tatomyr
left a comment
tatomyr
left a comment
There was a problem hiding this comment.
Left one comment.
Regarding the test failure -- the coverage just doesn't meet the threshold. Either add more tests or lower the threshold.
packages/cli/src/utils/platform.ts
Outdated
| return input.replace(/[^a-zA-Z0-9@._-]/g, ''); | ||
| }; | ||
|
|
||
| export function getPlatformArgs(argv: { path?: string; locale?: string; 'project-dir'?: string }) { |
There was a problem hiding this comment.
These properties are specific to individual commands, whilst utils are meant to be context-agnostic. Could you move them out to make the getter more general? I’d recommend returning sanitizers instead, so they can be used directly in the command handlers.
There was a problem hiding this comment.
I made it more agnostic by returning generic sanitize function.
| npxExecutableName, | ||
| ['-y', '@redocly/realm', 'translate', argv.locale, `-d=${argv['project-dir']}`], | ||
| { stdio: 'inherit' } | ||
| ['-y', '@redocly/realm', 'translate', locale ?? '', `-d=${projectDir}`], |
There was a problem hiding this comment.
Could you move the ?? '' into the sanitizer itself? For me it looks like it makes sense to transform all empty arguments into empty spaces.
| export const sanitizePath = (input: string): string => { | ||
| return input.replace(/[^a-zA-Z0-9 ._\-:\\/@]/g, ''); | ||
| }; | ||
|
|
||
| export const sanitizeLocale = (input: string): string => { |
There was a problem hiding this comment.
Could you leave a comment with an explanation regarding what is being sanitized? Later, it could be hard to figure out the difference between the sanitizers 😄
Or perhaps you could come up with more descriptive names?
tatomyr
left a comment
tatomyr
left a comment
There was a problem hiding this comment.
LGTM, thanks for your input!
What/Why/How?
CLI preview command was failing on Windows with specific Node versions.
Turns out there is a breaking change in Node API that prevents passing
.cmdfiles tospawnfunction without usingshell: trueoption. Since we're usingnpx.cmdshelloption is now necessary.Using
shell: truebrings an actual shell which introduces a risk of shell injection. This means that the arguments should be sanitized before passing. In our case those that require to be sanitized are strings not validated byyargsitself: paths and locale. For these values there are sanitize functions restricting input to the set of allowed characters, which significantly limits the possible risk (but also reduces characters that can be used in regular paths).To limit the risk exposure
shell: trueis used only on windows platform.Other options considered:
execFileinstead of spawn -execFiledoesn't spawn a shell which makes it safer, however it behaves differently and doesn't handle long running processes and user interaction likespawndoes.ejectuse case is fine withexecFile, butpreviewandtranslatewon't work in this casefs.existsSyncwouldn't prevent injections since a malformed path might be created (there are no limitations in characters)Reference
https://github.com/Redocly/redocly/issues/11284
Testing
Tested on Windows and MacOS with Node versions from before and after the change.
Screenshots (optional)
Check yourself
Security