process/task/terminal: refactor escaping/quoting

[paul-marechal]

What it does

Add utility functions in @theia/process/lib/common to escape common
shells' arguments. Refactored terminal processes to not handle shell
escaping anymore, it is the caller's responsability to provide the
escaped spawn options.

Escaping is now done for DAP's runInTerminal requests.

Most of the design is taken from VS Code, with minor tweaks.

Tried to test as much as possible since escaping rules can get complicated in corner cases.

Fixes #6684

How to test

1. Build and then run the test suites (@theia/process, @theia/task, and @theia/debug).
2. Test debugging a Python file:
   1. Download ms-python.vsix
   2. Setup debug config to run opened Python files.
   3. Try to debug a Python file with space in its filename, it should work with this patch but not without.
3. Test different shell tasks, using quoted strings (see VS Code Custom Tasks).

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[akosyakov]

I've only looked through the code, will test later. If some tests in the meantime it would help.

[elaihau]

In theia with this change:

in vscode

Task I tested with

      {
        "type": "shell",
        "command": "node",
        "args": [
          "-e",
          "\"setTimeout(()=>console.log(\\\"1 + 2\\\"),1000)\""
        ],
        "label": "test shell 211"
      }

Is the error due to my env settings?

[paul-marechal]

Thanks for the test @elaihau ! I was missing some task related tests, added them back and fixed the issue you reported :)

Although, the specific example you are using is annoying me in some way, because in my opinion it illustrates a behavior that I don't like from VS Code: Essentially, they battle to see if a user already escaped part of an argument, see related code.

From what I understand, when dealing with user defined shell task arguments, I see 2 scenarios:

1. User just writes the fully escaped command line in the command field, this is executed as is.

   This is the simplest case, since we can assume that it is the user's responsibility to properly escape arguments.

2. User write an executable path in the command field, and provides un-escaped arguments.

   Given the tests I have written, our escaping functions should handle about anything. This means that the user should not care about how the arguments will be passed to the shell, since this is now Theia's job to properly escape everything. You can still somewhat control how Theia will escape your argument by using the { value: 'a b', quoting: 'escape' } notation to tell it what strategy to use, making it Theia's responsibility.

   But VS Code seems to assume that people might try to escape their arguments themselves. To me this is more confusing than anything. What's more, they try to detect things such as partially escaped sequences: /path/with/"white space"/meh. This -to me- is an issue when the quotation mechanism is weak, and you lay the responsibility onto the user to do it properly. Then if VS Code detects that quotation is still needed because some white space was missed, it will simply overlay quotes around the whole thing. So /path/with/"white space"/and other white space/meh becomes '/path/with/"white space"/and other white space/meh', so now the inner quotes are useless, since VS Code might quote in a way that will make them break the path.

   Example 1:

   {
       "type": "shell",
       "command": "node",
       "args": [
           "-e",
           "setTimeout(()=>console.log('1 + 2'), 1000)"
       ],
       "label": "test shell 211"
   }

   Breaks, because VS Code sees the white space in front of 1000 and adds strong quotes, without caring about the content of what is being quoted. The single quotes already in place interfere and the command fails. Theia's implementation wouldn't fail.

   Example 2:

   {
       "type": "shell",
       "command": "node",
       "args": [
           "-e",
           "setTimeout(()=>console.log(process.argv), 1000)",
           "/path/with/\"white space\"/and other white space/meh"
       ],
       "label": "test shell 322"
   }

   This one also doesn't pass the right thing to the shell because the single quotes interfere with the quoting added by VS Code. Using double quote wouldn't fix much either since it will embed them into the path, which is not what you want.

   Example 3:

   {
       "type": "shell",
       "command": "node",
       "args": [
           "-e",
           "setTimeout(()=>console.log(1+2),1000)"
       ],
       "label": "test shell 433"
   }

   This one is even better, because even without white space, the command will fail if passed as is to bash. But since VS Code only looks at white space, this is passed un-escaped and fails. The best would be to simply escape everything properly, no questions asked. But in this case I slightly modified VS Code's trigger characters to fix that issue, since I'm already working on the feature.

   Conclusion: It is simplier to assume that arguments are un-escaped and that we should do it ourselves, rather than this mixed mode where it is confusing as to how should we quote things inside a JSON string, which we also must take care of escaping for (escape quotes for JSON, to escape stuff for shell X).

With all of that said, I still tried to mimic what VS Code is doing, without doing it the same way either. The quotation mechanism should be more stable and robust, and user shouldn't care about escaping a thing, unless it is running in an unknown shell for which Theia doesn't know how to escape. But that's kinda asking for trouble, and practically it shouldn't happen.

Now if you have a task command complex enough that you need crazy escaping, that Theia would natively handle but not VS Code, the solution is simply to refactor the command into a script and write a simple task to trigger said script, it will avoid a lot of trouble for everyone ahah.

[paul-marechal]

I would even go as far as adding a property for shell tasks named verbatimArguments: boolean, which would prevent any automatic quoting, so that white space or not, the user takes responsibility in regards to quoting. Even though in this case it might be 10x easier to not use arguments and rather craft a single command line, which will already be passed verbatim.

[akosyakov]

Although, the specific example you are using is annoying me in some way, because in my opinion it illustrates a behavior that I don't like from VS Code: Essentially, they battle to see if a user already escaped part of an argument, see related code.

We can do as we like, but we should support whatever VS Code allows as minimum.

[paul-marechal]

We can do as we like, but we should support whatever VS Code allows as minimum.

Indeed, but to me it seems a bit like bad UX from VS Code, having to think about 3 escaping stages (JSON, VS Code, Shell) instead of just having to care about escaping a string for JSON and leaving the rest to Theia. And if you want to escape yourself, either add the verbatim option, or write a full command line in the command field without args.

[paul-marechal]

Having little issues with Windows, will fix.

[paul-marechal]

Pending: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21515

[vince-fugnitto]

@marechal-p do you mind rebasing the pull-request (resolve the conflicts), I can take another high-level look at the pull-request :)

[paul-marechal]

I will now have to fix the new issues on Windows that appeared after the rebase, sigh.

CQ still pending too, so it's fine I guess.

[akosyakov]

@marechal-p CQ has checkin tag, it means we can merge

[tom-shan]

Looking forword to the landing of this PR which will fix the debug failure when using existing Python Debug Console of vscode python plugin in theia.

[paul-marechal]

@tom-shan I'm having issues with escaping for cmd.exe for tasks, I will take another crack at it Wednesday, once I get to work on a Windows machine. This shell alone makes everything more difficult, but oh well.

[paul-marechal]

I think I managed to tackle most of the issues I was having. Hopefully the CI will be green now.

This PR is finally re-ready for review. Sorry for the delay, it definitely was a struggle.

The code might not be perfect, but I'll leave this up to reviewers.

[akosyakov]

Code wise looks very good. I have not time to test changes yet.

I hope @elaihau @RomanNikitenko can help with testing the task extension.
And @vince-fugnitto @tolusha with testing the debug extension

[elaihau]

Besides common use cases I tested 2 edge cases this time:

      {
        "type": "shell",
        "command": "node",
        "args": [
            "-e",
            "\"console.log(\"1 + 2\")\""
        ],
        "label": "test node command"
      }

and

      {
        "type": "shell",
        "command": "node",
        "args": [
            "-e",
            "\"console.log(\\\"1 + 2\\\")\""
        ],
        "label": "test node command"
      }

This one works too

        {
            "type": "shell",
            "command": "node",
            "args": [
                "-e",
                "let a = 3; a+=\"5\"; console.log(a);"
            ],
            "label": "test node command"
        }

[vince-fugnitto]

I confirmed that debugging still works, and the use case in 'how to test' works successfully.
I am able to debug python files with spaces in their names, something that was otherwise not possible (ex: master).

I'm happy that we included multiple test cases for different edge cases and escaping for different shell types 👍

[kittaakos]

@marechal-p, I have noticed test failures in @theia/task:

• There are test failures on macOS (running them locally, we have verified on multiple OS X),
• There are test failures on Linux too (in Gitpod).

I was looking into the history of task-server.slow-spec.ts, and it seems the last change was with this PR. I have switched back to ce39872 which is the parent commit of your PR. Without your change, the tests are green. Can this PR break tests? Do the tests pass on your side, not on the CI? Thanks!

CC: @akosyakov

[paul-marechal]

@kittaakos

Do the tests pass on your side, not on the CI?

Hmm this PR was merged early April, since then the CI was green on all 3 platforms. @vince-fugnitto tried to run master's task tests on his own OS X and the only failure seemed to be related to nvm behaving weird and killing a terminal (1 failure only).

Is it possible that one of our dependencies updated? (not necessarily npm-related)

Something new in the environments where the failure happens?

note: I just tried on my Windows machine and the tests are passing.

[kittaakos]

note: I just tried on my Windows machine and the tests are passing.

Yes, I know. BTW, some tests are skipped on Windows.

Hmm this PR was merged early April, since then the CI was green on all 3 platforms.

I know. It is strange, I agree.

s it possible that one of our dependencies updated? (not necessarily npm-related)

Something new in the environments where the failure happens?

Well, I do not know, but I can reproduce the test failure with this change and it works without, so I do not think this is a non-npm dependency issue.

[paul-marechal]

Just tried on an Ubuntu 18.04 and no failure either.

I cannot efficiently troubleshoot your issue if I cannot reproduce though... Can you try to find the cause of the failures caused by this patch in your environment?

[kittaakos]

Can you try to find the cause of the failures caused by this patch in your environment?

Where did you get these, @marechal-p? For example, we did not have this code, or similar before, my command is /bin/zsh, it won't match any of the regexpes, is that expected? zsh is the default shell in macOS (10.15.+) This explains why the tests pass on the OS X CI but not locally.

Edit: (copied code because GH linking does not work)

get these,

if (/bash(.exe)?$/.test(command)) {
    quotingFunctions = BashQuotingFunctions;
    execArgs = ['-l', '-c'];

} else if (/wsl(.exe)?$/.test(command)) {
    quotingFunctions = BashQuotingFunctions;
    execArgs = ['-e'];

} else if (/cmd(.exe)?$/.test(command)) {
    quotingFunctions = CmdQuotingFunctions;
    execArgs = ['/S', '/C'];

} else if (/(ps|pwsh|powershell)(.exe)?/.test(command)) {
    quotingFunctions = PowershellQuotingFunctions;
    execArgs = ['-c'];
}

[kittaakos]

OK, I got it. Seems like we are already outdated compared to VS Code:
https://github.com/microsoft/vscode/blob/7e407b547294849e9a22b16e5aea28a56f88524c/src/vs/workbench/contrib/debug/node/terminals.ts#L78-L197

[paul-marechal]

Maybe just a matter of picking better defaults than what we currently have, see VS Code:

	} else if (env.isWindows) {
		shellType = ShellType.cmd; // pick a good default for Windows
	} else {
		shellType = ShellType.bash;	// pick a good default for anything else
	}

Currently there's no default being used.