Pre-process any commands

[maaslalani]

Fixes #51

Changes Introduced

• Allow arbitrary commands to be executed for pre-processing slides

• This allows for embedding graphs, diagrams, any output of any command, really

• This is a bit different than code execution because it entirely replaces the block of code with the output of the command with the input in the block.

[maaslalani]

@sibprogrammer @brittonhayes What do you think of this feature? Any input? Kind of concerned about security so I either want to make this opt-in or completely isolated from the file system / completely sandboxed.

But I'm leaning towards opt-in because accessing the file system is probably super useful to load images and convert them to ascii or xargs cat to add snippets.

Screen.Recording.2021-06-25.at.10.59.26.PM.mov

[brittonhayes]

I'd definitely lean towards having this be an opt-in feature. The primary risk for slides still being abuse between unexpected code execution when someone sends a slideshow markdown file to another user.

The most concerning thing to me is if slides is used by curling a remote slide.md file and executing the slideshow from stdin without checking what will execute before hand.

So the more you can do to set safe defaults, I think the better you'll feel here.

TLDR: I think you're on the right track with a manual opt-in that explains the risk to the user.

[maaslalani]

What about if we only pre-process files that are on disk (not from stdin) and they must be executable.

Which means to pre-process slides you need to run chmod +x file.md and have the file locally. This should (hopefully) ensure people have analyzed the contents.

Maybe we should also have a "block" list of commands that can't be executed on pre-process, like rm.

[brittonhayes]

That feels like a sane preventative measure blocking stdin from using the code execution feature. That'd definitely prevent most people from accidentally executing untrusted code without knowing what they're about to run.

So I think double up with the manual opt-in plus blocking code exec from stdin or just stick with the manual opt in.

The former being the security focused choice, the latter being the user convenience focused choice. Just depends where on the scale feels right for how you see users leveraging slides and where you're comfortable.

Both are solid choices 😄

[sibprogrammer]

The trick with chmod +x file.md looks interesting and probably it's enough.
The more straightforward way for me is to have something like this: slides --exec file.md (to provide an explicit option). But as far as I can see you're trying to avoid additional CLI options 🙂

Maybe we should also have a "block" list of commands that can't be executed on pre-process, like rm.

It doesn't improve the security because can be easily workarounded. Moreover the hacker can "wget" the trojan or upload sensitive user data somewhere. So it seems like there is no reason to invest in the "block" list.

[maaslalani]

This is pretty neat! We can import content from other files now

[maaslalani]

I changed the pre-processing to work only if a file is being read (not stdin, to avoid curling) and made it so that the file needs execution permissions (chmod +x file.md) to be pre-processed.

Live reloading "just works" because of this 🙂 but I will probably need to look at optimizing the pre-processing with multiple go-routines since it can probably take a bit of time to pre-process a bunch of commands.

[maaslalani]

But as far as I can see you're trying to avoid additional CLI options

Haha yea I'm trying to see how long I can go by not having any CLI flags 😄 I also think this would be a good candidate for a CLI flag so in the future I will probably make this a flag. But for now, I want to see how minimal I can keep the interface 🙂

Moreover the hacker can "wget" the trojan or upload sensitive user data somewhere

We would also probably add wget and curl to the block list, or maybe restrict network access during the execution.

[brittonhayes]

I think @sibprogrammer has the right point here on avoiding a block list. That has historically been a game of whack-a-mole that doesn't always yield the most effective results and could likely lead to user confusion when certain commands don't run.

There will pretty much always be a command that ya don't think of that could be used maliciously so it's a lot of effort for not a ton of pay off.

The chmod +x requirement and preventing stdin for pre-process exec will likely do the bulk of the heavy lifting here 😄

[maaslalani]

Sounds good on the block list, going to merge this and make a pre-release. Pretty happy with how much stuff this unlocks (especially with the live reload!)

[maaslalani]

If you get a chance to test this out (and find bugs! / useful use cases), otherwise I will release tomorrow 😄: https://github.com/maaslalani/slides/releases/tag/v0.4.0

Still pretty rough, need to make error handling much better and display errors to the users more nicely.

[brittonhayes]

I'll be able to poke around with the new release once I get done moving 😄 Stoked about this feature!

Appreciate you reaching out for a double check too. Feel free to ping whenever ya want another pair of eyes. Especially with security concerns.