mfontanini / presenterm

A markdown terminal slideshow tool
https://mfontanini.github.io/presenterm/
BSD 2-Clause "Simplified" License
1.33k stars 32 forks source link

feat: support hidden code lines #254

Closed dmackdev closed 4 months ago

dmackdev commented 5 months ago

This PR adds support for hidden code lines for the languages Rust, Python, Shell and Bash using prefixes at the start of the line, particular to each language. Support for more languages can be added in future.

The hidden code lines will not be visible in snippets in the presentation, but still be evaluated as normal if executed.

See the example presentation for demonstration.

dmackdev commented 5 months ago

From https://github.com/mfontanini/presenterm/issues/242#issuecomment-2133699531:

@dmackdev I think it would be nice if we didn't use /// for every language but this was configurable instead (defaulting to nothing so we don't have to preemptively define it for every language we support highlighting for). /// is valid rust so that would mean I can no longer use doc comments in code snippets, and even worse it would be pretty confusing if you don't know about this, add a /// section and then it doesn't show up in the presentation.

I like the doc tests approach in rust better where # is used to hide lines, see this. Could this be made optional and language-defined so CodeLanguage has some fn hidden_line_prefix(&self) -> Option<&'static str> that can be used to configure this? If you'd like to create the PR and move the conversation there that'd be great.

Totally makes sense @mfontanini - I can make those changes.

So is the thinking that from CodeLanguage::hidden_line_prefix, we return Some("#") for CodeLanguage::Rust, something for CodeLanguage::Bash/Shell, and None for all other languages for now, and we can support for "hidden code lines" for more languages as we go?

dmackdev commented 5 months ago

I'd imagine that not all of the supported languages definitely have some equivalent for this prefix in their own docs' syntax. Would we just decide on some sensible prefix in those cases, should the need arise?

mfontanini commented 5 months ago

Would we just decide on some sensible prefix in those cases, should the need arise?

Yeah I'd fix those as they come. I imagine in most cases, rust being an exception, the hidden line prefix would be an extension to the language's comment syntax. So e.g. for python/shell script it could be ##, for java /// (but I don't know if this is used for something specifically so I'm unsure). But I wouldn't commit to a default for all languages blindly just in case as it may have a meaning in some of them already.

mfontanini commented 5 months ago

I'm starting to have second thoughts about this change, specifically from the security angle. With this change you could run a code snippet in a presentation and it could do something like an rm -rf $HOME or other malicious actions without you knowing ahead of time. I think this could make people not want to share presentations / run someone else's presentations. The same goes for the "local executors to the presentation file" that we mentioned in the other issue. If we let the user define this on a per presentation basis, we'd be giving them arbitrary code execution capabilities on whoever runs their presentations' machines.

Thoughts? Am I too paranoid?

dmackdev commented 5 months ago

A good point. I think the risk of executing malicious code is present regardless of hidden code lines. All it would take is a user to execute code in a presentation without actually inspecting/understanding: the snippet as it is seen in the presentation, the markdown file, or any local executors. Even without hidden code lines and local executors, there has to be some level of trust that a shared/downloaded presentation does not contain malicious code snippets. Of course, hidden code lines/local executor scripts could elevate this risk - if users rely only on viewing the presentation output and do not inspect the sources.

Indeed, other tools like patat disclose a warning about executing arbitrary code from presentations.

But perhaps we could add some sort of safeguard around this for hidden code lines - maybe a CLI flag --enable-hidden-code-lines, so users have to explicitly take some action to actually hide the prefixed code lines in the presentation. Without the flag, the prefixed code lines would appear in the presentation as normal (without the prefix). Not sure if that would be too much overhead for users, but just a thought.

mfontanini commented 5 months ago

there has to be some level of trust that a shared/downloaded presentation does not contain malicious code snippets

Yeah, definitely, but hidden code lines make this a lot more dangerous.

maybe a CLI flag --enable-hidden-code-lines, so users have to explicitly take some action to actually hide the prefixed code lines in the presentation

I agree, if this was implemented we'd need this. However, then it would become annoying for the user that's building a presentation. Now, we could put this as a config option in the config file but this ends up defeating the purpose with foreign presentations. I have mixed feelings about this :/

dmackdev commented 5 months ago

Fair enough @mfontanini. In the absence of any other ideas or a way forward to address the security concerns, feel free to close this PR 🙂

mfontanini commented 4 months ago

Thanks, this is great!