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

*.trycmd files and sandboxing #379

Open
steveklabnik opened this issue Feb 7, 2025 · 2 comments
Open

*.trycmd files and sandboxing #379

steveklabnik opened this issue Feb 7, 2025 · 2 comments
Labels
A-trycmd Area: trycmd package enhancement Improve the expected

Comments

@steveklabnik
Copy link

In short, right now, it's not possible to use a tempdir to sandbox with *.trycmd files. I am in a specific situation with a CLI where I need the sandbox to be out of tree for things to work. In my case, I can't use .toml files, because the whole point is that I'm using the markdown with other tools.

I have things working by depending on my fork, but obviously this isn't useful to be pulled in upstream: steveklabnik@abf602a

While this means I'm good, I don't like depending on forks, and so want to talk about maybe figuring out a way to upstream a feature here.

I spoke with @epage about this, and he suggested some possibilities:

A potential approach is to provide a default configuration to the builder that .toml and .trycmd/.md files layer on top of. This could also help in reducing boilerplate.

Another is for this to be controlled in a code fence attribute.

I think both of these ideas are reasonable. The advantage of the first is that you only need to declare it once, the advantage of the latter is that you could control on a test-by-test basis to sandbox or not.

Is there a reason why sandboxing isn't the default? That is, in what circumstances would you want to opt out of sandboxing? Such a change would be a major revision bump, so I'm not suggesting this behavior be swapped, but it's what I personally would have assumed was the default. Regardless, it's a useful lens to think about which of the above makes sense, that is, if someone who wants sandboxing is likely to sandbox all of their tests, then the builder makes sense. But if there's situations in which you may want mixed tests, then the code fence makes more sense.

@epage epage added enhancement Improve the expected A-trycmd Area: trycmd package labels Feb 7, 2025
@epage
Copy link
Contributor

epage commented Feb 7, 2025

Is there a reason why sandboxing isn't the default? That is, in what circumstances would you want to opt out of sandboxing?

Keep in mind, I initially wrote all of this for clap where there are no file operations at all.

All of my other development has focused towards the .toml file inputs as they give more control (and auto-detection)

  • typos mostly reads files from a template directory
  • cargo-edit (before switching away) provides *.out directories which implicitly turn on sandboxing

With feature auto-detecion working in most of those cases, a "pay for what you use" approach isn't too bad.

@epage
Copy link
Contributor

epage commented Feb 7, 2025

I lean towards starting with layering and then adding code fences when people find a need for them.

Looking at our schema, I am leaning towards

  • Adding a top-level timeout that gets layered into each Step
  • Adding a top-level env that gets layered into each Step
  • Adding a new struct that has timeout, env, and fs that gets layered with the above and that the caller can set in the API

In doing this, we'd be sandboxing per .trycmd file. I'm assuming that would be sufficient but want to double check. Code fence attributes could offer more flexibility on this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trycmd Area: trycmd package enhancement Improve the expected
Projects
None yet
Development

No branches or pull requests

2 participants