feat(plugin): add exec_cmd helper for executing command in host #666

tw4452852 · 2021-08-27T09:07:52Z

imsnif · 2021-08-27T09:11:03Z

Hey, just to chime in here. While I think this is a really cool feature to have (I think it can also be pretty great to allow plugins to spawn new panes with arbitrary commands or terminals in them), I'm a little worried about security concerns.

We discussed things such as this in the past and agreed that before implementing this we need some sort of permission system. In the least something that when you load the plugin alerts you that this plugin has permissions to run arbitrary commands on your machine, giving you the chance to back down.

tw4452852 · 2021-08-31T05:48:22Z

Hi @imsnif ,

I've added a flag to allow the plugin to run host command (forbidden by default). Can you give it another look?

zellij-server/src/wasm_vm.rs

imsnif · 2021-09-01T14:14:04Z

Hey @tw4452852 - sorry for the misunderstanding. I didn't mean to add a flag that would allow all plugins to execute commands. That might be quite dangerous I find. I was thinking right now to do something like add this to the layout where we load the plugin. Maybe under "run"?

tw4452852 · 2021-09-02T08:35:49Z

Hi @imsnif ,

Sorry, I misunderstood, thanks for your advice anyway. How about adding the flag under "plugin"? Because it only applies to plugins I think. Like this:

run:
  plugin:
    path: path_or_name
    allow_exec_host_cmd: true/false

imsnif · 2021-09-02T08:51:58Z

@tw4452852 - sounds great!

tw4452852 · 2021-09-04T07:59:51Z

@imsnif
Already done.

TheLostLambda

Hey @tw4452852 ! Thank you for all of your excellent work here! I left a couple of small comments, but there is really just one thing we need to address before finally merging this!

Right now there is actually a pretty major security hole: everything in shim.rs is just convenience code for the plugins and isn't actually needed to write a plugin (it wouldn't be used at all in non-Rust plugins, for example). That means that any plugin can just directly call the host_exec_cmd without checking permissions.

I'll leave some comments with more details, but we essentially need to move the permissions check to the host side of things.

zellij-server/src/wasm_vm.rs

zellij-utils/src/input/unit/layout_test.rs

zellij-server/src/wasm_vm.rs

TheLostLambda · 2021-09-06T13:49:16Z

zellij-tile/src/shim.rs

@@ -37,6 +45,14 @@ pub fn open_file(path: &Path) {
 pub fn set_timeout(secs: f64) {
    unsafe { host_set_timeout(secs) };
 }
+pub fn exec_cmd(cmd: &[String]) -> Result<(), PermissionDenied> {


I don't think we need the Result here and I'd be fine with things failing silently (we don't currently have a way to see if the command was run successfully, even if we did have permissions).

I'd personally just return nothing here, but if you disagree, just let me know!

I'd personally prefer to return something if we wait for the command exit, users may know if it's successful or not.
But right now, we don't wait for the command to finish, so I agree with your opinion for simplicity.

@tw4452852 I totally agree :) I'd like to monitor the running of the command eventually, but I'm not sure how we'll do that just yet, so maybe it's best for a second PR :)

I originally wanted to return the exit code which indicates the success or not. But I'm not sure if this is enough or general. Let's hold on to this for this moment now.

zellij-tile/src/shim.rs

zellij-utils/src/input/layout.rs

tw4452852 · 2021-09-07T02:03:38Z

Hi @TheLostLambda , Thanks a lot for your review, they're really good advice, I adopt them all.

zellij-server/src/wasm_vm.rs

zellij-tile/src/shim.rs

tw4452852 · 2021-09-08T02:10:47Z

Hi @TheLostLambda

Thanks for your review comments. In the latest version, I also add more context to the log (the plugin's name and the command to run) for better debugging.

Signed-off-by: Tw <wei.tan@intel.com> Signed-off-by: Tw <tw19881113@gmail.com>

TheLostLambda

This looks great! Thank you for all of your hard work on this and feel free to merge things whenever you're ready!

TheLostLambda · 2021-09-08T16:44:56Z

Additionally, @tw4452852 if you can't actually press the merge button, even after approval, just let me know and I'll merge for you!

tw4452852 · 2021-09-09T00:37:05Z

Hi @TheLostLambda
It seems I don't have the permission, feel free to merge, I'm done.

TheLostLambda · 2021-09-09T10:45:16Z

No worries, all merged!

tw4452852 force-pushed the exec_cmd branch 2 times, most recently from c713bbc to b7b52ed Compare August 31, 2021 05:45

tw4452852 force-pushed the exec_cmd branch 2 times, most recently from 5afa69f to 0152498 Compare August 31, 2021 06:03

tw4452852 commented Aug 31, 2021

View reviewed changes

zellij-server/src/wasm_vm.rs Show resolved Hide resolved

tw4452852 force-pushed the exec_cmd branch from 0152498 to 3c38401 Compare September 4, 2021 07:56

imsnif requested a review from TheLostLambda September 6, 2021 13:29

TheLostLambda suggested changes Sep 6, 2021

View reviewed changes

tw4452852 force-pushed the exec_cmd branch from 3c38401 to 34778b6 Compare September 7, 2021 02:01

TheLostLambda suggested changes Sep 7, 2021

View reviewed changes

zellij-server/src/wasm_vm.rs Outdated Show resolved Hide resolved

TheLostLambda reviewed Sep 7, 2021

View reviewed changes

zellij-tile/src/shim.rs Outdated Show resolved Hide resolved

tw4452852 force-pushed the exec_cmd branch from 34778b6 to c19a340 Compare September 8, 2021 02:05

feat(plugin): add exec_cmd helper for executing command in host

0235666

Signed-off-by: Tw <wei.tan@intel.com> Signed-off-by: Tw <tw19881113@gmail.com>

tw4452852 force-pushed the exec_cmd branch from c19a340 to 0235666 Compare September 8, 2021 05:45

TheLostLambda approved these changes Sep 8, 2021

View reviewed changes

TheLostLambda merged commit 19b3f83 into zellij-org:main Sep 9, 2021

tw4452852 deleted the exec_cmd branch September 9, 2021 23:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin): add exec_cmd helper for executing command in host #666

feat(plugin): add exec_cmd helper for executing command in host #666

tw4452852 commented Aug 27, 2021

imsnif commented Aug 27, 2021

tw4452852 commented Aug 31, 2021 •

edited

Loading

imsnif commented Sep 1, 2021

tw4452852 commented Sep 2, 2021 •

edited

Loading

imsnif commented Sep 2, 2021

tw4452852 commented Sep 4, 2021

TheLostLambda left a comment

TheLostLambda Sep 6, 2021

tw4452852 Sep 7, 2021

TheLostLambda Sep 7, 2021

tw4452852 Sep 8, 2021

tw4452852 commented Sep 7, 2021

tw4452852 commented Sep 8, 2021 •

edited

Loading

TheLostLambda left a comment

TheLostLambda commented Sep 8, 2021

tw4452852 commented Sep 9, 2021

TheLostLambda commented Sep 9, 2021

feat(plugin): add exec_cmd helper for executing command in host #666

feat(plugin): add exec_cmd helper for executing command in host #666

Conversation

tw4452852 commented Aug 27, 2021

imsnif commented Aug 27, 2021

tw4452852 commented Aug 31, 2021 • edited Loading

imsnif commented Sep 1, 2021

tw4452852 commented Sep 2, 2021 • edited Loading

imsnif commented Sep 2, 2021

tw4452852 commented Sep 4, 2021

TheLostLambda left a comment

Choose a reason for hiding this comment

TheLostLambda Sep 6, 2021

Choose a reason for hiding this comment

tw4452852 Sep 7, 2021

Choose a reason for hiding this comment

TheLostLambda Sep 7, 2021

Choose a reason for hiding this comment

tw4452852 Sep 8, 2021

Choose a reason for hiding this comment

tw4452852 commented Sep 7, 2021

tw4452852 commented Sep 8, 2021 • edited Loading

TheLostLambda left a comment

Choose a reason for hiding this comment

TheLostLambda commented Sep 8, 2021

tw4452852 commented Sep 9, 2021

TheLostLambda commented Sep 9, 2021

tw4452852 commented Aug 31, 2021 •

edited

Loading

tw4452852 commented Sep 2, 2021 •

edited

Loading

tw4452852 commented Sep 8, 2021 •

edited

Loading