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

fix: cargo wasi test preopen cwd #129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ricochet
Copy link

This PR defaults cargo wasi test to always be run
with the current working directory preopened.
The goal is to allow existing tests to run without modification.

Fixes #128

This PR defaults `cargo wasi test` to always be run
with the current working directory preopened.
The goal is to allow existing tests to run without modification.

Fixes bytecodealliance#128
@ricochet
Copy link
Author

@alexcrichton and @peterhuene

@alexcrichton
Copy link
Member

Personally I'm not confident enough that this is the best way to approach this to merge. Part of WASI/wasm is the whole capability-based-security idea and implicitly granting access to things feels, at least to me, like it's going against that. I sympathize with the desire to run things out-of-the-box but that clashes with the capability goals of WASI where in theory the directories all need to be explicitly granted at some point.

I don't know personally how best to balance these concerns. Pushing entirely on "always say what you get" is not going to be fine but additionally implicitly granting access seems like it would inevitably cause issues as well.

@ricochet
Copy link
Author

ricochet commented Jan 3, 2023

I see cargo wasi test as a specific case, but I'm game to explore other options. I want to avoid always requiring command-line args for ergonomics.

A different approach could be to build out a custom section in Cargo.toml. If there is a section for WASI and various run modes like test, then cargo wasi could suggest defaults to add to the toml. When running cargo wasi test it would inspect global wasi configurations and run mode specific configurations. cargo component has its own component section, but at this time, WASI is a global context and perhaps should have its own section.

@autodidaddict
Copy link

I agree that implicitly adding preopens is potentially hazardous, but if I'm reading the PR right, this only happens in test mode, in which case conveniences are always welcome in the pursuit of ergonomics.

@FrankReh
Copy link

FrankReh commented Jan 6, 2023

Coming from looking at Deno recently, I see a lot of interest for it because of the security it gives users and companies that Deno will only give access to capabilities made explicit on the command line. And I worked on wasm with Go before WASI was a thing so I missed this boat and am trying to catch up a little now. I can say if I were to see the stack trace output from a simple cargo wasi test, 28 deep, with not piece of guidance in sight that directory access was needed, it would be daunting.

There's a tutorial for wasmtime that says an error about capabilities would be printed, error opening input test.txt: Capabilities insufficient but that doesn't seem to be the case when run from cargo wasi test. I'll spare you the stack trace.

Lucky for me, the issue referenced above showed how to make it work with a proper command line:

CARGO_TARGET_WASM32_WASI_RUNNER="wasmtime --dir=." cargo wasi test

Not to hijack this, but still looking for how to allow the capability for read-only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo wasi test should preopen the current dir by default
4 participants