Skip to content

Script arguments to debug adapter should be wrapped in quotes if they contain spaces #190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
daviwil opened this issue Mar 15, 2016 · 5 comments
Labels
Issue-Bug A bug to squash.

Comments

@daviwil
Copy link
Contributor

daviwil commented Mar 15, 2016

See the following two issues for more information.

microsoft/vscode#4057
PowerShell/vscode-powershell#117

@rkeithhill
Copy link
Contributor

We have to be careful/smart about this. If you wrap "-Index", "1, 2, 3" with quotes you get -Index "1, 2, 3" and now we've changed the type of the argument from [int[]] to [string]. You could say that folks should do this instead "-Index", "1", "2", "3" or "-Index 1, 2, 3". The first will work but the second will totally break because SomeCommand "-Index 1, 2, 3" is a lot different than SomeCommand -Index 1, 2, 3.

I dunno on this one. Seems like it would be a bit dangerous to do this.

Another solution would be to only support a single string and not an array of strings for args. Then it should make it more obvious that individual arguments with spaces need internal quoting e.g. "-Index 1, 2, 3 -Path 'C:\program files'". The downside is that this would be a breaking change.

@daviwil
Copy link
Contributor Author

daviwil commented Mar 25, 2016

Yeah, you're right, maybe we should just be accepting a single string. There's not really a good reason to accept an array of strings since it complicates everything.

Maybe we can just change our schema in VS Code for now to specify string for the args field so that the user gets a warning if they're using an array. In our LaunchRequest we can change the Args property to be JToken so that either a string or an array can be passed. We'd have to do an extra step to determine if Args contains a JValue or a JArray and then act accordingly.

@daviwil
Copy link
Contributor Author

daviwil commented Mar 25, 2016

Forgot to should add, we'd deprecate the array support after a version or two once we have appropriate documentation showing how to do args the right way. At the time of deprecation we could even give a specific error message from the debug adapter warning the user of this.

@rkeithhill
Copy link
Contributor

Another somewhat lame option if we were to keep array of strings is to add more help in the scheme e.g.:

vscodeargswithspacetip

The problem with this is that it isn't very likely users would hover over the args field to get this tip. Meh, probably better to change to using just a single string.

My original thinking with array of args was that we would eventually crack the nut on AddCommand instead of AddScript (to avoid the script injection issue Lee talked about) then we would need the array of args. I'm not sure that will ever happen at this point.

@daviwil
Copy link
Contributor Author

daviwil commented Mar 25, 2016

Yeah... I thought about that too. Though I think it's considered "more secure" to use AddCommand so I'd like to do it at some point. We may still be able to accomplish this with a single string by using PowerShell's parser to get an AST for the string to use for passing to AddParameter/AddArgument.

@daviwil daviwil modified the milestones: 0.7.0, 0.6.0 May 11, 2016
@daviwil daviwil modified the milestones: 0.7.0, Backlog Sep 2, 2016
@andyleejordan andyleejordan removed this from the Backlog milestone Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug A bug to squash.
Projects
None yet
Development

No branches or pull requests

3 participants