Skip to content
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

Add args to 'terminal:create-new' to change current directory and execute a command #7604

Closed
fcollonval opened this issue Dec 7, 2019 · 8 comments

Comments

@fcollonval
Copy link
Member

In @jupyterlab/git, a new command was created to open a terminal in a given directory and execute a command in it.

https://github.com/jupyterlab/jupyterlab-git/blob/master/src/gitMenuCommands.ts#L35-L65

This is a feature commonly used for example in VS Code by extensions. It would be nice to have it directly within the standard terminal:create-new command.

Xref: Related issue #6130

@fcollonval
Copy link
Member Author

This is not a good idea: see #7605 (comment)

@fcollonval
Copy link
Member Author

@ian-r-rose I may have close this a bit too early. I agree on the security problem regarding arbitrary command execution. But would it be acceptable to keep the first half of the proposal ?

open a terminal in a given directory

The terminal would execute 'cd ${args['path']}'

@fcollonval fcollonval reopened this Dec 9, 2019
@telamonian
Copy link
Member

But would it be acceptable to keep the first half of the proposal ?

open a terminal in a given directory

That does sound reasonable. The question is, does this more limited command still raise the same security issues as brought up in #5916 (comment)? My sense is no, but I would like at least a couple of other people with more experience to think it through.

If we do add the command, it would also be nice to have a "Open New Terminal in Current Directory" file menu item. If we decide the command is too much, we can just do the menu item instead

@jasongrout
Copy link
Contributor

I think we'd want to at least parse and escape the text we are slotting into a command line (like the path) - otherwise we end up with trivial string injection attacks.

@jasongrout
Copy link
Contributor

Perhaps the directory could just be transmitted to the server as a separate item, and the server could do the parsing and sanitization, check if the directory exists, etc., and open to that directory, rather than executing an arbitrary string from the frontend? That would also give the server a chance to switch directory separators if needed, etc.

@telamonian
Copy link
Member

otherwise we end up with trivial string injection attacks.

@jasongrout Lemme make sure I'm following correctly: the danger is that someone sets the directory arg to, say:

$(rm -rf /)

or something, right?

Offloading some or all of the change dir logic to the server makes sense. Currently, it looks like all new terminals started by a particular instance of jlab will start in that instance's root dir. Presumably we can just build on top of whatever mechanism is already in place that ensures that

@jasongrout
Copy link
Contributor

Or even more trivially, do something like an argument of /; rm -rf . or / && rm -rf .

@krassowski
Copy link
Member

Closing since the discussion narrowed down to changing cwd which was implemented in #12250 which was included in JupyterLab 3.4. Please reopen if I misunderstood.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants