Add usage of shell when gathering Jest settings

[stephtr]

This change will finally close #274. For simplicity I created a new PR instead of modifying #297.
Independently updating jest-editor-support to v23.0.0 isn't a problem, since there hadn't been any larger commits (and no breaking changes) since the previously used version.
I tested this PR on my Windows machine.

[coveralls]

Pull Request Test Coverage Report for Build 406

• 9 of 9 (100.0%) changed or added relevant lines in 3 files are covered.
• 16 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.1%) to 79.635%

---

Files with Coverage Reduction	New Missed Lines	%
src/helpers.ts	2	89.61%
src/JestExt.ts	14	53.78%

Totals
Change from base Build 404:	-0.1%
Covered Lines:	671
Relevant Lines:	816

---

💛 - Coveralls

[orta]

do we now ship this with jest-editor? if so -cooool

[connectdotz]

yes, this file one is no longer needed and is removed in my upcoming PR

[stephtr]

There was already previously a TypeScript definition within jest-editor-support, but it was out of sync.

[connectdotz]

typescript recommends to keep the type file close to the source package, therefore I think it is best to treat the typings in jest-editor-supports as the official types. IMHO, we should refrain from having a local copy here... in other words, we should add changes to jest-editor-supports instead.

[stephtr]

For this PR I had to update the jest-editor-support dependency, whoose recent versions contain already up-to-date typings, which is why I removed the local copy already with this PR.

[connectdotz]

i see, great, we are doing the same thing then.

Do you think with #332, we still need the shell option for windows?

[stephtr]

#332 and #324 just modify the default values. At the moment one still has to manually add the .cmd extension on Windows systems if one edits pathToJest.
I still prefer spawning the processes in a shell (at least on Windows) instead, such that we and the users don't have to care about the different extensions (and the same settings work across all plattforms).

[orta]

Good to go IMO, I don't mind about a tiny bit of code coverage changing.

[connectdotz]

@orta you are quick today 😉 can we also hold off merging for this PR, it is not clear to me if we still need the shell command after #332 has added logic for windows... and I vaguely remember there is open discussion on using shell commands... in short, I would like to request we discuss this PR a little bit more (espeically with @seanpoulter) before merging...

[stephtr]

It doesn't only affect the default settings but also custom settings. I would dislike having to write pathToJest: "jest.cmd" on Windows systems and pathToJest: "jest" on non-Windows ones, especially for Workspace settings being shared.

[connectdotz]

sorry, just saw your comment.

You meant we use shell just so users do not have to customize with their platform specific command?... giving the shell option comes with some security concern, I would think that we would prefer not to use it if we can... if the reason to use it is just for "convenience", I am not sure it outwieght the security risk we are trading off... If a window user choose to override pathToJest (that is different than the default one we provided with the correct .cmd), it is reasonable to assume they would provide the correct command otherwise the system will not work anyway...

is there any other reason to use the shell option? I also see it in jest Runner...

[stephtr]

In principle adding the .cmd by hand wouldn't be that hard for users (except that it is a breaking change, but on the other hand that feature had been broken already).
However there is an, I think relevant, exception: If one checks in VSCode's workspace settings with a custom pathToJest set to an VCS. That wouldn't work across different OSes, since Windows users need the same string as Linux users, but with the added .cmd.
If we remove the automatically set shell flag, I would therefore pledge for adding a separate pathToJest.windows setting, which would override the pathToJest setting on Windows systems.
Beside that, which security concerns are you taking about?

[connectdotz]

hmmm... you have a point. How about we take the middle ground:

1. add a new jest.windowsShell setting that can take either boolean or string, just like in node child_process.spawn(). So users not only can turn on/off, they can even specify their preferred shell if needed.
2. since our default jest.pathToJest already append the ".cmd" we could default jest.windowsShell to false. Window users who choose to customize their jest.pathToJest can also customize jest.windowsShell if needed.
3. If we worry about the backward compatibility, I guess we could even consider jest.windowsShell default to true. We just need to doucument this risk and let users know they can turn it off if they don't need it.

I guess that is why node has the shell as a separate option instead of hard coding for windows...

---

Beside that, which security concerns are you taking about?

• from node document:

If the shell option is enabled, do not pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution.

• Avoiding Command Injection in Node.js
• it is also less efficient

we might not be able to think of any actual incident right now, but why risk it if we can avoid...

[stephtr]

Before adding a windowsShell option, I would probably prefer a pathToJest.windows setting, since that would eliminate the need of the shell altogether.
What I still didn't understand: What's the problem with enabling shell by default? Node doesn't do it since in some cases (for example when running a web server) a user could inject its own commands into the shell. However in our case nobody else than our user and his opened files would have access to the parameters, but even without shell one could simply use pathToJest for executing a malicious command.

[connectdotz]

• pathToJest.windows without shell would probably break every window user who customized their pathToJest without .cmd. Not looking forward to the backlash.
• since we arelady pass shell option when running jest (in Runner), I figure it will be more compatible and less disrupting to going along the same route.
• allowing user to customize shell, while defaulting to yes, I think will give us the best of both world, that we can enable the windows exeuction seemlessly and consistently (both runner and setting), while allowing users to fully control if and which shell to use.
• security, I agree I can't think of a legitmate use case for our scenario right now. However since we don't control how users come up with their customization, and how shell might be exploit in the future, I didn't feel fully confident to ignore the risk completely either, maybe I am over cautious...?

Just curious, how does windows users get their jest config settings today? Do they all end up using the default? The default won't work for typescript files, how did they work around this problem?

[stephtr]

I don't think that there would be a reasonable amount of users which would even just read about such a shell setting, but I'm also fine with adding it.
I would assume that the behavior of Windows users doesn't differ from Linux ones (except probably the pathToJest setting with its previous bugs). For TypeScript one just has to edit the jest.config.js or use for example create-react-app with react-scripts-ts, which has TypeScript/Jest support already backed in. Both cases are supported by vscode-jest without having to change its settings (at least today, previously by modifying pathToJest).